Re: [PATCH 2/4] x86/iommu: Make x86-iommu a singleton object

[Markus Armbruster <armbru@redhat.com>]

Peter Xu <peterx@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 <peterx@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 <peterx@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