[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [PATCH v2 2/7] ppc/pnv: add a PnvChip object
From: |
David Gibson |
Subject: |
Re: [Qemu-ppc] [PATCH v2 2/7] ppc/pnv: add a PnvChip object |
Date: |
Tue, 6 Sep 2016 10:52:26 +1000 |
User-agent: |
Mutt/1.7.0 (2016-08-17) |
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 <address@hidden>
> >> ---
> >>
> >> 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
signature.asc
Description: PGP signature
Re: [Qemu-ppc] [PATCH v2 2/7] ppc/pnv: add a PnvChip object, Cédric Le Goater, 2016/09/05
- Re: [Qemu-ppc] [PATCH v2 2/7] ppc/pnv: add a PnvChip object,
David Gibson <=