[Qemu-devel] CPU hotplug, again

[Qemu-devel] CPU hotplug, again

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1674 bytes --]

Hi Andreas,

I've now found (with Thomas' help) your RFC series for socket/core
based cpu hotplug on x86
(https://github.com/afaerber/qemu-cpu/compare/qom-cpu-x86).  It seems
sensible enough as far as it goes, but doesn't seem to address a bunch
of the things that I was attempting to do with the cpu-package
proposal - and which we absolutely need for cpu hotplug on Power.

1) What interface do you envisage beyond cpu_add?

The patches I see just construct extra socket and core objects, but
still control hotplug (for x86) through the cpu_add interface.  That
interface is absolutely unusable on Power, since it operates on a
per-thread basis, whereas the PAPR guest<->host interfaces can only
communicate information at a per-core granularity.

2) When hotplugging at core or socket granularity, where would the
   code to construct the individual thread objects sit?

Your series has the construction done in both the machine init path
and the hotplug path.  The latter works because hotplug occurs at
thread granularity.  If we're hotplugging at core or socket
granularity what would do the construct?  The core/socket object
itself (in instance_init?  in realize?); the hotplug handler?
something else?

3) How does the management layer determine what is pluggable?

Both the number of pluggable slots, and what it will need to do to
populate them.

4) How do we enforce that toplogies illegal for the platform can't be
   constructed?

-- 
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: 819 bytes --]

Re: [Qemu-devel] CPU hotplug, again

[Bharata B Rao]

On Tue, Feb 23, 2016 at 04:24:31PM +1100, David Gibson wrote:
> Hi Andreas,
> 
> I've now found (with Thomas' help) your RFC series for socket/core
> based cpu hotplug on x86
> (https://github.com/afaerber/qemu-cpu/compare/qom-cpu-x86).  It seems
> sensible enough as far as it goes, but doesn't seem to address a bunch
> of the things that I was attempting to do with the cpu-package
> proposal - and which we absolutely need for cpu hotplug on Power.
> 
> 1) What interface do you envisage beyond cpu_add?
> 
> The patches I see just construct extra socket and core objects, but
> still control hotplug (for x86) through the cpu_add interface.  That
> interface is absolutely unusable on Power, since it operates on a
> per-thread basis, whereas the PAPR guest<->host interfaces can only
> communicate information at a per-core granularity.
> 
> 2) When hotplugging at core or socket granularity, where would the
>    code to construct the individual thread objects sit?
> 
> Your series has the construction done in both the machine init path
> and the hotplug path.  The latter works because hotplug occurs at
> thread granularity.  If we're hotplugging at core or socket
> granularity what would do the construct?  The core/socket object
> itself (in instance_init?  in realize?); the hotplug handler?
> something else?
> 
> 3) How does the management layer determine what is pluggable?
> 
> Both the number of pluggable slots, and what it will need to do to
> populate them.
> 
> 4) How do we enforce that toplogies illegal for the platform can't be
>    constructed?

5) QOM-links

Andreas, You have often talked about setting up links from machine object
to the CPU objects. Would the below code correctly capture that idea of
yours ?

#define SPAPR_MACHINE_CPU_CORE_PROP "core"

/* MachineClass.init for sPAPR */
static void ppc_spapr_init(MachineState *machine)
{
    sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
    int spapr_smp_cores = smp_cpus / smp_threads;
    int spapr_max_cores = max_cpus / smp_threads;

    ...
    for (i = 0; i < spapr_max_cores; i++) {
        Object *obj = object_new(TYPE_SPAPR_CPU_CORE);
        sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
        char name[32];

        snprintf(name, sizeof(name), "%s[%d]", SPAPR_MACHINE_CPU_CORE_PROP, i);

        /*
         * Create links from machine objects to all possible cores.
         */
        object_property_add_link(OBJECT(spapr), name, TYPE_SPAPR_CPU_CORE,
                                 (Object **)&spapr->core[i],
                                 NULL, NULL, &error_abort); 

        /*
         * Set the QOM link from machine object to core object for all
         * boot time CPUs specified with -smp. For rest of the hotpluggable
         * cores this is done from the core hotplug path.
         */
        if (i < spapr_smp_cores) {
            object_property_set_link(OBJECT(spapr), OBJECT(core),
                                     SPAPR_MACHINE_CPU_CORE_PROP, &error_abort);
        }
    ...
}

Re: [Qemu-devel] CPU hotplug, again

[Bharata B Rao]

On Tue, Feb 23, 2016 at 03:10:26PM +0530, Bharata B Rao wrote:
> 
> 5) QOM-links
> 
> Andreas, You have often talked about setting up links from machine object
> to the CPU objects. Would the below code correctly capture that idea of
> yours ?
> 
> #define SPAPR_MACHINE_CPU_CORE_PROP "core"
> 
> /* MachineClass.init for sPAPR */
> static void ppc_spapr_init(MachineState *machine)
> {
>     sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
>     int spapr_smp_cores = smp_cpus / smp_threads;
>     int spapr_max_cores = max_cpus / smp_threads;
> 
>     ...
>     for (i = 0; i < spapr_max_cores; i++) {
>         Object *obj = object_new(TYPE_SPAPR_CPU_CORE);
>         sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
>         char name[32];
> 
>         snprintf(name, sizeof(name), "%s[%d]", SPAPR_MACHINE_CPU_CORE_PROP, i);
> 
>         /*
>          * Create links from machine objects to all possible cores.
>          */
>         object_property_add_link(OBJECT(spapr), name, TYPE_SPAPR_CPU_CORE,
>                                  (Object **)&spapr->core[i],
>                                  NULL, NULL, &error_abort); 
> 
>         /*
>          * Set the QOM link from machine object to core object for all
>          * boot time CPUs specified with -smp. For rest of the hotpluggable
>          * cores this is done from the core hotplug path.
>          */
>         if (i < spapr_smp_cores) {
>             object_property_set_link(OBJECT(spapr), OBJECT(core),
>                                      SPAPR_MACHINE_CPU_CORE_PROP, &error_abort);
>         }

BTW s/SPAPR_MACHINE_CPU_CORE_PROP/name in object_property_set_link() above.

Re: [Qemu-devel] CPU hotplug, again

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3835 bytes --]

On Tue, Feb 23, 2016 at 03:10:26PM +0530, Bharata B Rao wrote:
> On Tue, Feb 23, 2016 at 04:24:31PM +1100, David Gibson wrote:
> > Hi Andreas,
> > 
> > I've now found (with Thomas' help) your RFC series for socket/core
> > based cpu hotplug on x86
> > (https://github.com/afaerber/qemu-cpu/compare/qom-cpu-x86).  It seems
> > sensible enough as far as it goes, but doesn't seem to address a bunch
> > of the things that I was attempting to do with the cpu-package
> > proposal - and which we absolutely need for cpu hotplug on Power.
> > 
> > 1) What interface do you envisage beyond cpu_add?
> > 
> > The patches I see just construct extra socket and core objects, but
> > still control hotplug (for x86) through the cpu_add interface.  That
> > interface is absolutely unusable on Power, since it operates on a
> > per-thread basis, whereas the PAPR guest<->host interfaces can only
> > communicate information at a per-core granularity.
> > 
> > 2) When hotplugging at core or socket granularity, where would the
> >    code to construct the individual thread objects sit?
> > 
> > Your series has the construction done in both the machine init path
> > and the hotplug path.  The latter works because hotplug occurs at
> > thread granularity.  If we're hotplugging at core or socket
> > granularity what would do the construct?  The core/socket object
> > itself (in instance_init?  in realize?); the hotplug handler?
> > something else?
> > 
> > 3) How does the management layer determine what is pluggable?
> > 
> > Both the number of pluggable slots, and what it will need to do to
> > populate them.
> > 
> > 4) How do we enforce that toplogies illegal for the platform can't be
> >    constructed?
> 
> 5) QOM-links
> 
> Andreas, You have often talked about setting up links from machine object
> to the CPU objects. Would the below code correctly capture that idea of
> yours ?
> 
> #define SPAPR_MACHINE_CPU_CORE_PROP "core"
> 
> /* MachineClass.init for sPAPR */
> static void ppc_spapr_init(MachineState *machine)
> {
>     sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
>     int spapr_smp_cores = smp_cpus / smp_threads;
>     int spapr_max_cores = max_cpus / smp_threads;
> 
>     ...
>     for (i = 0; i < spapr_max_cores; i++) {
>         Object *obj = object_new(TYPE_SPAPR_CPU_CORE);
>         sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
>         char name[32];
> 
>         snprintf(name, sizeof(name), "%s[%d]", SPAPR_MACHINE_CPU_CORE_PROP, i);
> 
>         /*
>          * Create links from machine objects to all possible cores.
>          */
>         object_property_add_link(OBJECT(spapr), name, TYPE_SPAPR_CPU_CORE,
>                                  (Object **)&spapr->core[i],
>                                  NULL, NULL, &error_abort); 
> 
>         /*
>          * Set the QOM link from machine object to core object for all
>          * boot time CPUs specified with -smp. For rest of the hotpluggable
>          * cores this is done from the core hotplug path.
>          */
>         if (i < spapr_smp_cores) {
>             object_property_set_link(OBJECT(spapr), OBJECT(core),
>                                      SPAPR_MACHINE_CPU_CORE_PROP, &error_abort);

I hope we can at least have a helper function to both construct the
core and create the links, if we can't handle the link creation in the
core object itself.

Having to open-code it in each machine sounds like a recipe for subtle
differences in presentation between platforms, which is exactly what
we want to avoid.

>         }
>     ...
> }
> 

-- 
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: 819 bytes --]

