| Message ID | 1373118856-30171-19-git-send-email-aik@ozlabs.ru |
|---|---|
| State | New |
| Headers | show |
On Sat, Jul 06, 2013 at 11:54:15PM +1000, Alexey Kardashevskiy wrote: > From: Prerna Saxena <prerna@linux.vnet.ibm.com> > > In absence of a -CPU parameter in the qemu command line, the nodes of > KVM-enabled guest device tree look like this : > > /proc/device-tree/cpus/HOST@0/... > /proc/device-tree/cpus/HOST@4/... > > This patch replaces this obscure 'HOST' label with a more descriptive label. > This is gathered by first identifying the PVR of the host, and then determining > the host CPU alias which corresponds to the model indicated by this PVR. > > Sample Final outcome for an KVM-enabled pseries guest running on POWER7: > /proc/device-tree/cpus/PowerPC,POWER7@0/... > /proc/device-tree/cpus/PowerPC,POWER7@4/... > > This also helps userspace tools like ppc64_cpu, which expect the device tree > to be in this format in the guest. > > Signed-off-by: Prerna Saxena <prerna@linux.vnet.ibm.com> > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > --- > hw/ppc/spapr.c | 17 ++++++++++++++--- > target-ppc/cpu-qom.h | 1 + > target-ppc/translate_init.c | 28 ++++++++++++++++++++++++++++ > 3 files changed, 43 insertions(+), 3 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 26dd3f7..5ecd81b 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -80,6 +80,7 @@ > > #define HTAB_SIZE(spapr) (1ULL << ((spapr)->htab_shift)) > > +#define PPC_DEVTREE_STR "PowerPC," I thought under PowerVM, modern CPUs showed up as simply e.g. "POWER7@0" not "PowerPC,POWER7@0". Have I misremembered? > sPAPREnvironment *spapr; > > int spapr_allocate_irq(int hint, bool lsi) > @@ -296,9 +297,12 @@ static void *spapr_create_fdt_skel(const char *cpu_model, > _FDT((fdt_property_cell(fdt, "#address-cells", 0x1))); > _FDT((fdt_property_cell(fdt, "#size-cells", 0x0))); > > - modelname = g_strdup(cpu_model); > + /* device tree nodes must look like this : > + * PowerPC,CPU_ALIAS@0 > + */ > + modelname = g_strdup_printf(PPC_DEVTREE_STR "%s", cpu_model); > > - for (i = 0; i < strlen(modelname); i++) { > + for (i = strlen(PPC_DEVTREE_STR); i < strlen(modelname); i++) { > modelname[i] = toupper(modelname[i]); > } > > @@ -1112,7 +1116,7 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args) > MemoryRegion *sysmem = get_system_memory(); > MemoryRegion *ram = g_new(MemoryRegion, 1); > hwaddr rma_alloc_size; > - uint32_t initrd_base = 0; > + uint32_t initrd_base = 0, pvr = 0; > long kernel_size = 0, initrd_size = 0; > long load_limit, rtas_limit, fw_size; > char *filename; > @@ -1342,6 +1346,13 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args) > register_savevm_live(NULL, "spapr/htab", -1, 1, > &savevm_htab_handlers, spapr); > > + /* Ensure that cpu_model is correctly reflected for a KVM guest */ > + if (kvm_enabled() && !strcmp(cpu_model, "host")) { > + asm ("mfpvr %0" > + : "=r"(pvr)); > + cpu_model = ppc_cpu_alias_by_pvr(pvr); This needs to be protected by an ifdef CONFIG_KVM or similar. If the compiler optimization level is turned down, so that it doesn't recognize that the kvm_enabled() is always false, then this could attempt to compile the ppc asm instructions on an x86 (or whatever) host.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Am 08.07.2013 03:09, schrieb David Gibson: > On Sat, Jul 06, 2013 at 11:54:15PM +1000, Alexey Kardashevskiy > wrote: >> @@ -1342,6 +1346,13 @@ static void >> ppc_spapr_init(QEMUMachineInitArgs *args) >> register_savevm_live(NULL, "spapr/htab", -1, 1, >> &savevm_htab_handlers, spapr); >> >> + /* Ensure that cpu_model is correctly reflected for a KVM >> guest */ + if (kvm_enabled() && !strcmp(cpu_model, "host")) { >> + asm ("mfpvr %0" + : "=r"(pvr)); + >> cpu_model = ppc_cpu_alias_by_pvr(pvr); > > This needs to be protected by an ifdef CONFIG_KVM or similar. If > the compiler optimization level is turned down, so that it doesn't > recognize that the kvm_enabled() is always false, then this could > attempt to compile the ppc asm instructions on an x86 (or > whatever) host. This hunk can be completely replaced by QOM mechanisms - just didn't get to replying yet... Andreas - -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendrffer; HRB 16746 AG Nrnberg -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.19 (GNU/Linux) iQIcBAEBAgAGBQJR2oAjAAoJEPou0S0+fgE/kxEP/2hvke1o/T4/h3Gl48W2+ASv 84iu5M7atndRF1L1bI6VogmQGlhE4qiAMxuLljpqriXz5lZndslMMcP3mx4skljD Y3YX9Hi37yR9KEaw0AzoQCBhhS5ZIMGjd/mtW/DqPDcN0H0IdCu340Mz/Lr+0HHy wp+ChUA8q8iYWJd6zmFmIvnaHUdbRoOHePhVlJD+GZQ2oBNu48DAaiiCdnrEJy+R ipyZJEF+QmO2RlNDgImOKfyKry6PhuWPBIjMB3qZWyuFzmkwNEcQILnOVSW/bBIl zXkEkWy3u5fES1+bYs1J4ZL6MZ+Edcd0c2BRKQ9JNUGM6mBj1S61aP8rC7u1VgLp eUfkSRYOrsvVvJJ/kpOzgWgcgYnfSYp/CUTRURHxlyIxNuvhjDllRhC4wxbF4Bk4 l6jbIDa8jAMTlbCj9EW03Fi+i+oGemkOg2g5Dxl5GnFwdPC95fE39RvSa5vB3X3q 6IgdkbicFReR1dY8JxdcJsTln6b2eMTSHvUjH56FEvDQ9Z/W7TM/qc1jpmNDX7WS bdWHcziPeAoY9Sk0aMK/LlTKmgZQM1gi5eyKIrL4ujtU3O4VKcNSihYu+Moc+oyx pEfJrkXP6cvYLwW60yxj8soBv9ssCSBU5ZqgcSK7NlfST0KxtQe4y+jwCT0LkhyS Qoat9lALzVlVlQwCWM6/ =2tHJ -----END PGP SIGNATURE-----
On 07/08/2013 02:32 PM, Andreas Färber wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > Am 08.07.2013 03:09, schrieb David Gibson: >> On Sat, Jul 06, 2013 at 11:54:15PM +1000, Alexey Kardashevskiy >> wrote: >>> @@ -1342,6 +1346,13 @@ static void >>> ppc_spapr_init(QEMUMachineInitArgs *args) >>> register_savevm_live(NULL, "spapr/htab", -1, 1, >>> &savevm_htab_handlers, spapr); >>> >>> + /* Ensure that cpu_model is correctly reflected for a KVM >>> guest */ + if (kvm_enabled() && !strcmp(cpu_model, "host")) { >>> + asm ("mfpvr %0" + : "=r"(pvr)); + >>> cpu_model = ppc_cpu_alias_by_pvr(pvr); >> >> This needs to be protected by an ifdef CONFIG_KVM or similar. If >> the compiler optimization level is turned down, so that it doesn't >> recognize that the kvm_enabled() is always false, then this could >> attempt to compile the ppc asm instructions on an x86 (or >> whatever) host. > > This hunk can be completely replaced by QOM mechanisms - just didn't > get to replying yet... > Hi Andreas, Sorry I already sent out a v2, and only then saw your message. Could you pls explain how I could use QOM to replace this code block ? Regards,
Hi, Am 08.07.2013 17:49, schrieb Prerna Saxena: > On 07/08/2013 02:32 PM, Andreas Färber wrote: >> -----BEGIN PGP SIGNED MESSAGE----- >> Hash: SHA1 >> >> Am 08.07.2013 03:09, schrieb David Gibson: >>> On Sat, Jul 06, 2013 at 11:54:15PM +1000, Alexey Kardashevskiy >>> wrote: >>>> @@ -1342,6 +1346,13 @@ static void >>>> ppc_spapr_init(QEMUMachineInitArgs *args) >>>> register_savevm_live(NULL, "spapr/htab", -1, 1, >>>> &savevm_htab_handlers, spapr); >>>> >>>> + /* Ensure that cpu_model is correctly reflected for a KVM >>>> guest */ + if (kvm_enabled() && !strcmp(cpu_model, "host")) { >>>> + asm ("mfpvr %0" + : "=r"(pvr)); + >>>> cpu_model = ppc_cpu_alias_by_pvr(pvr); >>> >>> This needs to be protected by an ifdef CONFIG_KVM or similar. If >>> the compiler optimization level is turned down, so that it doesn't >>> recognize that the kvm_enabled() is always false, then this could >>> attempt to compile the ppc asm instructions on an x86 (or >>> whatever) host. >> >> This hunk can be completely replaced by QOM mechanisms - just didn't >> get to replying yet... > > Sorry I already sent out a v2, and only then saw your message. Could you > pls explain how I could use QOM to replace this code block ? Well, in short the thing is it has not much to do with KVM. The KVM-specific host-powerpc64-cpu type is derived from the one you're looking for and thus you can use object_class_get_parent() to obtain the parent type and look at its name - stripping "-" TYPE_POWERPC_CPU from it should be much more efficient but will give you the detailed name including revision. I was planning to propose an alternative patch for that. Replacing a concrete model name with its simpler alias is a secondary issue (separate patch) that is not specific to KVM or -cpu host. Compare -cpu POWER8_v1.0 printing .../POWER8_v1.0@0/... presumably. Further, Alex has already applied a patch of his working around the alias table being a rather archaic construct, not intended for frequent use. Instead of adding even more functions that iterate it, we should turn it into a hashtable for efficient lookup. (Note that the cpu_model_str field may contain more than just the model name, it is otherwise unused in softmmu and I was therefore preparing a patch to ban its use to linux-user solely, so the type name seems the most reliable indicator we have and as a bonus no PVR needed for it.) Regards, Andreas
Hi Andreas, Thanks for the response. On 07/08/2013 10:15 PM, Andreas Färber wrote: > Hi, > > Am 08.07.2013 17:49, schrieb Prerna Saxena: >> On 07/08/2013 02:32 PM, Andreas Färber wrote: >>> -----BEGIN PGP SIGNED MESSAGE----- >>> Hash: SHA1 >>> >>> Am 08.07.2013 03:09, schrieb David Gibson: >>>> On Sat, Jul 06, 2013 at 11:54:15PM +1000, Alexey Kardashevskiy >>>> wrote: >>>>> @@ -1342,6 +1346,13 @@ static void >>>>> ppc_spapr_init(QEMUMachineInitArgs *args) >>>>> register_savevm_live(NULL, "spapr/htab", -1, 1, >>>>> &savevm_htab_handlers, spapr); >>>>> >>>>> + /* Ensure that cpu_model is correctly reflected for a KVM >>>>> guest */ + if (kvm_enabled() && !strcmp(cpu_model, "host")) { >>>>> + asm ("mfpvr %0" + : "=r"(pvr)); + >>>>> cpu_model = ppc_cpu_alias_by_pvr(pvr); >>>> >>>> This needs to be protected by an ifdef CONFIG_KVM or similar. If >>>> the compiler optimization level is turned down, so that it doesn't >>>> recognize that the kvm_enabled() is always false, then this could >>>> attempt to compile the ppc asm instructions on an x86 (or >>>> whatever) host. >>> >>> This hunk can be completely replaced by QOM mechanisms - just didn't >>> get to replying yet... >> >> Sorry I already sent out a v2, and only then saw your message. Could you >> pls explain how I could use QOM to replace this code block ? > > Well, in short the thing is it has not much to do with KVM. The > KVM-specific host-powerpc64-cpu type is derived from the one you're > looking for and thus you can use object_class_get_parent() to obtain the > parent type and look at its name - stripping "-" TYPE_POWERPC_CPU from > it should be much more efficient but will give you the detailed name > including revision. I was planning to propose an alternative patch for that. This is what my patch does :-) +const char *ppc_cpu_alias_by_pvr(uint32_t pvr) +{ + int i; + const char *cpu_alias; + char *offset, *model; + + cpu_alias = object_class_get_name(OBJECT_CLASS + (ppc_cpu_class_by_pvr(pvr))); + ....[snip] > > Replacing a concrete model name with its simpler alias is a secondary > issue (separate patch) that is not specific to KVM or -cpu host. Compare > -cpu POWER8_v1.0 printing .../POWER8_v1.0@0/... presumably. > Agree that this is not specific to KVM. That is the reason I have set it in a separate function, which can be called otherwise as well. Just to clarify your response, you want the function I coded to be split into 2 different pieces, to cater to the two specific requirements you mention ? That can be done, but not sure if it is too much code bloat. > Further, Alex has already applied a patch of his working around the > alias table being a rather archaic construct, not intended for frequent > use. Instead of adding even more functions that iterate it, we should > turn it into a hashtable for efficient lookup. > Can you / Alexander Graf point me to the fix ? I can rework my patch to consume it ? > (Note that the cpu_model_str field may contain more than just the model > name, it is otherwise unused in softmmu and I was therefore preparing a > patch to ban its use to linux-user solely, so the type name seems the > most reliable indicator we have and as a bonus no PVR needed for it.) > Hmm, maybe obsoleting PVR check is not such a great idea. I'm not sure if my earlier email clearly outlined the use-case this patch was attempting to fix. Here is a detailed explanation : We will still need PVR based lookups for cases such as the one I have described. As an illustration, consider running in a KVM environment where QEMU hasnt been started with a specific CPU type via "-CPU PPC_MODEL". In this case, we will be required to do a PVR_based lookup only -- to make sure the guest gets initialized with the same CPU as host. The notion of _same_cpu_model_ can only be built over a PVR check. Regards,
Am 10.07.2013 08:38, schrieb Prerna Saxena: > On 07/08/2013 10:15 PM, Andreas Färber wrote: >> Am 08.07.2013 17:49, schrieb Prerna Saxena: >>> On 07/08/2013 02:32 PM, Andreas Färber wrote: >>>> Am 08.07.2013 03:09, schrieb David Gibson: >>>>> On Sat, Jul 06, 2013 at 11:54:15PM +1000, Alexey Kardashevskiy >>>>> wrote: >>>>>> @@ -1342,6 +1346,13 @@ static void >>>>>> ppc_spapr_init(QEMUMachineInitArgs *args) >>>>>> register_savevm_live(NULL, "spapr/htab", -1, 1, >>>>>> &savevm_htab_handlers, spapr); >>>>>> >>>>>> + /* Ensure that cpu_model is correctly reflected for a KVM >>>>>> guest */ + if (kvm_enabled() && !strcmp(cpu_model, "host")) { >>>>>> + asm ("mfpvr %0" + : "=r"(pvr)); + >>>>>> cpu_model = ppc_cpu_alias_by_pvr(pvr); >>>>> >>>>> This needs to be protected by an ifdef CONFIG_KVM or similar. If >>>>> the compiler optimization level is turned down, so that it doesn't >>>>> recognize that the kvm_enabled() is always false, then this could >>>>> attempt to compile the ppc asm instructions on an x86 (or >>>>> whatever) host. >>>> >>>> This hunk can be completely replaced by QOM mechanisms - just didn't >>>> get to replying yet... >>> >>> Sorry I already sent out a v2, and only then saw your message. Could you >>> pls explain how I could use QOM to replace this code block ? >> >> Well, in short the thing is it has not much to do with KVM. The >> KVM-specific host-powerpc64-cpu type is derived from the one you're >> looking for and thus you can use object_class_get_parent() to obtain the >> parent type and look at its name - stripping "-" TYPE_POWERPC_CPU from >> it should be much more efficient but will give you the detailed name >> including revision. I was planning to propose an alternative patch for that. > > This is what my patch does :-) > > +const char *ppc_cpu_alias_by_pvr(uint32_t pvr) > +{ > + int i; > + const char *cpu_alias; > + char *offset, *model; > + > + cpu_alias = object_class_get_name(OBJECT_CLASS > + (ppc_cpu_class_by_pvr(pvr))); > + ....[snip] And I am complaining about code duplication: Your use of ppc_cpu_class_by_pvr() should be replaced with object_class_get_parent() as I said above, because the PVR lookup is already done in KVM code for you. :-) >> Replacing a concrete model name with its simpler alias is a secondary >> issue (separate patch) that is not specific to KVM or -cpu host. Compare >> -cpu POWER8_v1.0 printing .../POWER8_v1.0@0/... presumably. >> > > Agree that this is not specific to KVM. That is the reason I have set it > in a separate function, which can be called otherwise as well. > > Just to clarify your response, you want the function I coded to be split > into 2 different pieces, to cater to the two specific requirements you > mention ? That can be done, but not sure if it is too much code bloat. Your function duplicates runtime functionality (while the model list keeps growing...) and you are duplicating KVM code into sPAPR. I was asking you to make better reuse of existing code and I asked you whether we need the model-to-alias lookup at all. It should not be limited to your KVMish cpu_model == host check but either be dropped or called afterwards on any cpu_model for consistent results. >> (Note that the cpu_model_str field may contain more than just the model >> name, it is otherwise unused in softmmu and I was therefore preparing a >> patch to ban its use to linux-user solely, so the type name seems the >> most reliable indicator we have and as a bonus no PVR needed for it.) >> > > Hmm, maybe obsoleting PVR check is not such a great idea. > I'm not sure if my earlier email clearly outlined the use-case this > patch was attempting to fix. Here is a detailed explanation : > > We will still need PVR based lookups for cases such as the one I have > described. As an illustration, consider running in a KVM environment > where QEMU hasnt been started with a specific CPU type via "-CPU > PPC_MODEL". In this case, we will be required to do a PVR_based lookup > only -- to make sure the guest gets initialized with the same CPU as > host. The notion of _same_cpu_model_ can only be built over a PVR check. Sorry? That is done in kvm.c (I wrote the current form of that code!) and no one proposed changing it. What I am asking is not to introduce yet another mfpvr in your *sPAPR* code. There is no requirement to use -cpu host (the default) with KVM, you can use -cpu some_model with KVM just as well. For instance, when your PVR is not yet enabled in QEMU (e.g., try -cpu POWER7_v2.3 on POWER8 DD1 to see what I mean). Regards, Andreas
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 26dd3f7..5ecd81b 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -80,6 +80,7 @@ #define HTAB_SIZE(spapr) (1ULL << ((spapr)->htab_shift)) +#define PPC_DEVTREE_STR "PowerPC," sPAPREnvironment *spapr; int spapr_allocate_irq(int hint, bool lsi) @@ -296,9 +297,12 @@ static void *spapr_create_fdt_skel(const char *cpu_model, _FDT((fdt_property_cell(fdt, "#address-cells", 0x1))); _FDT((fdt_property_cell(fdt, "#size-cells", 0x0))); - modelname = g_strdup(cpu_model); + /* device tree nodes must look like this : + * PowerPC,CPU_ALIAS@0 + */ + modelname = g_strdup_printf(PPC_DEVTREE_STR "%s", cpu_model); - for (i = 0; i < strlen(modelname); i++) { + for (i = strlen(PPC_DEVTREE_STR); i < strlen(modelname); i++) { modelname[i] = toupper(modelname[i]); } @@ -1112,7 +1116,7 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args) MemoryRegion *sysmem = get_system_memory(); MemoryRegion *ram = g_new(MemoryRegion, 1); hwaddr rma_alloc_size; - uint32_t initrd_base = 0; + uint32_t initrd_base = 0, pvr = 0; long kernel_size = 0, initrd_size = 0; long load_limit, rtas_limit, fw_size; char *filename; @@ -1342,6 +1346,13 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args) register_savevm_live(NULL, "spapr/htab", -1, 1, &savevm_htab_handlers, spapr); + /* Ensure that cpu_model is correctly reflected for a KVM guest */ + if (kvm_enabled() && !strcmp(cpu_model, "host")) { + asm ("mfpvr %0" + : "=r"(pvr)); + cpu_model = ppc_cpu_alias_by_pvr(pvr); + } + /* Prepare the device tree */ spapr->fdt_skel = spapr_create_fdt_skel(cpu_model, initrd_base, initrd_size, diff --git a/target-ppc/cpu-qom.h b/target-ppc/cpu-qom.h index a14a3d9..2ad45c2 100644 --- a/target-ppc/cpu-qom.h +++ b/target-ppc/cpu-qom.h @@ -99,6 +99,7 @@ static inline PowerPCCPU *ppc_env_get_cpu(CPUPPCState *env) #define ENV_OFFSET offsetof(PowerPCCPU, env) PowerPCCPUClass *ppc_cpu_class_by_pvr(uint32_t pvr); +const char *ppc_cpu_alias_by_pvr(uint32_t pvr); void ppc_cpu_do_interrupt(CPUState *cpu); void ppc_cpu_dump_state(CPUState *cpu, FILE *f, fprintf_function cpu_fprintf, diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c index c4b466b..2b013c2 100644 --- a/target-ppc/translate_init.c +++ b/target-ppc/translate_init.c @@ -7947,6 +7947,34 @@ PowerPCCPUClass *ppc_cpu_class_by_pvr(uint32_t pvr) return pcc; } +const char *ppc_cpu_alias_by_pvr(uint32_t pvr) +{ + int i; + const char *cpu_alias; + char *offset, *model; + + cpu_alias = object_class_get_name(OBJECT_CLASS + (ppc_cpu_class_by_pvr(pvr))); + + /* Replace the full class name in cpu_alias with the CPU alias + * Eg, POWER7_V2.3-POWERPC64-CPU can simply be called + * POWER7 + */ + + offset = strstr(cpu_alias, "-" TYPE_POWERPC_CPU); + if (offset) { + model = g_strndup(cpu_alias, offset - cpu_alias); + for (i = 0; ppc_cpu_aliases[i].model != NULL; i++) { + if (strcmp(ppc_cpu_aliases[i].model, model) == 0) { + g_free(model); + return ppc_cpu_aliases[i].alias; + } + } + g_free(model); + } + return NULL; +} + static gint ppc_cpu_compare_class_name(gconstpointer a, gconstpointer b) { ObjectClass *oc = (ObjectClass *)a;