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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

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