Re: [PATCH v2 03/10] ppc/spapr|pnv: Remove SAO from pa-features

[Nicholas Piggin]

On Thu Mar 14, 2024 at 12:34 PM AEST, David Gibson wrote:
> On Tue, Mar 12, 2024 at 11:14:12PM +1000, Nicholas Piggin wrote:
> > SAO is a page table attribute that strengthens the memory ordering of
> > accesses. QEMU with MTTCG does not implement this, so clear it in
> > ibm,pa-features. This is an obscure feature that has been removed from
> > POWER10 ISA v3.1, there isn't much concern with removing it.
> > 
> > Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>
> Usually altering a user visible feature like this without versioning
> would be a no-no.  However, I think it's probably ok here: AFAICT the
> feature was basically never used, it didn't work in some cases anyway,
> and it's now gone away.

Thanks David, I appreciate you keeping an eye on these kinds of
compatibility issues from time to time.

Yeah, we established that it doesn't really matter for Linux code out
there, but you thought it's ugly to change this based on the host
configuration for pseries machines.

And if this change does cause problems, it's quite possible that
configuration was broken anyway, so that's arguably preferable to
continuing to advertise a broken or at least non-migratable feature.

Thanks,
Nick

>
> > ---
> >  hw/ppc/pnv.c   |  2 +-
> >  hw/ppc/spapr.c | 14 ++++++++++----
> >  2 files changed, 11 insertions(+), 5 deletions(-)
> > 
> > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> > index 0b47b92baa..aa9786e970 100644
> > --- a/hw/ppc/pnv.c
> > +++ b/hw/ppc/pnv.c
> > @@ -150,7 +150,7 @@ static void pnv_dt_core(PnvChip *chip, PnvCore *pc, 
> > void *fdt)
> >      uint32_t page_sizes_prop[64];
> >      size_t page_sizes_prop_size;
> >      const uint8_t pa_features[] = { 24, 0,
> > -                                    0xf6, 0x3f, 0xc7, 0xc0, 0x80, 0xf0,
> > +                                    0xf6, 0x3f, 0xc7, 0xc0, 0x00, 0xf0,
> >                                      0x80, 0x00, 0x00, 0x00, 0x00, 0x00,
> >                                      0x00, 0x00, 0x00, 0x00, 0x80, 0x00,
> >                                      0x80, 0x00, 0x80, 0x00, 0x80, 0x00 };
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 55263f0815..3108d7c532 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -233,17 +233,23 @@ static void spapr_dt_pa_features(SpaprMachineState 
> > *spapr,
> >                                   PowerPCCPU *cpu,
> >                                   void *fdt, int offset)
> >  {
> > +    /*
> > +     * SSO (SAO) ordering is supported on KVM and thread=single hosts,
> > +     * but not MTTCG, so disable it. To advertise it, a cap would have
> > +     * to be added, or support implemented for MTTCG.
> > +     */
> > +
> >      uint8_t pa_features_206[] = { 6, 0,
> > -        0xf6, 0x1f, 0xc7, 0x00, 0x80, 0xc0 };
> > +        0xf6, 0x1f, 0xc7, 0x00, 0x00, 0xc0 };
> >      uint8_t pa_features_207[] = { 24, 0,
> > -        0xf6, 0x1f, 0xc7, 0xc0, 0x80, 0xf0,
> > +        0xf6, 0x1f, 0xc7, 0xc0, 0x00, 0xf0,
> >          0x80, 0x00, 0x00, 0x00, 0x00, 0x00,
> >          0x00, 0x00, 0x00, 0x00, 0x80, 0x00,
> >          0x80, 0x00, 0x80, 0x00, 0x00, 0x00 };
> >      uint8_t pa_features_300[] = { 66, 0,
> >          /* 0: MMU|FPU|SLB|RUN|DABR|NX, 1: fri[nzpm]|DABRX|SPRG3|SLB0|PP110 
> > */
> > -        /* 2: VPM|DS205|PPR|DS202|DS206, 3: LSD|URG, SSO, 5: 
> > LE|CFAR|EB|LSQ */
> > -        0xf6, 0x1f, 0xc7, 0xc0, 0x80, 0xf0, /* 0 - 5 */
> > +        /* 2: VPM|DS205|PPR|DS202|DS206, 3: LSD|URG, 5: LE|CFAR|EB|LSQ */
> > +        0xf6, 0x1f, 0xc7, 0xc0, 0x00, 0xf0, /* 0 - 5 */
> >          /* 6: DS207 */
> >          0x80, 0x00, 0x00, 0x00, 0x00, 0x00, /* 6 - 11 */
> >          /* 16: Vector */