Re: [Qemu-devel] [PATCH v3 03/10] ppc/pnv: add a core mask to PnvChip
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>> --- >> >> 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, >> _fatal); >> object_property_set_int(chip, CHIP_HWID(i), "chip-id", >> _fatal); >> +object_property_set_int(chip, smp_cores, "nr-cores", _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", _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", _fatal); >> } >> g_free(chip_typename); >> } >> >> +/* Allowed core identifiers on a POWER8 Processor Chip : >> + * >> + * >> + * EX1 - Venice only >> + * EX2 - Venice only >> + * EX3 - Venice only >> + * EX4 >> + * EX5 >> + * EX6 >> + * >> + * EX9 - Venice only >> + * EX10 - Venice only >> + * EX11 - Venice only >> + * EX12 >> + * EX13 >> + * EX14 >> + * >> + */ >> +#define POWER8E_CORE_MASK (0x7070ull) >> +#define POWER8_CORE_MASK (0x7e7eull) >> + >> +/* >> + * POWER9 has 24 cores, ids starting at 0x20 >> + */ >> +#define POWER9_CORE_MASK (0xffull) >> + >> 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 = 0x221ef0498000ull; /* 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 = 0x220ea0498000ull; /* 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 = 0x120d30498000ull; /* 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 = 0x100d10498000ull; /* 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
Re: [Qemu-devel] [PATCH v3 03/10] ppc/pnv: add a core mask to PnvChip
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> --- > > 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, > _fatal); > object_property_set_int(chip, CHIP_HWID(i), "chip-id", _fatal); > +object_property_set_int(chip, smp_cores, "nr-cores", _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", _fatal); If you're selecting the default, why set it at all? > object_property_set_bool(chip, true, "realized", _fatal); > } > g_free(chip_typename); > } > > +/* Allowed core identifiers on a POWER8 Processor Chip : > + * > + * > + * EX1 - Venice only > + * EX2 - Venice only > + * EX3 - Venice only > + * EX4 > + * EX5 > + * EX6 > + * > + * EX9 - Venice only > + * EX10 - Venice only > + * EX11 - Venice only > + * EX12 > + * EX13 > + * EX14 > + * > + */ > +#define POWER8E_CORE_MASK (0x7070ull) > +#define POWER8_CORE_MASK (0x7e7eull) > + > +/* > + * POWER9 has 24 cores, ids starting at 0x20 > + */ > +#define POWER9_CORE_MASK (0xffull) > + > 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 = 0x221ef0498000ull; /* 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 = 0x220ea0498000ull; /* 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 = 0x120d30498000ull; /* 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 = 0x100d10498000ull; /* 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 =