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

[Philippe Mathieu-Daudé]

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 <th...@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 <th...@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);

---