All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] hw/s390x: Emit a warning if user tried to enable USB
@ 2019-10-17 14:21 Thomas Huth
  2019-10-17 14:34 ` Cornelia Huck
  2019-10-17 14:45 ` Christian Borntraeger
  0 siblings, 2 replies; 11+ messages in thread
From: Thomas Huth @ 2019-10-17 14:21 UTC (permalink / raw)
  To: Cornelia Huck, qemu-s390x; +Cc: Halil Pasic, Christian Borntraeger, qemu-devel

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



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] hw/s390x: Emit a warning if user tried to enable USB
  2019-10-17 14:21 [PATCH] hw/s390x: Emit a warning if user tried to enable USB Thomas Huth
@ 2019-10-17 14:34 ` Cornelia Huck
  2019-10-17 14:40   ` Thomas Huth
  2019-10-17 14:45 ` Christian Borntraeger
  1 sibling, 1 reply; 11+ messages in thread
From: Cornelia Huck @ 2019-10-17 14:34 UTC (permalink / raw)
  To: Thomas Huth; +Cc: Halil Pasic, Christian Borntraeger, qemu-s390x, qemu-devel

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);



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] hw/s390x: Emit a warning if user tried to enable USB
  2019-10-17 14:34 ` Cornelia Huck
@ 2019-10-17 14:40   ` Thomas Huth
  2019-10-17 15:29     ` Cornelia Huck
  2019-10-17 18:18     ` Philippe Mathieu-Daudé
  0 siblings, 2 replies; 11+ messages in thread
From: Thomas Huth @ 2019-10-17 14:40 UTC (permalink / raw)
  To: Cornelia Huck; +Cc: Halil Pasic, Christian Borntraeger, qemu-s390x, qemu-devel

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



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] hw/s390x: Emit a warning if user tried to enable USB
  2019-10-17 14:21 [PATCH] hw/s390x: Emit a warning if user tried to enable USB Thomas Huth
  2019-10-17 14:34 ` Cornelia Huck
@ 2019-10-17 14:45 ` Christian Borntraeger
  2019-10-17 16:14   ` Eric Blake
  1 sibling, 1 reply; 11+ messages in thread
From: Christian Borntraeger @ 2019-10-17 14:45 UTC (permalink / raw)
  To: Thomas Huth, Cornelia Huck, qemu-s390x; +Cc: Halil Pasic, qemu-devel



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);
> 



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] hw/s390x: Emit a warning if user tried to enable USB
  2019-10-17 14:40   ` Thomas Huth
@ 2019-10-17 15:29     ` Cornelia Huck
  2019-10-18  5:20       ` Markus Armbruster
  2019-10-17 18:18     ` Philippe Mathieu-Daudé
  1 sibling, 1 reply; 11+ messages in thread
From: Cornelia Huck @ 2019-10-17 15:29 UTC (permalink / raw)
  To: Thomas Huth; +Cc: Halil Pasic, Christian Borntraeger, qemu-s390x, qemu-devel

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...


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] hw/s390x: Emit a warning if user tried to enable USB
  2019-10-17 14:45 ` Christian Borntraeger
@ 2019-10-17 16:14   ` Eric Blake
  0 siblings, 0 replies; 11+ messages in thread
From: Eric Blake @ 2019-10-17 16:14 UTC (permalink / raw)
  To: Christian Borntraeger, Thomas Huth, Cornelia Huck, qemu-s390x
  Cc: Halil Pasic, qemu-devel

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] hw/s390x: Emit a warning if user tried to enable USB
  2019-10-17 14:40   ` Thomas Huth
  2019-10-17 15:29     ` Cornelia Huck
@ 2019-10-17 18:18     ` Philippe Mathieu-Daudé
  2019-10-18  6:35       ` Thomas Huth
  1 sibling, 1 reply; 11+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-10-17 18:18 UTC (permalink / raw)
  To: Thomas Huth, Cornelia Huck
  Cc: Halil Pasic, Christian Borntraeger, qemu-s390x, qemu-devel

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);

---


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] hw/s390x: Emit a warning if user tried to enable USB
  2019-10-17 15:29     ` Cornelia Huck
@ 2019-10-18  5:20       ` Markus Armbruster
  0 siblings, 0 replies; 11+ messages in thread
From: Markus Armbruster @ 2019-10-18  5:20 UTC (permalink / raw)
  To: Cornelia Huck
  Cc: Halil Pasic, Christian Borntraeger, Thomas Huth, qemu-s390x, qemu-devel

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

[...]


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] hw/s390x: Emit a warning if user tried to enable USB
  2019-10-17 18:18     ` Philippe Mathieu-Daudé
@ 2019-10-18  6:35       ` Thomas Huth
  2019-10-18  7:37         ` Philippe Mathieu-Daudé
  2019-10-18  8:41         ` Cornelia Huck
  0 siblings, 2 replies; 11+ messages in thread
From: Thomas Huth @ 2019-10-18  6:35 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, Cornelia Huck
  Cc: Halil Pasic, Christian Borntraeger, qemu-s390x, qemu-devel

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] hw/s390x: Emit a warning if user tried to enable USB
  2019-10-18  6:35       ` Thomas Huth
@ 2019-10-18  7:37         ` Philippe Mathieu-Daudé
  2019-10-18  8:41         ` Cornelia Huck
  1 sibling, 0 replies; 11+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-10-18  7:37 UTC (permalink / raw)
  To: Thomas Huth, Cornelia Huck
  Cc: Halil Pasic, Christian Borntraeger, qemu-s390x, qemu-devel

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 :)


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] hw/s390x: Emit a warning if user tried to enable USB
  2019-10-18  6:35       ` Thomas Huth
  2019-10-18  7:37         ` Philippe Mathieu-Daudé
@ 2019-10-18  8:41         ` Cornelia Huck
  1 sibling, 0 replies; 11+ messages in thread
From: Cornelia Huck @ 2019-10-18  8:41 UTC (permalink / raw)
  To: Thomas Huth
  Cc: Halil Pasic, Christian Borntraeger, qemu-s390x,
	Philippe Mathieu-Daudé,
	qemu-devel

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.


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2019-10-18  8:42 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-17 14:21 [PATCH] hw/s390x: Emit a warning if user tried to enable USB Thomas Huth
2019-10-17 14:34 ` Cornelia Huck
2019-10-17 14:40   ` Thomas Huth
2019-10-17 15:29     ` Cornelia Huck
2019-10-18  5:20       ` Markus Armbruster
2019-10-17 18:18     ` Philippe Mathieu-Daudé
2019-10-18  6:35       ` Thomas Huth
2019-10-18  7:37         ` Philippe Mathieu-Daudé
2019-10-18  8:41         ` Cornelia Huck
2019-10-17 14:45 ` Christian Borntraeger
2019-10-17 16:14   ` Eric Blake

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.