Re: [PATCH 0/3] Revert HAFDBS changes
On Fri, Nov 10, 2023 at 06:38:40PM +1300, Chris Packham wrote: > On Fri, 10 Nov 2023, 10:33 am Tom Rini, wrote: > > > On Fri, Oct 27, 2023 at 01:22:37PM -0400, Tom Rini wrote: > > > On Fri, Oct 27, 2023 at 10:49:47AM +0100, Pierre-Clément Tosi wrote: > > > > Hi Chris, > > > > > > > > On Fri, Oct 27, 2023 at 01:23:51PM +1300, Chris Packham wrote: > > > > > As discussed this series reverts the HAFDBS changes that caused an > > issue > > > > > on AC5/AC5X. I think there are some improvements that can be made to > > the > > > > > initial memory map for the AC5/AC5X but so far nothing I've found > > makes > > > > > it compatible with the HAFDBS changes. > > > > > > > > Would you mind briefly explaining what those issues are and/or point > > me to the > > > > discussion where it was decided to revert these patches? > > > > > > > > The feature is quite useful for users of CONFIG_CMO_BY_VA_ONLY, to > > speed up the > > > > boot sequence: instead of reverting these patches altogether, would a > > reasonable > > > > alternative be to put them behind a build option? > > > > > > > > Also, could you confirm that the "initial memory map" you are > > referring to above > > > > only describes actual memory as, IIRC, some boards were using mappings > > **much** > > > > larger than their DRAM address space? > > > > > > The most details are in this thread: > > > > > https://lore.kernel.org/u-boot/CAFOYHZC_Dveax85n0fLr5BFyZcLqsvUssn=j0ohyvn75bta...@mail.gmail.com/ > > > with some follow-up in: > > > > > https://lore.kernel.org/u-boot/CAFOYHZB7jJWwD24oWzx6u55w2whHYjK56f=qyn0lwu4z8ds...@mail.gmail.com/ > > > > Checking to see if you have any feedback for these platforms? I would > > like to have them working again one way or another in v2024.01, thanks. > > > > Either this series or > https://lore.kernel.org/u-boot/20231018205358.1557234-1-judge.pack...@gmail.com/ > will get the AC5X boards back working. I'm out of other ideas but happy to > test patches. Following up here on the cover letter as well that I've applied the revert for now. I'm happy to revert the revert if we can get the AC5X platforms fixed, and perhaps a little documentation about what was going wrong as I believe the other thread at least hinted that other platforms might be doing a workaround as well but didn't spell out why. -- Tom signature.asc Description: PGP signature
Re: [PATCH 0/3] Revert HAFDBS changes
On Fri, 10 Nov 2023, 10:33 am Tom Rini, wrote: > On Fri, Oct 27, 2023 at 01:22:37PM -0400, Tom Rini wrote: > > On Fri, Oct 27, 2023 at 10:49:47AM +0100, Pierre-Clément Tosi wrote: > > > Hi Chris, > > > > > > On Fri, Oct 27, 2023 at 01:23:51PM +1300, Chris Packham wrote: > > > > As discussed this series reverts the HAFDBS changes that caused an > issue > > > > on AC5/AC5X. I think there are some improvements that can be made to > the > > > > initial memory map for the AC5/AC5X but so far nothing I've found > makes > > > > it compatible with the HAFDBS changes. > > > > > > Would you mind briefly explaining what those issues are and/or point > me to the > > > discussion where it was decided to revert these patches? > > > > > > The feature is quite useful for users of CONFIG_CMO_BY_VA_ONLY, to > speed up the > > > boot sequence: instead of reverting these patches altogether, would a > reasonable > > > alternative be to put them behind a build option? > > > > > > Also, could you confirm that the "initial memory map" you are > referring to above > > > only describes actual memory as, IIRC, some boards were using mappings > **much** > > > larger than their DRAM address space? > > > > The most details are in this thread: > > > https://lore.kernel.org/u-boot/CAFOYHZC_Dveax85n0fLr5BFyZcLqsvUssn=j0ohyvn75bta...@mail.gmail.com/ > > with some follow-up in: > > > https://lore.kernel.org/u-boot/CAFOYHZB7jJWwD24oWzx6u55w2whHYjK56f=qyn0lwu4z8ds...@mail.gmail.com/ > > Checking to see if you have any feedback for these platforms? I would > like to have them working again one way or another in v2024.01, thanks. > Either this series or https://lore.kernel.org/u-boot/20231018205358.1557234-1-judge.pack...@gmail.com/ will get the AC5X boards back working. I'm out of other ideas but happy to test patches. >
Re: [PATCH 0/3] Revert HAFDBS changes
On Fri, Oct 27, 2023 at 01:22:37PM -0400, Tom Rini wrote: > On Fri, Oct 27, 2023 at 10:49:47AM +0100, Pierre-Clément Tosi wrote: > > Hi Chris, > > > > On Fri, Oct 27, 2023 at 01:23:51PM +1300, Chris Packham wrote: > > > As discussed this series reverts the HAFDBS changes that caused an issue > > > on AC5/AC5X. I think there are some improvements that can be made to the > > > initial memory map for the AC5/AC5X but so far nothing I've found makes > > > it compatible with the HAFDBS changes. > > > > Would you mind briefly explaining what those issues are and/or point me to > > the > > discussion where it was decided to revert these patches? > > > > The feature is quite useful for users of CONFIG_CMO_BY_VA_ONLY, to speed up > > the > > boot sequence: instead of reverting these patches altogether, would a > > reasonable > > alternative be to put them behind a build option? > > > > Also, could you confirm that the "initial memory map" you are referring to > > above > > only describes actual memory as, IIRC, some boards were using mappings > > **much** > > larger than their DRAM address space? > > The most details are in this thread: > https://lore.kernel.org/u-boot/CAFOYHZC_Dveax85n0fLr5BFyZcLqsvUssn=j0ohyvn75bta...@mail.gmail.com/ > with some follow-up in: > https://lore.kernel.org/u-boot/CAFOYHZB7jJWwD24oWzx6u55w2whHYjK56f=qyn0lwu4z8ds...@mail.gmail.com/ Checking to see if you have any feedback for these platforms? I would like to have them working again one way or another in v2024.01, thanks. -- Tom signature.asc Description: PGP signature
Re: [PATCH 0/3] Revert HAFDBS changes
On Fri, Oct 27, 2023 at 10:49:47AM +0100, Pierre-Clément Tosi wrote: > Hi Chris, > > On Fri, Oct 27, 2023 at 01:23:51PM +1300, Chris Packham wrote: > > As discussed this series reverts the HAFDBS changes that caused an issue > > on AC5/AC5X. I think there are some improvements that can be made to the > > initial memory map for the AC5/AC5X but so far nothing I've found makes > > it compatible with the HAFDBS changes. > > Would you mind briefly explaining what those issues are and/or point me to the > discussion where it was decided to revert these patches? > > The feature is quite useful for users of CONFIG_CMO_BY_VA_ONLY, to speed up > the > boot sequence: instead of reverting these patches altogether, would a > reasonable > alternative be to put them behind a build option? > > Also, could you confirm that the "initial memory map" you are referring to > above > only describes actual memory as, IIRC, some boards were using mappings > **much** > larger than their DRAM address space? The most details are in this thread: https://lore.kernel.org/u-boot/CAFOYHZC_Dveax85n0fLr5BFyZcLqsvUssn=j0ohyvn75bta...@mail.gmail.com/ with some follow-up in: https://lore.kernel.org/u-boot/CAFOYHZB7jJWwD24oWzx6u55w2whHYjK56f=qyn0lwu4z8ds...@mail.gmail.com/ -- Tom signature.asc Description: PGP signature
Re: [PATCH 0/3] Revert HAFDBS changes
Hi Marc, On Fri, Oct 27, 2023 at 03:11:15PM +0100, Marc Zyngier wrote: > Hi Pierre-Clément, > > On Fri, 27 Oct 2023 10:49:47 +0100, > Pierre-Clément Tosi wrote: > > > > Hi Chris, > > > > On Fri, Oct 27, 2023 at 01:23:51PM +1300, Chris Packham wrote: > > > As discussed this series reverts the HAFDBS changes that caused an issue > > > on AC5/AC5X. I think there are some improvements that can be made to the > > > initial memory map for the AC5/AC5X but so far nothing I've found makes > > > it compatible with the HAFDBS changes. > > > > Would you mind briefly explaining what those issues are and/or point > > me to the discussion where it was decided to revert these patches? > > It was me[1] who offered to revert them, as the "fix" that Chris came > up with didn't make much sense on its own, was provided without much > of an explanation, and nobody else with sufficient understanding of > the ARMv8 translation model seem to have access to this HW to debug > it (though if someone sends me a board, I'll happily look into it). Thanks for the context. I have checked with our internal users of the patches being reverted and they don't mind the performance hit; if no one else is worried about the potential regression, reverting (instead of properly addressing the root cause) might be a reasonable option. > To be clear, I'm pretty much convinced that the issue is due to the > way page tables are managed on the arm64 port. The whole "emergency > page table" switch doesn't make much sense, specially when changing > something as simple as some page attributes, and it isn't like "break > before make" is a brand new concept. It might not "make sense" from a functional/performance perspective (as it isn't strictly necessary) but I was assuming that it at least complied with the architecture (i.e. should not cause the CPU to lockup, as Chris reports). Even if the reverts get merged, it would be good to clear up if it was a bug in the logic (e.g. how is gd->arch.tlb_emerg affected by these patches?) or a violation of the architecture and if the issue was actually coming from the HAFDBS changes or if they were triggering it. Otherwise, these reverts might be masking the symptom instead of addressing the cause. As a result, we would be left with code that seems to "work" but is still as fragile as before, where any future patch is at risk of triggering the faulty behavior and of ending up also being reverted. > > > The feature is quite useful for users of CONFIG_CMO_BY_VA_ONLY, to > > speed up the boot sequence: instead of reverting these patches > > altogether, would a reasonable alternative be to put them behind a > > build option? > > I don't think this is right. Either we have something that works for > all ARMV8.1+ systems, or it just makes things a lot less maintainable. > > But maybe the u-boot maintainers have a different view on this. > > The core of the problem is that there is a truckload of code that > already exists in the tree and that somehow relies on the existing > (and funky) behaviours[2]. How do we go about fixing the core u-boot? > I have no idea. > > Cheers, > > M. > > [1] https://lore.kernel.org/r/063a16d559dc9cb327c9459803005...@kernel.org > [2] > https://lore.kernel.org/r/CAFOYHZC_Dveax85n0fLr5BFyZcLqsvUssn=j0ohyvn75bta...@mail.gmail.com > > -- > Without deviation from the norm, progress is not possible. Thanks, -- Pierre
Re: [PATCH 0/3] Revert HAFDBS changes
Hi Pierre-Clément, On Fri, 27 Oct 2023 10:49:47 +0100, Pierre-Clément Tosi wrote: > > Hi Chris, > > On Fri, Oct 27, 2023 at 01:23:51PM +1300, Chris Packham wrote: > > As discussed this series reverts the HAFDBS changes that caused an issue > > on AC5/AC5X. I think there are some improvements that can be made to the > > initial memory map for the AC5/AC5X but so far nothing I've found makes > > it compatible with the HAFDBS changes. > > Would you mind briefly explaining what those issues are and/or point > me to the discussion where it was decided to revert these patches? It was me[1] who offered to revert them, as the "fix" that Chris came up with didn't make much sense on its own, was provided without much of an explanation, and nobody else with sufficient understanding of the ARMv8 translation model seem to have access to this HW to debug it (though if someone sends me a board, I'll happily look into it). To be clear, I'm pretty much convinced that the issue is due to the way page tables are managed on the arm64 port. The whole "emergency page table" switch doesn't make much sense, specially when changing something as simple as some page attributes, and it isn't like "break before make" is a brand new concept. > The feature is quite useful for users of CONFIG_CMO_BY_VA_ONLY, to > speed up the boot sequence: instead of reverting these patches > altogether, would a reasonable alternative be to put them behind a > build option? I don't think this is right. Either we have something that works for all ARMV8.1+ systems, or it just makes things a lot less maintainable. But maybe the u-boot maintainers have a different view on this. The core of the problem is that there is a truckload of code that already exists in the tree and that somehow relies on the existing (and funky) behaviours[2]. How do we go about fixing the core u-boot? I have no idea. Cheers, M. [1] https://lore.kernel.org/r/063a16d559dc9cb327c9459803005...@kernel.org [2] https://lore.kernel.org/r/CAFOYHZC_Dveax85n0fLr5BFyZcLqsvUssn=j0ohyvn75bta...@mail.gmail.com -- Without deviation from the norm, progress is not possible.
Re: [PATCH 0/3] Revert HAFDBS changes
Hi Chris, On Fri, Oct 27, 2023 at 01:23:51PM +1300, Chris Packham wrote: > As discussed this series reverts the HAFDBS changes that caused an issue > on AC5/AC5X. I think there are some improvements that can be made to the > initial memory map for the AC5/AC5X but so far nothing I've found makes > it compatible with the HAFDBS changes. Would you mind briefly explaining what those issues are and/or point me to the discussion where it was decided to revert these patches? The feature is quite useful for users of CONFIG_CMO_BY_VA_ONLY, to speed up the boot sequence: instead of reverting these patches altogether, would a reasonable alternative be to put them behind a build option? Also, could you confirm that the "initial memory map" you are referring to above only describes actual memory as, IIRC, some boards were using mappings **much** larger than their DRAM address space? > > > Chris Packham (3): > Revert "armv8: enable HAFDBS for other ELx when FEAT_HAFDBS is > present" > Revert "arm64: Use level-2 for largest block mappings when FEAT_HAFDBS > is present" > Revert "arm64: Use FEAT_HAFDBS to track dirty pages when available" > > arch/arm/cpu/armv8/cache_v8.c | 30 +++--- > arch/arm/include/asm/armv8/mmu.h | 20 > arch/arm/include/asm/global_data.h | 2 -- > 3 files changed, 7 insertions(+), 45 deletions(-) > > -- > 2.42.0 > -- Pierre
[PATCH 0/3] Revert HAFDBS changes
As discussed this series reverts the HAFDBS changes that caused an issue on AC5/AC5X. I think there are some improvements that can be made to the initial memory map for the AC5/AC5X but so far nothing I've found makes it compatible with the HAFDBS changes. Chris Packham (3): Revert "armv8: enable HAFDBS for other ELx when FEAT_HAFDBS is present" Revert "arm64: Use level-2 for largest block mappings when FEAT_HAFDBS is present" Revert "arm64: Use FEAT_HAFDBS to track dirty pages when available" arch/arm/cpu/armv8/cache_v8.c | 30 +++--- arch/arm/include/asm/armv8/mmu.h | 20 arch/arm/include/asm/global_data.h | 2 -- 3 files changed, 7 insertions(+), 45 deletions(-) -- 2.42.0
