Re: [Qemu-devel] [PATCH v3 07/10] ppc/pnv: add XSCOM infrastructure

[Cédric Le Goater]

>> @@ -493,6 +525,8 @@ static void pnv_chip_power9_class_init(ObjectClass 
>> *klass, void *data)
>>      k->chip_cfam_id = 0x100d104980000000ull; /* P9 Nimbus DD1.0 */
>>      k->cores_mask = POWER9_CORE_MASK;
>>      k->core_pir = pnv_chip_core_pir_p9;
>> +    k->xscom_addr = pnv_chip_xscom_addr_p9;
>> +    k->xscom_pcba = pnv_chip_xscom_pcba_p9;
> 
> So if you do as BenH (and I) suggested and have the "scom address
> space" actually be addressed by (pcba << 3), I think you can probably
> avoid these.  

I will look at that option again. 

I was trying to untangle a few things at the same time. I have better
view of the problem to solve now. The bus is gone, that's was one 
thing. How we map these xscom regions is the next. 

Ben suggested to add some P7/P8 mangling before the dispatch in 
the &address_space_xscom. This should make things cleaner. I had 
not thought of doing that and this is why I introduced these helpers :

+uint32_t pnv_xscom_pcba(PnvXScomInterface *dev, uint64_t addr)
+uint64_t pnv_xscom_addr(PnvXScomInterface *dev, uint32_t pcba)

which I don't really like ...

but we must make sure that we can do the mapping of the xscom 
subregions in the &address_space_xscom using (pcba << 3)


> Instead you can handle it in the chip or ADU realize function by either:
> 
>     P8: * map one big subregion for the ADU into &address_space_memory
>         * have the handler for that subregion do the address mangling,
>           then redispatch into the xscom address space
>
>     P9: * Map the appropriate chunk of the xscom address space
>           directly into address_space_memory

Yes that was my feeling for a better solution but Ben chimed in with the 
HMER topic. I need to look at that.


