On Sun, Sep 2, 2012 at 10:45 PM, Alexander Graf <ag...@suse.de> wrote: > > > On 02.09.2012, at 09:50, Li Zhang <zhlci...@gmail.com> wrote: > >> On Tue, Aug 28, 2012 at 2:08 AM, Blue Swirl <blauwir...@gmail.com> wrote: >>> On Mon, Aug 27, 2012 at 1:59 AM, Alexander Graf <ag...@suse.de> wrote: >>>> >>>> >>>> On 26.08.2012, at 10:34, Blue Swirl <blauwir...@gmail.com> wrote: >>>> >>>>> On Sat, Aug 25, 2012 at 2:27 PM, Alexander Graf <ag...@suse.de> wrote: >>>>>> >>>>>> >>>>>> On 25.08.2012, at 00:43, Blue Swirl <blauwir...@gmail.com> wrote: >>>>>> >>>>>>> On Wed, Aug 22, 2012 at 10:31 AM, Li Zhang <zhlci...@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 <zhlci...@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 <zhlci...@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
-- Best Regards -Li