Re: [Qemu-devel] [Qemu-ppc][PATCH v5 1/4] Add usb option in machine options

[Li Zhang]

On Sun, Jul 8, 2012 at 11:02 PM, Alexander Graf <address@hidden> wrote:
>
>
> On 08.07.2012, at 16:46, Li Zhang <address@hidden> wrote:
>
>> On Sat, Jul 7, 2012 at 12:03 AM, Alexander Graf <address@hidden> wrote:
>>>
>>> On 06.07.2012, at 17:58, Andreas Färber wrote:
>>>
>>>> Am 06.07.2012 15:43, schrieb Alexander Graf:
>>>>>
>>>>> On 02.07.2012, at 07:25, address@hidden wrote:
>>>>>
>>>>>> From: Li Zhang <address@hidden>
>>>>>>
>>>>>> pSeries machine needs to enable USB to add a USB
>>>>>> keyboard or USB mouse. -usb option won't be used in
>>>>>> the future, and machine options are a better way to
>>>>>> enable USB.
>>>>>>
>>>>>> So this patch is to add USB option to machine options
>>>>>> (-machine type=pseries,usb=on/off) to enable/disable
>>>>>> USB controller. And machines will get the machine option
>>>>>> and create a USB controller if USB is on.
>>>>>>
>>>>>> By the way, USB is on by default on pSeries machine.
>>>>>> So USB controller should be turned off explicitly through
>>>>>> the command line for "-nodefault" case as the following:
>>>>>> -machine type=pseries,usb=off.
>>>>>>
>>>>>> Signed-off-by: Li Zhang <address@hidden>
>>>>>> Reviewed-by: Andreas Färber <address@hidden>
>>>>>> ---
>>>>>> hw/spapr.c    |   11 +++++++++++
>>>>>> qemu-config.c |    4 ++++
>>>>>> 2 files changed, 15 insertions(+)
>>>>>>
>>>>>> diff --git a/hw/spapr.c b/hw/spapr.c
>>>>>> index 81c9343..973de1b 100644
>>>>>> --- a/hw/spapr.c
>>>>>> +++ b/hw/spapr.c
>>>> [...]
>>>>>> @@ -710,6 +712,15 @@ static void ppc_spapr_init(ram_addr_t ram_size,
>>>>>>       spapr_vscsi_create(spapr->vio_bus);
>>>>>>   }
>>>>>>
>>>>>> +    machine_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
>>>>>> +    if (machine_opts) {
>>>>>> +        add_usb = qemu_opt_get_bool(machine_opts, "usb", true);
>>>>>> +    }
>>>>>> +
>>>>>> +    if (add_usb) {
>>>>>> +        pci_create_simple(QLIST_FIRST(&spapr->phbs)->host_state.bus,
>>>>>> +                          -1, "pci-ohci");
>>>>>> +    }
>>>>>
>>>>> Didn't I ask you to extract this out to generic code? I don't want to 
>>>>> have a "usb" machine opt that is only available for -M pseries.
>>>>
>>>> Isn't the mapping from "usb=on" to device-level actions
>>>> machine-specific? We have ohci, uhci, ehci, xhci to choose from. And the
>>>> bus to place it on is machine-specific, too.
>>>>
>>>> So did you rather mean adding usb= awareness to more machines? If we
>>>> generalize usb=on to usb=none/ohci/... plus some usbbus= we get -device.
>>>
>>> I was thinking of replacing usb_enabled with
>>>
>>> static inline int usb_enabled(bool default) {
>>>    machine_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
>>>    if (machine_opts) {
>>>        return qemu_opt_get_bool(machine_opts, "usb", default);
>>>    }
>>>    return default;
>>> }
>>>
>>> Whereas -usb in vl.c would set machine.usb = on. Then all users of 
>>> usb_enabled would automatically be converted to the new syntax and we could 
>>> even use the same logic in spapr.c.
>>>
>>
>> I see, you mean global varible usb_enabled is still used, and when
>> -usb is passed to qemu command line,
>> use_enabled will be assigned by the above function and set machine.usb=on.
>>
>> So all the places where usb_enabled is used still can work correctly.
>
> I mean convert usb_enabled into a function that checks machine.usb :). The 
> only global state should live in machine opts.
>
Oh, got it.
So usb_enabled should be cleared up in the places where it is used. :)

>
> Alex
>



-- 

Best Regards
-Li