>>      dc->desc = "PowerNV Chip POWER9";
>>  }
>>  
>> @@ -527,6 +561,16 @@ static void pnv_chip_core_sanitize(PnvChip *chip)
>>      chip->cores_mask &= pcc->cores_mask;
>>  }
>>  
>> +static void pnv_chip_init(Object *obj)
>> +{
>> +    PnvChip *chip = PNV_CHIP(obj);
>> +
>> +    object_initialize(&chip->xscom, sizeof(chip->xscom), TYPE_PNV_XSCOM);
>> +    object_property_add_child(obj, "xscom", OBJECT(&chip->xscom), NULL);
>> +    object_property_add_const_link(OBJECT(&chip->xscom), "chip",
>> +                                   OBJECT(chip), &error_abort);
>> +}
>> +
>>  static void pnv_chip_realize(DeviceState *dev, Error **errp)
>>  {
>>      PnvChip *chip = PNV_CHIP(dev);
>> @@ -540,6 +584,12 @@ static void pnv_chip_realize(DeviceState *dev, Error 
>> **errp)
>>          return;
>>      }
>>  
>> +    /* XSCOM bridge */
>> +    object_property_set_bool(OBJECT(&chip->xscom), true, "realized",
>> +                             &error_fatal);
>> +    sysbus_mmio_map(SYS_BUS_DEVICE(&chip->xscom), 0,
>> +                    PNV_XSCOM_BASE(chip->chip_id));
>> +
>>      /* Early checks on the core settings */
>>      pnv_chip_core_sanitize(chip);
>>  
>> @@ -597,6 +647,7 @@ static const TypeInfo pnv_chip_info = {
>>      .name          = TYPE_PNV_CHIP,
>>      .parent        = TYPE_SYS_BUS_DEVICE,
>>      .class_init    = pnv_chip_class_init,
>> +    .instance_init = pnv_chip_init,
>>      .class_size    = sizeof(PnvChipClass),
>>      .abstract      = true,
>>  };
>> diff --git a/hw/ppc/pnv_xscom.c b/hw/ppc/pnv_xscom.c
>> new file mode 100644
>> index 000000000000..019cd85428de
>> --- /dev/null
>> +++ b/hw/ppc/pnv_xscom.c
>> @@ -0,0 +1,308 @@
>> +/*
>> + * QEMU PowerPC PowerNV XSCOM bus
>> + *
>> + * Copyright (c) 2016, IBM Corporation.
>> + *
>> + * This library is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU Lesser General Public
>> + * License as published by the Free Software Foundation; either
>> + * version 2 of the License, or (at your option) any later version.
>> + *
>> + * This library is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * Lesser General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU Lesser General Public
>> + * License along with this library; if not, see 
>> <http://www.gnu.org/licenses/>.
>> + */
>> +#include "qemu/osdep.h"
>> +#include "qapi/error.h"
>> +#include "hw/hw.h"
>> +#include "qemu/log.h"
>> +#include "sysemu/kvm.h"
>> +#include "target-ppc/cpu.h"
>> +#include "hw/sysbus.h"
>> +
>> +#include "hw/ppc/fdt.h"
>> +#include "hw/ppc/pnv_xscom.h"
>> +#include "hw/ppc/pnv.h"
>> +
>> +#include <libfdt.h>
>> +
>> +static void xscom_complete(uint64_t hmer_bits)
>> +{
>> +    CPUState *cs = current_cpu;
> 
> Hmm.. is current_cpu a safe thing to use in the case of KVM or MTTCG?

hmm, indeed. I need to look at that.

>> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
>> +    CPUPPCState *env = &cpu->env;
>> +
>> +    cpu_synchronize_state(cs);
>> +    env->spr[SPR_HMER] |= hmer_bits;
>> +
>> +    /* XXX Need a CPU helper to set HMER, also handle gneeration
>> +     * of HMIs
>> +     */
>> +}
>> +
>> +static bool xscom_dispatch_read(PnvXScom *xscom, hwaddr addr, uint64_t *val)
>> +{
>> +    uint32_t success;
>> +    uint8_t data[8];
>> +
>> +    success = !address_space_rw(&xscom->xscom_as, addr, 
>> MEMTXATTRS_UNSPECIFIED,
>> +                                data, 8, false);
>> +    *val = (((uint64_t) data[0]) << 56 |
>> +            ((uint64_t) data[1]) << 48 |
>> +            ((uint64_t) data[2]) << 40 |
>> +            ((uint64_t) data[3]) << 32 |
>> +            ((uint64_t) data[4]) << 24 |
>> +            ((uint64_t) data[5]) << 16 |
>> +            ((uint64_t) data[6]) << 8  |
>> +            ((uint64_t) data[7]));
> 
> AFAICT this is basically assuming data is always encoded BE.  With the
> right choice of endian flags on the individual SCOM device
> registrations with the scom address space, I think you should be able
> to avoid this mangling.

yes. I should but curiously I had to do this, and this works the same on
an intel host or a ppc64 host.


>> +    return success;
>> +}
>> +
>> +static bool xscom_dispatch_write(PnvXScom *xscom, hwaddr addr, uint64_t val)
>> +{
>> +    uint32_t success;
>> +    uint8_t data[8];
>> +
>> +    data[0] = (val >> 56) & 0xff;
>> +    data[1] = (val >> 48) & 0xff;
>> +    data[2] = (val >> 40) & 0xff;
>> +    data[3] = (val >> 32) & 0xff;
>> +    data[4] = (val >> 24) & 0xff;
>> +    data[5] = (val >> 16) & 0xff;
>> +    data[6] = (val >> 8) & 0xff;
>> +    data[7] = val & 0xff;
>> +
>> +    success = !address_space_rw(&xscom->xscom_as, addr, 
>> MEMTXATTRS_UNSPECIFIED,
>> +                           data, 8, true);
>> +    return success;
>> +}
>> +
>> +static uint64_t xscom_read(void *opaque, hwaddr addr, unsigned width)
>> +{
>> +    PnvXScom *s = opaque;
>> +    uint32_t pcba = s->chip_class->xscom_pcba(addr);
>> +    uint64_t val = 0;
>> +
>> +    /* Handle some SCOMs here before dispatch */
>> +    switch (pcba) {
>> +    case 0xf000f:
>> +        val = s->chip_class->chip_cfam_id;
>> +        break;
>> +    case 0x1010c00:     /* PIBAM FIR */
>> +    case 0x1010c03:     /* PIBAM FIR MASK */
>> +    case 0x2020007:     /* ADU stuff */
>> +    case 0x2020009:     /* ADU stuff */
>> +    case 0x202000f:     /* ADU stuff */
>> +        val = 0;
>> +        break;
>> +    case 0x2013f00:     /* PBA stuff */
>> +    case 0x2013f01:     /* PBA stuff */
>> +    case 0x2013f02:     /* PBA stuff */
>> +    case 0x2013f03:     /* PBA stuff */
>> +    case 0x2013f04:     /* PBA stuff */
>> +    case 0x2013f05:     /* PBA stuff */
>> +    case 0x2013f06:     /* PBA stuff */
>> +    case 0x2013f07:     /* PBA stuff */
>> +        val = 0;
>> +        break;
> 
> It'd be theoretically nicer to actually register regions for these
> special case addresses, but handling it here is a reasonable hack to
> get things working quickly for the time being.

I will make a default region on the whole xscomm address space to catch 
these.

>> +    default:
>> +        if (!xscom_dispatch_read(s, addr, &val)) {
>> +            qemu_log_mask(LOG_GUEST_ERROR, "XSCOM read failed at @0x%"
>> +                          HWADDR_PRIx " pcba=0x%08x\n", addr, pcba);
>> +            xscom_complete(HMER_XSCOM_FAIL | HMER_XSCOM_DONE);
>> +            return 0;
>> +        }
>> +    }
>> +
>> +    xscom_complete(HMER_XSCOM_DONE);
>> +    return val;
>> +}
>> +
>> +static void xscom_write(void *opaque, hwaddr addr, uint64_t val,
>> +                        unsigned width)
>> +{
>> +    PnvXScom *s = opaque;
>> +    uint32_t pcba = s->chip_class->xscom_pcba(addr);
>> +
>> +    /* Handle some SCOMs here before dispatch */
>> +    switch (pcba) {
>> +        /* We ignore writes to these */
>> +    case 0xf000f:       /* chip id is RO */
>> +    case 0x1010c00:     /* PIBAM FIR */
>> +    case 0x1010c01:     /* PIBAM FIR */
>> +    case 0x1010c02:     /* PIBAM FIR */
>> +    case 0x1010c03:     /* PIBAM FIR MASK */
>> +    case 0x1010c04:     /* PIBAM FIR MASK */
>> +    case 0x1010c05:     /* PIBAM FIR MASK */
>> +    case 0x2020007:     /* ADU stuff */
>> +    case 0x2020009:     /* ADU stuff */
>> +    case 0x202000f:     /* ADU stuff */
>> +        break;
>> +    default:
>> +        if (!xscom_dispatch_write(s, addr, val)) {
>> +            qemu_log_mask(LOG_GUEST_ERROR, "XSCOM write failed at @0x%"
>> +                          HWADDR_PRIx " pcba=0x%08x data=0x%" PRIx64 "\n",
>> +                          addr, pcba, val);
>> +            xscom_complete(HMER_XSCOM_FAIL | HMER_XSCOM_DONE);
>> +            return;
>> +        }
>> +    }
>> +
>> +    xscom_complete(HMER_XSCOM_DONE);
>> +}
>> +
>> +const MemoryRegionOps pnv_xscom_ops = {
>> +    .read = xscom_read,
>> +    .write = xscom_write,
>> +    .valid.min_access_size = 8,
>> +    .valid.max_access_size = 8,
>> +    .impl.min_access_size = 8,
>> +    .impl.max_access_size = 8,
>> +    .endianness = DEVICE_BIG_ENDIAN,
>> +};
>> +
>> +static void pnv_xscom_realize(DeviceState *dev, Error **errp)
>> +{
>> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>> +    PnvXScom *s = PNV_XSCOM(dev);
>> +    char *name;
>> +    Object *obj;
>> +    Error *err = NULL;
>> +
>> +    obj = object_property_get_link(OBJECT(s), "chip", &err);
>> +    if (!obj) {
>> +        error_setg(errp, "%s: required link 'chip' not found: %s",
>> +                     __func__, error_get_pretty(err));
>> +        return;
>> +    }
>> +
>> +    s->chip_class = PNV_CHIP_GET_CLASS(obj);
>> +    s->chip_id = PNV_CHIP(obj)->chip_id;
>> +
>> +    if (s->chip_id < 0) {
>> +        error_setg(errp, "invalid chip id '%d'", s->chip_id);
>> +        return;
>> +    }
>> +
>> +    name = g_strdup_printf("xscom-%x", s->chip_id);
>> +    memory_region_init_io(&s->mem, OBJECT(s), &pnv_xscom_ops, s, name,
>> +                          PNV_XSCOM_SIZE);
>> +    sysbus_init_mmio(sbd, &s->mem);
>> +
>> +    memory_region_init(&s->xscom_mr, OBJECT(s), name, PNV_XSCOM_SIZE);
>> +    address_space_init(&s->xscom_as, &s->xscom_mr, name);
>> +    g_free(name);
>> +
>> +}
>> +
>> +static void pnv_xscom_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +
>> +    dc->realize = pnv_xscom_realize;
>> +}
>> +
>> +static const TypeInfo pnv_xscom_info = {
>> +    .name          = TYPE_PNV_XSCOM,
>> +    .parent        = TYPE_SYS_BUS_DEVICE,
>> +    .instance_size = sizeof(PnvXScom),
>> +    .class_init    = pnv_xscom_class_init,
>> +};
>> +
>> +static const TypeInfo pnv_xscom_interface_info = {
>> +    .name = TYPE_PNV_XSCOM_INTERFACE,
>> +    .parent = TYPE_INTERFACE,
>> +    .class_size = sizeof(PnvXScomInterfaceClass),
>> +};
>> +
>> +static void pnv_xscom_register_types(void)
>> +{
>> +    type_register_static(&pnv_xscom_info);
>> +    type_register_static(&pnv_xscom_interface_info);
>> +}
>> +
>> +type_init(pnv_xscom_register_types)
>> +
>> +typedef struct ForeachPopulateArgs {
>> +    void *fdt;
>> +    int xscom_offset;
>> +} ForeachPopulateArgs;
>> +
>> +static int xscom_populate_child(Object *child, void *opaque)
>> +{
>> +    if (object_dynamic_cast(child, TYPE_PNV_XSCOM_INTERFACE)) {
>> +        ForeachPopulateArgs *args = opaque;
>> +        PnvXScomInterface *xd = PNV_XSCOM_INTERFACE(child);;
>> +        PnvXScomInterfaceClass *xc = PNV_XSCOM_INTERFACE_GET_CLASS(xd);
>> +
>> +        if (xc->devnode) {
>> +            _FDT((xc->devnode(xd, args->fdt, args->xscom_offset)));
>> +        }
>> +    }
>> +    return 0;
>> +}
>> +
>> +int pnv_xscom_populate_fdt(PnvXScom *adu, void *fdt, int root_offset)
>> +{
>> +    const char compat[] = "ibm,power8-xscom\0ibm,xscom";
>> +    uint64_t reg[] = { cpu_to_be64(PNV_XSCOM_BASE(adu->chip_id)),
>> +                       cpu_to_be64(PNV_XSCOM_SIZE) };
>> +    int xscom_offset;
>> +    ForeachPopulateArgs args;
>> +    char *name;
>> +
>> +    name = g_strdup_printf("address@hidden" PRIx64, be64_to_cpu(reg[0]));
>> +    xscom_offset = fdt_add_subnode(fdt, root_offset, name);
>> +    _FDT(xscom_offset);
>> +    g_free(name);
>> +    _FDT((fdt_setprop_cell(fdt, xscom_offset, "ibm,chip-id", 
>> adu->chip_id)));
>> +    _FDT((fdt_setprop_cell(fdt, xscom_offset, "#address-cells", 1)));
>> +    _FDT((fdt_setprop_cell(fdt, xscom_offset, "#size-cells", 1)));
>> +    _FDT((fdt_setprop(fdt, xscom_offset, "reg", reg, sizeof(reg))));
>> +    _FDT((fdt_setprop(fdt, xscom_offset, "compatible", compat,
>> +                      sizeof(compat))));
>> +    _FDT((fdt_setprop(fdt, xscom_offset, "scom-controller", NULL, 0)));
>> +
>> +    args.fdt = fdt;
>> +    args.xscom_offset = xscom_offset;
>> +
>> +    object_child_foreach(OBJECT(adu), xscom_populate_child, &args);
>> +    return 0;
>> +}
>> +
>> +/*
>> + * XScom address translation depends on the chip type and not all
>> + * objects have backlink to it. Here's a helper to handle this case.
>> + * To be improved.
>> + */
> 
> Yeah.. this seems a bit hacky as you say.  Passing the individual
> device to these helpers just seems wrong, since it's a chip / machine
> dependent mapping.  Does this go away if you use the (pcba << 3)
> encoding?

