Patchwork [Qemu-ppc,v5,1/4] Add usb option in machine options

login
register
mail settings
Submitter zhlcindy@gmail.com
Date July 2, 2012, 5:25 a.m.
Message ID <b0d3ab48c19780a6861dd9677412ba279807da57.1341204647.git.zhlcindy@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/168493/
State New
Headers show

Comments

zhlcindy@gmail.com - July 2, 2012, 5:25 a.m.
From: Li Zhang <zhlcindy@linux.vnet.ibm.com>

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 <zhlcindy@linux.vnet.ibm.com>
Reviewed-by: Andreas Färber <afaerber@suse.de>
---
 hw/spapr.c    |   11 +++++++++++
 qemu-config.c |    4 ++++
 2 files changed, 15 insertions(+)
Alexander Graf - July 6, 2012, 1:43 p.m.
On 02.07.2012, at 07:25, zhlcindy@gmail.com wrote:

> From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
> 
> 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 <zhlcindy@linux.vnet.ibm.com>
> Reviewed-by: Andreas Färber <afaerber@suse.de>

Shouldn't you also convert -usb over to the new syntax, so that -usb basically is the same as -machine x,usb=on?


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.


Alex
Andreas Färber - July 6, 2012, 3:58 p.m.
Am 06.07.2012 15:43, schrieb Alexander Graf:
> 
> On 02.07.2012, at 07:25, zhlcindy@gmail.com wrote:
> 
>> From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>>
>> 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 <zhlcindy@linux.vnet.ibm.com>
>> Reviewed-by: Andreas Färber <afaerber@suse.de>
>> ---
>> 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.

Andreas
Alexander Graf - July 6, 2012, 4:03 p.m.
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, zhlcindy@gmail.com wrote:
>> 
>>> From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>>> 
>>> 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 <zhlcindy@linux.vnet.ibm.com>
>>> Reviewed-by: Andreas Färber <afaerber@suse.de>
>>> ---
>>> 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.


Alex
Andreas Färber - July 6, 2012, 4:08 p.m.
Am 06.07.2012 18:03, schrieb Alexander Graf:
> 
> 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, zhlcindy@gmail.com wrote:
>>>
>>>> From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>>>>
>>>> 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 <zhlcindy@linux.vnet.ibm.com>
>>>> Reviewed-by: Andreas Färber <afaerber@suse.de>
>>>> ---
>>>> 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.

Oh sure, if we return bool and keep QemuOpts* locally it's perfect. :)

I understood you wanted to generalize the if (add_usb) {} that you
replied under. ;)

Andreas
zhlcindy@gmail.com - July 8, 2012, 2:21 p.m.
On Fri, Jul 6, 2012 at 9:43 PM, Alexander Graf <agraf@suse.de> wrote:
>
> On 02.07.2012, at 07:25, zhlcindy@gmail.com wrote:
>
>> From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>>
>> 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 <zhlcindy@linux.vnet.ibm.com>
>> Reviewed-by: Andreas Färber <afaerber@suse.de>
>
> 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. :)

>
> 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
>
Alexander Graf - July 8, 2012, 2:45 p.m.
On 08.07.2012, at 16:21, Li Zhang <zhlcindy@gmail.com> wrote:

> On Fri, Jul 6, 2012 at 9:43 PM, Alexander Graf <agraf@suse.de> wrote:
>> 
>> On 02.07.2012, at 07:25, zhlcindy@gmail.com wrote:
>> 
>>> From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>>> 
>>> 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 <zhlcindy@linux.vnet.ibm.com>
>>> Reviewed-by: Andreas Färber <afaerber@suse.de>
>> 
>> 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
zhlcindy@gmail.com - July 8, 2012, 2:46 p.m.
On Sat, Jul 7, 2012 at 12:03 AM, Alexander Graf <agraf@suse.de> 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, zhlcindy@gmail.com wrote:
>>>
>>>> From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>>>>
>>>> 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 <zhlcindy@linux.vnet.ibm.com>
>>>> Reviewed-by: Andreas Färber <afaerber@suse.de>
>>>> ---
>>>> 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 think that's great, I will do that.

Thanks for your suggestions.

>
> Alex
>
zhlcindy@gmail.com - July 8, 2012, 2:52 p.m.
On Sun, Jul 8, 2012 at 10:45 PM, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 08.07.2012, at 16:21, Li Zhang <zhlcindy@gmail.com> wrote:
>
>> On Fri, Jul 6, 2012 at 9:43 PM, Alexander Graf <agraf@suse.de> wrote:
>>>
>>> On 02.07.2012, at 07:25, zhlcindy@gmail.com wrote:
>>>
>>>> From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>>>>
>>>> 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 <zhlcindy@linux.vnet.ibm.com>
>>>> Reviewed-by: Andreas Färber <afaerber@suse.de>
>>>
>>> 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.
>
Got it. I  will change it.
>
> 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
Alexander Graf - July 8, 2012, 3:02 p.m.
On 08.07.2012, at 16:46, Li Zhang <zhlcindy@gmail.com> wrote:

> On Sat, Jul 7, 2012 at 12:03 AM, Alexander Graf <agraf@suse.de> 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, zhlcindy@gmail.com wrote:
>>>> 
>>>>> From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>>>>> 
>>>>> 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 <zhlcindy@linux.vnet.ibm.com>
>>>>> Reviewed-by: Andreas Färber <afaerber@suse.de>
>>>>> ---
>>>>> 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.


Alex
zhlcindy@gmail.com - July 8, 2012, 3:15 p.m.
On Sun, Jul 8, 2012 at 11:02 PM, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 08.07.2012, at 16:46, Li Zhang <zhlcindy@gmail.com> wrote:
>
>> On Sat, Jul 7, 2012 at 12:03 AM, Alexander Graf <agraf@suse.de> 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, zhlcindy@gmail.com wrote:
>>>>>
>>>>>> From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>>>>>>
>>>>>> 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 <zhlcindy@linux.vnet.ibm.com>
>>>>>> Reviewed-by: Andreas Färber <afaerber@suse.de>
>>>>>> ---
>>>>>> 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
>

Patch

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");
+    }
     if (rma_size < (MIN_RMA_SLOF << 20)) {
         fprintf(stderr, "qemu: pSeries SLOF firmware requires >= "
                 "%ldM guest RMA (Real Mode Area memory)\n", MIN_RMA_SLOF);
diff --git a/qemu-config.c b/qemu-config.c
index 5c3296b..b86ee36 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -595,6 +595,10 @@  static QemuOptsList qemu_machine_opts = {
             .name = "dt_compatible",
             .type = QEMU_OPT_STRING,
             .help = "Overrides the \"compatible\" property of the dt root node",
+        },{
+            .name = "usb",
+            .type = QEMU_OPT_BOOL,
+            .help = "Set on/off to enable/disable usb",
         },
         { /* End of list */ }
     },