[PATCH] hw/s390x: Emit a warning if user tried to enable USB

Thomas Huth posted 1 patch 4 years, 8 months ago
Test FreeBSD passed
Test asan passed
Test checkpatch passed
Test docker-mingw@fedora passed
Test docker-quick@centos7 passed
Test docker-clang@ubuntu passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20191017142123.1236-1-thuth@redhat.com
Maintainers: Cornelia Huck <cohuck@redhat.com>, Halil Pasic <pasic@linux.ibm.com>, Christian Borntraeger <borntraeger@de.ibm.com>, Richard Henderson <rth@twiddle.net>, David Hildenbrand <david@redhat.com>
hw/s390x/s390-virtio-ccw.c | 4 ++++
1 file changed, 4 insertions(+)
[PATCH] hw/s390x: Emit a warning if user tried to enable USB
Posted by Thomas Huth 4 years, 8 months ago
There is no USB on s390x, so running qemu-system-s390x with
"-machine ...,usb=on" is certainly wrong. Emit a warning to make
the users aware of their misconfiguration.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 After a year or two, we could finally turn this into a hard error,
 but I think we should give the users some time to fix their command
 lines first, so I'm initially only emitting a warning here.

 hw/s390x/s390-virtio-ccw.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index d3edeef0ad..af8c4c0daf 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -243,6 +243,10 @@ static void ccw_init(MachineState *machine)
     VirtualCssBus *css_bus;
     DeviceState *dev;
 
+    if (machine->usb) {
+        warn_report("This machine does not support USB");
+    }
+
     s390_sclp_init();
     /* init memory + setup max page size. Required for the CPU model */
     s390_memory_init(machine->ram_size);
-- 
2.18.1


Re: [PATCH] hw/s390x: Emit a warning if user tried to enable USB
Posted by Christian Borntraeger 4 years, 8 months ago

On 17.10.19 16:21, Thomas Huth wrote:
> There is no USB on s390x, so running qemu-system-s390x with
> "-machine ...,usb=on" is certainly wrong. Emit a warning to make
> the users aware of their misconfiguration.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  After a year or two, we could finally turn this into a hard error,
>  but I think we should give the users some time to fix their command
>  lines first, so I'm initially only emitting a warning here.

I think a warn message is ok, but we should never make  this a hard
error.

I am pretty sure that there are some tools in the wild that create xmls
or qemu commands lines cross-platform and deploy those  dynamically.
These tools have probably been fixed to work good enough with s390x
but nobody with qemu clue has ever looked at these command lines. And
I am pretty sure that no user will actually see the command like nor
the error message.

So this warning will stay unnoticed until we make this a hard error. And
then we have broken a previously working setup.

In other words, I appreciate the willingness to detect mis-uses but I 
fear that we will never be able to assume that everything is fixed.



> 
>  hw/s390x/s390-virtio-ccw.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index d3edeef0ad..af8c4c0daf 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -243,6 +243,10 @@ static void ccw_init(MachineState *machine)
>      VirtualCssBus *css_bus;
>      DeviceState *dev;
>  
> +    if (machine->usb) {
> +        warn_report("This machine does not support USB");
> +    }
> +
>      s390_sclp_init();
>      /* init memory + setup max page size. Required for the CPU model */
>      s390_memory_init(machine->ram_size);
> 


Re: [PATCH] hw/s390x: Emit a warning if user tried to enable USB
Posted by Eric Blake 4 years, 8 months ago
On 10/17/19 9:45 AM, Christian Borntraeger wrote:
> 
> 
> On 17.10.19 16:21, Thomas Huth wrote:
>> There is no USB on s390x, so running qemu-system-s390x with
>> "-machine ...,usb=on" is certainly wrong. Emit a warning to make
>> the users aware of their misconfiguration.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   After a year or two, we could finally turn this into a hard error,
>>   but I think we should give the users some time to fix their command
>>   lines first, so I'm initially only emitting a warning here.
> 
> I think a warn message is ok, but we should never make  this a hard
> error.
> 
> I am pretty sure that there are some tools in the wild that create xmls
> or qemu commands lines cross-platform and deploy those  dynamically.
> These tools have probably been fixed to work good enough with s390x
> but nobody with qemu clue has ever looked at these command lines. And
> I am pretty sure that no user will actually see the command like nor
> the error message.
> 
> So this warning will stay unnoticed until we make this a hard error. And
> then we have broken a previously working setup.
> 
> In other words, I appreciate the willingness to detect mis-uses but I
> fear that we will never be able to assume that everything is fixed.

That's what the deprecation process is for. This patch is incomplete 
unless it also touches qemu-deprecated.texi.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH] hw/s390x: Emit a warning if user tried to enable USB
Posted by Cornelia Huck 4 years, 8 months ago
On Thu, 17 Oct 2019 16:21:23 +0200
Thomas Huth <thuth@redhat.com> wrote:

> There is no USB on s390x, so running qemu-system-s390x with
> "-machine ...,usb=on" is certainly wrong. Emit a warning to make
> the users aware of their misconfiguration.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  After a year or two, we could finally turn this into a hard error,
>  but I think we should give the users some time to fix their command
>  lines first, so I'm initially only emitting a warning here.
> 
>  hw/s390x/s390-virtio-ccw.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index d3edeef0ad..af8c4c0daf 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -243,6 +243,10 @@ static void ccw_init(MachineState *machine)
>      VirtualCssBus *css_bus;
>      DeviceState *dev;
>  
> +    if (machine->usb) {
> +        warn_report("This machine does not support USB");

I'm wondering if this is the only machine type not supporting usb...
if not, how are others handling it?

The usb parsing code in machine.c does not care if usb is even
configured (CONFIG_USB). There's other stuff in there like
igd-passthru, which seems to be x86 specific; probably historical
reasons?

> +    }
> +
>      s390_sclp_init();
>      /* init memory + setup max page size. Required for the CPU model */
>      s390_memory_init(machine->ram_size);


