Message ID | 1473943560-14846-4-git-send-email-clg@kaod.org |
---|---|
State | New |
Headers | show |
On Thu, Sep 15, 2016 at 02:45:53PM +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. > > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > > 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 | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > include/hw/ppc/pnv.h | 4 ++++ > 2 files changed, 70 insertions(+) > > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c > index 2aa5be56c8dc..ec7dd6ac5ea1 100644 > --- a/hw/ppc/pnv.c > +++ b/hw/ppc/pnv.c > @@ -226,11 +226,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); If you're selecting the default, why set it at all? > 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); > @@ -239,6 +272,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"; > } > > @@ -257,6 +291,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"; > } > > @@ -275,6 +310,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"; > } > > @@ -293,6 +329,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"; > } > > @@ -303,11 +340,38 @@ static const TypeInfo pnv_chip_power9_info = { > .class_init = pnv_chip_power9_class_init, > }; > > +static void pnv_chip_core_sanitize(PnvChip *chip) > +{ > + PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip); > + int cores_max = hweight_long(pcc->cores_mask); > + > + if (chip->nr_cores > cores_max) { > + error_report("warning: too many cores for chip ! Limiting to %d", > + cores_max); > + chip->nr_cores = cores_max; This is called from realize() which takes an errp argument. It would be better to pass that in and actually report the error up the chain here, rather than assuming a warning is the right answer. Also.. shouldn't you actually check nr_cores against the chip local mask instead of the class one? > + } > + > + /* 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_report("warning: invalid core mask for chip !"); Likewise here you should propagate the error. > + } > + chip->cores_mask &= pcc->cores_mask; > +} > + > static void pnv_chip_realize(DeviceState *dev, Error **errp) > { > PnvChip *chip = PNV_CHIP(dev); > PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip); > > + /* Early checks on the core settings */ > + pnv_chip_core_sanitize(chip); > + > if (pcc->realize) { > pcc->realize(chip, errp); > } > @@ -315,6 +379,8 @@ static void pnv_chip_realize(DeviceState *dev, Error **errp) > > 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 6e6628edcf6a..cfc32586320f 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; > > void (*realize)(PnvChip *dev, Error **errp); > } PnvChipClass;
On 09/20/2016 03:57 PM, David Gibson wrote: > On Thu, Sep 15, 2016 at 02:45:53PM +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. >> >> Signed-off-by: Cédric Le Goater <clg@kaod.org> >> --- >> >> 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 | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++ >> include/hw/ppc/pnv.h | 4 ++++ >> 2 files changed, 70 insertions(+) >> >> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c >> index 2aa5be56c8dc..ec7dd6ac5ea1 100644 >> --- a/hw/ppc/pnv.c >> +++ b/hw/ppc/pnv.c >> @@ -226,11 +226,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); > > If you're selecting the default, why set it at all? To remind that we could add a machine property to define the layout of the cores in each chip on the command line, something like : -machine powernv,accel=tcg,num-chips=2,cores-mask="0xdeadbeef,0xbadc0ffee" That would be nice to test different layouts. > >> 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); >> @@ -239,6 +272,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"; >> } >> >> @@ -257,6 +291,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"; >> } >> >> @@ -275,6 +310,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"; >> } >> >> @@ -293,6 +329,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"; >> } >> >> @@ -303,11 +340,38 @@ static const TypeInfo pnv_chip_power9_info = { >> .class_init = pnv_chip_power9_class_init, >> }; >> >> +static void pnv_chip_core_sanitize(PnvChip *chip) >> +{ >> + PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip); >> + int cores_max = hweight_long(pcc->cores_mask); >> + >> + if (chip->nr_cores > cores_max) { >> + error_report("warning: too many cores for chip ! Limiting to %d", >> + cores_max); >> + chip->nr_cores = cores_max; > > This is called from realize() which takes an errp argument. It would > be better to pass that in and actually report the error up the chain > here, rather than assuming a warning is the right answer. yes. I forgot that. > Also.. shouldn't you actually check nr_cores against the chip local > mask instead of the class one? I need to rework a bit that part because I want to use the mask for the "cpu hwid" -> "icp index" mapping table. So I would need to modify chip->cores_mask depending on chip->nr_cores in the case we ask for less cores than there is in the mask. Thanks, C. >> + } >> + >> + /* 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_report("warning: invalid core mask for chip !"); > > Likewise here you should propagate the error. > >> + } >> + chip->cores_mask &= pcc->cores_mask; >> +} >> + >> static void pnv_chip_realize(DeviceState *dev, Error **errp) >> { >> PnvChip *chip = PNV_CHIP(dev); >> PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip); >> >> + /* Early checks on the core settings */ >> + pnv_chip_core_sanitize(chip); >> + >> if (pcc->realize) { >> pcc->realize(chip, errp); >> } >> @@ -315,6 +379,8 @@ static void pnv_chip_realize(DeviceState *dev, Error **errp) >> >> 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 6e6628edcf6a..cfc32586320f 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; >> >> void (*realize)(PnvChip *dev, Error **errp); >> } PnvChipClass; >
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c index 2aa5be56c8dc..ec7dd6ac5ea1 100644 --- a/hw/ppc/pnv.c +++ b/hw/ppc/pnv.c @@ -226,11 +226,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); @@ -239,6 +272,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"; } @@ -257,6 +291,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"; } @@ -275,6 +310,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"; } @@ -293,6 +329,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"; } @@ -303,11 +340,38 @@ static const TypeInfo pnv_chip_power9_info = { .class_init = pnv_chip_power9_class_init, }; +static void pnv_chip_core_sanitize(PnvChip *chip) +{ + PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip); + int cores_max = hweight_long(pcc->cores_mask); + + if (chip->nr_cores > cores_max) { + error_report("warning: too many cores for chip ! Limiting to %d", + cores_max); + chip->nr_cores = 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_report("warning: invalid core mask for chip !"); + } + chip->cores_mask &= pcc->cores_mask; +} + static void pnv_chip_realize(DeviceState *dev, Error **errp) { PnvChip *chip = PNV_CHIP(dev); PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip); + /* Early checks on the core settings */ + pnv_chip_core_sanitize(chip); + if (pcc->realize) { pcc->realize(chip, errp); } @@ -315,6 +379,8 @@ static void pnv_chip_realize(DeviceState *dev, Error **errp) 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 6e6628edcf6a..cfc32586320f 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; void (*realize)(PnvChip *dev, Error **errp); } PnvChipClass;
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 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 | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++ include/hw/ppc/pnv.h | 4 ++++ 2 files changed, 70 insertions(+)