| Message ID | 1475479496-16158-4-git-send-email-clg@kaod.org |
|---|---|
| State | New |
| Headers | show |
On Mon, Oct 03, 2016 at 09:24:39AM +0200, Cédric Le Goater wrote: > This will be used to build real HW ids for the cores and enforce some > limits on the available cores per chip. Is there actually a practical reason to allow the user (or machine type) to override the default core mask? > > Signed-off-by: Cédric Le Goater <clg@kaod.org> Apart from the above and one comment below, Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > --- > > Changes since v3 : > > - reworked pnv_chip_core_sanitize() to return errors and to check the > maximum of cores against the instance cores_mask > > Changes since v2 : > > - added POWER9 support > - removed cores_max > - introduces a pnv_chip_core_sanitize() helper to check the core > ids_mask and the maximum number of cores > > hw/ppc/pnv.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++- > include/hw/ppc/pnv.h | 4 +++ > 2 files changed, 81 insertions(+), 1 deletion(-) > > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c > index 08f72dbdca97..fc930be94f53 100644 > --- a/hw/ppc/pnv.c > +++ b/hw/ppc/pnv.c > @@ -227,11 +227,44 @@ static void ppc_powernv_init(MachineState *machine) > snprintf(chip_name, sizeof(chip_name), "chip[%d]", CHIP_HWID(i)); > object_property_add_child(OBJECT(pnv), chip_name, chip, &error_fatal); > object_property_set_int(chip, CHIP_HWID(i), "chip-id", &error_fatal); > + object_property_set_int(chip, smp_cores, "nr-cores", &error_fatal); > + /* > + * We could customize cores_mask for the chip here. May be > + * using a powernv machine property, like 'num-chips'. Let the > + * chip choose the default for now. I don't think you need any special mechanism for this. If you just remove this explicit assignment the chip default will apply, but the user can alter it using -global. > + */ > + object_property_set_int(chip, 0x0, "cores-mask", &error_fatal); > object_property_set_bool(chip, true, "realized", &error_fatal); > } > g_free(chip_typename); > } > > +/* Allowed core identifiers on a POWER8 Processor Chip : > + * > + * <EX0 reserved> > + * EX1 - Venice only > + * EX2 - Venice only > + * EX3 - Venice only > + * EX4 > + * EX5 > + * EX6 > + * <EX7,8 reserved> <reserved> > + * EX9 - Venice only > + * EX10 - Venice only > + * EX11 - Venice only > + * EX12 > + * EX13 > + * EX14 > + * <EX15 reserved> > + */ > +#define POWER8E_CORE_MASK (0x7070ull) > +#define POWER8_CORE_MASK (0x7e7eull) > + > +/* > + * POWER9 has 24 cores, ids starting at 0x20 > + */ > +#define POWER9_CORE_MASK (0xffffff00000000ull) > + > static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > @@ -240,6 +273,7 @@ static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data) > k->cpu_model = "POWER8E"; > k->chip_type = PNV_CHIP_POWER8E; > k->chip_cfam_id = 0x221ef04980000000ull; /* P8 Murano DD2.1 */ > + k->cores_mask = POWER8E_CORE_MASK; > dc->desc = "PowerNV Chip POWER8E"; > } > > @@ -258,6 +292,7 @@ static void pnv_chip_power8_class_init(ObjectClass *klass, void *data) > k->cpu_model = "POWER8"; > k->chip_type = PNV_CHIP_POWER8; > k->chip_cfam_id = 0x220ea04980000000ull; /* P8 Venice DD2.0 */ > + k->cores_mask = POWER8_CORE_MASK; > dc->desc = "PowerNV Chip POWER8"; > } > > @@ -276,6 +311,7 @@ static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data) > k->cpu_model = "POWER8NVL"; > k->chip_type = PNV_CHIP_POWER8NVL; > k->chip_cfam_id = 0x120d304980000000ull; /* P8 Naples DD1.0 */ > + k->cores_mask = POWER8_CORE_MASK; > dc->desc = "PowerNV Chip POWER8NVL"; > } > > @@ -294,6 +330,7 @@ static void pnv_chip_power9_class_init(ObjectClass *klass, void *data) > k->cpu_model = "POWER9"; > k->chip_type = PNV_CHIP_POWER9; > k->chip_cfam_id = 0x100d104980000000ull; /* P9 Nimbus DD1.0 */ > + k->cores_mask = POWER9_CORE_MASK; > dc->desc = "PowerNV Chip POWER9"; > } > > @@ -304,13 +341,52 @@ static const TypeInfo pnv_chip_power9_info = { > .class_init = pnv_chip_power9_class_init, > }; > > +static void pnv_chip_core_sanitize(PnvChip *chip, Error **errp) > +{ > + PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip); > + int cores_max; > + > + /* > + * No custom mask for this chip, let's use the default one from * > + * the chip class > + */ > + if (!chip->cores_mask) { > + chip->cores_mask = pcc->cores_mask; > + } > + > + /* filter alien core ids ! some are reserved */ > + if ((chip->cores_mask & pcc->cores_mask) != chip->cores_mask) { > + error_setg(errp, "warning: invalid core mask for chip !"); > + return; > + } > + chip->cores_mask &= pcc->cores_mask; > + > + /* now that we have a sane layout, let check the number of cores */ > + cores_max = hweight_long(chip->cores_mask); > + if (chip->nr_cores > cores_max) { > + error_setg(errp, "warning: too many cores for chip ! Limit is %d", > + cores_max); > + return; > + } > +} > + > static void pnv_chip_realize(DeviceState *dev, Error **errp) > { > - /* left purposely empty */ > + PnvChip *chip = PNV_CHIP(dev); > + Error *error = NULL; > + > + /* Early checks on the core settings */ > + pnv_chip_core_sanitize(chip, &error); > + if (error) { > + error_propagate(errp, error); > + return; > + } > } > > static Property pnv_chip_properties[] = { > DEFINE_PROP_UINT32("chip-id", PnvChip, chip_id, 0), > + DEFINE_PROP_UINT32("nr-cores", PnvChip, nr_cores, 1), > + DEFINE_PROP_UINT64("cores-mask", PnvChip, cores_mask, 0x0), > DEFINE_PROP_END_OF_LIST(), > }; > > diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h > index da543ed81636..2c225c928974 100644 > --- a/include/hw/ppc/pnv.h > +++ b/include/hw/ppc/pnv.h > @@ -42,6 +42,9 @@ typedef struct PnvChip { > > /*< public >*/ > uint32_t chip_id; > + > + uint32_t nr_cores; > + uint64_t cores_mask; > } PnvChip; > > typedef struct PnvChipClass { > @@ -52,6 +55,7 @@ typedef struct PnvChipClass { > const char *cpu_model; > PnvChipType chip_type; > uint64_t chip_cfam_id; > + uint64_t cores_mask; > } PnvChipClass; > > #define TYPE_PNV_CHIP_POWER8E TYPE_PNV_CHIP "-POWER8E"
On Fri, 2016-10-07 at 15:32 +1100, David Gibson wrote: > On Mon, Oct 03, 2016 at 09:24:39AM +0200, Cédric Le Goater wrote: > > This will be used to build real HW ids for the cores and enforce > some > > limits on the available cores per chip. > > Is there actually a practical reason to allow the user (or machine > type) to override the default core mask? None other than mimmicing real HW ... some cores can be disabled on some chips and we *might* want to mimmic that for some test scenarios. Cheers, Ben.
On Fri, Oct 07, 2016 at 04:01:52PM +1100, Benjamin Herrenschmidt wrote: > On Fri, 2016-10-07 at 15:32 +1100, David Gibson wrote: > > On Mon, Oct 03, 2016 at 09:24:39AM +0200, Cédric Le Goater wrote: > > > This will be used to build real HW ids for the cores and enforce > > some > > > limits on the available cores per chip. > > > > Is there actually a practical reason to allow the user (or machine > > type) to override the default core mask? > > None other than mimmicing real HW ... some cores can be disabled > on some chips and we *might* want to mimmic that for some test > scenarios. Ok, sounds like a good enough reason to me.
On 10/07/2016 07:11 AM, David Gibson wrote: > On Fri, Oct 07, 2016 at 04:01:52PM +1100, Benjamin Herrenschmidt wrote: >> On Fri, 2016-10-07 at 15:32 +1100, David Gibson wrote: >>> On Mon, Oct 03, 2016 at 09:24:39AM +0200, Cédric Le Goater wrote: >>>> This will be used to build real HW ids for the cores and enforce >>> some >>>> limits on the available cores per chip. >>> >>> Is there actually a practical reason to allow the user (or machine >>> type) to override the default core mask? >> >> None other than mimmicing real HW ... some cores can be disabled >> on some chips and we *might* want to mimmic that for some test >> scenarios. > > Ok, sounds like a good enough reason to me. There are really different layouts on the field. Here is a S824, 16 cores, 4 sockets, with these HW ids : 0x4 0x4 0x5 0x4 0x5 0x5 0xc 0x5 0x6 0x6 0xd 0x6 0xe 0xe 0xe 0xd Long term, we will want to support unplug I suppose. C.
>> @@ -227,11 +227,44 @@ static void ppc_powernv_init(MachineState *machine) >> snprintf(chip_name, sizeof(chip_name), "chip[%d]", CHIP_HWID(i)); >> object_property_add_child(OBJECT(pnv), chip_name, chip, &error_fatal); >> object_property_set_int(chip, CHIP_HWID(i), "chip-id", &error_fatal); >> + object_property_set_int(chip, smp_cores, "nr-cores", &error_fatal); >> + /* >> + * We could customize cores_mask for the chip here. May be >> + * using a powernv machine property, like 'num-chips'. Let the >> + * chip choose the default for now. > > I don't think you need any special mechanism for this. If you just > remove this explicit assignment the chip default will apply, but the > user can alter it using -global. Using a command line with : -global powernv-chip-POWER8.cores-mask=0x7070 would work for one chip but not for more. Let's start with that, I will remove the comment for now. multiple chip is for later. Thanks, C.
On Mon, Oct 10, 2016 at 02:56:25PM +0200, Cédric Le Goater wrote: > > >> @@ -227,11 +227,44 @@ static void ppc_powernv_init(MachineState *machine) > >> snprintf(chip_name, sizeof(chip_name), "chip[%d]", CHIP_HWID(i)); > >> object_property_add_child(OBJECT(pnv), chip_name, chip, &error_fatal); > >> object_property_set_int(chip, CHIP_HWID(i), "chip-id", &error_fatal); > >> + object_property_set_int(chip, smp_cores, "nr-cores", &error_fatal); > >> + /* > >> + * We could customize cores_mask for the chip here. May be > >> + * using a powernv machine property, like 'num-chips'. Let the > >> + * chip choose the default for now. > > > > I don't think you need any special mechanism for this. If you just > > remove this explicit assignment the chip default will apply, but the > > user can alter it using -global. > > Using a command line with : > > -global powernv-chip-POWER8.cores-mask=0x7070 > > would work for one chip but not for more. Let's start with that, I will > remove the comment for now. multiple chip is for later. Well, it works for more than one chip if you want the same mask for each of them. If you want different masks, I think you can still do it with -set, but working out the right arguments can be a PITA.
On 10/11/2016 12:24 PM, David Gibson wrote: > On Mon, Oct 10, 2016 at 02:56:25PM +0200, Cédric Le Goater wrote: >> >>>> @@ -227,11 +227,44 @@ static void ppc_powernv_init(MachineState *machine) >>>> snprintf(chip_name, sizeof(chip_name), "chip[%d]", CHIP_HWID(i)); >>>> object_property_add_child(OBJECT(pnv), chip_name, chip, &error_fatal); >>>> object_property_set_int(chip, CHIP_HWID(i), "chip-id", &error_fatal); >>>> + object_property_set_int(chip, smp_cores, "nr-cores", &error_fatal); >>>> + /* >>>> + * We could customize cores_mask for the chip here. May be >>>> + * using a powernv machine property, like 'num-chips'. Let the >>>> + * chip choose the default for now. >>> >>> I don't think you need any special mechanism for this. If you just >>> remove this explicit assignment the chip default will apply, but the >>> user can alter it using -global. >> >> Using a command line with : >> >> -global powernv-chip-POWER8.cores-mask=0x7070 >> >> would work for one chip but not for more. Let's start with that, I will >> remove the comment for now. multiple chip is for later. > > Well, it works for more than one chip if you want the same mask for > each of them. If you want different masks, I think you can still do > it with -set, but working out the right arguments can be a PITA. That would be the best solution but I did not find a way to address one chip object to do a "qom set". (qemu) info qom-tree /machine (powernv-machine) /unattached (container) /system[0] (qemu:memory-region) /sysbus (System) /ppc_powernv.ram[0] (qemu:memory-region) /io[0] (qemu:memory-region) /peripheral-anon (container) /peripheral (container) /chip[1] (powernv-chip-POWER8) ... /chip[0] (powernv-chip-POWER8) ... We will have a similar need with the ram to spread the contents on the chips. C.
On Wed, Oct 12, 2016 at 10:53:12AM +0200, Cédric Le Goater wrote: > On 10/11/2016 12:24 PM, David Gibson wrote: > > On Mon, Oct 10, 2016 at 02:56:25PM +0200, Cédric Le Goater wrote: > >> > >>>> @@ -227,11 +227,44 @@ static void ppc_powernv_init(MachineState *machine) > >>>> snprintf(chip_name, sizeof(chip_name), "chip[%d]", CHIP_HWID(i)); > >>>> object_property_add_child(OBJECT(pnv), chip_name, chip, &error_fatal); > >>>> object_property_set_int(chip, CHIP_HWID(i), "chip-id", &error_fatal); > >>>> + object_property_set_int(chip, smp_cores, "nr-cores", &error_fatal); > >>>> + /* > >>>> + * We could customize cores_mask for the chip here. May be > >>>> + * using a powernv machine property, like 'num-chips'. Let the > >>>> + * chip choose the default for now. > >>> > >>> I don't think you need any special mechanism for this. If you just > >>> remove this explicit assignment the chip default will apply, but the > >>> user can alter it using -global. > >> > >> Using a command line with : > >> > >> -global powernv-chip-POWER8.cores-mask=0x7070 > >> > >> would work for one chip but not for more. Let's start with that, I will > >> remove the comment for now. multiple chip is for later. > > > > Well, it works for more than one chip if you want the same mask for > > each of them. If you want different masks, I think you can still do > > it with -set, but working out the right arguments can be a PITA. > > That would be the best solution but I did not find a way to address one > chip object to do a "qom set". Yeah, that's what I mean working out the arguments can be tricky. > > (qemu) info qom-tree > /machine (powernv-machine) > /unattached (container) > /system[0] (qemu:memory-region) > /sysbus (System) > /ppc_powernv.ram[0] (qemu:memory-region) > /io[0] (qemu:memory-region) > /peripheral-anon (container) > /peripheral (container) > /chip[1] (powernv-chip-POWER8) > ... > /chip[0] (powernv-chip-POWER8) > ... > > We will have a similar need with the ram to spread the contents on > the chips. Sure. Let's cross these bridges when we come to them.
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c index 08f72dbdca97..fc930be94f53 100644 --- a/hw/ppc/pnv.c +++ b/hw/ppc/pnv.c @@ -227,11 +227,44 @@ static void ppc_powernv_init(MachineState *machine) snprintf(chip_name, sizeof(chip_name), "chip[%d]", CHIP_HWID(i)); object_property_add_child(OBJECT(pnv), chip_name, chip, &error_fatal); object_property_set_int(chip, CHIP_HWID(i), "chip-id", &error_fatal); + object_property_set_int(chip, smp_cores, "nr-cores", &error_fatal); + /* + * We could customize cores_mask for the chip here. May be + * using a powernv machine property, like 'num-chips'. Let the + * chip choose the default for now. + */ + object_property_set_int(chip, 0x0, "cores-mask", &error_fatal); object_property_set_bool(chip, true, "realized", &error_fatal); } g_free(chip_typename); } +/* Allowed core identifiers on a POWER8 Processor Chip : + * + * <EX0 reserved> + * EX1 - Venice only + * EX2 - Venice only + * EX3 - Venice only + * EX4 + * EX5 + * EX6 + * <EX7,8 reserved> <reserved> + * EX9 - Venice only + * EX10 - Venice only + * EX11 - Venice only + * EX12 + * EX13 + * EX14 + * <EX15 reserved> + */ +#define POWER8E_CORE_MASK (0x7070ull) +#define POWER8_CORE_MASK (0x7e7eull) + +/* + * POWER9 has 24 cores, ids starting at 0x20 + */ +#define POWER9_CORE_MASK (0xffffff00000000ull) + static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); @@ -240,6 +273,7 @@ static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data) k->cpu_model = "POWER8E"; k->chip_type = PNV_CHIP_POWER8E; k->chip_cfam_id = 0x221ef04980000000ull; /* P8 Murano DD2.1 */ + k->cores_mask = POWER8E_CORE_MASK; dc->desc = "PowerNV Chip POWER8E"; } @@ -258,6 +292,7 @@ static void pnv_chip_power8_class_init(ObjectClass *klass, void *data) k->cpu_model = "POWER8"; k->chip_type = PNV_CHIP_POWER8; k->chip_cfam_id = 0x220ea04980000000ull; /* P8 Venice DD2.0 */ + k->cores_mask = POWER8_CORE_MASK; dc->desc = "PowerNV Chip POWER8"; } @@ -276,6 +311,7 @@ static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data) k->cpu_model = "POWER8NVL"; k->chip_type = PNV_CHIP_POWER8NVL; k->chip_cfam_id = 0x120d304980000000ull; /* P8 Naples DD1.0 */ + k->cores_mask = POWER8_CORE_MASK; dc->desc = "PowerNV Chip POWER8NVL"; } @@ -294,6 +330,7 @@ static void pnv_chip_power9_class_init(ObjectClass *klass, void *data) k->cpu_model = "POWER9"; k->chip_type = PNV_CHIP_POWER9; k->chip_cfam_id = 0x100d104980000000ull; /* P9 Nimbus DD1.0 */ + k->cores_mask = POWER9_CORE_MASK; dc->desc = "PowerNV Chip POWER9"; } @@ -304,13 +341,52 @@ static const TypeInfo pnv_chip_power9_info = { .class_init = pnv_chip_power9_class_init, }; +static void pnv_chip_core_sanitize(PnvChip *chip, Error **errp) +{ + PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip); + int cores_max; + + /* + * No custom mask for this chip, let's use the default one from * + * the chip class + */ + if (!chip->cores_mask) { + chip->cores_mask = pcc->cores_mask; + } + + /* filter alien core ids ! some are reserved */ + if ((chip->cores_mask & pcc->cores_mask) != chip->cores_mask) { + error_setg(errp, "warning: invalid core mask for chip !"); + return; + } + chip->cores_mask &= pcc->cores_mask; + + /* now that we have a sane layout, let check the number of cores */ + cores_max = hweight_long(chip->cores_mask); + if (chip->nr_cores > cores_max) { + error_setg(errp, "warning: too many cores for chip ! Limit is %d", + cores_max); + return; + } +} + static void pnv_chip_realize(DeviceState *dev, Error **errp) { - /* left purposely empty */ + PnvChip *chip = PNV_CHIP(dev); + Error *error = NULL; + + /* Early checks on the core settings */ + pnv_chip_core_sanitize(chip, &error); + if (error) { + error_propagate(errp, error); + return; + } } static Property pnv_chip_properties[] = { DEFINE_PROP_UINT32("chip-id", PnvChip, chip_id, 0), + DEFINE_PROP_UINT32("nr-cores", PnvChip, nr_cores, 1), + DEFINE_PROP_UINT64("cores-mask", PnvChip, cores_mask, 0x0), DEFINE_PROP_END_OF_LIST(), }; diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h index da543ed81636..2c225c928974 100644 --- a/include/hw/ppc/pnv.h +++ b/include/hw/ppc/pnv.h @@ -42,6 +42,9 @@ typedef struct PnvChip { /*< public >*/ uint32_t chip_id; + + uint32_t nr_cores; + uint64_t cores_mask; } PnvChip; typedef struct PnvChipClass { @@ -52,6 +55,7 @@ typedef struct PnvChipClass { const char *cpu_model; PnvChipType chip_type; uint64_t chip_cfam_id; + uint64_t cores_mask; } PnvChipClass; #define TYPE_PNV_CHIP_POWER8E TYPE_PNV_CHIP "-POWER8E"
This will be used to build real HW ids for the cores and enforce some limits on the available cores per chip. Signed-off-by: Cédric Le Goater <clg@kaod.org> --- Changes since v3 : - reworked pnv_chip_core_sanitize() to return errors and to check the maximum of cores against the instance cores_mask Changes since v2 : - added POWER9 support - removed cores_max - introduces a pnv_chip_core_sanitize() helper to check the core ids_mask and the maximum number of cores hw/ppc/pnv.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++- include/hw/ppc/pnv.h | 4 +++ 2 files changed, 81 insertions(+), 1 deletion(-)