Re: [Qemu-ppc] [PATCH v2 6/7] ppc/pnv: add a XScomDevice to PnvCore

[Cédric Le Goater]

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. 
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 {
>