[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [Qemu-devel][PATCH v9 1/1] Add USB option in machine opti
From: |
Li Zhang |
Subject: |
Re: [Qemu-ppc] [Qemu-devel][PATCH v9 1/1] Add USB option in machine options |
Date: |
Mon, 3 Sep 2012 09:34:24 +0800 |
On Sun, Sep 2, 2012 at 10:45 PM, Alexander Graf <address@hidden> wrote:
>
>
> On 02.09.2012, at 09:50, Li Zhang <address@hidden> wrote:
>
>> On Tue, Aug 28, 2012 at 2:08 AM, Blue Swirl <address@hidden> wrote:
>>> On Mon, Aug 27, 2012 at 1:59 AM, Alexander Graf <address@hidden> wrote:
>>>>
>>>>
>>>> On 26.08.2012, at 10:34, Blue Swirl <address@hidden> wrote:
>>>>
>>>>> On Sat, Aug 25, 2012 at 2:27 PM, Alexander Graf <address@hidden> wrote:
>>>>>>
>>>>>>
>>>>>> On 25.08.2012, at 00:43, Blue Swirl <address@hidden> wrote:
>>>>>>
>>>>>>> On Wed, Aug 22, 2012 at 10:31 AM, Li Zhang <address@hidden> 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 <address@hidden>
>>>>>>>> ---
>>>>>>>> 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 <address@hidden>
>>>>>>>>
>>>>>>>> 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