| Message ID | 20240118140942.164319-3-npiggin@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | ppc: Update targets for Power machines (spapr and pnv) | expand |
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
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.
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
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
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(+)