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 <npig...@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.
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.
Thanks,
Nick
