[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC PATCH 3/6] hw/ppc/pnv_homer: add homer/occ common
From: |
Cédric Le Goater |
Subject: |
Re: [Qemu-devel] [RFC PATCH 3/6] hw/ppc/pnv_homer: add homer/occ common area emulation for PowerNV |
Date: |
Thu, 8 Aug 2019 10:32:50 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 |
On 07/08/2019 12:07, Balamuruhan S wrote:
> On Wed, Aug 07, 2019 at 09:54:55AM +0200, Cédric Le Goater wrote:
>> On 07/08/2019 09:14, Balamuruhan S wrote:
>>> Add mmio callback functions to enable homer/occ common area
>>> to emulate pstate table, occ-sensors, slw, occ static and
>>> dynamic values for Power8 and Power9 chips. It also works for
>>> multiple chips as offset remains the same whereas the base
>>> address are handled appropriately while initializing device
>>> tree.
>>>
>>> currently skiboot disables the homer/occ code path with
>>> `QUIRK_NO_PBA`, this quirk have to be removed in skiboot
>>> for it to use this infrastructure.
>>
>>
>> I think this patch can come before the others as it is adding
>> support without the python extra facilities.
>
> Okay sure.
>
>>
>> Some comments below,
>>
>>> Signed-off-by: Hariharan T.S <address@hidden>
>>> Signed-off-by: Balamuruhan S <address@hidden>
>>> ---
>>> hw/ppc/Makefile.objs | 2 +-
>>> hw/ppc/pnv_homer.c | 185
>>> +++++++++++++++++++++++++++++++++++++++++++++
>>> include/hw/ppc/pnv.h | 14 ++++
>>> include/hw/ppc/pnv_homer.h | 41 ++++++++++
>>> 4 files changed, 241 insertions(+), 1 deletion(-)
>>> create mode 100644 hw/ppc/pnv_homer.c
>>> create mode 100644 include/hw/ppc/pnv_homer.h
>>>
>>> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
>>> index 9da93af905..7260b4a96c 100644
>>> --- a/hw/ppc/Makefile.objs
>>> +++ b/hw/ppc/Makefile.objs
>>> @@ -7,7 +7,7 @@ obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o
>>> obj-$(CONFIG_PSERIES) += spapr_cpu_core.o spapr_ovec.o spapr_irq.o
>>> obj-$(CONFIG_SPAPR_RNG) += spapr_rng.o
>>> # IBM PowerNV
>>> -obj-$(CONFIG_POWERNV) += pnv.o pnv_xscom.o pnv_core.o pnv_lpc.o pnv_psi.o
>>> pnv_occ.o pnv_bmc.o
>>> +obj-$(CONFIG_POWERNV) += pnv.o pnv_xscom.o pnv_core.o pnv_lpc.o pnv_psi.o
>>> pnv_occ.o pnv_bmc.o pnv_homer.o
>>
>> add an extra line.
>
> I will do that.
>
>>
>>> ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
>>> obj-y += spapr_pci_vfio.o spapr_pci_nvlink2.o
>>> endif
>>> diff --git a/hw/ppc/pnv_homer.c b/hw/ppc/pnv_homer.c
>>> new file mode 100644
>>> index 0000000000..73a94856d0
>>> --- /dev/null
>>> +++ b/hw/ppc/pnv_homer.c
>>> @@ -0,0 +1,185 @@
>>> +/*
>>> + * QEMU PowerPC PowerNV Homer and OCC common area region
>>> + *
>>> + * Copyright (c) 2019, 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 "sysemu/hw_accel.h"
>>> +#include "sysemu/cpus.h"
>>> +#include "hw/ppc/pnv.h"
>>> +
>>> +static bool core_max_array(hwaddr addr)
>>> +{
>>> + char *cpu_type;
>>> + hwaddr core_max_base = 0xe2819;
>>
>> What is this representing ?
>>
>>> + MachineState *ms = MACHINE(qdev_get_machine());
>>> + cpu_type = strstr(ms->cpu_type, "power8");
>>
>> you need to get this information some other way. The PnvChip should have it.
>
> okay I will use it, I think you mean PnvChipType chip_type.
you will need to fetch this information from some model instance or
class, PnvChip or a new one.
>>
>>> + if (cpu_type)
>>> + core_max_base = 0x1f8810;
>>
>> It could be a PnvChipClass value.
>
> or should we also consider it also as macro ?
it's a chip constant ? see the pnv_chip_*_class_init routine.
>>
>>> + for (int i = 0; i <= ms->smp.cores; i++)
>>> + if (addr == (core_max_base + i))
>>> + return true;
>>> + return false;
>>> +}
>>
>>
>>> +static uint64_t homer_read(void *opaque, hwaddr addr, unsigned width)
>>> +{
>>> + switch (addr) {
>>
>> We should be using defines for the case statements below.
>>
>> Are we accessing one or more structures which are mapped at specific
>> addresses ? If so I would define them in this file and change the
>> memory ops to use well known offsets.
>
> okay, but lot of defines have to be done, will that be fine ?
Not a problem. You can copy the definitions and the structures from
skiboot and adapt them to QEMU coding style. Look at the other Pnv
models PSI, XIVE, LPC.
>
>>
>> Are these structures the same on P9 and P8 ?
>
> no, stuctures are different on P9 and P8, occ pstate table version 0x90 is for
> P9 and 0x01, 0x02 is for P8.
These are class constants. most probably a new PnvXScom/PnvHomer or
PnvOCC
> I have mentioned P8 specific offsets in comments and rest are P9.
>
>>
>> Are there default values ? May be we could use a reset handler
>> in this case.
>
> yes, I tried to return default expected values for skiboot. I am
> not aware of reset handler, where can I check it ?
You need to introduce a new model and add a reset handler for it, or
use an existing one. I am not sure we need a PnvHomer model but we might
need to add PnvXscom and add the HOMER stuff into it.
>>> + case 0xe2006: /* max pstate ultra turbo */
>>> + case 0xe2018: /* pstate id for 0 */
>>> + case 0x1f8001: /* P8 occ pstate version */
>>> + case 0x1f8003: /* P8 pstate min */
>>> + case 0x1f8010: /* P8 pstate id for 0 */
>>> + return 0;
>>> + case 0xe2000: /* occ data area */
>>> + case 0xe2002: /* occ_role master/slave*/
>>> + case 0xe2004: /* pstate nom */
>>> + case 0xe2005: /* pstate turbo */
>>> + case 0xe2020: /* pstate id for 1 */
>>> + case 0xe2818: /* pstate ultra turbo */
>>> + case 0xe2b85: /* opal dynamic data (runtime) */
>>> + case 0x1f8000: /* P8 occ pstate valid */
>>> + case 0x1f8002: /* P8 throttle */
>>> + case 0x1f8004: /* P8 pstate nom */
>>> + case 0x1f8005: /* P8 pstate turbo */
>>> + case 0x1f8012: /* vdd voltage identifier */
>>> + case 0x1f8013: /* vcs voltage identifier */
>>> + case 0x1f8018: /* P8 pstate id for 1 */
>>> + return 1;
>>> + case 0xe2003: /* pstate min (2 as pstate min) */
>>> + case 0xe2028: /* pstate id for 2 */
>>> + case 0x1f8006: /* P8 pstate ultra turbo */
>>> + case 0x1f8020: /* P8 pstate id for 2 */
>>> + return 2;
>>> + case 0xe2001: /* major version */
>>> + return 0x90;
>>> + /* 3000 khz frequency for 0, 1, and 2 pstates */
>>> + case 0xe201c:
>>> + case 0xe2024:
>>> + case 0xe202c:
>>> + /* P8 frequency for 0, 1, and 2 pstates */
>>> + case 0x1f8014:
>>> + case 0x1f801c:
>>> + case 0x1f8024:
>>> + return 3000;
>>> + case 0x0: /* homer base */
>>> + case 0xe2008: /* occ data area + 8 */
>>> + case 0x1f8008: /* P8 occ data area + 8 */
>>> + case 0x200008: /* homer base access to get homer image pointer*/
>>> + return 0x1000000000000000;
>>> + }
>>> + /* pstate table core max array */
>>> + if (core_max_array(addr))
>>> + return 1;
>>
>> I don't understand what the core_max_array is returning
>
> core_max_array defaults to nominal pstate 1 as maximum sustainable
> pstate. please advise if you think it is not right.
I still do not understand what this is about :/
>
>>
>>> + return 0;
>>> +}
>>> +
>>> +static void homer_write(void *opaque, hwaddr addr, uint64_t val,
>>> + unsigned width)
>>> +{
>>> + /* callback function defined to homer write */
>>> + return;
>>> +}
>>> +
>>> +const MemoryRegionOps pnv_homer_ops = {
>>> + .read = homer_read,
>>> + .write = homer_write,
>>> + .valid.min_access_size = 1,
>>> + .valid.max_access_size = 8,
>>> + .impl.min_access_size = 1,
>>> + .impl.max_access_size = 8,
>>> + .endianness = DEVICE_BIG_ENDIAN,
>>> +};
>>> +
>>> +static uint64_t occ_common_area_read(void *opaque, hwaddr addr, unsigned
>>> width)
>>> +{
>>> + switch (addr) {
>>> + /*
>>> + * occ-sensor sanity check that asserts the sensor
>>> + * header block
>>> + */
>>
>> Same comments as above.
>
> okay, will do.
>>
>>> + case 0x580000: /* occ sensor data block */
>>> + case 0x580001: /* valid */
>>> + case 0x580002: /* version */
>>> + case 0x580004: /* reading_version */
>>> + case 0x580008: /* nr_sensors */
>>> + case 0x580010: /* names_offset */
>>> + case 0x580014: /* reading_ping_offset */
>>> + case 0x58000c: /* reading_pong_offset */
>>> + case 0x580023: /* structure_type */
>>> + return 1;
>>> + case 0x58000d: /* name length */
>>> + return 0x30;
>>> + case 0x580022: /* occ sensor loc core */
>>> + return 0x0040;
>>> + case 0x580003: /* occ sensor type power */
>>> + return 0x0080;
>>> + case 0x580005: /* sensor name */
>>> + return 0x1000;
>>> + case 0x58001e: /* HWMON_SENSORS_MASK */
>>> + case 0x580020:
>>> + return 0x8e00;
>>> + case 0x0: /* P8 slw base access for slw image size */
>>> + return 0x1000000000000000;
>>> + }
>>> + return 0;
>>> +}
>>> +
>>> +static void occ_common_area_write(void *opaque, hwaddr addr, uint64_t val,
>>> + unsigned width)
>>> +{
>>> + /* callback function defined to occ common area write */
>>> + return;
>>> +}
>>> +
>>> +const MemoryRegionOps pnv_occ_common_area_ops = {
>>> + .read = occ_common_area_read,
>>> + .write = occ_common_area_write,
>>> + .valid.min_access_size = 1,
>>> + .valid.max_access_size = 8,
>>> + .impl.min_access_size = 1,
>>> + .impl.max_access_size = 8,
>>> + .endianness = DEVICE_BIG_ENDIAN,
>>> +};
>>
>>
>> Why aren't you using the PnvOCC model ?
>
> I was not aware of how to use it here, I will check it to make use
> of it.
Yes. Check how the machine and the chips are defined in pnv.c and then
the other logic units, LPC, XIVE, OCC, etc. The XSCOM bus does not have
a model of its own because it is limited to an address space. We might
need to introduce a PnvXscom model and put the HOMER info there.
>
>>
>>> +void pnv_occ_common_area_realize(PnvChip *chip, Error **errp)
>>> +{
>>> + SysBusDevice *sbd = SYS_BUS_DEVICE(chip);
>>> + sbd->num_mmio = PNV_OCC_COMMON_AREA_SYSBUS;
>>> + char *occ_common_area;
>>> +
>>> + /* occ common area */
>>> + occ_common_area = g_strdup_printf("occ-common-area-%x", chip->chip_id);
>>> + memory_region_init_io(&chip->occ_common_area_mmio, OBJECT(chip),
>>> + &pnv_occ_common_area_ops, chip, occ_common_area,
>>> + PNV_OCC_COMMON_AREA_SIZE);
>>> + sysbus_init_mmio(sbd, &chip->occ_common_area_mmio);
>>> + g_free(occ_common_area);
>>> +}
>>
>>
>> May be this "device" deserves a PnvHomer model, one for P8 and one for P9.
>
> :+1:
For OCC stuff, please try to extend the OCC model with a new region mapping
the structures of the SRAM that the chip uses for sensors, pstate, etc.
>>
>>> +void pnv_homer_realize(PnvChip *chip, Error **errp)
>>> +{
>>> + SysBusDevice *sbd = SYS_BUS_DEVICE(chip);
>>> + sbd->num_mmio = PNV_HOMER_SYSBUS;
>>> + char *homer;
>>> +
>>> + /* homer region */
>>> + homer = g_strdup_printf("homer-%x", chip->chip_id);
>>> + memory_region_init_io(&chip->homer_mmio, OBJECT(chip), &pnv_homer_ops,
>>> + chip, homer, PNV_HOMER_SIZE);
>>> + sysbus_init_mmio(sbd, &chip->homer_mmio);
>>> + g_free(homer);
>>> +}
>>> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
>>> index fb123edc4e..6464e32892 100644
>>> --- a/include/hw/ppc/pnv.h
>>> +++ b/include/hw/ppc/pnv.h
>>> @@ -28,6 +28,7 @@
>>> #include "hw/ppc/pnv_occ.h"
>>> #include "hw/ppc/pnv_xive.h"
>>> #include "hw/ppc/pnv_core.h"
>>> +#include "hw/ppc/pnv_homer.h"
>>>
>>> #define TYPE_PNV_CHIP "pnv-chip"
>>> #define PNV_CHIP(obj) OBJECT_CHECK(PnvChip, (obj), TYPE_PNV_CHIP)
>>> @@ -36,6 +37,13 @@
>>> #define PNV_CHIP_GET_CLASS(obj) \
>>> OBJECT_GET_CLASS(PnvChipClass, (obj), TYPE_PNV_CHIP)
>>>
>>> +enum SysBusNum {
>>> + PNV_XSCOM_SYSBUS,
>>> + PNV_ICP_SYSBUS,
>>> + PNV_HOMER_SYSBUS,
>>> + PNV_OCC_COMMON_AREA_SYSBUS,
>>> +};
>>
>> What is this ?
>
> enumeration for SysBusDevice device init, it is needed for mapping to
> work, please correct me if it is not the right way.
I think you can add the HOMER memory region in a new PnvXscom model.
the SRAM memory region can go in the OCC model.
We need to check how all these models interacts.
Thanks,
C.
>
>>
>>
>>> typedef enum PnvChipType {
>>> PNV_CHIP_POWER8E, /* AKA Murano (default) */
>>> PNV_CHIP_POWER8, /* AKA Venice */
>>> @@ -56,6 +64,8 @@ typedef struct PnvChip {
>>> uint64_t cores_mask;
>>> void *cores;
>>>
>>> + MemoryRegion homer_mmio;
>>> + MemoryRegion occ_common_area_mmio;
>>> MemoryRegion xscom_mmio;
>>> MemoryRegion xscom;
>>> AddressSpace xscom_as;
>>> @@ -191,6 +201,10 @@ static inline bool pnv_is_power9(PnvMachineState *pnv)
>>> void pnv_dt_bmc_sensors(IPMIBmc *bmc, void *fdt);
>>> void pnv_bmc_powerdown(IPMIBmc *bmc);
>>>
>>> +extern void pnv_occ_common_area_realize(PnvChip *chip, Error **errp);
>>> +extern void pnv_homer_realize(PnvChip *chip, Error **errp);
>>> +
>>> +
>>> /*
>>> * POWER8 MMIO base addresses
>>> */
>>> diff --git a/include/hw/ppc/pnv_homer.h b/include/hw/ppc/pnv_homer.h
>>> new file mode 100644
>>> index 0000000000..0fe6469abe
>>> --- /dev/null
>>> +++ b/include/hw/ppc/pnv_homer.h
>>> @@ -0,0 +1,41 @@
>>> +/*
>>> + * QEMU PowerPC PowerNV Homer and occ common area definitions
>>> + *
>>> + * Copyright (c) 2019, 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_HOMER_H
>>> +#define _PPC_PNV_HOMER_H
>>> +
>>> +#include "qom/object.h"
>>> +
>>> +/*
>>> + * HOMER region size 4M per OCC (1 OCC is defined per chip in struct
>>> PnvChip)
>>> + * so chip_num can be used to offset between HOMER region from its base
>>> address
>>> + */
>>> +#define PNV_HOMER_SIZE 0x300000
>>> +#define PNV_OCC_COMMON_AREA_SIZE 0x700000
>>> +
>>> +#define PNV_HOMER_BASE(chip) \
>>> + (0x7ffd800000ull + ((uint64_t)(chip)->chip_num) * PNV_HOMER_SIZE)
>>> +#define PNV_OCC_COMMON_AREA(chip) \
>>> + (0x7fff800000ull + ((uint64_t)(chip)->chip_num) *
>>> PNV_OCC_COMMON_AREA_SIZE)
>>> +
>>> +#define PNV9_HOMER_BASE(chip) \
>>> + (0x203ffd800000ull + ((uint64_t)(chip)->chip_num) * PNV_HOMER_SIZE)
>>> +#define PNV9_OCC_COMMON_AREA(chip) \
>>> + (0x203fff800000ull + ((uint64_t)(chip)->chip_num) *
>>> PNV_OCC_COMMON_AREA_SIZE)
>>> +
>>> +#endif /* _PPC_PNV_HOMER_H */
>>>
>>
>
- Re: [Qemu-devel] [RFC PATCH 1/6] utils/python_api: add scripting interface for Qemu with python lib, (continued)
[Qemu-devel] [RFC PATCH 2/6] hw/ppc/pnv_xscom: extend xscom to use python interface, Balamuruhan S, 2019/08/07
[Qemu-devel] [RFC PATCH 3/6] hw/ppc/pnv_homer: add homer/occ common area emulation for PowerNV, Balamuruhan S, 2019/08/07
[Qemu-devel] [RFC PATCH 4/6] hw/ppc/pnv: initialize and realize homer/occ common area, Balamuruhan S, 2019/08/07
Re: [Qemu-devel] [RFC PATCH 4/6] hw/ppc/pnv: initialize and realize homer/occ common area, David Gibson, 2019/08/09
[Qemu-devel] [RFC PATCH 5/6] hw/ppc/pnv_xscom: retrieve homer/occ base address from PBA BARs, Balamuruhan S, 2019/08/07