Re: [PATCH v3] target/ppc: fix Hash64 MMU update of PTE bit R

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2715 bytes --]

On Sat, Nov 27, 2021 at 10:05:18AM +0100, Cédric le Goater wrote:
> On 11/27/21 04:00, David Gibson wrote:
> > On Fri, Nov 26, 2021 at 04:39:40PM -0300, Leandro Lupori wrote:
> > > When updating the R bit of a PTE, the Hash64 MMU was using a wrong byte
> > > offset, causing the first byte of the adjacent PTE to be corrupted.
> > > This caused a panic when booting FreeBSD, using the Hash MMU.
> > > 
> > > Fixes: a2dd4e83e76b ("ppc/hash64: Rework R and C bit updates")
> > > Signed-off-by: Leandro Lupori <leandro.lupori@eldorado.org.br>
> > > ---
> > > Changes from v2:
> > > - Add new defines for the byte offset of PTE bit C and
> > >    HASH_PTE_SIZE_64 / 2 (pte1)
> > > - Use new defines in hash64 and spapr code
> > > ---
> > >   hw/ppc/spapr.c          | 8 ++++----
> > >   hw/ppc/spapr_softmmu.c  | 2 +-
> > >   target/ppc/mmu-hash64.c | 4 ++--
> > >   target/ppc/mmu-hash64.h | 5 +++++
> > >   4 files changed, 12 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 163c90388a..8ebf85bad8 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -1414,7 +1414,7 @@ void spapr_store_hpte(PowerPCCPU *cpu, hwaddr ptex,
> > >           kvmppc_write_hpte(ptex, pte0, pte1);
> > >       } else {
> > >           if (pte0 & HPTE64_V_VALID) {
> > > -            stq_p(spapr->htab + offset + HASH_PTE_SIZE_64 / 2, pte1);
> > > +            stq_p(spapr->htab + offset + HPTE64_R_BYTE_OFFSET, pte1);
> > 
> > Urgh.. so, initially I thought this was wrong because I was confusing
> > HPTE64_R_BYTE_OFFSET with HPTE64_R_R_BYTE_OFFSET.  I doubt I'd be the
> > only one.
> > 
> > Calling something a BYTE_OFFSET then doing an stq to it is pretty
> > misleading I think.  WORD1_OFFSET or R_WORD_OFFSET might be better?
> 
> How about (inspired from XIVE) :
> 
>  #define HPTE64_W1    (HASH_PTE_SIZE_64 / 2)
>  #define HPTE64_W1_R  14 // or HPTE64_W1 + 6
>  #define HPTE64_W1_C  15 // or HPTE64_W1 + 7

Good enough.

> Really these should be bitfields describing both words like we have
> for XIVE. See include/hw/ppc/xive_regs.h. Is there a reason why ?

I don't really know what you're getting at here.

> > Or you could change these writebacks to byte writes, as powernv has
> > already been changed.
> 
> That's a bigger change. It depends if we want this fix for 6.2 or
> 7.0.

Good point; this is a nasty memory corruption bug, so we definitely
want to fix it for 6.2.  Further cleanups can wait.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]