Re: [PATCH 0/5] hw/ppc: Set QDev properties using QDev API (part 2/3)

[Daniel Henrique Barboza]

On 2/6/23 05:00, Cédric Le Goater wrote:
> On 2/3/23 22:16, Philippe Mathieu-Daudé wrote:
> > part 1 [*] cover:
> > --
> > QEMU provides the QOM API for core objects.
> > Devices are modelled on top of QOM as QDev objects.
> >
> > There is no point in using the lower level QOM API with
> > QDev; it makes the code more complex and harder to review.
> >
> > I first converted all the calls using errp=&error_abort or
> > &errp=NULL, then noticed the other uses weren't really
> > consistent.
>
> 6/8 years ago, we converted models to QOM, supposedly because the qdev
> interface was legacy and QOM was the new way. That's not true anymore ?
>
> That said, I am ok with changes, even for the best practices. I would
> like to know how to keep track. Do we have a model skeleton/reference ?

I second all that.

Last year we spent a considerable amount of time figuring out how to properly 
use
QOM in the pnv-phb controller, with a lot of code juggling to avoid using qdev
directly. And it's not like we were doing something that was novel - the core
hw/pci/pci.c files are filled with examples such as:

host_bridge = PCI_HOST_BRIDGE(rootbus->qbus.parent);

And this particular example (not accessing qbus.parent to get the parent bus) 
led
to *a lot* of QOM code being added to allow the pnv_phb_root_port to access the
parent bus because "you shouldn't access qdev in that way".

After all that, reading "There is no point in using the lower level QOM API
with QDev; it makes the code more complex and harder to review." is funny. I
know that there might be some nuance that I'm not aware of, and in the end
what we did last year and what Phil is doing today are both steps in the
same direction, but ATM this is confusing to me.

As Cedric said, I believe we should had a document laying out in a clear way 
when
it is OK to use QDEV APIs and when it is OK to use QOM APIs. It would also be 
nice
to document these (apparently) deprecated uses of these APIs that the core 
classes
are doing.


Thanks,

Daniel



> Thanks,
>
> C.
>
> > A QDev property defined with the DEFINE_PROP_xxx() macros
> > is always available, thus can't fail. When using hot-plug
> > devices, we only need to check for optional properties
> > registered at runtime with the object_property_add_XXX()
> > API. Some are even always registered in device instance_init.
> > --
> >
> > In this series PPC hw is converted. Only one call site in PNV
> > forwards the Error* argument and its conversion is justified.
> >
> > Based-on: <20230203180914.49112-1-philmd@linaro.org>
> > (in particular [PATCH 02/19] hw/qdev: Introduce qdev_prop_set_link():
> >   20230203180914.49112-3-philmd@linaro.org/">https://lore.kernel.org/qemu-devel/20230203180914.49112-3-philmd@linaro.org/)
> >
> > [*] 20230203180914.49112-1-philmd@linaro.org/">https://lore.kernel.org/qemu-devel/20230203180914.49112-1-philmd@linaro.org/
> >
> > Philippe Mathieu-Daudé (5):
> >    hw/misc/macio: Set QDev properties using QDev API
> >    hw/pci-host/raven: Set QDev properties using QDev API
> >    hw/ppc/ppc4xx: Set QDev properties using QDev API
> >    hw/ppc/spapr: Set QDev properties using QDev API
> >    hw/ppc/pnv: Set QDev properties using QDev API
> >
> >   hw/intc/pnv_xive.c         | 11 ++++------
> >   hw/intc/pnv_xive2.c        | 15 +++++---------
> >   hw/intc/spapr_xive.c       | 11 ++++------
> >   hw/intc/xics.c             |  4 ++--
> >   hw/intc/xive.c             |  4 ++--
> >   hw/misc/macio/macio.c      |  6 ++----
> >   hw/pci-host/pnv_phb3.c     |  9 +++------
> >   hw/pci-host/pnv_phb4.c     |  4 ++--
> >   hw/pci-host/pnv_phb4_pec.c | 10 +++-------
> >   hw/pci-host/raven.c        |  6 ++----
> >   hw/ppc/e500.c              |  3 +--
> >   hw/ppc/pnv.c               | 41 ++++++++++++++++----------------------
> >   hw/ppc/pnv_psi.c           | 10 +++-------
> >   hw/ppc/ppc405_boards.c     |  6 ++----
> >   hw/ppc/ppc405_uc.c         |  6 +++---
> >   hw/ppc/ppc440_bamboo.c     |  3 +--
> >   hw/ppc/ppc4xx_devs.c       |  2 +-
> >   hw/ppc/sam460ex.c          |  5 ++---
> >   hw/ppc/spapr_irq.c         |  8 +++-----
> >   19 files changed, 62 insertions(+), 102 deletions(-)