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

[Alexander Graf]

On 08.07.2012, at 16:21, Li Zhang <address@hidden> wrote:

> On Fri, Jul 6, 2012 at 9:43 PM, Alexander Graf <address@hidden> wrote:
>> 
>> 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>
>> 
>> Shouldn't you also convert -usb over to the new syntax, so that -usb 
>> basically is the same as -machine x,usb=on?
>> 
>   Assumely, -usb and -machine x, usb=on shouldn't be used at the same time.
>   When using -usb, and -machine x, usb=off, it's a little like to
> disable the default USB of machine,
>   and use another specific USB with -usb option.
> 
>   That's what my understanding. :)

It's not about passing them in at the same time, but about having a single 
semantic instance where "Do we expose USB?" gets stored. That makes the code 
easier.

Also if you change things the way I suggested, every platform will be able to 
use -usb and -machine usb=x. So things become a lot more logical from the 
command line perspective as well, since a management app can always pass in 
-machine usb=x and have the same semantics, regardless of the machine we're 
running on.


Alex

> 
>> 
>> Alex
>> 
>>> ---
>>> 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
>>> @@ -575,6 +575,8 @@ static void ppc_spapr_init(ram_addr_t ram_size,
>>>    long load_limit, rtas_limit, fw_size;
>>>    long pteg_shift = 17;
>>>    char *filename;
>>> +    QemuOpts *machine_opts;
>>> +    bool add_usb = true;
>>> 
>>>    spapr = g_malloc0(sizeof(*spapr));
>>>    QLIST_INIT(&spapr->phbs);
>>> @@ -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.
>> 
>  OK. I will do that. :)
>> 
>> Alex
>> 
> 
> 
> 
> -- 
> 
> Best Regards
> -Li