* Re: [Qemu-devel] [PATCH] spapr: Correct compatibility mode setting for hotplugged CPUs
2018-01-04 4:24 [Qemu-devel] [PATCH] spapr: Correct compatibility mode setting for hotplugged CPUs David Gibson
@ 2018-01-04 4:35 ` Alexey Kardashevskiy
2018-01-04 17:47 ` Greg Kurz
2018-01-04 18:02 ` [Qemu-devel] [Qemu-ppc] " Daniel Henrique Barboza
2 siblings, 0 replies; 9+ messages in thread
From: Alexey Kardashevskiy @ 2018-01-04 4:35 UTC (permalink / raw)
To: David Gibson, groug, mdroth, surajjs; +Cc: satheera, qemu-ppc, qemu-devel
On 04/01/18 15:24, David Gibson wrote:
> Currently the pseries machine sets the compatibility mode for the
> guest's cpus in two places: 1) at machine reset and 2) after CAS
> negotiation.
>
> This means that if we set or negotiate a compatiblity mode, then
> hotplug a cpu, the hotplugged cpu doesn't get the right mode set and
> will incorrectly have the full native features.
>
> To correct this, we set the compatibility mode on a cpu when it is
> brought online with the 'start-cpu' RTAS call. Given that we no
> longer need to set the compatibility mode on all CPUs at machine
> reset, so we change that to only set the mode for the boot cpu.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> hw/ppc/spapr.c | 2 +-
> hw/ppc/spapr_rtas.c | 8 ++++++++
> 2 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index e22888ba06..d1acfe8858 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1510,7 +1510,7 @@ static void spapr_machine_reset(void)
> spapr_ovec_cleanup(spapr->ov5_cas);
> spapr->ov5_cas = spapr_ovec_new();
>
> - ppc_set_compat_all(spapr->max_compat_pvr, &error_fatal);
> + ppc_set_compat(first_ppc_cpu, spapr->max_compat_pvr, &error_fatal);
> }
>
> fdt = spapr_build_fdt(spapr, rtas_addr, spapr->rtas_size);
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index 4bb939d3d1..2ed00548c1 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -163,6 +163,7 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
> CPUState *cs = CPU(cpu);
> CPUPPCState *env = &cpu->env;
> PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> + Error *local_err = NULL;
>
> if (!cs->halted) {
> rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> @@ -174,6 +175,13 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
> * new cpu enters */
> kvm_cpu_synchronize_state(cs);
>
> + /* Set compatibility mode to match existing cpus */
> + ppc_set_compat(cpu, POWERPC_CPU(first_cpu)->compat_pvr, &local_err);
> + if (local_err) {
> + rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> + return;
> + }
> +
> env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
>
> /* Enable Power-saving mode Exit Cause exceptions for the new CPU */
>
--
Alexey
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [PATCH] spapr: Correct compatibility mode setting for hotplugged CPUs
2018-01-04 4:24 [Qemu-devel] [PATCH] spapr: Correct compatibility mode setting for hotplugged CPUs David Gibson
2018-01-04 4:35 ` Alexey Kardashevskiy
@ 2018-01-04 17:47 ` Greg Kurz
2018-01-04 21:11 ` Michael Roth
2018-01-05 3:07 ` David Gibson
2018-01-04 18:02 ` [Qemu-devel] [Qemu-ppc] " Daniel Henrique Barboza
2 siblings, 2 replies; 9+ messages in thread
From: Greg Kurz @ 2018-01-04 17:47 UTC (permalink / raw)
To: David Gibson; +Cc: mdroth, surajjs, qemu-ppc, qemu-devel, satheera
On Thu, 4 Jan 2018 15:24:05 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:
> Currently the pseries machine sets the compatibility mode for the
> guest's cpus in two places: 1) at machine reset and 2) after CAS
> negotiation.
>
> This means that if we set or negotiate a compatiblity mode, then
> hotplug a cpu, the hotplugged cpu doesn't get the right mode set and
> will incorrectly have the full native features.
>
> To correct this, we set the compatibility mode on a cpu when it is
> brought online with the 'start-cpu' RTAS call. Given that we no
> longer need to set the compatibility mode on all CPUs at machine
> reset, so we change that to only set the mode for the boot cpu.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> hw/ppc/spapr.c | 2 +-
> hw/ppc/spapr_rtas.c | 8 ++++++++
> 2 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index e22888ba06..d1acfe8858 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1510,7 +1510,7 @@ static void spapr_machine_reset(void)
> spapr_ovec_cleanup(spapr->ov5_cas);
> spapr->ov5_cas = spapr_ovec_new();
>
> - ppc_set_compat_all(spapr->max_compat_pvr, &error_fatal);
> + ppc_set_compat(first_ppc_cpu, spapr->max_compat_pvr, &error_fatal);
> }
>
> fdt = spapr_build_fdt(spapr, rtas_addr, spapr->rtas_size);
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index 4bb939d3d1..2ed00548c1 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -163,6 +163,7 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
> CPUState *cs = CPU(cpu);
> CPUPPCState *env = &cpu->env;
> PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> + Error *local_err = NULL;
>
> if (!cs->halted) {
> rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> @@ -174,6 +175,13 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
> * new cpu enters */
> kvm_cpu_synchronize_state(cs);
>
> + /* Set compatibility mode to match existing cpus */
> + ppc_set_compat(cpu, POWERPC_CPU(first_cpu)->compat_pvr, &local_err);
Is it okay to report a simple HW error to the guest here instead of aborting
like we do with first_cpu at reset time ?
> + if (local_err) {
> + rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> + return;
> + }
> +
> env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
>
> /* Enable Power-saving mode Exit Cause exceptions for the new CPU */
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [PATCH] spapr: Correct compatibility mode setting for hotplugged CPUs
2018-01-04 17:47 ` Greg Kurz
@ 2018-01-04 21:11 ` Michael Roth
2018-01-05 3:37 ` David Gibson
2018-01-05 3:07 ` David Gibson
1 sibling, 1 reply; 9+ messages in thread
From: Michael Roth @ 2018-01-04 21:11 UTC (permalink / raw)
To: David Gibson, Greg Kurz; +Cc: surajjs, qemu-ppc, qemu-devel, satheera
Quoting Greg Kurz (2018-01-04 11:47:18)
> On Thu, 4 Jan 2018 15:24:05 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
>
> > Currently the pseries machine sets the compatibility mode for the
> > guest's cpus in two places: 1) at machine reset and 2) after CAS
> > negotiation.
> >
> > This means that if we set or negotiate a compatiblity mode, then
> > hotplug a cpu, the hotplugged cpu doesn't get the right mode set and
> > will incorrectly have the full native features.
> >
> > To correct this, we set the compatibility mode on a cpu when it is
> > brought online with the 'start-cpu' RTAS call. Given that we no
> > longer need to set the compatibility mode on all CPUs at machine
> > reset, so we change that to only set the mode for the boot cpu.
> >
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> > hw/ppc/spapr.c | 2 +-
> > hw/ppc/spapr_rtas.c | 8 ++++++++
> > 2 files changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index e22888ba06..d1acfe8858 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1510,7 +1510,7 @@ static void spapr_machine_reset(void)
> > spapr_ovec_cleanup(spapr->ov5_cas);
> > spapr->ov5_cas = spapr_ovec_new();
> >
> > - ppc_set_compat_all(spapr->max_compat_pvr, &error_fatal);
> > + ppc_set_compat(first_ppc_cpu, spapr->max_compat_pvr, &error_fatal);
> > }
> >
> > fdt = spapr_build_fdt(spapr, rtas_addr, spapr->rtas_size);
> > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> > index 4bb939d3d1..2ed00548c1 100644
> > --- a/hw/ppc/spapr_rtas.c
> > +++ b/hw/ppc/spapr_rtas.c
> > @@ -163,6 +163,7 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
> > CPUState *cs = CPU(cpu);
> > CPUPPCState *env = &cpu->env;
> > PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> > + Error *local_err = NULL;
> >
> > if (!cs->halted) {
> > rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> > @@ -174,6 +175,13 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
> > * new cpu enters */
> > kvm_cpu_synchronize_state(cs);
> >
> > + /* Set compatibility mode to match existing cpus */
> > + ppc_set_compat(cpu, POWERPC_CPU(first_cpu)->compat_pvr, &local_err);
>
> Is it okay to report a simple HW error to the guest here instead of aborting
> like we do with first_cpu at reset time ?
And if we don't opt for &error_fatal, we need an error_free() below.
>
> > + if (local_err) {
> > + rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> > + return;
> > + }
> > +
> > env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
> >
> > /* Enable Power-saving mode Exit Cause exceptions for the new CPU */
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [PATCH] spapr: Correct compatibility mode setting for hotplugged CPUs
2018-01-04 21:11 ` Michael Roth
@ 2018-01-05 3:37 ` David Gibson
0 siblings, 0 replies; 9+ messages in thread
From: David Gibson @ 2018-01-05 3:37 UTC (permalink / raw)
To: Michael Roth; +Cc: Greg Kurz, surajjs, qemu-ppc, qemu-devel, satheera
[-- Attachment #1: Type: text/plain, Size: 3428 bytes --]
On Thu, Jan 04, 2018 at 03:11:03PM -0600, Michael Roth wrote:
> Quoting Greg Kurz (2018-01-04 11:47:18)
> > On Thu, 4 Jan 2018 15:24:05 +1100
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >
> > > Currently the pseries machine sets the compatibility mode for the
> > > guest's cpus in two places: 1) at machine reset and 2) after CAS
> > > negotiation.
> > >
> > > This means that if we set or negotiate a compatiblity mode, then
> > > hotplug a cpu, the hotplugged cpu doesn't get the right mode set and
> > > will incorrectly have the full native features.
> > >
> > > To correct this, we set the compatibility mode on a cpu when it is
> > > brought online with the 'start-cpu' RTAS call. Given that we no
> > > longer need to set the compatibility mode on all CPUs at machine
> > > reset, so we change that to only set the mode for the boot cpu.
> > >
> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > ---
> > > hw/ppc/spapr.c | 2 +-
> > > hw/ppc/spapr_rtas.c | 8 ++++++++
> > > 2 files changed, 9 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index e22888ba06..d1acfe8858 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -1510,7 +1510,7 @@ static void spapr_machine_reset(void)
> > > spapr_ovec_cleanup(spapr->ov5_cas);
> > > spapr->ov5_cas = spapr_ovec_new();
> > >
> > > - ppc_set_compat_all(spapr->max_compat_pvr, &error_fatal);
> > > + ppc_set_compat(first_ppc_cpu, spapr->max_compat_pvr, &error_fatal);
> > > }
> > >
> > > fdt = spapr_build_fdt(spapr, rtas_addr, spapr->rtas_size);
> > > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> > > index 4bb939d3d1..2ed00548c1 100644
> > > --- a/hw/ppc/spapr_rtas.c
> > > +++ b/hw/ppc/spapr_rtas.c
> > > @@ -163,6 +163,7 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
> > > CPUState *cs = CPU(cpu);
> > > CPUPPCState *env = &cpu->env;
> > > PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> > > + Error *local_err = NULL;
> > >
> > > if (!cs->halted) {
> > > rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> > > @@ -174,6 +175,13 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
> > > * new cpu enters */
> > > kvm_cpu_synchronize_state(cs);
> > >
> > > + /* Set compatibility mode to match existing cpus */
> > > + ppc_set_compat(cpu, POWERPC_CPU(first_cpu)->compat_pvr, &local_err);
> >
> > Is it okay to report a simple HW error to the guest here instead of aborting
> > like we do with first_cpu at reset time ?
>
> And if we don't opt for &error_fatal, we need an error_free() below.
Or better yet an error_report_err() which will display it and free
it. I'll make that change.
>
> >
> > > + if (local_err) {
> > > + rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> > > + return;
> > > + }
> > > +
> > > env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
> > >
> > > /* Enable Power-saving mode Exit Cause exceptions for the new CPU */
> >
>
--
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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] spapr: Correct compatibility mode setting for hotplugged CPUs
2018-01-04 17:47 ` Greg Kurz
2018-01-04 21:11 ` Michael Roth
@ 2018-01-05 3:07 ` David Gibson
2018-01-11 12:24 ` Greg Kurz
1 sibling, 1 reply; 9+ messages in thread
From: David Gibson @ 2018-01-05 3:07 UTC (permalink / raw)
To: Greg Kurz; +Cc: mdroth, surajjs, qemu-ppc, qemu-devel, satheera
[-- Attachment #1: Type: text/plain, Size: 3262 bytes --]
On Thu, Jan 04, 2018 at 06:47:18PM +0100, Greg Kurz wrote:
> On Thu, 4 Jan 2018 15:24:05 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
>
> > Currently the pseries machine sets the compatibility mode for the
> > guest's cpus in two places: 1) at machine reset and 2) after CAS
> > negotiation.
> >
> > This means that if we set or negotiate a compatiblity mode, then
> > hotplug a cpu, the hotplugged cpu doesn't get the right mode set and
> > will incorrectly have the full native features.
> >
> > To correct this, we set the compatibility mode on a cpu when it is
> > brought online with the 'start-cpu' RTAS call. Given that we no
> > longer need to set the compatibility mode on all CPUs at machine
> > reset, so we change that to only set the mode for the boot cpu.
> >
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> > hw/ppc/spapr.c | 2 +-
> > hw/ppc/spapr_rtas.c | 8 ++++++++
> > 2 files changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index e22888ba06..d1acfe8858 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1510,7 +1510,7 @@ static void spapr_machine_reset(void)
> > spapr_ovec_cleanup(spapr->ov5_cas);
> > spapr->ov5_cas = spapr_ovec_new();
> >
> > - ppc_set_compat_all(spapr->max_compat_pvr, &error_fatal);
> > + ppc_set_compat(first_ppc_cpu, spapr->max_compat_pvr, &error_fatal);
> > }
> >
> > fdt = spapr_build_fdt(spapr, rtas_addr, spapr->rtas_size);
> > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> > index 4bb939d3d1..2ed00548c1 100644
> > --- a/hw/ppc/spapr_rtas.c
> > +++ b/hw/ppc/spapr_rtas.c
> > @@ -163,6 +163,7 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
> > CPUState *cs = CPU(cpu);
> > CPUPPCState *env = &cpu->env;
> > PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> > + Error *local_err = NULL;
> >
> > if (!cs->halted) {
> > rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> > @@ -174,6 +175,13 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
> > * new cpu enters */
> > kvm_cpu_synchronize_state(cs);
> >
> > + /* Set compatibility mode to match existing cpus */
> > + ppc_set_compat(cpu, POWERPC_CPU(first_cpu)->compat_pvr, &local_err);
>
> Is it okay to report a simple HW error to the guest here instead of aborting
> like we do with first_cpu at reset time ?
Should be: this happens before we turn the cpu on, so the effect will
be that the guest fails to online the cpu. That seems like a better
failure mode than killing the already running guest.
>
> > + if (local_err) {
> > + rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> > + return;
> > + }
> > +
> > env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
> >
> > /* Enable Power-saving mode Exit Cause exceptions for the new CPU */
>
--
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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [PATCH] spapr: Correct compatibility mode setting for hotplugged CPUs
2018-01-05 3:07 ` David Gibson
@ 2018-01-11 12:24 ` Greg Kurz
2018-01-12 1:47 ` David Gibson
0 siblings, 1 reply; 9+ messages in thread
From: Greg Kurz @ 2018-01-11 12:24 UTC (permalink / raw)
To: David Gibson; +Cc: mdroth, surajjs, qemu-ppc, qemu-devel, satheera
[-- Attachment #1: Type: text/plain, Size: 3565 bytes --]
On Fri, 5 Jan 2018 14:07:29 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:
> On Thu, Jan 04, 2018 at 06:47:18PM +0100, Greg Kurz wrote:
> > On Thu, 4 Jan 2018 15:24:05 +1100
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >
> > > Currently the pseries machine sets the compatibility mode for the
> > > guest's cpus in two places: 1) at machine reset and 2) after CAS
> > > negotiation.
> > >
> > > This means that if we set or negotiate a compatiblity mode, then
> > > hotplug a cpu, the hotplugged cpu doesn't get the right mode set and
> > > will incorrectly have the full native features.
> > >
> > > To correct this, we set the compatibility mode on a cpu when it is
> > > brought online with the 'start-cpu' RTAS call. Given that we no
> > > longer need to set the compatibility mode on all CPUs at machine
> > > reset, so we change that to only set the mode for the boot cpu.
> > >
> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > ---
> > > hw/ppc/spapr.c | 2 +-
> > > hw/ppc/spapr_rtas.c | 8 ++++++++
> > > 2 files changed, 9 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index e22888ba06..d1acfe8858 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -1510,7 +1510,7 @@ static void spapr_machine_reset(void)
> > > spapr_ovec_cleanup(spapr->ov5_cas);
> > > spapr->ov5_cas = spapr_ovec_new();
> > >
> > > - ppc_set_compat_all(spapr->max_compat_pvr, &error_fatal);
> > > + ppc_set_compat(first_ppc_cpu, spapr->max_compat_pvr, &error_fatal);
> > > }
> > >
> > > fdt = spapr_build_fdt(spapr, rtas_addr, spapr->rtas_size);
> > > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> > > index 4bb939d3d1..2ed00548c1 100644
> > > --- a/hw/ppc/spapr_rtas.c
> > > +++ b/hw/ppc/spapr_rtas.c
> > > @@ -163,6 +163,7 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
> > > CPUState *cs = CPU(cpu);
> > > CPUPPCState *env = &cpu->env;
> > > PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> > > + Error *local_err = NULL;
> > >
> > > if (!cs->halted) {
> > > rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> > > @@ -174,6 +175,13 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
> > > * new cpu enters */
> > > kvm_cpu_synchronize_state(cs);
> > >
> > > + /* Set compatibility mode to match existing cpus */
> > > + ppc_set_compat(cpu, POWERPC_CPU(first_cpu)->compat_pvr, &local_err);
> >
> > Is it okay to report a simple HW error to the guest here instead of aborting
> > like we do with first_cpu at reset time ?
>
> Should be: this happens before we turn the cpu on, so the effect will
> be that the guest fails to online the cpu. That seems like a better
> failure mode than killing the already running guest.
>
Of course, I generally agree with the "better failure mode". My point was
just that if we managed to set the compat mode with the first cpu but we
fail to propagate the same compat mode to subsequent cpus, then this is
a bug in QEMU or KVM.
> >
> > > + if (local_err) {
> > > + rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> > > + return;
> > > + }
> > > +
> > > env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
> > >
> > > /* Enable Power-saving mode Exit Cause exceptions for the new CPU */
> >
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [PATCH] spapr: Correct compatibility mode setting for hotplugged CPUs
2018-01-11 12:24 ` Greg Kurz
@ 2018-01-12 1:47 ` David Gibson
0 siblings, 0 replies; 9+ messages in thread
From: David Gibson @ 2018-01-12 1:47 UTC (permalink / raw)
To: Greg Kurz; +Cc: mdroth, surajjs, qemu-ppc, qemu-devel, satheera
[-- Attachment #1: Type: text/plain, Size: 3796 bytes --]
On Thu, Jan 11, 2018 at 01:24:30PM +0100, Greg Kurz wrote:
> On Fri, 5 Jan 2018 14:07:29 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
>
> > On Thu, Jan 04, 2018 at 06:47:18PM +0100, Greg Kurz wrote:
> > > On Thu, 4 Jan 2018 15:24:05 +1100
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >
> > > > Currently the pseries machine sets the compatibility mode for the
> > > > guest's cpus in two places: 1) at machine reset and 2) after CAS
> > > > negotiation.
> > > >
> > > > This means that if we set or negotiate a compatiblity mode, then
> > > > hotplug a cpu, the hotplugged cpu doesn't get the right mode set and
> > > > will incorrectly have the full native features.
> > > >
> > > > To correct this, we set the compatibility mode on a cpu when it is
> > > > brought online with the 'start-cpu' RTAS call. Given that we no
> > > > longer need to set the compatibility mode on all CPUs at machine
> > > > reset, so we change that to only set the mode for the boot cpu.
> > > >
> > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > > ---
> > > > hw/ppc/spapr.c | 2 +-
> > > > hw/ppc/spapr_rtas.c | 8 ++++++++
> > > > 2 files changed, 9 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > index e22888ba06..d1acfe8858 100644
> > > > --- a/hw/ppc/spapr.c
> > > > +++ b/hw/ppc/spapr.c
> > > > @@ -1510,7 +1510,7 @@ static void spapr_machine_reset(void)
> > > > spapr_ovec_cleanup(spapr->ov5_cas);
> > > > spapr->ov5_cas = spapr_ovec_new();
> > > >
> > > > - ppc_set_compat_all(spapr->max_compat_pvr, &error_fatal);
> > > > + ppc_set_compat(first_ppc_cpu, spapr->max_compat_pvr, &error_fatal);
> > > > }
> > > >
> > > > fdt = spapr_build_fdt(spapr, rtas_addr, spapr->rtas_size);
> > > > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> > > > index 4bb939d3d1..2ed00548c1 100644
> > > > --- a/hw/ppc/spapr_rtas.c
> > > > +++ b/hw/ppc/spapr_rtas.c
> > > > @@ -163,6 +163,7 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
> > > > CPUState *cs = CPU(cpu);
> > > > CPUPPCState *env = &cpu->env;
> > > > PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> > > > + Error *local_err = NULL;
> > > >
> > > > if (!cs->halted) {
> > > > rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> > > > @@ -174,6 +175,13 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
> > > > * new cpu enters */
> > > > kvm_cpu_synchronize_state(cs);
> > > >
> > > > + /* Set compatibility mode to match existing cpus */
> > > > + ppc_set_compat(cpu, POWERPC_CPU(first_cpu)->compat_pvr, &local_err);
> > >
> > > Is it okay to report a simple HW error to the guest here instead of aborting
> > > like we do with first_cpu at reset time ?
> >
> > Should be: this happens before we turn the cpu on, so the effect will
> > be that the guest fails to online the cpu. That seems like a better
> > failure mode than killing the already running guest.
> >
>
> Of course, I generally agree with the "better failure mode". My point was
> just that if we managed to set the compat mode with the first cpu but we
> fail to propagate the same compat mode to subsequent cpus, then this is
> a bug in QEMU or KVM.
Ah, I see your point. Yes, I guess that's true. Nonetheless, I think
the better failure mode is still a better idea, even if something goes
wrong elsewhere.
--
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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PATCH] spapr: Correct compatibility mode setting for hotplugged CPUs
2018-01-04 4:24 [Qemu-devel] [PATCH] spapr: Correct compatibility mode setting for hotplugged CPUs David Gibson
2018-01-04 4:35 ` Alexey Kardashevskiy
2018-01-04 17:47 ` Greg Kurz
@ 2018-01-04 18:02 ` Daniel Henrique Barboza
2 siblings, 0 replies; 9+ messages in thread
From: Daniel Henrique Barboza @ 2018-01-04 18:02 UTC (permalink / raw)
To: David Gibson, groug, mdroth, surajjs; +Cc: satheera, qemu-ppc, qemu-devel
On 01/04/2018 02:24 AM, David Gibson wrote:
> Currently the pseries machine sets the compatibility mode for the
> guest's cpus in two places: 1) at machine reset and 2) after CAS
> negotiation.
>
> This means that if we set or negotiate a compatiblity mode, then
s/compatiblity/compatibility
> hotplug a cpu, the hotplugged cpu doesn't get the right mode set and
> will incorrectly have the full native features.
>
> To correct this, we set the compatibility mode on a cpu when it is
> brought online with the 'start-cpu' RTAS call. Given that we no
> longer need to set the compatibility mode on all CPUs at machine
> reset, so we change that to only set the mode for the boot cpu.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
Reviewed-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>
> hw/ppc/spapr.c | 2 +-
> hw/ppc/spapr_rtas.c | 8 ++++++++
> 2 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index e22888ba06..d1acfe8858 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1510,7 +1510,7 @@ static void spapr_machine_reset(void)
> spapr_ovec_cleanup(spapr->ov5_cas);
> spapr->ov5_cas = spapr_ovec_new();
>
> - ppc_set_compat_all(spapr->max_compat_pvr, &error_fatal);
> + ppc_set_compat(first_ppc_cpu, spapr->max_compat_pvr, &error_fatal);
> }
>
> fdt = spapr_build_fdt(spapr, rtas_addr, spapr->rtas_size);
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index 4bb939d3d1..2ed00548c1 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -163,6 +163,7 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
> CPUState *cs = CPU(cpu);
> CPUPPCState *env = &cpu->env;
> PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> + Error *local_err = NULL;
>
> if (!cs->halted) {
> rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> @@ -174,6 +175,13 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
> * new cpu enters */
> kvm_cpu_synchronize_state(cs);
>
> + /* Set compatibility mode to match existing cpus */
> + ppc_set_compat(cpu, POWERPC_CPU(first_cpu)->compat_pvr, &local_err);
> + if (local_err) {
> + rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> + return;
> + }
> +
> env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
>
> /* Enable Power-saving mode Exit Cause exceptions for the new CPU */
^ permalink raw reply [flat|nested] 9+ messages in thread