Attention is currently required from: Subrata Banik, Wonkyu Kim.
Jérémy Compostella has posted comments on this change by Jérémy Compostella. ( https://review.coreboot.org/c/coreboot/+/83948?usp=email )
Change subject: soc/intel/common/block/cpu: Fix number of way computation regression
......................................................................
Patch Set 8:
(1 comment)
File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/83948/comment/a909b1b5_0e0560c9?us… :
PS5, Line 526: xor %edx, %edx
> I also checked other reference and it looks Jeremey's change is correct. […]
I added a comment.
--
To view, visit https://review.coreboot.org/c/coreboot/+/83948?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I5cb66da0aa977eecb64a0021268a6827747c521d
Gerrit-Change-Number: 83948
Gerrit-PatchSet: 8
Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Wonkyu Kim <wonkyu.kim(a)intel.com>
Gerrit-Attention: Wonkyu Kim <wonkyu.kim(a)intel.com>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Comment-Date: Mon, 19 Aug 2024 22:33:58 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Wonkyu Kim <wonkyu.kim(a)intel.com>
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Comment-In-Reply-To: Jérémy Compostella <jeremy.compostella(a)intel.com>
Attention is currently required from: Jérémy Compostella, Subrata Banik.
Hello Subrata Banik, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/83948?usp=email
to look at the new patch set (#8).
The following approvals got outdated and were removed:
Verified+1 by build bot (Jenkins)
Change subject: soc/intel/common/block/cpu: Fix number of way computation regression
......................................................................
soc/intel/common/block/cpu: Fix number of way computation regression
Commit 16ab9bdcd578612bb3822373547f939eb90afd82 ("soc/intel/common:
Calculate and configure SF Mask 2") breaks the computation of the
number of way and as result, all the derived masks. It results in MSR
such as `IA32_L3_MASK_1' to be improperly programmed yielding
unpredictable NEM issues such as hangs.
Indeed, this commit has introduced a backup of 0x1 into %edx before
comparing the requested cache-as-RAM size against the way size. When
the requested cache-as-RAM is larger, it reaches the second part of
the algorithm which computes the necessary number of way to fit the
requested cache-as-RAM.
This algorithm uses the `div' instruction. Per specification, the div
instruction divides the 64 bits combination of %edx and %eax register.
Since 0x1 got backed up in %edx and assuming a
`CONFIG_DCACHE_RAM_SIZE' of 0x200000, we end up dividing 0x100200000
by the way size instead of 0x200000 which result in a necessary number
of ways of 4098 for a way size of 0x100000.
This commit clears the %edx register before calling the `div'
instruction.
BUG=b:360332771
TEST=Verified on PTL Intel reference platform
Change-Id: I5cb66da0aa977eecb64a0021268a6827747c521d
Signed-off-by: Jeremy Compostella <jeremy.compostella(a)intel.com>
---
M src/soc/intel/common/block/cpu/car/cache_as_ram.S
1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/83948/8
--
To view, visit https://review.coreboot.org/c/coreboot/+/83948?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I5cb66da0aa977eecb64a0021268a6827747c521d
Gerrit-Change-Number: 83948
Gerrit-PatchSet: 8
Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Wonkyu Kim <wonkyu.kim(a)intel.com>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com>
Attention is currently required from: Angel Pons, Felix Singer, Paul Menzel.
Ingo Reitz has posted comments on this change by Ingo Reitz. ( https://review.coreboot.org/c/coreboot/+/82943?usp=email )
Change subject: util/nixshell: add dependencies to nix devshell reqired by some payloads
......................................................................
Patch Set 2:
(3 comments)
Patchset:
PS1:
> > That shouldn't be needed since the toolchain package includes GNAT. (...) […]
Acknowledged
Commit Message:
https://review.coreboot.org/c/coreboot/+/82943/comment/b5edc09f_42e00864?us… :
PS1, Line 2: ninelore
> Please use “Ingo Reitz”. […]
Done
https://review.coreboot.org/c/coreboot/+/82943/comment/0140cafa_d095873e?us… :
PS1, Line 8:
> It’d be great if you elaborated.
Done
--
To view, visit https://review.coreboot.org/c/coreboot/+/82943?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I880741567755712f4ab2c169a589d17bac491186
Gerrit-Change-Number: 82943
Gerrit-PatchSet: 2
Gerrit-Owner: Ingo Reitz <9l(a)9lo.re>
Gerrit-Reviewer: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Mon, 19 Aug 2024 22:29:03 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org>
Comment-In-Reply-To: Ingo Reitz <9l(a)9lo.re>
Attention is currently required from: Angel Pons, Felix Singer, Ingo Reitz.
Hello Felix Singer, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/82943?usp=email
to look at the new patch set (#2).
The following approvals got outdated and were removed:
Verified+1 by build bot (Jenkins)
Change subject: util/nixshell: add dependencies to nix devshell reqired by some payloads
......................................................................
util/nixshell: add dependencies to nix devshell reqired by some payloads
Currently this nix file only has dependencies to build coreboot itself,
but common payloads like EDK2 or Das U-Boot require additional
dependencies, which are added in this patch
Change-Id: I880741567755712f4ab2c169a589d17bac491186
Signed-off-by: Ingo Reitz <9l(a)9lo.re>
---
M util/nixshell/devshell-i386.nix
1 file changed, 3 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/82943/2
--
To view, visit https://review.coreboot.org/c/coreboot/+/82943?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I880741567755712f4ab2c169a589d17bac491186
Gerrit-Change-Number: 82943
Gerrit-PatchSet: 2
Gerrit-Owner: Ingo Reitz <9l(a)9lo.re>
Gerrit-Reviewer: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Ingo Reitz <9l(a)9lo.re>
Attention is currently required from: Jérémy Compostella, Subrata Banik.
Wonkyu Kim has posted comments on this change by Jérémy Compostella. ( https://review.coreboot.org/c/coreboot/+/83948?usp=email )
Change subject: soc/intel/common/block/cpu: Fix number of way computation regression
......................................................................
Patch Set 6:
(1 comment)
File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/83948/comment/89726c04_a23c5e49?us… :
PS5, Line 526: xor %edx, %edx
> > How does that matter, since line #528 will copy EAX into EDX anyway? […]
I also checked other reference and it looks Jeremey's change is correct.
Refer below part in https://www.aldeid.com/wiki/X86-assembly/Instructions/div
Always divides the 64 bits value across EDX:EAX by a value.
The operation 0x8003 / 0x100 can be written as follows:
mov edx, 0 ; clear dividend
mov eax, 0x8003 ; dividend
mov ecx, 0x100 ; divisor
div ecx ; EAX = 0x80, EDX = 0x
But can we add comments for reason to clear for readability to avoid confusion?
--
To view, visit https://review.coreboot.org/c/coreboot/+/83948?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I5cb66da0aa977eecb64a0021268a6827747c521d
Gerrit-Change-Number: 83948
Gerrit-PatchSet: 6
Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Wonkyu Kim <wonkyu.kim(a)intel.com>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Comment-Date: Mon, 19 Aug 2024 21:52:00 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Comment-In-Reply-To: Jérémy Compostella <jeremy.compostella(a)intel.com>
Attention is currently required from: Dinesh Gehlot, Eran Mitrani, Jakub Czapiga, Jayvik Desai, Kapil Porwal, Nick Vaccaro, Pranava Y N, Rishika Raj, Subrata Banik, Tarun.
Hello Dinesh Gehlot, Eran Mitrani, Jakub Czapiga, Jayvik Desai, Kapil Porwal, Nick Vaccaro, Pranava Y N, Rishika Raj, Subrata Banik, Tarun, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/83947?usp=email
to look at the new patch set (#7).
Change subject: soc/intel: Use NEM+ effective way size for the right platforms
......................................................................
soc/intel: Use NEM+ effective way size for the right platforms
Alder Lake, Meteor Lake and Panther Lake uses the effective way size
when setting up the Enhanced No-Eviction
Mode (cf. `INTEL_CAR_ENEM_USE_EFFECTIVE_WAY_SIZE').
BUG=b:360332771
TEST=Verified on PTL Intel reference platform
Change-Id: I5cb66da0aa977eecb64a0021268a6827747c521b
Signed-off-by: Jeremy Compostella <jeremy.compostella(a)intel.com>
---
M src/soc/intel/alderlake/Kconfig
M src/soc/intel/meteorlake/Kconfig
M src/soc/intel/pantherlake/Kconfig
3 files changed, 3 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/83947/7
--
To view, visit https://review.coreboot.org/c/coreboot/+/83947?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I5cb66da0aa977eecb64a0021268a6827747c521b
Gerrit-Change-Number: 83947
Gerrit-PatchSet: 7
Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: Dinesh Gehlot <digehlot(a)google.com>
Gerrit-Reviewer: Eran Mitrani <mitrani(a)google.com>
Gerrit-Reviewer: Jakub Czapiga <czapiga(a)google.com>
Gerrit-Reviewer: Jayvik Desai <jayvik(a)google.com>
Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org>
Gerrit-Reviewer: Pranava Y N <pranavayn(a)google.com>
Gerrit-Reviewer: Rishika Raj <rishikaraj(a)google.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Tarun <tstuli(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Jayvik Desai <jayvik(a)google.com>
Gerrit-Attention: Rishika Raj <rishikaraj(a)google.com>
Gerrit-Attention: Eran Mitrani <mitrani(a)google.com>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Jakub Czapiga <czapiga(a)google.com>
Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Attention: Dinesh Gehlot <digehlot(a)google.com>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)chromium.org>
Gerrit-Attention: Tarun <tstuli(a)gmail.com>
Gerrit-Attention: Pranava Y N <pranavayn(a)google.com>
Attention is currently required from: Subrata Banik.
Hello Subrata Banik, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/83948?usp=email
to look at the new patch set (#7).
Change subject: soc/intel/common/block/cpu: Fix number of way computation regression
......................................................................
soc/intel/common/block/cpu: Fix number of way computation regression
Commit 16ab9bdcd578612bb3822373547f939eb90afd82 ("soc/intel/common:
Calculate and configure SF Mask 2") breaks the computation of the
number of way and as result, all the derived masks. It results in MSR
such as `IA32_L3_MASK_1' to be improperly programmed yielding
unpredictable NEM issues such as hangs.
Indeed, this commit has introduced a backup of 0x1 into %edx before
comparing the requested cache-as-RAM size against the way size. When
the requested cache-as-RAM is larger, it reaches the second part of
the algorithm which computes the necessary number of way to fit the
requested cache-as-RAM.
This algorithm uses the `div' instruction. Per specification, the div
instruction divides the 64 bits combination of %edx and %eax register.
Since 0x1 got backed up in %edx and assuming a
`CONFIG_DCACHE_RAM_SIZE' of 0x200000, we end up dividing 0x100200000
by the way size instead of 0x200000 which result in a necessary number
of ways of 4098 for a way size of 0x100000.
This commit clears the %edx register before calling the `div'
instruction.
BUG=b:360332771
TEST=Verified on PTL Intel reference platform
Change-Id: I5cb66da0aa977eecb64a0021268a6827747c521d
Signed-off-by: Jeremy Compostella <jeremy.compostella(a)intel.com>
---
M src/soc/intel/common/block/cpu/car/cache_as_ram.S
1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/83948/7
--
To view, visit https://review.coreboot.org/c/coreboot/+/83948?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I5cb66da0aa977eecb64a0021268a6827747c521d
Gerrit-Change-Number: 83948
Gerrit-PatchSet: 7
Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/83982?usp=email
to look at the new patch set (#2).
Change subject: soc/intel/common/block/cpu: Fix number of way computation
......................................................................
soc/intel/common/block/cpu: Fix number of way computation
The way size is not necessarily a power of two. As a result, dividing
`CONFIG_DCACHE_RAM_SIZE' by the way size can leave a remainder. That
remainder should be taken into account to compute the number of way.
BUG=b:360332771
TEST=Verified on PTL Intel reference platform
Change-Id: I5cb66da0aa977eecb64a0021268a6827747c521e
Signed-off-by: Jeremy Compostella <jeremy.compostella(a)intel.com>
---
M src/soc/intel/common/block/cpu/car/cache_as_ram.S
1 file changed, 5 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/83982/2
--
To view, visit https://review.coreboot.org/c/coreboot/+/83982?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I5cb66da0aa977eecb64a0021268a6827747c521e
Gerrit-Change-Number: 83982
Gerrit-PatchSet: 2
Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Attention is currently required from: Subrata Banik.
Hello Subrata Banik, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/83946?usp=email
to look at the new patch set (#6).
Change subject: soc/intel/common/block/cpu: Use the effective way size for NEM+
......................................................................
soc/intel/common/block/cpu: Use the effective way size for NEM+
The Last Level Cache (LLC) way size (or sets) is not necessarily a
power of two. However, on some platforms, the effective way size, the
way size which should be considered for No-Eviction Mode (NEM)
purposes is the biggest power of two of the way size.
Alder Lake External Design Specification #627270 "3.5.2 No-Eviction
Mode (NEM) Sizes" provides some understanding that the maximum NEM
size depends on the number of CBO which used to be accessible via MSR
0x396. Unfortunately, this MSR is not available and as a general
implementation the recommendation is to use the biggest power of two
of the way size instead.
The Kconfig `INTEL_CAR_ENEM_USE_EFFECTIVE_WAY_SIZE' has been
introduced to control this behavior.
BUG=b:360332771
TEST=Verified on PTL Intel reference platform
Change-Id: I5cb66da0aa977eecb64a0021268a6827747c521c
Signed-off-by: Jeremy Compostella <jeremy.compostella(a)intel.com>
---
M src/soc/intel/common/block/cpu/Kconfig
M src/soc/intel/common/block/cpu/car/cache_as_ram.S
2 files changed, 20 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/46/83946/6
--
To view, visit https://review.coreboot.org/c/coreboot/+/83946?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I5cb66da0aa977eecb64a0021268a6827747c521c
Gerrit-Change-Number: 83946
Gerrit-PatchSet: 6
Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Attention is currently required from: Subrata Banik.
Jérémy Compostella has posted comments on this change by Jérémy Compostella. ( https://review.coreboot.org/c/coreboot/+/83948?usp=email )
Change subject: soc/intel/common/block/cpu: Fix number of way computation regression
......................................................................
Patch Set 6:
(1 comment)
File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/83948/comment/75089609_f4b1397b?us… :
PS5, Line 526: xor %edx, %edx
> How does that matter, since line #528 will copy EAX into EDX anyway?
Line #528 is indeed going to copy %eax to %edx but more importantly line #527 is using %edx per x86 div instruction specification and %edx is not 0 because you set it to 1 line #517.
As explained in the commit message, the `div` instruction specification is clear, it divides the 64-bit long %edx:%eax register combination by its parameter (here %ecx). As result of commit 16ab9bdcd578612bb3822373547f939eb90afd82, %edx contains 0x1 when the code reaches line 526.
Below is the code annotated with the value I dumped through some instrumentation. I used `$CONFIG_DCACHE_RAM_SIZE = 0x200000` and a LLC with a way size of 0x180000.
mov $CONFIG_DCACHE_RAM_SIZE, %eax ; 0x200000 -> eax
div %ecx ; 0x100200000 / 0x180000 = 0xaac
mov %eax, %edx ; edx = 0xaac
mov %eax, %ecx ; ecx = 0xaac
movl $0x01, %eax ; eax = 1
shl %cl, %eax ; eax = 0x1000
subl $0x01, %eax ; 0xfff
<https://www.felixcloutier.com/x86/div>
--
To view, visit https://review.coreboot.org/c/coreboot/+/83948?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I5cb66da0aa977eecb64a0021268a6827747c521d
Gerrit-Change-Number: 83948
Gerrit-PatchSet: 6
Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Comment-Date: Mon, 19 Aug 2024 21:24:49 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>