Re: [Qemu-devel] [PATCH v2] pnv: add a physical mapping array describing MMIO ranges in each chip
On 06/14/2018 08:44 AM, David Gibson wrote: > On Wed, Jun 13, 2018 at 09:03:22AM +0200, Cédric Le Goater wrote: >> On 06/13/2018 02:47 AM, David Gibson wrote: >>> On Tue, Jun 12, 2018 at 08:13:49AM +0200, Cédric Le Goater wrote: On 06/12/2018 07:58 AM, David Gibson wrote: > On Wed, Jun 06, 2018 at 09:04:10AM +0200, Cédric Le Goater wrote: >> On 06/06/2018 08:39 AM, David Gibson wrote: >>> On Wed, May 30, 2018 at 12:07:54PM +0200, Cédric Le Goater 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. >>> >>> I think this message needs some work. This says what it's for, but >>> what actually *is* this array, and how does it work? >> >> OK. It is relatively simple: each controller has an entry defining its >> MMIO range. >> >>> The outside-core differences between POWER8 and POWER9 are substantial >>> enough that I'm wondering if pnvP8 and pnvP9 would be better off as >>> different machine types (sharing some routines, of course). >> >> yes and no. I have survived using a common PnvChip framework but >> it is true that I had to add P9 classes for each: LPC, PSI, OCC >> They are very similar but not enough. P9 uses much more MMIOs than >> P8 which still uses a lot of XSCOM. I haven't looked at PHB4. > > Well, it's certainly *possible* to use the same machine type, I'm just > not convinced it's a good idea. It seems kind of dodgy to me that so > many peripherals on the system change as a side-effect of setting the > cpu. Compare to how x86 works where cpu really does change the CPU, > plugging it into the same virtual "chipset". Different chipsets *are* > different machine types there (pc vs. q35). OK, I agree, and we can use a set of common routines to instantiate the different chipset models. So we would have a common pnv_init() routine to initialize the different 'powernv8' and 'powernv9' machines and the PnvChip typename would be a machine class attribute ? >>> >>> Well.. that's one option. Usually for these things, it works out >>> better to instead of parameterizing big high-level routines like >>> pnv_init(), you have separate versions of those calling a combination >>> of case-specific and common routines as necessary. >>> >>> Mostly it just comes down to what is simplest to implement for you, though. >> >> I am introducing a powernv8 machine, the machine init routine is still >> generic and did not change much. But I have deepen the PnvChip class >> inheritance hierarchy with an intermediate class taking care of the >> Chip sub controllers, which gives us something like : >> >> pnv_init() >> . skiboot >> . kernel >> . initrd >> . chips creation >> . platform bus/device : >>isa bus >>pci layout >>bmc handling. >> >> p8 chip hierarchy: >> >>power8_v2.0-pnv-chip (gives the cpu type) >>pnv8-chip : creates the devices only >>pnv-chip: creates xscom and the cores >> >> The powervn9 machine has this hierarchy : >> >>power9_v2.0-pnv-chip >>pnv9-chip >>pnv-chip >> >> I had to introduce these new PnvChipClass ops : >> >> void (*realize)(PnvChip *chip, Error **errp); > > Looks sensible up to this point. yes. The common part only initializes the xscom bus. I wished we could create also the cores but, on the P9, we need the XIVE interrupt controller to be realized before :/ > >> Object *(*intc_create)(PnvChip *chip, Object *child, Error **errp); >> ISABus *(*isa_create)(PnvChip *chip); > > I'm a little more dubious about this - I would have thought your > realize() hook could construct the right intc and isa, rather than > going into generic code, then back out to the callback. The 'intc_create' op creates the interrupt presenter. it is called from the powernv core realize routine and each chip has a different interrupt controller, and so a different presenter type. We cannot create the presenter before and 'pass' it to the core object at realize time because the presenter links in the CPU state. It's a bit of a chicken and egg problem, but I think it is good to have a presenter object all well initialized before we use it. I am open to any suggestions to get rid of this op. sPAPR has exactly the same need. As for the isa_create op, this is because there is only one isa_bus per machine, but you might be right. We can probably create the isa bus on all chips and link the machine to the one on chip0. It should save the op. > But it's not
Re: [Qemu-devel] [PATCH v2] pnv: add a physical mapping array describing MMIO ranges in each chip
On Wed, Jun 13, 2018 at 09:03:22AM +0200, Cédric Le Goater wrote: > On 06/13/2018 02:47 AM, David Gibson wrote: > > On Tue, Jun 12, 2018 at 08:13:49AM +0200, Cédric Le Goater wrote: > >> On 06/12/2018 07:58 AM, David Gibson wrote: > >>> On Wed, Jun 06, 2018 at 09:04:10AM +0200, Cédric Le Goater wrote: > On 06/06/2018 08:39 AM, David Gibson wrote: > > On Wed, May 30, 2018 at 12:07:54PM +0200, Cédric Le Goater 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. > > > > I think this message needs some work. This says what it's for, but > > what actually *is* this array, and how does it work? > > OK. It is relatively simple: each controller has an entry defining its > MMIO range. > > > The outside-core differences between POWER8 and POWER9 are substantial > > enough that I'm wondering if pnvP8 and pnvP9 would be better off as > > different machine types (sharing some routines, of course). > > yes and no. I have survived using a common PnvChip framework but > it is true that I had to add P9 classes for each: LPC, PSI, OCC > They are very similar but not enough. P9 uses much more MMIOs than > P8 which still uses a lot of XSCOM. I haven't looked at PHB4. > >>> > >>> Well, it's certainly *possible* to use the same machine type, I'm just > >>> not convinced it's a good idea. It seems kind of dodgy to me that so > >>> many peripherals on the system change as a side-effect of setting the > >>> cpu. Compare to how x86 works where cpu really does change the CPU, > >>> plugging it into the same virtual "chipset". Different chipsets *are* > >>> different machine types there (pc vs. q35). > >> > >> OK, I agree, and we can use a set of common routines to instantiate the > >> different chipset models. > >> > >> So we would have a common pnv_init() routine to initialize the different > >> 'powernv8' and 'powernv9' machines and the PnvChip typename would be a > >> machine class attribute ? > > > > Well.. that's one option. Usually for these things, it works out > > better to instead of parameterizing big high-level routines like > > pnv_init(), you have separate versions of those calling a combination > > of case-specific and common routines as necessary. > > > > Mostly it just comes down to what is simplest to implement for you, though. > > I am introducing a powernv8 machine, the machine init routine is still > generic and did not change much. But I have deepen the PnvChip class > inheritance hierarchy with an intermediate class taking care of the > Chip sub controllers, which gives us something like : > > pnv_init() > . skiboot > . kernel > . initrd > . chips creation > . platform bus/device : >isa bus >pci layout >bmc handling. > > p8 chip hierarchy: > >power8_v2.0-pnv-chip (gives the cpu type) >pnv8-chip : creates the devices only >pnv-chip: creates xscom and the cores > > The powervn9 machine has this hierarchy : > >power9_v2.0-pnv-chip >pnv9-chip >pnv-chip > > I had to introduce these new PnvChipClass ops : > > void (*realize)(PnvChip *chip, Error **errp); Looks sensible up to this point. > Object *(*intc_create)(PnvChip *chip, Object *child, Error **errp); > ISABus *(*isa_create)(PnvChip *chip); I'm a little more dubious about this - I would have thought your realize() hook could construct the right intc and isa, rather than going into generic code, then back out to the callback. But it's not a big deal. > Overall, it's looking fine and it should remove most of these tests : > >pnv_chip_is_power9(chip) > > If not, it means we are missing a PnvChipClass ops anyway. Nice. > I will send a patchset this week, the final one will shuffle quite a > bit of code and the resulting diff will be a bit fuzy. You will have > to trust me on this one. > > > >> Nevertheless we would still need to introduce "a physical mapping array > >> describing MMIO ranges" but we can start by differentiating the chipsets > >> first. > > > > Well, maybe. I'm wondering if you can more easily encapsulate the > > information in that array in a top-level init routine, that calls > > common helpers with different addresses / device types / whatever. > > Hmm, I think I understand but could you give me a prototype example. > Please. To make sure. > > I would like to keep the array somewhere because, in a quick look, > it gives you an overview of the POWER Processor address space. Ok. Going to a data-driven approach
Re: [Qemu-devel] [PATCH v2] pnv: add a physical mapping array describing MMIO ranges in each chip
On 06/13/2018 02:47 AM, David Gibson wrote: > On Tue, Jun 12, 2018 at 08:13:49AM +0200, Cédric Le Goater wrote: >> On 06/12/2018 07:58 AM, David Gibson wrote: >>> On Wed, Jun 06, 2018 at 09:04:10AM +0200, Cédric Le Goater wrote: On 06/06/2018 08:39 AM, David Gibson wrote: > On Wed, May 30, 2018 at 12:07:54PM +0200, Cédric Le Goater 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. > > I think this message needs some work. This says what it's for, but > what actually *is* this array, and how does it work? OK. It is relatively simple: each controller has an entry defining its MMIO range. > The outside-core differences between POWER8 and POWER9 are substantial > enough that I'm wondering if pnvP8 and pnvP9 would be better off as > different machine types (sharing some routines, of course). yes and no. I have survived using a common PnvChip framework but it is true that I had to add P9 classes for each: LPC, PSI, OCC They are very similar but not enough. P9 uses much more MMIOs than P8 which still uses a lot of XSCOM. I haven't looked at PHB4. >>> >>> Well, it's certainly *possible* to use the same machine type, I'm just >>> not convinced it's a good idea. It seems kind of dodgy to me that so >>> many peripherals on the system change as a side-effect of setting the >>> cpu. Compare to how x86 works where cpu really does change the CPU, >>> plugging it into the same virtual "chipset". Different chipsets *are* >>> different machine types there (pc vs. q35). >> >> OK, I agree, and we can use a set of common routines to instantiate the >> different chipset models. >> >> So we would have a common pnv_init() routine to initialize the different >> 'powernv8' and 'powernv9' machines and the PnvChip typename would be a >> machine class attribute ? > > Well.. that's one option. Usually for these things, it works out > better to instead of parameterizing big high-level routines like > pnv_init(), you have separate versions of those calling a combination > of case-specific and common routines as necessary. > > Mostly it just comes down to what is simplest to implement for you, though. I am introducing a powernv8 machine, the machine init routine is still generic and did not change much. But I have deepen the PnvChip class inheritance hierarchy with an intermediate class taking care of the Chip sub controllers, which gives us something like : pnv_init() . skiboot . kernel . initrd . chips creation . platform bus/device : isa bus pci layout bmc handling. p8 chip hierarchy: power8_v2.0-pnv-chip (gives the cpu type) pnv8-chip : creates the devices only pnv-chip: creates xscom and the cores The powervn9 machine has this hierarchy : power9_v2.0-pnv-chip pnv9-chip pnv-chip I had to introduce these new PnvChipClass ops : void (*realize)(PnvChip *chip, Error **errp); Object *(*intc_create)(PnvChip *chip, Object *child, Error **errp); ISABus *(*isa_create)(PnvChip *chip); Overall, it's looking fine and it should remove most of these tests : pnv_chip_is_power9(chip) If not, it means we are missing a PnvChipClass ops anyway. I will send a patchset this week, the final one will shuffle quite a bit of code and the resulting diff will be a bit fuzy. You will have to trust me on this one. >> Nevertheless we would still need to introduce "a physical mapping array >> describing MMIO ranges" but we can start by differentiating the chipsets >> first. > > Well, maybe. I'm wondering if you can more easily encapsulate the > information in that array in a top-level init routine, that calls > common helpers with different addresses / device types / whatever. Hmm, I think I understand but could you give me a prototype example. Please. To make sure. I would like to keep the array somewhere because, in a quick look, it gives you an overview of the POWER Processor address space. Thanks, C.
Re: [Qemu-devel] [PATCH v2] pnv: add a physical mapping array describing MMIO ranges in each chip
On Tue, Jun 12, 2018 at 08:13:49AM +0200, Cédric Le Goater wrote: > On 06/12/2018 07:58 AM, David Gibson wrote: > > On Wed, Jun 06, 2018 at 09:04:10AM +0200, Cédric Le Goater wrote: > >> On 06/06/2018 08:39 AM, David Gibson wrote: > >>> On Wed, May 30, 2018 at 12:07:54PM +0200, Cédric Le Goater 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. > >>> > >>> I think this message needs some work. This says what it's for, but > >>> what actually *is* this array, and how does it work? > >> > >> OK. It is relatively simple: each controller has an entry defining its > >> MMIO range. > >> > >>> The outside-core differences between POWER8 and POWER9 are substantial > >>> enough that I'm wondering if pnvP8 and pnvP9 would be better off as > >>> different machine types (sharing some routines, of course). > >> > >> yes and no. I have survived using a common PnvChip framework but > >> it is true that I had to add P9 classes for each: LPC, PSI, OCC > >> They are very similar but not enough. P9 uses much more MMIOs than > >> P8 which still uses a lot of XSCOM. I haven't looked at PHB4. > > > > Well, it's certainly *possible* to use the same machine type, I'm just > > not convinced it's a good idea. It seems kind of dodgy to me that so > > many peripherals on the system change as a side-effect of setting the > > cpu. Compare to how x86 works where cpu really does change the CPU, > > plugging it into the same virtual "chipset". Different chipsets *are* > > different machine types there (pc vs. q35). > > OK, I agree, and we can use a set of common routines to instantiate the > different chipset models. > > So we would have a common pnv_init() routine to initialize the different > 'powernv8' and 'powernv9' machines and the PnvChip typename would be a > machine class attribute ? Well.. that's one option. Usually for these things, it works out better to instead of parameterizing big high-level routines like pnv_init(), you have separate versions of those calling a combination of case-specific and common routines as necessary. Mostly it just comes down to what is simplest to implement for you, though. > Nevertheless we would still need to introduce "a physical mapping array > describing MMIO ranges" but we can start by differentiating the chipsets > first. Well, maybe. I'm wondering if you can more easily encapsulate the information in that array in a top-level init routine, that calls common helpers with different addresses / device types / whatever. -- 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
Re: [Qemu-devel] [PATCH v2] pnv: add a physical mapping array describing MMIO ranges in each chip
On 06/12/2018 07:58 AM, David Gibson wrote:
> On Wed, Jun 06, 2018 at 09:04:10AM +0200, Cédric Le Goater wrote:
>> On 06/06/2018 08:39 AM, David Gibson wrote:
>>> On Wed, May 30, 2018 at 12:07:54PM +0200, Cédric Le Goater 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.
>>>
>>> I think this message needs some work. This says what it's for, but
>>> what actually *is* this array, and how does it work?
>>
>> OK. It is relatively simple: each controller has an entry defining its
>> MMIO range.
>>
>>> The outside-core differences between POWER8 and POWER9 are substantial
>>> enough that I'm wondering if pnvP8 and pnvP9 would be better off as
>>> different machine types (sharing some routines, of course).
>>
>> yes and no. I have survived using a common PnvChip framework but
>> it is true that I had to add P9 classes for each: LPC, PSI, OCC
>> They are very similar but not enough. P9 uses much more MMIOs than
>> P8 which still uses a lot of XSCOM. I haven't looked at PHB4.
>
> Well, it's certainly *possible* to use the same machine type, I'm just
> not convinced it's a good idea. It seems kind of dodgy to me that so
> many peripherals on the system change as a side-effect of setting the
> cpu. Compare to how x86 works where cpu really does change the CPU,
> plugging it into the same virtual "chipset". Different chipsets *are*
> different machine types there (pc vs. q35).
OK, I agree, and we can use a set of common routines to instantiate the
different chipset models.
So we would have a common pnv_init() routine to initialize the different
'powernv8' and 'powernv9' machines and the PnvChip typename would be a
machine class attribute ?
Nevertheless we would still need to introduce "a physical mapping array
describing MMIO ranges" but we can start by differentiating the chipsets
first.
C.
>> The interrupt controller is completely different of course.
>>
>> C.
>>
Signed-off-by: Cédric Le Goater
Reviewed-by: Philippe Mathieu-Daudé
---
Changes since v1:
- removed PNV_MAP_MAX which has unused
- introduced a chip class handler to calculate the base address of a
controller as suggested by Greg.
- fix error reporting in pnv_psi_realize()
include/hw/ppc/pnv.h | 51
++
hw/ppc/pnv.c | 53
hw/ppc/pnv_psi.c | 15 ---
hw/ppc/pnv_xscom.c | 8
4 files changed, 96 insertions(+), 31 deletions(-)
diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index 90759240a7b1..ffa4a0899705 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,18 @@ typedef struct PnvChip {
PnvOCC occ;
} PnvChip;
+typedef enum PnvPhysMapType {
+PNV_MAP_XSCOM,
+PNV_MAP_ICP,
+PNV_MAP_PSIHB,
+PNV_MAP_PSIHB_FSP,
+} PnvPhysMapType;
+
+typedef struct PnvPhysMapEntry {
+uint64_tbase;
+uint64_tsize;
+} PnvPhysMapEntry;
+
typedef struct PnvChipClass {
/*< private >*/
SysBusDeviceClass parent_class;
@@ -73,9 +84,10 @@ 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);
+uint64_t (*map_base)(const PnvChip *chip, PnvPhysMapType type);
} PnvChipClass;
#define PNV_CHIP_TYPE_SUFFIX "-" TYPE_PNV_CHIP
@@ -159,9 +171,21 @@ void pnv_bmc_powerdown(IPMIBmc *bmc);
/*
* POWER8 MMIO base addresses
*/
-#define PNV_XSCOM_SIZE0x8ull
-#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 = >phys_map[type];
+
+return map->size;
+}
+
+static inline uint64_t pnv_map_base(const PnvChip *chip,
Re: [Qemu-devel] [PATCH v2] pnv: add a physical mapping array describing MMIO ranges in each chip
On Wed, Jun 06, 2018 at 09:04:10AM +0200, Cédric Le Goater wrote:
> On 06/06/2018 08:39 AM, David Gibson wrote:
> > On Wed, May 30, 2018 at 12:07:54PM +0200, Cédric Le Goater 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.
> >
> > I think this message needs some work. This says what it's for, but
> > what actually *is* this array, and how does it work?
>
> OK. It is relatively simple: each controller has an entry defining its
> MMIO range.
>
> > The outside-core differences between POWER8 and POWER9 are substantial
> > enough that I'm wondering if pnvP8 and pnvP9 would be better off as
> > different machine types (sharing some routines, of course).
>
> yes and no. I have survived using a common PnvChip framework but
> it is true that I had to add P9 classes for each: LPC, PSI, OCC
> They are very similar but not enough. P9 uses much more MMIOs than
> P8 which still uses a lot of XSCOM. I haven't looked at PHB4.
Well, it's certainly *possible* to use the same machine type, I'm just
not convinced it's a good idea. It seems kind of dodgy to me that so
many peripherals on the system change as a side-effect of setting the
cpu. Compare to how x86 works where cpu really does change the CPU,
plugging it into the same virtual "chipset". Different chipsets *are*
different machine types there (pc vs. q35).
> The interrupt controller is completely different of course.
>
> C.
>
> >> Signed-off-by: Cédric Le Goater
> >> Reviewed-by: Philippe Mathieu-Daudé
> >> ---
> >>
> >> Changes since v1:
> >>
> >> - removed PNV_MAP_MAX which has unused
> >> - introduced a chip class handler to calculate the base address of a
> >>controller as suggested by Greg.
> >> - fix error reporting in pnv_psi_realize()
> >>
> >> include/hw/ppc/pnv.h | 51
> >> ++
> >> hw/ppc/pnv.c | 53
> >>
> >> hw/ppc/pnv_psi.c | 15 ---
> >> hw/ppc/pnv_xscom.c | 8
> >> 4 files changed, 96 insertions(+), 31 deletions(-)
> >>
> >> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> >> index 90759240a7b1..ffa4a0899705 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,18 @@ typedef struct PnvChip {
> >> PnvOCC occ;
> >> } PnvChip;
> >>
> >> +typedef enum PnvPhysMapType {
> >> +PNV_MAP_XSCOM,
> >> +PNV_MAP_ICP,
> >> +PNV_MAP_PSIHB,
> >> +PNV_MAP_PSIHB_FSP,
> >> +} PnvPhysMapType;
> >> +
> >> +typedef struct PnvPhysMapEntry {
> >> +uint64_tbase;
> >> +uint64_tsize;
> >> +} PnvPhysMapEntry;
> >> +
> >> typedef struct PnvChipClass {
> >> /*< private >*/
> >> SysBusDeviceClass parent_class;
> >> @@ -73,9 +84,10 @@ 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);
> >> +uint64_t (*map_base)(const PnvChip *chip, PnvPhysMapType type);
> >> } PnvChipClass;
> >>
> >> #define PNV_CHIP_TYPE_SUFFIX "-" TYPE_PNV_CHIP
> >> @@ -159,9 +171,21 @@ void pnv_bmc_powerdown(IPMIBmc *bmc);
> >> /*
> >> * POWER8 MMIO base addresses
> >> */
> >> -#define PNV_XSCOM_SIZE0x8ull
> >> -#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 = >phys_map[type];
> >> +
> >> +return map->size;
> >> +}
> >> +
> >> +static inline uint64_t pnv_map_base(const PnvChip *chip, PnvPhysMapType
> >> type)
> >> +{
> >> +return PNV_CHIP_GET_CLASS(chip)->map_base(chip, type);
> >> +}
> >> +
> >> +#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 +201,13 @@ void pnv_bmc_powerdown(IPMIBmc *bmc);
> >> * 0xe022 -> 0x00038080
> >> * 0xe026 ->
Re: [Qemu-devel] [PATCH v2] pnv: add a physical mapping array describing MMIO ranges in each chip
On 06/06/2018 08:39 AM, David Gibson wrote:
> On Wed, May 30, 2018 at 12:07:54PM +0200, Cédric Le Goater 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.
>
> I think this message needs some work. This says what it's for, but
> what actually *is* this array, and how does it work?
OK. It is relatively simple: each controller has an entry defining its
MMIO range.
> The outside-core differences between POWER8 and POWER9 are substantial
> enough that I'm wondering if pnvP8 and pnvP9 would be better off as
> different machine types (sharing some routines, of course).
yes and no. I have survived using a common PnvChip framework but
it is true that I had to add P9 classes for each: LPC, PSI, OCC
They are very similar but not enough. P9 uses much more MMIOs than
P8 which still uses a lot of XSCOM. I haven't looked at PHB4.
The interrupt controller is completely different of course.
C.
>> Signed-off-by: Cédric Le Goater
>> Reviewed-by: Philippe Mathieu-Daudé
>> ---
>>
>> Changes since v1:
>>
>> - removed PNV_MAP_MAX which has unused
>> - introduced a chip class handler to calculate the base address of a
>>controller as suggested by Greg.
>> - fix error reporting in pnv_psi_realize()
>>
>> include/hw/ppc/pnv.h | 51 ++
>> hw/ppc/pnv.c | 53
>>
>> hw/ppc/pnv_psi.c | 15 ---
>> hw/ppc/pnv_xscom.c | 8
>> 4 files changed, 96 insertions(+), 31 deletions(-)
>>
>> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
>> index 90759240a7b1..ffa4a0899705 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,18 @@ typedef struct PnvChip {
>> PnvOCC occ;
>> } PnvChip;
>>
>> +typedef enum PnvPhysMapType {
>> +PNV_MAP_XSCOM,
>> +PNV_MAP_ICP,
>> +PNV_MAP_PSIHB,
>> +PNV_MAP_PSIHB_FSP,
>> +} PnvPhysMapType;
>> +
>> +typedef struct PnvPhysMapEntry {
>> +uint64_tbase;
>> +uint64_tsize;
>> +} PnvPhysMapEntry;
>> +
>> typedef struct PnvChipClass {
>> /*< private >*/
>> SysBusDeviceClass parent_class;
>> @@ -73,9 +84,10 @@ 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);
>> +uint64_t (*map_base)(const PnvChip *chip, PnvPhysMapType type);
>> } PnvChipClass;
>>
>> #define PNV_CHIP_TYPE_SUFFIX "-" TYPE_PNV_CHIP
>> @@ -159,9 +171,21 @@ void pnv_bmc_powerdown(IPMIBmc *bmc);
>> /*
>> * POWER8 MMIO base addresses
>> */
>> -#define PNV_XSCOM_SIZE0x8ull
>> -#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 = >phys_map[type];
>> +
>> +return map->size;
>> +}
>> +
>> +static inline uint64_t pnv_map_base(const PnvChip *chip, PnvPhysMapType
>> type)
>> +{
>> +return PNV_CHIP_GET_CLASS(chip)->map_base(chip, type);
>> +}
>> +
>> +#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 +201,13 @@ void pnv_bmc_powerdown(IPMIBmc *bmc);
>> * 0xe022 -> 0x00038080
>> * 0xe026 -> 0x00038090
>> */
>> -#define PNV_ICP_SIZE 0x0010ull
>> -#define PNV_ICP_BASE(chip) \
>> -(0x00038000ull + (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 0x0010ull
>> -#define PNV_PSIHB_BASE(chip) \
>> -(0x0003fffe8000ull + (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)
Re: [Qemu-devel] [PATCH v2] pnv: add a physical mapping array describing MMIO ranges in each chip
On Wed, May 30, 2018 at 12:07:54PM +0200, Cédric Le Goater 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.
I think this message needs some work. This says what it's for, but
what actually *is* this array, and how does it work?
The outside-core differences between POWER8 and POWER9 are substantial
enough that I'm wondering if pnvP8 and pnvP9 would be better off as
different machine types (sharing some routines, of course).
> Signed-off-by: Cédric Le Goater
> Reviewed-by: Philippe Mathieu-Daudé
> ---
>
> Changes since v1:
>
> - removed PNV_MAP_MAX which has unused
> - introduced a chip class handler to calculate the base address of a
>controller as suggested by Greg.
> - fix error reporting in pnv_psi_realize()
>
> include/hw/ppc/pnv.h | 51 ++
> hw/ppc/pnv.c | 53
>
> hw/ppc/pnv_psi.c | 15 ---
> hw/ppc/pnv_xscom.c | 8
> 4 files changed, 96 insertions(+), 31 deletions(-)
>
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index 90759240a7b1..ffa4a0899705 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,18 @@ typedef struct PnvChip {
> PnvOCC occ;
> } PnvChip;
>
> +typedef enum PnvPhysMapType {
> +PNV_MAP_XSCOM,
> +PNV_MAP_ICP,
> +PNV_MAP_PSIHB,
> +PNV_MAP_PSIHB_FSP,
> +} PnvPhysMapType;
> +
> +typedef struct PnvPhysMapEntry {
> +uint64_tbase;
> +uint64_tsize;
> +} PnvPhysMapEntry;
> +
> typedef struct PnvChipClass {
> /*< private >*/
> SysBusDeviceClass parent_class;
> @@ -73,9 +84,10 @@ 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);
> +uint64_t (*map_base)(const PnvChip *chip, PnvPhysMapType type);
> } PnvChipClass;
>
> #define PNV_CHIP_TYPE_SUFFIX "-" TYPE_PNV_CHIP
> @@ -159,9 +171,21 @@ void pnv_bmc_powerdown(IPMIBmc *bmc);
> /*
> * POWER8 MMIO base addresses
> */
> -#define PNV_XSCOM_SIZE0x8ull
> -#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 = >phys_map[type];
> +
> +return map->size;
> +}
> +
> +static inline uint64_t pnv_map_base(const PnvChip *chip, PnvPhysMapType type)
> +{
> +return PNV_CHIP_GET_CLASS(chip)->map_base(chip, type);
> +}
> +
> +#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 +201,13 @@ void pnv_bmc_powerdown(IPMIBmc *bmc);
> * 0xe022 -> 0x00038080
> * 0xe026 -> 0x00038090
> */
> -#define PNV_ICP_SIZE 0x0010ull
> -#define PNV_ICP_BASE(chip) \
> -(0x00038000ull + (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 0x0010ull
> -#define PNV_PSIHB_BASE(chip) \
> -(0x0003fffe8000ull + (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 0x0001ull
> -#define PNV_PSIHB_FSP_BASE(chip) \
> -(0x0003ffe0ull + (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 */
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 031488131629..77caaea64b2f 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -712,6 +712,24 @@ static uint32_t pnv_chip_core_pir_p9(PnvChip *chip,
>
Re: [Qemu-devel] [PATCH v2] pnv: add a physical mapping array describing MMIO ranges in each chip
On 05/30/2018 12:23 PM, Greg Kurz wrote:
> On Wed, 30 May 2018 12:07:54 +0200
> Cédric Le Goater 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
>> Reviewed-by: Philippe Mathieu-Daudé
>> ---
>>
>> Changes since v1:
>>
>> - removed PNV_MAP_MAX which has unused
>> - introduced a chip class handler to calculate the base address of a
>>controller as suggested by Greg.
>> - fix error reporting in pnv_psi_realize()
>>
>> include/hw/ppc/pnv.h | 51 ++
>> hw/ppc/pnv.c | 53
>>
>> hw/ppc/pnv_psi.c | 15 ---
>> hw/ppc/pnv_xscom.c | 8
>> 4 files changed, 96 insertions(+), 31 deletions(-)
>>
>> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
>> index 90759240a7b1..ffa4a0899705 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,18 @@ typedef struct PnvChip {
>> PnvOCC occ;
>> } PnvChip;
>>
>> +typedef enum PnvPhysMapType {
>> +PNV_MAP_XSCOM,
>> +PNV_MAP_ICP,
>> +PNV_MAP_PSIHB,
>> +PNV_MAP_PSIHB_FSP,
>> +} PnvPhysMapType;
>> +
>> +typedef struct PnvPhysMapEntry {
>> +uint64_tbase;
>> +uint64_tsize;
>> +} PnvPhysMapEntry;
>> +
>> typedef struct PnvChipClass {
>> /*< private >*/
>> SysBusDeviceClass parent_class;
>> @@ -73,9 +84,10 @@ 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);
>> +uint64_t (*map_base)(const PnvChip *chip, PnvPhysMapType type);
>> } PnvChipClass;
>>
>> #define PNV_CHIP_TYPE_SUFFIX "-" TYPE_PNV_CHIP
>> @@ -159,9 +171,21 @@ void pnv_bmc_powerdown(IPMIBmc *bmc);
>> /*
>> * POWER8 MMIO base addresses
>> */
>> -#define PNV_XSCOM_SIZE0x8ull
>> -#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 = >phys_map[type];
>> +
>> +return map->size;
>> +}
>> +
>> +static inline uint64_t pnv_map_base(const PnvChip *chip, PnvPhysMapType
>> type)
>> +{
>> +return PNV_CHIP_GET_CLASS(chip)->map_base(chip, type);
>> +}
>> +
>> +#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 +201,13 @@ void pnv_bmc_powerdown(IPMIBmc *bmc);
>> * 0xe022 -> 0x00038080
>> * 0xe026 -> 0x00038090
>> */
>> -#define PNV_ICP_SIZE 0x0010ull
>> -#define PNV_ICP_BASE(chip) \
>> -(0x00038000ull + (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 0x0010ull
>> -#define PNV_PSIHB_BASE(chip) \
>> -(0x0003fffe8000ull + (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 0x0001ull
>> -#define PNV_PSIHB_FSP_BASE(chip) \
>> -(0x0003ffe0ull + (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 */
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index 031488131629..77caaea64b2f 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -712,6 +712,24 @@ static uint32_t pnv_chip_core_pir_p9(PnvChip *chip,
>> uint32_t core_id)
>> */
>> #define POWER9_CORE_MASK (0xffull)
>>
>> +/*
>> + * POWER8 MMIOs
>> + */
>> +static const PnvPhysMapEntry
Re: [Qemu-devel] [PATCH v2] pnv: add a physical mapping array describing MMIO ranges in each chip
On Wed, 30 May 2018 12:07:54 +0200
Cédric Le Goater 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
> Reviewed-by: Philippe Mathieu-Daudé
> ---
>
> Changes since v1:
>
> - removed PNV_MAP_MAX which has unused
> - introduced a chip class handler to calculate the base address of a
>controller as suggested by Greg.
> - fix error reporting in pnv_psi_realize()
>
> include/hw/ppc/pnv.h | 51 ++
> hw/ppc/pnv.c | 53
>
> hw/ppc/pnv_psi.c | 15 ---
> hw/ppc/pnv_xscom.c | 8
> 4 files changed, 96 insertions(+), 31 deletions(-)
>
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index 90759240a7b1..ffa4a0899705 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,18 @@ typedef struct PnvChip {
> PnvOCC occ;
> } PnvChip;
>
> +typedef enum PnvPhysMapType {
> +PNV_MAP_XSCOM,
> +PNV_MAP_ICP,
> +PNV_MAP_PSIHB,
> +PNV_MAP_PSIHB_FSP,
> +} PnvPhysMapType;
> +
> +typedef struct PnvPhysMapEntry {
> +uint64_tbase;
> +uint64_tsize;
> +} PnvPhysMapEntry;
> +
> typedef struct PnvChipClass {
> /*< private >*/
> SysBusDeviceClass parent_class;
> @@ -73,9 +84,10 @@ 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);
> +uint64_t (*map_base)(const PnvChip *chip, PnvPhysMapType type);
> } PnvChipClass;
>
> #define PNV_CHIP_TYPE_SUFFIX "-" TYPE_PNV_CHIP
> @@ -159,9 +171,21 @@ void pnv_bmc_powerdown(IPMIBmc *bmc);
> /*
> * POWER8 MMIO base addresses
> */
> -#define PNV_XSCOM_SIZE0x8ull
> -#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 = >phys_map[type];
> +
> +return map->size;
> +}
> +
> +static inline uint64_t pnv_map_base(const PnvChip *chip, PnvPhysMapType type)
> +{
> +return PNV_CHIP_GET_CLASS(chip)->map_base(chip, type);
> +}
> +
> +#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 +201,13 @@ void pnv_bmc_powerdown(IPMIBmc *bmc);
> * 0xe022 -> 0x00038080
> * 0xe026 -> 0x00038090
> */
> -#define PNV_ICP_SIZE 0x0010ull
> -#define PNV_ICP_BASE(chip) \
> -(0x00038000ull + (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 0x0010ull
> -#define PNV_PSIHB_BASE(chip) \
> -(0x0003fffe8000ull + (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 0x0001ull
> -#define PNV_PSIHB_FSP_BASE(chip) \
> -(0x0003ffe0ull + (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 */
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 031488131629..77caaea64b2f 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -712,6 +712,24 @@ static uint32_t pnv_chip_core_pir_p9(PnvChip *chip,
> uint32_t core_id)
> */
> #define POWER9_CORE_MASK (0xffull)
>
> +/*
> + * POWER8 MMIOs
> + */
> +static const PnvPhysMapEntry pnv_chip_power8_phys_map[] = {
> +[PNV_MAP_XSCOM] = { 0x0003fc00ull, 0x0008ull },
> +[PNV_MAP_ICP] = { 0x00038000ull, 0x0010ull
[Qemu-devel] [PATCH v2] pnv: add a physical mapping array describing MMIO ranges in each chip
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
Reviewed-by: Philippe Mathieu-Daudé
---
Changes since v1:
- removed PNV_MAP_MAX which has unused
- introduced a chip class handler to calculate the base address of a
controller as suggested by Greg.
- fix error reporting in pnv_psi_realize()
include/hw/ppc/pnv.h | 51 ++
hw/ppc/pnv.c | 53
hw/ppc/pnv_psi.c | 15 ---
hw/ppc/pnv_xscom.c | 8
4 files changed, 96 insertions(+), 31 deletions(-)
diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index 90759240a7b1..ffa4a0899705 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,18 @@ typedef struct PnvChip {
PnvOCC occ;
} PnvChip;
+typedef enum PnvPhysMapType {
+PNV_MAP_XSCOM,
+PNV_MAP_ICP,
+PNV_MAP_PSIHB,
+PNV_MAP_PSIHB_FSP,
+} PnvPhysMapType;
+
+typedef struct PnvPhysMapEntry {
+uint64_tbase;
+uint64_tsize;
+} PnvPhysMapEntry;
+
typedef struct PnvChipClass {
/*< private >*/
SysBusDeviceClass parent_class;
@@ -73,9 +84,10 @@ 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);
+uint64_t (*map_base)(const PnvChip *chip, PnvPhysMapType type);
} PnvChipClass;
#define PNV_CHIP_TYPE_SUFFIX "-" TYPE_PNV_CHIP
@@ -159,9 +171,21 @@ void pnv_bmc_powerdown(IPMIBmc *bmc);
/*
* POWER8 MMIO base addresses
*/
-#define PNV_XSCOM_SIZE0x8ull
-#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 = >phys_map[type];
+
+return map->size;
+}
+
+static inline uint64_t pnv_map_base(const PnvChip *chip, PnvPhysMapType type)
+{
+return PNV_CHIP_GET_CLASS(chip)->map_base(chip, type);
+}
+
+#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 +201,13 @@ void pnv_bmc_powerdown(IPMIBmc *bmc);
* 0xe022 -> 0x00038080
* 0xe026 -> 0x00038090
*/
-#define PNV_ICP_SIZE 0x0010ull
-#define PNV_ICP_BASE(chip) \
-(0x00038000ull + (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 0x0010ull
-#define PNV_PSIHB_BASE(chip) \
-(0x0003fffe8000ull + (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 0x0001ull
-#define PNV_PSIHB_FSP_BASE(chip) \
-(0x0003ffe0ull + (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 */
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 031488131629..77caaea64b2f 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -712,6 +712,24 @@ static uint32_t pnv_chip_core_pir_p9(PnvChip *chip,
uint32_t core_id)
*/
#define POWER9_CORE_MASK (0xffull)
+/*
+ * POWER8 MMIOs
+ */
+static const PnvPhysMapEntry pnv_chip_power8_phys_map[] = {
+[PNV_MAP_XSCOM] = { 0x0003fc00ull, 0x0008ull },
+[PNV_MAP_ICP] = { 0x00038000ull, 0x0010ull },
+[PNV_MAP_PSIHB] = { 0x0003fffe8000ull, 0x0010ull },
+[PNV_MAP_PSIHB_FSP] = { 0x0003ffe0ull, 0x0001ull },
+};
+
+static uint64_t pnv_chip_map_base_p8(const PnvChip *chip, PnvPhysMapType type)
+{
+PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
+const PnvPhysMapEntry *map =
