Re: [Qemu-devel] [PATCH 6/6] spapr: fix migration of ICP objects from/to older QEMU
On Wed, May 17, 2017 at 10:33:44PM +0200, Greg Kurz wrote:
> On Wed, 17 May 2017 14:18:16 +1000
> David Gibson wrote:
>
> > On Mon, May 15, 2017 at 06:11:27PM +0200, Cédric Le Goater wrote:
> > > >>> +int smt = kvmppc_smt_threads();
> > > >>> +int nr_servers = DIV_ROUND_UP(max_cpus * smt, smp_threads);
> > > >>
> > > >> may be we should reintroduce nr_servers at the machine level ?
> > > >>
> > > >
> > > > I had reintroduced it but then I realized it was only used in this
> > > > function.
> > >
> > > nr_servers is also used when the device tree is populated with the
> > > interrupt controller nodes. No big deal.
> >
> > Which is guest visible, so we should really make that stay the same
> > for older machine types. I'd like to avoid re-introducing nr_servers
> > as a property if we can, but maybe we can't.
> >
>
> Yes we can :) or at least maybe, if you can shed light on a guest
> visible change introduced by this commit in 2.8:
>
> commit 9b9a19080a6e548b91420ce7925f2ac81ef63ae8
> Author: David Gibson
> Date: Thu Oct 20 16:07:56 2016 +1100
>
> pseries: Move construction of /interrupt-controller fdt node
>
>
> It changes the "ibm,interrupt-server-ranges" property in the device
> tree from
>
> {0, cpu_to_be32(max_cpus)}
>
> to
>
> {0, cpu_to_be32(xics->nr_servers)}
>
> ie, {0, cpu_to_be32(DIV_ROUND_UP(max_cpus * smt, smp_threads))}
>
> And indeed, if I start QEMU with
>
> -smp cores=2,threads=4,maxcpus=16 -machine type=pseries-2.7,accel=kvm
>
> the following is exposed to the guest with 2.7:
>
> ibm,interrupt-server-ranges
> 0010
>
> and with 2.8 we get:
>
> ibm,interrupt-server-ranges
> 0020
>
> LoPAPR B.6.9.1.1 says that the range (ie, the second number) "shall be the
> number of contiguous server#s supported by the unit (this also corresponds
> to the number of “reg” entries)". I'm inclined to think this maps to max_cpus
> but I may be wrong... any clues ?
So, yes, it's a guest visible change in some configurations, but in
those configurations the previous behaviour was sufficiently broken
that I think that's preferable to not changing it.
Basically the ibm,interrupt-server-ranges property gives the total
range of valid server IDs. The server IDs assigned to each thread are
based on the dt_id, and are spaced by the number of host threads, not
the number of guest threads. That means that with 2.7, the last cpus
would have advertised ibm,ppc-interrupt-server#s properties with
values outside the range given by ibm,interrupt-server-ranages, which
is definitely wrong.
--
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
Re: [Qemu-devel] [PATCH 6/6] spapr: fix migration of ICP objects from/to older QEMU
On Wed, 17 May 2017 14:18:16 +1000
David Gibson wrote:
> On Mon, May 15, 2017 at 06:11:27PM +0200, Cédric Le Goater wrote:
> > >>> +int smt = kvmppc_smt_threads();
> > >>> +int nr_servers = DIV_ROUND_UP(max_cpus * smt, smp_threads);
> > >>
> > >> may be we should reintroduce nr_servers at the machine level ?
> > >>
> > >
> > > I had reintroduced it but then I realized it was only used in this
> > > function.
> >
> > nr_servers is also used when the device tree is populated with the
> > interrupt controller nodes. No big deal.
>
> Which is guest visible, so we should really make that stay the same
> for older machine types. I'd like to avoid re-introducing nr_servers
> as a property if we can, but maybe we can't.
>
Yes we can :) or at least maybe, if you can shed light on a guest
visible change introduced by this commit in 2.8:
commit 9b9a19080a6e548b91420ce7925f2ac81ef63ae8
Author: David Gibson
Date: Thu Oct 20 16:07:56 2016 +1100
pseries: Move construction of /interrupt-controller fdt node
It changes the "ibm,interrupt-server-ranges" property in the device
tree from
{0, cpu_to_be32(max_cpus)}
to
{0, cpu_to_be32(xics->nr_servers)}
ie, {0, cpu_to_be32(DIV_ROUND_UP(max_cpus * smt, smp_threads))}
And indeed, if I start QEMU with
-smp cores=2,threads=4,maxcpus=16 -machine type=pseries-2.7,accel=kvm
the following is exposed to the guest with 2.7:
ibm,interrupt-server-ranges
0010
and with 2.8 we get:
ibm,interrupt-server-ranges
0020
LoPAPR B.6.9.1.1 says that the range (ie, the second number) "shall be the
number of contiguous server#s supported by the unit (this also corresponds
to the number of “reg” entries)". I'm inclined to think this maps to max_cpus
but I may be wrong... any clues ?
Cheers,
--
Greg
pgpIaBt1hlddS.pgp
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 6/6] spapr: fix migration of ICP objects from/to older QEMU
On Mon, May 15, 2017 at 02:22:32PM +0200, Cédric Le Goater wrote:
> On 05/15/2017 01:40 PM, Greg Kurz wrote:
> > Commit 5bc8d26de20c ("spapr: allocate the ICPState object from under
> > sPAPRCPUCore") moved ICP objects from the machine to CPU cores. This
> > is an improvement since we no longer allocate ICP objects that will
> > never be used. But it has the side-effect of breaking migration of
> > older machine types from older QEMU versions.
> >
> > This patch introduces a compat flag in the sPAPR machine class so
> > that all pseries machine up to 2.9 go on with the previous behavior
> > of pre-allocating ICP objects.
>
> I think this is a quite elegant way to a handle the migration
> regression. Thanks for taking care of it.
>
> Have you tried to simply reparent the ICPs objects to OBJECT(spapr)
> instead of the OBJECT(cpu) ?
I actually kind of hate changing the QOM tree structure based on
machine type compatibility. Unfortunately, since we're matching up
the migration state based (essentially) on QOM path, I don't see any
easy alternative. I really wish there was a mechanism for defining
"alias paths" or something to handle this kind of migration
compatibility shim.
> See some minor comments below.
>
> > While here, we also ensure that object_property_add_child() errors cause
> > QEMU to abort for newer machines.
> >
> > Signed-off-by: Greg Kurz
> > ---
> > hw/ppc/spapr.c | 36
> > hw/ppc/spapr_cpu_core.c | 28
> > include/hw/ppc/spapr.h |2 ++
> > 3 files changed, 58 insertions(+), 8 deletions(-)
> >
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index c53989bb10b1..ab3683bcd677 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -126,6 +126,7 @@ error:
> > static void xics_system_init(MachineState *machine, int nr_irqs, Error
> > **errp)
> > {
> > sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
> > +sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> > Error *local_err = NULL;
> >
> > if (kvm_enabled()) {
> > @@ -151,6 +152,38 @@ static void xics_system_init(MachineState *machine,
> > int nr_irqs, Error **errp)
> >&local_err);
> > }
> >
> > +if (!spapr->ics) {
> > +goto out;
> > +}
> > +
> > +if (smc->must_pre_allocate_icps) {
>
> I am not sure I like 'must', I think 'pre_allocate_icps' should be enough ?
> or simply 'allocate_legacy_icps' ?
I'd actually prefer to make it explicit that this is a migration
compatibility shim and call it something like
'pre_2_10_icp_allocation'.
> > +int smt = kvmppc_smt_threads();
> > +int nr_servers = DIV_ROUND_UP(max_cpus * smt, smp_threads);
>
> may be we should reintroduce nr_servers at the machine level ?
>
> > +int i;
> > +
> > +spapr->legacy_icps = g_malloc0(nr_servers * sizeof(ICPState));
This isn't technically safe, although you'll probably get away with
it. spapr->icp_type is parameterized, which means it could be a
sub-class with a larger state structure than base ICPState.
> > +for (i = 0; i < nr_servers; i++) {
> > +void* obj = &spapr->legacy_icps[i];
>
> 'void *'
>
> > +
> > +object_initialize(obj, sizeof(ICPState), spapr->icp_type);
> > +object_property_add_child(OBJECT(spapr), "icp[*]", obj,
> > + &error_abort);
>
> David does not like the "icp[*]" syntax.
>
> > +object_unref(obj);
> > +object_property_add_const_link(obj, "xics", OBJECT(spapr),
> > + &error_abort);
> > +object_property_set_bool(obj, true, "realized", &local_err);
> > +if (local_err) {
> > +while (i--) {
> > +object_unparent(obj);
> > +}
> > +g_free(spapr->legacy_icps);
> > +break;
> > +}
> > +}
> > +}
> > +
> > +out:
> > error_propagate(errp, local_err);
> > }
> >
> > @@ -3256,8 +3289,11 @@ static void
> > spapr_machine_2_9_instance_options(MachineState *machine)
> >
> > static void spapr_machine_2_9_class_options(MachineClass *mc)
> > {
> > +sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
> > +
> > spapr_machine_2_10_class_options(mc);
> > SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_9);
> > +smc->must_pre_allocate_icps = true;
> > }
> >
> > DEFINE_SPAPR_MACHINE(2_9, "2.9", false);
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index 63d160f7e010..5476647efa06 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -119,6 +119,7 @@ static void spapr_cpu_core_unrealizefn(DeviceState
> > *dev, Error **errp)
> > size_t size = object_type_get_instance_size(typename);
> > CPUCore *cc = CPU_CORE(dev);
> > int i;
> > +sPAPRMachineState *spapr = SPAPR_MACHINE
Re: [Qemu-devel] [PATCH 6/6] spapr: fix migration of ICP objects from/to older QEMU
On Mon, May 15, 2017 at 06:20:06PM +0200, Greg Kurz wrote:
> On Mon, 15 May 2017 18:09:04 +0200
> Cédric Le Goater wrote:
>
> > On 05/15/2017 03:16 PM, Greg Kurz wrote:
> > > On Mon, 15 May 2017 14:22:32 +0200
> > > Cédric Le Goater wrote:
> > >
> > >> On 05/15/2017 01:40 PM, Greg Kurz wrote:
> > >>> Commit 5bc8d26de20c ("spapr: allocate the ICPState object from under
> > >>> sPAPRCPUCore") moved ICP objects from the machine to CPU cores. This
> > >>> is an improvement since we no longer allocate ICP objects that will
> > >>> never be used. But it has the side-effect of breaking migration of
> > >>> older machine types from older QEMU versions.
> > >>>
> > >>> This patch introduces a compat flag in the sPAPR machine class so
> > >>> that all pseries machine up to 2.9 go on with the previous behavior
> > >>> of pre-allocating ICP objects.
> > >>
> > >> I think this is a quite elegant way to a handle the migration
> > >> regression. Thanks for taking care of it.
> > >>
> > >> Have you tried to simply reparent the ICPs objects to OBJECT(spapr)
> > >> instead of the OBJECT(cpu) ?
> > >>
> > >
> > > Do you mean to reparent unconditionally to OBJECT(spapr) for all
> > > machine versions ?
> >
> > only in the case of smc->must_pre_allocate_icps
> >
> > > I'm not sure this would be beneficial, but I might be missing
> > > something...
> >
> > I think that we would not need to allocate the legacy_icps array.
> > Parenting the icp object to the spapr machine should be enough.
> > I might be wrong. my expertise on the migration stream is very
> > basic.
> >
>
> I don't think this would work because an older QEMU would still
> send state for objects that don't exist in the destination.
Right. We could create "dummy" objects that receive the ICP data,
then discard it. But it's probably more trouble than it's worty.
--
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
Re: [Qemu-devel] [PATCH 6/6] spapr: fix migration of ICP objects from/to older QEMU
On Mon, May 15, 2017 at 06:11:27PM +0200, Cédric Le Goater wrote: > >>> +int smt = kvmppc_smt_threads(); > >>> +int nr_servers = DIV_ROUND_UP(max_cpus * smt, smp_threads); > >> > >> may be we should reintroduce nr_servers at the machine level ? > >> > > > > I had reintroduced it but then I realized it was only used in this > > function. > > nr_servers is also used when the device tree is populated with the > interrupt controller nodes. No big deal. Which is guest visible, so we should really make that stay the same for older machine types. I'd like to avoid re-introducing nr_servers as a property if we can, but maybe we can't. -- 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
Re: [Qemu-devel] [PATCH 6/6] spapr: fix migration of ICP objects from/to older QEMU
On Mon, May 15, 2017 at 03:16:02PM +0200, Greg Kurz wrote: > On Mon, 15 May 2017 14:22:32 +0200 > Cédric Le Goater wrote: > > > On 05/15/2017 01:40 PM, Greg Kurz wrote: [snip] > > > + > > > +object_initialize(obj, sizeof(ICPState), spapr->icp_type); > > > +object_property_add_child(OBJECT(spapr), "icp[*]", obj, > > > + &error_abort); > > > > David does not like the "icp[*]" syntax. > > > > Ah... I wasn't aware of that. But I agree that I should probably create > the object names based on 'i', rather than relying on the more complex > logic in object_property_add(). Right, the non-obviousness of what the indicies will end up being is exactly why I dislike the "[*]" syntax. Especially here, where it's clearly important that the QOM paths end up exactly like in older qemu versions, I think it's better to be explicit. -- 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
Re: [Qemu-devel] [PATCH 6/6] spapr: fix migration of ICP objects from/to older QEMU
On Mon, 15 May 2017 18:11:27 +0200 Cédric Le Goater wrote: > >>> +int smt = kvmppc_smt_threads(); > >>> +int nr_servers = DIV_ROUND_UP(max_cpus * smt, smp_threads); > >> > >> may be we should reintroduce nr_servers at the machine level ? > >> > > > > I had reintroduced it but then I realized it was only used in this > > function. > > nr_servers is also used when the device tree is populated with the > interrupt controller nodes. No big deal. > Ah yes you're right. Maybe this can be factored out in a followup patch. > C. > pgpcr5oSf6V7q.pgp Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 6/6] spapr: fix migration of ICP objects from/to older QEMU
On Mon, 15 May 2017 18:09:04 +0200
Cédric Le Goater wrote:
> On 05/15/2017 03:16 PM, Greg Kurz wrote:
> > On Mon, 15 May 2017 14:22:32 +0200
> > Cédric Le Goater wrote:
> >
> >> On 05/15/2017 01:40 PM, Greg Kurz wrote:
> >>> Commit 5bc8d26de20c ("spapr: allocate the ICPState object from under
> >>> sPAPRCPUCore") moved ICP objects from the machine to CPU cores. This
> >>> is an improvement since we no longer allocate ICP objects that will
> >>> never be used. But it has the side-effect of breaking migration of
> >>> older machine types from older QEMU versions.
> >>>
> >>> This patch introduces a compat flag in the sPAPR machine class so
> >>> that all pseries machine up to 2.9 go on with the previous behavior
> >>> of pre-allocating ICP objects.
> >>
> >> I think this is a quite elegant way to a handle the migration
> >> regression. Thanks for taking care of it.
> >>
> >> Have you tried to simply reparent the ICPs objects to OBJECT(spapr)
> >> instead of the OBJECT(cpu) ?
> >>
> >
> > Do you mean to reparent unconditionally to OBJECT(spapr) for all
> > machine versions ?
>
> only in the case of smc->must_pre_allocate_icps
>
> > I'm not sure this would be beneficial, but I might be missing
> > something...
>
> I think that we would not need to allocate the legacy_icps array.
> Parenting the icp object to the spapr machine should be enough.
> I might be wrong. my expertise on the migration stream is very
> basic.
>
I don't think this would work because an older QEMU would still
send state for objects that don't exist in the destination.
> C.
>
pgpQEvqVArg1Y.pgp
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 6/6] spapr: fix migration of ICP objects from/to older QEMU
>>> +int smt = kvmppc_smt_threads(); >>> +int nr_servers = DIV_ROUND_UP(max_cpus * smt, smp_threads); >> >> may be we should reintroduce nr_servers at the machine level ? >> > > I had reintroduced it but then I realized it was only used in this > function. nr_servers is also used when the device tree is populated with the interrupt controller nodes. No big deal. C.
Re: [Qemu-devel] [PATCH 6/6] spapr: fix migration of ICP objects from/to older QEMU
On 05/15/2017 03:16 PM, Greg Kurz wrote:
> On Mon, 15 May 2017 14:22:32 +0200
> Cédric Le Goater wrote:
>
>> On 05/15/2017 01:40 PM, Greg Kurz wrote:
>>> Commit 5bc8d26de20c ("spapr: allocate the ICPState object from under
>>> sPAPRCPUCore") moved ICP objects from the machine to CPU cores. This
>>> is an improvement since we no longer allocate ICP objects that will
>>> never be used. But it has the side-effect of breaking migration of
>>> older machine types from older QEMU versions.
>>>
>>> This patch introduces a compat flag in the sPAPR machine class so
>>> that all pseries machine up to 2.9 go on with the previous behavior
>>> of pre-allocating ICP objects.
>>
>> I think this is a quite elegant way to a handle the migration
>> regression. Thanks for taking care of it.
>>
>> Have you tried to simply reparent the ICPs objects to OBJECT(spapr)
>> instead of the OBJECT(cpu) ?
>>
>
> Do you mean to reparent unconditionally to OBJECT(spapr) for all
> machine versions ?
only in the case of smc->must_pre_allocate_icps
> I'm not sure this would be beneficial, but I might be missing
> something...
I think that we would not need to allocate the legacy_icps array.
Parenting the icp object to the spapr machine should be enough.
I might be wrong. my expertise on the migration stream is very
basic.
C.
Re: [Qemu-devel] [PATCH 6/6] spapr: fix migration of ICP objects from/to older QEMU
On Mon, 15 May 2017 14:22:32 +0200
Cédric Le Goater wrote:
> On 05/15/2017 01:40 PM, Greg Kurz wrote:
> > Commit 5bc8d26de20c ("spapr: allocate the ICPState object from under
> > sPAPRCPUCore") moved ICP objects from the machine to CPU cores. This
> > is an improvement since we no longer allocate ICP objects that will
> > never be used. But it has the side-effect of breaking migration of
> > older machine types from older QEMU versions.
> >
> > This patch introduces a compat flag in the sPAPR machine class so
> > that all pseries machine up to 2.9 go on with the previous behavior
> > of pre-allocating ICP objects.
>
> I think this is a quite elegant way to a handle the migration
> regression. Thanks for taking care of it.
>
> Have you tried to simply reparent the ICPs objects to OBJECT(spapr)
> instead of the OBJECT(cpu) ?
>
Do you mean to reparent unconditionally to OBJECT(spapr) for all
machine versions ? I'm not sure this would be beneficial, but I
might be missing something...
> See some minor comments below.
>
> > While here, we also ensure that object_property_add_child() errors cause
> > QEMU to abort for newer machines.
> >
> > Signed-off-by: Greg Kurz
> > ---
> > hw/ppc/spapr.c | 36
> > hw/ppc/spapr_cpu_core.c | 28
> > include/hw/ppc/spapr.h |2 ++
> > 3 files changed, 58 insertions(+), 8 deletions(-)
> >
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index c53989bb10b1..ab3683bcd677 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -126,6 +126,7 @@ error:
> > static void xics_system_init(MachineState *machine, int nr_irqs, Error
> > **errp)
> > {
> > sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
> > +sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> > Error *local_err = NULL;
> >
> > if (kvm_enabled()) {
> > @@ -151,6 +152,38 @@ static void xics_system_init(MachineState *machine,
> > int nr_irqs, Error **errp)
> >&local_err);
> > }
> >
> > +if (!spapr->ics) {
> > +goto out;
> > +}
> > +
> > +if (smc->must_pre_allocate_icps) {
>
> I am not sure I like 'must', I think 'pre_allocate_icps' should be enough ?
> or simply 'allocate_legacy_icps' ?
>
Ok. I'll go for the latter if it's ok with David.
> > +int smt = kvmppc_smt_threads();
> > +int nr_servers = DIV_ROUND_UP(max_cpus * smt, smp_threads);
>
> may be we should reintroduce nr_servers at the machine level ?
>
I had reintroduced it but then I realized it was only used in this
function.
> > +int i;
> > +
> > +spapr->legacy_icps = g_malloc0(nr_servers * sizeof(ICPState));
> > +
> > +for (i = 0; i < nr_servers; i++) {
> > +void* obj = &spapr->legacy_icps[i];
>
> 'void *'
>
Yeah, patchew already yelled at me :)
> > +
> > +object_initialize(obj, sizeof(ICPState), spapr->icp_type);
> > +object_property_add_child(OBJECT(spapr), "icp[*]", obj,
> > + &error_abort);
>
> David does not like the "icp[*]" syntax.
>
Ah... I wasn't aware of that. But I agree that I should probably create
the object names based on 'i', rather than relying on the more complex
logic in object_property_add().
> > +object_unref(obj);
> > +object_property_add_const_link(obj, "xics", OBJECT(spapr),
> > + &error_abort);
> > +object_property_set_bool(obj, true, "realized", &local_err);
> > +if (local_err) {
> > +while (i--) {
> > +object_unparent(obj);
> > +}
> > +g_free(spapr->legacy_icps);
> > +break;
> > +}
> > +}
> > +}
> > +
> > +out:
> > error_propagate(errp, local_err);
> > }
> >
> > @@ -3256,8 +3289,11 @@ static void
> > spapr_machine_2_9_instance_options(MachineState *machine)
> >
> > static void spapr_machine_2_9_class_options(MachineClass *mc)
> > {
> > +sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
> > +
> > spapr_machine_2_10_class_options(mc);
> > SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_9);
> > +smc->must_pre_allocate_icps = true;
> > }
> >
> > DEFINE_SPAPR_MACHINE(2_9, "2.9", false);
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index 63d160f7e010..5476647efa06 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -119,6 +119,7 @@ static void spapr_cpu_core_unrealizefn(DeviceState
> > *dev, Error **errp)
> > size_t size = object_type_get_instance_size(typename);
> > CPUCore *cc = CPU_CORE(dev);
> > int i;
> > +sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> >
> > for (i = 0; i < cc->nr_threads; i++) {
> > void *obj = sc->threads + i * size;
> > @@ -127,7 +128,9 @@ static void
Re: [Qemu-devel] [PATCH 6/6] spapr: fix migration of ICP objects from/to older QEMU
On 05/15/2017 01:40 PM, Greg Kurz wrote:
> Commit 5bc8d26de20c ("spapr: allocate the ICPState object from under
> sPAPRCPUCore") moved ICP objects from the machine to CPU cores. This
> is an improvement since we no longer allocate ICP objects that will
> never be used. But it has the side-effect of breaking migration of
> older machine types from older QEMU versions.
>
> This patch introduces a compat flag in the sPAPR machine class so
> that all pseries machine up to 2.9 go on with the previous behavior
> of pre-allocating ICP objects.
I think this is a quite elegant way to a handle the migration
regression. Thanks for taking care of it.
Have you tried to simply reparent the ICPs objects to OBJECT(spapr)
instead of the OBJECT(cpu) ?
See some minor comments below.
> While here, we also ensure that object_property_add_child() errors cause
> QEMU to abort for newer machines.
>
> Signed-off-by: Greg Kurz
> ---
> hw/ppc/spapr.c | 36
> hw/ppc/spapr_cpu_core.c | 28
> include/hw/ppc/spapr.h |2 ++
> 3 files changed, 58 insertions(+), 8 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index c53989bb10b1..ab3683bcd677 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -126,6 +126,7 @@ error:
> static void xics_system_init(MachineState *machine, int nr_irqs, Error
> **errp)
> {
> sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
> +sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> Error *local_err = NULL;
>
> if (kvm_enabled()) {
> @@ -151,6 +152,38 @@ static void xics_system_init(MachineState *machine, int
> nr_irqs, Error **errp)
>&local_err);
> }
>
> +if (!spapr->ics) {
> +goto out;
> +}
> +
> +if (smc->must_pre_allocate_icps) {
I am not sure I like 'must', I think 'pre_allocate_icps' should be enough ?
or simply 'allocate_legacy_icps' ?
> +int smt = kvmppc_smt_threads();
> +int nr_servers = DIV_ROUND_UP(max_cpus * smt, smp_threads);
may be we should reintroduce nr_servers at the machine level ?
> +int i;
> +
> +spapr->legacy_icps = g_malloc0(nr_servers * sizeof(ICPState));
> +
> +for (i = 0; i < nr_servers; i++) {
> +void* obj = &spapr->legacy_icps[i];
'void *'
> +
> +object_initialize(obj, sizeof(ICPState), spapr->icp_type);
> +object_property_add_child(OBJECT(spapr), "icp[*]", obj,
> + &error_abort);
David does not like the "icp[*]" syntax.
> +object_unref(obj);
> +object_property_add_const_link(obj, "xics", OBJECT(spapr),
> + &error_abort);
> +object_property_set_bool(obj, true, "realized", &local_err);
> +if (local_err) {
> +while (i--) {
> +object_unparent(obj);
> +}
> +g_free(spapr->legacy_icps);
> +break;
> +}
> +}
> +}
> +
> +out:
> error_propagate(errp, local_err);
> }
>
> @@ -3256,8 +3289,11 @@ static void
> spapr_machine_2_9_instance_options(MachineState *machine)
>
> static void spapr_machine_2_9_class_options(MachineClass *mc)
> {
> +sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
> +
> spapr_machine_2_10_class_options(mc);
> SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_9);
> +smc->must_pre_allocate_icps = true;
> }
>
> DEFINE_SPAPR_MACHINE(2_9, "2.9", false);
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 63d160f7e010..5476647efa06 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -119,6 +119,7 @@ static void spapr_cpu_core_unrealizefn(DeviceState *dev,
> Error **errp)
> size_t size = object_type_get_instance_size(typename);
> CPUCore *cc = CPU_CORE(dev);
> int i;
> +sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>
> for (i = 0; i < cc->nr_threads; i++) {
> void *obj = sc->threads + i * size;
> @@ -127,7 +128,9 @@ static void spapr_cpu_core_unrealizefn(DeviceState *dev,
> Error **errp)
> PowerPCCPU *cpu = POWERPC_CPU(cs);
>
> spapr_cpu_destroy(cpu);
> -object_unparent(cpu->intc);
> +if (!spapr->legacy_icps) {
> +object_unparent(cpu->intc);
> +}
> cpu_remove_sync(cs);
> object_unparent(obj);
> }
> @@ -142,12 +145,19 @@ static void spapr_cpu_core_realize_child(Object *child,
> Error **errp)
> PowerPCCPU *cpu = POWERPC_CPU(cs);
> Object *obj;
>
> -obj = object_new(spapr->icp_type);
> -object_property_add_child(OBJECT(cpu), "icp", obj, NULL);
> -object_property_add_const_link(obj, "xics", OBJECT(spapr), &error_abort);
> -object_property_set_bool(obj, true, "realized", &local_err);
> -if (local_err) {
> -goto error;
> +
[Qemu-devel] [PATCH 6/6] spapr: fix migration of ICP objects from/to older QEMU
Commit 5bc8d26de20c ("spapr: allocate the ICPState object from under
sPAPRCPUCore") moved ICP objects from the machine to CPU cores. This
is an improvement since we no longer allocate ICP objects that will
never be used. But it has the side-effect of breaking migration of
older machine types from older QEMU versions.
This patch introduces a compat flag in the sPAPR machine class so
that all pseries machine up to 2.9 go on with the previous behavior
of pre-allocating ICP objects.
While here, we also ensure that object_property_add_child() errors cause
QEMU to abort for newer machines.
Signed-off-by: Greg Kurz
---
hw/ppc/spapr.c | 36
hw/ppc/spapr_cpu_core.c | 28
include/hw/ppc/spapr.h |2 ++
3 files changed, 58 insertions(+), 8 deletions(-)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index c53989bb10b1..ab3683bcd677 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -126,6 +126,7 @@ error:
static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp)
{
sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
+sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
Error *local_err = NULL;
if (kvm_enabled()) {
@@ -151,6 +152,38 @@ static void xics_system_init(MachineState *machine, int
nr_irqs, Error **errp)
&local_err);
}
+if (!spapr->ics) {
+goto out;
+}
+
+if (smc->must_pre_allocate_icps) {
+int smt = kvmppc_smt_threads();
+int nr_servers = DIV_ROUND_UP(max_cpus * smt, smp_threads);
+int i;
+
+spapr->legacy_icps = g_malloc0(nr_servers * sizeof(ICPState));
+
+for (i = 0; i < nr_servers; i++) {
+void* obj = &spapr->legacy_icps[i];
+
+object_initialize(obj, sizeof(ICPState), spapr->icp_type);
+object_property_add_child(OBJECT(spapr), "icp[*]", obj,
+ &error_abort);
+object_unref(obj);
+object_property_add_const_link(obj, "xics", OBJECT(spapr),
+ &error_abort);
+object_property_set_bool(obj, true, "realized", &local_err);
+if (local_err) {
+while (i--) {
+object_unparent(obj);
+}
+g_free(spapr->legacy_icps);
+break;
+}
+}
+}
+
+out:
error_propagate(errp, local_err);
}
@@ -3256,8 +3289,11 @@ static void
spapr_machine_2_9_instance_options(MachineState *machine)
static void spapr_machine_2_9_class_options(MachineClass *mc)
{
+sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
+
spapr_machine_2_10_class_options(mc);
SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_9);
+smc->must_pre_allocate_icps = true;
}
DEFINE_SPAPR_MACHINE(2_9, "2.9", false);
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 63d160f7e010..5476647efa06 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -119,6 +119,7 @@ static void spapr_cpu_core_unrealizefn(DeviceState *dev,
Error **errp)
size_t size = object_type_get_instance_size(typename);
CPUCore *cc = CPU_CORE(dev);
int i;
+sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
for (i = 0; i < cc->nr_threads; i++) {
void *obj = sc->threads + i * size;
@@ -127,7 +128,9 @@ static void spapr_cpu_core_unrealizefn(DeviceState *dev,
Error **errp)
PowerPCCPU *cpu = POWERPC_CPU(cs);
spapr_cpu_destroy(cpu);
-object_unparent(cpu->intc);
+if (!spapr->legacy_icps) {
+object_unparent(cpu->intc);
+}
cpu_remove_sync(cs);
object_unparent(obj);
}
@@ -142,12 +145,19 @@ static void spapr_cpu_core_realize_child(Object *child,
Error **errp)
PowerPCCPU *cpu = POWERPC_CPU(cs);
Object *obj;
-obj = object_new(spapr->icp_type);
-object_property_add_child(OBJECT(cpu), "icp", obj, NULL);
-object_property_add_const_link(obj, "xics", OBJECT(spapr), &error_abort);
-object_property_set_bool(obj, true, "realized", &local_err);
-if (local_err) {
-goto error;
+if (spapr->legacy_icps) {
+int index = cpu->parent_obj.cpu_index;
+
+obj = OBJECT(&spapr->legacy_icps[index]);
+} else {
+obj = object_new(spapr->icp_type);
+object_property_add_child(OBJECT(cpu), "icp", obj, &error_abort);
+object_property_add_const_link(obj, "xics", OBJECT(spapr),
+ &error_abort);
+object_property_set_bool(obj, true, "realized", &local_err);
+if (local_err) {
+goto error;
+}
}
object_property_set_bool(child, true, "realized", &local_err);
@@ -164,7 +174,9 @@ static void spapr_cpu_core_realize_child(Object *child,
Error **errp)
return;
error:
-object_unparent(obj);
+if (!spapr->legacy
