| Message ID | b0d3ab48c19780a6861dd9677412ba279807da57.1341204647.git.zhlcindy@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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 >
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
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 >
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
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
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 >
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 */ } },