[Qemu-ppc,v9,1/1] Add USB option in machine options

When -usb option is used, global varible usb_enabled is set.
And all the plafrom will create one USB controller according
to this variable. In fact, global varibles make code hard
to read.

So this patch is to remove global variable usb_enabled and
add USB option in machine options. All the plaforms will get
USB option value from machine options.

USB option of machine options will be set either by:
  * -usb
  * -machine type=pseries,usb=on

Both these ways can work now. They both set USB option in
machine options. In the future, the first way will be removed.

Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
---
 v7->v8 :
  * Declare usb_enabled() and set_usb_option() in sysemu.h
  * Separate USB enablement on sPAPR platform.

 v8->v9:
  * Fix usb_enable() default value on sPAPR and MAC99

Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>

On Wed, Aug 22, 2012 at 10:31 AM, Li Zhang <zhlcindy@gmail.com> wrote:
> When -usb option is used, global varible usb_enabled is set.
> And all the plafrom will create one USB controller according
> to this variable. In fact, global varibles make code hard
> to read.
>
> So this patch is to remove global variable usb_enabled and
> add USB option in machine options. All the plaforms will get
> USB option value from machine options.
>
> USB option of machine options will be set either by:
>   * -usb
>   * -machine type=pseries,usb=on
>
> Both these ways can work now. They both set USB option in
> machine options. In the future, the first way will be removed.
>
> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
> ---
>  v7->v8 :
>   * Declare usb_enabled() and set_usb_option() in sysemu.h
>   * Separate USB enablement on sPAPR platform.
>
>  v8->v9:
>   * Fix usb_enable() default value on sPAPR and MAC99
>
> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>
> diff --git a/hw/nseries.c b/hw/nseries.c
> index 4df2670..c67e95a 100644
> --- a/hw/nseries.c
> +++ b/hw/nseries.c
> @@ -1322,7 +1322,7 @@ static void n8x0_init(ram_addr_t ram_size, const char *boot_device,
>      n8x0_dss_setup(s);
>      n8x0_cbus_setup(s);
>      n8x0_uart_setup(s);
> -    if (usb_enabled)
> +    if (usb_enabled(false))

Please add braces.

I don't like this usb_enabled(false) way very much but I don't have
anything better to suggest.

>          n8x0_usb_setup(s);
>
>      if (kernel_filename) {
> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> index 0c0096f..b662192 100644
> --- a/hw/pc_piix.c
> +++ b/hw/pc_piix.c
> @@ -267,7 +267,7 @@ static void pc_init1(MemoryRegion *system_memory,
>      pc_cmos_init(below_4g_mem_size, above_4g_mem_size, boot_device,
>                   floppy, idebus[0], idebus[1], rtc_state);
>
> -    if (pci_enabled && usb_enabled) {
> +    if (pci_enabled && usb_enabled(false)) {
>          pci_create_simple(pci_bus, piix3_devfn + 2, "piix3-usb-uhci");
>      }
>
> diff --git a/hw/ppc_newworld.c b/hw/ppc_newworld.c
> index e95cfe8..1d4f494 100644
> --- a/hw/ppc_newworld.c
> +++ b/hw/ppc_newworld.c
> @@ -348,10 +348,6 @@ static void ppc_core99_init (ram_addr_t ram_size,
>      ide_mem[1] = pmac_ide_init(hd, pic[0x0d], dbdma, 0x16, pic[0x02]);
>      ide_mem[2] = pmac_ide_init(&hd[MAX_IDE_DEVS], pic[0x0e], dbdma, 0x1a, pic[0x02]);
>
> -    /* cuda also initialize ADB */
> -    if (machine_arch == ARCH_MAC99_U3) {
> -        usb_enabled = 1;
> -    }
>      cuda_init(&cuda_mem, pic[0x19]);
>
>      adb_kbd_init(&adb_bus);
> @@ -360,15 +356,14 @@ static void ppc_core99_init (ram_addr_t ram_size,
>      macio_init(pci_bus, PCI_DEVICE_ID_APPLE_UNI_N_KEYL, 0, pic_mem,
>                 dbdma_mem, cuda_mem, NULL, 3, ide_mem, escc_bar);
>
> -    if (usb_enabled) {
> +    if (usb_enabled(machine_arch == ARCH_MAC99_U3)) {
>          pci_create_simple(pci_bus, -1, "pci-ohci");
> -    }
> -
> -    /* U3 needs to use USB for input because Linux doesn't support via-cuda
> -       on PPC64 */
> -    if (machine_arch == ARCH_MAC99_U3) {
> -        usbdevice_create("keyboard");
> -        usbdevice_create("mouse");
> +        /* U3 needs to use USB for input because Linux doesn't support via-cuda
> +        on PPC64 */
> +        if (machine_arch == ARCH_MAC99_U3) {
> +            usbdevice_create("keyboard");
> +            usbdevice_create("mouse");
> +        }
>      }
>
>      if (graphic_depth != 15 && graphic_depth != 32 && graphic_depth != 8)
> diff --git a/hw/ppc_oldworld.c b/hw/ppc_oldworld.c
> index 1dcd8a6..1468a32 100644
> --- a/hw/ppc_oldworld.c
> +++ b/hw/ppc_oldworld.c
> @@ -286,7 +286,7 @@ static void ppc_heathrow_init (ram_addr_t ram_size,
>      macio_init(pci_bus, PCI_DEVICE_ID_APPLE_343S1201, 1, pic_mem,
>                 dbdma_mem, cuda_mem, nvr, 2, ide_mem, escc_bar);
>
> -    if (usb_enabled) {
> +    if (usb_enabled(false)) {
>          pci_create_simple(pci_bus, -1, "pci-ohci");
>      }
>
> diff --git a/hw/ppc_prep.c b/hw/ppc_prep.c
> index 7a87616..feeb903 100644
> --- a/hw/ppc_prep.c
> +++ b/hw/ppc_prep.c
> @@ -662,7 +662,7 @@ static void ppc_prep_init (ram_addr_t ram_size,
>      memory_region_add_subregion(sysmem, 0xFEFF0000, xcsr);
>  #endif
>
> -    if (usb_enabled) {
> +    if (usb_enabled(false)) {
>          pci_create_simple(pci_bus, -1, "pci-ohci");
>      }
>
> diff --git a/hw/pxa2xx.c b/hw/pxa2xx.c
> index d5f1420..4787279 100644
> --- a/hw/pxa2xx.c
> +++ b/hw/pxa2xx.c
> @@ -2108,7 +2108,7 @@ PXA2xxState *pxa270_init(MemoryRegion *address_space,
>          s->ssp[i] = (SSIBus *)qdev_get_child_bus(dev, "ssi");
>      }
>
> -    if (usb_enabled) {
> +    if (usb_enabled(false)) {
>          sysbus_create_simple("sysbus-ohci", 0x4c000000,
>                          qdev_get_gpio_in(s->pic, PXA2XX_PIC_USBH1));
>      }
> @@ -2239,7 +2239,7 @@ PXA2xxState *pxa255_init(MemoryRegion *address_space, unsigned int sdram_size)
>          s->ssp[i] = (SSIBus *)qdev_get_child_bus(dev, "ssi");
>      }
>
> -    if (usb_enabled) {
> +    if (usb_enabled(false)) {
>          sysbus_create_simple("sysbus-ohci", 0x4c000000,
>                          qdev_get_gpio_in(s->pic, PXA2XX_PIC_USBH1));
>      }
> diff --git a/hw/realview.c b/hw/realview.c
> index 19db4d0..a8d6f97 100644
> --- a/hw/realview.c
> +++ b/hw/realview.c
> @@ -227,7 +227,7 @@ static void realview_init(ram_addr_t ram_size,
>          sysbus_connect_irq(busdev, 2, pic[50]);
>          sysbus_connect_irq(busdev, 3, pic[51]);
>          pci_bus = (PCIBus *)qdev_get_child_bus(dev, "pci");
> -        if (usb_enabled) {
> +        if (usb_enabled(false)) {
>              pci_create_simple(pci_bus, -1, "pci-ohci");
>          }
>          n = drive_get_max_bus(IF_SCSI);
> diff --git a/hw/spapr.c b/hw/spapr.c
> index be533ee..2e31797 100644
> --- a/hw/spapr.c
> +++ b/hw/spapr.c
> @@ -766,7 +766,7 @@ static void ppc_spapr_init(ram_addr_t ram_size,
>          spapr->has_graphics = true;
>      }
>
> -    if (usb_enabled) {
> +    if (usb_enabled(spapr->has_graphics)) {
>          pci_create_simple(QLIST_FIRST(&spapr->phbs)->host_state.bus,
>                            -1, "pci-ohci");
>          if (spapr->has_graphics) {
> diff --git a/hw/versatilepb.c b/hw/versatilepb.c
> index 7a92034..df32c8b 100644
> --- a/hw/versatilepb.c
> +++ b/hw/versatilepb.c
> @@ -247,7 +247,7 @@ static void versatile_init(ram_addr_t ram_size,
>              pci_nic_init_nofail(nd, "rtl8139", NULL);
>          }
>      }
> -    if (usb_enabled) {
> +    if (usb_enabled(false)) {
>          pci_create_simple(pci_bus, -1, "pci-ohci");
>      }
>      n = drive_get_max_bus(IF_SCSI);
> diff --git a/qemu-config.c b/qemu-config.c
> index c05ffbc..d1a86cf 100644
> --- a/qemu-config.c
> +++ b/qemu-config.c
> @@ -612,6 +612,10 @@ static QemuOptsList qemu_machine_opts = {
>              .name = "dump-guest-core",
>              .type = QEMU_OPT_BOOL,
>              .help = "Include guest memory in  a core dump",
> +        },{
> +            .name = "usb",
> +            .type = QEMU_OPT_BOOL,
> +            .help = "Set on/off to enable/disable usb",
>          },
>          { /* End of list */ }
>      },
> diff --git a/sysemu.h b/sysemu.h
> index 65552ac..be0905e 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -119,7 +119,6 @@ extern const char *keyboard_layout;
>  extern int win2k_install_hack;
>  extern int alt_grab;
>  extern int ctrl_grab;
> -extern int usb_enabled;
>  extern int smp_cpus;
>  extern int max_cpus;
>  extern int cursor_hide;
> @@ -189,4 +188,8 @@ void register_devices(void);
>  void add_boot_device_path(int32_t bootindex, DeviceState *dev,
>                            const char *suffix);
>  char *get_boot_devices_list(uint32_t *size);
> +
> +bool usb_enabled(bool default_usb);
> +void set_usb_option(bool usb_option);
> +
>  #endif
> diff --git a/vl.c b/vl.c
> index 7c577fa..4702e33 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -203,7 +203,6 @@ CharDriverState *serial_hds[MAX_SERIAL_PORTS];
>  CharDriverState *parallel_hds[MAX_PARALLEL_PORTS];
>  CharDriverState *virtcon_hds[MAX_VIRTIO_CONSOLES];
>  int win2k_install_hack = 0;
> -int usb_enabled = 0;
>  int singlestep = 0;
>  int smp_cpus = 1;
>  int max_cpus = 0;
> @@ -790,6 +789,27 @@ static int parse_sandbox(QemuOpts *opts, void *opaque)
>      return 0;
>  }
>
> +/*********QEMU USB setting******/
> +bool usb_enabled(bool default_usb)
> +{
> +    QemuOpts *mach_opts;
> +    mach_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
> +    if (mach_opts) {
> +        return qemu_opt_get_bool(mach_opts, "usb", default_usb);
> +    }
> +    return default_usb;
> +}

It would be more optimal to go through the options only once after
machine selection in vl.c, setting some flags as a result. Then this
and below functions would only check against those flags.

> +
> +void set_usb_option(bool usb_option)
> +{
> +    QemuOpts *mach_opts;
> +    mach_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
> +    if (mach_opts) {
> +        qemu_opt_set_bool(mach_opts, "usb", usb_option);
> +    }
> +}
> +
> +
>  /***********************************************************/
>  /* QEMU Block devices */
>
> @@ -1078,8 +1098,9 @@ static int usb_device_add(const char *devname)
>      const char *p;
>      USBDevice *dev = NULL;
>
> -    if (!usb_enabled)
> +    if (!usb_enabled(false)) {
>          return -1;
> +    }
>
>      /* drivers with .usbdevice_name entry in USBDeviceInfo */
>      dev = usbdevice_create(devname);
> @@ -1115,8 +1136,9 @@ static int usb_device_del(const char *devname)
>      if (strstart(devname, "host:", &p))
>          return usb_host_device_close(p);
>
> -    if (!usb_enabled)
> +    if (!usb_enabled(false)) {
>          return -1;
> +    }
>
>      p = strchr(devname, '.');
>      if (!p)
> @@ -3062,10 +3084,10 @@ int main(int argc, char **argv, char **envp)
>                  }
>                  break;
>              case QEMU_OPTION_usb:
> -                usb_enabled = 1;
> +                set_usb_option(true);
>                  break;
>              case QEMU_OPTION_usbdevice:
> -                usb_enabled = 1;
> +                set_usb_option(true);
>                  add_device_config(DEV_USB, optarg);
>                  break;
>              case QEMU_OPTION_device:
> @@ -3623,7 +3645,7 @@ int main(int argc, char **argv, char **envp)
>      current_machine = machine;
>
>      /* init USB devices */
> -    if (usb_enabled) {
> +    if (usb_enabled(false)) {
>          if (foreach_device_config(DEV_USB, usb_parse) < 0)
>              exit(1);
>      }
> --
> 1.7.7.6
>
>

On 25.08.2012, at 00:43, Blue Swirl <blauwirbel@gmail.com> wrote:

> On Wed, Aug 22, 2012 at 10:31 AM, Li Zhang <zhlcindy@gmail.com> wrote:
>> When -usb option is used, global varible usb_enabled is set.
>> And all the plafrom will create one USB controller according
>> to this variable. In fact, global varibles make code hard
>> to read.
>> 
>> So this patch is to remove global variable usb_enabled and
>> add USB option in machine options. All the plaforms will get
>> USB option value from machine options.
>> 
>> USB option of machine options will be set either by:
>>  * -usb
>>  * -machine type=pseries,usb=on
>> 
>> Both these ways can work now. They both set USB option in
>> machine options. In the future, the first way will be removed.
>> 
>> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>> ---
>> v7->v8 :
>>  * Declare usb_enabled() and set_usb_option() in sysemu.h
>>  * Separate USB enablement on sPAPR platform.
>> 
>> v8->v9:
>>  * Fix usb_enable() default value on sPAPR and MAC99
>> 
>> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>> 
>> diff --git a/hw/nseries.c b/hw/nseries.c
>> index 4df2670..c67e95a 100644
>> --- a/hw/nseries.c
>> +++ b/hw/nseries.c
>> @@ -1322,7 +1322,7 @@ static void n8x0_init(ram_addr_t ram_size, const char *boot_device,
>>     n8x0_dss_setup(s);
>>     n8x0_cbus_setup(s);
>>     n8x0_uart_setup(s);
>> -    if (usb_enabled)
>> +    if (usb_enabled(false))
> 
> Please add braces.
> 
> I don't like this usb_enabled(false) way very much but I don't have
> anything better to suggest.
> 
>>         n8x0_usb_setup(s);
>> 
>>     if (kernel_filename) {
>> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
>> index 0c0096f..b662192 100644
>> --- a/hw/pc_piix.c
>> +++ b/hw/pc_piix.c
>> @@ -267,7 +267,7 @@ static void pc_init1(MemoryRegion *system_memory,
>>     pc_cmos_init(below_4g_mem_size, above_4g_mem_size, boot_device,
>>                  floppy, idebus[0], idebus[1], rtc_state);
>> 
>> -    if (pci_enabled && usb_enabled) {
>> +    if (pci_enabled && usb_enabled(false)) {
>>         pci_create_simple(pci_bus, piix3_devfn + 2, "piix3-usb-uhci");
>>     }
>> 
>> diff --git a/hw/ppc_newworld.c b/hw/ppc_newworld.c
>> index e95cfe8..1d4f494 100644
>> --- a/hw/ppc_newworld.c
>> +++ b/hw/ppc_newworld.c
>> @@ -348,10 +348,6 @@ static void ppc_core99_init (ram_addr_t ram_size,
>>     ide_mem[1] = pmac_ide_init(hd, pic[0x0d], dbdma, 0x16, pic[0x02]);
>>     ide_mem[2] = pmac_ide_init(&hd[MAX_IDE_DEVS], pic[0x0e], dbdma, 0x1a, pic[0x02]);
>> 
>> -    /* cuda also initialize ADB */
>> -    if (machine_arch == ARCH_MAC99_U3) {
>> -        usb_enabled = 1;
>> -    }
>>     cuda_init(&cuda_mem, pic[0x19]);
>> 
>>     adb_kbd_init(&adb_bus);
>> @@ -360,15 +356,14 @@ static void ppc_core99_init (ram_addr_t ram_size,
>>     macio_init(pci_bus, PCI_DEVICE_ID_APPLE_UNI_N_KEYL, 0, pic_mem,
>>                dbdma_mem, cuda_mem, NULL, 3, ide_mem, escc_bar);
>> 
>> -    if (usb_enabled) {
>> +    if (usb_enabled(machine_arch == ARCH_MAC99_U3)) {
>>         pci_create_simple(pci_bus, -1, "pci-ohci");
>> -    }
>> -
>> -    /* U3 needs to use USB for input because Linux doesn't support via-cuda
>> -       on PPC64 */
>> -    if (machine_arch == ARCH_MAC99_U3) {
>> -        usbdevice_create("keyboard");
>> -        usbdevice_create("mouse");
>> +        /* U3 needs to use USB for input because Linux doesn't support via-cuda
>> +        on PPC64 */
>> +        if (machine_arch == ARCH_MAC99_U3) {
>> +            usbdevice_create("keyboard");
>> +            usbdevice_create("mouse");
>> +        }
>>     }
>> 
>>     if (graphic_depth != 15 && graphic_depth != 32 && graphic_depth != 8)
>> diff --git a/hw/ppc_oldworld.c b/hw/ppc_oldworld.c
>> index 1dcd8a6..1468a32 100644
>> --- a/hw/ppc_oldworld.c
>> +++ b/hw/ppc_oldworld.c
>> @@ -286,7 +286,7 @@ static void ppc_heathrow_init (ram_addr_t ram_size,
>>     macio_init(pci_bus, PCI_DEVICE_ID_APPLE_343S1201, 1, pic_mem,
>>                dbdma_mem, cuda_mem, nvr, 2, ide_mem, escc_bar);
>> 
>> -    if (usb_enabled) {
>> +    if (usb_enabled(false)) {
>>         pci_create_simple(pci_bus, -1, "pci-ohci");
>>     }
>> 
>> diff --git a/hw/ppc_prep.c b/hw/ppc_prep.c
>> index 7a87616..feeb903 100644
>> --- a/hw/ppc_prep.c
>> +++ b/hw/ppc_prep.c
>> @@ -662,7 +662,7 @@ static void ppc_prep_init (ram_addr_t ram_size,
>>     memory_region_add_subregion(sysmem, 0xFEFF0000, xcsr);
>> #endif
>> 
>> -    if (usb_enabled) {
>> +    if (usb_enabled(false)) {
>>         pci_create_simple(pci_bus, -1, "pci-ohci");
>>     }
>> 
>> diff --git a/hw/pxa2xx.c b/hw/pxa2xx.c
>> index d5f1420..4787279 100644
>> --- a/hw/pxa2xx.c
>> +++ b/hw/pxa2xx.c
>> @@ -2108,7 +2108,7 @@ PXA2xxState *pxa270_init(MemoryRegion *address_space,
>>         s->ssp[i] = (SSIBus *)qdev_get_child_bus(dev, "ssi");
>>     }
>> 
>> -    if (usb_enabled) {
>> +    if (usb_enabled(false)) {
>>         sysbus_create_simple("sysbus-ohci", 0x4c000000,
>>                         qdev_get_gpio_in(s->pic, PXA2XX_PIC_USBH1));
>>     }
>> @@ -2239,7 +2239,7 @@ PXA2xxState *pxa255_init(MemoryRegion *address_space, unsigned int sdram_size)
>>         s->ssp[i] = (SSIBus *)qdev_get_child_bus(dev, "ssi");
>>     }
>> 
>> -    if (usb_enabled) {
>> +    if (usb_enabled(false)) {
>>         sysbus_create_simple("sysbus-ohci", 0x4c000000,
>>                         qdev_get_gpio_in(s->pic, PXA2XX_PIC_USBH1));
>>     }
>> diff --git a/hw/realview.c b/hw/realview.c
>> index 19db4d0..a8d6f97 100644
>> --- a/hw/realview.c
>> +++ b/hw/realview.c
>> @@ -227,7 +227,7 @@ static void realview_init(ram_addr_t ram_size,
>>         sysbus_connect_irq(busdev, 2, pic[50]);
>>         sysbus_connect_irq(busdev, 3, pic[51]);
>>         pci_bus = (PCIBus *)qdev_get_child_bus(dev, "pci");
>> -        if (usb_enabled) {
>> +        if (usb_enabled(false)) {
>>             pci_create_simple(pci_bus, -1, "pci-ohci");
>>         }
>>         n = drive_get_max_bus(IF_SCSI);
>> diff --git a/hw/spapr.c b/hw/spapr.c
>> index be533ee..2e31797 100644
>> --- a/hw/spapr.c
>> +++ b/hw/spapr.c
>> @@ -766,7 +766,7 @@ static void ppc_spapr_init(ram_addr_t ram_size,
>>         spapr->has_graphics = true;
>>     }
>> 
>> -    if (usb_enabled) {
>> +    if (usb_enabled(spapr->has_graphics)) {
>>         pci_create_simple(QLIST_FIRST(&spapr->phbs)->host_state.bus,
>>                           -1, "pci-ohci");
>>         if (spapr->has_graphics) {
>> diff --git a/hw/versatilepb.c b/hw/versatilepb.c
>> index 7a92034..df32c8b 100644
>> --- a/hw/versatilepb.c
>> +++ b/hw/versatilepb.c
>> @@ -247,7 +247,7 @@ static void versatile_init(ram_addr_t ram_size,
>>             pci_nic_init_nofail(nd, "rtl8139", NULL);
>>         }
>>     }
>> -    if (usb_enabled) {
>> +    if (usb_enabled(false)) {
>>         pci_create_simple(pci_bus, -1, "pci-ohci");
>>     }
>>     n = drive_get_max_bus(IF_SCSI);
>> diff --git a/qemu-config.c b/qemu-config.c
>> index c05ffbc..d1a86cf 100644
>> --- a/qemu-config.c
>> +++ b/qemu-config.c
>> @@ -612,6 +612,10 @@ static QemuOptsList qemu_machine_opts = {
>>             .name = "dump-guest-core",
>>             .type = QEMU_OPT_BOOL,
>>             .help = "Include guest memory in  a core dump",
>> +        },{
>> +            .name = "usb",
>> +            .type = QEMU_OPT_BOOL,
>> +            .help = "Set on/off to enable/disable usb",
>>         },
>>         { /* End of list */ }
>>     },
>> diff --git a/sysemu.h b/sysemu.h
>> index 65552ac..be0905e 100644
>> --- a/sysemu.h
>> +++ b/sysemu.h
>> @@ -119,7 +119,6 @@ extern const char *keyboard_layout;
>> extern int win2k_install_hack;
>> extern int alt_grab;
>> extern int ctrl_grab;
>> -extern int usb_enabled;
>> extern int smp_cpus;
>> extern int max_cpus;
>> extern int cursor_hide;
>> @@ -189,4 +188,8 @@ void register_devices(void);
>> void add_boot_device_path(int32_t bootindex, DeviceState *dev,
>>                           const char *suffix);
>> char *get_boot_devices_list(uint32_t *size);
>> +
>> +bool usb_enabled(bool default_usb);
>> +void set_usb_option(bool usb_option);
>> +
>> #endif
>> diff --git a/vl.c b/vl.c
>> index 7c577fa..4702e33 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -203,7 +203,6 @@ CharDriverState *serial_hds[MAX_SERIAL_PORTS];
>> CharDriverState *parallel_hds[MAX_PARALLEL_PORTS];
>> CharDriverState *virtcon_hds[MAX_VIRTIO_CONSOLES];
>> int win2k_install_hack = 0;
>> -int usb_enabled = 0;
>> int singlestep = 0;
>> int smp_cpus = 1;
>> int max_cpus = 0;
>> @@ -790,6 +789,27 @@ static int parse_sandbox(QemuOpts *opts, void *opaque)
>>     return 0;
>> }
>> 
>> +/*********QEMU USB setting******/
>> +bool usb_enabled(bool default_usb)
>> +{
>> +    QemuOpts *mach_opts;
>> +    mach_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
>> +    if (mach_opts) {
>> +        return qemu_opt_get_bool(mach_opts, "usb", default_usb);
>> +    }
>> +    return default_usb;
>> +}
> 
> It would be more optimal to go through the options only once after
> machine selection in vl.c, setting some flags as a result. Then this
> and below functions would only check against those flags.

This is not a performance critical code path, right? So I'd prefer to have a single point of information (machine opts) rather than yet another global variable.

Alex

> 
>> +
>> +void set_usb_option(bool usb_option)
>> +{
>> +    QemuOpts *mach_opts;
>> +    mach_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
>> +    if (mach_opts) {
>> +        qemu_opt_set_bool(mach_opts, "usb", usb_option);
>> +    }
>> +}
>> +
>> +
>> /***********************************************************/
>> /* QEMU Block devices */
>> 
>> @@ -1078,8 +1098,9 @@ static int usb_device_add(const char *devname)
>>     const char *p;
>>     USBDevice *dev = NULL;
>> 
>> -    if (!usb_enabled)
>> +    if (!usb_enabled(false)) {
>>         return -1;
>> +    }
>> 
>>     /* drivers with .usbdevice_name entry in USBDeviceInfo */
>>     dev = usbdevice_create(devname);
>> @@ -1115,8 +1136,9 @@ static int usb_device_del(const char *devname)
>>     if (strstart(devname, "host:", &p))
>>         return usb_host_device_close(p);
>> 
>> -    if (!usb_enabled)
>> +    if (!usb_enabled(false)) {
>>         return -1;
>> +    }
>> 
>>     p = strchr(devname, '.');
>>     if (!p)
>> @@ -3062,10 +3084,10 @@ int main(int argc, char **argv, char **envp)
>>                 }
>>                 break;
>>             case QEMU_OPTION_usb:
>> -                usb_enabled = 1;
>> +                set_usb_option(true);
>>                 break;
>>             case QEMU_OPTION_usbdevice:
>> -                usb_enabled = 1;
>> +                set_usb_option(true);
>>                 add_device_config(DEV_USB, optarg);
>>                 break;
>>             case QEMU_OPTION_device:
>> @@ -3623,7 +3645,7 @@ int main(int argc, char **argv, char **envp)
>>     current_machine = machine;
>> 
>>     /* init USB devices */
>> -    if (usb_enabled) {
>> +    if (usb_enabled(false)) {
>>         if (foreach_device_config(DEV_USB, usb_parse) < 0)
>>             exit(1);
>>     }
>> --
>> 1.7.7.6
>> 
>> 
>

On Sat, Aug 25, 2012 at 2:27 PM, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 25.08.2012, at 00:43, Blue Swirl <blauwirbel@gmail.com> wrote:
>
>> On Wed, Aug 22, 2012 at 10:31 AM, Li Zhang <zhlcindy@gmail.com> wrote:
>>> When -usb option is used, global varible usb_enabled is set.
>>> And all the plafrom will create one USB controller according
>>> to this variable. In fact, global varibles make code hard
>>> to read.
>>>
>>> So this patch is to remove global variable usb_enabled and
>>> add USB option in machine options. All the plaforms will get
>>> USB option value from machine options.
>>>
>>> USB option of machine options will be set either by:
>>>  * -usb
>>>  * -machine type=pseries,usb=on
>>>
>>> Both these ways can work now. They both set USB option in
>>> machine options. In the future, the first way will be removed.
>>>
>>> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>>> ---
>>> v7->v8 :
>>>  * Declare usb_enabled() and set_usb_option() in sysemu.h
>>>  * Separate USB enablement on sPAPR platform.
>>>
>>> v8->v9:
>>>  * Fix usb_enable() default value on sPAPR and MAC99
>>>
>>> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>>>
>>> diff --git a/hw/nseries.c b/hw/nseries.c
>>> index 4df2670..c67e95a 100644
>>> --- a/hw/nseries.c
>>> +++ b/hw/nseries.c
>>> @@ -1322,7 +1322,7 @@ static void n8x0_init(ram_addr_t ram_size, const char *boot_device,
>>>     n8x0_dss_setup(s);
>>>     n8x0_cbus_setup(s);
>>>     n8x0_uart_setup(s);
>>> -    if (usb_enabled)
>>> +    if (usb_enabled(false))
>>
>> Please add braces.
>>
>> I don't like this usb_enabled(false) way very much but I don't have
>> anything better to suggest.
>>
>>>         n8x0_usb_setup(s);
>>>
>>>     if (kernel_filename) {
>>> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
>>> index 0c0096f..b662192 100644
>>> --- a/hw/pc_piix.c
>>> +++ b/hw/pc_piix.c
>>> @@ -267,7 +267,7 @@ static void pc_init1(MemoryRegion *system_memory,
>>>     pc_cmos_init(below_4g_mem_size, above_4g_mem_size, boot_device,
>>>                  floppy, idebus[0], idebus[1], rtc_state);
>>>
>>> -    if (pci_enabled && usb_enabled) {
>>> +    if (pci_enabled && usb_enabled(false)) {
>>>         pci_create_simple(pci_bus, piix3_devfn + 2, "piix3-usb-uhci");
>>>     }
>>>
>>> diff --git a/hw/ppc_newworld.c b/hw/ppc_newworld.c
>>> index e95cfe8..1d4f494 100644
>>> --- a/hw/ppc_newworld.c
>>> +++ b/hw/ppc_newworld.c
>>> @@ -348,10 +348,6 @@ static void ppc_core99_init (ram_addr_t ram_size,
>>>     ide_mem[1] = pmac_ide_init(hd, pic[0x0d], dbdma, 0x16, pic[0x02]);
>>>     ide_mem[2] = pmac_ide_init(&hd[MAX_IDE_DEVS], pic[0x0e], dbdma, 0x1a, pic[0x02]);
>>>
>>> -    /* cuda also initialize ADB */
>>> -    if (machine_arch == ARCH_MAC99_U3) {
>>> -        usb_enabled = 1;
>>> -    }
>>>     cuda_init(&cuda_mem, pic[0x19]);
>>>
>>>     adb_kbd_init(&adb_bus);
>>> @@ -360,15 +356,14 @@ static void ppc_core99_init (ram_addr_t ram_size,
>>>     macio_init(pci_bus, PCI_DEVICE_ID_APPLE_UNI_N_KEYL, 0, pic_mem,
>>>                dbdma_mem, cuda_mem, NULL, 3, ide_mem, escc_bar);
>>>
>>> -    if (usb_enabled) {
>>> +    if (usb_enabled(machine_arch == ARCH_MAC99_U3)) {
>>>         pci_create_simple(pci_bus, -1, "pci-ohci");
>>> -    }
>>> -
>>> -    /* U3 needs to use USB for input because Linux doesn't support via-cuda
>>> -       on PPC64 */
>>> -    if (machine_arch == ARCH_MAC99_U3) {
>>> -        usbdevice_create("keyboard");
>>> -        usbdevice_create("mouse");
>>> +        /* U3 needs to use USB for input because Linux doesn't support via-cuda
>>> +        on PPC64 */
>>> +        if (machine_arch == ARCH_MAC99_U3) {
>>> +            usbdevice_create("keyboard");
>>> +            usbdevice_create("mouse");
>>> +        }
>>>     }
>>>
>>>     if (graphic_depth != 15 && graphic_depth != 32 && graphic_depth != 8)
>>> diff --git a/hw/ppc_oldworld.c b/hw/ppc_oldworld.c
>>> index 1dcd8a6..1468a32 100644
>>> --- a/hw/ppc_oldworld.c
>>> +++ b/hw/ppc_oldworld.c
>>> @@ -286,7 +286,7 @@ static void ppc_heathrow_init (ram_addr_t ram_size,
>>>     macio_init(pci_bus, PCI_DEVICE_ID_APPLE_343S1201, 1, pic_mem,
>>>                dbdma_mem, cuda_mem, nvr, 2, ide_mem, escc_bar);
>>>
>>> -    if (usb_enabled) {
>>> +    if (usb_enabled(false)) {
>>>         pci_create_simple(pci_bus, -1, "pci-ohci");
>>>     }
>>>
>>> diff --git a/hw/ppc_prep.c b/hw/ppc_prep.c
>>> index 7a87616..feeb903 100644
>>> --- a/hw/ppc_prep.c
>>> +++ b/hw/ppc_prep.c
>>> @@ -662,7 +662,7 @@ static void ppc_prep_init (ram_addr_t ram_size,
>>>     memory_region_add_subregion(sysmem, 0xFEFF0000, xcsr);
>>> #endif
>>>
>>> -    if (usb_enabled) {
>>> +    if (usb_enabled(false)) {
>>>         pci_create_simple(pci_bus, -1, "pci-ohci");
>>>     }
>>>
>>> diff --git a/hw/pxa2xx.c b/hw/pxa2xx.c
>>> index d5f1420..4787279 100644
>>> --- a/hw/pxa2xx.c
>>> +++ b/hw/pxa2xx.c
>>> @@ -2108,7 +2108,7 @@ PXA2xxState *pxa270_init(MemoryRegion *address_space,
>>>         s->ssp[i] = (SSIBus *)qdev_get_child_bus(dev, "ssi");
>>>     }
>>>
>>> -    if (usb_enabled) {
>>> +    if (usb_enabled(false)) {
>>>         sysbus_create_simple("sysbus-ohci", 0x4c000000,
>>>                         qdev_get_gpio_in(s->pic, PXA2XX_PIC_USBH1));
>>>     }
>>> @@ -2239,7 +2239,7 @@ PXA2xxState *pxa255_init(MemoryRegion *address_space, unsigned int sdram_size)
>>>         s->ssp[i] = (SSIBus *)qdev_get_child_bus(dev, "ssi");
>>>     }
>>>
>>> -    if (usb_enabled) {
>>> +    if (usb_enabled(false)) {
>>>         sysbus_create_simple("sysbus-ohci", 0x4c000000,
>>>                         qdev_get_gpio_in(s->pic, PXA2XX_PIC_USBH1));
>>>     }
>>> diff --git a/hw/realview.c b/hw/realview.c
>>> index 19db4d0..a8d6f97 100644
>>> --- a/hw/realview.c
>>> +++ b/hw/realview.c
>>> @@ -227,7 +227,7 @@ static void realview_init(ram_addr_t ram_size,
>>>         sysbus_connect_irq(busdev, 2, pic[50]);
>>>         sysbus_connect_irq(busdev, 3, pic[51]);
>>>         pci_bus = (PCIBus *)qdev_get_child_bus(dev, "pci");
>>> -        if (usb_enabled) {
>>> +        if (usb_enabled(false)) {
>>>             pci_create_simple(pci_bus, -1, "pci-ohci");
>>>         }
>>>         n = drive_get_max_bus(IF_SCSI);
>>> diff --git a/hw/spapr.c b/hw/spapr.c
>>> index be533ee..2e31797 100644
>>> --- a/hw/spapr.c
>>> +++ b/hw/spapr.c
>>> @@ -766,7 +766,7 @@ static void ppc_spapr_init(ram_addr_t ram_size,
>>>         spapr->has_graphics = true;
>>>     }
>>>
>>> -    if (usb_enabled) {
>>> +    if (usb_enabled(spapr->has_graphics)) {
>>>         pci_create_simple(QLIST_FIRST(&spapr->phbs)->host_state.bus,
>>>                           -1, "pci-ohci");
>>>         if (spapr->has_graphics) {
>>> diff --git a/hw/versatilepb.c b/hw/versatilepb.c
>>> index 7a92034..df32c8b 100644
>>> --- a/hw/versatilepb.c
>>> +++ b/hw/versatilepb.c
>>> @@ -247,7 +247,7 @@ static void versatile_init(ram_addr_t ram_size,
>>>             pci_nic_init_nofail(nd, "rtl8139", NULL);
>>>         }
>>>     }
>>> -    if (usb_enabled) {
>>> +    if (usb_enabled(false)) {
>>>         pci_create_simple(pci_bus, -1, "pci-ohci");
>>>     }
>>>     n = drive_get_max_bus(IF_SCSI);
>>> diff --git a/qemu-config.c b/qemu-config.c
>>> index c05ffbc..d1a86cf 100644
>>> --- a/qemu-config.c
>>> +++ b/qemu-config.c
>>> @@ -612,6 +612,10 @@ static QemuOptsList qemu_machine_opts = {
>>>             .name = "dump-guest-core",
>>>             .type = QEMU_OPT_BOOL,
>>>             .help = "Include guest memory in  a core dump",
>>> +        },{
>>> +            .name = "usb",
>>> +            .type = QEMU_OPT_BOOL,
>>> +            .help = "Set on/off to enable/disable usb",
>>>         },
>>>         { /* End of list */ }
>>>     },
>>> diff --git a/sysemu.h b/sysemu.h
>>> index 65552ac..be0905e 100644
>>> --- a/sysemu.h
>>> +++ b/sysemu.h
>>> @@ -119,7 +119,6 @@ extern const char *keyboard_layout;
>>> extern int win2k_install_hack;
>>> extern int alt_grab;
>>> extern int ctrl_grab;
>>> -extern int usb_enabled;
>>> extern int smp_cpus;
>>> extern int max_cpus;
>>> extern int cursor_hide;
>>> @@ -189,4 +188,8 @@ void register_devices(void);
>>> void add_boot_device_path(int32_t bootindex, DeviceState *dev,
>>>                           const char *suffix);
>>> char *get_boot_devices_list(uint32_t *size);
>>> +
>>> +bool usb_enabled(bool default_usb);
>>> +void set_usb_option(bool usb_option);
>>> +
>>> #endif
>>> diff --git a/vl.c b/vl.c
>>> index 7c577fa..4702e33 100644
>>> --- a/vl.c
>>> +++ b/vl.c
>>> @@ -203,7 +203,6 @@ CharDriverState *serial_hds[MAX_SERIAL_PORTS];
>>> CharDriverState *parallel_hds[MAX_PARALLEL_PORTS];
>>> CharDriverState *virtcon_hds[MAX_VIRTIO_CONSOLES];
>>> int win2k_install_hack = 0;
>>> -int usb_enabled = 0;
>>> int singlestep = 0;
>>> int smp_cpus = 1;
>>> int max_cpus = 0;
>>> @@ -790,6 +789,27 @@ static int parse_sandbox(QemuOpts *opts, void *opaque)
>>>     return 0;
>>> }
>>>
>>> +/*********QEMU USB setting******/
>>> +bool usb_enabled(bool default_usb)
>>> +{
>>> +    QemuOpts *mach_opts;
>>> +    mach_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
>>> +    if (mach_opts) {
>>> +        return qemu_opt_get_bool(mach_opts, "usb", default_usb);
>>> +    }
>>> +    return default_usb;
>>> +}
>>
>> It would be more optimal to go through the options only once after
>> machine selection in vl.c, setting some flags as a result. Then this
>> and below functions would only check against those flags.
>
> This is not a performance critical code path, right? So I'd prefer to have a single point of information (machine opts) rather than yet another global variable.

The flags don't have to be global, they can be static to vl.c.

>
> Alex
>
>>
>>> +
>>> +void set_usb_option(bool usb_option)
>>> +{
>>> +    QemuOpts *mach_opts;
>>> +    mach_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
>>> +    if (mach_opts) {
>>> +        qemu_opt_set_bool(mach_opts, "usb", usb_option);
>>> +    }
>>> +}
>>> +
>>> +
>>> /***********************************************************/
>>> /* QEMU Block devices */
>>>
>>> @@ -1078,8 +1098,9 @@ static int usb_device_add(const char *devname)
>>>     const char *p;
>>>     USBDevice *dev = NULL;
>>>
>>> -    if (!usb_enabled)
>>> +    if (!usb_enabled(false)) {
>>>         return -1;
>>> +    }
>>>
>>>     /* drivers with .usbdevice_name entry in USBDeviceInfo */
>>>     dev = usbdevice_create(devname);
>>> @@ -1115,8 +1136,9 @@ static int usb_device_del(const char *devname)
>>>     if (strstart(devname, "host:", &p))
>>>         return usb_host_device_close(p);
>>>
>>> -    if (!usb_enabled)
>>> +    if (!usb_enabled(false)) {
>>>         return -1;
>>> +    }
>>>
>>>     p = strchr(devname, '.');
>>>     if (!p)
>>> @@ -3062,10 +3084,10 @@ int main(int argc, char **argv, char **envp)
>>>                 }
>>>                 break;
>>>             case QEMU_OPTION_usb:
>>> -                usb_enabled = 1;
>>> +                set_usb_option(true);
>>>                 break;
>>>             case QEMU_OPTION_usbdevice:
>>> -                usb_enabled = 1;
>>> +                set_usb_option(true);
>>>                 add_device_config(DEV_USB, optarg);
>>>                 break;
>>>             case QEMU_OPTION_device:
>>> @@ -3623,7 +3645,7 @@ int main(int argc, char **argv, char **envp)
>>>     current_machine = machine;
>>>
>>>     /* init USB devices */
>>> -    if (usb_enabled) {
>>> +    if (usb_enabled(false)) {
>>>         if (foreach_device_config(DEV_USB, usb_parse) < 0)
>>>             exit(1);
>>>     }
>>> --
>>> 1.7.7.6
>>>
>>>
>>

On 26.08.2012, at 10:34, Blue Swirl <blauwirbel@gmail.com> wrote:

> On Sat, Aug 25, 2012 at 2:27 PM, Alexander Graf <agraf@suse.de> wrote:
>> 
>> 
>> On 25.08.2012, at 00:43, Blue Swirl <blauwirbel@gmail.com> wrote:
>> 
>>> On Wed, Aug 22, 2012 at 10:31 AM, Li Zhang <zhlcindy@gmail.com> wrote:
>>>> When -usb option is used, global varible usb_enabled is set.
>>>> And all the plafrom will create one USB controller according
>>>> to this variable. In fact, global varibles make code hard
>>>> to read.
>>>> 
>>>> So this patch is to remove global variable usb_enabled and
>>>> add USB option in machine options. All the plaforms will get
>>>> USB option value from machine options.
>>>> 
>>>> USB option of machine options will be set either by:
>>>> * -usb
>>>> * -machine type=pseries,usb=on
>>>> 
>>>> Both these ways can work now. They both set USB option in
>>>> machine options. In the future, the first way will be removed.
>>>> 
>>>> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>>>> ---
>>>> v7->v8 :
>>>> * Declare usb_enabled() and set_usb_option() in sysemu.h
>>>> * Separate USB enablement on sPAPR platform.
>>>> 
>>>> v8->v9:
>>>> * Fix usb_enable() default value on sPAPR and MAC99
>>>> 
>>>> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>>>> 
>>>> diff --git a/hw/nseries.c b/hw/nseries.c
>>>> index 4df2670..c67e95a 100644
>>>> --- a/hw/nseries.c
>>>> +++ b/hw/nseries.c
>>>> @@ -1322,7 +1322,7 @@ static void n8x0_init(ram_addr_t ram_size, const char *boot_device,
>>>>    n8x0_dss_setup(s);
>>>>    n8x0_cbus_setup(s);
>>>>    n8x0_uart_setup(s);
>>>> -    if (usb_enabled)
>>>> +    if (usb_enabled(false))
>>> 
>>> Please add braces.
>>> 
>>> I don't like this usb_enabled(false) way very much but I don't have
>>> anything better to suggest.
>>> 
>>>>        n8x0_usb_setup(s);
>>>> 
>>>>    if (kernel_filename) {
>>>> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
>>>> index 0c0096f..b662192 100644
>>>> --- a/hw/pc_piix.c
>>>> +++ b/hw/pc_piix.c
>>>> @@ -267,7 +267,7 @@ static void pc_init1(MemoryRegion *system_memory,
>>>>    pc_cmos_init(below_4g_mem_size, above_4g_mem_size, boot_device,
>>>>                 floppy, idebus[0], idebus[1], rtc_state);
>>>> 
>>>> -    if (pci_enabled && usb_enabled) {
>>>> +    if (pci_enabled && usb_enabled(false)) {
>>>>        pci_create_simple(pci_bus, piix3_devfn + 2, "piix3-usb-uhci");
>>>>    }
>>>> 
>>>> diff --git a/hw/ppc_newworld.c b/hw/ppc_newworld.c
>>>> index e95cfe8..1d4f494 100644
>>>> --- a/hw/ppc_newworld.c
>>>> +++ b/hw/ppc_newworld.c
>>>> @@ -348,10 +348,6 @@ static void ppc_core99_init (ram_addr_t ram_size,
>>>>    ide_mem[1] = pmac_ide_init(hd, pic[0x0d], dbdma, 0x16, pic[0x02]);
>>>>    ide_mem[2] = pmac_ide_init(&hd[MAX_IDE_DEVS], pic[0x0e], dbdma, 0x1a, pic[0x02]);
>>>> 
>>>> -    /* cuda also initialize ADB */
>>>> -    if (machine_arch == ARCH_MAC99_U3) {
>>>> -        usb_enabled = 1;
>>>> -    }
>>>>    cuda_init(&cuda_mem, pic[0x19]);
>>>> 
>>>>    adb_kbd_init(&adb_bus);
>>>> @@ -360,15 +356,14 @@ static void ppc_core99_init (ram_addr_t ram_size,
>>>>    macio_init(pci_bus, PCI_DEVICE_ID_APPLE_UNI_N_KEYL, 0, pic_mem,
>>>>               dbdma_mem, cuda_mem, NULL, 3, ide_mem, escc_bar);
>>>> 
>>>> -    if (usb_enabled) {
>>>> +    if (usb_enabled(machine_arch == ARCH_MAC99_U3)) {
>>>>        pci_create_simple(pci_bus, -1, "pci-ohci");
>>>> -    }
>>>> -
>>>> -    /* U3 needs to use USB for input because Linux doesn't support via-cuda
>>>> -       on PPC64 */
>>>> -    if (machine_arch == ARCH_MAC99_U3) {
>>>> -        usbdevice_create("keyboard");
>>>> -        usbdevice_create("mouse");
>>>> +        /* U3 needs to use USB for input because Linux doesn't support via-cuda
>>>> +        on PPC64 */
>>>> +        if (machine_arch == ARCH_MAC99_U3) {
>>>> +            usbdevice_create("keyboard");
>>>> +            usbdevice_create("mouse");
>>>> +        }
>>>>    }
>>>> 
>>>>    if (graphic_depth != 15 && graphic_depth != 32 && graphic_depth != 8)
>>>> diff --git a/hw/ppc_oldworld.c b/hw/ppc_oldworld.c
>>>> index 1dcd8a6..1468a32 100644
>>>> --- a/hw/ppc_oldworld.c
>>>> +++ b/hw/ppc_oldworld.c
>>>> @@ -286,7 +286,7 @@ static void ppc_heathrow_init (ram_addr_t ram_size,
>>>>    macio_init(pci_bus, PCI_DEVICE_ID_APPLE_343S1201, 1, pic_mem,
>>>>               dbdma_mem, cuda_mem, nvr, 2, ide_mem, escc_bar);
>>>> 
>>>> -    if (usb_enabled) {
>>>> +    if (usb_enabled(false)) {
>>>>        pci_create_simple(pci_bus, -1, "pci-ohci");
>>>>    }
>>>> 
>>>> diff --git a/hw/ppc_prep.c b/hw/ppc_prep.c
>>>> index 7a87616..feeb903 100644
>>>> --- a/hw/ppc_prep.c
>>>> +++ b/hw/ppc_prep.c
>>>> @@ -662,7 +662,7 @@ static void ppc_prep_init (ram_addr_t ram_size,
>>>>    memory_region_add_subregion(sysmem, 0xFEFF0000, xcsr);
>>>> #endif
>>>> 
>>>> -    if (usb_enabled) {
>>>> +    if (usb_enabled(false)) {
>>>>        pci_create_simple(pci_bus, -1, "pci-ohci");
>>>>    }
>>>> 
>>>> diff --git a/hw/pxa2xx.c b/hw/pxa2xx.c
>>>> index d5f1420..4787279 100644
>>>> --- a/hw/pxa2xx.c
>>>> +++ b/hw/pxa2xx.c
>>>> @@ -2108,7 +2108,7 @@ PXA2xxState *pxa270_init(MemoryRegion *address_space,
>>>>        s->ssp[i] = (SSIBus *)qdev_get_child_bus(dev, "ssi");
>>>>    }
>>>> 
>>>> -    if (usb_enabled) {
>>>> +    if (usb_enabled(false)) {
>>>>        sysbus_create_simple("sysbus-ohci", 0x4c000000,
>>>>                        qdev_get_gpio_in(s->pic, PXA2XX_PIC_USBH1));
>>>>    }
>>>> @@ -2239,7 +2239,7 @@ PXA2xxState *pxa255_init(MemoryRegion *address_space, unsigned int sdram_size)
>>>>        s->ssp[i] = (SSIBus *)qdev_get_child_bus(dev, "ssi");
>>>>    }
>>>> 
>>>> -    if (usb_enabled) {
>>>> +    if (usb_enabled(false)) {
>>>>        sysbus_create_simple("sysbus-ohci", 0x4c000000,
>>>>                        qdev_get_gpio_in(s->pic, PXA2XX_PIC_USBH1));
>>>>    }
>>>> diff --git a/hw/realview.c b/hw/realview.c
>>>> index 19db4d0..a8d6f97 100644
>>>> --- a/hw/realview.c
>>>> +++ b/hw/realview.c
>>>> @@ -227,7 +227,7 @@ static void realview_init(ram_addr_t ram_size,
>>>>        sysbus_connect_irq(busdev, 2, pic[50]);
>>>>        sysbus_connect_irq(busdev, 3, pic[51]);
>>>>        pci_bus = (PCIBus *)qdev_get_child_bus(dev, "pci");
>>>> -        if (usb_enabled) {
>>>> +        if (usb_enabled(false)) {
>>>>            pci_create_simple(pci_bus, -1, "pci-ohci");
>>>>        }
>>>>        n = drive_get_max_bus(IF_SCSI);
>>>> diff --git a/hw/spapr.c b/hw/spapr.c
>>>> index be533ee..2e31797 100644
>>>> --- a/hw/spapr.c
>>>> +++ b/hw/spapr.c
>>>> @@ -766,7 +766,7 @@ static void ppc_spapr_init(ram_addr_t ram_size,
>>>>        spapr->has_graphics = true;
>>>>    }
>>>> 
>>>> -    if (usb_enabled) {
>>>> +    if (usb_enabled(spapr->has_graphics)) {
>>>>        pci_create_simple(QLIST_FIRST(&spapr->phbs)->host_state.bus,
>>>>                          -1, "pci-ohci");
>>>>        if (spapr->has_graphics) {
>>>> diff --git a/hw/versatilepb.c b/hw/versatilepb.c
>>>> index 7a92034..df32c8b 100644
>>>> --- a/hw/versatilepb.c
>>>> +++ b/hw/versatilepb.c
>>>> @@ -247,7 +247,7 @@ static void versatile_init(ram_addr_t ram_size,
>>>>            pci_nic_init_nofail(nd, "rtl8139", NULL);
>>>>        }
>>>>    }
>>>> -    if (usb_enabled) {
>>>> +    if (usb_enabled(false)) {
>>>>        pci_create_simple(pci_bus, -1, "pci-ohci");
>>>>    }
>>>>    n = drive_get_max_bus(IF_SCSI);
>>>> diff --git a/qemu-config.c b/qemu-config.c
>>>> index c05ffbc..d1a86cf 100644
>>>> --- a/qemu-config.c
>>>> +++ b/qemu-config.c
>>>> @@ -612,6 +612,10 @@ static QemuOptsList qemu_machine_opts = {
>>>>            .name = "dump-guest-core",
>>>>            .type = QEMU_OPT_BOOL,
>>>>            .help = "Include guest memory in  a core dump",
>>>> +        },{
>>>> +            .name = "usb",
>>>> +            .type = QEMU_OPT_BOOL,
>>>> +            .help = "Set on/off to enable/disable usb",
>>>>        },
>>>>        { /* End of list */ }
>>>>    },
>>>> diff --git a/sysemu.h b/sysemu.h
>>>> index 65552ac..be0905e 100644
>>>> --- a/sysemu.h
>>>> +++ b/sysemu.h
>>>> @@ -119,7 +119,6 @@ extern const char *keyboard_layout;
>>>> extern int win2k_install_hack;
>>>> extern int alt_grab;
>>>> extern int ctrl_grab;
>>>> -extern int usb_enabled;
>>>> extern int smp_cpus;
>>>> extern int max_cpus;
>>>> extern int cursor_hide;
>>>> @@ -189,4 +188,8 @@ void register_devices(void);
>>>> void add_boot_device_path(int32_t bootindex, DeviceState *dev,
>>>>                          const char *suffix);
>>>> char *get_boot_devices_list(uint32_t *size);
>>>> +
>>>> +bool usb_enabled(bool default_usb);
>>>> +void set_usb_option(bool usb_option);
>>>> +
>>>> #endif
>>>> diff --git a/vl.c b/vl.c
>>>> index 7c577fa..4702e33 100644
>>>> --- a/vl.c
>>>> +++ b/vl.c
>>>> @@ -203,7 +203,6 @@ CharDriverState *serial_hds[MAX_SERIAL_PORTS];
>>>> CharDriverState *parallel_hds[MAX_PARALLEL_PORTS];
>>>> CharDriverState *virtcon_hds[MAX_VIRTIO_CONSOLES];
>>>> int win2k_install_hack = 0;
>>>> -int usb_enabled = 0;
>>>> int singlestep = 0;
>>>> int smp_cpus = 1;
>>>> int max_cpus = 0;
>>>> @@ -790,6 +789,27 @@ static int parse_sandbox(QemuOpts *opts, void *opaque)
>>>>    return 0;
>>>> }
>>>> 
>>>> +/*********QEMU USB setting******/
>>>> +bool usb_enabled(bool default_usb)
>>>> +{
>>>> +    QemuOpts *mach_opts;
>>>> +    mach_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
>>>> +    if (mach_opts) {
>>>> +        return qemu_opt_get_bool(mach_opts, "usb", default_usb);
>>>> +    }
>>>> +    return default_usb;
>>>> +}
>>> 
>>> It would be more optimal to go through the options only once after
>>> machine selection in vl.c, setting some flags as a result. Then this
>>> and below functions would only check against those flags.
>> 
>> This is not a performance critical code path, right? So I'd prefer to have a single point of information (machine opts) rather than yet another global variable.
> 
> The flags don't have to be global, they can be static to vl.c.

It would still be duplication of information bits that need to be synchronized. I don't see any point for the set_usb_option helper. So why bother with another variable when we have one in the machine opts?

Alex

> 
>> 
>> Alex
>> 
>>> 
>>>> +
>>>> +void set_usb_option(bool usb_option)
>>>> +{
>>>> +    QemuOpts *mach_opts;
>>>> +    mach_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
>>>> +    if (mach_opts) {
>>>> +        qemu_opt_set_bool(mach_opts, "usb", usb_option);
>>>> +    }
>>>> +}
>>>> +
>>>> +
>>>> /***********************************************************/
>>>> /* QEMU Block devices */
>>>> 
>>>> @@ -1078,8 +1098,9 @@ static int usb_device_add(const char *devname)
>>>>    const char *p;
>>>>    USBDevice *dev = NULL;
>>>> 
>>>> -    if (!usb_enabled)
>>>> +    if (!usb_enabled(false)) {
>>>>        return -1;
>>>> +    }
>>>> 
>>>>    /* drivers with .usbdevice_name entry in USBDeviceInfo */
>>>>    dev = usbdevice_create(devname);
>>>> @@ -1115,8 +1136,9 @@ static int usb_device_del(const char *devname)
>>>>    if (strstart(devname, "host:", &p))
>>>>        return usb_host_device_close(p);
>>>> 
>>>> -    if (!usb_enabled)
>>>> +    if (!usb_enabled(false)) {
>>>>        return -1;
>>>> +    }
>>>> 
>>>>    p = strchr(devname, '.');
>>>>    if (!p)
>>>> @@ -3062,10 +3084,10 @@ int main(int argc, char **argv, char **envp)
>>>>                }
>>>>                break;
>>>>            case QEMU_OPTION_usb:
>>>> -                usb_enabled = 1;
>>>> +                set_usb_option(true);
>>>>                break;
>>>>            case QEMU_OPTION_usbdevice:
>>>> -                usb_enabled = 1;
>>>> +                set_usb_option(true);
>>>>                add_device_config(DEV_USB, optarg);
>>>>                break;
>>>>            case QEMU_OPTION_device:
>>>> @@ -3623,7 +3645,7 @@ int main(int argc, char **argv, char **envp)
>>>>    current_machine = machine;
>>>> 
>>>>    /* init USB devices */
>>>> -    if (usb_enabled) {
>>>> +    if (usb_enabled(false)) {
>>>>        if (foreach_device_config(DEV_USB, usb_parse) < 0)
>>>>            exit(1);
>>>>    }
>>>> --
>>>> 1.7.7.6
>>>> 
>>>> 
>>>

On Mon, Aug 27, 2012 at 1:59 AM, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 26.08.2012, at 10:34, Blue Swirl <blauwirbel@gmail.com> wrote:
>
>> On Sat, Aug 25, 2012 at 2:27 PM, Alexander Graf <agraf@suse.de> wrote:
>>>
>>>
>>> On 25.08.2012, at 00:43, Blue Swirl <blauwirbel@gmail.com> wrote:
>>>
>>>> On Wed, Aug 22, 2012 at 10:31 AM, Li Zhang <zhlcindy@gmail.com> wrote:
>>>>> When -usb option is used, global varible usb_enabled is set.
>>>>> And all the plafrom will create one USB controller according
>>>>> to this variable. In fact, global varibles make code hard
>>>>> to read.
>>>>>
>>>>> So this patch is to remove global variable usb_enabled and
>>>>> add USB option in machine options. All the plaforms will get
>>>>> USB option value from machine options.
>>>>>
>>>>> USB option of machine options will be set either by:
>>>>> * -usb
>>>>> * -machine type=pseries,usb=on
>>>>>
>>>>> Both these ways can work now. They both set USB option in
>>>>> machine options. In the future, the first way will be removed.
>>>>>
>>>>> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>>>>> ---
>>>>> v7->v8 :
>>>>> * Declare usb_enabled() and set_usb_option() in sysemu.h
>>>>> * Separate USB enablement on sPAPR platform.
>>>>>
>>>>> v8->v9:
>>>>> * Fix usb_enable() default value on sPAPR and MAC99
>>>>>
>>>>> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>>>>>
>>>>> diff --git a/hw/nseries.c b/hw/nseries.c
>>>>> index 4df2670..c67e95a 100644
>>>>> --- a/hw/nseries.c
>>>>> +++ b/hw/nseries.c
>>>>> @@ -1322,7 +1322,7 @@ static void n8x0_init(ram_addr_t ram_size, const char *boot_device,
>>>>>    n8x0_dss_setup(s);
>>>>>    n8x0_cbus_setup(s);
>>>>>    n8x0_uart_setup(s);
>>>>> -    if (usb_enabled)
>>>>> +    if (usb_enabled(false))
>>>>
>>>> Please add braces.
>>>>
>>>> I don't like this usb_enabled(false) way very much but I don't have
>>>> anything better to suggest.
>>>>
>>>>>        n8x0_usb_setup(s);
>>>>>
>>>>>    if (kernel_filename) {
>>>>> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
>>>>> index 0c0096f..b662192 100644
>>>>> --- a/hw/pc_piix.c
>>>>> +++ b/hw/pc_piix.c
>>>>> @@ -267,7 +267,7 @@ static void pc_init1(MemoryRegion *system_memory,
>>>>>    pc_cmos_init(below_4g_mem_size, above_4g_mem_size, boot_device,
>>>>>                 floppy, idebus[0], idebus[1], rtc_state);
>>>>>
>>>>> -    if (pci_enabled && usb_enabled) {
>>>>> +    if (pci_enabled && usb_enabled(false)) {
>>>>>        pci_create_simple(pci_bus, piix3_devfn + 2, "piix3-usb-uhci");
>>>>>    }
>>>>>
>>>>> diff --git a/hw/ppc_newworld.c b/hw/ppc_newworld.c
>>>>> index e95cfe8..1d4f494 100644
>>>>> --- a/hw/ppc_newworld.c
>>>>> +++ b/hw/ppc_newworld.c
>>>>> @@ -348,10 +348,6 @@ static void ppc_core99_init (ram_addr_t ram_size,
>>>>>    ide_mem[1] = pmac_ide_init(hd, pic[0x0d], dbdma, 0x16, pic[0x02]);
>>>>>    ide_mem[2] = pmac_ide_init(&hd[MAX_IDE_DEVS], pic[0x0e], dbdma, 0x1a, pic[0x02]);
>>>>>
>>>>> -    /* cuda also initialize ADB */
>>>>> -    if (machine_arch == ARCH_MAC99_U3) {
>>>>> -        usb_enabled = 1;
>>>>> -    }
>>>>>    cuda_init(&cuda_mem, pic[0x19]);
>>>>>
>>>>>    adb_kbd_init(&adb_bus);
>>>>> @@ -360,15 +356,14 @@ static void ppc_core99_init (ram_addr_t ram_size,
>>>>>    macio_init(pci_bus, PCI_DEVICE_ID_APPLE_UNI_N_KEYL, 0, pic_mem,
>>>>>               dbdma_mem, cuda_mem, NULL, 3, ide_mem, escc_bar);
>>>>>
>>>>> -    if (usb_enabled) {
>>>>> +    if (usb_enabled(machine_arch == ARCH_MAC99_U3)) {
>>>>>        pci_create_simple(pci_bus, -1, "pci-ohci");
>>>>> -    }
>>>>> -
>>>>> -    /* U3 needs to use USB for input because Linux doesn't support via-cuda
>>>>> -       on PPC64 */
>>>>> -    if (machine_arch == ARCH_MAC99_U3) {
>>>>> -        usbdevice_create("keyboard");
>>>>> -        usbdevice_create("mouse");
>>>>> +        /* U3 needs to use USB for input because Linux doesn't support via-cuda
>>>>> +        on PPC64 */
>>>>> +        if (machine_arch == ARCH_MAC99_U3) {
>>>>> +            usbdevice_create("keyboard");
>>>>> +            usbdevice_create("mouse");
>>>>> +        }
>>>>>    }
>>>>>
>>>>>    if (graphic_depth != 15 && graphic_depth != 32 && graphic_depth != 8)
>>>>> diff --git a/hw/ppc_oldworld.c b/hw/ppc_oldworld.c
>>>>> index 1dcd8a6..1468a32 100644
>>>>> --- a/hw/ppc_oldworld.c
>>>>> +++ b/hw/ppc_oldworld.c
>>>>> @@ -286,7 +286,7 @@ static void ppc_heathrow_init (ram_addr_t ram_size,
>>>>>    macio_init(pci_bus, PCI_DEVICE_ID_APPLE_343S1201, 1, pic_mem,
>>>>>               dbdma_mem, cuda_mem, nvr, 2, ide_mem, escc_bar);
>>>>>
>>>>> -    if (usb_enabled) {
>>>>> +    if (usb_enabled(false)) {
>>>>>        pci_create_simple(pci_bus, -1, "pci-ohci");
>>>>>    }
>>>>>
>>>>> diff --git a/hw/ppc_prep.c b/hw/ppc_prep.c
>>>>> index 7a87616..feeb903 100644
>>>>> --- a/hw/ppc_prep.c
>>>>> +++ b/hw/ppc_prep.c
>>>>> @@ -662,7 +662,7 @@ static void ppc_prep_init (ram_addr_t ram_size,
>>>>>    memory_region_add_subregion(sysmem, 0xFEFF0000, xcsr);
>>>>> #endif
>>>>>
>>>>> -    if (usb_enabled) {
>>>>> +    if (usb_enabled(false)) {
>>>>>        pci_create_simple(pci_bus, -1, "pci-ohci");
>>>>>    }
>>>>>
>>>>> diff --git a/hw/pxa2xx.c b/hw/pxa2xx.c
>>>>> index d5f1420..4787279 100644
>>>>> --- a/hw/pxa2xx.c
>>>>> +++ b/hw/pxa2xx.c
>>>>> @@ -2108,7 +2108,7 @@ PXA2xxState *pxa270_init(MemoryRegion *address_space,
>>>>>        s->ssp[i] = (SSIBus *)qdev_get_child_bus(dev, "ssi");
>>>>>    }
>>>>>
>>>>> -    if (usb_enabled) {
>>>>> +    if (usb_enabled(false)) {
>>>>>        sysbus_create_simple("sysbus-ohci", 0x4c000000,
>>>>>                        qdev_get_gpio_in(s->pic, PXA2XX_PIC_USBH1));
>>>>>    }
>>>>> @@ -2239,7 +2239,7 @@ PXA2xxState *pxa255_init(MemoryRegion *address_space, unsigned int sdram_size)
>>>>>        s->ssp[i] = (SSIBus *)qdev_get_child_bus(dev, "ssi");
>>>>>    }
>>>>>
>>>>> -    if (usb_enabled) {
>>>>> +    if (usb_enabled(false)) {
>>>>>        sysbus_create_simple("sysbus-ohci", 0x4c000000,
>>>>>                        qdev_get_gpio_in(s->pic, PXA2XX_PIC_USBH1));
>>>>>    }
>>>>> diff --git a/hw/realview.c b/hw/realview.c
>>>>> index 19db4d0..a8d6f97 100644
>>>>> --- a/hw/realview.c
>>>>> +++ b/hw/realview.c
>>>>> @@ -227,7 +227,7 @@ static void realview_init(ram_addr_t ram_size,
>>>>>        sysbus_connect_irq(busdev, 2, pic[50]);
>>>>>        sysbus_connect_irq(busdev, 3, pic[51]);
>>>>>        pci_bus = (PCIBus *)qdev_get_child_bus(dev, "pci");
>>>>> -        if (usb_enabled) {
>>>>> +        if (usb_enabled(false)) {
>>>>>            pci_create_simple(pci_bus, -1, "pci-ohci");
>>>>>        }
>>>>>        n = drive_get_max_bus(IF_SCSI);
>>>>> diff --git a/hw/spapr.c b/hw/spapr.c
>>>>> index be533ee..2e31797 100644
>>>>> --- a/hw/spapr.c
>>>>> +++ b/hw/spapr.c
>>>>> @@ -766,7 +766,7 @@ static void ppc_spapr_init(ram_addr_t ram_size,
>>>>>        spapr->has_graphics = true;
>>>>>    }
>>>>>
>>>>> -    if (usb_enabled) {
>>>>> +    if (usb_enabled(spapr->has_graphics)) {
>>>>>        pci_create_simple(QLIST_FIRST(&spapr->phbs)->host_state.bus,
>>>>>                          -1, "pci-ohci");
>>>>>        if (spapr->has_graphics) {
>>>>> diff --git a/hw/versatilepb.c b/hw/versatilepb.c
>>>>> index 7a92034..df32c8b 100644
>>>>> --- a/hw/versatilepb.c
>>>>> +++ b/hw/versatilepb.c
>>>>> @@ -247,7 +247,7 @@ static void versatile_init(ram_addr_t ram_size,
>>>>>            pci_nic_init_nofail(nd, "rtl8139", NULL);
>>>>>        }
>>>>>    }
>>>>> -    if (usb_enabled) {
>>>>> +    if (usb_enabled(false)) {
>>>>>        pci_create_simple(pci_bus, -1, "pci-ohci");
>>>>>    }
>>>>>    n = drive_get_max_bus(IF_SCSI);
>>>>> diff --git a/qemu-config.c b/qemu-config.c
>>>>> index c05ffbc..d1a86cf 100644
>>>>> --- a/qemu-config.c
>>>>> +++ b/qemu-config.c
>>>>> @@ -612,6 +612,10 @@ static QemuOptsList qemu_machine_opts = {
>>>>>            .name = "dump-guest-core",
>>>>>            .type = QEMU_OPT_BOOL,
>>>>>            .help = "Include guest memory in  a core dump",
>>>>> +        },{
>>>>> +            .name = "usb",
>>>>> +            .type = QEMU_OPT_BOOL,
>>>>> +            .help = "Set on/off to enable/disable usb",
>>>>>        },
>>>>>        { /* End of list */ }
>>>>>    },
>>>>> diff --git a/sysemu.h b/sysemu.h
>>>>> index 65552ac..be0905e 100644
>>>>> --- a/sysemu.h
>>>>> +++ b/sysemu.h
>>>>> @@ -119,7 +119,6 @@ extern const char *keyboard_layout;
>>>>> extern int win2k_install_hack;
>>>>> extern int alt_grab;
>>>>> extern int ctrl_grab;
>>>>> -extern int usb_enabled;
>>>>> extern int smp_cpus;
>>>>> extern int max_cpus;
>>>>> extern int cursor_hide;
>>>>> @@ -189,4 +188,8 @@ void register_devices(void);
>>>>> void add_boot_device_path(int32_t bootindex, DeviceState *dev,
>>>>>                          const char *suffix);
>>>>> char *get_boot_devices_list(uint32_t *size);
>>>>> +
>>>>> +bool usb_enabled(bool default_usb);
>>>>> +void set_usb_option(bool usb_option);
>>>>> +
>>>>> #endif
>>>>> diff --git a/vl.c b/vl.c
>>>>> index 7c577fa..4702e33 100644
>>>>> --- a/vl.c
>>>>> +++ b/vl.c
>>>>> @@ -203,7 +203,6 @@ CharDriverState *serial_hds[MAX_SERIAL_PORTS];
>>>>> CharDriverState *parallel_hds[MAX_PARALLEL_PORTS];
>>>>> CharDriverState *virtcon_hds[MAX_VIRTIO_CONSOLES];
>>>>> int win2k_install_hack = 0;
>>>>> -int usb_enabled = 0;
>>>>> int singlestep = 0;
>>>>> int smp_cpus = 1;
>>>>> int max_cpus = 0;
>>>>> @@ -790,6 +789,27 @@ static int parse_sandbox(QemuOpts *opts, void *opaque)
>>>>>    return 0;
>>>>> }
>>>>>
>>>>> +/*********QEMU USB setting******/
>>>>> +bool usb_enabled(bool default_usb)
>>>>> +{
>>>>> +    QemuOpts *mach_opts;
>>>>> +    mach_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
>>>>> +    if (mach_opts) {
>>>>> +        return qemu_opt_get_bool(mach_opts, "usb", default_usb);
>>>>> +    }
>>>>> +    return default_usb;
>>>>> +}
>>>>
>>>> It would be more optimal to go through the options only once after
>>>> machine selection in vl.c, setting some flags as a result. Then this
>>>> and below functions would only check against those flags.
>>>
>>> This is not a performance critical code path, right? So I'd prefer to have a single point of information (machine opts) rather than yet another global variable.
>>
>> The flags don't have to be global, they can be static to vl.c.
>
> It would still be duplication of information bits that need to be synchronized. I don't see any point for the set_usb_option helper. So why bother with another variable when we have one in the machine opts?

If set_usb_option() is deleted (I don't see any callers), the variable
could be static to usb_enabled(). But yes, the function is probably
called only once at board init, so caching would not make sense.

>
> Alex
>
>>
>>>
>>> Alex
>>>
>>>>
>>>>> +
>>>>> +void set_usb_option(bool usb_option)
>>>>> +{
>>>>> +    QemuOpts *mach_opts;
>>>>> +    mach_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
>>>>> +    if (mach_opts) {
>>>>> +        qemu_opt_set_bool(mach_opts, "usb", usb_option);
>>>>> +    }
>>>>> +}
>>>>> +
>>>>> +
>>>>> /***********************************************************/
>>>>> /* QEMU Block devices */
>>>>>
>>>>> @@ -1078,8 +1098,9 @@ static int usb_device_add(const char *devname)
>>>>>    const char *p;
>>>>>    USBDevice *dev = NULL;
>>>>>
>>>>> -    if (!usb_enabled)
>>>>> +    if (!usb_enabled(false)) {
>>>>>        return -1;
>>>>> +    }
>>>>>
>>>>>    /* drivers with .usbdevice_name entry in USBDeviceInfo */
>>>>>    dev = usbdevice_create(devname);
>>>>> @@ -1115,8 +1136,9 @@ static int usb_device_del(const char *devname)
>>>>>    if (strstart(devname, "host:", &p))
>>>>>        return usb_host_device_close(p);
>>>>>
>>>>> -    if (!usb_enabled)
>>>>> +    if (!usb_enabled(false)) {
>>>>>        return -1;
>>>>> +    }
>>>>>
>>>>>    p = strchr(devname, '.');
>>>>>    if (!p)
>>>>> @@ -3062,10 +3084,10 @@ int main(int argc, char **argv, char **envp)
>>>>>                }
>>>>>                break;
>>>>>            case QEMU_OPTION_usb:
>>>>> -                usb_enabled = 1;
>>>>> +                set_usb_option(true);
>>>>>                break;
>>>>>            case QEMU_OPTION_usbdevice:
>>>>> -                usb_enabled = 1;
>>>>> +                set_usb_option(true);
>>>>>                add_device_config(DEV_USB, optarg);
>>>>>                break;
>>>>>            case QEMU_OPTION_device:
>>>>> @@ -3623,7 +3645,7 @@ int main(int argc, char **argv, char **envp)
>>>>>    current_machine = machine;
>>>>>
>>>>>    /* init USB devices */
>>>>> -    if (usb_enabled) {
>>>>> +    if (usb_enabled(false)) {
>>>>>        if (foreach_device_config(DEV_USB, usb_parse) < 0)
>>>>>            exit(1);
>>>>>    }
>>>>> --
>>>>> 1.7.7.6
>>>>>
>>>>>
>>>>

On Sat, Aug 25, 2012 at 3:43 PM, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Wed, Aug 22, 2012 at 10:31 AM, Li Zhang <zhlcindy@gmail.com> wrote:
>> When -usb option is used, global varible usb_enabled is set.
>> And all the plafrom will create one USB controller according
>> to this variable. In fact, global varibles make code hard
>> to read.
>>
>> So this patch is to remove global variable usb_enabled and
>> add USB option in machine options. All the plaforms will get
>> USB option value from machine options.
>>
>> USB option of machine options will be set either by:
>>   * -usb
>>   * -machine type=pseries,usb=on
>>
>> Both these ways can work now. They both set USB option in
>> machine options. In the future, the first way will be removed.
>>
>> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>> ---
>>  v7->v8 :
>>   * Declare usb_enabled() and set_usb_option() in sysemu.h
>>   * Separate USB enablement on sPAPR platform.
>>
>>  v8->v9:
>>   * Fix usb_enable() default value on sPAPR and MAC99
>>
>> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>>
>> diff --git a/hw/nseries.c b/hw/nseries.c
>> index 4df2670..c67e95a 100644
>> --- a/hw/nseries.c
>> +++ b/hw/nseries.c
>> @@ -1322,7 +1322,7 @@ static void n8x0_init(ram_addr_t ram_size, const char *boot_device,
>>      n8x0_dss_setup(s);
>>      n8x0_cbus_setup(s);
>>      n8x0_uart_setup(s);
>> -    if (usb_enabled)
>> +    if (usb_enabled(false))
>
> Please add braces.
OK, I will do that.

>
> I don't like this usb_enabled(false) way very much but I don't have
> anything better to suggest.

Thanks,
It seems that default value is false on most of platforms, only true
for several platforms.

>>          n8x0_usb_setup(s);
>>
>>      if (kernel_filename) {
>> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
>> index 0c0096f..b662192 100644
>> --- a/hw/pc_piix.c
>> +++ b/hw/pc_piix.c
>> @@ -267,7 +267,7 @@ static void pc_init1(MemoryRegion *system_memory,
>>      pc_cmos_init(below_4g_mem_size, above_4g_mem_size, boot_device,
>>                   floppy, idebus[0], idebus[1], rtc_state);
>>
>> -    if (pci_enabled && usb_enabled) {
>> +    if (pci_enabled && usb_enabled(false)) {
>>          pci_create_simple(pci_bus, piix3_devfn + 2, "piix3-usb-uhci");
>>      }
>>
>> diff --git a/hw/ppc_newworld.c b/hw/ppc_newworld.c
>> index e95cfe8..1d4f494 100644
>> --- a/hw/ppc_newworld.c
>> +++ b/hw/ppc_newworld.c
>> @@ -348,10 +348,6 @@ static void ppc_core99_init (ram_addr_t ram_size,
>>      ide_mem[1] = pmac_ide_init(hd, pic[0x0d], dbdma, 0x16, pic[0x02]);
>>      ide_mem[2] = pmac_ide_init(&hd[MAX_IDE_DEVS], pic[0x0e], dbdma, 0x1a, pic[0x02]);
>>
>> -    /* cuda also initialize ADB */
>> -    if (machine_arch == ARCH_MAC99_U3) {
>> -        usb_enabled = 1;
>> -    }
>>      cuda_init(&cuda_mem, pic[0x19]);
>>
>>      adb_kbd_init(&adb_bus);
>> @@ -360,15 +356,14 @@ static void ppc_core99_init (ram_addr_t ram_size,
>>      macio_init(pci_bus, PCI_DEVICE_ID_APPLE_UNI_N_KEYL, 0, pic_mem,
>>                 dbdma_mem, cuda_mem, NULL, 3, ide_mem, escc_bar);
>>
>> -    if (usb_enabled) {
>> +    if (usb_enabled(machine_arch == ARCH_MAC99_U3)) {
>>          pci_create_simple(pci_bus, -1, "pci-ohci");
>> -    }
>> -
>> -    /* U3 needs to use USB for input because Linux doesn't support via-cuda
>> -       on PPC64 */
>> -    if (machine_arch == ARCH_MAC99_U3) {
>> -        usbdevice_create("keyboard");
>> -        usbdevice_create("mouse");
>> +        /* U3 needs to use USB for input because Linux doesn't support via-cuda
>> +        on PPC64 */
>> +        if (machine_arch == ARCH_MAC99_U3) {
>> +            usbdevice_create("keyboard");
>> +            usbdevice_create("mouse");
>> +        }
>>      }
>>
>>      if (graphic_depth != 15 && graphic_depth != 32 && graphic_depth != 8)
>> diff --git a/hw/ppc_oldworld.c b/hw/ppc_oldworld.c
>> index 1dcd8a6..1468a32 100644
>> --- a/hw/ppc_oldworld.c
>> +++ b/hw/ppc_oldworld.c
>> @@ -286,7 +286,7 @@ static void ppc_heathrow_init (ram_addr_t ram_size,
>>      macio_init(pci_bus, PCI_DEVICE_ID_APPLE_343S1201, 1, pic_mem,
>>                 dbdma_mem, cuda_mem, nvr, 2, ide_mem, escc_bar);
>>
>> -    if (usb_enabled) {
>> +    if (usb_enabled(false)) {
>>          pci_create_simple(pci_bus, -1, "pci-ohci");
>>      }
>>
>> diff --git a/hw/ppc_prep.c b/hw/ppc_prep.c
>> index 7a87616..feeb903 100644
>> --- a/hw/ppc_prep.c
>> +++ b/hw/ppc_prep.c
>> @@ -662,7 +662,7 @@ static void ppc_prep_init (ram_addr_t ram_size,
>>      memory_region_add_subregion(sysmem, 0xFEFF0000, xcsr);
>>  #endif
>>
>> -    if (usb_enabled) {
>> +    if (usb_enabled(false)) {
>>          pci_create_simple(pci_bus, -1, "pci-ohci");
>>      }
>>
>> diff --git a/hw/pxa2xx.c b/hw/pxa2xx.c
>> index d5f1420..4787279 100644
>> --- a/hw/pxa2xx.c
>> +++ b/hw/pxa2xx.c
>> @@ -2108,7 +2108,7 @@ PXA2xxState *pxa270_init(MemoryRegion *address_space,
>>          s->ssp[i] = (SSIBus *)qdev_get_child_bus(dev, "ssi");
>>      }
>>
>> -    if (usb_enabled) {
>> +    if (usb_enabled(false)) {
>>          sysbus_create_simple("sysbus-ohci", 0x4c000000,
>>                          qdev_get_gpio_in(s->pic, PXA2XX_PIC_USBH1));
>>      }
>> @@ -2239,7 +2239,7 @@ PXA2xxState *pxa255_init(MemoryRegion *address_space, unsigned int sdram_size)
>>          s->ssp[i] = (SSIBus *)qdev_get_child_bus(dev, "ssi");
>>      }
>>
>> -    if (usb_enabled) {
>> +    if (usb_enabled(false)) {
>>          sysbus_create_simple("sysbus-ohci", 0x4c000000,
>>                          qdev_get_gpio_in(s->pic, PXA2XX_PIC_USBH1));
>>      }
>> diff --git a/hw/realview.c b/hw/realview.c
>> index 19db4d0..a8d6f97 100644
>> --- a/hw/realview.c
>> +++ b/hw/realview.c
>> @@ -227,7 +227,7 @@ static void realview_init(ram_addr_t ram_size,
>>          sysbus_connect_irq(busdev, 2, pic[50]);
>>          sysbus_connect_irq(busdev, 3, pic[51]);
>>          pci_bus = (PCIBus *)qdev_get_child_bus(dev, "pci");
>> -        if (usb_enabled) {
>> +        if (usb_enabled(false)) {
>>              pci_create_simple(pci_bus, -1, "pci-ohci");
>>          }
>>          n = drive_get_max_bus(IF_SCSI);
>> diff --git a/hw/spapr.c b/hw/spapr.c
>> index be533ee..2e31797 100644
>> --- a/hw/spapr.c
>> +++ b/hw/spapr.c
>> @@ -766,7 +766,7 @@ static void ppc_spapr_init(ram_addr_t ram_size,
>>          spapr->has_graphics = true;
>>      }
>>
>> -    if (usb_enabled) {
>> +    if (usb_enabled(spapr->has_graphics)) {
>>          pci_create_simple(QLIST_FIRST(&spapr->phbs)->host_state.bus,
>>                            -1, "pci-ohci");
>>          if (spapr->has_graphics) {
>> diff --git a/hw/versatilepb.c b/hw/versatilepb.c
>> index 7a92034..df32c8b 100644
>> --- a/hw/versatilepb.c
>> +++ b/hw/versatilepb.c
>> @@ -247,7 +247,7 @@ static void versatile_init(ram_addr_t ram_size,
>>              pci_nic_init_nofail(nd, "rtl8139", NULL);
>>          }
>>      }
>> -    if (usb_enabled) {
>> +    if (usb_enabled(false)) {
>>          pci_create_simple(pci_bus, -1, "pci-ohci");
>>      }
>>      n = drive_get_max_bus(IF_SCSI);
>> diff --git a/qemu-config.c b/qemu-config.c
>> index c05ffbc..d1a86cf 100644
>> --- a/qemu-config.c
>> +++ b/qemu-config.c
>> @@ -612,6 +612,10 @@ static QemuOptsList qemu_machine_opts = {
>>              .name = "dump-guest-core",
>>              .type = QEMU_OPT_BOOL,
>>              .help = "Include guest memory in  a core dump",
>> +        },{
>> +            .name = "usb",
>> +            .type = QEMU_OPT_BOOL,
>> +            .help = "Set on/off to enable/disable usb",
>>          },
>>          { /* End of list */ }
>>      },
>> diff --git a/sysemu.h b/sysemu.h
>> index 65552ac..be0905e 100644
>> --- a/sysemu.h
>> +++ b/sysemu.h
>> @@ -119,7 +119,6 @@ extern const char *keyboard_layout;
>>  extern int win2k_install_hack;
>>  extern int alt_grab;
>>  extern int ctrl_grab;
>> -extern int usb_enabled;
>>  extern int smp_cpus;
>>  extern int max_cpus;
>>  extern int cursor_hide;
>> @@ -189,4 +188,8 @@ void register_devices(void);
>>  void add_boot_device_path(int32_t bootindex, DeviceState *dev,
>>                            const char *suffix);
>>  char *get_boot_devices_list(uint32_t *size);
>> +
>> +bool usb_enabled(bool default_usb);
>> +void set_usb_option(bool usb_option);
>> +
>>  #endif
>> diff --git a/vl.c b/vl.c
>> index 7c577fa..4702e33 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -203,7 +203,6 @@ CharDriverState *serial_hds[MAX_SERIAL_PORTS];
>>  CharDriverState *parallel_hds[MAX_PARALLEL_PORTS];
>>  CharDriverState *virtcon_hds[MAX_VIRTIO_CONSOLES];
>>  int win2k_install_hack = 0;
>> -int usb_enabled = 0;
>>  int singlestep = 0;
>>  int smp_cpus = 1;
>>  int max_cpus = 0;
>> @@ -790,6 +789,27 @@ static int parse_sandbox(QemuOpts *opts, void *opaque)
>>      return 0;
>>  }
>>
>> +/*********QEMU USB setting******/
>> +bool usb_enabled(bool default_usb)
>> +{
>> +    QemuOpts *mach_opts;
>> +    mach_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
>> +    if (mach_opts) {
>> +        return qemu_opt_get_bool(mach_opts, "usb", default_usb);
>> +    }
>> +    return default_usb;
>> +}
>
> It would be more optimal to go through the options only once after
> machine selection in vl.c, setting some flags as a result. Then this
> and below functions would only check against those flags.
>
The following function is called when using -usb or -usbdevice.
It also sets USB option when -usb or -usbdevice is used.
The following function hasn't be called by other platforms.
>> +
>> +void set_usb_option(bool usb_option)
>> +{
>> +    QemuOpts *mach_opts;
>> +    mach_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
>> +    if (mach_opts) {
>> +        qemu_opt_set_bool(mach_opts, "usb", usb_option);
>> +    }
>> +}
>> +
>> +
>>  /***********************************************************/
>>  /* QEMU Block devices */
>>
>> @@ -1078,8 +1098,9 @@ static int usb_device_add(const char *devname)
>>      const char *p;
>>      USBDevice *dev = NULL;
>>
>> -    if (!usb_enabled)
>> +    if (!usb_enabled(false)) {
>>          return -1;
>> +    }
>>
>>      /* drivers with .usbdevice_name entry in USBDeviceInfo */
>>      dev = usbdevice_create(devname);
>> @@ -1115,8 +1136,9 @@ static int usb_device_del(const char *devname)
>>      if (strstart(devname, "host:", &p))
>>          return usb_host_device_close(p);
>>
>> -    if (!usb_enabled)
>> +    if (!usb_enabled(false)) {
>>          return -1;
>> +    }
>>
>>      p = strchr(devname, '.');
>>      if (!p)
>> @@ -3062,10 +3084,10 @@ int main(int argc, char **argv, char **envp)
>>                  }
>>                  break;
>>              case QEMU_OPTION_usb:
>> -                usb_enabled = 1;
>> +                set_usb_option(true);
>>                  break;
>>              case QEMU_OPTION_usbdevice:
>> -                usb_enabled = 1;
>> +                set_usb_option(true);
>>                  add_device_config(DEV_USB, optarg);
>>                  break;
>>              case QEMU_OPTION_device:
>> @@ -3623,7 +3645,7 @@ int main(int argc, char **argv, char **envp)
>>      current_machine = machine;
>>
>>      /* init USB devices */
>> -    if (usb_enabled) {
>> +    if (usb_enabled(false)) {
>>          if (foreach_device_config(DEV_USB, usb_parse) < 0)
>>              exit(1);
>>      }
>> --
>> 1.7.7.6
>>
>>

On Tue, Aug 28, 2012 at 2:08 AM, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Mon, Aug 27, 2012 at 1:59 AM, Alexander Graf <agraf@suse.de> wrote:
>>
>>
>> On 26.08.2012, at 10:34, Blue Swirl <blauwirbel@gmail.com> wrote:
>>
>>> On Sat, Aug 25, 2012 at 2:27 PM, Alexander Graf <agraf@suse.de> wrote:
>>>>
>>>>
>>>> On 25.08.2012, at 00:43, Blue Swirl <blauwirbel@gmail.com> wrote:
>>>>
>>>>> On Wed, Aug 22, 2012 at 10:31 AM, Li Zhang <zhlcindy@gmail.com> wrote:
>>>>>> When -usb option is used, global varible usb_enabled is set.
>>>>>> And all the plafrom will create one USB controller according
>>>>>> to this variable. In fact, global varibles make code hard
>>>>>> to read.
>>>>>>
>>>>>> So this patch is to remove global variable usb_enabled and
>>>>>> add USB option in machine options. All the plaforms will get
>>>>>> USB option value from machine options.
>>>>>>
>>>>>> USB option of machine options will be set either by:
>>>>>> * -usb
>>>>>> * -machine type=pseries,usb=on
>>>>>>
>>>>>> Both these ways can work now. They both set USB option in
>>>>>> machine options. In the future, the first way will be removed.
>>>>>>
>>>>>> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>>>>>> ---
>>>>>> v7->v8 :
>>>>>> * Declare usb_enabled() and set_usb_option() in sysemu.h
>>>>>> * Separate USB enablement on sPAPR platform.
>>>>>>
>>>>>> v8->v9:
>>>>>> * Fix usb_enable() default value on sPAPR and MAC99
>>>>>>
>>>>>> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>>>>>>
>>>>>> diff --git a/hw/nseries.c b/hw/nseries.c
>>>>>> index 4df2670..c67e95a 100644
>>>>>> --- a/hw/nseries.c
>>>>>> +++ b/hw/nseries.c
>>>>>> @@ -1322,7 +1322,7 @@ static void n8x0_init(ram_addr_t ram_size, const char *boot_device,
>>>>>>    n8x0_dss_setup(s);
>>>>>>    n8x0_cbus_setup(s);
>>>>>>    n8x0_uart_setup(s);
>>>>>> -    if (usb_enabled)
>>>>>> +    if (usb_enabled(false))
>>>>>
>>>>> Please add braces.
>>>>>
>>>>> I don't like this usb_enabled(false) way very much but I don't have
>>>>> anything better to suggest.
>>>>>
>>>>>>        n8x0_usb_setup(s);
>>>>>>
>>>>>>    if (kernel_filename) {
>>>>>> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
>>>>>> index 0c0096f..b662192 100644
>>>>>> --- a/hw/pc_piix.c
>>>>>> +++ b/hw/pc_piix.c
>>>>>> @@ -267,7 +267,7 @@ static void pc_init1(MemoryRegion *system_memory,
>>>>>>    pc_cmos_init(below_4g_mem_size, above_4g_mem_size, boot_device,
>>>>>>                 floppy, idebus[0], idebus[1], rtc_state);
>>>>>>
>>>>>> -    if (pci_enabled && usb_enabled) {
>>>>>> +    if (pci_enabled && usb_enabled(false)) {
>>>>>>        pci_create_simple(pci_bus, piix3_devfn + 2, "piix3-usb-uhci");
>>>>>>    }
>>>>>>
>>>>>> diff --git a/hw/ppc_newworld.c b/hw/ppc_newworld.c
>>>>>> index e95cfe8..1d4f494 100644
>>>>>> --- a/hw/ppc_newworld.c
>>>>>> +++ b/hw/ppc_newworld.c
>>>>>> @@ -348,10 +348,6 @@ static void ppc_core99_init (ram_addr_t ram_size,
>>>>>>    ide_mem[1] = pmac_ide_init(hd, pic[0x0d], dbdma, 0x16, pic[0x02]);
>>>>>>    ide_mem[2] = pmac_ide_init(&hd[MAX_IDE_DEVS], pic[0x0e], dbdma, 0x1a, pic[0x02]);
>>>>>>
>>>>>> -    /* cuda also initialize ADB */
>>>>>> -    if (machine_arch == ARCH_MAC99_U3) {
>>>>>> -        usb_enabled = 1;
>>>>>> -    }
>>>>>>    cuda_init(&cuda_mem, pic[0x19]);
>>>>>>
>>>>>>    adb_kbd_init(&adb_bus);
>>>>>> @@ -360,15 +356,14 @@ static void ppc_core99_init (ram_addr_t ram_size,
>>>>>>    macio_init(pci_bus, PCI_DEVICE_ID_APPLE_UNI_N_KEYL, 0, pic_mem,
>>>>>>               dbdma_mem, cuda_mem, NULL, 3, ide_mem, escc_bar);
>>>>>>
>>>>>> -    if (usb_enabled) {
>>>>>> +    if (usb_enabled(machine_arch == ARCH_MAC99_U3)) {
>>>>>>        pci_create_simple(pci_bus, -1, "pci-ohci");
>>>>>> -    }
>>>>>> -
>>>>>> -    /* U3 needs to use USB for input because Linux doesn't support via-cuda
>>>>>> -       on PPC64 */
>>>>>> -    if (machine_arch == ARCH_MAC99_U3) {
>>>>>> -        usbdevice_create("keyboard");
>>>>>> -        usbdevice_create("mouse");
>>>>>> +        /* U3 needs to use USB for input because Linux doesn't support via-cuda
>>>>>> +        on PPC64 */
>>>>>> +        if (machine_arch == ARCH_MAC99_U3) {
>>>>>> +            usbdevice_create("keyboard");
>>>>>> +            usbdevice_create("mouse");
>>>>>> +        }
>>>>>>    }
>>>>>>
>>>>>>    if (graphic_depth != 15 && graphic_depth != 32 && graphic_depth != 8)
>>>>>> diff --git a/hw/ppc_oldworld.c b/hw/ppc_oldworld.c
>>>>>> index 1dcd8a6..1468a32 100644
>>>>>> --- a/hw/ppc_oldworld.c
>>>>>> +++ b/hw/ppc_oldworld.c
>>>>>> @@ -286,7 +286,7 @@ static void ppc_heathrow_init (ram_addr_t ram_size,
>>>>>>    macio_init(pci_bus, PCI_DEVICE_ID_APPLE_343S1201, 1, pic_mem,
>>>>>>               dbdma_mem, cuda_mem, nvr, 2, ide_mem, escc_bar);
>>>>>>
>>>>>> -    if (usb_enabled) {
>>>>>> +    if (usb_enabled(false)) {
>>>>>>        pci_create_simple(pci_bus, -1, "pci-ohci");
>>>>>>    }
>>>>>>
>>>>>> diff --git a/hw/ppc_prep.c b/hw/ppc_prep.c
>>>>>> index 7a87616..feeb903 100644
>>>>>> --- a/hw/ppc_prep.c
>>>>>> +++ b/hw/ppc_prep.c
>>>>>> @@ -662,7 +662,7 @@ static void ppc_prep_init (ram_addr_t ram_size,
>>>>>>    memory_region_add_subregion(sysmem, 0xFEFF0000, xcsr);
>>>>>> #endif
>>>>>>
>>>>>> -    if (usb_enabled) {
>>>>>> +    if (usb_enabled(false)) {
>>>>>>        pci_create_simple(pci_bus, -1, "pci-ohci");
>>>>>>    }
>>>>>>
>>>>>> diff --git a/hw/pxa2xx.c b/hw/pxa2xx.c
>>>>>> index d5f1420..4787279 100644
>>>>>> --- a/hw/pxa2xx.c
>>>>>> +++ b/hw/pxa2xx.c
>>>>>> @@ -2108,7 +2108,7 @@ PXA2xxState *pxa270_init(MemoryRegion *address_space,
>>>>>>        s->ssp[i] = (SSIBus *)qdev_get_child_bus(dev, "ssi");
>>>>>>    }
>>>>>>
>>>>>> -    if (usb_enabled) {
>>>>>> +    if (usb_enabled(false)) {
>>>>>>        sysbus_create_simple("sysbus-ohci", 0x4c000000,
>>>>>>                        qdev_get_gpio_in(s->pic, PXA2XX_PIC_USBH1));
>>>>>>    }
>>>>>> @@ -2239,7 +2239,7 @@ PXA2xxState *pxa255_init(MemoryRegion *address_space, unsigned int sdram_size)
>>>>>>        s->ssp[i] = (SSIBus *)qdev_get_child_bus(dev, "ssi");
>>>>>>    }
>>>>>>
>>>>>> -    if (usb_enabled) {
>>>>>> +    if (usb_enabled(false)) {
>>>>>>        sysbus_create_simple("sysbus-ohci", 0x4c000000,
>>>>>>                        qdev_get_gpio_in(s->pic, PXA2XX_PIC_USBH1));
>>>>>>    }
>>>>>> diff --git a/hw/realview.c b/hw/realview.c
>>>>>> index 19db4d0..a8d6f97 100644
>>>>>> --- a/hw/realview.c
>>>>>> +++ b/hw/realview.c
>>>>>> @@ -227,7 +227,7 @@ static void realview_init(ram_addr_t ram_size,
>>>>>>        sysbus_connect_irq(busdev, 2, pic[50]);
>>>>>>        sysbus_connect_irq(busdev, 3, pic[51]);
>>>>>>        pci_bus = (PCIBus *)qdev_get_child_bus(dev, "pci");
>>>>>> -        if (usb_enabled) {
>>>>>> +        if (usb_enabled(false)) {
>>>>>>            pci_create_simple(pci_bus, -1, "pci-ohci");
>>>>>>        }
>>>>>>        n = drive_get_max_bus(IF_SCSI);
>>>>>> diff --git a/hw/spapr.c b/hw/spapr.c
>>>>>> index be533ee..2e31797 100644
>>>>>> --- a/hw/spapr.c
>>>>>> +++ b/hw/spapr.c
>>>>>> @@ -766,7 +766,7 @@ static void ppc_spapr_init(ram_addr_t ram_size,
>>>>>>        spapr->has_graphics = true;
>>>>>>    }
>>>>>>
>>>>>> -    if (usb_enabled) {
>>>>>> +    if (usb_enabled(spapr->has_graphics)) {
>>>>>>        pci_create_simple(QLIST_FIRST(&spapr->phbs)->host_state.bus,
>>>>>>                          -1, "pci-ohci");
>>>>>>        if (spapr->has_graphics) {
>>>>>> diff --git a/hw/versatilepb.c b/hw/versatilepb.c
>>>>>> index 7a92034..df32c8b 100644
>>>>>> --- a/hw/versatilepb.c
>>>>>> +++ b/hw/versatilepb.c
>>>>>> @@ -247,7 +247,7 @@ static void versatile_init(ram_addr_t ram_size,
>>>>>>            pci_nic_init_nofail(nd, "rtl8139", NULL);
>>>>>>        }
>>>>>>    }
>>>>>> -    if (usb_enabled) {
>>>>>> +    if (usb_enabled(false)) {
>>>>>>        pci_create_simple(pci_bus, -1, "pci-ohci");
>>>>>>    }
>>>>>>    n = drive_get_max_bus(IF_SCSI);
>>>>>> diff --git a/qemu-config.c b/qemu-config.c
>>>>>> index c05ffbc..d1a86cf 100644
>>>>>> --- a/qemu-config.c
>>>>>> +++ b/qemu-config.c
>>>>>> @@ -612,6 +612,10 @@ static QemuOptsList qemu_machine_opts = {
>>>>>>            .name = "dump-guest-core",
>>>>>>            .type = QEMU_OPT_BOOL,
>>>>>>            .help = "Include guest memory in  a core dump",
>>>>>> +        },{
>>>>>> +            .name = "usb",
>>>>>> +            .type = QEMU_OPT_BOOL,
>>>>>> +            .help = "Set on/off to enable/disable usb",
>>>>>>        },
>>>>>>        { /* End of list */ }
>>>>>>    },
>>>>>> diff --git a/sysemu.h b/sysemu.h
>>>>>> index 65552ac..be0905e 100644
>>>>>> --- a/sysemu.h
>>>>>> +++ b/sysemu.h
>>>>>> @@ -119,7 +119,6 @@ extern const char *keyboard_layout;
>>>>>> extern int win2k_install_hack;
>>>>>> extern int alt_grab;
>>>>>> extern int ctrl_grab;
>>>>>> -extern int usb_enabled;
>>>>>> extern int smp_cpus;
>>>>>> extern int max_cpus;
>>>>>> extern int cursor_hide;
>>>>>> @@ -189,4 +188,8 @@ void register_devices(void);
>>>>>> void add_boot_device_path(int32_t bootindex, DeviceState *dev,
>>>>>>                          const char *suffix);
>>>>>> char *get_boot_devices_list(uint32_t *size);
>>>>>> +
>>>>>> +bool usb_enabled(bool default_usb);
>>>>>> +void set_usb_option(bool usb_option);
>>>>>> +
>>>>>> #endif
>>>>>> diff --git a/vl.c b/vl.c
>>>>>> index 7c577fa..4702e33 100644
>>>>>> --- a/vl.c
>>>>>> +++ b/vl.c
>>>>>> @@ -203,7 +203,6 @@ CharDriverState *serial_hds[MAX_SERIAL_PORTS];
>>>>>> CharDriverState *parallel_hds[MAX_PARALLEL_PORTS];
>>>>>> CharDriverState *virtcon_hds[MAX_VIRTIO_CONSOLES];
>>>>>> int win2k_install_hack = 0;
>>>>>> -int usb_enabled = 0;
>>>>>> int singlestep = 0;
>>>>>> int smp_cpus = 1;
>>>>>> int max_cpus = 0;
>>>>>> @@ -790,6 +789,27 @@ static int parse_sandbox(QemuOpts *opts, void *opaque)
>>>>>>    return 0;
>>>>>> }
>>>>>>
>>>>>> +/*********QEMU USB setting******/
>>>>>> +bool usb_enabled(bool default_usb)
>>>>>> +{
>>>>>> +    QemuOpts *mach_opts;
>>>>>> +    mach_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
>>>>>> +    if (mach_opts) {
>>>>>> +        return qemu_opt_get_bool(mach_opts, "usb", default_usb);
>>>>>> +    }
>>>>>> +    return default_usb;
>>>>>> +}
>>>>>
>>>>> It would be more optimal to go through the options only once after
>>>>> machine selection in vl.c, setting some flags as a result. Then this
>>>>> and below functions would only check against those flags.
>>>>
>>>> This is not a performance critical code path, right? So I'd prefer to have a single point of information (machine opts) rather than yet another global variable.
>>>
>>> The flags don't have to be global, they can be static to vl.c.
>>
>> It would still be duplication of information bits that need to be synchronized. I don't see any point for the set_usb_option helper. So why bother with another variable when we have one in the machine opts?
>
> If set_usb_option() is deleted (I don't see any callers), the variable
When -usb or -usbdevice is used, this function is called.
It is as the following in my patch.
            case QEMU_OPTION_usb:
-                usb_enabled = 1;
+                set_usb_option(true);
                 break;
             case QEMU_OPTION_usbdevice:
-                usb_enabled = 1;
+                set_usb_option(true);
                 add_device_config(DEV_USB, optarg);
                 break;

> could be static to usb_enabled(). But yes, the function is probably
> called only once at board init, so caching would not make sense.
>
>>
>> Alex
>>
>>>
>>>>
>>>> Alex
>>>>
>>>>>
>>>>>> +
>>>>>> +void set_usb_option(bool usb_option)
>>>>>> +{
>>>>>> +    QemuOpts *mach_opts;
>>>>>> +    mach_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
>>>>>> +    if (mach_opts) {
>>>>>> +        qemu_opt_set_bool(mach_opts, "usb", usb_option);
>>>>>> +    }
>>>>>> +}
>>>>>> +
>>>>>> +
>>>>>> /***********************************************************/
>>>>>> /* QEMU Block devices */
>>>>>>
>>>>>> @@ -1078,8 +1098,9 @@ static int usb_device_add(const char *devname)
>>>>>>    const char *p;
>>>>>>    USBDevice *dev = NULL;
>>>>>>
>>>>>> -    if (!usb_enabled)
>>>>>> +    if (!usb_enabled(false)) {
>>>>>>        return -1;
>>>>>> +    }
>>>>>>
>>>>>>    /* drivers with .usbdevice_name entry in USBDeviceInfo */
>>>>>>    dev = usbdevice_create(devname);
>>>>>> @@ -1115,8 +1136,9 @@ static int usb_device_del(const char *devname)
>>>>>>    if (strstart(devname, "host:", &p))
>>>>>>        return usb_host_device_close(p);
>>>>>>
>>>>>> -    if (!usb_enabled)
>>>>>> +    if (!usb_enabled(false)) {
>>>>>>        return -1;
>>>>>> +    }
>>>>>>
>>>>>>    p = strchr(devname, '.');
>>>>>>    if (!p)
>>>>>> @@ -3062,10 +3084,10 @@ int main(int argc, char **argv, char **envp)
>>>>>>                }
>>>>>>                break;
>>>>>>            case QEMU_OPTION_usb:
>>>>>> -                usb_enabled = 1;
>>>>>> +                set_usb_option(true);
>>>>>>                break;
>>>>>>            case QEMU_OPTION_usbdevice:
>>>>>> -                usb_enabled = 1;
>>>>>> +                set_usb_option(true);
>>>>>>                add_device_config(DEV_USB, optarg);
>>>>>>                break;
>>>>>>            case QEMU_OPTION_device:
>>>>>> @@ -3623,7 +3645,7 @@ int main(int argc, char **argv, char **envp)
>>>>>>    current_machine = machine;
>>>>>>
>>>>>>    /* init USB devices */
>>>>>> -    if (usb_enabled) {
>>>>>> +    if (usb_enabled(false)) {
>>>>>>        if (foreach_device_config(DEV_USB, usb_parse) < 0)
>>>>>>            exit(1);
>>>>>>    }
>>>>>> --
>>>>>> 1.7.7.6
>>>>>>
>>>>>>
>>>>>

On 02.09.2012, at 09:50, Li Zhang <zhlcindy@gmail.com> wrote:

> On Tue, Aug 28, 2012 at 2:08 AM, Blue Swirl <blauwirbel@gmail.com> wrote:
>> On Mon, Aug 27, 2012 at 1:59 AM, Alexander Graf <agraf@suse.de> wrote:
>>> 
>>> 
>>> On 26.08.2012, at 10:34, Blue Swirl <blauwirbel@gmail.com> wrote:
>>> 
>>>> On Sat, Aug 25, 2012 at 2:27 PM, Alexander Graf <agraf@suse.de> wrote:
>>>>> 
>>>>> 
>>>>> On 25.08.2012, at 00:43, Blue Swirl <blauwirbel@gmail.com> wrote:
>>>>> 
>>>>>> On Wed, Aug 22, 2012 at 10:31 AM, Li Zhang <zhlcindy@gmail.com> wrote:
>>>>>>> When -usb option is used, global varible usb_enabled is set.
>>>>>>> And all the plafrom will create one USB controller according
>>>>>>> to this variable. In fact, global varibles make code hard
>>>>>>> to read.
>>>>>>> 
>>>>>>> So this patch is to remove global variable usb_enabled and
>>>>>>> add USB option in machine options. All the plaforms will get
>>>>>>> USB option value from machine options.
>>>>>>> 
>>>>>>> USB option of machine options will be set either by:
>>>>>>> * -usb
>>>>>>> * -machine type=pseries,usb=on
>>>>>>> 
>>>>>>> Both these ways can work now. They both set USB option in
>>>>>>> machine options. In the future, the first way will be removed.
>>>>>>> 
>>>>>>> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>>>>>>> ---
>>>>>>> v7->v8 :
>>>>>>> * Declare usb_enabled() and set_usb_option() in sysemu.h
>>>>>>> * Separate USB enablement on sPAPR platform.
>>>>>>> 
>>>>>>> v8->v9:
>>>>>>> * Fix usb_enable() default value on sPAPR and MAC99
>>>>>>> 
>>>>>>> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>>>>>>> 
>>>>>>> diff --git a/hw/nseries.c b/hw/nseries.c
>>>>>>> index 4df2670..c67e95a 100644
>>>>>>> --- a/hw/nseries.c
>>>>>>> +++ b/hw/nseries.c
>>>>>>> @@ -1322,7 +1322,7 @@ static void n8x0_init(ram_addr_t ram_size, const char *boot_device,
>>>>>>>   n8x0_dss_setup(s);
>>>>>>>   n8x0_cbus_setup(s);
>>>>>>>   n8x0_uart_setup(s);
>>>>>>> -    if (usb_enabled)
>>>>>>> +    if (usb_enabled(false))
>>>>>> 
>>>>>> Please add braces.
>>>>>> 
>>>>>> I don't like this usb_enabled(false) way very much but I don't have
>>>>>> anything better to suggest.
>>>>>> 
>>>>>>>       n8x0_usb_setup(s);
>>>>>>> 
>>>>>>>   if (kernel_filename) {
>>>>>>> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
>>>>>>> index 0c0096f..b662192 100644
>>>>>>> --- a/hw/pc_piix.c
>>>>>>> +++ b/hw/pc_piix.c
>>>>>>> @@ -267,7 +267,7 @@ static void pc_init1(MemoryRegion *system_memory,
>>>>>>>   pc_cmos_init(below_4g_mem_size, above_4g_mem_size, boot_device,
>>>>>>>                floppy, idebus[0], idebus[1], rtc_state);
>>>>>>> 
>>>>>>> -    if (pci_enabled && usb_enabled) {
>>>>>>> +    if (pci_enabled && usb_enabled(false)) {
>>>>>>>       pci_create_simple(pci_bus, piix3_devfn + 2, "piix3-usb-uhci");
>>>>>>>   }
>>>>>>> 
>>>>>>> diff --git a/hw/ppc_newworld.c b/hw/ppc_newworld.c
>>>>>>> index e95cfe8..1d4f494 100644
>>>>>>> --- a/hw/ppc_newworld.c
>>>>>>> +++ b/hw/ppc_newworld.c
>>>>>>> @@ -348,10 +348,6 @@ static void ppc_core99_init (ram_addr_t ram_size,
>>>>>>>   ide_mem[1] = pmac_ide_init(hd, pic[0x0d], dbdma, 0x16, pic[0x02]);
>>>>>>>   ide_mem[2] = pmac_ide_init(&hd[MAX_IDE_DEVS], pic[0x0e], dbdma, 0x1a, pic[0x02]);
>>>>>>> 
>>>>>>> -    /* cuda also initialize ADB */
>>>>>>> -    if (machine_arch == ARCH_MAC99_U3) {
>>>>>>> -        usb_enabled = 1;
>>>>>>> -    }
>>>>>>>   cuda_init(&cuda_mem, pic[0x19]);
>>>>>>> 
>>>>>>>   adb_kbd_init(&adb_bus);
>>>>>>> @@ -360,15 +356,14 @@ static void ppc_core99_init (ram_addr_t ram_size,
>>>>>>>   macio_init(pci_bus, PCI_DEVICE_ID_APPLE_UNI_N_KEYL, 0, pic_mem,
>>>>>>>              dbdma_mem, cuda_mem, NULL, 3, ide_mem, escc_bar);
>>>>>>> 
>>>>>>> -    if (usb_enabled) {
>>>>>>> +    if (usb_enabled(machine_arch == ARCH_MAC99_U3)) {
>>>>>>>       pci_create_simple(pci_bus, -1, "pci-ohci");
>>>>>>> -    }
>>>>>>> -
>>>>>>> -    /* U3 needs to use USB for input because Linux doesn't support via-cuda
>>>>>>> -       on PPC64 */
>>>>>>> -    if (machine_arch == ARCH_MAC99_U3) {
>>>>>>> -        usbdevice_create("keyboard");
>>>>>>> -        usbdevice_create("mouse");
>>>>>>> +        /* U3 needs to use USB for input because Linux doesn't support via-cuda
>>>>>>> +        on PPC64 */
>>>>>>> +        if (machine_arch == ARCH_MAC99_U3) {
>>>>>>> +            usbdevice_create("keyboard");
>>>>>>> +            usbdevice_create("mouse");
>>>>>>> +        }
>>>>>>>   }
>>>>>>> 
>>>>>>>   if (graphic_depth != 15 && graphic_depth != 32 && graphic_depth != 8)
>>>>>>> diff --git a/hw/ppc_oldworld.c b/hw/ppc_oldworld.c
>>>>>>> index 1dcd8a6..1468a32 100644
>>>>>>> --- a/hw/ppc_oldworld.c
>>>>>>> +++ b/hw/ppc_oldworld.c
>>>>>>> @@ -286,7 +286,7 @@ static void ppc_heathrow_init (ram_addr_t ram_size,
>>>>>>>   macio_init(pci_bus, PCI_DEVICE_ID_APPLE_343S1201, 1, pic_mem,
>>>>>>>              dbdma_mem, cuda_mem, nvr, 2, ide_mem, escc_bar);
>>>>>>> 
>>>>>>> -    if (usb_enabled) {
>>>>>>> +    if (usb_enabled(false)) {
>>>>>>>       pci_create_simple(pci_bus, -1, "pci-ohci");
>>>>>>>   }
>>>>>>> 
>>>>>>> diff --git a/hw/ppc_prep.c b/hw/ppc_prep.c
>>>>>>> index 7a87616..feeb903 100644
>>>>>>> --- a/hw/ppc_prep.c
>>>>>>> +++ b/hw/ppc_prep.c
>>>>>>> @@ -662,7 +662,7 @@ static void ppc_prep_init (ram_addr_t ram_size,
>>>>>>>   memory_region_add_subregion(sysmem, 0xFEFF0000, xcsr);
>>>>>>> #endif
>>>>>>> 
>>>>>>> -    if (usb_enabled) {
>>>>>>> +    if (usb_enabled(false)) {
>>>>>>>       pci_create_simple(pci_bus, -1, "pci-ohci");
>>>>>>>   }
>>>>>>> 
>>>>>>> diff --git a/hw/pxa2xx.c b/hw/pxa2xx.c
>>>>>>> index d5f1420..4787279 100644
>>>>>>> --- a/hw/pxa2xx.c
>>>>>>> +++ b/hw/pxa2xx.c
>>>>>>> @@ -2108,7 +2108,7 @@ PXA2xxState *pxa270_init(MemoryRegion *address_space,
>>>>>>>       s->ssp[i] = (SSIBus *)qdev_get_child_bus(dev, "ssi");
>>>>>>>   }
>>>>>>> 
>>>>>>> -    if (usb_enabled) {
>>>>>>> +    if (usb_enabled(false)) {
>>>>>>>       sysbus_create_simple("sysbus-ohci", 0x4c000000,
>>>>>>>                       qdev_get_gpio_in(s->pic, PXA2XX_PIC_USBH1));
>>>>>>>   }
>>>>>>> @@ -2239,7 +2239,7 @@ PXA2xxState *pxa255_init(MemoryRegion *address_space, unsigned int sdram_size)
>>>>>>>       s->ssp[i] = (SSIBus *)qdev_get_child_bus(dev, "ssi");
>>>>>>>   }
>>>>>>> 
>>>>>>> -    if (usb_enabled) {
>>>>>>> +    if (usb_enabled(false)) {
>>>>>>>       sysbus_create_simple("sysbus-ohci", 0x4c000000,
>>>>>>>                       qdev_get_gpio_in(s->pic, PXA2XX_PIC_USBH1));
>>>>>>>   }
>>>>>>> diff --git a/hw/realview.c b/hw/realview.c
>>>>>>> index 19db4d0..a8d6f97 100644
>>>>>>> --- a/hw/realview.c
>>>>>>> +++ b/hw/realview.c
>>>>>>> @@ -227,7 +227,7 @@ static void realview_init(ram_addr_t ram_size,
>>>>>>>       sysbus_connect_irq(busdev, 2, pic[50]);
>>>>>>>       sysbus_connect_irq(busdev, 3, pic[51]);
>>>>>>>       pci_bus = (PCIBus *)qdev_get_child_bus(dev, "pci");
>>>>>>> -        if (usb_enabled) {
>>>>>>> +        if (usb_enabled(false)) {
>>>>>>>           pci_create_simple(pci_bus, -1, "pci-ohci");
>>>>>>>       }
>>>>>>>       n = drive_get_max_bus(IF_SCSI);
>>>>>>> diff --git a/hw/spapr.c b/hw/spapr.c
>>>>>>> index be533ee..2e31797 100644
>>>>>>> --- a/hw/spapr.c
>>>>>>> +++ b/hw/spapr.c
>>>>>>> @@ -766,7 +766,7 @@ static void ppc_spapr_init(ram_addr_t ram_size,
>>>>>>>       spapr->has_graphics = true;
>>>>>>>   }
>>>>>>> 
>>>>>>> -    if (usb_enabled) {
>>>>>>> +    if (usb_enabled(spapr->has_graphics)) {
>>>>>>>       pci_create_simple(QLIST_FIRST(&spapr->phbs)->host_state.bus,
>>>>>>>                         -1, "pci-ohci");
>>>>>>>       if (spapr->has_graphics) {
>>>>>>> diff --git a/hw/versatilepb.c b/hw/versatilepb.c
>>>>>>> index 7a92034..df32c8b 100644
>>>>>>> --- a/hw/versatilepb.c
>>>>>>> +++ b/hw/versatilepb.c
>>>>>>> @@ -247,7 +247,7 @@ static void versatile_init(ram_addr_t ram_size,
>>>>>>>           pci_nic_init_nofail(nd, "rtl8139", NULL);
>>>>>>>       }
>>>>>>>   }
>>>>>>> -    if (usb_enabled) {
>>>>>>> +    if (usb_enabled(false)) {
>>>>>>>       pci_create_simple(pci_bus, -1, "pci-ohci");
>>>>>>>   }
>>>>>>>   n = drive_get_max_bus(IF_SCSI);
>>>>>>> diff --git a/qemu-config.c b/qemu-config.c
>>>>>>> index c05ffbc..d1a86cf 100644
>>>>>>> --- a/qemu-config.c
>>>>>>> +++ b/qemu-config.c
>>>>>>> @@ -612,6 +612,10 @@ static QemuOptsList qemu_machine_opts = {
>>>>>>>           .name = "dump-guest-core",
>>>>>>>           .type = QEMU_OPT_BOOL,
>>>>>>>           .help = "Include guest memory in  a core dump",
>>>>>>> +        },{
>>>>>>> +            .name = "usb",
>>>>>>> +            .type = QEMU_OPT_BOOL,
>>>>>>> +            .help = "Set on/off to enable/disable usb",
>>>>>>>       },
>>>>>>>       { /* End of list */ }
>>>>>>>   },
>>>>>>> diff --git a/sysemu.h b/sysemu.h
>>>>>>> index 65552ac..be0905e 100644
>>>>>>> --- a/sysemu.h
>>>>>>> +++ b/sysemu.h
>>>>>>> @@ -119,7 +119,6 @@ extern const char *keyboard_layout;
>>>>>>> extern int win2k_install_hack;
>>>>>>> extern int alt_grab;
>>>>>>> extern int ctrl_grab;
>>>>>>> -extern int usb_enabled;
>>>>>>> extern int smp_cpus;
>>>>>>> extern int max_cpus;
>>>>>>> extern int cursor_hide;
>>>>>>> @@ -189,4 +188,8 @@ void register_devices(void);
>>>>>>> void add_boot_device_path(int32_t bootindex, DeviceState *dev,
>>>>>>>                         const char *suffix);
>>>>>>> char *get_boot_devices_list(uint32_t *size);
>>>>>>> +
>>>>>>> +bool usb_enabled(bool default_usb);
>>>>>>> +void set_usb_option(bool usb_option);
>>>>>>> +
>>>>>>> #endif
>>>>>>> diff --git a/vl.c b/vl.c
>>>>>>> index 7c577fa..4702e33 100644
>>>>>>> --- a/vl.c
>>>>>>> +++ b/vl.c
>>>>>>> @@ -203,7 +203,6 @@ CharDriverState *serial_hds[MAX_SERIAL_PORTS];
>>>>>>> CharDriverState *parallel_hds[MAX_PARALLEL_PORTS];
>>>>>>> CharDriverState *virtcon_hds[MAX_VIRTIO_CONSOLES];
>>>>>>> int win2k_install_hack = 0;
>>>>>>> -int usb_enabled = 0;
>>>>>>> int singlestep = 0;
>>>>>>> int smp_cpus = 1;
>>>>>>> int max_cpus = 0;
>>>>>>> @@ -790,6 +789,27 @@ static int parse_sandbox(QemuOpts *opts, void *opaque)
>>>>>>>   return 0;
>>>>>>> }
>>>>>>> 
>>>>>>> +/*********QEMU USB setting******/
>>>>>>> +bool usb_enabled(bool default_usb)
>>>>>>> +{
>>>>>>> +    QemuOpts *mach_opts;
>>>>>>> +    mach_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
>>>>>>> +    if (mach_opts) {
>>>>>>> +        return qemu_opt_get_bool(mach_opts, "usb", default_usb);
>>>>>>> +    }
>>>>>>> +    return default_usb;
>>>>>>> +}
>>>>>> 
>>>>>> It would be more optimal to go through the options only once after
>>>>>> machine selection in vl.c, setting some flags as a result. Then this
>>>>>> and below functions would only check against those flags.
>>>>> 
>>>>> This is not a performance critical code path, right? So I'd prefer to have a single point of information (machine opts) rather than yet another global variable.
>>>> 
>>>> The flags don't have to be global, they can be static to vl.c.
>>> 
>>> It would still be duplication of information bits that need to be synchronized. I don't see any point for the set_usb_option helper. So why bother with another variable when we have one in the machine opts?
>> 
>> If set_usb_option() is deleted (I don't see any callers), the variable
> When -usb or -usbdevice is used, this function is called.
> It is as the following in my patch.
>            case QEMU_OPTION_usb:
> -                usb_enabled = 1;
> +                set_usb_option(true);
>                 break;
>             case QEMU_OPTION_usbdevice:
> -                usb_enabled = 1;
> +                set_usb_option(true);

Yes, just open-code it or move the function to vl.c.

Alex

>                 add_device_config(DEV_USB, optarg);
>                 break;
> 
>> could be static to usb_enabled(). But yes, the function is probably
>> called only once at board init, so caching would not make sense.
>> 
>>> 
>>> Alex
>>> 
>>>> 
>>>>> 
>>>>> Alex
>>>>> 
>>>>>> 
>>>>>>> +
>>>>>>> +void set_usb_option(bool usb_option)
>>>>>>> +{
>>>>>>> +    QemuOpts *mach_opts;
>>>>>>> +    mach_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
>>>>>>> +    if (mach_opts) {
>>>>>>> +        qemu_opt_set_bool(mach_opts, "usb", usb_option);
>>>>>>> +    }
>>>>>>> +}
>>>>>>> +
>>>>>>> +
>>>>>>> /***********************************************************/
>>>>>>> /* QEMU Block devices */
>>>>>>> 
>>>>>>> @@ -1078,8 +1098,9 @@ static int usb_device_add(const char *devname)
>>>>>>>   const char *p;
>>>>>>>   USBDevice *dev = NULL;
>>>>>>> 
>>>>>>> -    if (!usb_enabled)
>>>>>>> +    if (!usb_enabled(false)) {
>>>>>>>       return -1;
>>>>>>> +    }
>>>>>>> 
>>>>>>>   /* drivers with .usbdevice_name entry in USBDeviceInfo */
>>>>>>>   dev = usbdevice_create(devname);
>>>>>>> @@ -1115,8 +1136,9 @@ static int usb_device_del(const char *devname)
>>>>>>>   if (strstart(devname, "host:", &p))
>>>>>>>       return usb_host_device_close(p);
>>>>>>> 
>>>>>>> -    if (!usb_enabled)
>>>>>>> +    if (!usb_enabled(false)) {
>>>>>>>       return -1;
>>>>>>> +    }
>>>>>>> 
>>>>>>>   p = strchr(devname, '.');
>>>>>>>   if (!p)
>>>>>>> @@ -3062,10 +3084,10 @@ int main(int argc, char **argv, char **envp)
>>>>>>>               }
>>>>>>>               break;
>>>>>>>           case QEMU_OPTION_usb:
>>>>>>> -                usb_enabled = 1;
>>>>>>> +                set_usb_option(true);
>>>>>>>               break;
>>>>>>>           case QEMU_OPTION_usbdevice:
>>>>>>> -                usb_enabled = 1;
>>>>>>> +                set_usb_option(true);
>>>>>>>               add_device_config(DEV_USB, optarg);
>>>>>>>               break;
>>>>>>>           case QEMU_OPTION_device:
>>>>>>> @@ -3623,7 +3645,7 @@ int main(int argc, char **argv, char **envp)
>>>>>>>   current_machine = machine;
>>>>>>> 
>>>>>>>   /* init USB devices */
>>>>>>> -    if (usb_enabled) {
>>>>>>> +    if (usb_enabled(false)) {
>>>>>>>       if (foreach_device_config(DEV_USB, usb_parse) < 0)
>>>>>>>           exit(1);
>>>>>>>   }
>>>>>>> --
>>>>>>> 1.7.7.6
>>>>>>> 
>>>>>>> 
>>>>>> 
> 
> 
> 
> -- 
> 
> Best Regards
> -Li

On Sun, Sep 2, 2012 at 10:45 PM, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 02.09.2012, at 09:50, Li Zhang <zhlcindy@gmail.com> wrote:
>
>> On Tue, Aug 28, 2012 at 2:08 AM, Blue Swirl <blauwirbel@gmail.com> wrote:
>>> On Mon, Aug 27, 2012 at 1:59 AM, Alexander Graf <agraf@suse.de> wrote:
>>>>
>>>>
>>>> On 26.08.2012, at 10:34, Blue Swirl <blauwirbel@gmail.com> wrote:
>>>>
>>>>> On Sat, Aug 25, 2012 at 2:27 PM, Alexander Graf <agraf@suse.de> wrote:
>>>>>>
>>>>>>
>>>>>> On 25.08.2012, at 00:43, Blue Swirl <blauwirbel@gmail.com> wrote:
>>>>>>
>>>>>>> On Wed, Aug 22, 2012 at 10:31 AM, Li Zhang <zhlcindy@gmail.com> wrote:
>>>>>>>> When -usb option is used, global varible usb_enabled is set.
>>>>>>>> And all the plafrom will create one USB controller according
>>>>>>>> to this variable. In fact, global varibles make code hard
>>>>>>>> to read.
>>>>>>>>
>>>>>>>> So this patch is to remove global variable usb_enabled and
>>>>>>>> add USB option in machine options. All the plaforms will get
>>>>>>>> USB option value from machine options.
>>>>>>>>
>>>>>>>> USB option of machine options will be set either by:
>>>>>>>> * -usb
>>>>>>>> * -machine type=pseries,usb=on
>>>>>>>>
>>>>>>>> Both these ways can work now. They both set USB option in
>>>>>>>> machine options. In the future, the first way will be removed.
>>>>>>>>
>>>>>>>> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>>>>>>>> ---
>>>>>>>> v7->v8 :
>>>>>>>> * Declare usb_enabled() and set_usb_option() in sysemu.h
>>>>>>>> * Separate USB enablement on sPAPR platform.
>>>>>>>>
>>>>>>>> v8->v9:
>>>>>>>> * Fix usb_enable() default value on sPAPR and MAC99
>>>>>>>>
>>>>>>>> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>>>>>>>>
>>>>>>>> diff --git a/hw/nseries.c b/hw/nseries.c
>>>>>>>> index 4df2670..c67e95a 100644
>>>>>>>> --- a/hw/nseries.c
>>>>>>>> +++ b/hw/nseries.c
>>>>>>>> @@ -1322,7 +1322,7 @@ static void n8x0_init(ram_addr_t ram_size, const char *boot_device,
>>>>>>>>   n8x0_dss_setup(s);
>>>>>>>>   n8x0_cbus_setup(s);
>>>>>>>>   n8x0_uart_setup(s);
>>>>>>>> -    if (usb_enabled)
>>>>>>>> +    if (usb_enabled(false))
>>>>>>>
>>>>>>> Please add braces.
>>>>>>>
>>>>>>> I don't like this usb_enabled(false) way very much but I don't have
>>>>>>> anything better to suggest.
>>>>>>>
>>>>>>>>       n8x0_usb_setup(s);
>>>>>>>>
>>>>>>>>   if (kernel_filename) {
>>>>>>>> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
>>>>>>>> index 0c0096f..b662192 100644
>>>>>>>> --- a/hw/pc_piix.c
>>>>>>>> +++ b/hw/pc_piix.c
>>>>>>>> @@ -267,7 +267,7 @@ static void pc_init1(MemoryRegion *system_memory,
>>>>>>>>   pc_cmos_init(below_4g_mem_size, above_4g_mem_size, boot_device,
>>>>>>>>                floppy, idebus[0], idebus[1], rtc_state);
>>>>>>>>
>>>>>>>> -    if (pci_enabled && usb_enabled) {
>>>>>>>> +    if (pci_enabled && usb_enabled(false)) {
>>>>>>>>       pci_create_simple(pci_bus, piix3_devfn + 2, "piix3-usb-uhci");
>>>>>>>>   }
>>>>>>>>
>>>>>>>> diff --git a/hw/ppc_newworld.c b/hw/ppc_newworld.c
>>>>>>>> index e95cfe8..1d4f494 100644
>>>>>>>> --- a/hw/ppc_newworld.c
>>>>>>>> +++ b/hw/ppc_newworld.c
>>>>>>>> @@ -348,10 +348,6 @@ static void ppc_core99_init (ram_addr_t ram_size,
>>>>>>>>   ide_mem[1] = pmac_ide_init(hd, pic[0x0d], dbdma, 0x16, pic[0x02]);
>>>>>>>>   ide_mem[2] = pmac_ide_init(&hd[MAX_IDE_DEVS], pic[0x0e], dbdma, 0x1a, pic[0x02]);
>>>>>>>>
>>>>>>>> -    /* cuda also initialize ADB */
>>>>>>>> -    if (machine_arch == ARCH_MAC99_U3) {
>>>>>>>> -        usb_enabled = 1;
>>>>>>>> -    }
>>>>>>>>   cuda_init(&cuda_mem, pic[0x19]);
>>>>>>>>
>>>>>>>>   adb_kbd_init(&adb_bus);
>>>>>>>> @@ -360,15 +356,14 @@ static void ppc_core99_init (ram_addr_t ram_size,
>>>>>>>>   macio_init(pci_bus, PCI_DEVICE_ID_APPLE_UNI_N_KEYL, 0, pic_mem,
>>>>>>>>              dbdma_mem, cuda_mem, NULL, 3, ide_mem, escc_bar);
>>>>>>>>
>>>>>>>> -    if (usb_enabled) {
>>>>>>>> +    if (usb_enabled(machine_arch == ARCH_MAC99_U3)) {
>>>>>>>>       pci_create_simple(pci_bus, -1, "pci-ohci");
>>>>>>>> -    }
>>>>>>>> -
>>>>>>>> -    /* U3 needs to use USB for input because Linux doesn't support via-cuda
>>>>>>>> -       on PPC64 */
>>>>>>>> -    if (machine_arch == ARCH_MAC99_U3) {
>>>>>>>> -        usbdevice_create("keyboard");
>>>>>>>> -        usbdevice_create("mouse");
>>>>>>>> +        /* U3 needs to use USB for input because Linux doesn't support via-cuda
>>>>>>>> +        on PPC64 */
>>>>>>>> +        if (machine_arch == ARCH_MAC99_U3) {
>>>>>>>> +            usbdevice_create("keyboard");
>>>>>>>> +            usbdevice_create("mouse");
>>>>>>>> +        }
>>>>>>>>   }
>>>>>>>>
>>>>>>>>   if (graphic_depth != 15 && graphic_depth != 32 && graphic_depth != 8)
>>>>>>>> diff --git a/hw/ppc_oldworld.c b/hw/ppc_oldworld.c
>>>>>>>> index 1dcd8a6..1468a32 100644
>>>>>>>> --- a/hw/ppc_oldworld.c
>>>>>>>> +++ b/hw/ppc_oldworld.c
>>>>>>>> @@ -286,7 +286,7 @@ static void ppc_heathrow_init (ram_addr_t ram_size,
>>>>>>>>   macio_init(pci_bus, PCI_DEVICE_ID_APPLE_343S1201, 1, pic_mem,
>>>>>>>>              dbdma_mem, cuda_mem, nvr, 2, ide_mem, escc_bar);
>>>>>>>>
>>>>>>>> -    if (usb_enabled) {
>>>>>>>> +    if (usb_enabled(false)) {
>>>>>>>>       pci_create_simple(pci_bus, -1, "pci-ohci");
>>>>>>>>   }
>>>>>>>>
>>>>>>>> diff --git a/hw/ppc_prep.c b/hw/ppc_prep.c
>>>>>>>> index 7a87616..feeb903 100644
>>>>>>>> --- a/hw/ppc_prep.c
>>>>>>>> +++ b/hw/ppc_prep.c
>>>>>>>> @@ -662,7 +662,7 @@ static void ppc_prep_init (ram_addr_t ram_size,
>>>>>>>>   memory_region_add_subregion(sysmem, 0xFEFF0000, xcsr);
>>>>>>>> #endif
>>>>>>>>
>>>>>>>> -    if (usb_enabled) {
>>>>>>>> +    if (usb_enabled(false)) {
>>>>>>>>       pci_create_simple(pci_bus, -1, "pci-ohci");
>>>>>>>>   }
>>>>>>>>
>>>>>>>> diff --git a/hw/pxa2xx.c b/hw/pxa2xx.c
>>>>>>>> index d5f1420..4787279 100644
>>>>>>>> --- a/hw/pxa2xx.c
>>>>>>>> +++ b/hw/pxa2xx.c
>>>>>>>> @@ -2108,7 +2108,7 @@ PXA2xxState *pxa270_init(MemoryRegion *address_space,
>>>>>>>>       s->ssp[i] = (SSIBus *)qdev_get_child_bus(dev, "ssi");
>>>>>>>>   }
>>>>>>>>
>>>>>>>> -    if (usb_enabled) {
>>>>>>>> +    if (usb_enabled(false)) {
>>>>>>>>       sysbus_create_simple("sysbus-ohci", 0x4c000000,
>>>>>>>>                       qdev_get_gpio_in(s->pic, PXA2XX_PIC_USBH1));
>>>>>>>>   }
>>>>>>>> @@ -2239,7 +2239,7 @@ PXA2xxState *pxa255_init(MemoryRegion *address_space, unsigned int sdram_size)
>>>>>>>>       s->ssp[i] = (SSIBus *)qdev_get_child_bus(dev, "ssi");
>>>>>>>>   }
>>>>>>>>
>>>>>>>> -    if (usb_enabled) {
>>>>>>>> +    if (usb_enabled(false)) {
>>>>>>>>       sysbus_create_simple("sysbus-ohci", 0x4c000000,
>>>>>>>>                       qdev_get_gpio_in(s->pic, PXA2XX_PIC_USBH1));
>>>>>>>>   }
>>>>>>>> diff --git a/hw/realview.c b/hw/realview.c
>>>>>>>> index 19db4d0..a8d6f97 100644
>>>>>>>> --- a/hw/realview.c
>>>>>>>> +++ b/hw/realview.c
>>>>>>>> @@ -227,7 +227,7 @@ static void realview_init(ram_addr_t ram_size,
>>>>>>>>       sysbus_connect_irq(busdev, 2, pic[50]);
>>>>>>>>       sysbus_connect_irq(busdev, 3, pic[51]);
>>>>>>>>       pci_bus = (PCIBus *)qdev_get_child_bus(dev, "pci");
>>>>>>>> -        if (usb_enabled) {
>>>>>>>> +        if (usb_enabled(false)) {
>>>>>>>>           pci_create_simple(pci_bus, -1, "pci-ohci");
>>>>>>>>       }
>>>>>>>>       n = drive_get_max_bus(IF_SCSI);
>>>>>>>> diff --git a/hw/spapr.c b/hw/spapr.c
>>>>>>>> index be533ee..2e31797 100644
>>>>>>>> --- a/hw/spapr.c
>>>>>>>> +++ b/hw/spapr.c
>>>>>>>> @@ -766,7 +766,7 @@ static void ppc_spapr_init(ram_addr_t ram_size,
>>>>>>>>       spapr->has_graphics = true;
>>>>>>>>   }
>>>>>>>>
>>>>>>>> -    if (usb_enabled) {
>>>>>>>> +    if (usb_enabled(spapr->has_graphics)) {
>>>>>>>>       pci_create_simple(QLIST_FIRST(&spapr->phbs)->host_state.bus,
>>>>>>>>                         -1, "pci-ohci");
>>>>>>>>       if (spapr->has_graphics) {
>>>>>>>> diff --git a/hw/versatilepb.c b/hw/versatilepb.c
>>>>>>>> index 7a92034..df32c8b 100644
>>>>>>>> --- a/hw/versatilepb.c
>>>>>>>> +++ b/hw/versatilepb.c
>>>>>>>> @@ -247,7 +247,7 @@ static void versatile_init(ram_addr_t ram_size,
>>>>>>>>           pci_nic_init_nofail(nd, "rtl8139", NULL);
>>>>>>>>       }
>>>>>>>>   }
>>>>>>>> -    if (usb_enabled) {
>>>>>>>> +    if (usb_enabled(false)) {
>>>>>>>>       pci_create_simple(pci_bus, -1, "pci-ohci");
>>>>>>>>   }
>>>>>>>>   n = drive_get_max_bus(IF_SCSI);
>>>>>>>> diff --git a/qemu-config.c b/qemu-config.c
>>>>>>>> index c05ffbc..d1a86cf 100644
>>>>>>>> --- a/qemu-config.c
>>>>>>>> +++ b/qemu-config.c
>>>>>>>> @@ -612,6 +612,10 @@ static QemuOptsList qemu_machine_opts = {
>>>>>>>>           .name = "dump-guest-core",
>>>>>>>>           .type = QEMU_OPT_BOOL,
>>>>>>>>           .help = "Include guest memory in  a core dump",
>>>>>>>> +        },{
>>>>>>>> +            .name = "usb",
>>>>>>>> +            .type = QEMU_OPT_BOOL,
>>>>>>>> +            .help = "Set on/off to enable/disable usb",
>>>>>>>>       },
>>>>>>>>       { /* End of list */ }
>>>>>>>>   },
>>>>>>>> diff --git a/sysemu.h b/sysemu.h
>>>>>>>> index 65552ac..be0905e 100644
>>>>>>>> --- a/sysemu.h
>>>>>>>> +++ b/sysemu.h
>>>>>>>> @@ -119,7 +119,6 @@ extern const char *keyboard_layout;
>>>>>>>> extern int win2k_install_hack;
>>>>>>>> extern int alt_grab;
>>>>>>>> extern int ctrl_grab;
>>>>>>>> -extern int usb_enabled;
>>>>>>>> extern int smp_cpus;
>>>>>>>> extern int max_cpus;
>>>>>>>> extern int cursor_hide;
>>>>>>>> @@ -189,4 +188,8 @@ void register_devices(void);
>>>>>>>> void add_boot_device_path(int32_t bootindex, DeviceState *dev,
>>>>>>>>                         const char *suffix);
>>>>>>>> char *get_boot_devices_list(uint32_t *size);
>>>>>>>> +
>>>>>>>> +bool usb_enabled(bool default_usb);
>>>>>>>> +void set_usb_option(bool usb_option);
>>>>>>>> +
>>>>>>>> #endif
>>>>>>>> diff --git a/vl.c b/vl.c
>>>>>>>> index 7c577fa..4702e33 100644
>>>>>>>> --- a/vl.c
>>>>>>>> +++ b/vl.c
>>>>>>>> @@ -203,7 +203,6 @@ CharDriverState *serial_hds[MAX_SERIAL_PORTS];
>>>>>>>> CharDriverState *parallel_hds[MAX_PARALLEL_PORTS];
>>>>>>>> CharDriverState *virtcon_hds[MAX_VIRTIO_CONSOLES];
>>>>>>>> int win2k_install_hack = 0;
>>>>>>>> -int usb_enabled = 0;
>>>>>>>> int singlestep = 0;
>>>>>>>> int smp_cpus = 1;
>>>>>>>> int max_cpus = 0;
>>>>>>>> @@ -790,6 +789,27 @@ static int parse_sandbox(QemuOpts *opts, void *opaque)
>>>>>>>>   return 0;
>>>>>>>> }
>>>>>>>>
>>>>>>>> +/*********QEMU USB setting******/
>>>>>>>> +bool usb_enabled(bool default_usb)
>>>>>>>> +{
>>>>>>>> +    QemuOpts *mach_opts;
>>>>>>>> +    mach_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
>>>>>>>> +    if (mach_opts) {
>>>>>>>> +        return qemu_opt_get_bool(mach_opts, "usb", default_usb);
>>>>>>>> +    }
>>>>>>>> +    return default_usb;
>>>>>>>> +}
>>>>>>>
>>>>>>> It would be more optimal to go through the options only once after
>>>>>>> machine selection in vl.c, setting some flags as a result. Then this
>>>>>>> and below functions would only check against those flags.
>>>>>>
>>>>>> This is not a performance critical code path, right? So I'd prefer to have a single point of information (machine opts) rather than yet another global variable.
>>>>>
>>>>> The flags don't have to be global, they can be static to vl.c.
>>>>
>>>> It would still be duplication of information bits that need to be synchronized. I don't see any point for the set_usb_option helper. So why bother with another variable when we have one in the machine opts?
>>>
>>> If set_usb_option() is deleted (I don't see any callers), the variable
>> When -usb or -usbdevice is used, this function is called.
>> It is as the following in my patch.
>>            case QEMU_OPTION_usb:
>> -                usb_enabled = 1;
>> +                set_usb_option(true);
>>                 break;
>>             case QEMU_OPTION_usbdevice:
>> -                usb_enabled = 1;
>> +                set_usb_option(true);
>
> Yes, just open-code it or move the function to vl.c.
>
OK, thanks. -:)
> Alex
>
>>                 add_device_config(DEV_USB, optarg);
>>                 break;
>>
>>> could be static to usb_enabled(). But yes, the function is probably
>>> called only once at board init, so caching would not make sense.
>>>
>>>>
>>>> Alex
>>>>
>>>>>
>>>>>>
>>>>>> Alex
>>>>>>
>>>>>>>
>>>>>>>> +
>>>>>>>> +void set_usb_option(bool usb_option)
>>>>>>>> +{
>>>>>>>> +    QemuOpts *mach_opts;
>>>>>>>> +    mach_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
>>>>>>>> +    if (mach_opts) {
>>>>>>>> +        qemu_opt_set_bool(mach_opts, "usb", usb_option);
>>>>>>>> +    }
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +
>>>>>>>> /***********************************************************/
>>>>>>>> /* QEMU Block devices */
>>>>>>>>
>>>>>>>> @@ -1078,8 +1098,9 @@ static int usb_device_add(const char *devname)
>>>>>>>>   const char *p;
>>>>>>>>   USBDevice *dev = NULL;
>>>>>>>>
>>>>>>>> -    if (!usb_enabled)
>>>>>>>> +    if (!usb_enabled(false)) {
>>>>>>>>       return -1;
>>>>>>>> +    }
>>>>>>>>
>>>>>>>>   /* drivers with .usbdevice_name entry in USBDeviceInfo */
>>>>>>>>   dev = usbdevice_create(devname);
>>>>>>>> @@ -1115,8 +1136,9 @@ static int usb_device_del(const char *devname)
>>>>>>>>   if (strstart(devname, "host:", &p))
>>>>>>>>       return usb_host_device_close(p);
>>>>>>>>
>>>>>>>> -    if (!usb_enabled)
>>>>>>>> +    if (!usb_enabled(false)) {
>>>>>>>>       return -1;
>>>>>>>> +    }
>>>>>>>>
>>>>>>>>   p = strchr(devname, '.');
>>>>>>>>   if (!p)
>>>>>>>> @@ -3062,10 +3084,10 @@ int main(int argc, char **argv, char **envp)
>>>>>>>>               }
>>>>>>>>               break;
>>>>>>>>           case QEMU_OPTION_usb:
>>>>>>>> -                usb_enabled = 1;
>>>>>>>> +                set_usb_option(true);
>>>>>>>>               break;
>>>>>>>>           case QEMU_OPTION_usbdevice:
>>>>>>>> -                usb_enabled = 1;
>>>>>>>> +                set_usb_option(true);
>>>>>>>>               add_device_config(DEV_USB, optarg);
>>>>>>>>               break;
>>>>>>>>           case QEMU_OPTION_device:
>>>>>>>> @@ -3623,7 +3645,7 @@ int main(int argc, char **argv, char **envp)
>>>>>>>>   current_machine = machine;
>>>>>>>>
>>>>>>>>   /* init USB devices */
>>>>>>>> -    if (usb_enabled) {
>>>>>>>> +    if (usb_enabled(false)) {
>>>>>>>>       if (foreach_device_config(DEV_USB, usb_parse) < 0)
>>>>>>>>           exit(1);
>>>>>>>>   }
>>>>>>>> --
>>>>>>>> 1.7.7.6
>>>>>>>>
>>>>>>>>
>>>>>>>
>>
>>
>>
>> --
>>
>> Best Regards
>> -Li