Re: [PATCH 00/17] powernv: introduce pnv-phb unified devices - Mark Cave-Ayland

From: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
To: Daniel Henrique Barboza <danielhb413@gmail.com>, qemu-devel@nongnu.org
Cc: qemu-ppc@nongnu.org, david@gibson.dropbear.id.au, clg@kaod.org,
	fbarrat@linux.ibm.com
Subject: Re: [PATCH 00/17] powernv: introduce pnv-phb unified devices
Date: Tue, 10 May 2022 08:57:28 +0100	[thread overview]
Message-ID: <f0492ebb-d742-d465-421b-8c412de65f65@ilande.co.uk> (raw)
In-Reply-To: <e7f2e6bc-b443-ed96-683b-835fbe7a55b8@gmail.com>

On 09/05/2022 23:30, Daniel Henrique Barboza wrote:

> On 5/9/22 18:17, Mark Cave-Ayland wrote:
>> On 07/05/2022 20:06, Daniel Henrique Barboza wrote:
>>
>>> Hi,
>>>
>>> Since the 7.0.0 release cycle we have a desire to use the powernv
>>> emulation with libvirt. To do that we need to enable user creatable
>>> pnv-phb devices to allow more user customization an to avoid spamming
>>> multiple default devices in the domain XML. In the middle of the
>>> previous cycle we experimented with user created
>>> pnv-phb3/pnv-phb3-root-port/pnv-phb4/pnv-phb4-root-port/pnv-phb5. The
>>> end result, although functional, is that the user needs to deal with a
>>> lot of versioned devices that, from the user perspective, does the same
>>> thing. In a way we outsourced the implementation details of the PHBs
>>> (e.g. pnv8 uses phb3, pnv9 uses phb4) to the command line. Having more
>>> devices also puts an extra burden in the libvirt support we want to
>>> have.
>>>
>>> To solve this, Cedric and Frederic gave the idea of adding a common
>>> virtual pnv-phb device that the user can add in the command line, and
>>> QEMU handles the details internally. Unfortunatelly this idea turned out
>>> to be way harder than anticipated. Between creating a device that would
>>> just forward the callbacks to the existing devices internally, creating
>>> a PnvPHB device with the minimal attributes and making the other devices
>>> inherit from it, and making an 'abstract' device that could be casted
>>> for both phb3 and phb4 PHBs,
>>
>> This bit sounds all good...
>>
>>> all sorts of very obscure problems occured:
>>> PHBs not being detected, interrupts not being delivered and memory
>>> regions not being able to read/write registers. My initial impression is
>>> that there are assumptions made both in ppc/pnv and hw/pci-host that
>>> requires the memory region and the bus being in the same device. Even
>>> if we somehow figure all this out, the resulting code is hacky and
>>> annoying to maitain.
>>
>> But this seems really surprising since there are many examples of similar QOM 
>> patterns within the codebase, and in my experience they work really well. Do you 
>> have a particular example that you can share to demonstrate why the result of the 
>> QOM mapping is making things more difficult?
> 
> 
> It's not so much about the OOM getting in the way, but rather myself not being
> able to use QOM in a way to get what I want. There is a good probability that
> QOM is able to do it.
> 
> Talking about the 2 PHBs pnv-phb3 (powernv8 only) and pnv-phb4 (powernv9 only),
> what we want is a way to let the user instantiate both using an alias, or
> an abstract object if you will, called "pnv-phb". This alias/abstraction would
> instantiate either a pnv-phb3 or a pnv-phb4 depending on the actual machine
> running.
> 
> QOM has the "interface" concept that is used internally to make the device behave
> like the interface describes it, but I wasn't able to expose this kind of object
> to the user. It also has the "abstract" concept that, by the documentation itself,
> isn't supposed to be user created. Eventually I gave up this idea, accepting that
> only real devices can be exposed to the user (please correct me if I'm wrong).

Sorry, I should have clarified this a bit more: introducing an abstract type is a 
separate task from adding your proxy device type, but I think will definitely help 
maintaining the individual versions. Certainly it will make it easier to see which 
fields are in use by specific versions, and it also gives you a QOM cast for the 
superclass of all PHB devices to help with type checking.

> After that I tried to create a pnv-phb object that contains the common attributes
> of both pnv-phb3 and pnv-phb4. This object would be the parent of phb3 and phb4,
> and during realize time creating a pnv-phb object would create a pnv-phb3/4 and
> we go from there. This attempt went far after a few tweaks but then I started
> to hit the problems I mentioned above: some strange behaviors with interrupts,
> PHBs not appearing and so on. I got a little farther by moving all the memory
> regions from phb3/phb4 to the parent phb object and I got up to the guest login,
> but without network. Since moving the memory regions in the same object as the
> pci root bus got me farther I concluded that there might some relation/assumption
> made somewhere that I was breaking before.

That sounds really odd. The normal reasons for this in my experience are i) 
forgetting to update the class/instance size in the class_init function (an 
--enable-sanitizers build will catch this) and ii) not propagating device 
reset/realize functions to a parent device leading to uninitialised state.

