Message ID | 1345631518-32758-1-git-send-email-zhlcindy@linux.vnet.ibm.com |
---|---|
State | New |
Headers | show |
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
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)) 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; +} + +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); }