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},
};