> After all that I got more than halfway of what I ended up doing. I decided to
> unify phb3/phb4 into a single device, renamed their attributes that has different
> meanings between v3 and v4 code, and I ended up with this series.

As a rough outline for a pnv-phb device, I'd aim for creating a proxy for the 
underlying device rather than manually invoking the QOM instance and qdev-related 
functions:

struct PnvPHB {
     ....
     uint32_t version;
     Object *phb_dev;  /* Could be PHBCommonBase if it exists */
};

DECLARE_SIMPLE_OBJECT_TYPE(...)

...
...

static Property pnv_phb_properties[] = {
     DEFINE_PROP_UINT32("version", PnvPHB, version, 0),
     DEFINE_PROP_END_OF_LIST(),
};

static void pnv_phb_realize(DeviceState *dev, Error **errp)
{
     PnvPHB *pnv_phb = PNV_PHB(dev);
     g_autofree char *phb_typename;

     if (!pnv_phb->version) {
         error_setg("version not specified", errp);
         return;
     }

     switch (pnv_phb->version) {
     case 3:
         phb_typename = g_strdup(TYPE_PNV_PHB3);
         break;
     case 4:
         phb_typename = g_strdup(TYPE_PNV_PHB4);
         break;
     default:
         g_assert_unreached();
     }

     pnv_phb->phb_dev = object_new(phb_typename);
     object_property_add_child(OBJECT(dev), "phb-device", pnv_phb->phb_dev);

     if (!qdev_realize(DEVICE(pnv_phb->phb_dev), errp)) {
         return;
     }

     /* Passthrough child device properties to the proxy device */
     qdev_alias_all_properties(dev, OBJECT(pnv_phb->phb_dev));
}

Finally you can set the pnv-phb version on a per-machine basis by adding the version 
to the machine compat_props:

static GlobalProperty compat[] = {
     { TYPE_PHB_PNV, "version", 3},
};