Re: [Qemu-devel] CPU hotplug, again

[Igor Mammedov]

On Tue, 23 Feb 2016 21:05:04 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Tue, Feb 23, 2016 at 03:10:26PM +0530, Bharata B Rao wrote:
> > On Tue, Feb 23, 2016 at 04:24:31PM +1100, David Gibson wrote:  
> > > Hi Andreas,
> > > 
> > > I've now found (with Thomas' help) your RFC series for socket/core
> > > based cpu hotplug on x86
> > > (https://github.com/afaerber/qemu-cpu/compare/qom-cpu-x86).  It seems
> > > sensible enough as far as it goes, but doesn't seem to address a bunch
> > > of the things that I was attempting to do with the cpu-package
> > > proposal - and which we absolutely need for cpu hotplug on Power.
> > > 
> > > 1) What interface do you envisage beyond cpu_add?
> > > 
> > > The patches I see just construct extra socket and core objects, but
> > > still control hotplug (for x86) through the cpu_add interface.  That
> > > interface is absolutely unusable on Power, since it operates on a
> > > per-thread basis, whereas the PAPR guest<->host interfaces can only
> > > communicate information at a per-core granularity.
> > > 
> > > 2) When hotplugging at core or socket granularity, where would the
> > >    code to construct the individual thread objects sit?
> > > 
> > > Your series has the construction done in both the machine init path
> > > and the hotplug path.  The latter works because hotplug occurs at
> > > thread granularity.  If we're hotplugging at core or socket
> > > granularity what would do the construct?  The core/socket object
> > > itself (in instance_init?  in realize?); the hotplug handler?
> > > something else?
> > > 
> > > 3) How does the management layer determine what is pluggable?
> > > 
> > > Both the number of pluggable slots, and what it will need to do to
> > > populate them.
> > > 
> > > 4) How do we enforce that toplogies illegal for the platform can't be
> > >    constructed?  
> > 
> > 5) QOM-links
> > 
> > Andreas, You have often talked about setting up links from machine object
> > to the CPU objects. Would the below code correctly capture that idea of
> > yours ?
> > 
> > #define SPAPR_MACHINE_CPU_CORE_PROP "core"
> > 
> > /* MachineClass.init for sPAPR */
> > static void ppc_spapr_init(MachineState *machine)
> > {
> >     sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
> >     int spapr_smp_cores = smp_cpus / smp_threads;
> >     int spapr_max_cores = max_cpus / smp_threads;
> > 
> >     ...
> >     for (i = 0; i < spapr_max_cores; i++) {
> >         Object *obj = object_new(TYPE_SPAPR_CPU_CORE);
> >         sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> >         char name[32];
> > 
> >         snprintf(name, sizeof(name), "%s[%d]", SPAPR_MACHINE_CPU_CORE_PROP, i);
> > 
> >         /*
> >          * Create links from machine objects to all possible cores.
> >          */
> >         object_property_add_link(OBJECT(spapr), name, TYPE_SPAPR_CPU_CORE,
> >                                  (Object **)&spapr->core[i],
> >                                  NULL, NULL, &error_abort); 
> > 
> >         /*
> >          * Set the QOM link from machine object to core object for all
> >          * boot time CPUs specified with -smp. For rest of the hotpluggable
> >          * cores this is done from the core hotplug path.
> >          */
> >         if (i < spapr_smp_cores) {
> >             object_property_set_link(OBJECT(spapr), OBJECT(core),
> >                                      SPAPR_MACHINE_CPU_CORE_PROP, &error_abort);  
> 
> I hope we can at least have a helper function to both construct the
> core and create the links, if we can't handle the link creation in the
> core object itself.
> 
> Having to open-code it in each machine sounds like a recipe for subtle
> differences in presentation between platforms, which is exactly what
> we want to avoid.
Creating links doesn't give us much, it's just adds means for mgmt
to check how many CPUs could be hotplugged  without keeping that
state in mgmt like it's now, so links are mostly useless if one
care where CPU is being plugged in.
The rest like enumerating exiting CPUs could be done by
traversing QOM tree, links would just simplify finding
CPUs putting them at fixed namespace.

> 
> >         }
> >     ...
> > }
> >   
> 

Re: [Qemu-devel] CPU hotplug, again

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4910 bytes --]

