Re: [Qemu-devel] [PATCH 1/2] Add usb option in machine options to enable/disable usb

[Andreas Färber]

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 <address@hidden>
> ---
>  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 */ }
>      },


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg