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

[Cornelia Huck]

On Fri, 18 Oct 2019 08:35:17 +0200
Thomas Huth <address@hidden> 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 <address@hidden> 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 <address@hidden>
> >>>> ---
> >>>>   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.