Hopefully. the goal is to solve that in v4.

>> +uint32_t pnv_xscom_pcba(PnvXScomInterface *dev, uint64_t addr)
>> +{
>> +    PnvXScomInterfaceClass *xc = PNV_XSCOM_INTERFACE_GET_CLASS(dev);
>> +
>> +    if (!xc->xscom_pcba) {
>> +        PnvMachineState *pnv = POWERNV_MACHINE(qdev_get_machine());
>> +        PnvChipClass *pcc = PNV_CHIP_GET_CLASS(pnv->chips[0]);
>> +
>> +        xc->xscom_pcba = pcc->xscom_pcba;
>> +    }
>> +
>> +    return xc->xscom_pcba(addr);
>> +}
>> +
>> +uint64_t pnv_xscom_addr(PnvXScomInterface *dev, uint32_t pcba)
>> +{
>> +     PnvXScomInterfaceClass *xc = PNV_XSCOM_INTERFACE_GET_CLASS(dev);
>> +
>> +    if (!xc->xscom_addr) {
>> +        PnvMachineState *pnv = POWERNV_MACHINE(qdev_get_machine());
>> +        PnvChipClass *pcc = PNV_CHIP_GET_CLASS(pnv->chips[0]);
>> +
>> +        xc->xscom_addr = pcc->xscom_addr;
>> +    }
>> +
>> +    return xc->xscom_addr(pcba);
>> +}
>> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
>> index 262faa59a75f..0371710b1882 100644
>> --- a/include/hw/ppc/pnv.h
>> +++ b/include/hw/ppc/pnv.h
>> @@ -21,6 +21,7 @@
>>  
>>  #include "hw/boards.h"
>>  #include "hw/sysbus.h"
>> +#include "hw/ppc/pnv_xscom.h"
>>  
>>  #define TYPE_PNV_CHIP "powernv-chip"
>>  #define PNV_CHIP(obj) OBJECT_CHECK(PnvChip, (obj), TYPE_PNV_CHIP)
>> @@ -42,6 +43,7 @@ typedef struct PnvChip {
>>  
>>      /*< public >*/
>>      uint32_t     chip_id;
>> +    PnvXScom     xscom;
>>  
>>      uint32_t  nr_cores;
>>      uint64_t  cores_mask;
>> @@ -60,6 +62,8 @@ typedef struct PnvChipClass {
>>  
>>      void (*realize)(PnvChip *dev, Error **errp);
>>      uint32_t (*core_pir)(PnvChip *chip, uint32_t core_id);
>> +    uint64_t (*xscom_addr)(uint32_t pcba);
>> +    uint32_t (*xscom_pcba)(uint64_t addr);
>>  } PnvChipClass;
>>  
>>  #define TYPE_PNV_CHIP_POWER8E TYPE_PNV_CHIP "-POWER8E"
>> diff --git a/include/hw/ppc/pnv_xscom.h b/include/hw/ppc/pnv_xscom.h
>> new file mode 100644
>> index 000000000000..0a03d533db59
>> --- /dev/null
>> +++ b/include/hw/ppc/pnv_xscom.h
>> @@ -0,0 +1,74 @@
>> +/*
>> + * QEMU PowerPC PowerNV XSCOM bus definitions
>> + *
>> + * Copyright (c) 2016, IBM Corporation.
>> + *
>> + * This library is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU Lesser General Public
>> + * License as published by the Free Software Foundation; either
>> + * version 2 of the License, or (at your option) any later version.
>> + *
>> + * This library is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * Lesser General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU Lesser General Public
>> + * License along with this library; if not, see 
>> <http://www.gnu.org/licenses/>.
>> + */
>> +#ifndef _PPC_PNV_XSCOM_H
>> +#define _PPC_PNV_XSCOM_H
>> +
>> +#include "hw/sysbus.h"
>> +
>> +typedef struct PnvXScomInterface {
>> +    Object parent;
>> +} PnvXScomInterface;
>> +
>> +#define TYPE_PNV_XSCOM_INTERFACE "pnv-xscom-interface"
>> +#define PNV_XSCOM_INTERFACE(obj) \
>> +     OBJECT_CHECK(PnvXScomInterface, (obj), TYPE_PNV_XSCOM_INTERFACE)
>> +#define PNV_XSCOM_INTERFACE_CLASS(klass)                \
>> +    OBJECT_CLASS_CHECK(PnvXScomInterfaceClass, (klass), \
>> +                       TYPE_PNV_XSCOM_INTERFACE)
>> +#define PNV_XSCOM_INTERFACE_GET_CLASS(obj) \
>> +     OBJECT_GET_CLASS(PnvXScomInterfaceClass, (obj), 
>> TYPE_PNV_XSCOM_INTERFACE)
>> +
>> +typedef struct PnvXScomInterfaceClass {
>> +    InterfaceClass parent;
>> +    int (*devnode)(PnvXScomInterface *dev, void *fdt, int offset);
>> +
>> +    uint64_t (*xscom_addr)(uint32_t pcba);
>> +    uint32_t (*xscom_pcba)(uint64_t addr);
> 
> Allowing the SCOM device to override the mapping just seems bogus.
> Surely this should always go via the chip.

yes.

>> +} PnvXScomInterfaceClass;
>> +
>> +#define TYPE_PNV_XSCOM "pnv-xscom"
>> +#define PNV_XSCOM(obj) OBJECT_CHECK(PnvXScom, (obj), TYPE_PNV_XSCOM)
>> +
>> +typedef struct PnvChipClass PnvChipClass;
>> +
>> +typedef struct PnvXScom {
>> +    /*< private >*/
>> +    SysBusDevice parent_obj;
>> +    /*< public >*/
>> +
>> +    MemoryRegion mem;
>> +    int32_t chip_id;
>> +    PnvChipClass *chip_class;
> 
> Just having a pointer to the chip seems simpler than storing both id
> and class here.  But I guess it'll go away anyway if you merge the ADU
> into the chip object itself.

I will merge in v4.

Thanks,

C. 

>> +    MemoryRegion xscom_mr;
>> +    AddressSpace xscom_as;
>> +} PnvXScom;
>> +
>> +#define PNV_XSCOM_SIZE        0x800000000ull
>> +#define PNV_XSCOM_BASE(chip)                                    \
>> +    (0x3fc0000000000ull + ((uint64_t)(chip)) * PNV_XSCOM_SIZE)
>> +
>> +extern int pnv_xscom_populate_fdt(PnvXScom *xscom, void *fdt, int offset);
>> +
>> +/*
>> + * helpers to translate to XScomm PCB addresses
>> + */
>> +extern uint32_t pnv_xscom_pcba(PnvXScomInterface *dev, uint64_t addr);
>> +extern uint64_t pnv_xscom_addr(PnvXScomInterface *dev, uint32_t pcba);
>> +
>> +#endif /* _PPC_PNV_XSCOM_H */
>