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.
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.
> 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.
> ===8<===
>
> The idea is still the same. Meanwhile I'll test more to see whether it has
> other issues.
>
> Thanks,
>
>> Then callers who can be invoked earlier
>> could then handle NULL properly, in this case..
>>
>> ===8<===
>> diff --git a/hw/i386/x86-iommu.c b/hw/i386/x86-iommu.c
>> index 4bfeb08705..fceb7adfe0 100644
>> --- a/hw/i386/x86-iommu.c
>> +++ b/hw/i386/x86-iommu.c
>> @@ -80,9 +80,15 @@ void x86_iommu_irq_to_msi_message(X86IOMMUIrq *irq,
>> MSIMessage *msg_out)
>>
>> X86IOMMUState *x86_iommu_get_default(void)
>> {
>> - MachineState *ms = MACHINE(qdev_get_machine());
>> - PCMachineState *pcms =
>> - PC_MACHINE(object_dynamic_cast(OBJECT(ms), TYPE_PC_MACHINE));
>> + Object *machine = qdev_get_machine();
>> + PCMachineState *pcms;
>> +
>> + /* If machine has not been created, so is the vIOMMU */
>> + if (!machine) {
>> + return NULL;
>> + }
>> +
>> + pcms = PC_MACHINE(object_dynamic_cast(machine, TYPE_PC_MACHINE));
>>
>> if (pcms &&
>> object_dynamic_cast(OBJECT(pcms->iommu), TYPE_X86_IOMMU_DEVICE)) {
>> ===8<===
>>
>> I'll make sure this works if I'll repost.
>>
>> Thanks,
>>
>> --
>> Peter Xu
