| Message ID | CAD8of+qG=WgPVqQgzjhmj=mnWaBFqfsop+Vas7fvJ2XqK0w15Q@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
Hi Li Zhang, Perhaps you miss "[PATCH v3 1/2]" in the subject? Regards, chenwj
Hi Andreas, I send out this patch with vga enablement on papr, which needs usb enabled. Is there any more suggestion about this patch? Who else need to ack it? Thank you very much. :) On Mon, Jun 18, 2012 at 5:22 PM, Li Zhang <zhlcindy@gmail.com> wrote: > For pseries machine, it needs to enable usb to add > keyboard or usb mouse. -usb option won't be used in > the future, and machine options is a better way to > enable usb. > > So this patch is to add usb option to machine options > (-machine type=psereis,usb=on/off)to enable/disable > usb controller. > > For specific machines, they will get the machine option > and then create usb controller according to usb option. > > In this patch, usb is on by default on pseries. > So, for -nodefault,usb should be set off in the command > line as the following: > -machine type=pseries,usb=off. > > Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> > --- > hw/spapr.c | 10 ++++++++++ > qemu-config.c | 4 ++++ > 2 files changed, 14 insertions(+), 0 deletions(-) > > diff --git a/hw/spapr.c b/hw/spapr.c > index d0bddbc..8d158d7 100644 > --- a/hw/spapr.c > +++ b/hw/spapr.c > @@ -529,6 +529,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 usb_on = false; > > spapr = g_malloc0(sizeof(*spapr)); > QLIST_INIT(&spapr->phbs); > @@ -661,6 +663,14 @@ 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) > + usb_on = qemu_opt_get_bool(machine_opts, "usb", true); > + > + if (usb_on) { > + 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 bb3bff4..cdab765 100644 > --- a/qemu-config.c > +++ b/qemu-config.c > @@ -583,6 +583,10 @@ static QemuOptsList qemu_machine_opts = { > .name = "dtb", > .type = QEMU_OPT_STRING, > .help = "Linux kernel device tree file", > + },{ > + .name = "usb", > + .type = QEMU_OPT_BOOL, > + .help = "Set on/off to enable/disable usb", > }, > { /* End of list */ } > }, > -- > 1.7.7.6
Am 18.06.2012 11:22, schrieb Li Zhang: > For pseries machine, it needs to enable usb to add > keyboard or usb mouse. -usb option won't be used in > the future, and machine options is a better way to > enable usb. > > So this patch is to add usb option to machine options > (-machine type=psereis,usb=on/off)to enable/disable "pseries" Space after ) please, Western fonts are probably more narrow. ;) > usb controller. > > For specific machines, they will get the machine option > and then create usb controller according to usb option. > > In this patch, usb is on by default on pseries. > So, for -nodefault,usb should be set off in the command Space before "usb" please. > line as the following: > -machine type=pseries,usb=off. > > Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> > --- > hw/spapr.c | 10 ++++++++++ > qemu-config.c | 4 ++++ > 2 files changed, 14 insertions(+), 0 deletions(-) > > diff --git a/hw/spapr.c b/hw/spapr.c > index d0bddbc..8d158d7 100644 > --- a/hw/spapr.c > +++ b/hw/spapr.c > @@ -529,6 +529,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; QemuOpts *machine_opts; checkpatch.pl sometimes emits bogus warnings about this. > + bool usb_on = false; Didn't you want this to be true for sPAPR in absence of -machine? Maybe use a more functional naming? It's either added or not, so add_usb perhaps or provide_usb? Purely stylistic of course. > > spapr = g_malloc0(sizeof(*spapr)); > QLIST_INIT(&spapr->phbs); > @@ -661,6 +663,14 @@ 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) > + usb_on = qemu_opt_get_bool(machine_opts, "usb", true); Still missing braces for if. Other than that looking good to me now. Where's 2/2? Please post the fixed version as a top-level [PATCH v4] along with a small change log after --- or in a cover letter. Andreas > + > + if (usb_on) { > + 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 bb3bff4..cdab765 100644 > --- a/qemu-config.c > +++ b/qemu-config.c > @@ -583,6 +583,10 @@ static QemuOptsList qemu_machine_opts = { > .name = "dtb", > .type = QEMU_OPT_STRING, > .help = "Linux kernel device tree file", > + },{ > + .name = "usb", > + .type = QEMU_OPT_BOOL, > + .help = "Set on/off to enable/disable usb", > }, > { /* End of list */ } > },
On Wed, Jun 27, 2012 at 8:00 PM, Andreas Färber <afaerber@suse.de> wrote: > Am 18.06.2012 11:22, schrieb Li Zhang: >> For pseries machine, it needs to enable usb to add >> keyboard or usb mouse. -usb option won't be used in >> the future, and machine options is a better way to >> enable usb. >> >> So this patch is to add usb option to machine options >> (-machine type=psereis,usb=on/off)to enable/disable > > "pseries" > > Space after ) please, Western fonts are probably more narrow. ;) > OK, I will correct it. >> usb controller. >> >> For specific machines, they will get the machine option >> and then create usb controller according to usb option. >> >> In this patch, usb is on by default on pseries. >> So, for -nodefault,usb should be set off in the command > > Space before "usb" please. > >> line as the following: >> -machine type=pseries,usb=off. >> >> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> >> --- >> hw/spapr.c | 10 ++++++++++ >> qemu-config.c | 4 ++++ >> 2 files changed, 14 insertions(+), 0 deletions(-) >> >> diff --git a/hw/spapr.c b/hw/spapr.c >> index d0bddbc..8d158d7 100644 >> --- a/hw/spapr.c >> +++ b/hw/spapr.c >> @@ -529,6 +529,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; > > QemuOpts *machine_opts; > > checkpatch.pl sometimes emits bogus warnings about this. > OK. I will use it to do that. :) >> + bool usb_on = false; > > Didn't you want this to be true for sPAPR in absence of -machine? > It is set in the following: if (machine_opts) usb_on = qemu_opt_get_bool(machine_opts, "usb", true); It means that when using "-machine" option, usb_on is set as true if usb option is not specified. > Maybe use a more functional naming? It's either added or not, so add_usb > perhaps or provide_usb? Purely stylistic of course. > >> >> spapr = g_malloc0(sizeof(*spapr)); >> QLIST_INIT(&spapr->phbs); >> @@ -661,6 +663,14 @@ 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) >> + usb_on = qemu_opt_get_bool(machine_opts, "usb", true); > > Still missing braces for if. > > Other than that looking good to me now. Where's 2/2? 2/2's title is as the following: [Qemu-devel][PATCH 2/2] spapr: Add support for -vga option. :) > > Please post the fixed version as a top-level [PATCH v4] along with a > small change log after --- or in a cover letter. > OK. I will do that. :) > Andreas > >> + >> + if (usb_on) { >> + 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 bb3bff4..cdab765 100644 >> --- a/qemu-config.c >> +++ b/qemu-config.c >> @@ -583,6 +583,10 @@ static QemuOptsList qemu_machine_opts = { >> .name = "dtb", >> .type = QEMU_OPT_STRING, >> .help = "Linux kernel device tree file", >> + },{ >> + .name = "usb", >> + .type = QEMU_OPT_BOOL, >> + .help = "Set on/off to enable/disable usb", >> }, >> { /* End of list */ } >> }, > > > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
On Wed, Jun 27, 2012 at 11:13 PM, Li Zhang <zhlcindy@gmail.com> wrote: > On Wed, Jun 27, 2012 at 8:00 PM, Andreas Färber <afaerber@suse.de> wrote: >> Am 18.06.2012 11:22, schrieb Li Zhang: >>> For pseries machine, it needs to enable usb to add >>> keyboard or usb mouse. -usb option won't be used in Grammar: The pseries machine needs to enable USB to add a keyboard or USB mouse. The -usb option ... >>> the future, and machine options is a better way to "are" a better way >>> enable usb. >>> >>> So this patch is to add usb option to machine options So this patch adds a USB option ... >>> (-machine type=psereis,usb=on/off)to enable/disable >> >> "pseries" >> >> Space after ) please, Western fonts are probably more narrow. ;) >> > OK, I will correct it. >>> usb controller. >>> >>> For specific machines, they will get the machine option >>> and then create usb controller according to usb option. >>> >>> In this patch, usb is on by default on pseries. >>> So, for -nodefault,usb should be set off in the command >> >> Space before "usb" please. >> >>> line as the following: >>> -machine type=pseries,usb=off. >>> >>> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> >>> --- >>> hw/spapr.c | 10 ++++++++++ >>> qemu-config.c | 4 ++++ >>> 2 files changed, 14 insertions(+), 0 deletions(-) >>> >>> diff --git a/hw/spapr.c b/hw/spapr.c >>> index d0bddbc..8d158d7 100644 >>> --- a/hw/spapr.c >>> +++ b/hw/spapr.c >>> @@ -529,6 +529,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; >> >> QemuOpts *machine_opts; >> >> checkpatch.pl sometimes emits bogus warnings about this. >> > OK. I will use it to do that. :) > >>> + bool usb_on = false; >> >> Didn't you want this to be true for sPAPR in absence of -machine? >> > It is set in the following: > > if (machine_opts) > usb_on = qemu_opt_get_bool(machine_opts, "usb", true); > > It means that when using "-machine" option, usb_on is set as true if > usb option is not specified. > >> Maybe use a more functional naming? It's either added or not, so add_usb >> perhaps or provide_usb? Purely stylistic of course. >> >>> >>> spapr = g_malloc0(sizeof(*spapr)); >>> QLIST_INIT(&spapr->phbs); >>> @@ -661,6 +663,14 @@ 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) >>> + usb_on = qemu_opt_get_bool(machine_opts, "usb", true); >> >> Still missing braces for if. >> >> Other than that looking good to me now. Where's 2/2? > 2/2's title is as the following: > [Qemu-devel][PATCH 2/2] spapr: Add support for -vga option. :) > >> >> Please post the fixed version as a top-level [PATCH v4] along with a >> small change log after --- or in a cover letter. >> > OK. I will do that. :) > >> Andreas >> >>> + >>> + if (usb_on) { >>> + 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 bb3bff4..cdab765 100644 >>> --- a/qemu-config.c >>> +++ b/qemu-config.c >>> @@ -583,6 +583,10 @@ static QemuOptsList qemu_machine_opts = { >>> .name = "dtb", >>> .type = QEMU_OPT_STRING, >>> .help = "Linux kernel device tree file", >>> + },{ >>> + .name = "usb", >>> + .type = QEMU_OPT_BOOL, >>> + .help = "Set on/off to enable/disable usb", >>> }, >>> { /* End of list */ } >>> }, >> >> >> -- >> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany >> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg > > > > -- > > Best Regards > -Li >
Am 27.06.2012 15:13, schrieb Li Zhang: > On Wed, Jun 27, 2012 at 8:00 PM, Andreas Färber <afaerber@suse.de> wrote: >> Am 18.06.2012 11:22, schrieb Li Zhang: >>> + bool usb_on = false; >> >> Didn't you want this to be true for sPAPR in absence of -machine? >> > It is set in the following: > > if (machine_opts) > usb_on = qemu_opt_get_bool(machine_opts, "usb", true); > > It means that when using "-machine" option, usb_on is set as true if > usb option is not specified. What I mean is: -machine usb=on => usb_on == true -machine usb=off => usb_on == false -machine => usb_on == true (nothing) => usb_on == false <-- this !machine_opts case There you should assign true as default, false will be assigned only from qemu_opt_get_bool(). >> Other than that looking good to me now. Where's 2/2? > 2/2's title is as the following: > [Qemu-devel][PATCH 2/2] spapr: Add support for -vga option. :) Please thread the messages together then when sending out the next version. :) /-F
On Wed, Jun 27, 2012 at 9:22 PM, Peter Crosthwaite <peter.crosthwaite@petalogix.com> wrote: > On Wed, Jun 27, 2012 at 11:13 PM, Li Zhang <zhlcindy@gmail.com> wrote: >> On Wed, Jun 27, 2012 at 8:00 PM, Andreas Färber <afaerber@suse.de> wrote: >>> Am 18.06.2012 11:22, schrieb Li Zhang: >>>> For pseries machine, it needs to enable usb to add >>>> keyboard or usb mouse. -usb option won't be used in > > Grammar: The pseries machine needs to enable USB to add a keyboard or > USB mouse. The -usb option ... > >>>> the future, and machine options is a better way to > > "are" a better way > >>>> enable usb. >>>> >>>> So this patch is to add usb option to machine options > > So this patch adds a USB option ... Peter, thank you for correcting my English faults. I will modify it and be more careful next time. :) > >>>> (-machine type=psereis,usb=on/off)to enable/disable >>> >>> "pseries" >>> >>> Space after ) please, Western fonts are probably more narrow. ;) >>> >> OK, I will correct it. >>>> usb controller. >>>> >>>> For specific machines, they will get the machine option >>>> and then create usb controller according to usb option. >>>> >>>> In this patch, usb is on by default on pseries. >>>> So, for -nodefault,usb should be set off in the command >>> >>> Space before "usb" please. >>> >>>> line as the following: >>>> -machine type=pseries,usb=off. >>>> >>>> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> >>>> --- >>>> hw/spapr.c | 10 ++++++++++ >>>> qemu-config.c | 4 ++++ >>>> 2 files changed, 14 insertions(+), 0 deletions(-) >>>> >>>> diff --git a/hw/spapr.c b/hw/spapr.c >>>> index d0bddbc..8d158d7 100644 >>>> --- a/hw/spapr.c >>>> +++ b/hw/spapr.c >>>> @@ -529,6 +529,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; >>> >>> QemuOpts *machine_opts; >>> >>> checkpatch.pl sometimes emits bogus warnings about this. >>> >> OK. I will use it to do that. :) >> >>>> + bool usb_on = false; >>> >>> Didn't you want this to be true for sPAPR in absence of -machine? >>> >> It is set in the following: >> >> if (machine_opts) >> usb_on = qemu_opt_get_bool(machine_opts, "usb", true); >> >> It means that when using "-machine" option, usb_on is set as true if >> usb option is not specified. >> >>> Maybe use a more functional naming? It's either added or not, so add_usb >>> perhaps or provide_usb? Purely stylistic of course. >>> >>>> >>>> spapr = g_malloc0(sizeof(*spapr)); >>>> QLIST_INIT(&spapr->phbs); >>>> @@ -661,6 +663,14 @@ 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) >>>> + usb_on = qemu_opt_get_bool(machine_opts, "usb", true); >>> >>> Still missing braces for if. >>> >>> Other than that looking good to me now. Where's 2/2? >> 2/2's title is as the following: >> [Qemu-devel][PATCH 2/2] spapr: Add support for -vga option. :) >> >>> >>> Please post the fixed version as a top-level [PATCH v4] along with a >>> small change log after --- or in a cover letter. >>> >> OK. I will do that. :) >> >>> Andreas >>> >>>> + >>>> + if (usb_on) { >>>> + 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 bb3bff4..cdab765 100644 >>>> --- a/qemu-config.c >>>> +++ b/qemu-config.c >>>> @@ -583,6 +583,10 @@ static QemuOptsList qemu_machine_opts = { >>>> .name = "dtb", >>>> .type = QEMU_OPT_STRING, >>>> .help = "Linux kernel device tree file", >>>> + },{ >>>> + .name = "usb", >>>> + .type = QEMU_OPT_BOOL, >>>> + .help = "Set on/off to enable/disable usb", >>>> }, >>>> { /* End of list */ } >>>> }, >>> >>> >>> -- >>> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany >>> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg >> >> >> >> -- >> >> Best Regards >> -Li >>
On Wed, Jun 27, 2012 at 9:24 PM, Andreas Färber <afaerber@suse.de> wrote: > Am 27.06.2012 15:13, schrieb Li Zhang: >> On Wed, Jun 27, 2012 at 8:00 PM, Andreas Färber <afaerber@suse.de> wrote: >>> Am 18.06.2012 11:22, schrieb Li Zhang: >>>> + bool usb_on = false; >>> >>> Didn't you want this to be true for sPAPR in absence of -machine? >>> >> It is set in the following: >> >> if (machine_opts) >> usb_on = qemu_opt_get_bool(machine_opts, "usb", true); >> >> It means that when using "-machine" option, usb_on is set as true if >> usb option is not specified. > > What I mean is: > > -machine usb=on => usb_on == true > -machine usb=off => usb_on == false > -machine => usb_on == true > (nothing) => usb_on == false <-- this !machine_opts case > Oh, I see. I will modify it. I thought we only set it as true when using -machine option. > There you should assign true as default, false will be assigned only > from qemu_opt_get_bool(). > >>> Other than that looking good to me now. Where's 2/2? >> 2/2's title is as the following: >> [Qemu-devel][PATCH 2/2] spapr: Add support for -vga option. :) > > Please thread the messages together then when sending out the next > version. :) > OK, got it. > /-F > > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
diff --git a/hw/spapr.c b/hw/spapr.c index d0bddbc..8d158d7 100644 --- a/hw/spapr.c +++ b/hw/spapr.c @@ -529,6 +529,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 usb_on = false; spapr = g_malloc0(sizeof(*spapr)); QLIST_INIT(&spapr->phbs); @@ -661,6 +663,14 @@ 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) + usb_on = qemu_opt_get_bool(machine_opts, "usb", true); + + if (usb_on) { + 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 bb3bff4..cdab765 100644 --- a/qemu-config.c +++ b/qemu-config.c @@ -583,6 +583,10 @@ static QemuOptsList qemu_machine_opts = { .name = "dtb", .type = QEMU_OPT_STRING, .help = "Linux kernel device tree file", + },{ + .name = "usb", + .type = QEMU_OPT_BOOL, + .help = "Set on/off to enable/disable usb", }, { /* End of list */ } },
For pseries machine, it needs to enable usb to add keyboard or usb mouse. -usb option won't be used in the future, and machine options is a better way to enable usb. So this patch is to add usb option to machine options (-machine type=psereis,usb=on/off)to enable/disable usb controller. For specific machines, they will get the machine option and then create usb controller according to usb option. In this patch, usb is on by default on pseries. So, for -nodefault,usb should be set off in the command line as the following: -machine type=pseries,usb=off. Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> --- hw/spapr.c | 10 ++++++++++ qemu-config.c | 4 ++++ 2 files changed, 14 insertions(+), 0 deletions(-)