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

[David Gibson]

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.


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().

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.  It will probably make accessing the object innards in
response to SCOM requests more straightforward as well.

I'm not entirely sure if sharing just an interface will be sufficient
for devices under a bus, but it's worth investigating.

> +
> +        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?

> +    }
> +
> +   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