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.lup...@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
signature.asc
Description: PGP signature
