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

[Philippe Mathieu-Daudé]

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

Yes, clever names :)