Re: [PATCH] hw/s390x: Emit a warning if user tried to enable USB
Posted by Thomas Huth 4 years, 8 months ago
On 17/10/2019 16.34, Cornelia Huck wrote:
> On Thu, 17 Oct 2019 16:21:23 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> There is no USB on s390x, so running qemu-system-s390x with
>> "-machine ...,usb=on" is certainly wrong. Emit a warning to make
>> the users aware of their misconfiguration.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  After a year or two, we could finally turn this into a hard error,
>>  but I think we should give the users some time to fix their command
>>  lines first, so I'm initially only emitting a warning here.
>>
>>  hw/s390x/s390-virtio-ccw.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index d3edeef0ad..af8c4c0daf 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -243,6 +243,10 @@ static void ccw_init(MachineState *machine)
>>      VirtualCssBus *css_bus;
>>      DeviceState *dev;
>>  
>> +    if (machine->usb) {
>> +        warn_report("This machine does not support USB");
> 
> I'm wondering if this is the only machine type not supporting usb...
> if not, how are others handling it?

I think most machines are silently ignoring it, like we did on s390x
until now, too.

> The usb parsing code in machine.c does not care if usb is even
> configured (CONFIG_USB).

machine.c is common code, so you can not use CONFIG_USB there.

> There's other stuff in there like
> igd-passthru, which seems to be x86 specific; probably historical
> reasons?

IMHO igd-passthru should be moved to the xen machine, which seems to be
the only user.

 Thomas


Re: [PATCH] hw/s390x: Emit a warning if user tried to enable USB
Posted by Cornelia Huck 4 years, 8 months ago
On Thu, 17 Oct 2019 16:40:56 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 17/10/2019 16.34, Cornelia Huck wrote:
> > On Thu, 17 Oct 2019 16:21:23 +0200
> > Thomas Huth <thuth@redhat.com> wrote:
> >   
> >> There is no USB on s390x, so running qemu-system-s390x with
> >> "-machine ...,usb=on" is certainly wrong. Emit a warning to make
> >> the users aware of their misconfiguration.
> >>
> >> Signed-off-by: Thomas Huth <thuth@redhat.com>
> >> ---
> >>  After a year or two, we could finally turn this into a hard error,
> >>  but I think we should give the users some time to fix their command
> >>  lines first, so I'm initially only emitting a warning here.
> >>
> >>  hw/s390x/s390-virtio-ccw.c | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> >> index d3edeef0ad..af8c4c0daf 100644
> >> --- a/hw/s390x/s390-virtio-ccw.c
> >> +++ b/hw/s390x/s390-virtio-ccw.c
> >> @@ -243,6 +243,10 @@ static void ccw_init(MachineState *machine)
> >>      VirtualCssBus *css_bus;
> >>      DeviceState *dev;
> >>  
> >> +    if (machine->usb) {
> >> +        warn_report("This machine does not support USB");  
> > 
> > I'm wondering if this is the only machine type not supporting usb...
> > if not, how are others handling it?  
> 
> I think most machines are silently ignoring it, like we did on s390x
> until now, too.

I'm wondering how many options we have that do nothing when configured
:)

> 
> > The usb parsing code in machine.c does not care if usb is even
> > configured (CONFIG_USB).  
> 
> machine.c is common code, so you can not use CONFIG_USB there.

Hm, yes.

> 
> > There's other stuff in there like
> > igd-passthru, which seems to be x86 specific; probably historical
> > reasons?  
> 
> IMHO igd-passthru should be moved to the xen machine, which seems to be
> the only user.

OTOH, I can currently specify igd-passthru=on on any machine, which
would break if we moved it. Not that it makes any sense...

Re: [PATCH] hw/s390x: Emit a warning if user tried to enable USB
Posted by Markus Armbruster 4 years, 8 months ago
Cornelia Huck <cohuck@redhat.com> writes:

> On Thu, 17 Oct 2019 16:40:56 +0200
> Thomas Huth <thuth@redhat.com> wrote:
>
>> On 17/10/2019 16.34, Cornelia Huck wrote:
>> > On Thu, 17 Oct 2019 16:21:23 +0200
>> > Thomas Huth <thuth@redhat.com> wrote:
>> >   
>> >> There is no USB on s390x, so running qemu-system-s390x with
>> >> "-machine ...,usb=on" is certainly wrong. Emit a warning to make
>> >> the users aware of their misconfiguration.
>> >>
>> >> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> >> ---
>> >>  After a year or two, we could finally turn this into a hard error,
>> >>  but I think we should give the users some time to fix their command
>> >>  lines first, so I'm initially only emitting a warning here.
>> >>
>> >>  hw/s390x/s390-virtio-ccw.c | 4 ++++
>> >>  1 file changed, 4 insertions(+)
>> >>
>> >> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> >> index d3edeef0ad..af8c4c0daf 100644
>> >> --- a/hw/s390x/s390-virtio-ccw.c
>> >> +++ b/hw/s390x/s390-virtio-ccw.c
>> >> @@ -243,6 +243,10 @@ static void ccw_init(MachineState *machine)
>> >>      VirtualCssBus *css_bus;
>> >>      DeviceState *dev;
>> >>  
>> >> +    if (machine->usb) {
>> >> +        warn_report("This machine does not support USB");  
>> > 
>> > I'm wondering if this is the only machine type not supporting usb...
>> > if not, how are others handling it?  
>> 
>> I think most machines are silently ignoring it, like we did on s390x
>> until now, too.
>
> I'm wondering how many options we have that do nothing when configured
> :)

Plenty.

-machine usb=on (and its sugared form -usb) are just one of many options
that deposit configuration for the board to pick up.  Some boards do,
some don't.  Some boards pick up and reject some configuration they
can't use.  Some don't.  It's a big family of hack jobs.

For -drive (and its sugared forms), we have generic code to turn "not
picked up" into an error: drive_check_orphaned().
a66c9dc734 "blockdev: Orphaned drive search", 2014-10-03
720b8dc052 "blockdev: Make orphaned -drive fatal", 2017-02-21

[...]

Re: [PATCH] hw/s390x: Emit a warning if user tried to enable USB
Posted by Philippe Mathieu-Daudé 4 years, 8 months ago
On 10/17/19 4:40 PM, Thomas Huth wrote:
> On 17/10/2019 16.34, Cornelia Huck wrote:
>> On Thu, 17 Oct 2019 16:21:23 +0200
>> Thomas Huth <thuth@redhat.com> wrote:
>>
>>> There is no USB on s390x, so running qemu-system-s390x with
>>> "-machine ...,usb=on" is certainly wrong. Emit a warning to make
>>> the users aware of their misconfiguration.
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>   After a year or two, we could finally turn this into a hard error,
>>>   but I think we should give the users some time to fix their command
>>>   lines first, so I'm initially only emitting a warning here.
>>>
>>>   hw/s390x/s390-virtio-ccw.c | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>> index d3edeef0ad..af8c4c0daf 100644
>>> --- a/hw/s390x/s390-virtio-ccw.c
>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>> @@ -243,6 +243,10 @@ static void ccw_init(MachineState *machine)
>>>       VirtualCssBus *css_bus;
>>>       DeviceState *dev;
>>>   
>>> +    if (machine->usb) {
>>> +        warn_report("This machine does not support USB");
>>
>> I'm wondering if this is the only machine type not supporting usb...
>> if not, how are others handling it?
> 
> I think most machines are silently ignoring it, like we did on s390x
> until now, too.
> 
>> The usb parsing code in machine.c does not care if usb is even
>> configured (CONFIG_USB).
> 
> machine.c is common code, so you can not use CONFIG_USB there.

We already have:

bool target_words_bigendian(void)
{
#if defined(TARGET_WORDS_BIGENDIAN)
     return true;
#else
     return false;
#endif
}

What about something such:

-- >8 --
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 1689ad3bf8..0c45ab042b 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -986,7 +986,7 @@ static void machine_finalize(Object *obj)

  bool machine_usb(MachineState *machine)
  {
-    return machine->usb;
+    return machine_has_usb() && machine->usb;
  }

  bool machine_kernel_irqchip_allowed(MachineState *machine)
diff --git a/hw/usb/Makefile.objs b/hw/usb/Makefile.objs
index 303ac084a0..ac545cdd2e 100644
--- a/hw/usb/Makefile.objs
+++ b/hw/usb/Makefile.objs
@@ -1,6 +1,7 @@
  # usb subsystem core
  common-obj-y += core.o combined-packet.o bus.o libhw.o
  common-obj-$(CONFIG_USB) += desc.o desc-msos.o
+obj-y += machine.o

  # usb host adapters
  common-obj-$(CONFIG_USB_UHCI) += hcd-uhci.o
diff --git a/hw/usb/machine.c b/hw/usb/machine.c
new file mode 100644
index 0000000000..5381928479
--- /dev/null
+++ b/hw/usb/machine.c
@@ -0,0 +1,12 @@
+#include "qemu/osdep.h"
+#include "hw/boards.h"
+#include "config-devices.h"
+
+bool machine_has_usb(void)
+{
+#if defined(CONFIG_USB)
+    return true;
+#else
+    return false;
+#endif
+}
diff --git a/include/hw/boards.h b/include/hw/boards.h
index be18a5c032..e4518b73b1 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -63,6 +63,7 @@ extern MachineState *current_machine;

  void machine_run_board_init(MachineState *machine);
  bool machine_usb(MachineState *machine);
+bool machine_has_usb(void); /* or target_has_usb()? */
  bool machine_kernel_irqchip_allowed(MachineState *machine);
  bool machine_kernel_irqchip_required(MachineState *machine);
  bool machine_kernel_irqchip_split(MachineState *machine);

---

Re: [PATCH] hw/s390x: Emit a warning if user tried to enable USB
Posted by Thomas Huth 4 years, 8 months ago
On 17/10/2019 20.18, Philippe Mathieu-Daudé wrote:
> On 10/17/19 4:40 PM, Thomas Huth wrote:
>> On 17/10/2019 16.34, Cornelia Huck wrote:
>>> On Thu, 17 Oct 2019 16:21:23 +0200
>>> Thomas Huth <thuth@redhat.com> wrote:
>>>
>>>> There is no USB on s390x, so running qemu-system-s390x with
>>>> "-machine ...,usb=on" is certainly wrong. Emit a warning to make
>>>> the users aware of their misconfiguration.
>>>>
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>>   After a year or two, we could finally turn this into a hard error,
>>>>   but I think we should give the users some time to fix their command
>>>>   lines first, so I'm initially only emitting a warning here.
>>>>
>>>>   hw/s390x/s390-virtio-ccw.c | 4 ++++
>>>>   1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>>> index d3edeef0ad..af8c4c0daf 100644
>>>> --- a/hw/s390x/s390-virtio-ccw.c
>>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>>> @@ -243,6 +243,10 @@ static void ccw_init(MachineState *machine)
>>>>       VirtualCssBus *css_bus;
>>>>       DeviceState *dev;
>>>>   +    if (machine->usb) {
>>>> +        warn_report("This machine does not support USB");
>>>
>>> I'm wondering if this is the only machine type not supporting usb...
>>> if not, how are others handling it?
>>
>> I think most machines are silently ignoring it, like we did on s390x
>> until now, too.
>>
>>> The usb parsing code in machine.c does not care if usb is even
>>> configured (CONFIG_USB).
>>
>> machine.c is common code, so you can not use CONFIG_USB there.
> 
> We already have:
> 
> bool target_words_bigendian(void)
> {
> #if defined(TARGET_WORDS_BIGENDIAN)
>     return true;
> #else
>     return false;
> #endif
> }

... and kvm_available() and xen_available() ...

> diff --git a/hw/usb/machine.c b/hw/usb/machine.c
> new file mode 100644
> index 0000000000..5381928479
> --- /dev/null
> +++ b/hw/usb/machine.c
> @@ -0,0 +1,12 @@
> +#include "qemu/osdep.h"
> +#include "hw/boards.h"
> +#include "config-devices.h"
> +
> +bool machine_has_usb(void)
> +{
> +#if defined(CONFIG_USB)
> +    return true;
> +#else
> +    return false;
> +#endif
> +}

I think I'd rather call it usb_available() (like the other _available()
functions) and put it into arch_init.c (and rename that file to arch.c
or target.c or something like that).

 Thomas

Re: [PATCH] hw/s390x: Emit a warning if user tried to enable USB
Posted by Philippe Mathieu-Daudé 4 years, 8 months ago
On 10/18/19 8:35 AM, Thomas Huth wrote:
> On 17/10/2019 20.18, Philippe Mathieu-Daudé wrote:
>> On 10/17/19 4:40 PM, Thomas Huth wrote:
>>> On 17/10/2019 16.34, Cornelia Huck wrote:
>>>> On Thu, 17 Oct 2019 16:21:23 +0200
>>>> Thomas Huth <thuth@redhat.com> wrote:
>>>>
>>>>> There is no USB on s390x, so running qemu-system-s390x with
>>>>> "-machine ...,usb=on" is certainly wrong. Emit a warning to make
>>>>> the users aware of their misconfiguration.
>>>>>
>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>>> ---
>>>>>    After a year or two, we could finally turn this into a hard error,
>>>>>    but I think we should give the users some time to fix their command
>>>>>    lines first, so I'm initially only emitting a warning here.
>>>>>
>>>>>    hw/s390x/s390-virtio-ccw.c | 4 ++++
>>>>>    1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>>>> index d3edeef0ad..af8c4c0daf 100644
>>>>> --- a/hw/s390x/s390-virtio-ccw.c
>>>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>>>> @@ -243,6 +243,10 @@ static void ccw_init(MachineState *machine)
>>>>>        VirtualCssBus *css_bus;
>>>>>        DeviceState *dev;
>>>>>    +    if (machine->usb) {
>>>>> +        warn_report("This machine does not support USB");
>>>>
>>>> I'm wondering if this is the only machine type not supporting usb...
>>>> if not, how are others handling it?
>>>
>>> I think most machines are silently ignoring it, like we did on s390x
>>> until now, too.
>>>
>>>> The usb parsing code in machine.c does not care if usb is even
>>>> configured (CONFIG_USB).
>>>
>>> machine.c is common code, so you can not use CONFIG_USB there.
>>
>> We already have:
>>
>> bool target_words_bigendian(void)
>> {
>> #if defined(TARGET_WORDS_BIGENDIAN)
>>      return true;
>> #else
>>      return false;
>> #endif
>> }
> 
> ... and kvm_available() and xen_available() ...
> 
>> diff --git a/hw/usb/machine.c b/hw/usb/machine.c
>> new file mode 100644
>> index 0000000000..5381928479
>> --- /dev/null
>> +++ b/hw/usb/machine.c
>> @@ -0,0 +1,12 @@
>> +#include "qemu/osdep.h"
>> +#include "hw/boards.h"
>> +#include "config-devices.h"
>> +
>> +bool machine_has_usb(void)
>> +{
>> +#if defined(CONFIG_USB)
>> +    return true;
>> +#else
>> +    return false;
>> +#endif
>> +}
> 
> I think I'd rather call it usb_available() (like the other _available()
> functions) and put it into arch_init.c (and rename that file to arch.c
> or target.c or something like that).

Yes, clever names :)

Re: [PATCH] hw/s390x: Emit a warning if user tried to enable USB
Posted by Cornelia Huck 4 years, 8 months ago
On Fri, 18 Oct 2019 08:35:17 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 17/10/2019 20.18, Philippe Mathieu-Daudé wrote:
> > On 10/17/19 4:40 PM, Thomas Huth wrote:  
> >> On 17/10/2019 16.34, Cornelia Huck wrote:  
> >>> On Thu, 17 Oct 2019 16:21:23 +0200
> >>> Thomas Huth <thuth@redhat.com> wrote:
> >>>  
> >>>> There is no USB on s390x, so running qemu-system-s390x with
> >>>> "-machine ...,usb=on" is certainly wrong. Emit a warning to make
> >>>> the users aware of their misconfiguration.
> >>>>
> >>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
> >>>> ---
> >>>>   After a year or two, we could finally turn this into a hard error,
> >>>>   but I think we should give the users some time to fix their command
> >>>>   lines first, so I'm initially only emitting a warning here.
> >>>>
> >>>>   hw/s390x/s390-virtio-ccw.c | 4 ++++
> >>>>   1 file changed, 4 insertions(+)
> >>>>
> >>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> >>>> index d3edeef0ad..af8c4c0daf 100644
> >>>> --- a/hw/s390x/s390-virtio-ccw.c
> >>>> +++ b/hw/s390x/s390-virtio-ccw.c
> >>>> @@ -243,6 +243,10 @@ static void ccw_init(MachineState *machine)
> >>>>       VirtualCssBus *css_bus;
> >>>>       DeviceState *dev;
> >>>>   +    if (machine->usb) {
> >>>> +        warn_report("This machine does not support USB");  
> >>>
> >>> I'm wondering if this is the only machine type not supporting usb...
> >>> if not, how are others handling it?  
> >>
> >> I think most machines are silently ignoring it, like we did on s390x
> >> until now, too.
> >>  
> >>> The usb parsing code in machine.c does not care if usb is even
> >>> configured (CONFIG_USB).  
> >>
> >> machine.c is common code, so you can not use CONFIG_USB there.  
> > 
> > We already have:
> > 
> > bool target_words_bigendian(void)
> > {
> > #if defined(TARGET_WORDS_BIGENDIAN)
> >     return true;
> > #else
> >     return false;
> > #endif
> > }  
> 
> ... and kvm_available() and xen_available() ...
> 
> > diff --git a/hw/usb/machine.c b/hw/usb/machine.c
> > new file mode 100644
> > index 0000000000..5381928479
> > --- /dev/null
> > +++ b/hw/usb/machine.c
> > @@ -0,0 +1,12 @@
> > +#include "qemu/osdep.h"
> > +#include "hw/boards.h"
> > +#include "config-devices.h"
> > +
> > +bool machine_has_usb(void)
> > +{
> > +#if defined(CONFIG_USB)
> > +    return true;
> > +#else
> > +    return false;
> > +#endif
> > +}  
> 
> I think I'd rather call it usb_available() (like the other _available()
> functions) and put it into arch_init.c (and rename that file to arch.c
> or target.c or something like that).

I like 'usb_available()'.

Maybe we should also warn for igd_passthru if not xen_available()? Not
sure how helpful that is, though. Even if we warn and put it in the
deprecation notes, I'm not sure how much we'd gain if we were to make
it an actual error.