Re: [Qemu-devel] [PATCH 2/7] tests/qgraph: pci-pc driver and interface nodes
On 07/18/2018 09:33 PM, Paolo Bonzini wrote: On 18/07/2018 20:29, Emanuele wrote: I had to put this patch here because it also introduces qpci_device_init, used by sdhci (patch 3). For the next version I plan to have a patch X where I rename all occurrences of qpci_init_pc in qpci_pc_new, and a patch X+1 that introduces qpci_init_pc (was qpci_set_pc) and the other changes. Should I only introduce qpci_device_init in patch 3 and the remaining things in patch 5? I think the general problem here is that in some patches I create functions that are planned to only be used only in next patches (of the current series). I think it's okay this way, however you should justify the changes you make to "qgraph-ify" each component. For patch 1, let's wait for Stefan's reply. Because patch 1 is introducing the infrastructure, I think it is acceptable that some definitions are introduced early as long as they have doc comments; it would make little sense to introduce get_device in patch 4 just because there are no "contains" edges until then. However, introducing the qos-test directly at the beginning is also a possibility. In either case, we need better doc comments for the function pointers in QOSGraphObject. What would you suggest as better doc comments? For version 2 I have written a little introduction like in qom/object.h, essentially pasting the cover letter and a working example. Paolo
Re: [Qemu-devel] [PATCH 2/7] tests/qgraph: pci-pc driver and interface nodes
On 11/07/2018 19:46, Emanuele wrote:
>>> +static void qpci(void)
>>> +{
>>> + qos_node_create_interface("pci-bus");
>>> +}
>>> +
>>> +libqos_init(qpci);
>> Why does an interface need to be created? The drivers declare which
>> interfaces they support?
>>
>> I don't think this can be used to detect typoes in the driver's
>> qos_node_produces() call since there is no explicit control over the
>> order in which libqos_init() functions are called. So the driver may
>> call qos_node_produces() before the qos_node_create_interface() is
>> called?
> The interface is what is actually given to the test, so from there it
> can test the functions and fields regardless of which driver is actually
> implementing them.
> For example, sdhci-test uses the QSDHCI functions, and depending on the
> path the generic-sdhci or sdhci-pci functions will be used.
I think Stefan is right, if you adjust qos_node_create_interface so that
it is idempotent(*), you can make it static and call it from
qos_node_produces and qos_node_consumes.
(*) that is, fail like now if the node exists and is not an interface;
but, succeed if the node exists and is an interface.
Thanks,
Paolo
Re: [Qemu-devel] [PATCH 2/7] tests/qgraph: pci-pc driver and interface nodes
On 18/07/2018 20:29, Emanuele wrote: > I had to put this patch here because it also introduces > qpci_device_init, used by sdhci (patch 3). > > For the next version I plan to have a patch X where I rename all > occurrences of qpci_init_pc in qpci_pc_new, and a patch X+1 that > introduces qpci_init_pc (was qpci_set_pc) and the other changes. > > Should I only introduce qpci_device_init in patch 3 and the remaining > things in patch 5? > > I think the general problem here is that in some patches I create > functions that are planned to only be used only in next patches (of the > current series). I think it's okay this way, however you should justify the changes you make to "qgraph-ify" each component. For patch 1, let's wait for Stefan's reply. Because patch 1 is introducing the infrastructure, I think it is acceptable that some definitions are introduced early as long as they have doc comments; it would make little sense to introduce get_device in patch 4 just because there are no "contains" edges until then. However, introducing the qos-test directly at the beginning is also a possibility. In either case, we need better doc comments for the function pointers in QOSGraphObject. Paolo
Re: [Qemu-devel] [PATCH 2/7] tests/qgraph: pci-pc driver and interface nodes
On 07/18/2018 04:29 PM, Stefan Hajnoczi wrote:
On Wed, Jul 11, 2018 at 05:18:03PM +0200, Paolo Bonzini wrote:
On 11/07/2018 16:49, Stefan Hajnoczi wrote:
On Mon, Jul 09, 2018 at 11:11:31AM +0200, Emanuele Giuseppe Esposito wrote:
-QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc)
+static void *qpci_get_driver(void *obj, const char *interface)
{
-QPCIBusPC *ret = g_new0(QPCIBusPC, 1);
+QPCIBusPC *qpci = obj;
+if (!g_strcmp0(interface, "pci-bus")) {
+return >bus;
+}
+printf("%s not present in pci-bus-pc", interface);
+abort();
+}
At this point I wonder if it makes sense to use the QEMU Object Model
(QOM), which has interfaces and inheritance. qgraph duplicates part of
the object model.
Replying for Emanuele on this point since we didn't really go through
QOM yet; I'll let him answer the comments that are more related to the code.
QOM is much heavier weight than qgraph, and adds a lot more boilerplate:
- QOM properties interact with QAPI and bring in a lot of baggage;
qgraph would only use "child" properties to implement containment.
- QOM objects are long-lived, qgraph objects only last for the duration
of a single test. qgraph doesn't need reference counting or complex
two-phase initialization like "realize" or "user_complete"
- QOM's hierarchy is shallow, but qgraph doesn't really need _any_
hierarchy. Because it focuses on interface rather than hierarchy,
everything can be expressed simply through structs contained into one
another.
Consider that qgraph is 1/4th the size of QOM, and a large part of it is
the graph data structure that (as you said) would not be provided by QOM.
There are two things where using QOM would save a little bit of
duplicated concepts, namely the get_driver (get interface, what you
quote above) and get_device (get contained object) callbacks. However,
it wouldn't simplify the code at all, because it would require the
introduction of separate instance and class structs. We didn't want to
add all too much boilerplate for people that want to write qtest, as you
pointed out in the review of patch 4.
Yes, I think these are good points. QOM does involve a lot of
boilerplate for defining objects.
+void qpci_set_pc(QPCIBusPC *ret, QTestState *qts, QGuestAllocator *alloc)
It's not clear to me what the purpose of this function is - at least the
name is a bit cryptic since it seems more like an initialization
function than 'setting pc' on QPCIBusPC. How about inlining this in
qpci_init_pc() instead of keeping a separate function?
I agree about the naming. Perhaps we can rename the existing
qpci_init_pc to qpci_pc_new, and qpci_set_pc to qpci_pc_init.
Would you prefer if the split was done in the patch that introduces the
user for qpci_set_pc (patch 5)? We did it here because this patch
prepares qpci-pc
Either way is fine. I suggested inlining mainly because it avoids the
need to pick a good name :).
I had to put this patch here because it also introduces
qpci_device_init, used by sdhci (patch 3).
For the next version I plan to have a patch X where I rename all
occurrences of qpci_init_pc in qpci_pc_new, and a patch X+1 that
introduces qpci_init_pc (was qpci_set_pc) and the other changes.
Should I only introduce qpci_device_init in patch 3 and the remaining
things in patch 5?
I think the general problem here is that in some patches I create
functions that are planned to only be used only in next patches (of the
current series).
Stefan
Re: [Qemu-devel] [PATCH 2/7] tests/qgraph: pci-pc driver and interface nodes
On Wed, Jul 11, 2018 at 07:46:09PM +0200, Emanuele wrote:
> On 07/11/2018 04:49 PM, Stefan Hajnoczi wrote:
> > On Mon, Jul 09, 2018 at 11:11:31AM +0200, Emanuele Giuseppe Esposito wrote:
> > > +void qpci_device_init(QPCIDevice *dev, QPCIBus *bus, int devfn)
> > > +{
> > > +if (!bus) {
> > > +return;
> > > +}
> > When does this happen and why?
> Ok maybe this is unneeded, I added it because I was creating a test with a
> NULL bus, but we ended up putting it aside. So you're right, this should be
> eliminated.
Thanks!
> > > +dev->bus = bus;
> > > +dev->devfn = devfn;
> > > +
> > > +if (qpci_config_readw(dev, PCI_VENDOR_ID) == 0x) {
> > > +printf("PCI Device not found\n");
> > > +abort();
> > > +}
> > > +qpci_device_enable(dev);
> > > +}
> > > +
> > > +void qpci_set_pc(QPCIBusPC *ret, QTestState *qts, QGuestAllocator *alloc)
> > It's not clear to me what the purpose of this function is - at least the
> > name is a bit cryptic since it seems more like an initialization
> > function than 'setting pc' on QPCIBusPC. How about inlining this in
> > qpci_init_pc() instead of keeping a separate function?
> The idea is that, following the graph that Paolo wrote in the GSoC project
> wiki (https://wiki.qemu.org/Features/qtest_driver_framework), QPCIBusPC is
> "contained" in i440FX-pcihost. This means that the i440FX-pcihost struct has
> a QPCIBusPC field.
>
> Therefore I had to put QPCIBusPC as public (in order to have it as field),
> even though qpci_init_pc() was not what I needed to initialize it, because
> that function is allocating a new QPCIBusPC, while I just needed to "set"
> its values.
> From here, qpci_set_pc().
I see. Renaming qpci_set_pc() to qpci_pc_init() and including a doc
comment explaining that this is the in-place initialization function
would make things clear.
> > > +{
> > > assert(qts);
> > > ret->bus.pio_readb = qpci_pc_pio_readb;
> > > @@ -147,11 +164,23 @@ QPCIBus *qpci_init_pc(QTestState *qts,
> > > QGuestAllocator *alloc)
> > > ret->bus.mmio_alloc_ptr = 0xE000;
> > > ret->bus.mmio_limit = 0x1ULL;
> > > +ret->obj.get_driver = qpci_get_driver;
> > > +}
> > > +
> > > +QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc)
> > > +{
> > > +QPCIBusPC *ret = g_new0(QPCIBusPC, 1);
> > > +qpci_set_pc(ret, qts, alloc);
> > > +
> > > return >bus;
> > > }
> > > void qpci_free_pc(QPCIBus *bus)
> > > {
> > > +if (!bus) {
> > > +return;
> > > +}
> > Why is this needed now?
> Not having this would mean failure of tests like vrtio-user-test, because
> now QPCIBusPC has two fields, and QPCIBus bus; is not the first one either.
> Therefore, if I remember correctly doing something like
> container_of(NULL, QPCIBusPC, bus)
> where bus is not the first field of QPCIBusPC would result in a negative
> number, and freeing that would make the test crash.
>
> I discovered this issue while doing some checks, and already proposed a
> patch for it, even though we ended up agreeing that this fix was only needed
> in my patch and not in the current QEMU implementation.
I see now:
void free_ahci_device(QPCIDevice *dev)
{
QPCIBus *pcibus = dev ? dev->bus : NULL;
/* libqos doesn't have a function for this, so free it manually */
g_free(dev);
qpci_free_pc(pcibus);
}
The caller is assuming it's okay to pass NULL.
This is a good candidate for a separate patch so that you can explain
the rationale ("Although containerof(NULL, QPCIBusPC, bus) happens to
evaluate to NULL today thanks to the QPCIBusPC struct layout, it's
generally bad practice to rely on that. Normally containerof() is used
precisely because an offset into the struct is required and a
straightforward cast would not work. We got lucky, but don't depend on
it anymore.").
> > > +
> > > QPCIBusPC *s = container_of(bus, QPCIBusPC, bus);
> > > g_free(s);
> > > @@ -176,3 +205,11 @@ void qpci_unplug_acpi_device_test(const char *id,
> > > uint8_t slot)
> > > qmp_eventwait("DEVICE_DELETED");
> > > }
> > > +
> > > +static void qpci_pc(void)
> > > +{
> > > +qos_node_create_driver("pci-bus-pc", NULL);
> > > +qos_node_produces("pci-bus-pc", "pci-bus");
> > In QOM pci-bus-pc would be a class, pci-bus would be an interface. From
> > a driver perspective it seems QOM can already do what is needed and the
> > qgraph infrastructure isn't necessary.
> >
> > Obviously the depth-first search *is* unique and not in QOM, although
> > QOM does offer a tree namespace which can be used for looking up object
> > instances and I guess this could be used to configure tests at runtime.
> >
> > I'll think about this more as I read the rest of the patches.
> >
> > > +}
> > > +
> > > +libqos_init(qpci_pc);
> > > diff --git a/tests/libqos/pci-pc.h b/tests/libqos/pci-pc.h
> > > index 491eeac756..ee381c5667 100644
> > > --- a/tests/libqos/pci-pc.h
> > > +++ b/tests/libqos/pci-pc.h
> >
Re: [Qemu-devel] [PATCH 2/7] tests/qgraph: pci-pc driver and interface nodes
On Wed, Jul 11, 2018 at 05:18:03PM +0200, Paolo Bonzini wrote:
> On 11/07/2018 16:49, Stefan Hajnoczi wrote:
> > On Mon, Jul 09, 2018 at 11:11:31AM +0200, Emanuele Giuseppe Esposito wrote:
> >> -QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc)
> >> +static void *qpci_get_driver(void *obj, const char *interface)
> >> {
> >> -QPCIBusPC *ret = g_new0(QPCIBusPC, 1);
> >> +QPCIBusPC *qpci = obj;
> >> +if (!g_strcmp0(interface, "pci-bus")) {
> >> +return >bus;
> >> +}
> >> +printf("%s not present in pci-bus-pc", interface);
> >> +abort();
> >> +}
> >
> > At this point I wonder if it makes sense to use the QEMU Object Model
> > (QOM), which has interfaces and inheritance. qgraph duplicates part of
> > the object model.
>
> Replying for Emanuele on this point since we didn't really go through
> QOM yet; I'll let him answer the comments that are more related to the code.
>
> QOM is much heavier weight than qgraph, and adds a lot more boilerplate:
>
> - QOM properties interact with QAPI and bring in a lot of baggage;
> qgraph would only use "child" properties to implement containment.
>
> - QOM objects are long-lived, qgraph objects only last for the duration
> of a single test. qgraph doesn't need reference counting or complex
> two-phase initialization like "realize" or "user_complete"
>
> - QOM's hierarchy is shallow, but qgraph doesn't really need _any_
> hierarchy. Because it focuses on interface rather than hierarchy,
> everything can be expressed simply through structs contained into one
> another.
>
> Consider that qgraph is 1/4th the size of QOM, and a large part of it is
> the graph data structure that (as you said) would not be provided by QOM.
>
> There are two things where using QOM would save a little bit of
> duplicated concepts, namely the get_driver (get interface, what you
> quote above) and get_device (get contained object) callbacks. However,
> it wouldn't simplify the code at all, because it would require the
> introduction of separate instance and class structs. We didn't want to
> add all too much boilerplate for people that want to write qtest, as you
> pointed out in the review of patch 4.
Yes, I think these are good points. QOM does involve a lot of
boilerplate for defining objects.
> >> +void qpci_set_pc(QPCIBusPC *ret, QTestState *qts, QGuestAllocator *alloc)
> >
> > It's not clear to me what the purpose of this function is - at least the
> > name is a bit cryptic since it seems more like an initialization
> > function than 'setting pc' on QPCIBusPC. How about inlining this in
> > qpci_init_pc() instead of keeping a separate function?
>
> I agree about the naming. Perhaps we can rename the existing
> qpci_init_pc to qpci_pc_new, and qpci_set_pc to qpci_pc_init.
>
> Would you prefer if the split was done in the patch that introduces the
> user for qpci_set_pc (patch 5)? We did it here because this patch
> prepares qpci-pc
Either way is fine. I suggested inlining mainly because it avoids the
need to pick a good name :).
Stefan
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH 2/7] tests/qgraph: pci-pc driver and interface nodes
Hi Emanuele,
On 07/09/2018 06:11 AM, Emanuele Giuseppe Esposito wrote:
> Add pci-bus-pc node and pci-bus interface, moved QPCIBusPC struct
"move"
> declaration in its header (since it will be needed by other drivers)
> and introduced a setter method for drivers that do not need to allocate
"introduce"
> but have to initialize QPCIBusPC.
>
> Signed-off-by: Emanuele Giuseppe Esposito
> ---
> tests/libqos/pci-pc.c | 53 ---
> tests/libqos/pci-pc.h | 8 +++
> tests/libqos/pci.c| 8 +++
> 3 files changed, 61 insertions(+), 8 deletions(-)
>
> diff --git a/tests/libqos/pci-pc.c b/tests/libqos/pci-pc.c
> index a7803308b7..f1c1741279 100644
> --- a/tests/libqos/pci-pc.c
> +++ b/tests/libqos/pci-pc.c
> @@ -18,15 +18,9 @@
>
> #include "qemu-common.h"
>
> -
> #define ACPI_PCIHP_ADDR 0xae00
> #define PCI_EJ_BASE 0x0008
>
> -typedef struct QPCIBusPC
> -{
> -QPCIBus bus;
> -} QPCIBusPC;
> -
> static uint8_t qpci_pc_pio_readb(QPCIBus *bus, uint32_t addr)
> {
> return inb(addr);
> @@ -115,10 +109,33 @@ static void qpci_pc_config_writel(QPCIBus *bus, int
> devfn, uint8_t offset, uint3
> outl(0xcfc, value);
> }
>
> -QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc)
> +static void *qpci_get_driver(void *obj, const char *interface)
> {
> -QPCIBusPC *ret = g_new0(QPCIBusPC, 1);
> +QPCIBusPC *qpci = obj;
> +if (!g_strcmp0(interface, "pci-bus")) {
> +return >bus;
> +}
> +printf("%s not present in pci-bus-pc", interface);
> +abort();
> +}
>
> +void qpci_device_init(QPCIDevice *dev, QPCIBus *bus, int devfn)
Maybe this one belongs to "pci.c" (not being 'pc' related).
> +{
> +if (!bus) {
> +return;
> +}
> +dev->bus = bus;
> +dev->devfn = devfn;
> +
> +if (qpci_config_readw(dev, PCI_VENDOR_ID) == 0x) {
> +printf("PCI Device not found\n");
> +abort();
> +}
> +qpci_device_enable(dev);
> +}
> +
> +void qpci_set_pc(QPCIBusPC *ret, QTestState *qts, QGuestAllocator *alloc)
> +{
> assert(qts);
>
> ret->bus.pio_readb = qpci_pc_pio_readb;
> @@ -147,11 +164,23 @@ QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator
> *alloc)
> ret->bus.mmio_alloc_ptr = 0xE000;
> ret->bus.mmio_limit = 0x1ULL;
>
> +ret->obj.get_driver = qpci_get_driver;
> +}
> +
> +QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc)
> +{
> +QPCIBusPC *ret = g_new0(QPCIBusPC, 1);
> +qpci_set_pc(ret, qts, alloc);
> +
> return >bus;
> }
>
> void qpci_free_pc(QPCIBus *bus)
> {
> +if (!bus) {
> +return;
> +}
> +
> QPCIBusPC *s = container_of(bus, QPCIBusPC, bus);
>
> g_free(s);
> @@ -176,3 +205,11 @@ void qpci_unplug_acpi_device_test(const char *id,
> uint8_t slot)
>
> qmp_eventwait("DEVICE_DELETED");
> }
> +
> +static void qpci_pc(void)
> +{
> +qos_node_create_driver("pci-bus-pc", NULL);
> +qos_node_produces("pci-bus-pc", "pci-bus");
> +}
> +
> +libqos_init(qpci_pc);
> diff --git a/tests/libqos/pci-pc.h b/tests/libqos/pci-pc.h
> index 491eeac756..ee381c5667 100644
> --- a/tests/libqos/pci-pc.h
> +++ b/tests/libqos/pci-pc.h
> @@ -15,7 +15,15 @@
>
> #include "libqos/pci.h"
> #include "libqos/malloc.h"
> +#include "qgraph.h"
>
> +typedef struct QPCIBusPC {
> +QOSGraphObject obj;
> +QPCIBus bus;
> +} QPCIBusPC;
> +
> +void qpci_device_init(QPCIDevice *dev, QPCIBus *bus, int devfn);
Ditto, belongs to "libqos/pci.h".
> +void qpci_set_pc(QPCIBusPC *ret, QTestState *qts, QGuestAllocator *alloc);
> QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc);
> void qpci_free_pc(QPCIBus *bus);
>
> diff --git a/tests/libqos/pci.c b/tests/libqos/pci.c
> index 0b73cb23d0..c51c186867 100644
> --- a/tests/libqos/pci.c
> +++ b/tests/libqos/pci.c
> @@ -15,6 +15,7 @@
>
> #include "hw/pci/pci_regs.h"
> #include "qemu/host-utils.h"
> +#include "qgraph.h"
>
> void qpci_device_foreach(QPCIBus *bus, int vendor_id, int device_id,
> void (*func)(QPCIDevice *dev, int devfn, void
> *data),
> @@ -402,3 +403,10 @@ void qpci_plug_device_test(const char *driver, const
> char *id,
> qtest_qmp_device_add(driver, id, "'addr': '%d'%s%s", slot,
> opts ? ", " : "", opts ? opts : "");
> }
> +
> +static void qpci(void)
> +{
> +qos_node_create_interface("pci-bus");
> +}
> +
> +libqos_init(qpci);
>
Re: [Qemu-devel] [PATCH 2/7] tests/qgraph: pci-pc driver and interface nodes
On 07/11/2018 04:49 PM, Stefan Hajnoczi wrote:
On Mon, Jul 09, 2018 at 11:11:31AM +0200, Emanuele Giuseppe Esposito wrote:
-QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc)
+static void *qpci_get_driver(void *obj, const char *interface)
{
-QPCIBusPC *ret = g_new0(QPCIBusPC, 1);
+QPCIBusPC *qpci = obj;
+if (!g_strcmp0(interface, "pci-bus")) {
+return >bus;
+}
+printf("%s not present in pci-bus-pc", interface);
+abort();
+}
At this point I wonder if it makes sense to use the QEMU Object Model
(QOM), which has interfaces and inheritance. qgraph duplicates part of
the object model.
+void qpci_device_init(QPCIDevice *dev, QPCIBus *bus, int devfn)
+{
+if (!bus) {
+return;
+}
When does this happen and why?
Ok maybe this is unneeded, I added it because I was creating a test with
a NULL bus, but we ended up putting it aside. So you're right, this
should be eliminated.
+dev->bus = bus;
+dev->devfn = devfn;
+
+if (qpci_config_readw(dev, PCI_VENDOR_ID) == 0x) {
+printf("PCI Device not found\n");
+abort();
+}
+qpci_device_enable(dev);
+}
+
+void qpci_set_pc(QPCIBusPC *ret, QTestState *qts, QGuestAllocator *alloc)
It's not clear to me what the purpose of this function is - at least the
name is a bit cryptic since it seems more like an initialization
function than 'setting pc' on QPCIBusPC. How about inlining this in
qpci_init_pc() instead of keeping a separate function?
The idea is that, following the graph that Paolo wrote in the GSoC
project wiki (https://wiki.qemu.org/Features/qtest_driver_framework),
QPCIBusPC is "contained" in i440FX-pcihost. This means that the
i440FX-pcihost struct has a QPCIBusPC field.
Therefore I had to put QPCIBusPC as public (in order to have it as
field), even though qpci_init_pc() was not what I needed to initialize
it, because that function is allocating a new QPCIBusPC, while I just
needed to "set" its values.
From here, qpci_set_pc().
+{
assert(qts);
ret->bus.pio_readb = qpci_pc_pio_readb;
@@ -147,11 +164,23 @@ QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator
*alloc)
ret->bus.mmio_alloc_ptr = 0xE000;
ret->bus.mmio_limit = 0x1ULL;
+ret->obj.get_driver = qpci_get_driver;
+}
+
+QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc)
+{
+QPCIBusPC *ret = g_new0(QPCIBusPC, 1);
+qpci_set_pc(ret, qts, alloc);
+
return >bus;
}
void qpci_free_pc(QPCIBus *bus)
{
+if (!bus) {
+return;
+}
Why is this needed now?
Not having this would mean failure of tests like vrtio-user-test,
because now QPCIBusPC has two fields, and QPCIBus bus; is not the first
one either.
Therefore, if I remember correctly doing something like
container_of(NULL, QPCIBusPC, bus)
where bus is not the first field of QPCIBusPC would result in a negative
number, and freeing that would make the test crash.
I discovered this issue while doing some checks, and already proposed a
patch for it, even though we ended up agreeing that this fix was only
needed in my patch and not in the current QEMU implementation.
+
QPCIBusPC *s = container_of(bus, QPCIBusPC, bus);
g_free(s);
@@ -176,3 +205,11 @@ void qpci_unplug_acpi_device_test(const char *id, uint8_t
slot)
qmp_eventwait("DEVICE_DELETED");
}
+
+static void qpci_pc(void)
+{
+qos_node_create_driver("pci-bus-pc", NULL);
+qos_node_produces("pci-bus-pc", "pci-bus");
In QOM pci-bus-pc would be a class, pci-bus would be an interface. From
a driver perspective it seems QOM can already do what is needed and the
qgraph infrastructure isn't necessary.
Obviously the depth-first search *is* unique and not in QOM, although
QOM does offer a tree namespace which can be used for looking up object
instances and I guess this could be used to configure tests at runtime.
I'll think about this more as I read the rest of the patches.
+}
+
+libqos_init(qpci_pc);
diff --git a/tests/libqos/pci-pc.h b/tests/libqos/pci-pc.h
index 491eeac756..ee381c5667 100644
--- a/tests/libqos/pci-pc.h
+++ b/tests/libqos/pci-pc.h
@@ -15,7 +15,15 @@
#include "libqos/pci.h"
#include "libqos/malloc.h"
+#include "qgraph.h"
+typedef struct QPCIBusPC {
+QOSGraphObject obj;
+QPCIBus bus;
+} QPCIBusPC;
Why does this need to be public?
See previous answer
+
+void qpci_device_init(QPCIDevice *dev, QPCIBus *bus, int devfn);
Why does this need to be public?
I'll use that in the next patches. I also realized this function should
go in pci.h, and not pci-pc.h
+void qpci_set_pc(QPCIBusPC *ret, QTestState *qts, QGuestAllocator *alloc);
Why does this need to be public?
Because I use it in the next patch to set the QPCIBusPC in the x86_64/pc
machine.
QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc);
void qpci_free_pc(QPCIBus *bus);
diff --git a/tests/libqos/pci.c b/tests/libqos/pci.c
index
Re: [Qemu-devel] [PATCH 2/7] tests/qgraph: pci-pc driver and interface nodes
On 11/07/2018 16:49, Stefan Hajnoczi wrote:
> On Mon, Jul 09, 2018 at 11:11:31AM +0200, Emanuele Giuseppe Esposito wrote:
>> -QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc)
>> +static void *qpci_get_driver(void *obj, const char *interface)
>> {
>> -QPCIBusPC *ret = g_new0(QPCIBusPC, 1);
>> +QPCIBusPC *qpci = obj;
>> +if (!g_strcmp0(interface, "pci-bus")) {
>> +return >bus;
>> +}
>> +printf("%s not present in pci-bus-pc", interface);
>> +abort();
>> +}
>
> At this point I wonder if it makes sense to use the QEMU Object Model
> (QOM), which has interfaces and inheritance. qgraph duplicates part of
> the object model.
Replying for Emanuele on this point since we didn't really go through
QOM yet; I'll let him answer the comments that are more related to the code.
QOM is much heavier weight than qgraph, and adds a lot more boilerplate:
- QOM properties interact with QAPI and bring in a lot of baggage;
qgraph would only use "child" properties to implement containment.
- QOM objects are long-lived, qgraph objects only last for the duration
of a single test. qgraph doesn't need reference counting or complex
two-phase initialization like "realize" or "user_complete"
- QOM's hierarchy is shallow, but qgraph doesn't really need _any_
hierarchy. Because it focuses on interface rather than hierarchy,
everything can be expressed simply through structs contained into one
another.
Consider that qgraph is 1/4th the size of QOM, and a large part of it is
the graph data structure that (as you said) would not be provided by QOM.
There are two things where using QOM would save a little bit of
duplicated concepts, namely the get_driver (get interface, what you
quote above) and get_device (get contained object) callbacks. However,
it wouldn't simplify the code at all, because it would require the
introduction of separate instance and class structs. We didn't want to
add all too much boilerplate for people that want to write qtest, as you
pointed out in the review of patch 4.
>> +void qpci_set_pc(QPCIBusPC *ret, QTestState *qts, QGuestAllocator *alloc)
>
> It's not clear to me what the purpose of this function is - at least the
> name is a bit cryptic since it seems more like an initialization
> function than 'setting pc' on QPCIBusPC. How about inlining this in
> qpci_init_pc() instead of keeping a separate function?
I agree about the naming. Perhaps we can rename the existing
qpci_init_pc to qpci_pc_new, and qpci_set_pc to qpci_pc_init.
Would you prefer if the split was done in the patch that introduces the
user for qpci_set_pc (patch 5)? We did it here because this patch
prepares qpci-pc
Thanks,
Paolo
>> +{
>> assert(qts);
>>
>> ret->bus.pio_readb = qpci_pc_pio_readb;
>> @@ -147,11 +164,23 @@ QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator
>> *alloc)
>> ret->bus.mmio_alloc_ptr = 0xE000;
>> ret->bus.mmio_limit = 0x1ULL;
>>
>> +ret->obj.get_driver = qpci_get_driver;
>> +}
>> +
>> +QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc)
>> +{
>> +QPCIBusPC *ret = g_new0(QPCIBusPC, 1);
>> +qpci_set_pc(ret, qts, alloc);
>> +
>> return >bus;
>> }
>>
>> void qpci_free_pc(QPCIBus *bus)
>> {
>> +if (!bus) {
>> +return;
>> +}
>
> Why is this needed now?
>
>> +
>> QPCIBusPC *s = container_of(bus, QPCIBusPC, bus);
>>
>> g_free(s);
>> @@ -176,3 +205,11 @@ void qpci_unplug_acpi_device_test(const char *id,
>> uint8_t slot)
>>
>> qmp_eventwait("DEVICE_DELETED");
>> }
>> +
>> +static void qpci_pc(void)
>> +{
>> +qos_node_create_driver("pci-bus-pc", NULL);
>> +qos_node_produces("pci-bus-pc", "pci-bus");
>
> In QOM pci-bus-pc would be a class, pci-bus would be an interface. From
> a driver perspective it seems QOM can already do what is needed and the
> qgraph infrastructure isn't necessary.
>
> Obviously the depth-first search *is* unique and not in QOM, although
> QOM does offer a tree namespace which can be used for looking up object
> instances and I guess this could be used to configure tests at runtime.
>
> I'll think about this more as I read the rest of the patches.
>
>> +}
>> +
>> +libqos_init(qpci_pc);
>> diff --git a/tests/libqos/pci-pc.h b/tests/libqos/pci-pc.h
>> index 491eeac756..ee381c5667 100644
>> --- a/tests/libqos/pci-pc.h
>> +++ b/tests/libqos/pci-pc.h
>> @@ -15,7 +15,15 @@
>>
>> #include "libqos/pci.h"
>> #include "libqos/malloc.h"
>> +#include "qgraph.h"
>>
>> +typedef struct QPCIBusPC {
>> +QOSGraphObject obj;
>> +QPCIBus bus;
>> +} QPCIBusPC;
>
> Why does this need to be public?
>
>> +
>> +void qpci_device_init(QPCIDevice *dev, QPCIBus *bus, int devfn);
>
> Why does this need to be public?
>
>> +void qpci_set_pc(QPCIBusPC *ret, QTestState *qts, QGuestAllocator *alloc);
>
> Why does this need to be public?
>
>> QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc);
>> void qpci_free_pc(QPCIBus
Re: [Qemu-devel] [PATCH 2/7] tests/qgraph: pci-pc driver and interface nodes
On Mon, Jul 09, 2018 at 11:11:31AM +0200, Emanuele Giuseppe Esposito wrote:
> -QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc)
> +static void *qpci_get_driver(void *obj, const char *interface)
> {
> -QPCIBusPC *ret = g_new0(QPCIBusPC, 1);
> +QPCIBusPC *qpci = obj;
> +if (!g_strcmp0(interface, "pci-bus")) {
> +return >bus;
> +}
> +printf("%s not present in pci-bus-pc", interface);
> +abort();
> +}
At this point I wonder if it makes sense to use the QEMU Object Model
(QOM), which has interfaces and inheritance. qgraph duplicates part of
the object model.
> +void qpci_device_init(QPCIDevice *dev, QPCIBus *bus, int devfn)
> +{
> +if (!bus) {
> +return;
> +}
When does this happen and why?
> +dev->bus = bus;
> +dev->devfn = devfn;
> +
> +if (qpci_config_readw(dev, PCI_VENDOR_ID) == 0x) {
> +printf("PCI Device not found\n");
> +abort();
> +}
> +qpci_device_enable(dev);
> +}
> +
> +void qpci_set_pc(QPCIBusPC *ret, QTestState *qts, QGuestAllocator *alloc)
It's not clear to me what the purpose of this function is - at least the
name is a bit cryptic since it seems more like an initialization
function than 'setting pc' on QPCIBusPC. How about inlining this in
qpci_init_pc() instead of keeping a separate function?
> +{
> assert(qts);
>
> ret->bus.pio_readb = qpci_pc_pio_readb;
> @@ -147,11 +164,23 @@ QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator
> *alloc)
> ret->bus.mmio_alloc_ptr = 0xE000;
> ret->bus.mmio_limit = 0x1ULL;
>
> +ret->obj.get_driver = qpci_get_driver;
> +}
> +
> +QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc)
> +{
> +QPCIBusPC *ret = g_new0(QPCIBusPC, 1);
> +qpci_set_pc(ret, qts, alloc);
> +
> return >bus;
> }
>
> void qpci_free_pc(QPCIBus *bus)
> {
> +if (!bus) {
> +return;
> +}
Why is this needed now?
> +
> QPCIBusPC *s = container_of(bus, QPCIBusPC, bus);
>
> g_free(s);
> @@ -176,3 +205,11 @@ void qpci_unplug_acpi_device_test(const char *id,
> uint8_t slot)
>
> qmp_eventwait("DEVICE_DELETED");
> }
> +
> +static void qpci_pc(void)
> +{
> +qos_node_create_driver("pci-bus-pc", NULL);
> +qos_node_produces("pci-bus-pc", "pci-bus");
In QOM pci-bus-pc would be a class, pci-bus would be an interface. From
a driver perspective it seems QOM can already do what is needed and the
qgraph infrastructure isn't necessary.
Obviously the depth-first search *is* unique and not in QOM, although
QOM does offer a tree namespace which can be used for looking up object
instances and I guess this could be used to configure tests at runtime.
I'll think about this more as I read the rest of the patches.
> +}
> +
> +libqos_init(qpci_pc);
> diff --git a/tests/libqos/pci-pc.h b/tests/libqos/pci-pc.h
> index 491eeac756..ee381c5667 100644
> --- a/tests/libqos/pci-pc.h
> +++ b/tests/libqos/pci-pc.h
> @@ -15,7 +15,15 @@
>
> #include "libqos/pci.h"
> #include "libqos/malloc.h"
> +#include "qgraph.h"
>
> +typedef struct QPCIBusPC {
> +QOSGraphObject obj;
> +QPCIBus bus;
> +} QPCIBusPC;
Why does this need to be public?
> +
> +void qpci_device_init(QPCIDevice *dev, QPCIBus *bus, int devfn);
Why does this need to be public?
> +void qpci_set_pc(QPCIBusPC *ret, QTestState *qts, QGuestAllocator *alloc);
Why does this need to be public?
> QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc);
> void qpci_free_pc(QPCIBus *bus);
>
> diff --git a/tests/libqos/pci.c b/tests/libqos/pci.c
> index 0b73cb23d0..c51c186867 100644
> --- a/tests/libqos/pci.c
> +++ b/tests/libqos/pci.c
> @@ -15,6 +15,7 @@
>
> #include "hw/pci/pci_regs.h"
> #include "qemu/host-utils.h"
> +#include "qgraph.h"
>
> void qpci_device_foreach(QPCIBus *bus, int vendor_id, int device_id,
> void (*func)(QPCIDevice *dev, int devfn, void
> *data),
> @@ -402,3 +403,10 @@ void qpci_plug_device_test(const char *driver, const
> char *id,
> qtest_qmp_device_add(driver, id, "'addr': '%d'%s%s", slot,
> opts ? ", " : "", opts ? opts : "");
> }
> +
> +static void qpci(void)
> +{
> +qos_node_create_interface("pci-bus");
> +}
> +
> +libqos_init(qpci);
Why does an interface need to be created? The drivers declare which
interfaces they support?
I don't think this can be used to detect typoes in the driver's
qos_node_produces() call since there is no explicit control over the
order in which libqos_init() functions are called. So the driver may
call qos_node_produces() before the qos_node_create_interface() is
called?
signature.asc
Description: PGP signature
