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

[David Gibson]

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.

> +    }
> +
>      if (ppc_hash64_has(cpu, PPC_HASH64_CI_LARGEPAGE)) {
>          /*
>           * Note: we keep CI large pages off by default because a 64K capable

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