Re: [Qemu-ppc] [PATCH] pnv: add a physical mapping array describing MMIO ranges in each chip

[Cédric Le Goater]

On 05/23/2018 04:04 PM, Greg Kurz wrote:
> On Wed, 23 May 2018 14:37:30 +0200
> Cédric Le Goater <address@hidden> wrote:
> 
>> On 05/18/2018 11:00 AM, Greg Kurz wrote:
>>> On Thu, 17 May 2018 15:18:14 +0200
>>> Cédric Le Goater <address@hidden> wrote:
>>>   
>>>> Based on previous work done in skiboot, the physical mapping array
>>>> helps in calculating the MMIO ranges of each controller depending on
>>>> the chip id and the chip type. This is will be particularly useful for
>>>> the P9 models which use less the XSCOM bus and rely more on MMIOs.
>>>>
>>>> A link on the chip is now necessary to calculate MMIO BARs and
>>>> sizes. This is why such a link is introduced in the PSIHB model.
>>>>
>>>> Signed-off-by: Cédric Le Goater <address@hidden>
>>>> ---
>>>>  hw/ppc/pnv.c         | 32 +++++++++++++++++++++--------
>>>>  hw/ppc/pnv_psi.c     | 11 +++++++++-
>>>>  hw/ppc/pnv_xscom.c   |  8 ++++----
>>>>  include/hw/ppc/pnv.h | 58 
>>>> +++++++++++++++++++++++++++++++++++++---------------
>>>>  4 files changed, 80 insertions(+), 29 deletions(-)
>>>>
>>>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>>>> index 031488131629..330bf6f74810 100644
>>>> --- a/hw/ppc/pnv.c
>>>> +++ b/hw/ppc/pnv.c
>>>> @@ -712,6 +712,16 @@ static uint32_t pnv_chip_core_pir_p9(PnvChip *chip, 
>>>> uint32_t core_id)
>>>>   */
>>>>  #define POWER9_CORE_MASK   (0xffffffffffffffull)
>>>>  
>>>> +/*
>>>> + * POWER8 MMIOs
>>>> + */
>>>> +static const PnvPhysMapEntry pnv_chip_power8_phys_map[] = {
>>>> +    [PNV_MAP_XSCOM]     = { 0x0003fc0000000000ull, 0x0000000800000000ull 
>>>> },
>>>> +    [PNV_MAP_ICP]       = { 0x0003ffff80000000ull, 0x0000000000100000ull 
>>>> },
>>>> +    [PNV_MAP_PSIHB]     = { 0x0003fffe80000000ull, 0x0000000000100000ull 
>>>> },
>>>> +    [PNV_MAP_PSIHB_FSP] = { 0x0003ffe000000000ull, 0x0000000100000000ull 
>>>> },
>>>> +};
>>>> +
>>>>  static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data)
>>>>  {
>>>>      DeviceClass *dc = DEVICE_CLASS(klass);
>>>> @@ -721,7 +731,7 @@ static void pnv_chip_power8e_class_init(ObjectClass 
>>>> *klass, void *data)
>>>>      k->chip_cfam_id = 0x221ef04980000000ull;  /* P8 Murano DD2.1 */
>>>>      k->cores_mask = POWER8E_CORE_MASK;
>>>>      k->core_pir = pnv_chip_core_pir_p8;
>>>> -    k->xscom_base = 0x003fc0000000000ull;
>>>> +    k->phys_map = pnv_chip_power8_phys_map;
>>>>      dc->desc = "PowerNV Chip POWER8E";
>>>>  }
>>>>  
>>>> @@ -734,7 +744,7 @@ static void pnv_chip_power8_class_init(ObjectClass 
>>>> *klass, void *data)
>>>>      k->chip_cfam_id = 0x220ea04980000000ull; /* P8 Venice DD2.0 */
>>>>      k->cores_mask = POWER8_CORE_MASK;
>>>>      k->core_pir = pnv_chip_core_pir_p8;
>>>> -    k->xscom_base = 0x003fc0000000000ull;
>>>> +    k->phys_map = pnv_chip_power8_phys_map;
>>>>      dc->desc = "PowerNV Chip POWER8";
>>>>  }
>>>>  
>>>> @@ -747,10 +757,17 @@ static void 
>>>> pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data)
>>>>      k->chip_cfam_id = 0x120d304980000000ull;  /* P8 Naples DD1.0 */
>>>>      k->cores_mask = POWER8_CORE_MASK;
>>>>      k->core_pir = pnv_chip_core_pir_p8;
>>>> -    k->xscom_base = 0x003fc0000000000ull;
>>>> +    k->phys_map = pnv_chip_power8_phys_map;
>>>>      dc->desc = "PowerNV Chip POWER8NVL";
>>>>  }
>>>>  
>>>> +/*
>>>> + * POWER9 MMIOs
>>>> + */
>>>> +static const PnvPhysMapEntry pnv_chip_power9_phys_map[] = {
>>>> +    [PNV_MAP_XSCOM]     = { 0x000603fc00000000ull, 0x0000040000000000ull 
>>>> },
>>>> +};
>>>> +
>>>>  static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
>>>>  {
>>>>      DeviceClass *dc = DEVICE_CLASS(klass);
>>>> @@ -760,7 +777,7 @@ static void pnv_chip_power9_class_init(ObjectClass 
>>>> *klass, void *data)
>>>>      k->chip_cfam_id = 0x220d104900008000ull; /* P9 Nimbus DD2.0 */
>>>>      k->cores_mask = POWER9_CORE_MASK;
>>>>      k->core_pir = pnv_chip_core_pir_p9;
>>>> -    k->xscom_base = 0x00603fc00000000ull;
>>>> +    k->phys_map = pnv_chip_power9_phys_map;
>>>>      dc->desc = "PowerNV Chip POWER9";
>>>>  }
>>>>  
>>>> @@ -797,15 +814,14 @@ static void pnv_chip_core_sanitize(PnvChip *chip, 
>>>> Error **errp)
>>>>  static void pnv_chip_init(Object *obj)
>>>>  {
>>>>      PnvChip *chip = PNV_CHIP(obj);
>>>> -    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
>>>> -
>>>> -    chip->xscom_base = pcc->xscom_base;
>>>>  
>>>>      object_initialize(&chip->lpc, sizeof(chip->lpc), TYPE_PNV_LPC);
>>>>      object_property_add_child(obj, "lpc", OBJECT(&chip->lpc), NULL);
>>>>  
>>>>      object_initialize(&chip->psi, sizeof(chip->psi), TYPE_PNV_PSI);
>>>>      object_property_add_child(obj, "psi", OBJECT(&chip->psi), NULL);
>>>> +    object_property_add_const_link(OBJECT(&chip->psi), "chip", obj,
>>>> +                                   &error_abort);
>>>>      object_property_add_const_link(OBJECT(&chip->psi), "xics",
>>>>                                     OBJECT(qdev_get_machine()), 
>>>> &error_abort);
>>>>  
>>>> @@ -829,7 +845,7 @@ static void pnv_chip_icp_realize(PnvChip *chip, Error 
>>>> **errp)
>>>>      XICSFabric *xi = XICS_FABRIC(qdev_get_machine());
>>>>  
>>>>      name = g_strdup_printf("icp-%x", chip->chip_id);
>>>> -    memory_region_init(&chip->icp_mmio, OBJECT(chip), name, PNV_ICP_SIZE);
>>>> +    memory_region_init(&chip->icp_mmio, OBJECT(chip), name, 
>>>> PNV_ICP_SIZE(chip));
>>>>      sysbus_init_mmio(SYS_BUS_DEVICE(chip), &chip->icp_mmio);
>>>>      g_free(name);
>>>>  
>>>> diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
>>>> index 5b969127c303..dd7707b971c9 100644
>>>> --- a/hw/ppc/pnv_psi.c
>>>> +++ b/hw/ppc/pnv_psi.c
>>>> @@ -465,6 +465,15 @@ static void pnv_psi_realize(DeviceState *dev, Error 
>>>> **errp)
>>>>      Object *obj;
>>>>      Error *err = NULL;
>>>>      unsigned int i;
>>>> +    PnvChip *chip;
>>>> +
>>>> +    obj = object_property_get_link(OBJECT(dev), "chip", &err);
>>>> +    if (!obj) {
>>>> +        error_setg(errp, "%s: required link 'chip' not found: %s",
>>>> +                   __func__, error_get_pretty(err));
>>>> +        return;  
>>>
>>> err was dynamically allocated and must be freed with error_free().  
>>
>> ok. There are quite a few places not doing it right then. 
>>
> 
> Yeah, error_get_pretty users are the usual suspects here.
> 
>>> This being said, the link is supposed to have been set in pnv_chip_init()
>>> already, so if we can't get it here, it's likely a bug in QEMU. I'd rather
>>> pass &error_abort.  
>>
>> The followed pattern in pnv is most of the time :
>>
>>     object_property_add_const_link(OBJECT(&chip->bar), "foo", obj,
>>                                    &error_abort);
>>
>> and in the realize function : 
>>
>>     Error *local_err = NULL;
>>    
>>     obj = object_property_get_link(OBJECT(dev), "foo", &local_err);
>>     if (!obj) {
>>         error_setg(errp, "%s: required link 'foo' not found: %s",
>>                    __func__, error_get_pretty(local_err));
>>         return;
>>     }
>>
>> but you propose that we should rather simply do :
>>   
>>     obj = object_property_get_link(OBJECT(dev), "foo", &error_abort);
>>
>> Correct ? 
>>  
> 
> Yes, but this is questionable :)
> 
> The realize function has an Error ** arg, ie, it is expected to propagate
> errors and let the caller decide... so it is a matter of taste here.
> 
> Another option to using error_get_pretty() is to use error_propagate() and
> error_prepend(), as suggested in include/qapi/error.h .
> 
> See commit a1a6bbde4f6a29368f8f605cea2e73630ec1bc7c for an example.
> 
>>>> +    }
>>>> +    chip = PNV_CHIP(obj);
>>>>  
>>>>      obj = object_property_get_link(OBJECT(dev), "xics", &err);
>>>>      if (!obj) {
>>>> @@ -497,7 +506,7 @@ static void pnv_psi_realize(DeviceState *dev, Error 
>>>> **errp)
>>>>  
>>>>      /* Initialize MMIO region */
>>>>      memory_region_init_io(&psi->regs_mr, OBJECT(dev), &psi_mmio_ops, psi,
>>>> -                          "psihb", PNV_PSIHB_SIZE);
>>>> +                          "psihb", PNV_PSIHB_SIZE(chip));
>>>>  
>>>>      /* Default BAR for MMIO region */
>>>>      pnv_psi_set_bar(psi, psi->bar | PSIHB_BAR_EN);
>>>> diff --git a/hw/ppc/pnv_xscom.c b/hw/ppc/pnv_xscom.c
>>>> index 46fae41f32b0..20ffc233174c 100644
>>>> --- a/hw/ppc/pnv_xscom.c
>>>> +++ b/hw/ppc/pnv_xscom.c
>>>> @@ -50,7 +50,7 @@ static void xscom_complete(CPUState *cs, uint64_t 
>>>> hmer_bits)
>>>>  
>>>>  static uint32_t pnv_xscom_pcba(PnvChip *chip, uint64_t addr)
>>>>  {
>>>> -    addr &= (PNV_XSCOM_SIZE - 1);
>>>> +    addr &= (PNV_XSCOM_SIZE(chip) - 1);
>>>>  
>>>>      if (pnv_chip_is_power9(chip)) {
>>>>          return addr >> 3;
>>>> @@ -179,10 +179,10 @@ void pnv_xscom_realize(PnvChip *chip, Error **errp)
>>>>  
>>>>      name = g_strdup_printf("xscom-%x", chip->chip_id);
>>>>      memory_region_init_io(&chip->xscom_mmio, OBJECT(chip), &pnv_xscom_ops,
>>>> -                          chip, name, PNV_XSCOM_SIZE);
>>>> +                          chip, name, PNV_XSCOM_SIZE(chip));
>>>>      sysbus_init_mmio(sbd, &chip->xscom_mmio);
>>>>  
>>>> -    memory_region_init(&chip->xscom, OBJECT(chip), name, PNV_XSCOM_SIZE);
>>>> +    memory_region_init(&chip->xscom, OBJECT(chip), name, 
>>>> PNV_XSCOM_SIZE(chip));
>>>>      address_space_init(&chip->xscom_as, &chip->xscom, name);
>>>>      g_free(name);
>>>>  }
>>>> @@ -225,7 +225,7 @@ static const char compat_p9[] = 
>>>> "ibm,power9-xscom\0ibm,xscom";
>>>>  int pnv_dt_xscom(PnvChip *chip, void *fdt, int root_offset)
>>>>  {
>>>>      uint64_t reg[] = { cpu_to_be64(PNV_XSCOM_BASE(chip)),
>>>> -                       cpu_to_be64(PNV_XSCOM_SIZE) };
>>>> +                       cpu_to_be64(PNV_XSCOM_SIZE(chip)) };
>>>>      int xscom_offset;
>>>>      ForeachPopulateArgs args;
>>>>      char *name;
>>>> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
>>>> index 90759240a7b1..b810e7b84ec0 100644
>>>> --- a/include/hw/ppc/pnv.h
>>>> +++ b/include/hw/ppc/pnv.h
>>>> @@ -53,7 +53,6 @@ typedef struct PnvChip {
>>>>      uint64_t     cores_mask;
>>>>      void         *cores;
>>>>  
>>>> -    hwaddr       xscom_base;
>>>>      MemoryRegion xscom_mmio;
>>>>      MemoryRegion xscom;
>>>>      AddressSpace xscom_as;
>>>> @@ -64,6 +63,19 @@ typedef struct PnvChip {
>>>>      PnvOCC       occ;
>>>>  } PnvChip;
>>>>  
>>>> +typedef enum PnvPhysMapType {
>>>> +    PNV_MAP_XSCOM,
>>>> +    PNV_MAP_ICP,
>>>> +    PNV_MAP_PSIHB,
>>>> +    PNV_MAP_PSIHB_FSP,
>>>> +    PNV_MAP_MAX,
>>>> +} PnvPhysMapType;
>>>> +
>>>> +typedef struct PnvPhysMapEntry {
>>>> +    uint64_t        base;
>>>> +    uint64_t        size;
>>>> +} PnvPhysMapEntry;
>>>> +
>>>>  typedef struct PnvChipClass {
>>>>      /*< private >*/
>>>>      SysBusDeviceClass parent_class;
>>>> @@ -73,7 +85,7 @@ typedef struct PnvChipClass {
>>>>      uint64_t     chip_cfam_id;
>>>>      uint64_t     cores_mask;
>>>>  
>>>> -    hwaddr       xscom_base;
>>>> +    const PnvPhysMapEntry *phys_map;
>>>>  
>>>>      uint32_t (*core_pir)(PnvChip *chip, uint32_t core_id);
>>>>  } PnvChipClass;
>>>> @@ -159,9 +171,28 @@ void pnv_bmc_powerdown(IPMIBmc *bmc);
>>>>  /*
>>>>   * POWER8 MMIO base addresses
>>>>   */
>>>> -#define PNV_XSCOM_SIZE        0x800000000ull
>>>> -#define PNV_XSCOM_BASE(chip)                                            \
>>>> -    (chip->xscom_base + ((uint64_t)(chip)->chip_id) * PNV_XSCOM_SIZE)
>>>> +static inline uint64_t pnv_map_size(const PnvChip *chip, PnvPhysMapType 
>>>> type)
>>>> +{
>>>> +    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
>>>> +    const PnvPhysMapEntry *map = &pcc->phys_map[type];
>>>> +
>>>> +    return map->size;
>>>> +}
>>>> +
>>>> +static inline uint64_t pnv_map_base(const PnvChip *chip, PnvPhysMapType 
>>>> type)
>>>> +{
>>>> +    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
>>>> +    const PnvPhysMapEntry *map = &pcc->phys_map[type];
>>>> +
>>>> +    if (pnv_chip_is_power9(chip)) {
>>>> +        return map->base + ((uint64_t) chip->chip_id << 42);  
>>>
>>> So this patch also changes the way the base address is computed
>>> with POWER9. Shouldn't this go to a separate patch or at least
>>> mention the functional change in the changelog ?  
>>
>> P9 is not really supported, multi P9 chips even less. But yes, I can
>> mention that the P9 MMIO base address is being fixed. 
>>  
>>> Also, shouldn't this be a PnvChipClass level function rather than
>>> doing a runtime check ?  
>>
>> Hmm. pnv_map_base() needs the 'chip_id' to calculate the base
>> address which is not a class level attribute. The P9 check only 
>> needs the chip class but it is more convenient to use a PnvChip. 
>> Only class_init routines use class variables. 
> 
> Not sure to understand... My suggestion was just to add a new function
> attribute to PnvChipClass:
> 
>     uint64_t (*map_base)(const PnvChip *chip, PnvPhysMapType type);
> 
> And have two separate implementations for P8 and P9:
> 
> static uint64_t pnv_chip_map_base_p8(const PnvChip *chip,
>                                      const PnvPhysMapEntry *map)
> {
>     return map->base + chip->chip_id * map->size;
> }
> 
> static uint64_t pnv_chip_map_base_p9(const PnvChip *chip,
>                                      const PnvPhysMapEntry *map)
> {
>     return map->base + ((uint64_t) chip->chip_id << 42);
> }
> 
> And pnv_map_base() would be:
> 
> static inline uint64_t pnv_map_base(const PnvChip *chip, PnvPhysMapType type)
> {
>     PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
> 
>     return pcc->map_base(chip, &pcc->phys_map[type]);
> }

ok. yes, that's a nice proposition. 

Thanks,

C.


> 
>>> The rest of the patch looks good.  
>>
>> Thanks,
>>
>> C. 
>>  
>>>> +    } else  {
>>>> +        return map->base + chip->chip_id * map->size;
>>>> +    }
>>>> +}
>>>> +
>>>> +#define PNV_XSCOM_SIZE(chip)         pnv_map_size(chip, PNV_MAP_XSCOM)
>>>> +#define PNV_XSCOM_BASE(chip)         pnv_map_base(chip, PNV_MAP_XSCOM)
>>>>  
>>>>  /*
>>>>   * XSCOM 0x20109CA defines the ICP BAR:
>>>> @@ -177,18 +208,13 @@ void pnv_bmc_powerdown(IPMIBmc *bmc);
>>>>   *      0xffffe02200000000 -> 0x0003ffff80800000
>>>>   *      0xffffe02600000000 -> 0x0003ffff80900000
>>>>   */
>>>> -#define PNV_ICP_SIZE         0x0000000000100000ull
>>>> -#define PNV_ICP_BASE(chip)                                              \
>>>> -    (0x0003ffff80000000ull + (uint64_t) PNV_CHIP_INDEX(chip) * 
>>>> PNV_ICP_SIZE)
>>>> -
>>>> +#define PNV_ICP_SIZE(chip)           pnv_map_size(chip, PNV_MAP_ICP)
>>>> +#define PNV_ICP_BASE(chip)           pnv_map_base(chip, PNV_MAP_ICP)
>>>>  
>>>> -#define PNV_PSIHB_SIZE       0x0000000000100000ull
>>>> -#define PNV_PSIHB_BASE(chip) \
>>>> -    (0x0003fffe80000000ull + (uint64_t)PNV_CHIP_INDEX(chip) * 
>>>> PNV_PSIHB_SIZE)
>>>> +#define PNV_PSIHB_SIZE(chip)         pnv_map_size(chip, PNV_MAP_PSIHB)
>>>> +#define PNV_PSIHB_BASE(chip)         pnv_map_base(chip, PNV_MAP_PSIHB)
>>>>  
>>>> -#define PNV_PSIHB_FSP_SIZE   0x0000000100000000ull
>>>> -#define PNV_PSIHB_FSP_BASE(chip) \
>>>> -    (0x0003ffe000000000ull + (uint64_t)PNV_CHIP_INDEX(chip) * \
>>>> -     PNV_PSIHB_FSP_SIZE)
>>>> +#define PNV_PSIHB_FSP_SIZE(chip)     pnv_map_size(chip, PNV_MAP_PSIHB_FSP)
>>>> +#define PNV_PSIHB_FSP_BASE(chip)     pnv_map_base(chip, PNV_MAP_PSIHB_FSP)
>>>>  
>>>>  #endif /* _PPC_PNV_H */  
>>>   
>>
>