On Tue, Feb 23, 2016 at 12:18:59PM +0100, Igor Mammedov wrote:
> On Tue, 23 Feb 2016 21:05:04 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Tue, Feb 23, 2016 at 03:10:26PM +0530, Bharata B Rao wrote:
> > > On Tue, Feb 23, 2016 at 04:24:31PM +1100, David Gibson wrote:  
> > > > Hi Andreas,
> > > > 
> > > > I've now found (with Thomas' help) your RFC series for socket/core
> > > > based cpu hotplug on x86
> > > > (https://github.com/afaerber/qemu-cpu/compare/qom-cpu-x86).  It seems
> > > > sensible enough as far as it goes, but doesn't seem to address a bunch
> > > > of the things that I was attempting to do with the cpu-package
> > > > proposal - and which we absolutely need for cpu hotplug on Power.
> > > > 
> > > > 1) What interface do you envisage beyond cpu_add?
> > > > 
> > > > The patches I see just construct extra socket and core objects, but
> > > > still control hotplug (for x86) through the cpu_add interface.  That
> > > > interface is absolutely unusable on Power, since it operates on a
> > > > per-thread basis, whereas the PAPR guest<->host interfaces can only
> > > > communicate information at a per-core granularity.
> > > > 
> > > > 2) When hotplugging at core or socket granularity, where would the
> > > >    code to construct the individual thread objects sit?
> > > > 
> > > > Your series has the construction done in both the machine init path
> > > > and the hotplug path.  The latter works because hotplug occurs at
> > > > thread granularity.  If we're hotplugging at core or socket
> > > > granularity what would do the construct?  The core/socket object
> > > > itself (in instance_init?  in realize?); the hotplug handler?
> > > > something else?
> > > > 
> > > > 3) How does the management layer determine what is pluggable?
> > > > 
> > > > Both the number of pluggable slots, and what it will need to do to
> > > > populate them.
> > > > 
> > > > 4) How do we enforce that toplogies illegal for the platform can't be
> > > >    constructed?  
> > > 
> > > 5) QOM-links
> > > 
> > > Andreas, You have often talked about setting up links from machine object
> > > to the CPU objects. Would the below code correctly capture that idea of
> > > yours ?
> > > 
> > > #define SPAPR_MACHINE_CPU_CORE_PROP "core"
> > > 
> > > /* MachineClass.init for sPAPR */
> > > static void ppc_spapr_init(MachineState *machine)
> > > {
> > >     sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
> > >     int spapr_smp_cores = smp_cpus / smp_threads;
> > >     int spapr_max_cores = max_cpus / smp_threads;
> > > 
> > >     ...
> > >     for (i = 0; i < spapr_max_cores; i++) {
> > >         Object *obj = object_new(TYPE_SPAPR_CPU_CORE);
> > >         sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > >         char name[32];
> > > 
> > >         snprintf(name, sizeof(name), "%s[%d]", SPAPR_MACHINE_CPU_CORE_PROP, i);
> > > 
> > >         /*
> > >          * Create links from machine objects to all possible cores.
> > >          */
> > >         object_property_add_link(OBJECT(spapr), name, TYPE_SPAPR_CPU_CORE,
> > >                                  (Object **)&spapr->core[i],
> > >                                  NULL, NULL, &error_abort); 
> > > 
> > >         /*
> > >          * Set the QOM link from machine object to core object for all
> > >          * boot time CPUs specified with -smp. For rest of the hotpluggable
> > >          * cores this is done from the core hotplug path.
> > >          */
> > >         if (i < spapr_smp_cores) {
> > >             object_property_set_link(OBJECT(spapr), OBJECT(core),
> > >                                      SPAPR_MACHINE_CPU_CORE_PROP, &error_abort);  
> > 
> > I hope we can at least have a helper function to both construct the
> > core and create the links, if we can't handle the link creation in the
> > core object itself.
> > 
> > Having to open-code it in each machine sounds like a recipe for subtle
> > differences in presentation between platforms, which is exactly what
> > we want to avoid.
> Creating links doesn't give us much, it's just adds means for mgmt
> to check how many CPUs could be hotplugged  without keeping that
> state in mgmt like it's now, so links are mostly useless if one
> care where CPU is being plugged in.
> The rest like enumerating exiting CPUs could be done by
> traversing QOM tree, links would just simplify finding
> CPUs putting them at fixed namespace.

Simplifying finding CPUs is pretty much all we intended the links for.
Well, and then I was expecting a different set of links to simplify
enumerating all the threads in a cpu package/core/socket/whatever.

-- 
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: 819 bytes --]

Re: [Qemu-devel] CPU hotplug, again

[Bharata B Rao]

On Wed, Feb 24, 2016 at 01:01:06PM +1100, David Gibson wrote:
> On Tue, Feb 23, 2016 at 12:18:59PM +0100, Igor Mammedov wrote:
> > On Tue, 23 Feb 2016 21:05:04 +1100
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> > 
> > > On Tue, Feb 23, 2016 at 03:10:26PM +0530, Bharata B Rao wrote:
> > > > On Tue, Feb 23, 2016 at 04:24:31PM +1100, David Gibson wrote:  
> > > > > Hi Andreas,
> > > > > 
> > > > > I've now found (with Thomas' help) your RFC series for socket/core
> > > > > based cpu hotplug on x86
> > > > > (https://github.com/afaerber/qemu-cpu/compare/qom-cpu-x86).  It seems
> > > > > sensible enough as far as it goes, but doesn't seem to address a bunch
> > > > > of the things that I was attempting to do with the cpu-package
> > > > > proposal - and which we absolutely need for cpu hotplug on Power.
> > > > > 
> > > > > 1) What interface do you envisage beyond cpu_add?
> > > > > 
> > > > > The patches I see just construct extra socket and core objects, but
> > > > > still control hotplug (for x86) through the cpu_add interface.  That
> > > > > interface is absolutely unusable on Power, since it operates on a
> > > > > per-thread basis, whereas the PAPR guest<->host interfaces can only
> > > > > communicate information at a per-core granularity.
> > > > > 
> > > > > 2) When hotplugging at core or socket granularity, where would the
> > > > >    code to construct the individual thread objects sit?
> > > > > 
> > > > > Your series has the construction done in both the machine init path
> > > > > and the hotplug path.  The latter works because hotplug occurs at
> > > > > thread granularity.  If we're hotplugging at core or socket
> > > > > granularity what would do the construct?  The core/socket object
> > > > > itself (in instance_init?  in realize?); the hotplug handler?
> > > > > something else?
> > > > > 
> > > > > 3) How does the management layer determine what is pluggable?
> > > > > 
> > > > > Both the number of pluggable slots, and what it will need to do to
> > > > > populate them.
> > > > > 
> > > > > 4) How do we enforce that toplogies illegal for the platform can't be
> > > > >    constructed?  
> > > > 
> > > > 5) QOM-links
> > > > 
> > > > Andreas, You have often talked about setting up links from machine object
> > > > to the CPU objects. Would the below code correctly capture that idea of
> > > > yours ?
> > > > 
> > > > #define SPAPR_MACHINE_CPU_CORE_PROP "core"
> > > > 
> > > > /* MachineClass.init for sPAPR */
> > > > static void ppc_spapr_init(MachineState *machine)
> > > > {
> > > >     sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
> > > >     int spapr_smp_cores = smp_cpus / smp_threads;
> > > >     int spapr_max_cores = max_cpus / smp_threads;
> > > > 
> > > >     ...
> > > >     for (i = 0; i < spapr_max_cores; i++) {
> > > >         Object *obj = object_new(TYPE_SPAPR_CPU_CORE);
> > > >         sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > > >         char name[32];
> > > > 
> > > >         snprintf(name, sizeof(name), "%s[%d]", SPAPR_MACHINE_CPU_CORE_PROP, i);
> > > > 
> > > >         /*
> > > >          * Create links from machine objects to all possible cores.
> > > >          */
> > > >         object_property_add_link(OBJECT(spapr), name, TYPE_SPAPR_CPU_CORE,
> > > >                                  (Object **)&spapr->core[i],
> > > >                                  NULL, NULL, &error_abort); 
> > > > 
> > > >         /*
> > > >          * Set the QOM link from machine object to core object for all
> > > >          * boot time CPUs specified with -smp. For rest of the hotpluggable
> > > >          * cores this is done from the core hotplug path.
> > > >          */
> > > >         if (i < spapr_smp_cores) {
> > > >             object_property_set_link(OBJECT(spapr), OBJECT(core),
> > > >                                      SPAPR_MACHINE_CPU_CORE_PROP, &error_abort);  
> > > 
> > > I hope we can at least have a helper function to both construct the
> > > core and create the links, if we can't handle the link creation in the
> > > core object itself.
> > > 
> > > Having to open-code it in each machine sounds like a recipe for subtle
> > > differences in presentation between platforms, which is exactly what
> > > we want to avoid.
> > Creating links doesn't give us much, it's just adds means for mgmt
> > to check how many CPUs could be hotplugged  without keeping that
> > state in mgmt like it's now, so links are mostly useless if one
> > care where CPU is being plugged in.
> > The rest like enumerating exiting CPUs could be done by
> > traversing QOM tree, links would just simplify finding
> > CPUs putting them at fixed namespace.
> 
> Simplifying finding CPUs is pretty much all we intended the links for.
> Well, and then I was expecting a different set of links to simplify
> enumerating all the threads in a cpu package/core/socket/whatever.

That's what child links (socket to core to thread on x86 and core to thread
on powerpc) will give us, no ?

Regards,
Bharata.

Re: [Qemu-devel] CPU hotplug, again

[Igor Mammedov]

On Wed, 24 Feb 2016 13:01:06 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Tue, Feb 23, 2016 at 12:18:59PM +0100, Igor Mammedov wrote:
> > On Tue, 23 Feb 2016 21:05:04 +1100
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > On Tue, Feb 23, 2016 at 03:10:26PM +0530, Bharata B Rao wrote:  
> > > > On Tue, Feb 23, 2016 at 04:24:31PM +1100, David Gibson wrote:    
> > > > > Hi Andreas,
> > > > > 
> > > > > I've now found (with Thomas' help) your RFC series for socket/core
> > > > > based cpu hotplug on x86
> > > > > (https://github.com/afaerber/qemu-cpu/compare/qom-cpu-x86).  It seems
> > > > > sensible enough as far as it goes, but doesn't seem to address a bunch
> > > > > of the things that I was attempting to do with the cpu-package
> > > > > proposal - and which we absolutely need for cpu hotplug on Power.
> > > > > 
> > > > > 1) What interface do you envisage beyond cpu_add?
> > > > > 
> > > > > The patches I see just construct extra socket and core objects, but
> > > > > still control hotplug (for x86) through the cpu_add interface.  That
> > > > > interface is absolutely unusable on Power, since it operates on a
> > > > > per-thread basis, whereas the PAPR guest<->host interfaces can only
> > > > > communicate information at a per-core granularity.
> > > > > 
> > > > > 2) When hotplugging at core or socket granularity, where would the
> > > > >    code to construct the individual thread objects sit?
> > > > > 
> > > > > Your series has the construction done in both the machine init path
> > > > > and the hotplug path.  The latter works because hotplug occurs at
> > > > > thread granularity.  If we're hotplugging at core or socket
> > > > > granularity what would do the construct?  The core/socket object
> > > > > itself (in instance_init?  in realize?); the hotplug handler?
> > > > > something else?
> > > > > 
> > > > > 3) How does the management layer determine what is pluggable?
> > > > > 
> > > > > Both the number of pluggable slots, and what it will need to do to
> > > > > populate them.
> > > > > 
> > > > > 4) How do we enforce that toplogies illegal for the platform can't be
> > > > >    constructed?    
> > > > 
> > > > 5) QOM-links
> > > > 
> > > > Andreas, You have often talked about setting up links from machine object
> > > > to the CPU objects. Would the below code correctly capture that idea of
> > > > yours ?
> > > > 
> > > > #define SPAPR_MACHINE_CPU_CORE_PROP "core"
> > > > 
> > > > /* MachineClass.init for sPAPR */
> > > > static void ppc_spapr_init(MachineState *machine)
> > > > {
> > > >     sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
> > > >     int spapr_smp_cores = smp_cpus / smp_threads;
> > > >     int spapr_max_cores = max_cpus / smp_threads;
> > > > 
> > > >     ...
> > > >     for (i = 0; i < spapr_max_cores; i++) {
> > > >         Object *obj = object_new(TYPE_SPAPR_CPU_CORE);
> > > >         sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > > >         char name[32];
> > > > 
> > > >         snprintf(name, sizeof(name), "%s[%d]", SPAPR_MACHINE_CPU_CORE_PROP, i);
> > > > 
> > > >         /*
> > > >          * Create links from machine objects to all possible cores.
> > > >          */
> > > >         object_property_add_link(OBJECT(spapr), name, TYPE_SPAPR_CPU_CORE,
> > > >                                  (Object **)&spapr->core[i],
> > > >                                  NULL, NULL, &error_abort); 
> > > > 
> > > >         /*
> > > >          * Set the QOM link from machine object to core object for all
> > > >          * boot time CPUs specified with -smp. For rest of the hotpluggable
> > > >          * cores this is done from the core hotplug path.
> > > >          */
> > > >         if (i < spapr_smp_cores) {
> > > >             object_property_set_link(OBJECT(spapr), OBJECT(core),
> > > >                                      SPAPR_MACHINE_CPU_CORE_PROP, &error_abort);    
> > > 
> > > I hope we can at least have a helper function to both construct the
> > > core and create the links, if we can't handle the link creation in the
> > > core object itself.
> > > 
> > > Having to open-code it in each machine sounds like a recipe for subtle
> > > differences in presentation between platforms, which is exactly what
> > > we want to avoid.  
> > Creating links doesn't give us much, it's just adds means for mgmt
> > to check how many CPUs could be hotplugged  without keeping that
> > state in mgmt like it's now, so links are mostly useless if one
> > care where CPU is being plugged in.
> > The rest like enumerating exiting CPUs could be done by
> > traversing QOM tree, links would just simplify finding
> > CPUs putting them at fixed namespace.  
> 
> Simplifying finding CPUs is pretty much all we intended the links for.
Do mgmt really needs it? For machine it's easy to find CPUs under
/machine/[peripheral|unattached] by enumerating entries over there.
For human, one would need to implement a dedicated HMP command that
would do the same, so it doesn't really matter where links are located.


> Well, and then I was expecting a different set of links to simplify
> enumerating all the threads in a cpu package/core/socket/whatever.
> 

Re: [Qemu-devel] CPU hotplug, again

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 6020 bytes --]

On Wed, Feb 24, 2016 at 11:48:33AM +0100, Igor Mammedov wrote:
> On Wed, 24 Feb 2016 13:01:06 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Tue, Feb 23, 2016 at 12:18:59PM +0100, Igor Mammedov wrote:
> > > On Tue, 23 Feb 2016 21:05:04 +1100
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >   
> > > > On Tue, Feb 23, 2016 at 03:10:26PM +0530, Bharata B Rao wrote:  
> > > > > On Tue, Feb 23, 2016 at 04:24:31PM +1100, David Gibson wrote:    
> > > > > > Hi Andreas,
> > > > > > 
> > > > > > I've now found (with Thomas' help) your RFC series for socket/core
> > > > > > based cpu hotplug on x86
> > > > > > (https://github.com/afaerber/qemu-cpu/compare/qom-cpu-x86).  It seems
> > > > > > sensible enough as far as it goes, but doesn't seem to address a bunch
> > > > > > of the things that I was attempting to do with the cpu-package
> > > > > > proposal - and which we absolutely need for cpu hotplug on Power.
> > > > > > 
> > > > > > 1) What interface do you envisage beyond cpu_add?
> > > > > > 
> > > > > > The patches I see just construct extra socket and core objects, but
> > > > > > still control hotplug (for x86) through the cpu_add interface.  That
> > > > > > interface is absolutely unusable on Power, since it operates on a
> > > > > > per-thread basis, whereas the PAPR guest<->host interfaces can only
> > > > > > communicate information at a per-core granularity.
> > > > > > 
> > > > > > 2) When hotplugging at core or socket granularity, where would the
> > > > > >    code to construct the individual thread objects sit?
> > > > > > 
> > > > > > Your series has the construction done in both the machine init path
> > > > > > and the hotplug path.  The latter works because hotplug occurs at
> > > > > > thread granularity.  If we're hotplugging at core or socket
> > > > > > granularity what would do the construct?  The core/socket object
> > > > > > itself (in instance_init?  in realize?); the hotplug handler?
> > > > > > something else?
> > > > > > 
> > > > > > 3) How does the management layer determine what is pluggable?
> > > > > > 
> > > > > > Both the number of pluggable slots, and what it will need to do to
> > > > > > populate them.
> > > > > > 
> > > > > > 4) How do we enforce that toplogies illegal for the platform can't be
> > > > > >    constructed?    
> > > > > 
> > > > > 5) QOM-links
> > > > > 
> > > > > Andreas, You have often talked about setting up links from machine object
> > > > > to the CPU objects. Would the below code correctly capture that idea of
> > > > > yours ?
> > > > > 
> > > > > #define SPAPR_MACHINE_CPU_CORE_PROP "core"
> > > > > 
> > > > > /* MachineClass.init for sPAPR */
> > > > > static void ppc_spapr_init(MachineState *machine)
> > > > > {
> > > > >     sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
> > > > >     int spapr_smp_cores = smp_cpus / smp_threads;
> > > > >     int spapr_max_cores = max_cpus / smp_threads;
> > > > > 
> > > > >     ...
> > > > >     for (i = 0; i < spapr_max_cores; i++) {
> > > > >         Object *obj = object_new(TYPE_SPAPR_CPU_CORE);
> > > > >         sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > > > >         char name[32];
> > > > > 
> > > > >         snprintf(name, sizeof(name), "%s[%d]", SPAPR_MACHINE_CPU_CORE_PROP, i);
> > > > > 
> > > > >         /*
> > > > >          * Create links from machine objects to all possible cores.
> > > > >          */
> > > > >         object_property_add_link(OBJECT(spapr), name, TYPE_SPAPR_CPU_CORE,
> > > > >                                  (Object **)&spapr->core[i],
> > > > >                                  NULL, NULL, &error_abort); 
> > > > > 
> > > > >         /*
> > > > >          * Set the QOM link from machine object to core object for all
> > > > >          * boot time CPUs specified with -smp. For rest of the hotpluggable
> > > > >          * cores this is done from the core hotplug path.
> > > > >          */
> > > > >         if (i < spapr_smp_cores) {
> > > > >             object_property_set_link(OBJECT(spapr), OBJECT(core),
> > > > >                                      SPAPR_MACHINE_CPU_CORE_PROP, &error_abort);    
> > > > 
> > > > I hope we can at least have a helper function to both construct the
> > > > core and create the links, if we can't handle the link creation in the
> > > > core object itself.
> > > > 
> > > > Having to open-code it in each machine sounds like a recipe for subtle
> > > > differences in presentation between platforms, which is exactly what
> > > > we want to avoid.  
> > > Creating links doesn't give us much, it's just adds means for mgmt
> > > to check how many CPUs could be hotplugged  without keeping that
> > > state in mgmt like it's now, so links are mostly useless if one
> > > care where CPU is being plugged in.
> > > The rest like enumerating exiting CPUs could be done by
> > > traversing QOM tree, links would just simplify finding
> > > CPUs putting them at fixed namespace.  
> > 
> > Simplifying finding CPUs is pretty much all we intended the links for.
> Do mgmt really needs it? For machine it's easy to find CPUs under
> /machine/[peripheral|unattached] by enumerating entries over there.
> For human, one would need to implement a dedicated HMP command that
> would do the same, so it doesn't really matter where links are
> located.

If we require management to go searching the whole device tree for
cpus, I'm concerned they'll just assume they're in the x86 location
instead, and we'll have to fix it over and over for every platform
that puts them somewhere different.

> > Well, and then I was expecting a different set of links to simplify
> > enumerating all the threads in a cpu package/core/socket/whatever.
> > 
> 

-- 
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: 819 bytes --]

Re: [Qemu-devel] CPU hotplug, again

[Igor Mammedov]

On Wed, 24 Feb 2016 22:28:22 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Feb 24, 2016 at 11:48:33AM +0100, Igor Mammedov wrote:
> > On Wed, 24 Feb 2016 13:01:06 +1100
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > On Tue, Feb 23, 2016 at 12:18:59PM +0100, Igor Mammedov wrote:  
> > > > On Tue, 23 Feb 2016 21:05:04 +1100
> > > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > >     
> > > > > On Tue, Feb 23, 2016 at 03:10:26PM +0530, Bharata B Rao wrote:    
> > > > > > On Tue, Feb 23, 2016 at 04:24:31PM +1100, David Gibson wrote:      
> > > > > > > Hi Andreas,
> > > > > > > 
> > > > > > > I've now found (with Thomas' help) your RFC series for socket/core
> > > > > > > based cpu hotplug on x86
> > > > > > > (https://github.com/afaerber/qemu-cpu/compare/qom-cpu-x86).  It seems
> > > > > > > sensible enough as far as it goes, but doesn't seem to address a bunch
> > > > > > > of the things that I was attempting to do with the cpu-package
> > > > > > > proposal - and which we absolutely need for cpu hotplug on Power.
> > > > > > > 
> > > > > > > 1) What interface do you envisage beyond cpu_add?
> > > > > > > 
> > > > > > > The patches I see just construct extra socket and core objects, but
> > > > > > > still control hotplug (for x86) through the cpu_add interface.  That
> > > > > > > interface is absolutely unusable on Power, since it operates on a
> > > > > > > per-thread basis, whereas the PAPR guest<->host interfaces can only
> > > > > > > communicate information at a per-core granularity.
> > > > > > > 
> > > > > > > 2) When hotplugging at core or socket granularity, where would the
> > > > > > >    code to construct the individual thread objects sit?
> > > > > > > 
> > > > > > > Your series has the construction done in both the machine init path
> > > > > > > and the hotplug path.  The latter works because hotplug occurs at
> > > > > > > thread granularity.  If we're hotplugging at core or socket
> > > > > > > granularity what would do the construct?  The core/socket object
> > > > > > > itself (in instance_init?  in realize?); the hotplug handler?
> > > > > > > something else?
> > > > > > > 
> > > > > > > 3) How does the management layer determine what is pluggable?
> > > > > > > 
> > > > > > > Both the number of pluggable slots, and what it will need to do to
> > > > > > > populate them.
> > > > > > > 
> > > > > > > 4) How do we enforce that toplogies illegal for the platform can't be
> > > > > > >    constructed?      
> > > > > > 
> > > > > > 5) QOM-links
> > > > > > 
> > > > > > Andreas, You have often talked about setting up links from machine object
> > > > > > to the CPU objects. Would the below code correctly capture that idea of
> > > > > > yours ?
> > > > > > 
> > > > > > #define SPAPR_MACHINE_CPU_CORE_PROP "core"
> > > > > > 
> > > > > > /* MachineClass.init for sPAPR */
> > > > > > static void ppc_spapr_init(MachineState *machine)
> > > > > > {
> > > > > >     sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
> > > > > >     int spapr_smp_cores = smp_cpus / smp_threads;
> > > > > >     int spapr_max_cores = max_cpus / smp_threads;
> > > > > > 
> > > > > >     ...
> > > > > >     for (i = 0; i < spapr_max_cores; i++) {
> > > > > >         Object *obj = object_new(TYPE_SPAPR_CPU_CORE);
> > > > > >         sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > > > > >         char name[32];
> > > > > > 
> > > > > >         snprintf(name, sizeof(name), "%s[%d]", SPAPR_MACHINE_CPU_CORE_PROP, i);
> > > > > > 
> > > > > >         /*
> > > > > >          * Create links from machine objects to all possible cores.
> > > > > >          */
> > > > > >         object_property_add_link(OBJECT(spapr), name, TYPE_SPAPR_CPU_CORE,
> > > > > >                                  (Object **)&spapr->core[i],
> > > > > >                                  NULL, NULL, &error_abort); 
> > > > > > 
> > > > > >         /*
> > > > > >          * Set the QOM link from machine object to core object for all
> > > > > >          * boot time CPUs specified with -smp. For rest of the hotpluggable
> > > > > >          * cores this is done from the core hotplug path.
> > > > > >          */
> > > > > >         if (i < spapr_smp_cores) {
> > > > > >             object_property_set_link(OBJECT(spapr), OBJECT(core),
> > > > > >                                      SPAPR_MACHINE_CPU_CORE_PROP, &error_abort);      
> > > > > 
> > > > > I hope we can at least have a helper function to both construct the
> > > > > core and create the links, if we can't handle the link creation in the
> > > > > core object itself.
> > > > > 
> > > > > Having to open-code it in each machine sounds like a recipe for subtle
> > > > > differences in presentation between platforms, which is exactly what
> > > > > we want to avoid.    
> > > > Creating links doesn't give us much, it's just adds means for mgmt
> > > > to check how many CPUs could be hotplugged  without keeping that
> > > > state in mgmt like it's now, so links are mostly useless if one
> > > > care where CPU is being plugged in.
> > > > The rest like enumerating exiting CPUs could be done by
> > > > traversing QOM tree, links would just simplify finding
> > > > CPUs putting them at fixed namespace.    
> > > 
> > > Simplifying finding CPUs is pretty much all we intended the links for.  
> > Do mgmt really needs it? For machine it's easy to find CPUs under
> > /machine/[peripheral|unattached] by enumerating entries over there.
> > For human, one would need to implement a dedicated HMP command that
> > would do the same, so it doesn't really matter where links are
> > located.  
> 
> If we require management to go searching the whole device tree for
> cpus, I'm concerned they'll just assume they're in the x86 location
> instead, and we'll have to fix it over and over for every platform
> that puts them somewhere different.
CPUs are inherited from Device so inherited behaviour is that they
are pretty much at fixed location /machine/[peripheral|unattached]
regardless of platform QOM tree wise, like every other device.


> > > Well, and then I was expecting a different set of links to simplify
> > > enumerating all the threads in a cpu package/core/socket/whatever.
> > >   
> >   
> 

Re: [Qemu-devel] CPU hotplug, again

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 7037 bytes --]

On Wed, Feb 24, 2016 at 02:41:17PM +0100, Igor Mammedov wrote:
> On Wed, 24 Feb 2016 22:28:22 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Wed, Feb 24, 2016 at 11:48:33AM +0100, Igor Mammedov wrote:
> > > On Wed, 24 Feb 2016 13:01:06 +1100
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >   
> > > > On Tue, Feb 23, 2016 at 12:18:59PM +0100, Igor Mammedov wrote:  
> > > > > On Tue, 23 Feb 2016 21:05:04 +1100
> > > > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > > >     
> > > > > > On Tue, Feb 23, 2016 at 03:10:26PM +0530, Bharata B Rao wrote:    
> > > > > > > On Tue, Feb 23, 2016 at 04:24:31PM +1100, David Gibson wrote:      
> > > > > > > > Hi Andreas,
> > > > > > > > 
> > > > > > > > I've now found (with Thomas' help) your RFC series for socket/core
> > > > > > > > based cpu hotplug on x86
> > > > > > > > (https://github.com/afaerber/qemu-cpu/compare/qom-cpu-x86).  It seems
> > > > > > > > sensible enough as far as it goes, but doesn't seem to address a bunch
> > > > > > > > of the things that I was attempting to do with the cpu-package
> > > > > > > > proposal - and which we absolutely need for cpu hotplug on Power.
> > > > > > > > 
> > > > > > > > 1) What interface do you envisage beyond cpu_add?
> > > > > > > > 
> > > > > > > > The patches I see just construct extra socket and core objects, but
> > > > > > > > still control hotplug (for x86) through the cpu_add interface.  That
> > > > > > > > interface is absolutely unusable on Power, since it operates on a
> > > > > > > > per-thread basis, whereas the PAPR guest<->host interfaces can only
> > > > > > > > communicate information at a per-core granularity.
> > > > > > > > 
> > > > > > > > 2) When hotplugging at core or socket granularity, where would the
> > > > > > > >    code to construct the individual thread objects sit?
> > > > > > > > 
> > > > > > > > Your series has the construction done in both the machine init path
> > > > > > > > and the hotplug path.  The latter works because hotplug occurs at
> > > > > > > > thread granularity.  If we're hotplugging at core or socket
> > > > > > > > granularity what would do the construct?  The core/socket object
> > > > > > > > itself (in instance_init?  in realize?); the hotplug handler?
> > > > > > > > something else?
> > > > > > > > 
> > > > > > > > 3) How does the management layer determine what is pluggable?
> > > > > > > > 
> > > > > > > > Both the number of pluggable slots, and what it will need to do to
> > > > > > > > populate them.
> > > > > > > > 
> > > > > > > > 4) How do we enforce that toplogies illegal for the platform can't be
> > > > > > > >    constructed?      
> > > > > > > 
> > > > > > > 5) QOM-links
> > > > > > > 
> > > > > > > Andreas, You have often talked about setting up links from machine object
> > > > > > > to the CPU objects. Would the below code correctly capture that idea of
> > > > > > > yours ?
> > > > > > > 
> > > > > > > #define SPAPR_MACHINE_CPU_CORE_PROP "core"
> > > > > > > 
> > > > > > > /* MachineClass.init for sPAPR */
> > > > > > > static void ppc_spapr_init(MachineState *machine)
> > > > > > > {
> > > > > > >     sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
> > > > > > >     int spapr_smp_cores = smp_cpus / smp_threads;
> > > > > > >     int spapr_max_cores = max_cpus / smp_threads;
> > > > > > > 
> > > > > > >     ...
> > > > > > >     for (i = 0; i < spapr_max_cores; i++) {
> > > > > > >         Object *obj = object_new(TYPE_SPAPR_CPU_CORE);
> > > > > > >         sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > > > > > >         char name[32];
> > > > > > > 
> > > > > > >         snprintf(name, sizeof(name), "%s[%d]", SPAPR_MACHINE_CPU_CORE_PROP, i);
> > > > > > > 
> > > > > > >         /*
> > > > > > >          * Create links from machine objects to all possible cores.
> > > > > > >          */
> > > > > > >         object_property_add_link(OBJECT(spapr), name, TYPE_SPAPR_CPU_CORE,
> > > > > > >                                  (Object **)&spapr->core[i],
> > > > > > >                                  NULL, NULL, &error_abort); 
> > > > > > > 
> > > > > > >         /*
> > > > > > >          * Set the QOM link from machine object to core object for all
> > > > > > >          * boot time CPUs specified with -smp. For rest of the hotpluggable
> > > > > > >          * cores this is done from the core hotplug path.
> > > > > > >          */
> > > > > > >         if (i < spapr_smp_cores) {
> > > > > > >             object_property_set_link(OBJECT(spapr), OBJECT(core),
> > > > > > >                                      SPAPR_MACHINE_CPU_CORE_PROP, &error_abort);      
> > > > > > 
> > > > > > I hope we can at least have a helper function to both construct the
> > > > > > core and create the links, if we can't handle the link creation in the
> > > > > > core object itself.
> > > > > > 
> > > > > > Having to open-code it in each machine sounds like a recipe for subtle
> > > > > > differences in presentation between platforms, which is exactly what
> > > > > > we want to avoid.    
> > > > > Creating links doesn't give us much, it's just adds means for mgmt
> > > > > to check how many CPUs could be hotplugged  without keeping that
> > > > > state in mgmt like it's now, so links are mostly useless if one
> > > > > care where CPU is being plugged in.
> > > > > The rest like enumerating exiting CPUs could be done by
> > > > > traversing QOM tree, links would just simplify finding
> > > > > CPUs putting them at fixed namespace.    
> > > > 
> > > > Simplifying finding CPUs is pretty much all we intended the links for.  
> > > Do mgmt really needs it? For machine it's easy to find CPUs under
> > > /machine/[peripheral|unattached] by enumerating entries over there.
> > > For human, one would need to implement a dedicated HMP command that
> > > would do the same, so it doesn't really matter where links are
> > > located.  
> > 
> > If we require management to go searching the whole device tree for
> > cpus, I'm concerned they'll just assume they're in the x86 location
> > instead, and we'll have to fix it over and over for every platform
> > that puts them somewhere different.
> CPUs are inherited from Device so inherited behaviour is that they
> are pretty much at fixed location /machine/[peripheral|unattached]
> regardless of platform QOM tree wise, like every other device.

Hmm.. that's true now, but I can see reasons you might want to put
CPUs on a different bus in future.  In particular consider a machine
type modelling real hardware for a modern multisocket machine - these
are often built from several chips on a common fabric, each containing
several CPU cores, but also other peripherals and bus bridges.

-- 
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: 819 bytes --]

Re: [Qemu-devel] CPU hotplug, again

[Igor Mammedov]

On Thu, 25 Feb 2016 17:41:14 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Feb 24, 2016 at 02:41:17PM +0100, Igor Mammedov wrote:
> > On Wed, 24 Feb 2016 22:28:22 +1100
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > On Wed, Feb 24, 2016 at 11:48:33AM +0100, Igor Mammedov wrote:  
> > > > On Wed, 24 Feb 2016 13:01:06 +1100
> > > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > >     
> > > > > On Tue, Feb 23, 2016 at 12:18:59PM +0100, Igor Mammedov wrote:    
> > > > > > On Tue, 23 Feb 2016 21:05:04 +1100
> > > > > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > > > >       
> > > > > > > On Tue, Feb 23, 2016 at 03:10:26PM +0530, Bharata B Rao wrote:      
> > > > > > > > On Tue, Feb 23, 2016 at 04:24:31PM +1100, David Gibson wrote:        
> > > > > > > > > Hi Andreas,
> > > > > > > > > 
> > > > > > > > > I've now found (with Thomas' help) your RFC series for socket/core
> > > > > > > > > based cpu hotplug on x86
> > > > > > > > > (https://github.com/afaerber/qemu-cpu/compare/qom-cpu-x86).  It seems
> > > > > > > > > sensible enough as far as it goes, but doesn't seem to address a bunch
> > > > > > > > > of the things that I was attempting to do with the cpu-package
> > > > > > > > > proposal - and which we absolutely need for cpu hotplug on Power.
> > > > > > > > > 
> > > > > > > > > 1) What interface do you envisage beyond cpu_add?
> > > > > > > > > 
> > > > > > > > > The patches I see just construct extra socket and core objects, but
> > > > > > > > > still control hotplug (for x86) through the cpu_add interface.  That
> > > > > > > > > interface is absolutely unusable on Power, since it operates on a
> > > > > > > > > per-thread basis, whereas the PAPR guest<->host interfaces can only
> > > > > > > > > communicate information at a per-core granularity.
> > > > > > > > > 
> > > > > > > > > 2) When hotplugging at core or socket granularity, where would the
> > > > > > > > >    code to construct the individual thread objects sit?
> > > > > > > > > 
> > > > > > > > > Your series has the construction done in both the machine init path
> > > > > > > > > and the hotplug path.  The latter works because hotplug occurs at
> > > > > > > > > thread granularity.  If we're hotplugging at core or socket
> > > > > > > > > granularity what would do the construct?  The core/socket object
> > > > > > > > > itself (in instance_init?  in realize?); the hotplug handler?
> > > > > > > > > something else?
> > > > > > > > > 
> > > > > > > > > 3) How does the management layer determine what is pluggable?
> > > > > > > > > 
> > > > > > > > > Both the number of pluggable slots, and what it will need to do to
> > > > > > > > > populate them.
> > > > > > > > > 
> > > > > > > > > 4) How do we enforce that toplogies illegal for the platform can't be
> > > > > > > > >    constructed?        
> > > > > > > > 
> > > > > > > > 5) QOM-links
> > > > > > > > 
> > > > > > > > Andreas, You have often talked about setting up links from machine object
> > > > > > > > to the CPU objects. Would the below code correctly capture that idea of
> > > > > > > > yours ?
> > > > > > > > 
> > > > > > > > #define SPAPR_MACHINE_CPU_CORE_PROP "core"
> > > > > > > > 
> > > > > > > > /* MachineClass.init for sPAPR */
> > > > > > > > static void ppc_spapr_init(MachineState *machine)
> > > > > > > > {
> > > > > > > >     sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
> > > > > > > >     int spapr_smp_cores = smp_cpus / smp_threads;
> > > > > > > >     int spapr_max_cores = max_cpus / smp_threads;
> > > > > > > > 
> > > > > > > >     ...
> > > > > > > >     for (i = 0; i < spapr_max_cores; i++) {
> > > > > > > >         Object *obj = object_new(TYPE_SPAPR_CPU_CORE);
> > > > > > > >         sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > > > > > > >         char name[32];
> > > > > > > > 
> > > > > > > >         snprintf(name, sizeof(name), "%s[%d]", SPAPR_MACHINE_CPU_CORE_PROP, i);
> > > > > > > > 
> > > > > > > >         /*
> > > > > > > >          * Create links from machine objects to all possible cores.
> > > > > > > >          */
> > > > > > > >         object_property_add_link(OBJECT(spapr), name, TYPE_SPAPR_CPU_CORE,
> > > > > > > >                                  (Object **)&spapr->core[i],
> > > > > > > >                                  NULL, NULL, &error_abort); 
> > > > > > > > 
> > > > > > > >         /*
> > > > > > > >          * Set the QOM link from machine object to core object for all
> > > > > > > >          * boot time CPUs specified with -smp. For rest of the hotpluggable
> > > > > > > >          * cores this is done from the core hotplug path.
> > > > > > > >          */
> > > > > > > >         if (i < spapr_smp_cores) {
> > > > > > > >             object_property_set_link(OBJECT(spapr), OBJECT(core),
> > > > > > > >                                      SPAPR_MACHINE_CPU_CORE_PROP, &error_abort);        
> > > > > > > 
> > > > > > > I hope we can at least have a helper function to both construct the
> > > > > > > core and create the links, if we can't handle the link creation in the
> > > > > > > core object itself.
> > > > > > > 
> > > > > > > Having to open-code it in each machine sounds like a recipe for subtle
> > > > > > > differences in presentation between platforms, which is exactly what
> > > > > > > we want to avoid.      
> > > > > > Creating links doesn't give us much, it's just adds means for mgmt
> > > > > > to check how many CPUs could be hotplugged  without keeping that
> > > > > > state in mgmt like it's now, so links are mostly useless if one
> > > > > > care where CPU is being plugged in.
> > > > > > The rest like enumerating exiting CPUs could be done by
> > > > > > traversing QOM tree, links would just simplify finding
> > > > > > CPUs putting them at fixed namespace.      
> > > > > 
> > > > > Simplifying finding CPUs is pretty much all we intended the links for.    
> > > > Do mgmt really needs it? For machine it's easy to find CPUs under
> > > > /machine/[peripheral|unattached] by enumerating entries over there.
> > > > For human, one would need to implement a dedicated HMP command that
> > > > would do the same, so it doesn't really matter where links are
> > > > located.    
> > > 
> > > If we require management to go searching the whole device tree for
> > > cpus, I'm concerned they'll just assume they're in the x86 location
> > > instead, and we'll have to fix it over and over for every platform
> > > that puts them somewhere different.  
> > CPUs are inherited from Device so inherited behaviour is that they
> > are pretty much at fixed location /machine/[peripheral|unattached]
> > regardless of platform QOM tree wise, like every other device.  
> 
> Hmm.. that's true now, but I can see reasons you might want to put
> CPUs on a different bus in future.  In particular consider a machine
> type modelling real hardware for a modern multisocket machine - these
> are often built from several chips on a common fabric, each containing
> several CPU cores, but also other peripherals and bus bridges.
yes, currently QOM tree doesn't express device models as composition tree
or as bus tree but in future it might. How it will be in the future
it's hard to say that's one of the reasons I prefer QMP vs qom-get/set
interface as it abstracts us from it.