| Message ID | 1467795561-1007-4-git-send-email-bharata@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 6 Jul 2016 14:29:19 +0530 Bharata B Rao <bharata@linux.vnet.ibm.com> wrote: > cpu_index is used as migration_id by default. For machine type > versions that set use-migration-id property, cpu_dt_it is returned. > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > --- > target-ppc/translate_init.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c > index efd6b88..9ca2f5e 100644 > --- a/target-ppc/translate_init.c > +++ b/target-ppc/translate_init.c > @@ -10359,6 +10359,17 @@ static gchar *ppc_gdb_arch_name(CPUState *cs) > #endif > } > > +static int ppc_cpu_get_migration_id(CPUState *cs) > +{ > + PowerPCCPU *cpu = POWERPC_CPU(cs); > + > + if (cs->use_migration_id) { > + return (int) cpu->cpu_dt_id; Could cpu_dt_id have value bigger than 32bit int? If yes, it's not safe to do so, that's the reason why I'm going to use index in possible_cpus on ARM and for the sake of uniformity do the same for x86 (even though it's possible to use 32bit APIC ID). > + } else { > + return cs->cpu_index; > + } > +} > + > static void ppc_cpu_class_init(ObjectClass *oc, void *data) > { > PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc); > @@ -10412,6 +10423,7 @@ static void ppc_cpu_class_init(ObjectClass > *oc, void *data) #ifndef CONFIG_USER_ONLY > cc->virtio_is_big_endian = ppc_cpu_is_big_endian; > #endif > + cc->get_migration_id = ppc_cpu_get_migration_id; > > dc->fw_name = "PowerPC,UNKNOWN"; > }
On Wed, Jul 06, 2016 at 02:01:14PM +0200, Igor Mammedov wrote: > On Wed, 6 Jul 2016 14:29:19 +0530 > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote: > > > cpu_index is used as migration_id by default. For machine type > > versions that set use-migration-id property, cpu_dt_it is returned. > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > --- > > target-ppc/translate_init.c | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c > > index efd6b88..9ca2f5e 100644 > > --- a/target-ppc/translate_init.c > > +++ b/target-ppc/translate_init.c > > @@ -10359,6 +10359,17 @@ static gchar *ppc_gdb_arch_name(CPUState *cs) > > #endif > > } > > > > +static int ppc_cpu_get_migration_id(CPUState *cs) > > +{ > > + PowerPCCPU *cpu = POWERPC_CPU(cs); > > + > > + if (cs->use_migration_id) { > > + return (int) cpu->cpu_dt_id; > Could cpu_dt_id have value bigger than 32bit int? If yes, it's not safe > to do so, that's the reason why I'm going to use index in possible_cpus > on ARM and for the sake of uniformity do the same for x86 (even though > it's possible to use 32bit APIC ID). For the existing max_cpus limit, 32 bit should be fine, but obviously that is not good and future safe. I had a brief look at the implementation around possible_cpus in x86, let me see if I can do something similar for generating stable ids for PowerPC. I thought device tree ID would be our stable id, but that being 64 bit and migration code requiring 32 bit value isn't helping :( Regards, Bharata.
On Wed, 6 Jul 2016 14:29:19 +0530 Bharata B Rao <bharata@linux.vnet.ibm.com> wrote: > cpu_index is used as migration_id by default. For machine type versions > that set use-migration-id property, cpu_dt_it is returned. > It looks wrong to hijack cpu_dt_id to fix migration. For pseries-2.7, you need to pass the sum of the index of the core in spapr->cores and the index of the thread in sc->threads. All other machines just need to pass the index of the cpu in their respective arrays, which happens to be equal to cs->cpu_index. > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > --- > target-ppc/translate_init.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c > index efd6b88..9ca2f5e 100644 > --- a/target-ppc/translate_init.c > +++ b/target-ppc/translate_init.c > @@ -10359,6 +10359,17 @@ static gchar *ppc_gdb_arch_name(CPUState *cs) > #endif > } > > +static int ppc_cpu_get_migration_id(CPUState *cs) > +{ > + PowerPCCPU *cpu = POWERPC_CPU(cs); > + > + if (cs->use_migration_id) { > + return (int) cpu->cpu_dt_id; > + } else { > + return cs->cpu_index; > + } > +} > + > static void ppc_cpu_class_init(ObjectClass *oc, void *data) > { > PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc); > @@ -10412,6 +10423,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data) > #ifndef CONFIG_USER_ONLY > cc->virtio_is_big_endian = ppc_cpu_is_big_endian; > #endif > + cc->get_migration_id = ppc_cpu_get_migration_id; > > dc->fw_name = "PowerPC,UNKNOWN"; > }
On Wed, 6 Jul 2016 19:51:01 +0530 Bharata B Rao <bharata@linux.vnet.ibm.com> wrote: > On Wed, Jul 06, 2016 at 02:01:14PM +0200, Igor Mammedov wrote: > > On Wed, 6 Jul 2016 14:29:19 +0530 > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote: > > > > > cpu_index is used as migration_id by default. For machine type > > > versions that set use-migration-id property, cpu_dt_it is returned. > > > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > > --- > > > target-ppc/translate_init.c | 12 ++++++++++++ > > > 1 file changed, 12 insertions(+) > > > > > > diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c > > > index efd6b88..9ca2f5e 100644 > > > --- a/target-ppc/translate_init.c > > > +++ b/target-ppc/translate_init.c > > > @@ -10359,6 +10359,17 @@ static gchar *ppc_gdb_arch_name(CPUState *cs) > > > #endif > > > } > > > > > > +static int ppc_cpu_get_migration_id(CPUState *cs) > > > +{ > > > + PowerPCCPU *cpu = POWERPC_CPU(cs); > > > + > > > + if (cs->use_migration_id) { > > > + return (int) cpu->cpu_dt_id; > > Could cpu_dt_id have value bigger than 32bit int? If yes, it's not safe > > to do so, that's the reason why I'm going to use index in possible_cpus > > on ARM and for the sake of uniformity do the same for x86 (even though > > it's possible to use 32bit APIC ID). > > For the existing max_cpus limit, 32 bit should be fine, but obviously > that is not good and future safe. > And BTW, cpu_dt_id is actually an int > I had a brief look at the implementation around possible_cpus in > x86, let me see if I can do something similar for generating stable > ids for PowerPC. I thought device tree ID would be our stable id, but > that being 64 bit and migration code requiring 32 bit value isn't > helping :( > Yes you have stable ids, look at my other answer to this patch. > Regards, > Bharata. >
On Wed, Jul 06, 2016 at 04:35:28PM +0200, Greg Kurz wrote: > On Wed, 6 Jul 2016 14:29:19 +0530 > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote: > > cpu_index is used as migration_id by default. For machine type versions > > that set use-migration-id property, cpu_dt_it is returned. > > > > It looks wrong to hijack cpu_dt_id to fix migration. > > For pseries-2.7, you need to pass the sum of the index of the core in > spapr->cores and the index of the thread in sc->threads. > > All other machines just need to pass the index of the cpu in their > respective arrays, which happens to be equal to cs->cpu_index. Yes, that is indeed good and simplifies things. Thanks for noting this. Regards, Bharata.
On Wed, Jul 06, 2016 at 02:01:14PM +0200, Igor Mammedov wrote: > On Wed, 6 Jul 2016 14:29:19 +0530 > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote: > > > cpu_index is used as migration_id by default. For machine type > > versions that set use-migration-id property, cpu_dt_it is returned. > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > --- > > target-ppc/translate_init.c | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c > > index efd6b88..9ca2f5e 100644 > > --- a/target-ppc/translate_init.c > > +++ b/target-ppc/translate_init.c > > @@ -10359,6 +10359,17 @@ static gchar *ppc_gdb_arch_name(CPUState *cs) > > #endif > > } > > > > +static int ppc_cpu_get_migration_id(CPUState *cs) > > +{ > > + PowerPCCPU *cpu = POWERPC_CPU(cs); > > + > > + if (cs->use_migration_id) { > > + return (int) cpu->cpu_dt_id; > Could cpu_dt_id have value bigger than 32bit int? If yes, it's not safe > to do so, that's the reason why I'm going to use index in possible_cpus > on ARM and for the sake of uniformity do the same for x86 (even though > it's possible to use 32bit APIC ID). No, I'm pretty sure the DT id fits in a 32-bit field in the device tree itself, so it should be safe. > > > + } else { > > + return cs->cpu_index; > > + } > > +} > > + > > static void ppc_cpu_class_init(ObjectClass *oc, void *data) > > { > > PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc); > > @@ -10412,6 +10423,7 @@ static void ppc_cpu_class_init(ObjectClass > > *oc, void *data) #ifndef CONFIG_USER_ONLY > > cc->virtio_is_big_endian = ppc_cpu_is_big_endian; > > #endif > > + cc->get_migration_id = ppc_cpu_get_migration_id; > > > > dc->fw_name = "PowerPC,UNKNOWN"; > > } >
On Wed, Jul 06, 2016 at 07:51:01PM +0530, Bharata B Rao wrote: > On Wed, Jul 06, 2016 at 02:01:14PM +0200, Igor Mammedov wrote: > > On Wed, 6 Jul 2016 14:29:19 +0530 > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote: > > > > > cpu_index is used as migration_id by default. For machine type > > > versions that set use-migration-id property, cpu_dt_it is returned. > > > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > > --- > > > target-ppc/translate_init.c | 12 ++++++++++++ > > > 1 file changed, 12 insertions(+) > > > > > > diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c > > > index efd6b88..9ca2f5e 100644 > > > --- a/target-ppc/translate_init.c > > > +++ b/target-ppc/translate_init.c > > > @@ -10359,6 +10359,17 @@ static gchar *ppc_gdb_arch_name(CPUState *cs) > > > #endif > > > } > > > > > > +static int ppc_cpu_get_migration_id(CPUState *cs) > > > +{ > > > + PowerPCCPU *cpu = POWERPC_CPU(cs); > > > + > > > + if (cs->use_migration_id) { > > > + return (int) cpu->cpu_dt_id; > > Could cpu_dt_id have value bigger than 32bit int? If yes, it's not safe > > to do so, that's the reason why I'm going to use index in possible_cpus > > on ARM and for the sake of uniformity do the same for x86 (even though > > it's possible to use 32bit APIC ID). > > For the existing max_cpus limit, 32 bit should be fine, but obviously > that is not good and future safe. I think we should be OK. The hardware IDs these are modelled on are 32-bit - I'm pretty sure #address-cells in /cpus is 1, not 2. And even with a lot of wastage for alignment gaps of various sorts, we're a *long* way off from supporting 4 billion cpus... > I had a brief look at the implementation around possible_cpus in > x86, let me see if I can do something similar for generating stable > ids for PowerPC. I thought device tree ID would be our stable id, but > that being 64 bit and migration code requiring 32 bit value isn't > helping :( > > Regards, > Bharata. >
On Wed, Jul 06, 2016 at 02:29:19PM +0530, Bharata B Rao wrote: > cpu_index is used as migration_id by default. For machine type versions > that set use-migration-id property, cpu_dt_it is returned. > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> This seems ceonceptually wrong. The logic to determine the stable ID is still coming from the CPU code. But kind of the point here is that in most cases the stable IDs really belong to the machine type, not the CPU. Would it make more sense to just have migration-id be a settable property, which the machine type needs to set between init and realize? (or leave as default == cpu_index). That way we could have machine version dependent differences without this awkward use_migration_id flag (after all, if it's not always used as the id for migration, 'migration_id' is kind of a bad name...). > --- > target-ppc/translate_init.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c > index efd6b88..9ca2f5e 100644 > --- a/target-ppc/translate_init.c > +++ b/target-ppc/translate_init.c > @@ -10359,6 +10359,17 @@ static gchar *ppc_gdb_arch_name(CPUState *cs) > #endif > } > > +static int ppc_cpu_get_migration_id(CPUState *cs) > +{ > + PowerPCCPU *cpu = POWERPC_CPU(cs); > + > + if (cs->use_migration_id) { > + return (int) cpu->cpu_dt_id; > + } else { > + return cs->cpu_index; > + } > +} > + > static void ppc_cpu_class_init(ObjectClass *oc, void *data) > { > PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc); > @@ -10412,6 +10423,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data) > #ifndef CONFIG_USER_ONLY > cc->virtio_is_big_endian = ppc_cpu_is_big_endian; > #endif > + cc->get_migration_id = ppc_cpu_get_migration_id; > > dc->fw_name = "PowerPC,UNKNOWN"; > }
On Thu, 7 Jul 2016 10:57:28 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > On Wed, Jul 06, 2016 at 07:51:01PM +0530, Bharata B Rao wrote: > > On Wed, Jul 06, 2016 at 02:01:14PM +0200, Igor Mammedov wrote: > > > On Wed, 6 Jul 2016 14:29:19 +0530 > > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote: > > > > > > > cpu_index is used as migration_id by default. For machine type > > > > versions that set use-migration-id property, cpu_dt_it is returned. > > > > > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > > > --- > > > > target-ppc/translate_init.c | 12 ++++++++++++ > > > > 1 file changed, 12 insertions(+) > > > > > > > > diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c > > > > index efd6b88..9ca2f5e 100644 > > > > --- a/target-ppc/translate_init.c > > > > +++ b/target-ppc/translate_init.c > > > > @@ -10359,6 +10359,17 @@ static gchar *ppc_gdb_arch_name(CPUState *cs) > > > > #endif > > > > } > > > > > > > > +static int ppc_cpu_get_migration_id(CPUState *cs) > > > > +{ > > > > + PowerPCCPU *cpu = POWERPC_CPU(cs); > > > > + > > > > + if (cs->use_migration_id) { > > > > + return (int) cpu->cpu_dt_id; > > > Could cpu_dt_id have value bigger than 32bit int? If yes, it's not safe > > > to do so, that's the reason why I'm going to use index in possible_cpus > > > on ARM and for the sake of uniformity do the same for x86 (even though > > > it's possible to use 32bit APIC ID). > > > > For the existing max_cpus limit, 32 bit should be fine, but obviously > > that is not good and future safe. > > I think we should be OK. The hardware IDs these are modelled on are > 32-bit - I'm pretty sure #address-cells in /cpus is 1, not 2. > > And even with a lot of wastage for alignment gaps of various sorts, > we're a *long* way off from supporting 4 billion cpus... > Indeed ! Even in the "worst" case, which is running a guest with 1 thread per core on a POWER8 host with full cores (8 HW threads), the current maximum value for cpu_dt_id is 254 * 8 == 2032 :) > > I had a brief look at the implementation around possible_cpus in > > x86, let me see if I can do something similar for generating stable > > ids for PowerPC. I thought device tree ID would be our stable id, but > > that being 64 bit and migration code requiring 32 bit value isn't > > helping :( > > > > Regards, > > Bharata. > > >
On 07/07/16 02:00, David Gibson wrote: > On Wed, Jul 06, 2016 at 02:29:19PM +0530, Bharata B Rao wrote: >> cpu_index is used as migration_id by default. For machine type versions >> that set use-migration-id property, cpu_dt_it is returned. >> >> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > This seems ceonceptually wrong. The logic to determine the stable ID > is still coming from the CPU code. But kind of the point here is that > in most cases the stable IDs really belong to the machine type, not > the CPU. > > Would it make more sense to just have migration-id be a settable > property, which the machine type needs to set between init and > realize? (or leave as default == cpu_index). > > That way we could have machine version dependent differences without > this awkward use_migration_id flag (after all, if it's not always used > as the id for migration, 'migration_id' is kind of a bad name...). > >> --- >> target-ppc/translate_init.c | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c >> index efd6b88..9ca2f5e 100644 >> --- a/target-ppc/translate_init.c >> +++ b/target-ppc/translate_init.c >> @@ -10359,6 +10359,17 @@ static gchar *ppc_gdb_arch_name(CPUState *cs) >> #endif >> } >> >> +static int ppc_cpu_get_migration_id(CPUState *cs) >> +{ >> + PowerPCCPU *cpu = POWERPC_CPU(cs); >> + >> + if (cs->use_migration_id) { >> + return (int) cpu->cpu_dt_id; >> + } else { >> + return cs->cpu_index; >> + } >> +} >> + >> static void ppc_cpu_class_init(ObjectClass *oc, void *data) >> { >> PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc); >> @@ -10412,6 +10423,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data) >> #ifndef CONFIG_USER_ONLY >> cc->virtio_is_big_endian = ppc_cpu_is_big_endian; >> #endif >> + cc->get_migration_id = ppc_cpu_get_migration_id; >> >> dc->fw_name = "PowerPC,UNKNOWN"; >> } > Could this be another potential user for the PPCMachineClass solution I was looking at for timebase migration in https://lists.gnu.org/archive/html/qemu-devel/2016-04/msg01208.html? It seems like a similar problem where machine-level state is needed for migration purposes. ATB, Mark.
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c index efd6b88..9ca2f5e 100644 --- a/target-ppc/translate_init.c +++ b/target-ppc/translate_init.c @@ -10359,6 +10359,17 @@ static gchar *ppc_gdb_arch_name(CPUState *cs) #endif } +static int ppc_cpu_get_migration_id(CPUState *cs) +{ + PowerPCCPU *cpu = POWERPC_CPU(cs); + + if (cs->use_migration_id) { + return (int) cpu->cpu_dt_id; + } else { + return cs->cpu_index; + } +} + static void ppc_cpu_class_init(ObjectClass *oc, void *data) { PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc); @@ -10412,6 +10423,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data) #ifndef CONFIG_USER_ONLY cc->virtio_is_big_endian = ppc_cpu_is_big_endian; #endif + cc->get_migration_id = ppc_cpu_get_migration_id; dc->fw_name = "PowerPC,UNKNOWN"; }
cpu_index is used as migration_id by default. For machine type versions that set use-migration-id property, cpu_dt_it is returned. Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> --- target-ppc/translate_init.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)