Re: [Qemu-devel] [PATCH v2 2/7] ppc/pnv: add a PnvChip object

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4538 bytes --]

On Mon, Sep 05, 2016 at 09:56:03AM +0200, Cédric Le Goater wrote:
> On 09/05/2016 04:58 AM, David Gibson wrote:
> > On Wed, Aug 31, 2016 at 06:34:10PM +0200, Cédric Le Goater wrote:
> >> This is is an abstraction of a POWER8 chip which is a set of cores
> >> plus other 'units', like the pervasive unit, the interrupt controller,
> >> the memory controller, the on-chip microcontroller, etc. The whole can
> >> be seen as a socket. It depends on a cpu model and its characteristics,
> >> max cores, specific init are defined in a PnvChipClass.
> >>
> >> We start with an near empty PnvChip with only a few cpu constants
> >> which we will grow in the subsequent patches with the controllers
> >> required to run the system.
> >>
> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >> ---
> >>
> >>  Changes since v1:
> >>  
> >>  - introduced a PnvChipClass depending on the cpu model. It also
> >>    provides some chip constants used by devices, like the cpu model hw
> >>    id (f000f), a enum type (not sure this is useful yet), a custom
> >>    realize ops for customization.
> >>  - the num-chips property can be configured on the command line.
> >>  
> >>  Maybe this object deserves its own file hw/ppc/pnv_chip.c ? 
> >>
> >>  hw/ppc/pnv.c         | 154 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  include/hw/ppc/pnv.h |  71 ++++++++++++++++++++++++
> >>  2 files changed, 225 insertions(+)
> >>
> >> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> >> index 70413e3c5740..06051268e200 100644
> >> --- a/hw/ppc/pnv.c
> >> +++ b/hw/ppc/pnv.c
> >> @@ -168,6 +168,8 @@ static void ppc_powernv_init(MachineState *machine)
> >>      char *fw_filename;
> >>      long fw_size;
> >>      long kernel_size;
> >> +    int i;
> >> +    char *chip_typename;
> >>  
> >>      /* allocate RAM */
> >>      if (ram_size < (1 * G_BYTE)) {
> >> @@ -212,6 +214,153 @@ static void ppc_powernv_init(MachineState *machine)
> >>              exit(1);
> >>          }
> >>      }
> >> +
> >> +    /* Create the processor chips */
> >> +    chip_typename = g_strdup_printf(TYPE_PNV_CHIP "-%s", machine->cpu_model);
> >> +
> >> +    pnv->chips = g_new0(PnvChip *, pnv->num_chips);
> >> +    for (i = 0; i < pnv->num_chips; i++) {
> >> +        Object *chip = object_new(chip_typename);
> >> +        object_property_set_int(chip, CHIP_HWID(i), "chip-id", &error_abort);
> >> +        object_property_set_bool(chip, true, "realized", &error_abort);
> > 
> > I'm guessing these could fail due to bad user supplied parameters, not
> > just internal bugs.  In which case this should be &error_fatal rather
> > than &error_abort.
> 
> yes.
> 
> > 
> >> +        pnv->chips[i] = PNV_CHIP(chip);
> >> +    }
> >> +    g_free(chip_typename);
> >> +}
> >> +
> >> +static void pnv_chip_power8nvl_realize(PnvChip *chip, Error **errp)
> >> +{
> >> +    ;
> >> +}
> > 
> > I don't think you should need to define an empty realize function.  
> > Or is this just a placeholder for things future patches will add?
> 
> yes that was the plan but maybe this is a little early. chip_type
> proved to be useful enough for the moment in the full patchset 
> I maintain.
> 
> P9 will use a xive object when available and not a xics. I think 
> this is when the real big difference will show up. So I should 
> make realize() optional.
> 
> >> +
> >> +static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data)
> >> +{
> >> +    DeviceClass *dc = DEVICE_CLASS(klass);
> >> +    PnvChipClass *k = PNV_CHIP_CLASS(klass);
> >> +
> >> +    k->realize = pnv_chip_power8nvl_realize;
> >> +    k->cpu_model = "POWER8NVL";
> >> +    k->chip_type = PNV_CHIP_P8NVL;
> > 
> > With the new chip class per cpu class, does this chip_type field serve
> > any purpose any more?
> 
> It does look a bit redundant, I agree. But it is useful for xscom 
> address translation (P9 is a little different), for xscom devices 
> in general, for core id to pir conversion. It also does for lpc_irq 
> support, which applies  to P8NVL (and upper I suppose). 
> 
> Let's keep it for the moment as it serves its purpose, which is 
> to handle small differences without too much cost. If this is 
> going too far, I will propose a set of ops per chip type.

Ok, fair enough.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]