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

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

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

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.

> 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.

-- 
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 --]