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 <c...@kaod.org> >> --- >> 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. 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 { >