[Qemu-devel] [PATCH v3 0/2] Minor cleanups when parsing the "iommu" option

[Qemu-devel] [PATCH v3 0/2] Minor cleanups when parsing the "iommu" option

[Bandan Das]

Small cleanup changes. The first removes the helper function by directly
checking the property and the second adds a error message if user tries
to use "-machine iommu=on" with i440fx.

v3:
error_report does not need a \n!

v2:
2/2: use error_report for the warning message

Bandan Das (2):
  q35: Check propery to determine if iommu is set
  i440fx: print an error message if user tries to enable iommu

 hw/core/machine.c   | 5 -----
 hw/pci-host/piix.c  | 5 +++++
 hw/pci-host/q35.c   | 2 +-
 include/hw/boards.h | 1 -
 4 files changed, 6 insertions(+), 7 deletions(-)

-- 
2.5.0

[Qemu-devel] [PATCH v3 1/2] q35: Check propery to determine if iommu is set

[Bandan Das]

The helper function machine_iommu() isn't necesary. We can
directly check for the property.

Signed-off-by: Bandan Das <bsd@redhat.com>
---
 hw/core/machine.c   | 5 -----
 hw/pci-host/q35.c   | 2 +-
 include/hw/boards.h | 1 -
 3 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index f4db340..acca00d 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -462,11 +462,6 @@ bool machine_usb(MachineState *machine)
     return machine->usb;
 }
 
-bool machine_iommu(MachineState *machine)
-{
-    return machine->iommu;
-}
-
 bool machine_kernel_irqchip_allowed(MachineState *machine)
 {
     return machine->kernel_irqchip_allowed;
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index c81507d..1fb4707 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -506,7 +506,7 @@ static void mch_realize(PCIDevice *d, Error **errp)
                  PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
     }
     /* Intel IOMMU (VT-d) */
-    if (machine_iommu(current_machine)) {
+    if (object_property_get_bool(qdev_get_machine(), "iommu", NULL)) {
         mch_init_dmar(mch);
     }
 }
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 3e9a92c..24eb6f0 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -33,7 +33,6 @@ MachineClass *find_default_machine(void);
 extern MachineState *current_machine;
 
 bool machine_usb(MachineState *machine);
-bool machine_iommu(MachineState *machine);
 bool machine_kernel_irqchip_allowed(MachineState *machine);
 bool machine_kernel_irqchip_required(MachineState *machine);
 int machine_kvm_shadow_mem(MachineState *machine);
-- 
2.5.0

Re: [Qemu-devel] [PATCH v3 1/2] q35: Check propery to determine if iommu is set

[Markus Armbruster]

Bandan Das <bsd@redhat.com> writes:

> The helper function machine_iommu() isn't necesary. We can
> directly check for the property.
>
> Signed-off-by: Bandan Das <bsd@redhat.com>
> ---
>  hw/core/machine.c   | 5 -----
>  hw/pci-host/q35.c   | 2 +-
>  include/hw/boards.h | 1 -
>  3 files changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index f4db340..acca00d 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -462,11 +462,6 @@ bool machine_usb(MachineState *machine)
>      return machine->usb;
>  }
>  
> -bool machine_iommu(MachineState *machine)
> -{
> -    return machine->iommu;
> -}
> -
>  bool machine_kernel_irqchip_allowed(MachineState *machine)
>  {
>      return machine->kernel_irqchip_allowed;
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index c81507d..1fb4707 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -506,7 +506,7 @@ static void mch_realize(PCIDevice *d, Error **errp)
>                   PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
>      }
>      /* Intel IOMMU (VT-d) */
> -    if (machine_iommu(current_machine)) {
> +    if (object_property_get_bool(qdev_get_machine(), "iommu", NULL)) {
>          mch_init_dmar(mch);
>      }
>  }

There's ample prededence for this technique.

> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 3e9a92c..24eb6f0 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -33,7 +33,6 @@ MachineClass *find_default_machine(void);
>  extern MachineState *current_machine;
>  
>  bool machine_usb(MachineState *machine);
> -bool machine_iommu(MachineState *machine);
>  bool machine_kernel_irqchip_allowed(MachineState *machine);
>  bool machine_kernel_irqchip_required(MachineState *machine);
>  int machine_kvm_shadow_mem(MachineState *machine);

Reviewed-by: Markus Armbruster <armbru@redhat.com>

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

[Bandan Das]

There's no indication of any sort that i440fx doesn't support
"iommu=on"

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Bandan Das <bsd@redhat.com>
---
 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,
-- 
2.5.0

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

[Markus Armbruster]

Bandan Das <bsd@redhat.com> writes:

> There's no indication of any sort that i440fx doesn't support
> "iommu=on"
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Bandan Das <bsd@redhat.com>
> ---
>  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).

Most machines don't support it.  You add a warning to one of them.
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.

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

[Bandan Das]

Markus Armbruster <armbru@redhat.com> writes:

> Bandan Das <bsd@redhat.com> writes:
>
>> There's no indication of any sort that i440fx doesn't support
>> "iommu=on"
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> Signed-off-by: Bandan Das <bsd@redhat.com>
>> ---
>>  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).
>
> 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). 

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.

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

[Markus Armbruster]

Bandan Das <bsd@redhat.com> writes:

> Markus Armbruster <armbru@redhat.com> writes:
>
>> Bandan Das <bsd@redhat.com> writes:
>>
>>> There's no indication of any sort that i440fx doesn't support
>>> "iommu=on"
>>>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>> Signed-off-by: Bandan Das <bsd@redhat.com>
>>> ---
>>>  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.