[PATCH 0/3] Revert HAFDBS changes

[Pierre-Clément Tosi]

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 <ptosi at google.com> 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/063a16d559dc9cb327c9459803005006@kernel.org
> [2] https://lore.kernel.org/r/CAFOYHZC_Dveax85n0fLr5BFyZcLqsvUssn=J0oHyvN75bTaCUw@mail.gmail.com
> 
> -- 
> Without deviation from the norm, progress is not possible.

Thanks,

-- 
Pierre