Re: [Qemu-devel] [PATCH v3 2/2] i440fx: print an error message if user tries to enable iommu

[Markus Armbruster]

Bandan Das <address@hidden> writes:

> Markus Armbruster <address@hidden> writes:
>
>> Bandan Das <address@hidden> writes:
>>
>>> There's no indication of any sort that i440fx doesn't support
>>> "iommu=on"
>>>
>>> Reviewed-by: Eric Blake <address@hidden>
>>> Signed-off-by: Bandan Das <address@hidden>
>>> ---
>>>  hw/pci-host/piix.c | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
>>> index 7b2fbf9..715208b 100644
>>> --- a/hw/pci-host/piix.c
>>> +++ b/hw/pci-host/piix.c
>>> @@ -34,6 +34,7 @@
>>>  #include "sysemu/sysemu.h"
>>>  #include "hw/i386/ioapic.h"
>>>  #include "qapi/visitor.h"
>>> +#include "qemu/error-report.h"
>>>  
>>>  /*
>>>   * I440FX chipset data sheet.
>>> @@ -301,6 +302,10 @@ static void i440fx_pcihost_realize(DeviceState *dev, 
>>> Error **errp)
>>>  static void i440fx_realize(PCIDevice *dev, Error **errp)
>>>  {
>>>      dev->config[I440FX_SMRAM] = 0x02;
>>> +
>>> +    if (object_property_get_bool(qdev_get_machine(), "iommu", NULL)) {
>>> +        error_report("warning: i440fx doesn't support emulated iommu");
>>> +    }
>>>  }
>>>  
>>>  PCIBus *i440fx_init(const char *host_type, const char *pci_type,
>>
>> Hmm.
>>
>> If I understand things correctly, we add property "iommu" to *any*
>> machine, whether it supports it or not (see machine_initfn() in
>> hw/core/machine.c).

Many more properties there that make sense only for some machines.

>> Most machines don't support it.  You add a warning to one of them.
>
> Yeah, I guess the only one that does is q35. Although, iommu should
> be theoretically applicable to all machine types, a generic "iommu"
> property common to all types makes little sense to me. 
>
> One way is to remove the common property and make "iommu" a property of
> Q35 only (through a custom instance_init). 

Technically an incompatible change: a bogus iommu= is no longer silently
ignored.  I can think of four replies:

0. We don't care about bogus properties, so leave machine.c alone.  You
   can add warnings wherever you like.  If you want to find out whether
   the machine supports an IOMMU, use something else than QOM
   introspection.

1. Backward compatibility idolatry: we must keep silently ignoring bogus
   iommu=... forever.

2. Not rejecting iommu=on when the machine has none is a bug, accepting
   iommu=off always is a feature.  We must keep accepting iommu=off, and
   reject iommu=on.  To make IOMMU-support introspectable in QOM, you
   could give the iommu property a silly type with only one value "off"
   when the machine doesn't support it.

3. Does a compatility break make a noise when nobody's watching it?
   Just drop the property for machines where it doesn't make sense.

I like 3.  Without an actual QMP client interested in examining machine
capabilities via QOM introspection, I can't reject 0.

> Other is to simply make the property common to pc by moving it to pc.c
> But that still doesn't solve the problem since i440fx cannot emulate it yet.
>
>> Why to that one and not the others?
>>
>> Shouldn't we add properties only to machines where they make sense?
>> Adding them indiscrimiately defeats QOM introspection.
>
> For now, maybe we can just skip this patch. I will post another
> generic solution.
> It's just weird that qemu runs happily without complaining when iommu is
> specified with an unsupported type.

Yes, but it's just as weird for all the other machines, too :)

Making all machines warn about machine properties they don't understand
won't fly.

A generic mechanism that records when a machine uses a property and
warns about unused ones might work.