On Thu, Nov 07, 2024 at 12:12:10PM +0100, Markus Armbruster wrote:
> Peter Xu <pet...@redhat.com> writes:
>
> > On Fri, Oct 25, 2024 at 05:55:59PM -0400, Peter Xu wrote:
> >> On Fri, Oct 25, 2024 at 11:25:23AM +0200, Markus Armbruster wrote:
> >> > Peter Xu <pet...@redhat.com> writes:
> >> >
> >> > > X86 IOMMUs cannot be created more than one on a system yet. Make it a
> >> > > singleton so it guards the system from accidentally create yet another
> >> > > IOMMU object when one already presents.
> >> > >
> >> > > Now if someone tries to create more than one, e.g., via:
> >> > >
> >> > > ./qemu -M q35 -device intel-iommu -device intel-iommu
> >> > >
> >> > > The error will change from:
> >> > >
> >> > > qemu-system-x86_64: -device intel-iommu: QEMU does not support
> >> > > multiple vIOMMUs for x86 yet.
> >> > >
> >> > > To:
> >> > >
> >> > > qemu-system-x86_64: -device intel-iommu: Class 'intel-iommu' only
> >> > > supports one instance
> >> > >
> >> > > Unfortunately, yet we can't remove the singleton check in the machine
> >> > > hook (pc_machine_device_pre_plug_cb), because there can also be
> >> > > virtio-iommu involved, which doesn't share a common parent class yet.
> >> > >
> >> > > But with this, it should be closer to reach that goal to check
> >> > > singleton by
> >> > > QOM one day.
> >> > >
> >> > > Signed-off-by: Peter Xu <pet...@redhat.com>
> >> >
> >> > $ qemu-system-x86_64 -device amd-iommu,help
> >> > /work/armbru/qemu/include/hw/boards.h:24:MACHINE: Object 0x56473906f960
> >> > is not an instance of type machine
> >> > Aborted (core dumped)
>
> [...]
>
> >> Thanks for the report!
> >>
> >> It turns out that qdev_get_machine() cannot be invoked too early, and the
> >> singleton code can make it earlier..
> >>
> >> We may want a pre-requisite patch to allow qdev_get_machine() to be invoked
> >> anytime, like:
> >>
> >> ===8<===
> >> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> >> index db36f54d91..7ceae47139 100644
> >> --- a/hw/core/qdev.c
> >> +++ b/hw/core/qdev.c
> >> @@ -831,6 +831,16 @@ Object *qdev_get_machine(void)
> >> {
> >> static Object *dev;
> >>
> >> + if (!phase_check(PHASE_MACHINE_CREATED)) {
> >> + /*
> >> + * When the machine is not created, below can wrongly create
> >> + * /machine to be a container.. this enables qdev_get_machine() to
> >> + * be used at any time and return NULL properly when machine is
> >> not
> >> + * created.
> >> + */
> >> + return NULL;
> >> + }
> >> +
> >> if (dev == NULL) {
> >> dev = container_get(object_get_root(), "/machine");
> >> }
> >> ===8<===
> >>
> >> I hope it makes sense on its own.
> >
> > My apologies, spoke too soon here. This helper is used too after machine
> > is created, but right before switching to PHASE_MACHINE_CREATE stage..
>
> container_get() is a trap.
I had the same feeling.. Though I'd confess I'm not familiar enough with
this part of code.
>
> When the object to be gotten is always "container", it merely
> complicates container creation: it's implicitly created on first get.
> Which of the calls creates may be less than obvious.
>
> When the object to be gotten is something else, such as a machine,
> container_get() before creation is *wrong*, and will lead to trouble
> later.
>
> In my opinion:
>
> * Hiding creation in getters is a bad idea unless creation has no
> material side effects.
>
> * Getting anything but a container with container_get() is in bad taste.
Agreed.
IMHO container_get() interface might still be ok to implicitly create
containers, but only if it will: (1) always make sure what it walks is a
container along the way, and (2) never return any non-container.
>
>
> > So we need another way, like:
> >
> > ===8<===
> >
> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > index db36f54d91..36a9fdb428 100644
> > --- a/hw/core/qdev.c
> > +++ b/hw/core/qdev.c
> > @@ -832,7 +832,13 @@ Object *qdev_get_machine(void)
> > static Object *dev;
> >
> > if (dev == NULL) {
> > - dev = container_get(object_get_root(), "/machine");
> > + /*
> > + * NOTE: dev can keep being NULL if machine is not yet created!
> > + * In which case the function will properly return NULL.
> > + *
> > + * Whenever machine object is created and found once, we cache it.
> > + */
> > + dev = object_resolve_path_component(object_get_root(), "machine");
> > }
> >
> > return dev;
>
> Now returns null instead of a bogus container when called before machine
> creation. Improvement of sorts. But none of the callers expect null...
> shouldn't we assert(dev) here?
>
> Hmm, below you add a caller that checks for null.
>
> Another nice mess.
I plan to put aside the application of singletons to x86-iommu as of now,
due to the fact that qdev complexity may better be done separately.
IOW, before that, I wonder whether we should clean up the container_get()
as you discussed: it doesn't sound like a good interface to return
non-container objects.
I had a quick look, I only see two outliers of such, and besides the
"abuse" in qdev_get_machine(), the only other one is
e500_pcihost_bridge_realize():
*** hw/core/qdev.c:
qdev_get_machine[820] dev = container_get(object_get_root(),
"/machine");
*** hw/pci-host/ppce500.c:
e500_pcihost_bridge_realize[422] PPCE500CCSRState *ccsr =
CCSR(container_get(qdev_get_machine(),
If any of us thinks this is the right way to go, I can try to clean it up
(for 10.0). qdev_get_machine() may still need to be able to return NULL
when singleton applies to IOMMUs, but that can be for later. Before that,
we can still assert(qdev), I think.
Just to mention I've posted rfcv2 for this series, again feel free to
ignore patch 3-5 as of now:
[PATCH RFC v2 0/7] QOM: Singleton interface
https://lore.kernel.org/r/20241029211607.2114845-1-pet...@redhat.com
I think the plan is Dan may keep collecting feedbacks on his other rfc:
[RFC 0/5] RFC: require error handling for dynamically created objects
https://lore.kernel.org/r/20241031155350.3240361-1-berra...@redhat.com
Then after Dan's lands, I'll rebase my rfcv2 on top of his, dropping
iommu/qdev changes.
Thanks,
--
Peter Xu
