hw/s390x/s390-virtio-ccw.c | 4 ++++ 1 file changed, 4 insertions(+)
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
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); >
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
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);
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
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...
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 [...]
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); ---
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
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 :)
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.
© 2016 - 2024 Red Hat, Inc.