| Message ID | 20230203211623.50930-1-philmd@linaro.org |
|---|---|
| Headers | show |
| Series | hw/ppc: Set QDev properties using QDev API (part 2/3) | expand |
On 2/3/23 18: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. > > 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. Feel free to take the 4 patches I acked via your tree when pushing it together with part 1/3. I can't ack macio because that's Mark's turf. Thanks, Daniel > > Based-on: <20230203180914.49112-1-philmd@linaro.org> > (in particular [PATCH 02/19] hw/qdev: Introduce qdev_prop_set_link(): > https://lore.kernel.org/qemu-devel/20230203180914.49112-3-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(-) >
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 ? 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(): > https://lore.kernel.org/qemu-devel/20230203180914.49112-3-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(-) >
On 06/02/2023 08: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 is a good question, and something that we really should decide first before going ahead with these changes. My understanding is that architectures with newer machines (particularly ARM and PPC) use QOM APIs directly, however more recently Markus did some improvements to qdev which largely eliminated the gap between the two. Hence why these days the two are mostly interchangeable: the main difference is that qdev has a notion of a parent which can be useful during device modelling. > 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 ? Agreed. I've added Peter on CC as I know he has had some thoughts on QOM vs. qdev, but certainly as a reviewer it would be great to know which way we should be heading in the future. ATB, Mark.
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(): >> https://lore.kernel.org/qemu-devel/20230203180914.49112-3-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(-) >> >