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

[Thomas Huth]

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

 Thomas