| Message ID | 20130808125629.4e5f435d@zephyr |
|---|---|
| State | New |
| Headers | show |
Am 08.08.2013 09:26, schrieb Prerna Saxena: > > From: Prerna Saxena <prerna@linux.vnet.ibm.com> > Date: Thu, 8 Aug 2013 06:38:03 +0530 > Subject: [PATCH 2/2] Enhance CPU nodes of device tree to be PAPR compliant. > > This is based on patch from Andreas which enables the default CPU with KVM > to show up as "-cpu <type>", such as "POWER7_V2.3@0" > > While this is definitely, more descriptive, PAPR mandates the device tree CPU > node names to be of the form : "PowerPC,<name>" where <name> should not have > underscores. > Hence replacing the CPU model (which has underscores) with CPU alias. > > With this patch, the CPU nodes of device tree show up as : > /proc/device-tree/cpus/PowerPC,POWER7@0/... > /proc/device-tree/cpus/PowerPC,POWER7@4/... > > Signed-off-by: Prerna Saxena <prerna@linux.vnet.ibm.com> Not yet happy... > --- > hw/ppc/spapr.c | 22 ++++++++++++++++++++-- > 1 file changed, 20 insertions(+), 2 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 59e2fea..8efd84e 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -43,6 +43,7 @@ > #include "hw/pci-host/spapr.h" > #include "hw/ppc/xics.h" > #include "hw/pci/msi.h" > +#include "cpu-models.h" > > #include "hw/pci/pci.h" > > @@ -80,6 +81,8 @@ > > #define HTAB_SIZE(spapr) (1ULL << ((spapr)->htab_shift)) > > +#define PPC_DEVTREE_STR "PowerPC," > + > sPAPREnvironment *spapr; > > int spapr_allocate_irq(int hint, bool lsi) > @@ -322,9 +325,16 @@ 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); > + /* > + * PAPR convention mandates that > + * Device tree nodes must be named as: > + * PowerPC,CPU-NAME@... > + * Also, CPU-NAME must not have underscores.(hence use of CPU-ALIAS) > + */ > + > + 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]); > } > One of your colleagues had brought up that "PowerPC," prefix were not mandatory - is it *required* by the PAPR spec now, or is it just that the IBM CPUs used with PAPR happen to have such a name? > @@ -1315,6 +1325,14 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args) > > cpu_model = g_strndup(parent_name, > strlen(parent_name) - strlen("-" TYPE_POWERPC_CPU)); > + > + for (i = 0; ppc_cpu_aliases[i].model != NULL; i++) { > + if (strcmp(ppc_cpu_aliases[i].model, cpu_model) == 0) { > + g_free(cpu_model); > + cpu_model = g_strndup(ppc_cpu_aliases[i].alias, > + strlen(ppc_cpu_aliases[i].alias)); > + } > + } > } > > /* Prepare the device tree */ This is still fixing up the name in the wrong place: -cpu POWER7_v2.3 will not get fixed, only -cpu host or KVM's default. The solution I had discussed with Alex is the following: When devices need to expose their name to firmware in a special way, we have the DeviceClass::fw_name field. All we have to do is assign it and use it instead of cpu_model if non-NULL, just like we assign DeviceClass::desc. The way to do it would be to extend the family of POWERPC_DEF* macros to specify the additional field on the relevant CPU models. Therefore my above question: Would it be sufficient to explicitly name POWER7_v2.3 PowerPC,POWER7 etc. and to drop the upper-casing? Or would we also need to name a CPU such as MPC8572E (random Freescale CPU where I don't know the expected fw_name and that is unlikely to occur/work in sPAPR) "PowerPC,MPC8572E" if someone specified it with -cpu MPC8572E? Regards, Andreas
On 08/08/2013 04:04 PM, Andreas Färber wrote: > Am 08.08.2013 09:26, schrieb Prerna Saxena: >> >> From: Prerna Saxena <prerna@linux.vnet.ibm.com> >> Date: Thu, 8 Aug 2013 06:38:03 +0530 >> Subject: [PATCH 2/2] Enhance CPU nodes of device tree to be PAPR compliant. >> >> This is based on patch from Andreas which enables the default CPU with KVM >> to show up as "-cpu <type>", such as "POWER7_V2.3@0" >> >> While this is definitely, more descriptive, PAPR mandates the device tree CPU >> node names to be of the form : "PowerPC,<name>" where <name> should not have >> underscores. >> Hence replacing the CPU model (which has underscores) with CPU alias. >> >> With this patch, the CPU nodes of device tree show up as : >> /proc/device-tree/cpus/PowerPC,POWER7@0/... >> /proc/device-tree/cpus/PowerPC,POWER7@4/... >> >> Signed-off-by: Prerna Saxena <prerna@linux.vnet.ibm.com> > > Not yet happy... :( > >> --- >> hw/ppc/spapr.c | 22 ++++++++++++++++++++-- >> 1 file changed, 20 insertions(+), 2 deletions(-) >> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c >> index 59e2fea..8efd84e 100644 >> --- a/hw/ppc/spapr.c >> +++ b/hw/ppc/spapr.c >> @@ -43,6 +43,7 @@ >> #include "hw/pci-host/spapr.h" >> #include "hw/ppc/xics.h" >> #include "hw/pci/msi.h" >> +#include "cpu-models.h" >> >> #include "hw/pci/pci.h" >> >> @@ -80,6 +81,8 @@ >> >> #define HTAB_SIZE(spapr) (1ULL << ((spapr)->htab_shift)) >> >> +#define PPC_DEVTREE_STR "PowerPC," >> + >> sPAPREnvironment *spapr; >> >> int spapr_allocate_irq(int hint, bool lsi) >> @@ -322,9 +325,16 @@ 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); >> + /* >> + * PAPR convention mandates that >> + * Device tree nodes must be named as: >> + * PowerPC,CPU-NAME@... >> + * Also, CPU-NAME must not have underscores.(hence use of CPU-ALIAS) >> + */ >> + >> + 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]); >> } >> > > One of your colleagues had brought up that "PowerPC," prefix were not > mandatory - is it *required* by the PAPR spec now, or is it just that > the IBM CPUs used with PAPR happen to have such a name? I dont know what context lead to this observation. However, PAPR mentions the following nomenclature guideline: "The value of this property shall be of the form: “PowerPC,<name>”, where <name> is the name of the processor chip which may be displayed to the user. <name> shall not contain underscores." I think this name guideline will hold good for all PAPR compliant processors. > >> @@ -1315,6 +1325,14 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args) >> >> cpu_model = g_strndup(parent_name, >> strlen(parent_name) - strlen("-" TYPE_POWERPC_CPU)); >> + >> + for (i = 0; ppc_cpu_aliases[i].model != NULL; i++) { >> + if (strcmp(ppc_cpu_aliases[i].model, cpu_model) == 0) { >> + g_free(cpu_model); >> + cpu_model = g_strndup(ppc_cpu_aliases[i].alias, >> + strlen(ppc_cpu_aliases[i].alias)); >> + } >> + } >> } >> >> /* Prepare the device tree */ > > This is still fixing up the name in the wrong place: -cpu POWER7_v2.3 > will not get fixed, only -cpu host or KVM's default. > > The solution I had discussed with Alex is the following: When devices > need to expose their name to firmware in a special way, we have the > DeviceClass::fw_name field. All we have to do is assign it and use it > instead of cpu_model if non-NULL, just like we assign DeviceClass::desc. > The way to do it would be to extend the family of POWERPC_DEF* macros to > specify the additional field on the relevant CPU models. > Would this be the same use-case as reflected by: ppc_cpu_aliases.alias ? If so, do we really need a separate field to convey the same information ? > Therefore my above question: Would it be sufficient to explicitly name > POWER7_v2.3 PowerPC,POWER7 etc. and to drop the upper-casing? > Or would we also need to name a CPU such as MPC8572E (random Freescale > CPU where I don't know the expected fw_name and that is unlikely to > occur/work in sPAPR) "PowerPC,MPC8572E" if someone specified it with > -cpu MPC8572E? > If this is not a PAPR-compliant CPU, I dont think the PAPR naming convention is of any good. I havent worked with non-PAPR cpus. Is the device tree for such CPUs generated by routines in hw/ppc/spapr.c ? Or do they have custom routines to generate appropriate device tree nodes ? Regards,
On Mon, 2013-08-12 at 10:07 +0530, Prerna Saxena wrote: .../... > I dont know what context lead to this observation. > However, PAPR mentions the following nomenclature guideline: > > "The value of this property shall be of the form: “PowerPC,<name>”, > where <name> is the name of the processor chip which may be displayed to > the user. <name> shall not contain underscores." This actually comes from the original Open Firmware binding for PowerPC processors, which PAPR inherits largely from. Thus this naming scheme should apply to all PowerPC processors when a device-tree is involved. > I think this name guideline will hold good for all PAPR compliant > processors. Also PAPR is not a processor architecture, it's a platform and firmware architecture, so "PAPR-compliant CPU" has little meaning :-) Cheers, Ben. > > > >> @@ -1315,6 +1325,14 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args) > >> > >> cpu_model = g_strndup(parent_name, > >> strlen(parent_name) - strlen("-" TYPE_POWERPC_CPU)); > >> + > >> + for (i = 0; ppc_cpu_aliases[i].model != NULL; i++) { > >> + if (strcmp(ppc_cpu_aliases[i].model, cpu_model) == 0) { > >> + g_free(cpu_model); > >> + cpu_model = g_strndup(ppc_cpu_aliases[i].alias, > >> + strlen(ppc_cpu_aliases[i].alias)); > >> + } > >> + } > >> } > >> > >> /* Prepare the device tree */ > > > > This is still fixing up the name in the wrong place: -cpu POWER7_v2.3 > > will not get fixed, only -cpu host or KVM's default. > > > > The solution I had discussed with Alex is the following: When devices > > need to expose their name to firmware in a special way, we have the > > DeviceClass::fw_name field. All we have to do is assign it and use it > > instead of cpu_model if non-NULL, just like we assign DeviceClass::desc. > > The way to do it would be to extend the family of POWERPC_DEF* macros to > > specify the additional field on the relevant CPU models. > > > > Would this be the same use-case as reflected by: ppc_cpu_aliases.alias ? > If so, do we really need a separate field to convey the same information ? > > > Therefore my above question: Would it be sufficient to explicitly name > > POWER7_v2.3 PowerPC,POWER7 etc. and to drop the upper-casing? > > Or would we also need to name a CPU such as MPC8572E (random Freescale > > CPU where I don't know the expected fw_name and that is unlikely to > > occur/work in sPAPR) "PowerPC,MPC8572E" if someone specified it with > > -cpu MPC8572E? > > > > If this is not a PAPR-compliant CPU, I dont think the PAPR naming > convention is of any good. > I havent worked with non-PAPR cpus. Is the device tree for such CPUs > generated by routines in hw/ppc/spapr.c ? Or do they have custom > routines to generate appropriate device tree nodes ? > > Regards,
Am Mon, 12 Aug 2013 16:03:24 +1000 schrieb Benjamin Herrenschmidt <benh@kernel.crashing.org>: > On Mon, 2013-08-12 at 10:07 +0530, Prerna Saxena wrote: > > .../... > > > I dont know what context lead to this observation. > > However, PAPR mentions the following nomenclature guideline: > > > > "The value of this property shall be of the form: “PowerPC,<name>”, > > where <name> is the name of the processor chip which may be displayed to > > the user. <name> shall not contain underscores." > > This actually comes from the original Open Firmware binding for PowerPC > processors, which PAPR inherits largely from. Thus this naming scheme > should apply to all PowerPC processors when a device-tree is involved. Well, I think it should be used when an Open Firmware environment is used. When you boot via ePAPR device tree, the name should be "cpu" instead, according to the ePAPR specification. Thomas
On Mon, 2013-08-12 at 21:17 +0200, Thomas Huth wrote: > Am Mon, 12 Aug 2013 16:03:24 +1000 > schrieb Benjamin Herrenschmidt <benh@kernel.crashing.org>: > > > On Mon, 2013-08-12 at 10:07 +0530, Prerna Saxena wrote: > > > > .../... > > > > > I dont know what context lead to this observation. > > > However, PAPR mentions the following nomenclature guideline: > > > > > > "The value of this property shall be of the form: “PowerPC,<name>”, > > > where <name> is the name of the processor chip which may be displayed to > > > the user. <name> shall not contain underscores." > > > > This actually comes from the original Open Firmware binding for PowerPC > > processors, which PAPR inherits largely from. Thus this naming scheme > > should apply to all PowerPC processors when a device-tree is involved. > > Well, I think it should be used when an Open Firmware environment is > used. When you boot via ePAPR device tree, the name should be "cpu" > instead, according to the ePAPR specification. Yeah well ... this is a gratuituous change in ePAPR, I don't think it matters really what the name is anyway. I'd suggest sticking to the original OF binding. Ben.
On 12.08.2013, at 23:22, Benjamin Herrenschmidt wrote: > On Mon, 2013-08-12 at 21:17 +0200, Thomas Huth wrote: >> Am Mon, 12 Aug 2013 16:03:24 +1000 >> schrieb Benjamin Herrenschmidt <benh@kernel.crashing.org>: >> >>> On Mon, 2013-08-12 at 10:07 +0530, Prerna Saxena wrote: >>> >>> .../... >>> >>>> I dont know what context lead to this observation. >>>> However, PAPR mentions the following nomenclature guideline: >>>> >>>> "The value of this property shall be of the form: “PowerPC,<name>”, >>>> where <name> is the name of the processor chip which may be displayed to >>>> the user. <name> shall not contain underscores." >>> >>> This actually comes from the original Open Firmware binding for PowerPC >>> processors, which PAPR inherits largely from. Thus this naming scheme >>> should apply to all PowerPC processors when a device-tree is involved. >> >> Well, I think it should be used when an Open Firmware environment is >> used. When you boot via ePAPR device tree, the name should be "cpu" >> instead, according to the ePAPR specification. > > Yeah well ... this is a gratuituous change in ePAPR, I don't think it > matters really what the name is anyway. I'd suggest sticking to the > original OF binding. Can't we just include the PowerPC, bit as part of the fw_name field in the class? I don't think we have any CPUs that can be used both in ePAPR and sPAPR environments. So the POWER7 fw_name field would just contain "PowerPC,POWER7" and the device tree creation code merely appends the @%d piece. Alex
Am 14.08.2013 12:18, schrieb Alexander Graf: > > On 12.08.2013, at 23:22, Benjamin Herrenschmidt wrote: > >> On Mon, 2013-08-12 at 21:17 +0200, Thomas Huth wrote: >>> Am Mon, 12 Aug 2013 16:03:24 +1000 >>> schrieb Benjamin Herrenschmidt <benh@kernel.crashing.org>: >>> >>>> On Mon, 2013-08-12 at 10:07 +0530, Prerna Saxena wrote: >>>> >>>> .../... >>>> >>>>> I dont know what context lead to this observation. >>>>> However, PAPR mentions the following nomenclature guideline: >>>>> >>>>> "The value of this property shall be of the form: “PowerPC,<name>”, >>>>> where <name> is the name of the processor chip which may be displayed to >>>>> the user. <name> shall not contain underscores." >>>> >>>> This actually comes from the original Open Firmware binding for PowerPC >>>> processors, which PAPR inherits largely from. Thus this naming scheme >>>> should apply to all PowerPC processors when a device-tree is involved. >>> >>> Well, I think it should be used when an Open Firmware environment is >>> used. When you boot via ePAPR device tree, the name should be "cpu" >>> instead, according to the ePAPR specification. >> >> Yeah well ... this is a gratuituous change in ePAPR, I don't think it >> matters really what the name is anyway. I'd suggest sticking to the >> original OF binding. > > Can't we just include the PowerPC, bit as part of the fw_name field in the class? I don't think we have any CPUs that can be used both in ePAPR and sPAPR environments. So the POWER7 fw_name field would just contain "PowerPC,POWER7" and the device tree creation code merely appends the @%d piece. That's exactly what my patch series does, just be patient. :) Andreas
Am 14.08.2013 12:18, schrieb Alexander Graf:
> So the POWER7 fw_name field would just contain "PowerPC,POWER7" and the device tree creation code merely appends the @%d piece.
Speaking of POWER7, Paul said that POWER7+ would look like
PowerPC,POWER7+@0 but my POWER5+ is
/proc/device-tree/cpus/PowerPC,POWER5@0
without the plus.
Could you double-check the name please?
A preview of my patch with "POWER7+" is here:
https://github.com/afaerber/qemu-cpu/commits/spapr
Thanks,
Andreas
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 59e2fea..8efd84e 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -43,6 +43,7 @@ #include "hw/pci-host/spapr.h" #include "hw/ppc/xics.h" #include "hw/pci/msi.h" +#include "cpu-models.h" #include "hw/pci/pci.h" @@ -80,6 +81,8 @@ #define HTAB_SIZE(spapr) (1ULL << ((spapr)->htab_shift)) +#define PPC_DEVTREE_STR "PowerPC," + sPAPREnvironment *spapr; int spapr_allocate_irq(int hint, bool lsi) @@ -322,9 +325,16 @@ 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); + /* + * PAPR convention mandates that + * Device tree nodes must be named as: + * PowerPC,CPU-NAME@... + * Also, CPU-NAME must not have underscores.(hence use of CPU-ALIAS) + */ + + 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]); } @@ -1315,6 +1325,14 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args) cpu_model = g_strndup(parent_name, strlen(parent_name) - strlen("-" TYPE_POWERPC_CPU)); + + for (i = 0; ppc_cpu_aliases[i].model != NULL; i++) { + if (strcmp(ppc_cpu_aliases[i].model, cpu_model) == 0) { + g_free(cpu_model); + cpu_model = g_strndup(ppc_cpu_aliases[i].alias, + strlen(ppc_cpu_aliases[i].alias)); + } + } } /* Prepare the device tree */