[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [PATCH v2 6/7] ppc/pnv: add a XScomDevice to PnvCore
From: |
David Gibson |
Subject: |
Re: [Qemu-ppc] [PATCH v2 6/7] ppc/pnv: add a XScomDevice to PnvCore |
Date: |
Wed, 7 Sep 2016 11:51:58 +1000 |
User-agent: |
Mutt/1.7.0 (2016-08-17) |
On Tue, Sep 06, 2016 at 03:54:10PM +0200, Cédric Le Goater wrote:
> On 09/05/2016 06:19 AM, David Gibson wrote:
> > On Wed, Aug 31, 2016 at 06:34:14PM +0200, Cédric Le Goater wrote:
> >> Now that we are using real HW ids for the cores in PowerNV chips, we
> >> can route the XSCOM accesses to them. We just need to attach a
> >> XScomDevice to each core with the associated ranges in the XSCOM
> >> address space.
> >>
> >> To start with, let's install the DTS (Digital Thermal Sensor) handlers
> >> which are easy to handle.
> >>
> >> Signed-off-by: Cédric Le Goater <address@hidden>
> >> ---
> >> hw/ppc/pnv.c | 9 +++++++
> >> hw/ppc/pnv_core.c | 67
> >> +++++++++++++++++++++++++++++++++++++++++++++++
> >> include/hw/ppc/pnv_core.h | 13 +++++++++
> >> 3 files changed, 89 insertions(+)
> >>
> >> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> >> index daf9f459ab0e..a31568415192 100644
> >> --- a/hw/ppc/pnv.c
> >> +++ b/hw/ppc/pnv.c
> >> @@ -527,6 +527,7 @@ static void pnv_chip_realize(DeviceState *dev, Error
> >> **errp)
> >> for (i = 0, core_hwid = 0; (core_hwid < sizeof(chip->cores_mask) * 8)
> >> && (i < chip->num_cores); core_hwid++) {
> >> PnvCore *pnv_core = &chip->cores[i];
> >> + DeviceState *qdev;
> >>
> >> if (!(chip->cores_mask & (1 << core_hwid))) {
> >> continue;
> >> @@ -542,6 +543,14 @@ static void pnv_chip_realize(DeviceState *dev, Error
> >> **errp)
> >> &error_fatal);
> >> object_unref(OBJECT(pnv_core));
> >> i++;
> >> +
> >> + /* Attach the core to its XSCOM bus */
> >> + qdev = qdev_create(&chip->xscom->bus, TYPE_PNV_CORE_XSCOM);
> >
> > Again, I think this breaks QOM lifetime rules.
> >
> >> + qdev_prop_set_uint32(qdev, "core-pir",
> >> + P8_PIR(chip->chip_id, core_hwid));
> >> + qdev_init_nofail(qdev);
> >
> > qdev_init_nofail() - which will abort on failure - shouldn't be used
> > in a realize() function, which has a way to gracefully report errors.
>
> yes. I kind of knew that was ugly but the main purpose of the patch
> is the use of scom. So I took the quick and dirty path :)
>
> we should do what you propose below:
>
> > So, in terms of the lifetime thing. I think one permitted solution is
> > to embed the scom device state in the core device state and use
> > object_initialize().
>
> yes.
>
> > Alternatively, since SCOM is by its nature a sideband bus, I wonder if
> > it would make sense to have ScomDevice be a QOM interface, rather than
> > a full QOM class. That way the core device itself (and other devices
> > with SCOM control) could present the SCOM device interface and be
> > placed directly on the SCOM bus without having to introduce an extra
> > object.
>
> what you are proposing is to have a PnvCore inherit from CPUCore and
> XScomDevice ? I tried that and did not find a way for multi-inheritance.
QOM supports multiple interface inheritance, but not multiple direct
inheritance. That's why youd need to change XScomDevice from a class
to an interface, as I propose in a different mail. Not sure if that
will cause other problems with the qbus infrastructure. Have a look
at say, 'HotplugHandler' for an example of a QOM interface.
> But yes, we would get rid of the extra object. At the same time, that
> extra object represents the xscom interface unit in a chiplet which
> exists on real HW.
>
> > It will probably make accessing the object innards in response to
> > SCOM requests more straightforward as well.
>
> The address space is probably the best approach. I have some experimental
> patches which look pretty good :
>
> address-space: xscom-0
> 0000000000000000-00000007ffffffff (prio 0, RW): xscom-0
> 0000000000b00200-0000000000b0021f (prio 0, RW): lpc-xscom
> 0000000120000000-00000001207fffff (prio 0, RW): core-20-xscom
> 0000000121000000-00000001217fffff (prio 0, RW): core-21-xscom
> 0000000122000000-00000001227fffff (prio 0, RW): core-22-xscom
> 0000000123000000-00000001237fffff (prio 0, RW): core-23-xscom
> 0000000124000000-00000001247fffff (prio 0, RW): core-24-xscom
> 0000000125000000-00000001257fffff (prio 0, RW): core-25-xscom
> 0000000126000000-00000001267fffff (prio 0, RW): core-26-xscom
> ....
>
>
> > I'm not entirely sure if sharing just an interface will be sufficient
> > for devices under a bus, but it's worth investigating.
>
> yes. I need to step back a bit and look how I could rework the patchset
> to QOMify the whole. This is really qdev oriented and there are a couple
> of places where fixes are needed. I am seeing that better now.
>
> The address space should be investigated also.
>
> >> +
> >> + pnv_core->xd = PNV_CORE_XSCOM(qdev);
> >> }
> >> g_free(typename);
> >>
> >> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> >> index 825aea1194a1..feba374740dc 100644
> >> --- a/hw/ppc/pnv_core.c
> >> +++ b/hw/ppc/pnv_core.c
> >> @@ -18,7 +18,9 @@
> >> */
> >> #include "qemu/osdep.h"
> >> #include "sysemu/sysemu.h"
> >> +#include "qemu/error-report.h"
> >> #include "qapi/error.h"
> >> +#include "qemu/log.h"
> >> #include "target-ppc/cpu.h"
> >> #include "hw/ppc/ppc.h"
> >> #include "hw/ppc/pnv.h"
> >> @@ -144,10 +146,75 @@ static const TypeInfo pnv_core_info = {
> >> .abstract = true,
> >> };
> >>
> >> +
> >> +#define DTS_RESULT0 0x50000
> >> +#define DTS_RESULT1 0x50001
> >> +
> >> +static bool pnv_core_xscom_read(XScomDevice *dev, uint32_t range,
> >> + uint32_t offset, uint64_t *out_val)
> >> +{
> >> + switch (offset) {
> >> + case DTS_RESULT0:
> >> + *out_val = 0x26f024f023f0000ull;
> >> + break;
> >> + case DTS_RESULT1:
> >> + *out_val = 0x24f000000000000ull;
> >> + break;
> >> + default:
> >> + qemu_log_mask(LOG_GUEST_ERROR, "Warning: reading reg=0x%08x",
> >> offset);
> >
> > Can't you just return false here and let the caller handle the error
> > reporting?
>
> yes.
>
> Thanks,
>
> C.
>
> >> + }
> >> +
> >> + return true;
> >> +}
> >> +
> >> +static bool pnv_core_xscom_write(XScomDevice *dev, uint32_t range,
> >> + uint32_t offset, uint64_t val)
> >> +{
> >> + qemu_log_mask(LOG_GUEST_ERROR, "Warning: writing to reg=0x%08x",
> >> offset);
> >> + return true;
> >> +}
> >> +
> >> +#define EX_XSCOM_BASE 0x10000000
> >> +#define EX_XSCOM_SIZE 0x100000
> >> +
> >> +static void pnv_core_xscom_realize(DeviceState *dev, Error **errp)
> >> +{
> >> + XScomDevice *xd = XSCOM_DEVICE(dev);
> >> + PnvCoreXScom *pnv_xd = PNV_CORE_XSCOM(dev);
> >> +
> >> + xd->ranges[0].addr = EX_XSCOM_BASE | P8_PIR2COREID(pnv_xd->core_pir)
> >> << 24;
> >> + xd->ranges[0].size = EX_XSCOM_SIZE;
> >> +}
> >> +
> >> +static Property pnv_core_xscom_properties[] = {
> >> + DEFINE_PROP_UINT32("core-pir", PnvCoreXScom, core_pir, 0),
> >> + DEFINE_PROP_END_OF_LIST(),
> >> +};
> >> +
> >> +static void pnv_core_xscom_class_init(ObjectClass *klass, void *data)
> >> +{
> >> + DeviceClass *dc = DEVICE_CLASS(klass);
> >> + XScomDeviceClass *xdc = XSCOM_DEVICE_CLASS(klass);
> >> +
> >> + xdc->read = pnv_core_xscom_read;
> >> + xdc->write = pnv_core_xscom_write;
> >> +
> >> + dc->realize = pnv_core_xscom_realize;
> >> + dc->props = pnv_core_xscom_properties;
> >> +}
> >> +
> >> +static const TypeInfo pnv_core_xscom_type_info = {
> >> + .name = TYPE_PNV_CORE_XSCOM,
> >> + .parent = TYPE_XSCOM_DEVICE,
> >> + .instance_size = sizeof(PnvCoreXScom),
> >> + .class_init = pnv_core_xscom_class_init,
> >> +};
> >> +
> >> static void pnv_core_register_types(void)
> >> {
> >> int i ;
> >>
> >> + type_register_static(&pnv_core_xscom_type_info);
> >> type_register_static(&pnv_core_info);
> >> for (i = 0; i < ARRAY_SIZE(pnv_core_models); ++i) {
> >> TypeInfo ti = {
> >> diff --git a/include/hw/ppc/pnv_core.h b/include/hw/ppc/pnv_core.h
> >> index 832c8756afaa..72936ccfd22f 100644
> >> --- a/include/hw/ppc/pnv_core.h
> >> +++ b/include/hw/ppc/pnv_core.h
> >> @@ -20,6 +20,18 @@
> >> #define _PPC_PNV_CORE_H
> >>
> >> #include "hw/cpu/core.h"
> >> +#include "hw/ppc/pnv_xscom.h"
> >> +
> >> +#define TYPE_PNV_CORE_XSCOM "powernv-cpu-core-xscom"
> >> +#define PNV_CORE_XSCOM(obj) \
> >> + OBJECT_CHECK(PnvCoreXScom, (obj), TYPE_PNV_CORE_XSCOM)
> >> +
> >> +typedef struct PnvCoreXScom {
> >> + XScomDevice xd;
> >> + uint32_t core_pir;
> >> +} PnvCoreXScom;
> >> +
> >> +#define P8_PIR2COREID(pir) (((pir) >> 3) & 0xf)
> >>
> >> #define TYPE_PNV_CORE "powernv-cpu-core"
> >> #define PNV_CORE(obj) \
> >> @@ -35,6 +47,7 @@ typedef struct PnvCore {
> >>
> >> /*< public >*/
> >> void *threads;
> >> + PnvCoreXScom *xd;
> >> } PnvCore;
> >>
> >> typedef struct PnvCoreClass {
> >
>
--
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