Re: [PATCH 2/8] ppc/spapr|pnv: Remove SAO from pa-features when running MTTCG

["Nicholas Piggin" <npiggin@gmail.com>]

On Thu Jan 25, 2024 at 1:11 PM AEST, David Gibson wrote:
> On Tue, Jan 23, 2024 at 11:57:56AM +1000, Nicholas Piggin wrote:
> > On Fri Jan 19, 2024 at 10:23 AM AEST, David Gibson wrote:
> > > On Fri, Jan 19, 2024 at 12:09:36AM +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. There is a complication with spapr migration that is
> > > > addressed with comments, it is not a new problem here.
> > > > 
> > > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > > > ---
> > > >  hw/ppc/pnv.c   |  5 +++++
> > > >  hw/ppc/spapr.c | 15 +++++++++++++++
> > > >  2 files changed, 20 insertions(+)
> > > > 
> > > > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> > > > index b949398689..4969fbdb05 100644
> > > > --- a/hw/ppc/pnv.c
> > > > +++ b/hw/ppc/pnv.c
> > > > @@ -158,6 +158,11 @@ static void pnv_dt_core(PnvChip *chip, PnvCore *pc, void *fdt)
> > > >      char *nodename;
> > > >      int cpus_offset = get_cpus_node(fdt);
> > > >  
> > > > +    if (qemu_tcg_mttcg_enabled()) {
> > > > +        /* SSO (SAO) ordering is not supported under MTTCG. */
> > > > +        pa_features[4 + 2] &= ~0x80;
> > > > +    }
> > > > +
> > > >      nodename = g_strdup_printf("%s@%x", dc->fw_name, pc->pir);
> > > >      offset = fdt_add_subnode(fdt, cpus_offset, nodename);
> > > >      _FDT(offset);
> > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > index 021b1a00e1..1c79d5670d 100644
> > > > --- a/hw/ppc/spapr.c
> > > > +++ b/hw/ppc/spapr.c
> > > > @@ -284,6 +284,21 @@ static void spapr_dt_pa_features(SpaprMachineState *spapr,
> > > >          return;
> > > >      }
> > > >  
> > > > +    if (qemu_tcg_mttcg_enabled()) {
> > > > +        /*
> > > > +         * SSO (SAO) ordering is not supported under MTTCG, so disable it.
> > > > +         * There is no cap for this, so there is a migration bug here.
> > > > +         * However don't disable it entirely, to allow it to be used under
> > > > +         * KVM. This is a minor concern because:
> > > > +         * - SAO is an obscure an rarely (if ever) used feature.
> > > > +         * - SAO is removed from POWER10 / v3.1, so there is already a
> > > > +         *   migration problem today.
> > > > +         * - Linux does not test this pa-features bit today anyway, so it's
> > > > +         *   academic.
> > > > +         */
> > > > +        pa_features[4 + 2] &= ~0x80;
> > >
> > > Oof.. I see the reasoning but modifying guest visible parameters based
> > > on host capabilities without a cap really worries me nonetheless.
> > 
> > Yeah :( It's not a new problem, but changing it based on host
> > does make it look uglier I guess.
>
> It's not really about whether it looks uglier, it's the fact that any
> dependency of guest visible aspects of the VM on host properties is a
> potential landmine for migration.
>
> The qemu migration model is - pretty fundamentally - that the VM
> should look and behave, from the point of view of the guest, the same
> before and after migration.  If the behaviour of the VM changes based
> on host properties it breaks that assumption, and it does so in a way
> that the user can't control or even easily predict.  Tools such as
> libvirt, or even qemu itself, can't verify that the migration is valid
> if there are effectively invisible parameters to the VM configuration
> that come from the host instead of the command line.

I agree, you did manage to drill this lesson in.

What I meant was that -- today we have a problem where a target can
run on a host with buggy implementation, whether boot or migration.
This patch addresses the boot case. Migration case is still broken,
arguably the situation is still improved. Anyway...

> > Other option could be to just disable it always. I don't mind
> > but someone did mention experimenting with it when I asked
> > about removing support from Linux. They could still test with
> > bare metal, and if ever started actually being used then we
> > could add a cap for it.
>
> I think that's a better idea.

Alright we'll go that way.

Thanks,
Nick