From f4bbaaf584ed8d0a430b467bace15f338cba4c57 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 17 Feb 2015 14:28:02 +0100 Subject: [PATCH 01/13] usb: Propagate errors through usb_register_companion() This loses the messages explaining the error printed with error_printf_unless_qmp(). The next commit will make up for the loss. Signed-off-by: Markus Armbruster Signed-off-by: Gerd Hoffmann --- hw/usb/bus.c | 17 ++++++++++------- hw/usb/hcd-ehci.c | 23 +++++++++++++---------- hw/usb/hcd-ohci.c | 10 +++++++--- hw/usb/hcd-uhci.c | 10 +++++++--- include/hw/usb.h | 12 +++++++----- 5 files changed, 44 insertions(+), 28 deletions(-) diff --git a/hw/usb/bus.c b/hw/usb/bus.c index 91fc3e20d9..98e33ea31a 100644 --- a/hw/usb/bus.c +++ b/hw/usb/bus.c @@ -360,9 +360,10 @@ void usb_register_port(USBBus *bus, USBPort *port, void *opaque, int index, bus->nfree++; } -int usb_register_companion(const char *masterbus, USBPort *ports[], - uint32_t portcount, uint32_t firstport, - void *opaque, USBPortOps *ops, int speedmask) +void usb_register_companion(const char *masterbus, USBPort *ports[], + uint32_t portcount, uint32_t firstport, + void *opaque, USBPortOps *ops, int speedmask, + Error **errp) { USBBus *bus; int i; @@ -374,21 +375,23 @@ int usb_register_companion(const char *masterbus, USBPort *ports[], } if (!bus || !bus->ops->register_companion) { - qerror_report(QERR_INVALID_PARAMETER_VALUE, "masterbus", - "an USB masterbus"); + error_set(errp, QERR_INVALID_PARAMETER_VALUE, "masterbus", + "an USB masterbus"); +#if 0 /* conversion from qerror_report() to error_set() broke this: */ if (bus) { error_printf_unless_qmp( "USB bus '%s' does not allow companion controllers\n", masterbus); } - return -1; +#endif + return; } for (i = 0; i < portcount; i++) { usb_fill_port(ports[i], opaque, i, ops, speedmask); } - return bus->ops->register_companion(bus, ports, portcount, firstport); + bus->ops->register_companion(bus, ports, portcount, firstport, errp); } void usb_port_location(USBPort *downstream, USBPort *upstream, int portnr) diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c index ccf54b6e09..7d16ba83bf 100644 --- a/hw/usb/hcd-ehci.c +++ b/hw/usb/hcd-ehci.c @@ -769,30 +769,35 @@ static void ehci_wakeup(USBPort *port) qemu_bh_schedule(s->async_bh); } -static int ehci_register_companion(USBBus *bus, USBPort *ports[], - uint32_t portcount, uint32_t firstport) +static void ehci_register_companion(USBBus *bus, USBPort *ports[], + uint32_t portcount, uint32_t firstport, + Error **errp) { EHCIState *s = container_of(bus, EHCIState, bus); uint32_t i; if (firstport + portcount > NB_PORTS) { - qerror_report(QERR_INVALID_PARAMETER_VALUE, "firstport", - "firstport on masterbus"); + error_set(errp, QERR_INVALID_PARAMETER_VALUE, "firstport", + "firstport on masterbus"); +#if 0 /* conversion from qerror_report() to error_set() broke this: */ error_printf_unless_qmp( "firstport value of %u makes companion take ports %u - %u, which " "is outside of the valid range of 0 - %u\n", firstport, firstport, firstport + portcount - 1, NB_PORTS - 1); - return -1; +#endif + return; } for (i = 0; i < portcount; i++) { if (s->companion_ports[firstport + i]) { - qerror_report(QERR_INVALID_PARAMETER_VALUE, "masterbus", - "an USB masterbus"); + error_set(errp, QERR_INVALID_PARAMETER_VALUE, "masterbus", + "an USB masterbus"); +#if 0 /* conversion from qerror_report() to error_set() broke this: */ error_printf_unless_qmp( "port %u on masterbus %s already has a companion assigned\n", firstport + i, bus->qbus.name); - return -1; +#endif + return; } } @@ -806,8 +811,6 @@ static int ehci_register_companion(USBBus *bus, USBPort *ports[], s->companion_count++; s->caps[0x05] = (s->companion_count << 4) | portcount; - - return 0; } static void ehci_wakeup_endpoint(USBBus *bus, USBEndpoint *ep, diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c index a0d478e63e..21ec65f14b 100644 --- a/hw/usb/hcd-ohci.c +++ b/hw/usb/hcd-ohci.c @@ -1832,6 +1832,7 @@ static int usb_ohci_init(OHCIState *ohci, DeviceState *dev, char *masterbus, uint32_t firstport, AddressSpace *as) { + Error *err = NULL; int i; ohci->as = as; @@ -1857,9 +1858,12 @@ static int usb_ohci_init(OHCIState *ohci, DeviceState *dev, for(i = 0; i < num_ports; i++) { ports[i] = &ohci->rhport[i].port; } - if (usb_register_companion(masterbus, ports, num_ports, - firstport, ohci, &ohci_port_ops, - USB_SPEED_MASK_LOW | USB_SPEED_MASK_FULL) != 0) { + usb_register_companion(masterbus, ports, num_ports, + firstport, ohci, &ohci_port_ops, + USB_SPEED_MASK_LOW | USB_SPEED_MASK_FULL, + &err); + if (err) { + error_report_err(err); return -1; } } else { diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c index f903de7072..2ca8de3268 100644 --- a/hw/usb/hcd-uhci.c +++ b/hw/usb/hcd-uhci.c @@ -1192,6 +1192,7 @@ static USBBusOps uhci_bus_ops = { static int usb_uhci_common_initfn(PCIDevice *dev) { + Error *err = NULL; PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev); UHCIPCIDeviceClass *u = container_of(pc, UHCIPCIDeviceClass, parent_class); UHCIState *s = DO_UPCAST(UHCIState, dev, dev); @@ -1209,9 +1210,12 @@ static int usb_uhci_common_initfn(PCIDevice *dev) for(i = 0; i < NB_PORTS; i++) { ports[i] = &s->ports[i].port; } - if (usb_register_companion(s->masterbus, ports, NB_PORTS, - s->firstport, s, &uhci_port_ops, - USB_SPEED_MASK_LOW | USB_SPEED_MASK_FULL) != 0) { + usb_register_companion(s->masterbus, ports, NB_PORTS, + s->firstport, s, &uhci_port_ops, + USB_SPEED_MASK_LOW | USB_SPEED_MASK_FULL, + &err); + if (err) { + error_report_err(err); return -1; } } else { diff --git a/include/hw/usb.h b/include/hw/usb.h index e6dfb87e76..5be29375a2 100644 --- a/include/hw/usb.h +++ b/include/hw/usb.h @@ -526,8 +526,9 @@ struct USBBus { }; struct USBBusOps { - int (*register_companion)(USBBus *bus, USBPort *ports[], - uint32_t portcount, uint32_t firstport); + void (*register_companion)(USBBus *bus, USBPort *ports[], + uint32_t portcount, uint32_t firstport, + Error **errp); void (*wakeup_endpoint)(USBBus *bus, USBEndpoint *ep, unsigned int stream); }; @@ -543,9 +544,10 @@ USBDevice *usb_create_simple(USBBus *bus, const char *name); USBDevice *usbdevice_create(const char *cmdline); void usb_register_port(USBBus *bus, USBPort *port, void *opaque, int index, USBPortOps *ops, int speedmask); -int usb_register_companion(const char *masterbus, USBPort *ports[], - uint32_t portcount, uint32_t firstport, - void *opaque, USBPortOps *ops, int speedmask); +void usb_register_companion(const char *masterbus, USBPort *ports[], + uint32_t portcount, uint32_t firstport, + void *opaque, USBPortOps *ops, int speedmask, + Error **errp); void usb_port_location(USBPort *downstream, USBPort *upstream, int portnr); void usb_unregister_port(USBBus *bus, USBPort *port); void usb_claim_port(USBDevice *dev, Error **errp); From 2e269f3d9d806987977b3c76deb26647f2bf33e1 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 17 Feb 2015 14:28:03 +0100 Subject: [PATCH 02/13] usb: Improve companion configuration error messages The previous commit broke the additional messages explaining the error messages. Improve the error messages, so they don't need explaining so much. Helps QMP users as well, unlike additional explanations. Signed-off-by: Markus Armbruster Signed-off-by: Gerd Hoffmann --- hw/usb/bus.c | 18 ++++++++---------- hw/usb/hcd-ehci.c | 21 ++++++--------------- 2 files changed, 14 insertions(+), 25 deletions(-) diff --git a/hw/usb/bus.c b/hw/usb/bus.c index 98e33ea31a..375167573d 100644 --- a/hw/usb/bus.c +++ b/hw/usb/bus.c @@ -374,16 +374,14 @@ void usb_register_companion(const char *masterbus, USBPort *ports[], } } - if (!bus || !bus->ops->register_companion) { - error_set(errp, QERR_INVALID_PARAMETER_VALUE, "masterbus", - "an USB masterbus"); -#if 0 /* conversion from qerror_report() to error_set() broke this: */ - if (bus) { - error_printf_unless_qmp( - "USB bus '%s' does not allow companion controllers\n", - masterbus); - } -#endif + if (!bus) { + error_setg(errp, "USB bus '%s' not found", masterbus); + return; + } + if (!bus->ops->register_companion) { + error_setg(errp, "Can't use USB bus '%s' as masterbus," + " it doesn't support companion controllers", + masterbus); return; } diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c index 7d16ba83bf..5c2a452b97 100644 --- a/hw/usb/hcd-ehci.c +++ b/hw/usb/hcd-ehci.c @@ -777,26 +777,17 @@ static void ehci_register_companion(USBBus *bus, USBPort *ports[], uint32_t i; if (firstport + portcount > NB_PORTS) { - error_set(errp, QERR_INVALID_PARAMETER_VALUE, "firstport", - "firstport on masterbus"); -#if 0 /* conversion from qerror_report() to error_set() broke this: */ - error_printf_unless_qmp( - "firstport value of %u makes companion take ports %u - %u, which " - "is outside of the valid range of 0 - %u\n", firstport, firstport, - firstport + portcount - 1, NB_PORTS - 1); -#endif + error_setg(errp, "firstport must be between 0 and %u", + NB_PORTS - portcount); return; } for (i = 0; i < portcount; i++) { if (s->companion_ports[firstport + i]) { - error_set(errp, QERR_INVALID_PARAMETER_VALUE, "masterbus", - "an USB masterbus"); -#if 0 /* conversion from qerror_report() to error_set() broke this: */ - error_printf_unless_qmp( - "port %u on masterbus %s already has a companion assigned\n", - firstport + i, bus->qbus.name); -#endif + error_setg(errp, "firstport %u asks for ports %u-%u," + " but port %u has a companion assigned already", + firstport, firstport, firstport + portcount - 1, + firstport + i); return; } } From 87581feaa112733e8d999ade8a4d08816e908268 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 17 Feb 2015 14:28:04 +0100 Subject: [PATCH 03/13] ohci: Complete conversion to realize Commit 457215ec "ohci: Use QOM realize for OHCI" converted only "sysbus-ohci". Finish the job: convert "pci-ohci". Signed-off-by: Markus Armbruster Signed-off-by: Gerd Hoffmann --- hw/usb/hcd-ohci.c | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c index 21ec65f14b..e180a178af 100644 --- a/hw/usb/hcd-ohci.c +++ b/hw/usb/hcd-ohci.c @@ -1827,10 +1827,10 @@ static USBPortOps ohci_port_ops = { static USBBusOps ohci_bus_ops = { }; -static int usb_ohci_init(OHCIState *ohci, DeviceState *dev, - int num_ports, dma_addr_t localmem_base, - char *masterbus, uint32_t firstport, - AddressSpace *as) +static void usb_ohci_init(OHCIState *ohci, DeviceState *dev, + int num_ports, dma_addr_t localmem_base, + char *masterbus, uint32_t firstport, + AddressSpace *as, Error **errp) { Error *err = NULL; int i; @@ -1863,8 +1863,8 @@ static int usb_ohci_init(OHCIState *ohci, DeviceState *dev, USB_SPEED_MASK_LOW | USB_SPEED_MASK_FULL, &err); if (err) { - error_report_err(err); - return -1; + error_propagate(errp, err); + return; } } else { usb_bus_new(&ohci->bus, sizeof(ohci->bus), &ohci_bus_ops, dev); @@ -1884,8 +1884,6 @@ static int usb_ohci_init(OHCIState *ohci, DeviceState *dev, ohci->async_td = 0; qemu_register_reset(ohci_reset, ohci); - - return 0; } #define TYPE_PCI_OHCI "pci-ohci" @@ -1918,22 +1916,24 @@ static void ohci_die(OHCIState *ohci) PCI_STATUS_DETECTED_PARITY); } -static int usb_ohci_initfn_pci(PCIDevice *dev) +static void usb_ohci_realize_pci(PCIDevice *dev, Error **errp) { + Error *err = NULL; OHCIPCIState *ohci = PCI_OHCI(dev); dev->config[PCI_CLASS_PROG] = 0x10; /* OHCI */ dev->config[PCI_INTERRUPT_PIN] = 0x01; /* interrupt pin A */ - if (usb_ohci_init(&ohci->state, DEVICE(dev), ohci->num_ports, 0, - ohci->masterbus, ohci->firstport, - pci_get_address_space(dev)) != 0) { - return -1; + usb_ohci_init(&ohci->state, DEVICE(dev), ohci->num_ports, 0, + ohci->masterbus, ohci->firstport, + pci_get_address_space(dev), &err); + if (err) { + error_propagate(errp, err); + return; } - ohci->state.irq = pci_allocate_irq(dev); + ohci->state.irq = pci_allocate_irq(dev); pci_register_bar(dev, 0, 0, &ohci->state.mem); - return 0; } static void usb_ohci_exit(PCIDevice *dev) @@ -1975,7 +1975,7 @@ static void ohci_realize_pxa(DeviceState *dev, Error **errp) /* Cannot fail as we pass NULL for masterbus */ usb_ohci_init(&s->ohci, dev, s->num_ports, s->dma_offset, NULL, 0, - &address_space_memory); + &address_space_memory, &error_abort); sysbus_init_irq(sbd, &s->ohci.irq); sysbus_init_mmio(sbd, &s->ohci.mem); } @@ -2091,7 +2091,7 @@ static void ohci_pci_class_init(ObjectClass *klass, void *data) DeviceClass *dc = DEVICE_CLASS(klass); PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); - k->init = usb_ohci_initfn_pci; + k->realize = usb_ohci_realize_pci; k->exit = usb_ohci_exit; k->vendor_id = PCI_VENDOR_ID_APPLE; k->device_id = PCI_DEVICE_ID_APPLE_IPID_USB; From 63216dc78d2d52448afdfbe0e0bacb2c669083b4 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 17 Feb 2015 14:28:05 +0100 Subject: [PATCH 04/13] uhci: Convert to realize Signed-off-by: Markus Armbruster Signed-off-by: Gerd Hoffmann --- hw/usb/hcd-uhci.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c index 2ca8de3268..e791377eeb 100644 --- a/hw/usb/hcd-uhci.c +++ b/hw/usb/hcd-uhci.c @@ -66,7 +66,7 @@ struct UHCIInfo { uint16_t device_id; uint8_t revision; uint8_t irq_pin; - int (*initfn)(PCIDevice *dev); + void (*realize)(PCIDevice *dev, Error **errp); bool unplug; }; @@ -1190,7 +1190,7 @@ static USBPortOps uhci_port_ops = { static USBBusOps uhci_bus_ops = { }; -static int usb_uhci_common_initfn(PCIDevice *dev) +static void usb_uhci_common_realize(PCIDevice *dev, Error **errp) { Error *err = NULL; PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev); @@ -1215,8 +1215,8 @@ static int usb_uhci_common_initfn(PCIDevice *dev) USB_SPEED_MASK_LOW | USB_SPEED_MASK_FULL, &err); if (err) { - error_report_err(err); - return -1; + error_propagate(errp, err); + return; } } else { usb_bus_new(&s->bus, sizeof(s->bus), &uhci_bus_ops, DEVICE(dev)); @@ -1238,11 +1238,9 @@ static int usb_uhci_common_initfn(PCIDevice *dev) /* Use region 4 for consistency with real hardware. BSD guests seem to rely on this. */ pci_register_bar(&s->dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &s->io_bar); - - return 0; } -static int usb_uhci_vt82c686b_initfn(PCIDevice *dev) +static void usb_uhci_vt82c686b_realize(PCIDevice *dev, Error **errp) { UHCIState *s = DO_UPCAST(UHCIState, dev, dev); uint8_t *pci_conf = s->dev.config; @@ -1254,7 +1252,7 @@ static int usb_uhci_vt82c686b_initfn(PCIDevice *dev) /* USB legacy support */ pci_set_long(pci_conf + 0xc0,0x00002000); - return usb_uhci_common_initfn(dev); + usb_uhci_common_realize(dev, errp); } static void usb_uhci_exit(PCIDevice *dev) @@ -1300,7 +1298,7 @@ static void uhci_class_init(ObjectClass *klass, void *data) UHCIPCIDeviceClass *u = container_of(k, UHCIPCIDeviceClass, parent_class); UHCIInfo *info = data; - k->init = info->initfn ? info->initfn : usb_uhci_common_initfn; + k->realize = info->realize ? info->realize : usb_uhci_common_realize; k->exit = info->unplug ? usb_uhci_exit : NULL; k->vendor_id = info->vendor_id; k->device_id = info->device_id; @@ -1339,7 +1337,7 @@ static UHCIInfo uhci_info[] = { .device_id = PCI_DEVICE_ID_VIA_UHCI, .revision = 0x01, .irq_pin = 3, - .initfn = usb_uhci_vt82c686b_initfn, + .realize = usb_uhci_vt82c686b_realize, .unplug = true, },{ .name = "ich9-usb-uhci1", /* 00:1d.0 */ From bcf5d19c59a527c91bc29704f3e4956119c050cf Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Thu, 12 Mar 2015 17:26:46 +0100 Subject: [PATCH 05/13] monitor: Drop dead QMP check from monitor_read_password() Function is only called in HMP context since commit 333a96e "qapi: Convert change". Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Signed-off-by: Gerd Hoffmann --- monitor.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/monitor.c b/monitor.c index 8b703f97be..1b46d0718d 100644 --- a/monitor.c +++ b/monitor.c @@ -266,10 +266,7 @@ void monitor_read_command(Monitor *mon, int show_prompt) int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func, void *opaque) { - if (monitor_ctrl_mode(mon)) { - qerror_report(QERR_MISSING_PARAMETER, "password"); - return -EINVAL; - } else if (mon->rs) { + if (mon->rs) { readline_start(mon->rs, "Password: ", 1, readline_func, opaque); /* prompt is printed on return from the command handler */ return 0; From 988e0f06621fde11ec0d319a6fd0ab3ccef0602f Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Thu, 12 Mar 2015 17:26:47 +0100 Subject: [PATCH 06/13] monitor: Plug memory leak in monitor_read_bdrv_key_start() Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Signed-off-by: Gerd Hoffmann --- monitor.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/monitor.c b/monitor.c index 1b46d0718d..bc774152a1 100644 --- a/monitor.c +++ b/monitor.c @@ -5391,9 +5391,11 @@ int monitor_read_bdrv_key_start(Monitor *mon, BlockDriverState *bs, if (monitor_ctrl_mode(mon)) { qerror_report_err(local_err); + error_free(local_err); return -1; } + error_free(local_err); monitor_printf(mon, "%s (%s) is encrypted.\n", bdrv_get_device_name(bs), bdrv_get_encrypted_filename(bs)); From 9b14e0efcc9a6ea41b7265538f6ec4c53e2ba270 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Thu, 12 Mar 2015 17:26:48 +0100 Subject: [PATCH 07/13] monitor usb: Inline monitor_read_bdrv_key_start()'s first part monitor_read_bdrv_key_start() does several things: 1. If no key is needed, call completion_cb() and succeed 2. If we're in QMP context, call qerror_report_err() and fail 3. Start reading the key in the monitor. This is two things too many. Inline 1. and 2. into its callers monitor_read_block_device_key() and usb_msd_realize_storage(). Since monitor_read_block_device_key() only ever runs in HMP context, drop 2. there. The next commit will clean up the result in usb_msd_realize_storage(). Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Signed-off-by: Gerd Hoffmann --- hw/usb/dev-storage.c | 13 +++++++++++-- monitor.c | 29 +++++++++++------------------ 2 files changed, 22 insertions(+), 20 deletions(-) diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c index dacefd71a5..f47c8561ef 100644 --- a/hw/usb/dev-storage.c +++ b/hw/usb/dev-storage.c @@ -641,8 +641,17 @@ static void usb_msd_realize_storage(USBDevice *dev, Error **errp) if (bdrv_key_required(blk_bs(blk))) { if (cur_mon) { - monitor_read_bdrv_key_start(cur_mon, blk_bs(blk), - usb_msd_password_cb, s); + bdrv_add_key(blk_bs(blk), NULL, &err); + if (!err) { + usb_msd_password_cb(s, 0); + } else if (monitor_cur_is_qmp()) { + qerror_report_err(err); + error_free(err); + } else { + error_free(err); + monitor_read_bdrv_key_start(cur_mon, blk_bs(blk), + usb_msd_password_cb, s); + } s->dev.auto_attach = 0; } else { autostart = 0; diff --git a/monitor.c b/monitor.c index bc774152a1..61c00ac2fb 100644 --- a/monitor.c +++ b/monitor.c @@ -5377,25 +5377,8 @@ int monitor_read_bdrv_key_start(Monitor *mon, BlockDriverState *bs, BlockCompletionFunc *completion_cb, void *opaque) { - Error *local_err = NULL; int err; - bdrv_add_key(bs, NULL, &local_err); - if (!local_err) { - if (completion_cb) - completion_cb(opaque, 0); - return 0; - } - - /* Need a key for @bs */ - - if (monitor_ctrl_mode(mon)) { - qerror_report_err(local_err); - error_free(local_err); - return -1; - } - - error_free(local_err); monitor_printf(mon, "%s (%s) is encrypted.\n", bdrv_get_device_name(bs), bdrv_get_encrypted_filename(bs)); @@ -5414,6 +5397,7 @@ int monitor_read_block_device_key(Monitor *mon, const char *device, BlockCompletionFunc *completion_cb, void *opaque) { + Error *err = NULL; BlockBackend *blk; blk = blk_by_name(device); @@ -5422,7 +5406,16 @@ int monitor_read_block_device_key(Monitor *mon, const char *device, return -1; } - return monitor_read_bdrv_key_start(mon, blk_bs(blk), completion_cb, opaque); + bdrv_add_key(blk_bs(blk), NULL, &err); + if (err) { + error_free(err); + return monitor_read_bdrv_key_start(mon, blk_bs(blk), completion_cb, opaque); + } + + if (completion_cb) { + completion_cb(opaque, 0); + } + return 0; } QemuOptsList qemu_mon_opts = { From 7afcc1f9bae3e857834a3bb8247be101e2354998 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Thu, 12 Mar 2015 17:26:49 +0100 Subject: [PATCH 08/13] usb/dev-storage: Fix QMP device_add missing encryption key failure When the image is encrypted, QMP device_add creates the device, defers actually attaching it to when the key becomes available, then returns an error. This is wrong. device_add must either create the device and succeed, or do nothing and fail. The bug is in usb_msd_realize_storage(). It posts an error with qerror_report_err(), and returns success. Device realization relies on the return value, and completes. The QMP monitor, however, relies on the posted error, and sends it in an error reply. Reproducer: $ qemu-system-x86_64 -nodefaults -display none -usb -qmp stdio -drive if=none,id=foo,file=geheim.qcow2 {"QMP": {"version": {"qemu": {"micro": 50, "minor": 2, "major": 2}, "package": ""}, "capabilities": []}} { "execute": "qmp_capabilities" } {"return": {}} { "execute": "device_add", "arguments": { "driver": "usb-storage", "id": "bar", "drive": "foo" } } {"error": {"class": "DeviceEncrypted", "desc": "'foo' (geheim.qcow2) is encrypted"}} Even though we got an error back, the device got created just fine. To demonstrate, let's unplug it again: {"execute":"device_del","arguments": { "id": "bar" } } {"timestamp": {"seconds": 1426003440, "microseconds": 237181}, "event": "DEVICE_DELETED", "data": {"path": "/machine/peripheral/bar/bar.0/legacy[0]"}} {"timestamp": {"seconds": 1426003440, "microseconds": 238231}, "event": "DEVICE_DELETED", "data": {"device": "bar", "path": "/machine/peripheral/bar"}} {"return": {}} Fix by making usb_msd_realize_storage() fail properly. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Signed-off-by: Gerd Hoffmann --- hw/usb/dev-storage.c | 36 +++++++++++++++++------------------- 1 file changed, 17 insertions(+), 19 deletions(-) diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c index f47c8561ef..f50bcb83e2 100644 --- a/hw/usb/dev-storage.c +++ b/hw/usb/dev-storage.c @@ -610,6 +610,23 @@ static void usb_msd_realize_storage(USBDevice *dev, Error **errp) return; } + bdrv_add_key(blk_bs(blk), NULL, &err); + if (err) { + if (monitor_cur_is_qmp()) { + error_propagate(errp, err); + return; + } + error_free(err); + err = NULL; + if (cur_mon) { + monitor_read_bdrv_key_start(cur_mon, blk_bs(blk), + usb_msd_password_cb, s); + s->dev.auto_attach = 0; + } else { + autostart = 0; + } + } + blkconf_serial(&s->conf, &dev->serial); blkconf_blocksizes(&s->conf); @@ -638,25 +655,6 @@ static void usb_msd_realize_storage(USBDevice *dev, Error **errp) } usb_msd_handle_reset(dev); s->scsi_dev = scsi_dev; - - if (bdrv_key_required(blk_bs(blk))) { - if (cur_mon) { - bdrv_add_key(blk_bs(blk), NULL, &err); - if (!err) { - usb_msd_password_cb(s, 0); - } else if (monitor_cur_is_qmp()) { - qerror_report_err(err); - error_free(err); - } else { - error_free(err); - monitor_read_bdrv_key_start(cur_mon, blk_bs(blk), - usb_msd_password_cb, s); - } - s->dev.auto_attach = 0; - } else { - autostart = 0; - } - } } static void usb_msd_realize_bot(USBDevice *dev, Error **errp) From c326529b74aa37adb216604bb6ca93cd49007012 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Thu, 12 Mar 2015 17:26:50 +0100 Subject: [PATCH 09/13] usb/dev-storage: Avoid qerror_report_err() outside QMP handlers qerror_report_err() is a transitional interface to help with converting existing monitor commands to QMP. It should not be used elsewhere. usb_msd_password_cb() is only called from within an HMP command handler. Replace by error_report_err(). Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Signed-off-by: Gerd Hoffmann --- hw/usb/dev-storage.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c index f50bcb83e2..ae8d40dc77 100644 --- a/hw/usb/dev-storage.c +++ b/hw/usb/dev-storage.c @@ -559,8 +559,7 @@ static void usb_msd_password_cb(void *opaque, int err) } if (local_err) { - qerror_report_err(local_err); - error_free(local_err); + error_report_err(local_err); qdev_unplug(&s->dev.qdev, NULL); } } From 8ffd9f4dd41f0423f0df8bef8f2e25ab4bb1a3f3 Mon Sep 17 00:00:00 2001 From: Thomas Huth Date: Tue, 17 Mar 2015 14:52:54 +0100 Subject: [PATCH 10/13] hw/usb: Include USB files only if necessary Boards that do not include an USB controller should not provide USB devices. However, when running "qemu-system-s390x -device help" for example, there's still a usb-hub, usb-kbd, usb-mouse and usb-tablet in the list of "supported" devices. Let's fix that by compiling and linking the USB files only if it is really necessary. Signed-off-by: Thomas Huth Signed-off-by: Gerd Hoffmann --- default-configs/arm-softmmu.mak | 1 + default-configs/usb.mak | 1 + hw/usb/Makefile.objs | 8 ++++---- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak index 87d4e34d15..a767e4b708 100644 --- a/default-configs/arm-softmmu.mak +++ b/default-configs/arm-softmmu.mak @@ -32,6 +32,7 @@ CONFIG_DS1338=y CONFIG_PFLASH_CFI01=y CONFIG_PFLASH_CFI02=y CONFIG_MICRODRIVE=y +CONFIG_USB=y CONFIG_USB_MUSB=y CONFIG_USB_EHCI_SYSBUS=y CONFIG_PLATFORM_BUS=y diff --git a/default-configs/usb.mak b/default-configs/usb.mak index 73d84895aa..f4b85684f0 100644 --- a/default-configs/usb.mak +++ b/default-configs/usb.mak @@ -1,3 +1,4 @@ +CONFIG_USB=y CONFIG_USB_TABLET_WACOM=y CONFIG_USB_STORAGE_BOT=y CONFIG_USB_STORAGE_UAS=y diff --git a/hw/usb/Makefile.objs b/hw/usb/Makefile.objs index 0ccd477577..7443e386b3 100644 --- a/hw/usb/Makefile.objs +++ b/hw/usb/Makefile.objs @@ -1,6 +1,6 @@ # usb subsystem core -common-obj-y += core.o combined-packet.o bus.o desc.o desc-msos.o -common-obj-y += libhw.o +common-obj-y += core.o combined-packet.o bus.o libhw.o +common-obj-$(CONFIG_USB) += desc.o desc-msos.o # usb host adapters common-obj-$(CONFIG_USB_UHCI) += hcd-uhci.o @@ -11,8 +11,8 @@ common-obj-$(CONFIG_USB_XHCI) += hcd-xhci.o common-obj-$(CONFIG_USB_MUSB) += hcd-musb.o # emulated usb devices -common-obj-y += dev-hub.o -common-obj-y += dev-hid.o +common-obj-$(CONFIG_USB) += dev-hub.o +common-obj-$(CONFIG_USB) += dev-hid.o common-obj-$(CONFIG_USB_TABLET_WACOM) += dev-wacom.o common-obj-$(CONFIG_USB_STORAGE_BOT) += dev-storage.o common-obj-$(CONFIG_USB_STORAGE_UAS) += dev-uas.o From 537e572a7f807d7371a73ea5ffd9ce8d2487ff0c Mon Sep 17 00:00:00 2001 From: Gonglei Date: Wed, 18 Mar 2015 17:33:46 +0800 Subject: [PATCH 11/13] uhci: fix segfault when hot-unplugging uhci controller When hot-unplugging the usb controllers (ehci/uhci), we have to clean all resouce of these devices, involved registered reset handler. Otherwise, it may cause NULL pointer access and/or segmentation fault if we reboot the guest os after hot-unplugging. Let's hook up reset via DeviceClass->reset() and drop the qemu_register_reset() call. Then Qemu will register and unregister the reset handler automatically. Cc: qemu-stable Reported-by: Lidonglin Signed-off-by: Gonglei Signed-off-by: Gerd Hoffmann --- hw/usb/hcd-uhci.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c index e791377eeb..327f26da70 100644 --- a/hw/usb/hcd-uhci.c +++ b/hw/usb/hcd-uhci.c @@ -348,9 +348,10 @@ static void uhci_update_irq(UHCIState *s) pci_set_irq(&s->dev, level); } -static void uhci_reset(void *opaque) +static void uhci_reset(DeviceState *dev) { - UHCIState *s = opaque; + PCIDevice *d = PCI_DEVICE(dev); + UHCIState *s = DO_UPCAST(UHCIState, dev, d); uint8_t *pci_conf; int i; UHCIPort *port; @@ -454,11 +455,11 @@ static void uhci_port_write(void *opaque, hwaddr addr, port = &s->ports[i]; usb_device_reset(port->port.dev); } - uhci_reset(s); + uhci_reset(DEVICE(s)); return; } if (val & UHCI_CMD_HCRESET) { - uhci_reset(s); + uhci_reset(DEVICE(s)); return; } s->cmd = val; @@ -1230,8 +1231,6 @@ static void usb_uhci_common_realize(PCIDevice *dev, Error **errp) s->num_ports_vmstate = NB_PORTS; QTAILQ_INIT(&s->queues); - qemu_register_reset(uhci_reset, s); - memory_region_init_io(&s->io_bar, OBJECT(s), &uhci_ioport_ops, s, "uhci", 0x20); @@ -1305,6 +1304,7 @@ static void uhci_class_init(ObjectClass *klass, void *data) k->revision = info->revision; k->class_id = PCI_CLASS_SERIAL_USB; dc->vmsd = &vmstate_uhci; + dc->reset = uhci_reset; if (!info->unplug) { /* uhci controllers in companion setups can't be hotplugged */ dc->hotpluggable = false; From 88dd1b8d0063ff16c54dc19c8b52508a00108f50 Mon Sep 17 00:00:00 2001 From: Gonglei Date: Wed, 18 Mar 2015 17:33:48 +0800 Subject: [PATCH 12/13] ohci: fix resource cleanup leak When hot-unplugging the usb controllers (ehci/uhci), we have to clean all resouce of these devices, involved registered reset handler. Otherwise, it may cause NULL pointer access and/or segmentation fault if we reboot the guest os after hot-unplugging. Let's hook up reset via DeviceClass->reset() and drop the qemu_register_reset() call. Then Qemu will register and unregister the reset handler automatically. Ohci does't support hotplugging/hotunplugging yet, but existing resource cleanup leak logic likes ehci/uhci. Cc: qemu-stable Signed-off-by: Gonglei Signed-off-by: Gerd Hoffmann --- hw/usb/hcd-ohci.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c index e180a178af..1a22c9c0cb 100644 --- a/hw/usb/hcd-ohci.c +++ b/hw/usb/hcd-ohci.c @@ -1883,7 +1883,6 @@ static void usb_ohci_init(OHCIState *ohci, DeviceState *dev, usb_packet_init(&ohci->usb_packet); ohci->async_td = 0; - qemu_register_reset(ohci_reset, ohci); } #define TYPE_PCI_OHCI "pci-ohci" @@ -1955,6 +1954,15 @@ static void usb_ohci_exit(PCIDevice *dev) } } +static void usb_ohci_reset_pci(DeviceState *d) +{ + PCIDevice *dev = PCI_DEVICE(d); + OHCIPCIState *ohci = PCI_OHCI(dev); + OHCIState *s = &ohci->state; + + ohci_reset(s); +} + #define TYPE_SYSBUS_OHCI "sysbus-ohci" #define SYSBUS_OHCI(obj) OBJECT_CHECK(OHCISysBusState, (obj), TYPE_SYSBUS_OHCI) @@ -1980,6 +1988,14 @@ static void ohci_realize_pxa(DeviceState *dev, Error **errp) sysbus_init_mmio(sbd, &s->ohci.mem); } +static void usb_ohci_reset_sysbus(DeviceState *dev) +{ + OHCISysBusState *s = SYSBUS_OHCI(dev); + OHCIState *ohci = &s->ohci; + + ohci_reset(ohci); +} + static Property ohci_pci_properties[] = { DEFINE_PROP_STRING("masterbus", OHCIPCIState, masterbus), DEFINE_PROP_UINT32("num-ports", OHCIPCIState, num_ports, 3), @@ -2101,6 +2117,7 @@ static void ohci_pci_class_init(ObjectClass *klass, void *data) dc->props = ohci_pci_properties; dc->hotpluggable = false; dc->vmsd = &vmstate_ohci; + dc->reset = usb_ohci_reset_pci; } static const TypeInfo ohci_pci_info = { @@ -2124,6 +2141,7 @@ static void ohci_sysbus_class_init(ObjectClass *klass, void *data) set_bit(DEVICE_CATEGORY_USB, dc->categories); dc->desc = "OHCI USB Controller"; dc->props = ohci_sysbus_properties; + dc->reset = usb_ohci_reset_sysbus; } static const TypeInfo ohci_sysbus_info = { From 4e289b1b62c8e271e3400317b4c3d98909093bc4 Mon Sep 17 00:00:00 2001 From: Gonglei Date: Wed, 18 Mar 2015 17:33:47 +0800 Subject: [PATCH 13/13] ehci: fix segfault when hot-unplugging ehci controller When hot-unplugging the usb controllers (ehci/uhci), we have to clean all resouce of these devices, involved registered reset handler. Otherwise, it may cause NULL pointer access and/or segmentation fault if we reboot the guest os after hot-unplugging. Let's hook up reset via DeviceClass->reset() and drop the qemu_register_reset() call. Then Qemu will register and unregister the reset handler automatically. Cc: qemu-stable Reported-by: Lidonglin Signed-off-by: Gonglei Signed-off-by: Gerd Hoffmann --- hw/usb/hcd-ehci-pci.c | 10 ++++++++++ hw/usb/hcd-ehci-sysbus.c | 10 ++++++++++ hw/usb/hcd-ehci.c | 3 +-- hw/usb/hcd-ehci.h | 1 + 4 files changed, 22 insertions(+), 2 deletions(-) diff --git a/hw/usb/hcd-ehci-pci.c b/hw/usb/hcd-ehci-pci.c index 4c80707f85..7afa5f9d67 100644 --- a/hw/usb/hcd-ehci-pci.c +++ b/hw/usb/hcd-ehci-pci.c @@ -101,6 +101,15 @@ static void usb_ehci_pci_exit(PCIDevice *dev) } } +static void usb_ehci_pci_reset(DeviceState *dev) +{ + PCIDevice *pci_dev = PCI_DEVICE(dev); + EHCIPCIState *i = PCI_EHCI(pci_dev); + EHCIState *s = &i->ehci; + + ehci_reset(s); +} + static void usb_ehci_pci_write_config(PCIDevice *dev, uint32_t addr, uint32_t val, int l) { @@ -143,6 +152,7 @@ static void ehci_class_init(ObjectClass *klass, void *data) k->config_write = usb_ehci_pci_write_config; dc->vmsd = &vmstate_ehci_pci; dc->props = ehci_pci_properties; + dc->reset = usb_ehci_pci_reset; } static const TypeInfo ehci_pci_type_info = { diff --git a/hw/usb/hcd-ehci-sysbus.c b/hw/usb/hcd-ehci-sysbus.c index 19ed2c26aa..cd1cc142ab 100644 --- a/hw/usb/hcd-ehci-sysbus.c +++ b/hw/usb/hcd-ehci-sysbus.c @@ -42,6 +42,15 @@ static void usb_ehci_sysbus_realize(DeviceState *dev, Error **errp) sysbus_init_irq(d, &s->irq); } +static void usb_ehci_sysbus_reset(DeviceState *dev) +{ + SysBusDevice *d = SYS_BUS_DEVICE(dev); + EHCISysBusState *i = SYS_BUS_EHCI(d); + EHCIState *s = &i->ehci; + + ehci_reset(s); +} + static void ehci_sysbus_init(Object *obj) { SysBusDevice *d = SYS_BUS_DEVICE(obj); @@ -70,6 +79,7 @@ static void ehci_sysbus_class_init(ObjectClass *klass, void *data) dc->realize = usb_ehci_sysbus_realize; dc->vmsd = &vmstate_ehci_sysbus; dc->props = ehci_sysbus_properties; + dc->reset = usb_ehci_sysbus_reset; set_bit(DEVICE_CATEGORY_USB, dc->categories); } diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c index 5c2a452b97..d4d754765b 100644 --- a/hw/usb/hcd-ehci.c +++ b/hw/usb/hcd-ehci.c @@ -839,7 +839,7 @@ static USBDevice *ehci_find_device(EHCIState *ehci, uint8_t addr) } /* 4.1 host controller initialization */ -static void ehci_reset(void *opaque) +void ehci_reset(void *opaque) { EHCIState *s = opaque; int i; @@ -2465,7 +2465,6 @@ void usb_ehci_realize(EHCIState *s, DeviceState *dev, Error **errp) s->async_bh = qemu_bh_new(ehci_frame_timer, s); s->device = dev; - qemu_register_reset(ehci_reset, s); s->vmstate = qemu_add_vm_change_state_handler(usb_ehci_vm_state_change, s); } diff --git a/hw/usb/hcd-ehci.h b/hw/usb/hcd-ehci.h index 2bc259c9b4..87b240f70a 100644 --- a/hw/usb/hcd-ehci.h +++ b/hw/usb/hcd-ehci.h @@ -325,6 +325,7 @@ extern const VMStateDescription vmstate_ehci; void usb_ehci_init(EHCIState *s, DeviceState *dev); void usb_ehci_realize(EHCIState *s, DeviceState *dev, Error **errp); void usb_ehci_unrealize(EHCIState *s, DeviceState *dev, Error **errp); +void ehci_reset(void *opaque); #define TYPE_PCI_EHCI "pci-ehci-usb" #define PCI_EHCI(obj) OBJECT_CHECK(EHCIPCIState, (obj), TYPE_PCI_EHCI)