Sharing info about flashrom HW seq code refactoring https://review.coreboot.org/q/topic:PCH_HW_SEQ_Cleanup
Hello Team, Hope you are doing well. I would like bring this activity in your notice https://review.coreboot.org/q/topic:PCH_HW_SEQ_Cleanup and seek help to review the code. The reason for this refactoring of HW sequencing SPI driver code are: 1. We (Chrome OS team) recently ran into a firmware update issue on dogfooders (test device utilise by real users and share feedback) systems, where the attempt to perform SPI flash update operation from host-cpu (using underlying flashrom utility) is silently failing. Furthermore, we debug the issue with help of Intel and figure out the problem is related to multiple master is accessing the SPI flash and operation triggered by host-cpu side using flashrom (write and erase operation) is getting timed out (due to given lesser timeout boundary) due to underlying SPI bus is occupied by other master. This is the special case, starting from Tiger Lake Chrome Platform, where we have enable the PVAP (Protected Audio Video Path) which request CSE to perform some erase and write operation as needed. Now an attempt to perform firmware update from host-cpu side might coincide with the CSE performing SPI operations. The recommendation that coming out from Intel is to account the multiple master SPI operation while we are issuing the timeout from flashrom side. We are also expecting a doc update from Intel to share the exact timeout calculation (although I have attempted to document that early here https://review.coreboot.org/c/flashrom/+/62867/1/ichspi.c#35). 2. Maintain the code symmetry between coreboot and flashrom SPI hardware sequencing operations. From the testing side, I have tested this code on Alderlake `Brya`, Tigerlake `volteer` and Kabylake `eve` devices, and looking for help from community to check on other systems as well (if possible). Furthermore, I have attempted to increase the timeout which won't impact the flashrom performance on the older and/or existing systems. Looking forward for your help on this request. Thanks, Subrata
Hi Subrata, On 16.03.22 17:19, Subrata Banik wrote:
Hope you are doing well. I would like bring this activity in your notice https://review.coreboot.org/q/topic:PCH_HW_SEQ_Cleanup and seek help to review the code.
thank you for bringing this to the mailing list. Not only but also because `ichspi` is maybe the most important driver in flashrom it is important to discuss changes very early. We have to avoid re- gressions at all cost, so all changes need to be fully understood before it makes sense to look into the implementation details.
The reason for this refactoring of HW sequencing SPI driver code are: 1. We (Chrome OS team) recently ran into a firmware update issue on dogfooders (test device utilise by real users and share feedback) systems, where the attempt to perform SPI flash update operation from host-cpu (using underlying flashrom utility) is silently failing. Furthermore, we debug the issue with help of Intel and figure out the problem is related to multiple master is accessing the SPI flash and operation triggered by host-cpu side using flashrom (write and erase operation) is getting timed out (due to given lesser timeout boundary) due to underlying SPI bus is occupied by other master.
Does the issue persist with the following commit applied to libflashrom, activated and integrated into futility? commit 330088d77 [CHROMIUM]: libflashrom: Also use USE_BIG_LOCK This would give us a hint if the issue is indeed due to multiple masters or actually due to multiple programs trying to control the same master.
This is the special case, starting from Tiger Lake Chrome Platform, where we have enable the PVAP (Protected Audio Video Path) which request CSE to perform some erase and write operation as needed.
We used flashrom on PAVP enabled systems for more than a decade without issues. Did something change in that area for Tiger Lake in particular?
2. Maintain the code symmetry between coreboot and flashrom SPI hardware sequencing operations.
Flashrom has tighter requirements compared to coreboot because we try to support all compatible chipset generations with the same code. If you want to align the two code-bases, please use flashrom as reference and patch coreboot. Also, if you want to save some time in the long run: If we'd focus on the flashrom code-base, e.g. prepare libflashrom for easier integration into embedded environments, we could drop the redundant code in core- boot. Cheers, Nico
HI Nico, Thanks for your email. Please find my response below. Thanks, Subrata On Thu, Mar 17, 2022 at 6:08 PM Nico Huber <nico.h@gmx.de> wrote:
Hi Subrata,
On 16.03.22 17:19, Subrata Banik wrote:
Hope you are doing well. I would like bring this activity in your notice https://review.coreboot.org/q/topic:PCH_HW_SEQ_Cleanup and seek help to review the code.
thank you for bringing this to the mailing list. Not only but also because `ichspi` is maybe the most important driver in flashrom it is important to discuss changes very early. We have to avoid re- gressions at all cost, so all changes need to be fully understood before it makes sense to look into the implementation details.
[Subrata] Sure. I will try to explain the situation better and seek help from Anastasia to fill in if still some gap exists.
The reason for this refactoring of HW sequencing SPI driver code are: 1. We (Chrome OS team) recently ran into a firmware update issue on dogfooders (test device utilise by real users and share feedback) systems, where the attempt to perform SPI flash update operation from host-cpu (using underlying flashrom utility) is silently failing. Furthermore, we debug the issue with help of Intel and figure out the problem is related to multiple master is accessing the SPI flash and operation triggered by host-cpu side using flashrom (write and erase operation) is getting timed out (due to given lesser timeout boundary) due to underlying SPI bus is occupied by other master.
Does the issue persist with the following commit applied to libflashrom, activated and integrated into futility?
commit 330088d77 [CHROMIUM]: libflashrom: Also use USE_BIG_LOCK
This would give us a hint if the issue is indeed due to multiple masters or actually due to multiple programs trying to control the same master.
[Subrata] Yes, we are seeing failure even with USE_BIG_LOCK code changes being integrated. The only code change that helps us to W/A this issue is here https://chromium-review.googlesource.com/c/chromiumos/third_party/flashrom/+... (increase timeout in flashrom operations). Also, Intel disable the CSE runtime access to SPI using a custom tool and that helped to fix this update failure as well. These are the evidence towards multi master theory in AU failure issue.
This is the special case, starting from Tiger Lake Chrome Platform, where
we have enable the PVAP (Protected Audio Video Path) which request CSE to perform some erase and write operation as needed.
We used flashrom on PAVP enabled systems for more than a decade without issues. Did something change in that area for Tiger Lake in particular?
[Subrata] I'm not sure about other platforms, but from TGL onwards, on Chrome devices we have enabled PAVP use case and Intel had tried to replicate this issue on older platforms as well and informed us it's replicated the issue on TGL platform as well where PAVP first being enabled and not on JSL (which doesn't enable PAVP). Right now the recommendation that is coming out from Intel is that, on latest Chrome OS devices, there might be concurrent SPI accesses between host CPU and CSE. But I will ask this question about the older generation platform as you have highlighted which has PAVP enabled. Can you please let me know those platform names ? Also, do you know on those devices if we are trying to update the FW using flashrom tool as well or not?
2. Maintain the code symmetry between coreboot and flashrom SPI hardware sequencing operations.
Flashrom has tighter requirements compared to coreboot because we try to support all compatible chipset generations with the same code. If you want to align the two code-bases, please use flashrom as reference and patch coreboot.
[Subrata] Let me understand this a little better, are you suggesting not to increase the SPI operation timing in flashrom upstream master ? knowing that there is a hidden issue acknowledged by Intel with their validation data, also, promised to update the SPI programming guide to capture the recommendation of timeout in multi master scenarios. Apart from timeout increasing CLs, other code changes are to increase the code modularity (like using macro and helper function), are you suggesting to not touch the flashrom code? can't we all contribute towards validating the concerned target platforms. At Least in the coreboot community we all help each other to validate the platform if we are out of required devices.
Also, if you want to save some time in the long run: If we'd focus on the flashrom code-base, e.g. prepare libflashrom for easier integration into embedded environments, we could drop the redundant code in core- boot.
[Subrata] Edward has some suggestions for increasing the code reusability
(between coreboot and flaashrom) going forward, and will let him share his thoughts on this.
Cheers, Nico
On 17.03.22 15:35, Subrata Banik wrote:
On Thu, Mar 17, 2022 at 6:08 PM Nico Huber <nico.h@gmx.de> wrote:
On 16.03.22 17:19, Subrata Banik wrote:
The reason for this refactoring of HW sequencing SPI driver code are: 1. We (Chrome OS team) recently ran into a firmware update issue on dogfooders (test device utilise by real users and share feedback) systems, where the attempt to perform SPI flash update operation from host-cpu (using underlying flashrom utility) is silently failing. Furthermore, we debug the issue with help of Intel and figure out the problem is related to multiple master is accessing the SPI flash and operation triggered by host-cpu side using flashrom (write and erase operation) is getting timed out (due to given lesser timeout boundary) due to underlying SPI bus is occupied by other master.
Does the issue persist with the following commit applied to libflashrom, activated and integrated into futility?
commit 330088d77 [CHROMIUM]: libflashrom: Also use USE_BIG_LOCK
This would give us a hint if the issue is indeed due to multiple masters or actually due to multiple programs trying to control the same master.
[Subrata] Yes, we are seeing failure even with USE_BIG_LOCK code changes being integrated. The only code change that helps us to W/A this issue is here https://chromium-review.googlesource.com/c/chromiumos/third_party/flashrom/+... (increase timeout in flashrom operations). Also, Intel disable the CSE runtime access to SPI using a custom tool and that helped to fix this update failure as well. These are the evidence towards multi master theory in AU failure issue.
I see. I was probably too distracted by the refactoring patches earlier. CB:62867 is indeed necessary. We already assumed a 5s timeout is needed for erasing a block. Worst case seems to be that all masters try to do this at once and then one master would have to wait (NM-1)*5s before its transaction even starts (NM being the number of masters). We could make it depend on the number of masters, or just choose some- thing safe like 30s. WDYT?
This is the special case, starting from Tiger Lake Chrome Platform, where
we have enable the PVAP (Protected Audio Video Path) which request CSE to perform some erase and write operation as needed.
We used flashrom on PAVP enabled systems for more than a decade without issues. Did something change in that area for Tiger Lake in particular?
[Subrata] I'm not sure about other platforms, but from TGL onwards, on Chrome devices we have enabled PAVP use case and Intel had tried to replicate this issue on older platforms as well and informed us it's replicated the issue on TGL platform as well where PAVP first being enabled and not on JSL (which doesn't enable PAVP). Right now the recommendation that is coming out from Intel is that, on latest Chrome OS devices, there might be concurrent SPI accesses between host CPU and CSE. But I will ask this question about the older generation platform as you have highlighted which has PAVP enabled. Can you please let me know those platform names ? Also, do you know on those devices if we are trying to update the FW using flashrom tool as well or not?
Thanks for looking into it. As our timeouts were way below the worst case, I assume enabling PAVP just makes the problem more visible and is not directly related. It could also be any other code running on the CSE that changed between JSL and TGL (unless you know, ofc., that PAVP is the only code that writes to flash). Flashrom basically runs on everything so the platform names would be all platforms that support PAVP. Probably not worth looking further into it unless there are still issues with a reasonable timeout.
2. Maintain the code symmetry between coreboot and flashrom SPI hardware sequencing operations.
Flashrom has tighter requirements compared to coreboot because we try to support all compatible chipset generations with the same code. If you want to align the two code-bases, please use flashrom as reference and patch coreboot.
[Subrata] Let me understand this a little better, are you suggesting not to increase the SPI operation timing in flashrom upstream master ? knowing that there is a hidden issue acknowledged by Intel with their validation data, also, promised to update the SPI programming guide to capture the recommendation of timeout in multi master scenarios.
No I'm not suggesting any such thing. What I wrote was related to your 2. point as quoted above.
Apart from timeout increasing CLs, other code changes are to increase the code modularity (like using macro and helper function), are you suggesting to not touch the flashrom code? can't we all contribute towards validating the concerned target platforms. At Least in the coreboot community we all help each other to validate the platform if we are out of required devices.
We can have a look at the individual changes if there is a reason to change the code, i.e. to make it better. Only your reasoning "maintain the code symmetry" is not enough to justify changes. I'm sorry if this affects your work, but we have very bad experience with unnecessary code changes by CrOS developers. Currently `sb600spi` (the equivalent to `ichspi` for AMD platforms) is broken since months and only CrOS developers could clarify why the code was changed as it was. We need to prevent that this happens to `ichspi` too, right? Nico
On Thu, Mar 17, 2022 at 10:35 AM Nico Huber <nico.h@gmx.de> wrote:
On 17.03.22 15:35, Subrata Banik wrote:
On Thu, Mar 17, 2022 at 6:08 PM Nico Huber <nico.h@gmx.de> wrote:
On 16.03.22 17:19, Subrata Banik wrote:
The reason for this refactoring of HW sequencing SPI driver code are: 1. We (Chrome OS team) recently ran into a firmware update issue on dogfooders (test device utilise by real users and share feedback) systems, where the attempt to perform SPI flash update operation from host-cpu (using underlying flashrom utility) is silently failing. Furthermore, we debug the issue with help of Intel and figure out the problem is related to multiple master is accessing the SPI flash and operation triggered by host-cpu side using flashrom (write and erase operation) is getting timed out (due to given lesser timeout boundary) due to underlying SPI bus is occupied by other master.
Does the issue persist with the following commit applied to libflashrom, activated and integrated into futility?
commit 330088d77 [CHROMIUM]: libflashrom: Also use USE_BIG_LOCK
This would give us a hint if the issue is indeed due to multiple masters or actually due to multiple programs trying to control the same master.
[Subrata] Yes, we are seeing failure even with USE_BIG_LOCK code changes being integrated. The only code change that helps us to W/A this issue is here
https://chromium-review.googlesource.com/c/chromiumos/third_party/flashrom/+...
(increase timeout in flashrom operations). Also, Intel disable the CSE runtime access to SPI using a custom tool and that helped to fix this update failure as well. These are the evidence towards multi master theory in AU failure issue.
I see. I was probably too distracted by the refactoring patches earlier. CB:62867 is indeed necessary.
We seem to have actually figured out what the actual root cause of our problem was, and it indeed does seem to be the multi-master problem and flashrom not waiting long enough in these cases. tl;dr widevine provisioning was failing, and repeating endlessly in a loop (~each second or so) and each attempt actually causes the CSE to write to its data area in flash. Unfortunately, we do not have fantastic visibility into the processes that are running during AU, and so this was very hard to track down. We have fixed the issues with widevine provisioning and also added a max 5 attempts to provision a device instead of writing endlessly to the flash in the failure case.
We already assumed a 5s timeout is needed for erasing a block. Worst case seems to be that all masters try to do this at once and then one master would have to wait (NM-1)*5s before its transaction even starts (NM being the number of masters).
We could make it depend on the number of masters, or just choose some- thing safe like 30s. WDYT?
I think the longer the timeout (until it gets ridiculous), probably the better. Unless there is buggy hardware out there, there is no good reason for these operations to timeout; there could for sure be SPI flash chips out there that perhaps aren't always performing to-spec, who knows. But in this case, since do we know the maximum number of masters, and 5s per erase seems reasonable too, then 30 seconds makes sense to me.
This is the special case, starting from Tiger Lake Chrome Platform, where
we have enable the PVAP (Protected Audio Video Path) which request CSE to perform some erase and write operation as needed.
We used flashrom on PAVP enabled systems for more than a decade without issues. Did something change in that area for Tiger Lake in particular?
[Subrata] I'm not sure about other platforms, but from TGL onwards, on Chrome devices we have enabled PAVP use case and Intel had tried to replicate this issue on older platforms as well and informed us it's replicated the issue on TGL platform as well where PAVP first being enabled and not on JSL (which doesn't enable PAVP). Right now the recommendation that is coming out from Intel is that, on latest Chrome OS devices, there might be concurrent SPI accesses between host CPU and CSE. But I will ask this question about the older generation platform as you have highlighted which has PAVP enabled. Can you please let me know those platform names ? Also, do you know on those devices if we are trying to update the FW using flashrom tool as well or not?
Thanks for looking into it. As our timeouts were way below the worst case, I assume enabling PAVP just makes the problem more visible and is not directly related. It could also be any other code running on the CSE that changed between JSL and TGL (unless you know, ofc., that PAVP is the only code that writes to flash).
PAVP is the only thing we know of ATM that writes to the flash aside from perhaps after the first flash of the ME region. The reason this became visible on brya only now I explained just above :)
Flashrom basically runs on everything so the platform names would be all platforms that support PAVP. Probably not worth looking further into it unless there are still issues with a reasonable timeout.
2. Maintain the code symmetry between coreboot and flashrom SPI
hardware
sequencing operations.
Flashrom has tighter requirements compared to coreboot because we try to support all compatible chipset generations with the same code. If you want to align the two code-bases, please use flashrom as reference and patch coreboot.
[Subrata] Let me understand this a little better, are you suggesting not to increase the SPI operation timing in flashrom upstream master ? knowing that there is a hidden issue acknowledged by Intel with their validation data, also, promised to update the SPI programming guide to capture the recommendation of timeout in multi master scenarios.
No I'm not suggesting any such thing. What I wrote was related to your 2. point as quoted above.
Apart from timeout increasing CLs, other code changes are to increase the code modularity (like using macro and helper function), are you
suggesting
to not touch the flashrom code? can't we all contribute towards validating the concerned target platforms. At Least in the coreboot community we all help each other to validate the platform if we are out of required devices.
We can have a look at the individual changes if there is a reason to change the code, i.e. to make it better. Only your reasoning "maintain the code symmetry" is not enough to justify changes.
I'm sorry if this affects your work, but we have very bad experience with unnecessary code changes by CrOS developers. Currently `sb600spi` (the equivalent to `ichspi` for AMD platforms) is broken since months and only CrOS developers could clarify why the code was changed as it was. We need to prevent that this happens to `ichspi` too, right?
Nico
We are certainly not intending to break ichspi or make it worse, we are trying to make it better, perhaps we were a little hasty in trying to get through solutions that were not perhaps the root of the problem, but were discovered along the way. We are also working with Intel to get some documentation out about the expected behaviors with regard to the multi-master scenarios, hopefully that will help clarify things some in this regard. -Tim
Hi Tim, On 17.03.22 17:54, Tim Wawrzynczak wrote:
On Thu, Mar 17, 2022 at 10:35 AM Nico Huber <nico.h@gmx.de> wrote:
I'm sorry if this affects your work, but we have very bad experience with unnecessary code changes by CrOS developers. Currently `sb600spi` (the equivalent to `ichspi` for AMD platforms) is broken since months and only CrOS developers could clarify why the code was changed as it was. We need to prevent that this happens to `ichspi` too, right?
Nico
We are certainly not intending to break ichspi or make it worse, we are trying to make it better
I always assume that's the case for everybody :) Still, sometimes things break and we know already that things can break even due to unnecessary refactorings. And saying the reason for a change is to align with a project with lower quality require- ments seems kind of odd, doesn't it? And when reviewers are busy with refactorings instead of attending the changes they already merged, that feels very wrong :-/
, perhaps we were a little hasty in trying to get through solutions that were not perhaps the root of the problem, but were discovered along the way.
We are also working with Intel to get some documentation out about the expected behaviors with regard to the multi-master scenarios, hopefully that will help clarify things some in this regard.
If the timeout is the only issue, the hardware works as expected. Stating an expected timeout in the datasheets would have avoided the problem of course :) Nico
I agree with Tim and will try to provide more requested information and testing with those CLs.
Only your reasoning "maintain the code symmetry" is not enough to justify changes.
Kindly ignore my #2 comment, re-reading it doesn't make sense to me :) to maintain the symmetry is the real reason for those code changes. Looking for review comments and I will address those earliest. Thanks for having good discussion here. Thanks, Subrata On Thu, Mar 17, 2022 at 10:24 PM Tim Wawrzynczak <twawrzynczak@google.com> wrote:
On Thu, Mar 17, 2022 at 10:35 AM Nico Huber <nico.h@gmx.de> wrote:
On 17.03.22 15:35, Subrata Banik wrote:
On Thu, Mar 17, 2022 at 6:08 PM Nico Huber <nico.h@gmx.de> wrote:
On 16.03.22 17:19, Subrata Banik wrote:
The reason for this refactoring of HW sequencing SPI driver code are: 1. We (Chrome OS team) recently ran into a firmware update issue on dogfooders (test device utilise by real users and share feedback) systems, where the attempt to perform SPI flash update operation from host-cpu (using underlying flashrom utility) is silently failing. Furthermore, we debug the issue with help of Intel and figure out the problem is related to multiple master is accessing the SPI flash and operation triggered by host-cpu side using flashrom (write and erase operation) is getting timed out (due to given lesser timeout boundary) due to underlying SPI bus is occupied by other master.
Does the issue persist with the following commit applied to libflashrom, activated and integrated into futility?
commit 330088d77 [CHROMIUM]: libflashrom: Also use USE_BIG_LOCK
This would give us a hint if the issue is indeed due to multiple masters or actually due to multiple programs trying to control the same master.
[Subrata] Yes, we are seeing failure even with USE_BIG_LOCK code changes being integrated. The only code change that helps us to W/A this issue is here
https://chromium-review.googlesource.com/c/chromiumos/third_party/flashrom/+...
(increase timeout in flashrom operations). Also, Intel disable the CSE runtime access to SPI using a custom tool and that helped to fix this update failure as well. These are the evidence towards multi master theory in AU failure issue.
I see. I was probably too distracted by the refactoring patches earlier. CB:62867 is indeed necessary.
We seem to have actually figured out what the actual root cause of our problem was, and it indeed does seem to be the multi-master problem and flashrom not waiting long enough in these cases. tl;dr widevine provisioning was failing, and repeating endlessly in a loop (~each second or so) and each attempt actually causes the CSE to write to its data area in flash. Unfortunately, we do not have fantastic visibility into the processes that are running during AU, and so this was very hard to track down. We have fixed the issues with widevine provisioning and also added a max 5 attempts to provision a device instead of writing endlessly to the flash in the failure case.
We already assumed a 5s timeout is needed for erasing a block. Worst case seems to be that all masters try to do this at once and then one master would have to wait (NM-1)*5s before its transaction even starts (NM being the number of masters).
We could make it depend on the number of masters, or just choose some- thing safe like 30s. WDYT?
I think the longer the timeout (until it gets ridiculous), probably the better. Unless there is buggy hardware out there, there is no good reason for these operations to timeout; there could for sure be SPI flash chips out there that perhaps aren't always performing to-spec, who knows.
But in this case, since do we know the maximum number of masters, and 5s per erase seems reasonable too, then 30 seconds makes sense to me.
This is the special case, starting from Tiger Lake Chrome Platform, where
we have enable the PVAP (Protected Audio Video Path) which request CSE to perform some erase and write operation as needed.
We used flashrom on PAVP enabled systems for more than a decade without issues. Did something change in that area for Tiger Lake in particular?
[Subrata] I'm not sure about other platforms, but from TGL onwards, on Chrome devices we have enabled PAVP use case and Intel had tried to replicate this issue on older platforms as well and informed us it's replicated the issue on TGL platform as well where PAVP first being enabled and not on JSL (which doesn't enable PAVP). Right now the recommendation that is coming out from Intel is that, on latest Chrome OS devices, there might be concurrent SPI accesses between host CPU and CSE. But I will ask this question about the older generation platform as you have highlighted which has PAVP enabled. Can you please let me know those platform names ? Also, do you know on those devices if we are trying to update the FW using flashrom tool as well or not?
Thanks for looking into it. As our timeouts were way below the worst case, I assume enabling PAVP just makes the problem more visible and is not directly related. It could also be any other code running on the CSE that changed between JSL and TGL (unless you know, ofc., that PAVP is the only code that writes to flash).
PAVP is the only thing we know of ATM that writes to the flash aside from perhaps after the first flash of the ME region.
The reason this became visible on brya only now I explained just above :)
Flashrom basically runs on everything so the platform names would be all platforms that support PAVP. Probably not worth looking further into it unless there are still issues with a reasonable timeout.
2. Maintain the code symmetry between coreboot and flashrom SPI
hardware
sequencing operations.
Flashrom has tighter requirements compared to coreboot because we try to support all compatible chipset generations with the same code. If you want to align the two code-bases, please use flashrom as reference and patch coreboot.
[Subrata] Let me understand this a little better, are you suggesting not to increase the SPI operation timing in flashrom upstream master ? knowing that there is a hidden issue acknowledged by Intel with their validation data, also, promised to update the SPI programming guide to capture the recommendation of timeout in multi master scenarios.
No I'm not suggesting any such thing. What I wrote was related to your 2. point as quoted above.
Apart from timeout increasing CLs, other code changes are to increase
the
code modularity (like using macro and helper function), are you suggesting to not touch the flashrom code? can't we all contribute towards validating the concerned target platforms. At Least in the coreboot community we all help each other to validate the platform if we are out of required devices.
We can have a look at the individual changes if there is a reason to change the code, i.e. to make it better. Only your reasoning "maintain the code symmetry" is not enough to justify changes.
I'm sorry if this affects your work, but we have very bad experience with unnecessary code changes by CrOS developers. Currently `sb600spi` (the equivalent to `ichspi` for AMD platforms) is broken since months and only CrOS developers could clarify why the code was changed as it was. We need to prevent that this happens to `ichspi` too, right?
Nico
We are certainly not intending to break ichspi or make it worse, we are trying to make it better, perhaps we were a little hasty in trying to get through solutions that were not perhaps the root of the problem, but were discovered along the way.
We are also working with Intel to get some documentation out about the expected behaviors with regard to the multi-master scenarios, hopefully that will help clarify things some in this regard.
-Tim
HI Team, Hope you are doing well. Can I request you to support review for https://review.coreboot.org/q/topic:PCH_HW_SEQ_Cleanup and validation on older ICH/PCH chipsets. I have done my testing so far on Chrome OS devices but would like to get your help on older devices if possible. Thanks, Subrata On Fri, Mar 18, 2022 at 12:11 AM Subrata Banik <subratabanik@google.com> wrote:
I agree with Tim and will try to provide more requested information and testing with those CLs.
Only your reasoning "maintain the code symmetry" is not enough to justify changes.
Kindly ignore my #2 comment, re-reading it doesn't make sense to me :) to maintain the symmetry is the real reason for those code changes.
Looking for review comments and I will address those earliest.
Thanks for having good discussion here.
Thanks, Subrata
On Thu, Mar 17, 2022 at 10:24 PM Tim Wawrzynczak <twawrzynczak@google.com> wrote:
On Thu, Mar 17, 2022 at 10:35 AM Nico Huber <nico.h@gmx.de> wrote:
On 17.03.22 15:35, Subrata Banik wrote:
On Thu, Mar 17, 2022 at 6:08 PM Nico Huber <nico.h@gmx.de> wrote:
On 16.03.22 17:19, Subrata Banik wrote:
The reason for this refactoring of HW sequencing SPI driver code are: 1. We (Chrome OS team) recently ran into a firmware update issue on dogfooders (test device utilise by real users and share feedback) systems, where the attempt to perform SPI flash update operation from host-cpu (using underlying flashrom utility) is silently failing. Furthermore, we debug the issue with help of Intel and figure out the problem is related to multiple master is accessing the SPI flash and operation triggered by host-cpu side using flashrom (write and erase operation) is getting timed out (due to given lesser timeout boundary) due to underlying SPI bus is occupied by other master.
Does the issue persist with the following commit applied to libflashrom, activated and integrated into futility?
commit 330088d77 [CHROMIUM]: libflashrom: Also use USE_BIG_LOCK
This would give us a hint if the issue is indeed due to multiple masters or actually due to multiple programs trying to control the same master.
[Subrata] Yes, we are seeing failure even with USE_BIG_LOCK code changes being integrated. The only code change that helps us to W/A this issue is here
https://chromium-review.googlesource.com/c/chromiumos/third_party/flashrom/+...
(increase timeout in flashrom operations). Also, Intel disable the CSE runtime access to SPI using a custom tool and that helped to fix this update failure as well. These are the evidence towards multi master theory in AU failure issue.
I see. I was probably too distracted by the refactoring patches earlier. CB:62867 is indeed necessary.
We seem to have actually figured out what the actual root cause of our problem was, and it indeed does seem to be the multi-master problem and flashrom not waiting long enough in these cases. tl;dr widevine provisioning was failing, and repeating endlessly in a loop (~each second or so) and each attempt actually causes the CSE to write to its data area in flash. Unfortunately, we do not have fantastic visibility into the processes that are running during AU, and so this was very hard to track down. We have fixed the issues with widevine provisioning and also added a max 5 attempts to provision a device instead of writing endlessly to the flash in the failure case.
We already assumed a 5s timeout is needed for erasing a block. Worst case seems to be that all masters try to do this at once and then one master would have to wait (NM-1)*5s before its transaction even starts (NM being the number of masters).
We could make it depend on the number of masters, or just choose some- thing safe like 30s. WDYT?
I think the longer the timeout (until it gets ridiculous), probably the better. Unless there is buggy hardware out there, there is no good reason for these operations to timeout; there could for sure be SPI flash chips out there that perhaps aren't always performing to-spec, who knows.
But in this case, since do we know the maximum number of masters, and 5s per erase seems reasonable too, then 30 seconds makes sense to me.
This is the special case, starting from Tiger Lake Chrome Platform, where
we have enable the PVAP (Protected Audio Video Path) which request CSE to perform some erase and write operation as needed.
We used flashrom on PAVP enabled systems for more than a decade without issues. Did something change in that area for Tiger Lake in particular?
[Subrata] I'm not sure about other platforms, but from TGL onwards, on Chrome devices we have enabled PAVP use case and Intel had tried to replicate this issue on older platforms as well and informed us it's replicated the issue on TGL platform as well where PAVP first being enabled and not on JSL (which doesn't enable PAVP). Right now the recommendation that is coming out from Intel is that, on latest Chrome OS devices, there might be concurrent SPI accesses between host CPU and CSE. But I will ask this question about the older generation platform as you have highlighted which has PAVP enabled. Can you please let me know those platform names ? Also, do you know on those devices if we are trying to update the FW using flashrom tool as well or not?
Thanks for looking into it. As our timeouts were way below the worst case, I assume enabling PAVP just makes the problem more visible and is not directly related. It could also be any other code running on the CSE that changed between JSL and TGL (unless you know, ofc., that PAVP is the only code that writes to flash).
PAVP is the only thing we know of ATM that writes to the flash aside from perhaps after the first flash of the ME region.
The reason this became visible on brya only now I explained just above :)
Flashrom basically runs on everything so the platform names would be all platforms that support PAVP. Probably not worth looking further into it unless there are still issues with a reasonable timeout.
2. Maintain the code symmetry between coreboot and flashrom SPI
hardware
sequencing operations.
Flashrom has tighter requirements compared to coreboot because we try to support all compatible chipset generations with the same code. If you want to align the two code-bases, please use flashrom as reference and patch coreboot.
[Subrata] Let me understand this a little better, are you suggesting not to increase the SPI operation timing in flashrom upstream master ? knowing that there is a hidden issue acknowledged by Intel with their validation data, also, promised to update the SPI programming guide to capture the recommendation of timeout in multi master scenarios.
No I'm not suggesting any such thing. What I wrote was related to your 2. point as quoted above.
Apart from timeout increasing CLs, other code changes are to increase
the
code modularity (like using macro and helper function), are you suggesting to not touch the flashrom code? can't we all contribute towards validating the concerned target platforms. At Least in the coreboot community we all help each other to validate the platform if we are out of required devices.
We can have a look at the individual changes if there is a reason to change the code, i.e. to make it better. Only your reasoning "maintain the code symmetry" is not enough to justify changes.
I'm sorry if this affects your work, but we have very bad experience with unnecessary code changes by CrOS developers. Currently `sb600spi` (the equivalent to `ichspi` for AMD platforms) is broken since months and only CrOS developers could clarify why the code was changed as it was. We need to prevent that this happens to `ichspi` too, right?
Nico
We are certainly not intending to break ichspi or make it worse, we are trying to make it better, perhaps we were a little hasty in trying to get through solutions that were not perhaps the root of the problem, but were discovered along the way.
We are also working with Intel to get some documentation out about the expected behaviors with regard to the multi-master scenarios, hopefully that will help clarify things some in this regard.
-Tim
participants (3)
-
Nico Huber -
Subrata Banik -
Tim Wawrzynczak