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;
+ }
+
if (ppc_hash64_has(cpu, PPC_HASH64_CI_LARGEPAGE)) {
/*
* Note: we keep CI large pages off by default because a 64K capable
--
2.42.0On 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
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.
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
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
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
© 2016 - 2024 Red Hat, Inc.