| Message ID | 1467903025-13383-4-git-send-email-bharata@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 7 Jul 2016 20:20:23 +0530 Bharata B Rao <bharata@linux.vnet.ibm.com> wrote: > Conditonally set stable_cpu_id for CPU threads that are created as part > of spapr CPU cores. The use of stable_cpu_id is enabled for pseries-2.7 > onwards. > The last sentence is a bit confusing since the enablement actually happens in patch 5/5. > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > --- > hw/ppc/spapr_cpu_core.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > index b104778..0ec3513 100644 > --- a/hw/ppc/spapr_cpu_core.c > +++ b/hw/ppc/spapr_cpu_core.c > @@ -293,8 +293,15 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp) > for (i = 0; i < cc->nr_threads; i++) { > char id[32]; > obj = sc->threads + i * size; > + CPUState *cs; > > object_initialize(obj, size, typename); > + cs = CPU(obj); > + > + /* Use core_id (which is actually cpu_dt_id) as stable CPU id */ It isn't what I had in mind. More something like below: In ppc_spapr_init(): for (i = 0; i < spapr_max_cores; i++) { spapr->cores[i]->stable_id = i * smp_threads; } In spapr_cpu_core_realize(): for (j = 0; j < cc->nr_threads; j++) { stable_cpu_id = cc->stable_id + j; } So we need to introduce cc->stable_id. I think stable_cpu_id is the prerequisite to compute both cpu_dt_id and instance_id. Makes sense ? > + if (cs->has_stable_cpu_id) { > + cs->stable_cpu_id = cc->core_id + i; > + } > snprintf(id, sizeof(id), "thread[%d]", i); > object_property_add_child(OBJECT(sc), id, obj, &local_err); > if (local_err) {
On Thu, Jul 07, 2016 at 08:20:23PM +0530, Bharata B Rao wrote: > Conditonally set stable_cpu_id for CPU threads that are created as part > of spapr CPU cores. The use of stable_cpu_id is enabled for pseries-2.7 > onwards. > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > --- > hw/ppc/spapr_cpu_core.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > index b104778..0ec3513 100644 > --- a/hw/ppc/spapr_cpu_core.c > +++ b/hw/ppc/spapr_cpu_core.c > @@ -293,8 +293,15 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp) > for (i = 0; i < cc->nr_threads; i++) { > char id[32]; > obj = sc->threads + i * size; > + CPUState *cs; > > object_initialize(obj, size, typename); > + cs = CPU(obj); > + > + /* Use core_id (which is actually cpu_dt_id) as stable CPU id */ > + if (cs->has_stable_cpu_id) { > + cs->stable_cpu_id = cc->core_id + i; > + } Testing cs->has_stable_cpu_id here in machine type specific code seems really weird. It's the machine type that knows whether it has a stable ID to give to the CPU or not, rather than the other way around. Since we haven't yet had a release with cpu cores, I think the right thing is for cpu_core to unconditionally set the stable ID (and set has_stable_id to true). The backup path that does thread-based cpu init, can set has_stable_id to false (if that's not the default). > snprintf(id, sizeof(id), "thread[%d]", i); > object_property_add_child(OBJECT(sc), id, obj, &local_err); > if (local_err) {
On Thu, Jul 07, 2016 at 06:11:31PM +0200, Greg Kurz wrote: > On Thu, 7 Jul 2016 20:20:23 +0530 > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote: > > > Conditonally set stable_cpu_id for CPU threads that are created as part > > of spapr CPU cores. The use of stable_cpu_id is enabled for pseries-2.7 > > onwards. > > > > The last sentence is a bit confusing since the enablement actually happens > in patch 5/5. > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > --- > > hw/ppc/spapr_cpu_core.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > > index b104778..0ec3513 100644 > > --- a/hw/ppc/spapr_cpu_core.c > > +++ b/hw/ppc/spapr_cpu_core.c > > @@ -293,8 +293,15 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp) > > for (i = 0; i < cc->nr_threads; i++) { > > char id[32]; > > obj = sc->threads + i * size; > > + CPUState *cs; > > > > object_initialize(obj, size, typename); > > + cs = CPU(obj); > > + > > + /* Use core_id (which is actually cpu_dt_id) as stable CPU id */ > > It isn't what I had in mind. More something like below: > > In ppc_spapr_init(): > > for (i = 0; i < spapr_max_cores; i++) { > spapr->cores[i]->stable_id = i * smp_threads; > } > > > In spapr_cpu_core_realize(): > > for (j = 0; j < cc->nr_threads; j++) { > stable_cpu_id = cc->stable_id + j; > } > > So we need to introduce cc->stable_id. No, we don't. Cores have had a stable ID since they were introduced. Instead we should be setting the thread stable ids based on the core stable id. > I think stable_cpu_id is the prerequisite to compute both cpu_dt_id and instance_id. > > Makes sense ? > > > + if (cs->has_stable_cpu_id) { > > + cs->stable_cpu_id = cc->core_id + i; > > + } > > snprintf(id, sizeof(id), "thread[%d]", i); > > object_property_add_child(OBJECT(sc), id, obj, &local_err); > > if (local_err) { >
On Fri, Jul 08, 2016 at 03:24:13PM +1000, David Gibson wrote: > On Thu, Jul 07, 2016 at 08:20:23PM +0530, Bharata B Rao wrote: > > Conditonally set stable_cpu_id for CPU threads that are created as part > > of spapr CPU cores. The use of stable_cpu_id is enabled for pseries-2.7 > > onwards. > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > --- > > hw/ppc/spapr_cpu_core.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > > index b104778..0ec3513 100644 > > --- a/hw/ppc/spapr_cpu_core.c > > +++ b/hw/ppc/spapr_cpu_core.c > > @@ -293,8 +293,15 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp) > > for (i = 0; i < cc->nr_threads; i++) { > > char id[32]; > > obj = sc->threads + i * size; > > + CPUState *cs; > > > > object_initialize(obj, size, typename); > > + cs = CPU(obj); > > + > > + /* Use core_id (which is actually cpu_dt_id) as stable CPU id */ > > + if (cs->has_stable_cpu_id) { > > + cs->stable_cpu_id = cc->core_id + i; > > + } > > Testing cs->has_stable_cpu_id here in machine type specific code seems > really weird. It's the machine type that knows whether it has a > stable ID to give to the CPU or not, rather than the other way around. > > Since we haven't yet had a release with cpu cores, I think the right > thing is for cpu_core to unconditionally set the stable ID (and set > has_stable_id to true). Right, we can set cpu_stable_id unconditionally here since this code path (core realize) will be taken only for pseries-2.7 onwards. has_stable_id will get set as part of the property we defined in SPAPR_COMPAT_2_7. > The backup path that does thread-based cpu > init, can set has_stable_id to false (if that's not the default). Default is off, but turning it on for 2.7 will be inherited by 2.6 and others below. Hence I have code to explicitly disable this prop for 2.6 and below via SPAPR_COMPAT_2_6. Regards, Bharata.
On Fri, Jul 08, 2016 at 12:11:12PM +0530, Bharata B Rao wrote: > On Fri, Jul 08, 2016 at 03:24:13PM +1000, David Gibson wrote: > > On Thu, Jul 07, 2016 at 08:20:23PM +0530, Bharata B Rao wrote: > > > Conditonally set stable_cpu_id for CPU threads that are created as part > > > of spapr CPU cores. The use of stable_cpu_id is enabled for pseries-2.7 > > > onwards. > > > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > > --- > > > hw/ppc/spapr_cpu_core.c | 7 +++++++ > > > 1 file changed, 7 insertions(+) > > > > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > > > index b104778..0ec3513 100644 > > > --- a/hw/ppc/spapr_cpu_core.c > > > +++ b/hw/ppc/spapr_cpu_core.c > > > @@ -293,8 +293,15 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp) > > > for (i = 0; i < cc->nr_threads; i++) { > > > char id[32]; > > > obj = sc->threads + i * size; > > > + CPUState *cs; > > > > > > object_initialize(obj, size, typename); > > > + cs = CPU(obj); > > > + > > > + /* Use core_id (which is actually cpu_dt_id) as stable CPU id */ > > > + if (cs->has_stable_cpu_id) { > > > + cs->stable_cpu_id = cc->core_id + i; > > > + } > > > > Testing cs->has_stable_cpu_id here in machine type specific code seems > > really weird. It's the machine type that knows whether it has a > > stable ID to give to the CPU or not, rather than the other way around. > > > > Since we haven't yet had a release with cpu cores, I think the right > > thing is for cpu_core to unconditionally set the stable ID (and set > > has_stable_id to true). > > Right, we can set cpu_stable_id unconditionally here since this code path > (core realize) will be taken only for pseries-2.7 onwards. has_stable_id > will get set as part of the property we defined in SPAPR_COMPAT_2_7. Hrm, that's true. But when you describe it like that it sounds like a really non-obvious and fragile dependency between different components. > > The backup path that does thread-based cpu > > init, can set has_stable_id to false (if that's not the default). > > Default is off, but turning it on for 2.7 will be inherited by 2.6 > and others below. Hence I have code to explicitly disable this prop > for 2.6 and below via SPAPR_COMPAT_2_6. This is all seeming terribly awkward. Can we try investigating a different approach: 1. Rename cpu_index to cpu_id, but it's still used in the same places it's used. 2. Remove assumptions that cpu_id values are contiguous or dense 3. Machine type decides whether it wants to populate the cpu_id values explicitly, or leave it to generic code to calculate them as cpu_index is calculated now. 4. Ideally, generic code enforces that the machine type populates either all or none of the cpu_id values. Does that seem workable?
On Fri, 8 Jul 2016 15:25:33 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > On Thu, Jul 07, 2016 at 06:11:31PM +0200, Greg Kurz wrote: > > On Thu, 7 Jul 2016 20:20:23 +0530 > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote: > > > > > Conditonally set stable_cpu_id for CPU threads that are created as part > > > of spapr CPU cores. The use of stable_cpu_id is enabled for pseries-2.7 > > > onwards. > > > > > > > The last sentence is a bit confusing since the enablement actually happens > > in patch 5/5. > > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > > --- > > > hw/ppc/spapr_cpu_core.c | 7 +++++++ > > > 1 file changed, 7 insertions(+) > > > > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > > > index b104778..0ec3513 100644 > > > --- a/hw/ppc/spapr_cpu_core.c > > > +++ b/hw/ppc/spapr_cpu_core.c > > > @@ -293,8 +293,15 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp) > > > for (i = 0; i < cc->nr_threads; i++) { > > > char id[32]; > > > obj = sc->threads + i * size; > > > + CPUState *cs; > > > > > > object_initialize(obj, size, typename); > > > + cs = CPU(obj); > > > + > > > + /* Use core_id (which is actually cpu_dt_id) as stable CPU id */ > > > > It isn't what I had in mind. More something like below: > > > > In ppc_spapr_init(): > > > > for (i = 0; i < spapr_max_cores; i++) { > > spapr->cores[i]->stable_id = i * smp_threads; > > } > > > > > > In spapr_cpu_core_realize(): > > > > for (j = 0; j < cc->nr_threads; j++) { > > stable_cpu_id = cc->stable_id + j; > > } > > > > So we need to introduce cc->stable_id. > > No, we don't. Cores have had a stable ID since they were introduced. > I agree core_dt_id is stable but it is a DT concept. static void ppc_spapr_init(MachineState *machine) { [...] for (i = 0; i < spapr_max_cores; i++) { int core_dt_id = i * smt; [...] object_property_set_int(core, core_dt_id, CPU_CORE_PROP_CORE_ID, &error_fatal); This patch produces stable_cpu_id in the [0...smt * smp_cores) range. I find it awkward it depends on the host setup. I'm suggesting we introduce cc->stable_id to be able to compute a simple stable_cpu_id in the range [0...max_cpus), like x86 and ARM. > Instead we should be setting the thread stable ids based on the core > stable id. > > > I think stable_cpu_id is the prerequisite to compute both cpu_dt_id and instance_id. > > > > Makes sense ? > > > > > + if (cs->has_stable_cpu_id) { > > > + cs->stable_cpu_id = cc->core_id + i; > > > + } > > > snprintf(id, sizeof(id), "thread[%d]", i); > > > object_property_add_child(OBJECT(sc), id, obj, &local_err); > > > if (local_err) { > > >
On Fri, Jul 08, 2016 at 09:46:47AM +0200, Greg Kurz wrote: > On Fri, 8 Jul 2016 15:25:33 +1000 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > On Thu, Jul 07, 2016 at 06:11:31PM +0200, Greg Kurz wrote: > > > On Thu, 7 Jul 2016 20:20:23 +0530 > > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote: > > > > > > > Conditonally set stable_cpu_id for CPU threads that are created as part > > > > of spapr CPU cores. The use of stable_cpu_id is enabled for pseries-2.7 > > > > onwards. > > > > > > > > > > The last sentence is a bit confusing since the enablement actually happens > > > in patch 5/5. > > > > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > > > --- > > > > hw/ppc/spapr_cpu_core.c | 7 +++++++ > > > > 1 file changed, 7 insertions(+) > > > > > > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > > > > index b104778..0ec3513 100644 > > > > --- a/hw/ppc/spapr_cpu_core.c > > > > +++ b/hw/ppc/spapr_cpu_core.c > > > > @@ -293,8 +293,15 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp) > > > > for (i = 0; i < cc->nr_threads; i++) { > > > > char id[32]; > > > > obj = sc->threads + i * size; > > > > + CPUState *cs; > > > > > > > > object_initialize(obj, size, typename); > > > > + cs = CPU(obj); > > > > + > > > > + /* Use core_id (which is actually cpu_dt_id) as stable CPU id */ > > > > > > It isn't what I had in mind. More something like below: > > > > > > In ppc_spapr_init(): > > > > > > for (i = 0; i < spapr_max_cores; i++) { > > > spapr->cores[i]->stable_id = i * smp_threads; > > > } > > > > > > > > > In spapr_cpu_core_realize(): > > > > > > for (j = 0; j < cc->nr_threads; j++) { > > > stable_cpu_id = cc->stable_id + j; > > > } > > > > > > So we need to introduce cc->stable_id. > > > > No, we don't. Cores have had a stable ID since they were introduced. > > > > I agree core_dt_id is stable but it is a DT concept. There is no core_dt_id. There's just core-id, which is machine assigned (via the query hotpluggable cpus interface) and stable. > static void ppc_spapr_init(MachineState *machine) > { > [...] > for (i = 0; i < spapr_max_cores; i++) { > int core_dt_id = i * smt; ..uh, ok, except for that poorly named variable. But that's because this is in the machine type, and it knows it's going to use the same ids to give to the core object and to put in the device tree. > [...] > object_property_set_int(core, core_dt_id, CPU_CORE_PROP_CORE_ID, > &error_fatal); > > This patch produces stable_cpu_id in the [0...smt * smp_cores) range. I find it > awkward it depends on the host setup. True. Possibly we should set these as i * (maximum plausible number of threads). The gotcha is that currently we're using the same "dt_id" to control KVM's cpu id and that in turn controls the SMT level. That's a poor interface on the kernel side (my bad), but we have to live with it now. However we could de-couple that KVM id from the core-id. It'd no doubt cause some complications with kvm-xics, but we can probably handle it. > I'm suggesting we introduce cc->stable_id to be able to compute a simple > stable_cpu_id in the range [0...max_cpus), like x86 and ARM. I really don't see what properties this is supposed to have that are different from the existing core-id. > > > Instead we should be setting the thread stable ids based on the core > > stable id. > > > > > I think stable_cpu_id is the prerequisite to compute both cpu_dt_id and instance_id. > > > > > > Makes sense ? > > > > > > > + if (cs->has_stable_cpu_id) { > > > > + cs->stable_cpu_id = cc->core_id + i; > > > > + } > > > > snprintf(id, sizeof(id), "thread[%d]", i); > > > > object_property_add_child(OBJECT(sc), id, obj, &local_err); > > > > if (local_err) { > > > > > >
On Fri, 8 Jul 2016 17:39:52 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > On Fri, Jul 08, 2016 at 12:11:12PM +0530, Bharata B Rao wrote: > > On Fri, Jul 08, 2016 at 03:24:13PM +1000, David Gibson wrote: > > > On Thu, Jul 07, 2016 at 08:20:23PM +0530, Bharata B Rao wrote: > > > > Conditonally set stable_cpu_id for CPU threads that are created as part > > > > of spapr CPU cores. The use of stable_cpu_id is enabled for pseries-2.7 > > > > onwards. > > > > > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > > > --- > > > > hw/ppc/spapr_cpu_core.c | 7 +++++++ > > > > 1 file changed, 7 insertions(+) > > > > > > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > > > > index b104778..0ec3513 100644 > > > > --- a/hw/ppc/spapr_cpu_core.c > > > > +++ b/hw/ppc/spapr_cpu_core.c > > > > @@ -293,8 +293,15 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp) > > > > for (i = 0; i < cc->nr_threads; i++) { > > > > char id[32]; > > > > obj = sc->threads + i * size; > > > > + CPUState *cs; > > > > > > > > object_initialize(obj, size, typename); > > > > + cs = CPU(obj); > > > > + > > > > + /* Use core_id (which is actually cpu_dt_id) as stable CPU id */ > > > > + if (cs->has_stable_cpu_id) { > > > > + cs->stable_cpu_id = cc->core_id + i; > > > > + } > > > > > > Testing cs->has_stable_cpu_id here in machine type specific code seems > > > really weird. It's the machine type that knows whether it has a > > > stable ID to give to the CPU or not, rather than the other way around. > > > > > > Since we haven't yet had a release with cpu cores, I think the right > > > thing is for cpu_core to unconditionally set the stable ID (and set > > > has_stable_id to true). > > > > Right, we can set cpu_stable_id unconditionally here since this code path > > (core realize) will be taken only for pseries-2.7 onwards. has_stable_id > > will get set as part of the property we defined in SPAPR_COMPAT_2_7. > > Hrm, that's true. But when you describe it like that it sounds like a > really non-obvious and fragile dependency between different components. that's how compat stuff is typically done for devices, CPUs shouldn't be an exception. (consistency with other devices helps here in long run) > > > The backup path that does thread-based cpu > > > init, can set has_stable_id to false (if that's not the default). > > > > Default is off, but turning it on for 2.7 will be inherited by 2.6 > > and others below. Hence I have code to explicitly disable this prop > > for 2.6 and below via SPAPR_COMPAT_2_6. > > This is all seeming terribly awkward. Typically default is set the way so new machine type doesn't have to enable it explicitly. However the way it's done here helps not to touch/check every user that uses cpu_index, limiting series impact only to code that asks for it, it look a lot safer to got this rout for now. > Can we try investigating a > different approach: > > 1. Rename cpu_index to cpu_id, but it's still used in the same > places it's used. > > 2. Remove assumptions that cpu_id values are contiguous or > dense > > 3. Machine type decides whether it wants to populate the cpu_id > values explicitly, or leave it to generic code to calculate > them as cpu_index is calculated now. > > 4. Ideally, generic code enforces that the machine type populates > either all or none of the cpu_id values. > > Does that seem workable? Ideally we will get there some day (and may be get rid of cpu_index altogether), but for now it seems too invasive with a lot of chances to introduce non obvious regression. So I'd keep approach used in this series.
On Fri, 8 Jul 2016 17:59:07 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > On Fri, Jul 08, 2016 at 09:46:47AM +0200, Greg Kurz wrote: > > On Fri, 8 Jul 2016 15:25:33 +1000 > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > On Thu, Jul 07, 2016 at 06:11:31PM +0200, Greg Kurz wrote: > > > > On Thu, 7 Jul 2016 20:20:23 +0530 > > > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote: > > > > > > > > > Conditonally set stable_cpu_id for CPU threads that are created as part > > > > > of spapr CPU cores. The use of stable_cpu_id is enabled for pseries-2.7 > > > > > onwards. > > > > > > > > > > > > > The last sentence is a bit confusing since the enablement actually happens > > > > in patch 5/5. > > > > > > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > > > > --- > > > > > hw/ppc/spapr_cpu_core.c | 7 +++++++ > > > > > 1 file changed, 7 insertions(+) > > > > > > > > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > > > > > index b104778..0ec3513 100644 > > > > > --- a/hw/ppc/spapr_cpu_core.c > > > > > +++ b/hw/ppc/spapr_cpu_core.c > > > > > @@ -293,8 +293,15 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp) > > > > > for (i = 0; i < cc->nr_threads; i++) { > > > > > char id[32]; > > > > > obj = sc->threads + i * size; > > > > > + CPUState *cs; > > > > > > > > > > object_initialize(obj, size, typename); > > > > > + cs = CPU(obj); > > > > > + > > > > > + /* Use core_id (which is actually cpu_dt_id) as stable CPU id */ > > > > > > > > It isn't what I had in mind. More something like below: > > > > > > > > In ppc_spapr_init(): > > > > > > > > for (i = 0; i < spapr_max_cores; i++) { > > > > spapr->cores[i]->stable_id = i * smp_threads; > > > > } > > > > > > > > > > > > In spapr_cpu_core_realize(): > > > > > > > > for (j = 0; j < cc->nr_threads; j++) { > > > > stable_cpu_id = cc->stable_id + j; > > > > } > > > > > > > > So we need to introduce cc->stable_id. > > > > > > No, we don't. Cores have had a stable ID since they were introduced. > > > > > > > I agree core_dt_id is stable but it is a DT concept. > > There is no core_dt_id. There's just core-id, which is machine > assigned (via the query hotpluggable cpus interface) and stable. > > > static void ppc_spapr_init(MachineState *machine) > > { > > [...] > > for (i = 0; i < spapr_max_cores; i++) { > > int core_dt_id = i * smt; > > ..uh, ok, except for that poorly named variable. But that's because > this is in the machine type, and it knows it's going to use the same > ids to give to the core object and to put in the device tree. > It is core_id everywhere else. $ git grep core_id hw/ppc/ hw/ppc/spapr.c: cpu_props->has_core_id = true; hw/ppc/spapr.c: cpu_props->core_id = i * smt; hw/ppc/spapr_cpu_core.c: spapr->cores[cc->core_id / smt] = NULL; hw/ppc/spapr_cpu_core.c: drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, cc->core_id); hw/ppc/spapr_cpu_core.c: index = cc->core_id / smt; hw/ppc/spapr_cpu_core.c: if (cc->core_id % smt) { hw/ppc/spapr_cpu_core.c: error_setg(&local_err, "invalid core id %d\n", cc->core_id); hw/ppc/spapr_cpu_core.c: index = cc->core_id / smt; hw/ppc/spapr_cpu_core.c: error_setg(&local_err, "core id %d out of range", cc->core_id); hw/ppc/spapr_cpu_core.c: error_setg(&local_err, "core %d already populated", cc->core_id); $ git grep core_dt_id hw/ppc/ hw/ppc/spapr.c: int core_dt_id = i * smt; hw/ppc/spapr.c: SPAPR_DR_CONNECTOR_TYPE_CPU, core_dt_id); hw/ppc/spapr.c: object_property_set_int(core, core_dt_id, CPU_CORE_PROP_CORE_ID, I got confused because the current code still puts cpu_dt_id of thread0 in the device tree. And since cpu_dt_id is still being computed on cpu_index, it is a different beast (which needs to be killed since it even crashes simple hotplug/unplug scenarios). > > [...] > > object_property_set_int(core, core_dt_id, CPU_CORE_PROP_CORE_ID, > > &error_fatal); > > > > This patch produces stable_cpu_id in the [0...smt * smp_cores) range. I find it > > awkward it depends on the host setup. > > True. Possibly we should set these as i * (maximum plausible number > of threads). > > The gotcha is that currently we're using the same "dt_id" to control > KVM's cpu id and that in turn controls the SMT level. That's a poor > interface on the kernel side (my bad), but we have to live with it > now. However we could de-couple that KVM id from the core-id. It'd > no doubt cause some complications with kvm-xics, but we can probably > handle it. > > > I'm suggesting we introduce cc->stable_id to be able to compute a simple > > stable_cpu_id in the range [0...max_cpus), like x86 and ARM. > > I really don't see what properties this is supposed to have that are > different from the existing core-id. > Simplicity of always having CPU0, 1, 2, 3... max_cpus in QEMU, and try to hide the "poor interface on the kernel side" from the code that does not need it... but maybe that is not that important. > > > > > Instead we should be setting the thread stable ids based on the core > > > stable id. > > > > > > > I think stable_cpu_id is the prerequisite to compute both cpu_dt_id and instance_id. > > > > > > > > Makes sense ? > > > > > > > > > + if (cs->has_stable_cpu_id) { > > > > > + cs->stable_cpu_id = cc->core_id + i; > > > > > + } > > > > > snprintf(id, sizeof(id), "thread[%d]", i); > > > > > object_property_add_child(OBJECT(sc), id, obj, &local_err); > > > > > if (local_err) { > > > > > > > > > > > >
On Fri, Jul 08, 2016 at 12:59:59PM +0200, Igor Mammedov wrote: > On Fri, 8 Jul 2016 17:39:52 +1000 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > On Fri, Jul 08, 2016 at 12:11:12PM +0530, Bharata B Rao wrote: > > > On Fri, Jul 08, 2016 at 03:24:13PM +1000, David Gibson wrote: > > > > On Thu, Jul 07, 2016 at 08:20:23PM +0530, Bharata B Rao wrote: > > > > > Conditonally set stable_cpu_id for CPU threads that are created as part > > > > > of spapr CPU cores. The use of stable_cpu_id is enabled for pseries-2.7 > > > > > onwards. > > > > > > > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > > > > --- > > > > > hw/ppc/spapr_cpu_core.c | 7 +++++++ > > > > > 1 file changed, 7 insertions(+) > > > > > > > > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > > > > > index b104778..0ec3513 100644 > > > > > --- a/hw/ppc/spapr_cpu_core.c > > > > > +++ b/hw/ppc/spapr_cpu_core.c > > > > > @@ -293,8 +293,15 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp) > > > > > for (i = 0; i < cc->nr_threads; i++) { > > > > > char id[32]; > > > > > obj = sc->threads + i * size; > > > > > + CPUState *cs; > > > > > > > > > > object_initialize(obj, size, typename); > > > > > + cs = CPU(obj); > > > > > + > > > > > + /* Use core_id (which is actually cpu_dt_id) as stable CPU id */ > > > > > + if (cs->has_stable_cpu_id) { > > > > > + cs->stable_cpu_id = cc->core_id + i; > > > > > + } > > > > > > > > Testing cs->has_stable_cpu_id here in machine type specific code seems > > > > really weird. It's the machine type that knows whether it has a > > > > stable ID to give to the CPU or not, rather than the other way around. > > > > > > > > Since we haven't yet had a release with cpu cores, I think the right > > > > thing is for cpu_core to unconditionally set the stable ID (and set > > > > has_stable_id to true). > > > > > > Right, we can set cpu_stable_id unconditionally here since this code path > > > (core realize) will be taken only for pseries-2.7 onwards. has_stable_id > > > will get set as part of the property we defined in SPAPR_COMPAT_2_7. > > > > Hrm, that's true. But when you describe it like that it sounds like a > > really non-obvious and fragile dependency between different components. > that's how compat stuff is typically done for devices, > CPUs shouldn't be an exception. > (consistency with other devices helps here in long run) > > > > > The backup path that does thread-based cpu > > > > init, can set has_stable_id to false (if that's not the default). > > > > > > Default is off, but turning it on for 2.7 will be inherited by 2.6 > > > and others below. Hence I have code to explicitly disable this prop > > > for 2.6 and below via SPAPR_COMPAT_2_6. > > > > This is all seeming terribly awkward. > Typically default is set the way so new machine type doesn't have > to enable it explicitly. > > However the way it's done here helps not to touch/check every user > that uses cpu_index, limiting series impact only to code that > asks for it, it look a lot safer to got this rout for now. David, - I believe there's a consensus on using core-id as the stable_cpu_id. - You weren't liking the use of a separate property user-stable-cpu-id to control/enable the use of stable_cpu_id. After Igor's reply above, should we stick with that approach ? - I am planning to drop the code that introduces cpu_common_unrealize() and that moves vmstate_[un]register() calls to qom/cpu.c as that affects all other archs. Instead lets just check for use_stable_cpu_id from exec.c itself and use it appropriately. If you are ok with all the above, I shall post the next version on top of Greg's patchset. Regards, Bharata.
On Fri, Jul 08, 2016 at 05:24:24PM +0200, Greg Kurz wrote: > On Fri, 8 Jul 2016 17:59:07 +1000 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > On Fri, Jul 08, 2016 at 09:46:47AM +0200, Greg Kurz wrote: > > > On Fri, 8 Jul 2016 15:25:33 +1000 > > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > > > On Thu, Jul 07, 2016 at 06:11:31PM +0200, Greg Kurz wrote: > > > > > On Thu, 7 Jul 2016 20:20:23 +0530 > > > > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote: > > > > > > > > > > > Conditonally set stable_cpu_id for CPU threads that are created as part > > > > > > of spapr CPU cores. The use of stable_cpu_id is enabled for pseries-2.7 > > > > > > onwards. > > > > > > > > > > > > > > > > The last sentence is a bit confusing since the enablement actually happens > > > > > in patch 5/5. > > > > > > > > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > > > > > --- > > > > > > hw/ppc/spapr_cpu_core.c | 7 +++++++ > > > > > > 1 file changed, 7 insertions(+) > > > > > > > > > > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > > > > > > index b104778..0ec3513 100644 > > > > > > --- a/hw/ppc/spapr_cpu_core.c > > > > > > +++ b/hw/ppc/spapr_cpu_core.c > > > > > > @@ -293,8 +293,15 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp) > > > > > > for (i = 0; i < cc->nr_threads; i++) { > > > > > > char id[32]; > > > > > > obj = sc->threads + i * size; > > > > > > + CPUState *cs; > > > > > > > > > > > > object_initialize(obj, size, typename); > > > > > > + cs = CPU(obj); > > > > > > + > > > > > > + /* Use core_id (which is actually cpu_dt_id) as stable CPU id */ > > > > > > > > > > It isn't what I had in mind. More something like below: > > > > > > > > > > In ppc_spapr_init(): > > > > > > > > > > for (i = 0; i < spapr_max_cores; i++) { > > > > > spapr->cores[i]->stable_id = i * smp_threads; > > > > > } > > > > > > > > > > > > > > > In spapr_cpu_core_realize(): > > > > > > > > > > for (j = 0; j < cc->nr_threads; j++) { > > > > > stable_cpu_id = cc->stable_id + j; > > > > > } > > > > > > > > > > So we need to introduce cc->stable_id. > > > > > > > > No, we don't. Cores have had a stable ID since they were introduced. > > > > > > > > > > I agree core_dt_id is stable but it is a DT concept. > > > > There is no core_dt_id. There's just core-id, which is machine > > assigned (via the query hotpluggable cpus interface) and stable. > > > > > static void ppc_spapr_init(MachineState *machine) > > > { > > > [...] > > > for (i = 0; i < spapr_max_cores; i++) { > > > int core_dt_id = i * smt; > > > > ..uh, ok, except for that poorly named variable. But that's because > > this is in the machine type, and it knows it's going to use the same > > ids to give to the core object and to put in the device tree. > > > > It is core_id everywhere else. > > $ git grep core_id hw/ppc/ > hw/ppc/spapr.c: cpu_props->has_core_id = true; > hw/ppc/spapr.c: cpu_props->core_id = i * smt; > hw/ppc/spapr_cpu_core.c: spapr->cores[cc->core_id / smt] = NULL; > hw/ppc/spapr_cpu_core.c: drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, cc->core_id); > hw/ppc/spapr_cpu_core.c: index = cc->core_id / smt; > hw/ppc/spapr_cpu_core.c: if (cc->core_id % smt) { > hw/ppc/spapr_cpu_core.c: error_setg(&local_err, "invalid core id %d\n", cc->core_id); > hw/ppc/spapr_cpu_core.c: index = cc->core_id / smt; > hw/ppc/spapr_cpu_core.c: error_setg(&local_err, "core id %d out of range", cc->core_id); > hw/ppc/spapr_cpu_core.c: error_setg(&local_err, "core %d already populated", cc->core_id); > > $ git grep core_dt_id hw/ppc/ > hw/ppc/spapr.c: int core_dt_id = i * smt; > hw/ppc/spapr.c: SPAPR_DR_CONNECTOR_TYPE_CPU, core_dt_id); > hw/ppc/spapr.c: object_property_set_int(core, core_dt_id, CPU_CORE_PROP_CORE_ID, > > I got confused because the current code still puts cpu_dt_id of thread0 in the > device tree. And since cpu_dt_id is still being computed on cpu_index, it is > a different beast (which needs to be killed since it even crashes simple > hotplug/unplug scenarios). > > > > [...] > > > object_property_set_int(core, core_dt_id, CPU_CORE_PROP_CORE_ID, > > > &error_fatal); > > > > > > This patch produces stable_cpu_id in the [0...smt * smp_cores) range. I find it > > > awkward it depends on the host setup. > > > > True. Possibly we should set these as i * (maximum plausible number > > of threads). > > > > The gotcha is that currently we're using the same "dt_id" to control > > KVM's cpu id and that in turn controls the SMT level. That's a poor > > interface on the kernel side (my bad), but we have to live with it > > now. However we could de-couple that KVM id from the core-id. It'd > > no doubt cause some complications with kvm-xics, but we can probably > > handle it. > > > > > I'm suggesting we introduce cc->stable_id to be able to compute a simple > > > stable_cpu_id in the range [0...max_cpus), like x86 and ARM. > > > > I really don't see what properties this is supposed to have that are > > different from the existing core-id. > > > > Simplicity of always having CPU0, 1, 2, 3... max_cpus in QEMU, and try > to hide the "poor interface on the kernel side" from the code that > does not need it... but maybe that is not that important. Merely having contiguous values seems like a very poor trade off for having two different IDs for the same object to get confused between.
On Fri, Jul 08, 2016 at 12:59:59PM +0200, Igor Mammedov wrote: > On Fri, 8 Jul 2016 17:39:52 +1000 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > On Fri, Jul 08, 2016 at 12:11:12PM +0530, Bharata B Rao wrote: > > > On Fri, Jul 08, 2016 at 03:24:13PM +1000, David Gibson wrote: > > > > On Thu, Jul 07, 2016 at 08:20:23PM +0530, Bharata B Rao wrote: > > > > > Conditonally set stable_cpu_id for CPU threads that are created as part > > > > > of spapr CPU cores. The use of stable_cpu_id is enabled for pseries-2.7 > > > > > onwards. > > > > > > > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > > > > --- > > > > > hw/ppc/spapr_cpu_core.c | 7 +++++++ > > > > > 1 file changed, 7 insertions(+) > > > > > > > > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > > > > > index b104778..0ec3513 100644 > > > > > --- a/hw/ppc/spapr_cpu_core.c > > > > > +++ b/hw/ppc/spapr_cpu_core.c > > > > > @@ -293,8 +293,15 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp) > > > > > for (i = 0; i < cc->nr_threads; i++) { > > > > > char id[32]; > > > > > obj = sc->threads + i * size; > > > > > + CPUState *cs; > > > > > > > > > > object_initialize(obj, size, typename); > > > > > + cs = CPU(obj); > > > > > + > > > > > + /* Use core_id (which is actually cpu_dt_id) as stable CPU id */ > > > > > + if (cs->has_stable_cpu_id) { > > > > > + cs->stable_cpu_id = cc->core_id + i; > > > > > + } > > > > > > > > Testing cs->has_stable_cpu_id here in machine type specific code seems > > > > really weird. It's the machine type that knows whether it has a > > > > stable ID to give to the CPU or not, rather than the other way around. > > > > > > > > Since we haven't yet had a release with cpu cores, I think the right > > > > thing is for cpu_core to unconditionally set the stable ID (and set > > > > has_stable_id to true). > > > > > > Right, we can set cpu_stable_id unconditionally here since this code path > > > (core realize) will be taken only for pseries-2.7 onwards. has_stable_id > > > will get set as part of the property we defined in SPAPR_COMPAT_2_7. > > > > Hrm, that's true. But when you describe it like that it sounds like a > > really non-obvious and fragile dependency between different components. > that's how compat stuff is typically done for devices, > CPUs shouldn't be an exception. > (consistency with other devices helps here in long run) > > > > > The backup path that does thread-based cpu > > > > init, can set has_stable_id to false (if that's not the default). > > > > > > Default is off, but turning it on for 2.7 will be inherited by 2.6 > > > and others below. Hence I have code to explicitly disable this prop > > > for 2.6 and below via SPAPR_COMPAT_2_6. > > > > This is all seeming terribly awkward. > Typically default is set the way so new machine type doesn't have > to enable it explicitly. > > However the way it's done here helps not to touch/check every user > that uses cpu_index, limiting series impact only to code that > asks for it, it look a lot safer to got this rout for now. > > > > Can we try investigating a > > different approach: > > > > 1. Rename cpu_index to cpu_id, but it's still used in the same > > places it's used. > > > > 2. Remove assumptions that cpu_id values are contiguous or > > dense > > > > 3. Machine type decides whether it wants to populate the cpu_id > > values explicitly, or leave it to generic code to calculate > > them as cpu_index is calculated now. > > > > 4. Ideally, generic code enforces that the machine type populates > > either all or none of the cpu_id values. > > > > Does that seem workable? > Ideally we will get there some day (and may be get rid of cpu_index altogether), > but for now it seems too invasive with a lot of chances to introduce non obvious > regression. Yes, that's a risk. But I'm basically no longer convinced that it's any higher than the risk of the same thing with the current approach. > So I'd keep approach used in this series.
On Mon, 11 Jul 2016 13:26:01 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > On Fri, Jul 08, 2016 at 12:59:59PM +0200, Igor Mammedov wrote: > > On Fri, 8 Jul 2016 17:39:52 +1000 > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > On Fri, Jul 08, 2016 at 12:11:12PM +0530, Bharata B Rao wrote: > > > > On Fri, Jul 08, 2016 at 03:24:13PM +1000, David Gibson wrote: > > > > > On Thu, Jul 07, 2016 at 08:20:23PM +0530, Bharata B Rao wrote: > > > > > > Conditonally set stable_cpu_id for CPU threads that are created as part > > > > > > of spapr CPU cores. The use of stable_cpu_id is enabled for pseries-2.7 > > > > > > onwards. > > > > > > > > > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > > > > > --- > > > > > > hw/ppc/spapr_cpu_core.c | 7 +++++++ > > > > > > 1 file changed, 7 insertions(+) > > > > > > > > > > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > > > > > > index b104778..0ec3513 100644 > > > > > > --- a/hw/ppc/spapr_cpu_core.c > > > > > > +++ b/hw/ppc/spapr_cpu_core.c > > > > > > @@ -293,8 +293,15 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp) > > > > > > for (i = 0; i < cc->nr_threads; i++) { > > > > > > char id[32]; > > > > > > obj = sc->threads + i * size; > > > > > > + CPUState *cs; > > > > > > > > > > > > object_initialize(obj, size, typename); > > > > > > + cs = CPU(obj); > > > > > > + > > > > > > + /* Use core_id (which is actually cpu_dt_id) as stable CPU id */ > > > > > > + if (cs->has_stable_cpu_id) { > > > > > > + cs->stable_cpu_id = cc->core_id + i; > > > > > > + } > > > > > > > > > > Testing cs->has_stable_cpu_id here in machine type specific code seems > > > > > really weird. It's the machine type that knows whether it has a > > > > > stable ID to give to the CPU or not, rather than the other way around. > > > > > > > > > > Since we haven't yet had a release with cpu cores, I think the right > > > > > thing is for cpu_core to unconditionally set the stable ID (and set > > > > > has_stable_id to true). > > > > > > > > Right, we can set cpu_stable_id unconditionally here since this code path > > > > (core realize) will be taken only for pseries-2.7 onwards. has_stable_id > > > > will get set as part of the property we defined in SPAPR_COMPAT_2_7. > > > > > > Hrm, that's true. But when you describe it like that it sounds like a > > > really non-obvious and fragile dependency between different components. > > that's how compat stuff is typically done for devices, > > CPUs shouldn't be an exception. > > (consistency with other devices helps here in long run) > > > > > > > The backup path that does thread-based cpu > > > > > init, can set has_stable_id to false (if that's not the default). > > > > > > > > Default is off, but turning it on for 2.7 will be inherited by 2.6 > > > > and others below. Hence I have code to explicitly disable this prop > > > > for 2.6 and below via SPAPR_COMPAT_2_6. > > > > > > This is all seeming terribly awkward. > > Typically default is set the way so new machine type doesn't have > > to enable it explicitly. > > > > However the way it's done here helps not to touch/check every user > > that uses cpu_index, limiting series impact only to code that > > asks for it, it look a lot safer to got this rout for now. > > > > > > > Can we try investigating a > > > different approach: > > > > > > 1. Rename cpu_index to cpu_id, but it's still used in the same > > > places it's used. > > > > > > 2. Remove assumptions that cpu_id values are contiguous or > > > dense > > > > > > 3. Machine type decides whether it wants to populate the cpu_id > > > values explicitly, or leave it to generic code to calculate > > > them as cpu_index is calculated now. > > > > > > 4. Ideally, generic code enforces that the machine type populates > > > either all or none of the cpu_id values. > > > > > > Does that seem workable? > > Ideally we will get there some day (and may be get rid of cpu_index altogether), > > but for now it seems too invasive with a lot of chances to introduce non obvious > > regression. > > Yes, that's a risk. But I'm basically no longer convinced that it's > any higher than the risk of the same thing with the current approach. to me it looks much less risky as it affects only migration path and could be done in time for 2.7 if we go to cpu_index re-factoring path then I'm afraid it quite a bit more complex and it's not a 2.7 material. > > > So I'd keep approach used in this series. >
On Mon, Jul 11, 2016 at 10:15:57AM +0200, Igor Mammedov wrote: > On Mon, 11 Jul 2016 13:26:01 +1000 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > On Fri, Jul 08, 2016 at 12:59:59PM +0200, Igor Mammedov wrote: > > > On Fri, 8 Jul 2016 17:39:52 +1000 > > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > > > On Fri, Jul 08, 2016 at 12:11:12PM +0530, Bharata B Rao wrote: > > > > > On Fri, Jul 08, 2016 at 03:24:13PM +1000, David Gibson wrote: > > > > > > On Thu, Jul 07, 2016 at 08:20:23PM +0530, Bharata B Rao wrote: > > > > > > > Conditonally set stable_cpu_id for CPU threads that are created as part > > > > > > > of spapr CPU cores. The use of stable_cpu_id is enabled for pseries-2.7 > > > > > > > onwards. > > > > > > > > > > > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > > > > > > --- > > > > > > > hw/ppc/spapr_cpu_core.c | 7 +++++++ > > > > > > > 1 file changed, 7 insertions(+) > > > > > > > > > > > > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > > > > > > > index b104778..0ec3513 100644 > > > > > > > --- a/hw/ppc/spapr_cpu_core.c > > > > > > > +++ b/hw/ppc/spapr_cpu_core.c > > > > > > > @@ -293,8 +293,15 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp) > > > > > > > for (i = 0; i < cc->nr_threads; i++) { > > > > > > > char id[32]; > > > > > > > obj = sc->threads + i * size; > > > > > > > + CPUState *cs; > > > > > > > > > > > > > > object_initialize(obj, size, typename); > > > > > > > + cs = CPU(obj); > > > > > > > + > > > > > > > + /* Use core_id (which is actually cpu_dt_id) as stable CPU id */ > > > > > > > + if (cs->has_stable_cpu_id) { > > > > > > > + cs->stable_cpu_id = cc->core_id + i; > > > > > > > + } > > > > > > > > > > > > Testing cs->has_stable_cpu_id here in machine type specific code seems > > > > > > really weird. It's the machine type that knows whether it has a > > > > > > stable ID to give to the CPU or not, rather than the other way around. > > > > > > > > > > > > Since we haven't yet had a release with cpu cores, I think the right > > > > > > thing is for cpu_core to unconditionally set the stable ID (and set > > > > > > has_stable_id to true). > > > > > > > > > > Right, we can set cpu_stable_id unconditionally here since this code path > > > > > (core realize) will be taken only for pseries-2.7 onwards. has_stable_id > > > > > will get set as part of the property we defined in SPAPR_COMPAT_2_7. > > > > > > > > Hrm, that's true. But when you describe it like that it sounds like a > > > > really non-obvious and fragile dependency between different components. > > > that's how compat stuff is typically done for devices, > > > CPUs shouldn't be an exception. > > > (consistency with other devices helps here in long run) > > > > > > > > > The backup path that does thread-based cpu > > > > > > init, can set has_stable_id to false (if that's not the default). > > > > > > > > > > Default is off, but turning it on for 2.7 will be inherited by 2.6 > > > > > and others below. Hence I have code to explicitly disable this prop > > > > > for 2.6 and below via SPAPR_COMPAT_2_6. > > > > > > > > This is all seeming terribly awkward. > > > Typically default is set the way so new machine type doesn't have > > > to enable it explicitly. > > > > > > However the way it's done here helps not to touch/check every user > > > that uses cpu_index, limiting series impact only to code that > > > asks for it, it look a lot safer to got this rout for now. > > > > > > > > > > Can we try investigating a > > > > different approach: > > > > > > > > 1. Rename cpu_index to cpu_id, but it's still used in the same > > > > places it's used. > > > > > > > > 2. Remove assumptions that cpu_id values are contiguous or > > > > dense > > > > > > > > 3. Machine type decides whether it wants to populate the cpu_id > > > > values explicitly, or leave it to generic code to calculate > > > > them as cpu_index is calculated now. > > > > > > > > 4. Ideally, generic code enforces that the machine type populates > > > > either all or none of the cpu_id values. > > > > > > > > Does that seem workable? > > > Ideally we will get there some day (and may be get rid of cpu_index altogether), > > > but for now it seems too invasive with a lot of chances to introduce non obvious > > > regression. > > > > Yes, that's a risk. But I'm basically no longer convinced that it's > > any higher than the risk of the same thing with the current approach. > to me it looks much less risky as it affects only migration path and > could be done in time for 2.7 > > if we go to cpu_index re-factoring path then I'm afraid it quite a bit > more complex and it's not a 2.7 material. Yeah, ok. Having looked in a bit more depth, I agree with you. > > > > > > > So I'd keep approach used in this series. > > >
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c index b104778..0ec3513 100644 --- a/hw/ppc/spapr_cpu_core.c +++ b/hw/ppc/spapr_cpu_core.c @@ -293,8 +293,15 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp) for (i = 0; i < cc->nr_threads; i++) { char id[32]; obj = sc->threads + i * size; + CPUState *cs; object_initialize(obj, size, typename); + cs = CPU(obj); + + /* Use core_id (which is actually cpu_dt_id) as stable CPU id */ + if (cs->has_stable_cpu_id) { + cs->stable_cpu_id = cc->core_id + i; + } snprintf(id, sizeof(id), "thread[%d]", i); object_property_add_child(OBJECT(sc), id, obj, &local_err); if (local_err) {
Conditonally set stable_cpu_id for CPU threads that are created as part of spapr CPU cores. The use of stable_cpu_id is enabled for pseries-2.7 onwards. Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> --- hw/ppc/spapr_cpu_core.c | 7 +++++++ 1 file changed, 7 insertions(+)