>>> This brings us to this series. The cleaner way I found to accomplish
>>> what we want to do is to create real, unified pnv-phb/phb-phb-root-port
>>> devices, and get rid of all the versioned ones. This is done by
>>> merging all the PHB3/PHB4 attributes in unified devices. pnv_phb3* and pnv_phb4*
>>> files end up using the same pnv-phb and phb-phb-root-port unified devices,
>>> with the difference that pnv_phb3* only cares about version 3 attributes
>>> and pnv_phb4* only cares about PHB4 attributes. Introducing new PHB
>>> versions in the future will be a matter of adding any non-existent
>>> attributes in the unified pnv-phb* devices and using them in separated
>>> pnv_phbN* files.
>>>
>>> The pnv-phb implementation per se is just a router for either phb3 or
>>> phb4 logic, done in init() and realize() time, after we check which powernv
>>> machine we're running. If running with powernv8 we redirect control to
>>> pnv_phb3_realize(), otherwise we redirect to pnv_phb4_realize(). The
>>> unified device does not do anything per se other than handling control
>>> to the right version.
>>>
>>> After this series, this same powernv8 command line that boots a powernv8
>>> machine with some phbs and root ports and with network:
>>>
>>> ./qemu-system-ppc64 -m 4G \
>>> -machine powernv8 -smp 2,sockets=2,cores=1,threads=1  \
>>> -accel tcg,thread=multi -bios skiboot.lid  \
>>> -kernel vmlinux -initrd buildroot.rootfs.cpio -append 'console=hvc0 ro xmon=on'  \
>>> -nodefaults  -serial mon:stdio -nographic  \
>>> -device pnv-phb,chip-id=0,index=0,id=pcie.0 \
>>> -device pnv-phb,chip-id=0,index=1,id=pcie.1 \
>>> -device pnv-phb,chip-id=1,index=2,id=pcie.2 \
>>> -device pnv-phb-root-port,id=root0,bus=pcie.2 \
>>> -device pnv-phb-root-port,id=root1,bus=pcie.1 \
>>> -device pcie-pci-bridge,id=bridge1,bus=root0,addr=0x0  \
>>> -device nvme,bus=bridge1,addr=0x1,drive=drive0,serial=1234  \
>>> -drive file=./simics-disk.raw,if=none,id=drive0,format=raw,cache=none  \
>>> -device e1000e,netdev=net0,mac=C0:ff:EE:00:01:04,bus=bridge1,addr=0x3 \
>>> -netdev bridge,helper=/usr/libexec/qemu-bridge-helper,br=virbr0,id=net0 \
>>> -device nec-usb-xhci,bus=bridge1,addr=0x2
>>>
>>>
>>> Can be used to boot powernv9 and powernv10 machines with the same attributes
>>> just by changing the machine type.
>>>
>>>
>>> Daniel Henrique Barboza (17):
>>>    ppc/pnv: rename PnvPHB3.ioda* to PnvPHB3.ioda2*
>>>    ppc/pnv: rename PnvPHB3.regs[] to PnvPHB3.regs3[]
>>>    ppc/pnv: rename PnvPHB3.dma_spaces to PnvPHB3.v3_dma_spaces
>>>    ppc/pnv: add unified pnv-phb header
>>>    ppc/pnv: add pnv-phb device
>>>    ppc/pnv: remove PnvPHB3
>>>    ppc/pnv: user created pnv-phb for powernv8
>>>    ppc/pnv: remove PnvPHB4
>>>    ppc/pnv: user creatable pnv-phb for powernv9
>>>    ppc/pnv: use PnvPHB.version
>>>    ppc/pnv: change pnv_phb4_get_pec() to also retrieve chip10->pecs
>>>    ppc/pnv: user creatable pnv-phb for powernv10
>>>    ppc/pnv: add pnv_phb_get_current_machine()
>>>    ppc/pnv: add pnv-phb-root-port device
>>>    ppc/pnv: remove pnv-phb3-root-port
>>>    ppc/pnv: remove pnv-phb4-root-port
>>>    ppc/pnv: remove pecc->rp_model
>>>
>>>   hw/pci-host/meson.build        |   3 +-
>>>   hw/pci-host/pnv_phb.c          | 258 ++++++++++++++++++++++++++++
>>>   hw/pci-host/pnv_phb3.c         | 256 +++++++++++-----------------
>>>   hw/pci-host/pnv_phb3_msi.c     |  12 +-
>>>   hw/pci-host/pnv_phb3_pbcq.c    |   8 +-
>>>   hw/pci-host/pnv_phb4.c         | 298 ++++++++++++---------------------
>>>   hw/pci-host/pnv_phb4_pec.c     |  14 +-
>>>   hw/ppc/pnv.c                   |  41 ++++-
>>>   include/hw/pci-host/pnv_phb.h  | 224 +++++++++++++++++++++++++
>>>   include/hw/pci-host/pnv_phb3.h | 118 +------------
>>>   include/hw/pci-host/pnv_phb4.h | 108 ++----------
>>>   include/hw/ppc/pnv.h           |   3 +-
>>>   12 files changed, 757 insertions(+), 586 deletions(-)
>>>   create mode 100644 hw/pci-host/pnv_phb.c
>>>   create mode 100644 include/hw/pci-host/pnv_phb.h
>>
>> I'm completely on-board with having a proxy-like PHB device that maps to the 
>> correct underlying PHB version, but to me it feels like combining multiple separate 
>> devices into a single one is going to make things more complicated to maintain in 
>> the long term.
>  
> I'm all up for a "virtual pnv-phb" device that can be used together with the
> existing versioned phbs. I wasn't able to pull that off.

ATB,

Mark.