From 5ec7d09818881b87052c41259e5cb781683977d2 Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann Date: Fri, 24 Jun 2016 13:35:17 +0200 Subject: [PATCH 01/30] xen: fix ram init regression Commit "8156d48 pc: allow raising low memory via max-ram-below-4g option" causes a regression on xen, because it uses a different memory split. This patch initializes max-ram-below-4g to zero and leaves the initialization to the memory initialization functions. That way they can pick different default values (max-ram-below-4g is zero still) or use the user supplied value (max-ram-below-4g is non-zero). Also skip the whole ram split calculation on Xen. xen_ram_init() does its own split calculation anyway so it is superfluous, also this way xen_ram_init can actually see whenever max-ram-below-4g is zero or not. Reported-by: Anthony PERARD Tested-by: Anthony PERARD Signed-off-by: Gerd Hoffmann Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/i386/pc.c | 2 +- hw/i386/pc_piix.c | 52 ++++++++++++++++++++++++++--------------------- hw/i386/pc_q35.c | 3 +++ xen-hvm.c | 3 +++ 4 files changed, 36 insertions(+), 24 deletions(-) diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 44a8f3bcbd..cd1745ebaf 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -1919,7 +1919,7 @@ static void pc_machine_initfn(Object *obj) pc_machine_get_hotplug_memory_region_size, NULL, NULL, NULL, &error_abort); - pcms->max_ram_below_4g = 0xe0000000; /* 3.5G */ + pcms->max_ram_below_4g = 0; /* use default */ object_property_add(obj, PC_MACHINE_MAX_RAM_BELOW_4G, "size", pc_machine_get_max_ram_below_4g, pc_machine_set_max_ram_below_4g, diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c index c7d70af253..a07dc816bf 100644 --- a/hw/i386/pc_piix.c +++ b/hw/i386/pc_piix.c @@ -108,37 +108,43 @@ static void pc_init1(MachineState *machine, * so legacy non-PAE guests can get as much memory as possible in * the 32bit address space below 4G. * + * - Note that Xen has its own ram setp code in xen_ram_init(), + * called via xen_hvm_init(). + * * Examples: * qemu -M pc-1.7 -m 4G (old default) -> 3584M low, 512M high * qemu -M pc -m 4G (new default) -> 3072M low, 1024M high * qemu -M pc,max-ram-below-4g=2G -m 4G -> 2048M low, 2048M high * qemu -M pc,max-ram-below-4g=4G -m 3968M -> 3968M low (=4G-128M) */ - lowmem = pcms->max_ram_below_4g; - if (machine->ram_size >= pcms->max_ram_below_4g) { - if (pcmc->gigabyte_align) { - if (lowmem > 0xc0000000) { - lowmem = 0xc0000000; - } - if (lowmem & ((1ULL << 30) - 1)) { - error_report("Warning: Large machine and max_ram_below_4g " - "(%" PRIu64 ") not a multiple of 1G; " - "possible bad performance.", - pcms->max_ram_below_4g); - } - } - } - - if (machine->ram_size >= lowmem) { - pcms->above_4g_mem_size = machine->ram_size - lowmem; - pcms->below_4g_mem_size = lowmem; - } else { - pcms->above_4g_mem_size = 0; - pcms->below_4g_mem_size = machine->ram_size; - } - if (xen_enabled()) { xen_hvm_init(pcms, &ram_memory); + } else { + if (!pcms->max_ram_below_4g) { + pcms->max_ram_below_4g = 0xe0000000; /* default: 3.5G */ + } + lowmem = pcms->max_ram_below_4g; + if (machine->ram_size >= pcms->max_ram_below_4g) { + if (pcmc->gigabyte_align) { + if (lowmem > 0xc0000000) { + lowmem = 0xc0000000; + } + if (lowmem & ((1ULL << 30) - 1)) { + error_report("Warning: Large machine and max_ram_below_4g " + "(%" PRIu64 ") not a multiple of 1G; " + "possible bad performance.", + pcms->max_ram_below_4g); + } + } + } + + if (machine->ram_size >= lowmem) { + pcms->above_4g_mem_size = machine->ram_size - lowmem; + pcms->below_4g_mem_size = lowmem; + } else { + pcms->above_4g_mem_size = 0; + pcms->below_4g_mem_size = machine->ram_size; + } } pc_cpus_init(pcms); diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c index 04b2684d37..cd57bced1a 100644 --- a/hw/i386/pc_q35.c +++ b/hw/i386/pc_q35.c @@ -94,6 +94,9 @@ static void pc_q35_init(MachineState *machine) /* Handle the machine opt max-ram-below-4g. It is basically doing * min(qemu limit, user limit). */ + if (!pcms->max_ram_below_4g) { + pcms->max_ram_below_4g = 1ULL << 32; /* default: 4G */; + } if (lowmem > pcms->max_ram_below_4g) { lowmem = pcms->max_ram_below_4g; if (machine->ram_size - lowmem > lowmem && diff --git a/xen-hvm.c b/xen-hvm.c index 98ea44fdf3..eb577926a1 100644 --- a/xen-hvm.c +++ b/xen-hvm.c @@ -190,6 +190,9 @@ static void xen_ram_init(PCMachineState *pcms, /* Handle the machine opt max-ram-below-4g. It is basically doing * min(xen limit, user limit). */ + if (!user_lowmem) { + user_lowmem = HVM_BELOW_4G_RAM_END; /* default */ + } if (HVM_BELOW_4G_RAM_END <= user_lowmem) { user_lowmem = HVM_BELOW_4G_RAM_END; } From 1b04cc801adff9ddf5aca9afe0e19df54290ca2c Mon Sep 17 00:00:00 2001 From: Marcel Apfelbaum Date: Mon, 27 Jun 2016 18:38:31 +0300 Subject: [PATCH 02/30] hw/ppc: realize the PCI root bus as part of mac99 init Mac99's PCI root bus is not part of a host bridge, realize it manually. Signed-off-by: Marcel Apfelbaum Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/ppc/mac_newworld.c | 1 + 1 file changed, 1 insertion(+) diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c index 32e88b3786..7d2510658d 100644 --- a/hw/ppc/mac_newworld.c +++ b/hw/ppc/mac_newworld.c @@ -380,6 +380,7 @@ static void ppc_core99_init(MachineState *machine) pci_bus = pci_pmac_init(pic, get_system_memory(), get_system_io()); machine_arch = ARCH_MAC99; } + object_property_set_bool(OBJECT(pci_bus), true, "realized", &error_abort); machine->usb |= defaults_enabled() && !machine->usb_disabled; From b86eacb804bdb92acc48cab8cd2a465714a829ab Mon Sep 17 00:00:00 2001 From: Marcel Apfelbaum Date: Mon, 27 Jun 2016 18:38:32 +0300 Subject: [PATCH 03/30] hw/pci: delay bus_master_enable_region initialization Skip bus_master_enable region creation on PCI device init in order to be sure the IOMMU device (if present) would be created in advance. Add this memory region at machine_done time. Signed-off-by: Marcel Apfelbaum Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/pci/pci.c | 41 +++++++++++++++++++++++++++++++--------- include/hw/pci/pci_bus.h | 2 ++ include/sysemu/sysemu.h | 1 + vl.c | 5 +++++ 4 files changed, 40 insertions(+), 9 deletions(-) diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 4b585f47b6..ee385eba91 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -78,10 +78,37 @@ static const VMStateDescription vmstate_pcibus = { } }; +static void pci_init_bus_master(PCIDevice *pci_dev) +{ + AddressSpace *dma_as = pci_device_iommu_address_space(pci_dev); + + memory_region_init_alias(&pci_dev->bus_master_enable_region, + OBJECT(pci_dev), "bus master", + dma_as->root, 0, memory_region_size(dma_as->root)); + memory_region_set_enabled(&pci_dev->bus_master_enable_region, false); + address_space_init(&pci_dev->bus_master_as, + &pci_dev->bus_master_enable_region, pci_dev->name); +} + +static void pcibus_machine_done(Notifier *notifier, void *data) +{ + PCIBus *bus = container_of(notifier, PCIBus, machine_done); + int i; + + for (i = 0; i < ARRAY_SIZE(bus->devices); ++i) { + if (bus->devices[i]) { + pci_init_bus_master(bus->devices[i]); + } + } +} + static void pci_bus_realize(BusState *qbus, Error **errp) { PCIBus *bus = PCI_BUS(qbus); + bus->machine_done.notify = pcibus_machine_done; + qemu_add_machine_init_done_notifier(&bus->machine_done); + vmstate_register(NULL, -1, &vmstate_pcibus, bus); } @@ -89,6 +116,8 @@ static void pci_bus_unrealize(BusState *qbus, Error **errp) { PCIBus *bus = PCI_BUS(qbus); + qemu_remove_machine_init_done_notifier(&bus->machine_done); + vmstate_unregister(NULL, &vmstate_pcibus, bus); } @@ -920,7 +949,6 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus, PCIConfigReadFunc *config_read = pc->config_read; PCIConfigWriteFunc *config_write = pc->config_write; Error *local_err = NULL; - AddressSpace *dma_as; DeviceState *dev = DEVICE(pci_dev); pci_dev->bus = bus; @@ -961,15 +989,10 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus, pci_dev->devfn = devfn; pci_dev->requester_id_cache = pci_req_id_cache_get(pci_dev); - dma_as = pci_device_iommu_address_space(pci_dev); - - memory_region_init_alias(&pci_dev->bus_master_enable_region, - OBJECT(pci_dev), "bus master", - dma_as->root, 0, memory_region_size(dma_as->root)); - memory_region_set_enabled(&pci_dev->bus_master_enable_region, false); - address_space_init(&pci_dev->bus_master_as, &pci_dev->bus_master_enable_region, - name); + if (qdev_hotplug) { + pci_init_bus_master(pci_dev); + } pstrcpy(pci_dev->name, sizeof(pci_dev->name), name); pci_dev->irq_state = 0; pci_config_alloc(pci_dev); diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h index 403fec6e58..5484a9b5c5 100644 --- a/include/hw/pci/pci_bus.h +++ b/include/hw/pci/pci_bus.h @@ -39,6 +39,8 @@ struct PCIBus { Keep a count of the number of devices with raised IRQs. */ int nirq; int *irq_count; + + Notifier machine_done; }; typedef struct PCIBridgeWindows PCIBridgeWindows; diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h index 7313673b66..ee7c7608e0 100644 --- a/include/sysemu/sysemu.h +++ b/include/sysemu/sysemu.h @@ -75,6 +75,7 @@ void qemu_add_exit_notifier(Notifier *notify); void qemu_remove_exit_notifier(Notifier *notify); void qemu_add_machine_init_done_notifier(Notifier *notify); +void qemu_remove_machine_init_done_notifier(Notifier *notify); void hmp_savevm(Monitor *mon, const QDict *qdict); int load_vmstate(const char *name); diff --git a/vl.c b/vl.c index 9bb7f4cd70..5cd0f2a7db 100644 --- a/vl.c +++ b/vl.c @@ -2675,6 +2675,11 @@ void qemu_add_machine_init_done_notifier(Notifier *notify) } } +void qemu_remove_machine_init_done_notifier(Notifier *notify) +{ + notifier_remove(notify); +} + static void qemu_run_machine_init_done_notifiers(void) { notifier_list_notify(&machine_init_done_notifiers, NULL); From bf8d492405feaee2c1685b3b9d5e03228ed3e47f Mon Sep 17 00:00:00 2001 From: Marcel Apfelbaum Date: Mon, 27 Jun 2016 18:38:33 +0300 Subject: [PATCH 04/30] q35: allow dynamic sysbus Allow adding sysbus devices with -device on Q35. At first Q35 will support only intel-iommu to be added this way, however the command line will support all sysbus devices. Mark with 'cannot_instantiate_with_device_add_yet' the ones causing immediate problems (e.g. crashes). Signed-off-by: Marcel Apfelbaum Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/i386/pc_q35.c | 1 + hw/pci-bridge/pci_expander_bridge.c | 2 ++ hw/pci-host/piix.c | 2 ++ hw/pci-host/q35.c | 2 ++ 4 files changed, 7 insertions(+) diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c index cd57bced1a..fbaf2e6ca2 100644 --- a/hw/i386/pc_q35.c +++ b/hw/i386/pc_q35.c @@ -290,6 +290,7 @@ static void pc_q35_machine_options(MachineClass *m) m->default_machine_opts = "firmware=bios-256k.bin"; m->default_display = "std"; m->no_floppy = 1; + m->has_dynamic_sysbus = true; } static void pc_q35_2_7_machine_options(MachineClass *m) diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c index ba320bd857..ab8612158d 100644 --- a/hw/pci-bridge/pci_expander_bridge.c +++ b/hw/pci-bridge/pci_expander_bridge.c @@ -149,6 +149,8 @@ static void pxb_host_class_init(ObjectClass *class, void *data) PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(class); dc->fw_name = "pci"; + /* Reason: Internal part of the pxb/pxb-pcie device, not usable by itself */ + dc->cannot_instantiate_with_device_add_yet = true; sbc->explicit_ofw_unit_address = pxb_host_ofw_unit_address; hc->root_bus_path = pxb_host_root_bus_path; } diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c index df2b0e26f5..7167e58a12 100644 --- a/hw/pci-host/piix.c +++ b/hw/pci-host/piix.c @@ -865,6 +865,8 @@ static void i440fx_pcihost_class_init(ObjectClass *klass, void *data) dc->realize = i440fx_pcihost_realize; dc->fw_name = "pci"; dc->props = i440fx_props; + /* Reason: needs to be wired up by pc_init1 */ + dc->cannot_instantiate_with_device_add_yet = true; } static const TypeInfo i440fx_pcihost_info = { diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c index 03be05dc0d..015003906a 100644 --- a/hw/pci-host/q35.c +++ b/hw/pci-host/q35.c @@ -142,6 +142,8 @@ static void q35_host_class_init(ObjectClass *klass, void *data) hc->root_bus_path = q35_host_root_bus_path; dc->realize = q35_host_realize; dc->props = mch_props; + /* Reason: needs to be wired up by pc_q35_init */ + dc->cannot_instantiate_with_device_add_yet = true; set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories); dc->fw_name = "pci"; } From 621d983a1f9051f4cfc3f402569b46b77d8449fc Mon Sep 17 00:00:00 2001 From: Marcel Apfelbaum Date: Mon, 27 Jun 2016 18:38:34 +0300 Subject: [PATCH 05/30] hw/iommu: enable iommu with -device Use the standard '-device intel-iommu' to create the IOMMU device. The legacy '-machine,iommu=on' can still be used. Signed-off-by: Marcel Apfelbaum Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/i386/intel_iommu.c | 16 ++++++++++++++++ hw/i386/pc_q35.c | 1 - hw/pci-host/q35.c | 17 +---------------- 3 files changed, 17 insertions(+), 17 deletions(-) diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index 5eba704477..464f2a0518 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -25,6 +25,7 @@ #include "intel_iommu_internal.h" #include "hw/pci/pci.h" #include "hw/pci/pci_bus.h" +#include "hw/i386/pc.h" /*#define DEBUG_INTEL_IOMMU*/ #ifdef DEBUG_INTEL_IOMMU @@ -2026,8 +2027,20 @@ static void vtd_reset(DeviceState *dev) vtd_init(s); } +static AddressSpace *vtd_host_dma_iommu(PCIBus *bus, void *opaque, int devfn) +{ + IntelIOMMUState *s = opaque; + VTDAddressSpace *vtd_as; + + assert(0 <= devfn && devfn <= VTD_PCI_DEVFN_MAX); + + vtd_as = vtd_find_add_as(s, bus, devfn); + return &vtd_as->as; +} + static void vtd_realize(DeviceState *dev, Error **errp) { + PCIBus *bus = PC_MACHINE(qdev_get_machine())->bus; IntelIOMMUState *s = INTEL_IOMMU_DEVICE(dev); VTD_DPRINTF(GENERAL, ""); @@ -2041,6 +2054,8 @@ static void vtd_realize(DeviceState *dev, Error **errp) s->vtd_as_by_busptr = g_hash_table_new_full(vtd_uint64_hash, vtd_uint64_equal, g_free, g_free); vtd_init(s); + sysbus_mmio_map(SYS_BUS_DEVICE(s), 0, Q35_HOST_BRIDGE_IOMMU_ADDR); + pci_setup_iommu(bus, vtd_host_dma_iommu, dev); } static void vtd_class_init(ObjectClass *klass, void *data) @@ -2051,6 +2066,7 @@ static void vtd_class_init(ObjectClass *klass, void *data) dc->realize = vtd_realize; dc->vmsd = &vtd_vmstate; dc->props = vtd_properties; + dc->hotpluggable = false; } static const TypeInfo vtd_info = { diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c index fbaf2e6ca2..c0b9961928 100644 --- a/hw/i386/pc_q35.c +++ b/hw/i386/pc_q35.c @@ -179,7 +179,6 @@ static void pc_q35_init(MachineState *machine) qdev_init_nofail(DEVICE(q35_host)); phb = PCI_HOST_BRIDGE(q35_host); host_bus = phb->bus; - pcms->bus = phb->bus; /* create ISA bus */ lpc = pci_create_simple_multifunction(host_bus, PCI_DEVFN(ICH9_LPC_DEV, ICH9_LPC_FUNC), true, diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c index 015003906a..8d060a5b98 100644 --- a/hw/pci-host/q35.c +++ b/hw/pci-host/q35.c @@ -52,6 +52,7 @@ static void q35_host_realize(DeviceState *dev, Error **errp) pci->bus = pci_bus_new(DEVICE(s), "pcie.0", s->mch.pci_address_space, s->mch.address_space_io, 0, TYPE_PCIE_BUS); + PC_MACHINE(qdev_get_machine())->bus = pci->bus; qdev_set_parent_bus(DEVICE(&s->mch), BUS(pci->bus)); qdev_init_nofail(DEVICE(&s->mch)); } @@ -446,28 +447,12 @@ static void mch_reset(DeviceState *qdev) mch_update(mch); } -static AddressSpace *q35_host_dma_iommu(PCIBus *bus, void *opaque, int devfn) -{ - IntelIOMMUState *s = opaque; - VTDAddressSpace *vtd_as; - - assert(0 <= devfn && devfn <= VTD_PCI_DEVFN_MAX); - - vtd_as = vtd_find_add_as(s, bus, devfn); - return &vtd_as->as; -} - static void mch_init_dmar(MCHPCIState *mch) { - PCIBus *pci_bus = PCI_BUS(qdev_get_parent_bus(DEVICE(mch))); - mch->iommu = INTEL_IOMMU_DEVICE(qdev_create(NULL, TYPE_INTEL_IOMMU_DEVICE)); object_property_add_child(OBJECT(mch), "intel-iommu", OBJECT(mch->iommu), NULL); qdev_init_nofail(DEVICE(mch->iommu)); - sysbus_mmio_map(SYS_BUS_DEVICE(mch->iommu), 0, Q35_HOST_BRIDGE_IOMMU_ADDR); - - pci_setup_iommu(pci_bus, q35_host_dma_iommu, mch->iommu); } static void mch_realize(PCIDevice *d, Error **errp) From 10d01f73e39100701028c7badd6ece52990cf758 Mon Sep 17 00:00:00 2001 From: Marcel Apfelbaum Date: Mon, 27 Jun 2016 18:38:35 +0300 Subject: [PATCH 06/30] machine: remove iommu property Since iommu devices can be created with '-device' there is no need to keep iommu as machine and mch property. Signed-off-by: Marcel Apfelbaum Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/core/machine.c | 20 -------------------- hw/pci-host/q35.c | 12 ------------ include/hw/pci-host/q35.h | 1 - qemu-options.hx | 3 --- 4 files changed, 36 deletions(-) diff --git a/hw/core/machine.c b/hw/core/machine.c index ccdd5fa3e7..8f943013e5 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -300,20 +300,6 @@ static void machine_set_firmware(Object *obj, const char *value, Error **errp) ms->firmware = g_strdup(value); } -static bool machine_get_iommu(Object *obj, Error **errp) -{ - MachineState *ms = MACHINE(obj); - - return ms->iommu; -} - -static void machine_set_iommu(Object *obj, bool value, Error **errp) -{ - MachineState *ms = MACHINE(obj); - - ms->iommu = value; -} - static void machine_set_suppress_vmdesc(Object *obj, bool value, Error **errp) { MachineState *ms = MACHINE(obj); @@ -493,12 +479,6 @@ static void machine_initfn(Object *obj) object_property_set_description(obj, "firmware", "Firmware image", NULL); - object_property_add_bool(obj, "iommu", - machine_get_iommu, - machine_set_iommu, NULL); - object_property_set_description(obj, "iommu", - "Set on/off to enable/disable Intel IOMMU (VT-d)", - NULL); object_property_add_bool(obj, "suppress-vmdesc", machine_get_suppress_vmdesc, machine_set_suppress_vmdesc, NULL); diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c index 8d060a5b98..eb1b2f77b7 100644 --- a/hw/pci-host/q35.c +++ b/hw/pci-host/q35.c @@ -447,14 +447,6 @@ static void mch_reset(DeviceState *qdev) mch_update(mch); } -static void mch_init_dmar(MCHPCIState *mch) -{ - mch->iommu = INTEL_IOMMU_DEVICE(qdev_create(NULL, TYPE_INTEL_IOMMU_DEVICE)); - object_property_add_child(OBJECT(mch), "intel-iommu", - OBJECT(mch->iommu), NULL); - qdev_init_nofail(DEVICE(mch->iommu)); -} - static void mch_realize(PCIDevice *d, Error **errp) { int i; @@ -513,10 +505,6 @@ static void mch_realize(PCIDevice *d, Error **errp) mch->pci_address_space, &mch->pam_regions[i+1], PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE); } - /* Intel IOMMU (VT-d) */ - if (object_property_get_bool(qdev_get_machine(), "iommu", NULL)) { - mch_init_dmar(mch); - } } uint64_t mch_mcfg_base(void) diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h index 1075f3ea50..73992b4ed6 100644 --- a/include/hw/pci-host/q35.h +++ b/include/hw/pci-host/q35.h @@ -60,7 +60,6 @@ typedef struct MCHPCIState { uint64_t above_4g_mem_size; uint64_t pci_hole64_size; uint32_t short_root_bus; - IntelIOMMUState *iommu; } MCHPCIState; typedef struct Q35PCIHost { diff --git a/qemu-options.hx b/qemu-options.hx index a95a936e55..9692e53af6 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -38,7 +38,6 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \ " kvm_shadow_mem=size of KVM shadow MMU in bytes\n" " dump-guest-core=on|off include guest memory in a core dump (default=on)\n" " mem-merge=on|off controls memory merge support (default: on)\n" - " iommu=on|off controls emulated Intel IOMMU (VT-d) support (default=off)\n" " igd-passthru=on|off controls IGD GFX passthrough support (default=off)\n" " aes-key-wrap=on|off controls support for AES key wrapping (default=on)\n" " dea-key-wrap=on|off controls support for DEA key wrapping (default=on)\n" @@ -73,8 +72,6 @@ Include guest memory in a core dump. The default is on. Enables or disables memory merge support. This feature, when supported by the host, de-duplicates identical memory pages among VMs instances (enabled by default). -@item iommu=on|off -Enables or disables emulated Intel IOMMU (VT-d) support. The default is off. @item aes-key-wrap=on|off Enables or disables AES key wrapping support on s390-ccw hosts. This feature controls whether AES wrapping keys will be created to allow From 97a83ec3a9d83f2e86b8b93178d8e8b64ccc7486 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 15 Jun 2016 19:56:30 +0200 Subject: [PATCH 07/30] piix: Set I440FXState member pci_info.w32 in one place Range pci_info.w32 records the location of the PCI hole. It's initialized to empty when QOM zeroes I440FXState. That's a fine value for a still unknown PCI hole. i440fx_init() sets pci_info.w32.begin = below_4g_mem_size. Changes the PCI hole from empty to [below_4g_mem_size, UINT64_MAX]. That's a bogus value. i440fx_pcihost_initfn() sets pci_info.end = IO_APIC_DEFAULT_ADDRESS. Since i440fx_init() ran already, this changes the PCI hole to [below_4g_mem_size, IO_APIC_DEFAULT_ADDRESS-1]. That's the correct value. Setting the bounds of the PCI hole in two separate places is confusing, and begs the question whether the bogus intermediate value could be used by something, or what would happen if we somehow managed to realize an i440FX device without having run the board init function i440fx_init() first. Avoid the confusion by setting the (constant) upper bound along with the lower bound in i440fx_init(). Signed-off-by: Markus Armbruster Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Reviewed-by: Eric Blake Reviewed-by: Marcel Apfelbaum --- hw/pci-host/piix.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c index 7167e58a12..d0b76c906e 100644 --- a/hw/pci-host/piix.c +++ b/hw/pci-host/piix.c @@ -263,7 +263,6 @@ static void i440fx_pcihost_get_pci_hole64_end(Object *obj, Visitor *v, static void i440fx_pcihost_initfn(Object *obj) { PCIHostState *s = PCI_HOST_BRIDGE(obj); - I440FXState *d = I440FX_PCI_HOST_BRIDGE(obj); memory_region_init_io(&s->conf_mem, obj, &pci_host_conf_le_ops, s, "pci-conf-idx", 4); @@ -285,8 +284,6 @@ static void i440fx_pcihost_initfn(Object *obj) object_property_add(obj, PCI_HOST_PROP_PCI_HOLE64_END, "int", i440fx_pcihost_get_pci_hole64_end, NULL, NULL, NULL, NULL); - - d->pci_info.w32.end = IO_APIC_DEFAULT_ADDRESS; } static void i440fx_pcihost_realize(DeviceState *dev, Error **errp) @@ -348,6 +345,7 @@ PCIBus *i440fx_init(const char *host_type, const char *pci_type, i440fx = I440FX_PCI_HOST_BRIDGE(dev); i440fx->pci_info.w32.begin = below_4g_mem_size; + i440fx->pci_info.w32.end = IO_APIC_DEFAULT_ADDRESS; /* setup pci memory mapping */ pc_pci_as_mapping_init(OBJECT(f), f->system_memory, From 01c9742d9d5a48c55a0155875657944c2159762c Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 15 Jun 2016 19:56:31 +0200 Subject: [PATCH 08/30] pc: Eliminate PcPciInfo PcPciInfo has two (ill-named) members: Range w32 is the PCI hole, and w64 is the PCI64 hole. Three users: * I440FXState and MCHPCIState have a member PcPciInfo pci_info, but only pci_info.w32 is actually used. This is confusing. Replace by Range pci_hole. * acpi_build() uses auto PcPciInfo pci_info to forward both PCI holes from acpi_get_pci_info() to build_dsdt(). Replace by two variables Range pci_hole, pci_hole64. Rename acpi_get_pci_info() to acpi_get_pci_holes(). PcPciInfo is now unused; drop it. Signed-off-by: Markus Armbruster Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Reviewed-by: Eric Blake Reviewed-by: Marcel Apfelbaum --- hw/i386/acpi-build.c | 43 ++++++++++++++++++++------------------- hw/pci-host/piix.c | 10 ++++----- hw/pci-host/q35.c | 12 +++++------ include/hw/i386/pc.h | 5 ----- include/hw/pci-host/q35.h | 2 +- 5 files changed, 34 insertions(+), 38 deletions(-) diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index 5a594be8ee..576c7af657 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -229,26 +229,25 @@ static Object *acpi_get_i386_pci_host(void) return OBJECT(host); } -static void acpi_get_pci_info(PcPciInfo *info) +static void acpi_get_pci_holes(Range *hole, Range *hole64) { Object *pci_host; - pci_host = acpi_get_i386_pci_host(); g_assert(pci_host); - info->w32.begin = object_property_get_int(pci_host, - PCI_HOST_PROP_PCI_HOLE_START, - NULL); - info->w32.end = object_property_get_int(pci_host, - PCI_HOST_PROP_PCI_HOLE_END, - NULL); - info->w64.begin = object_property_get_int(pci_host, - PCI_HOST_PROP_PCI_HOLE64_START, - NULL); - info->w64.end = object_property_get_int(pci_host, - PCI_HOST_PROP_PCI_HOLE64_END, + hole->begin = object_property_get_int(pci_host, + PCI_HOST_PROP_PCI_HOLE_START, + NULL); + hole->end = object_property_get_int(pci_host, + PCI_HOST_PROP_PCI_HOLE_END, + NULL); + hole64->begin = object_property_get_int(pci_host, + PCI_HOST_PROP_PCI_HOLE64_START, NULL); + hole64->end = object_property_get_int(pci_host, + PCI_HOST_PROP_PCI_HOLE64_END, + NULL); } #define ACPI_PORT_SMI_CMD 0x00b2 /* TODO: this is APM_CNT_IOPORT */ @@ -1890,7 +1889,7 @@ static Aml *build_q35_osc_method(void) static void build_dsdt(GArray *table_data, BIOSLinker *linker, AcpiPmInfo *pm, AcpiMiscInfo *misc, - PcPciInfo *pci, MachineState *machine) + Range *pci_hole, Range *pci_hole64, MachineState *machine) { CrsRangeEntry *entry; Aml *dsdt, *sb_scope, *scope, *dev, *method, *field, *pkg, *crs; @@ -2047,7 +2046,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, AML_CACHEABLE, AML_READ_WRITE, 0, 0x000A0000, 0x000BFFFF, 0, 0x00020000)); - crs_replace_with_free_ranges(mem_ranges, pci->w32.begin, pci->w32.end - 1); + crs_replace_with_free_ranges(mem_ranges, + pci_hole->begin, pci_hole->end - 1); for (i = 0; i < mem_ranges->len; i++) { entry = g_ptr_array_index(mem_ranges, i); aml_append(crs, @@ -2057,12 +2057,12 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, 0, entry->limit - entry->base + 1)); } - if (pci->w64.begin) { + if (pci_hole64->begin) { aml_append(crs, aml_qword_memory(AML_POS_DECODE, AML_MIN_FIXED, AML_MAX_FIXED, AML_CACHEABLE, AML_READ_WRITE, - 0, pci->w64.begin, pci->w64.end - 1, 0, - pci->w64.end - pci->w64.begin)); + 0, pci_hole64->begin, pci_hole64->end - 1, 0, + pci_hole64->end - pci_hole64->begin)); } if (misc->tpm_version != TPM_VERSION_UNSPEC) { @@ -2554,7 +2554,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine) AcpiPmInfo pm; AcpiMiscInfo misc; AcpiMcfgInfo mcfg; - PcPciInfo pci; + Range pci_hole, pci_hole64; uint8_t *u; size_t aml_len = 0; GArray *tables_blob = tables->table_data; @@ -2562,7 +2562,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine) acpi_get_pm_info(&pm); acpi_get_misc_info(&misc); - acpi_get_pci_info(&pci); + acpi_get_pci_holes(&pci_hole, &pci_hole64); acpi_get_slic_oem(&slic_oem); table_offsets = g_array_new(false, true /* clear */, @@ -2584,7 +2584,8 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine) /* DSDT is pointed to by FADT */ dsdt = tables_blob->len; - build_dsdt(tables_blob, tables->linker, &pm, &misc, &pci, machine); + build_dsdt(tables_blob, tables->linker, &pm, &misc, + &pci_hole, &pci_hole64, machine); /* Count the size of the DSDT and SSDT, we will need it for legacy * sizing of ACPI tables. diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c index d0b76c906e..b1bfc59379 100644 --- a/hw/pci-host/piix.c +++ b/hw/pci-host/piix.c @@ -48,7 +48,7 @@ typedef struct I440FXState { PCIHostState parent_obj; - PcPciInfo pci_info; + Range pci_hole; uint64_t pci_hole64_size; uint32_t short_root_bus; } I440FXState; @@ -221,7 +221,7 @@ static void i440fx_pcihost_get_pci_hole_start(Object *obj, Visitor *v, Error **errp) { I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj); - uint32_t value = s->pci_info.w32.begin; + uint32_t value = s->pci_hole.begin; visit_type_uint32(v, name, &value, errp); } @@ -231,7 +231,7 @@ static void i440fx_pcihost_get_pci_hole_end(Object *obj, Visitor *v, Error **errp) { I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj); - uint32_t value = s->pci_info.w32.end; + uint32_t value = s->pci_hole.end; visit_type_uint32(v, name, &value, errp); } @@ -344,8 +344,8 @@ PCIBus *i440fx_init(const char *host_type, const char *pci_type, f->ram_memory = ram_memory; i440fx = I440FX_PCI_HOST_BRIDGE(dev); - i440fx->pci_info.w32.begin = below_4g_mem_size; - i440fx->pci_info.w32.end = IO_APIC_DEFAULT_ADDRESS; + i440fx->pci_hole.begin = below_4g_mem_size; + i440fx->pci_hole.end = IO_APIC_DEFAULT_ADDRESS; /* setup pci memory mapping */ pc_pci_as_mapping_init(OBJECT(f), f->system_memory, diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c index eb1b2f77b7..f908ba36be 100644 --- a/hw/pci-host/q35.c +++ b/hw/pci-host/q35.c @@ -74,7 +74,7 @@ static void q35_host_get_pci_hole_start(Object *obj, Visitor *v, Error **errp) { Q35PCIHost *s = Q35_HOST_DEVICE(obj); - uint32_t value = s->mch.pci_info.w32.begin; + uint32_t value = s->mch.pci_hole.begin; visit_type_uint32(v, name, &value, errp); } @@ -84,7 +84,7 @@ static void q35_host_get_pci_hole_end(Object *obj, Visitor *v, Error **errp) { Q35PCIHost *s = Q35_HOST_DEVICE(obj); - uint32_t value = s->mch.pci_info.w32.end; + uint32_t value = s->mch.pci_hole.end; visit_type_uint32(v, name, &value, errp); } @@ -205,9 +205,9 @@ static void q35_host_initfn(Object *obj) * it's not a power of two, which means an MTRR * can't cover it exactly. */ - s->mch.pci_info.w32.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT + + s->mch.pci_hole.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT + MCH_HOST_BRIDGE_PCIEXBAR_MAX; - s->mch.pci_info.w32.end = IO_APIC_DEFAULT_ADDRESS; + s->mch.pci_hole.end = IO_APIC_DEFAULT_ADDRESS; } static const TypeInfo q35_host_info = { @@ -288,9 +288,9 @@ static void mch_update_pciexbar(MCHPCIState *mch) * which means an MTRR can't cover it exactly. */ if (enable) { - mch->pci_info.w32.begin = addr + length; + mch->pci_hole.begin = addr + length; } else { - mch->pci_info.w32.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT; + mch->pci_hole.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT; } } diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h index 7e43b20b4b..d44e5e922b 100644 --- a/include/hw/i386/pc.h +++ b/include/hw/i386/pc.h @@ -150,11 +150,6 @@ struct PCMachineClass { /* PC-style peripherals (also used by other machines). */ -typedef struct PcPciInfo { - Range w32; - Range w64; -} PcPciInfo; - #define ACPI_PM_PROP_S3_DISABLED "disable_s3" #define ACPI_PM_PROP_S4_DISABLED "disable_s4" #define ACPI_PM_PROP_S4_VAL "s4_val" diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h index 73992b4ed6..0d64032d87 100644 --- a/include/hw/pci-host/q35.h +++ b/include/hw/pci-host/q35.h @@ -55,7 +55,7 @@ typedef struct MCHPCIState { MemoryRegion smram_region, open_high_smram; MemoryRegion smram, low_smram, high_smram; MemoryRegion tseg_blackhole, tseg_window; - PcPciInfo pci_info; + Range pci_hole; uint64_t below_4g_mem_size; uint64_t above_4g_mem_size; uint64_t pci_hole64_size; From 0830c96d70b24ee76864f4e164b164bb43f24f09 Mon Sep 17 00:00:00 2001 From: Cornelia Huck Date: Thu, 30 Jun 2016 17:31:42 +0200 Subject: [PATCH 09/30] virtio: revert host notifiers to old semantics MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The host notifier rework tried both to unify host notifiers across transports and plug a possible hole during host notifier re-assignment. Unfortunately, this meant a change in semantics that breaks vhost and iSCSI+dataplane. As the minimal fix, keep the common host notifier code but revert to the old semantics so that we have time to figure out the proper fix. Fixes: 6798e245a3 ("virtio-bus: common ioeventfd infrastructure") Reported-by: Peter Lieven Reported-by: Jason Wang Reported-by: Marc-AndrĂ© Lureau Signed-off-by: Cornelia Huck Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Tested-by: Marc-AndrĂ© Lureau Reviewed-by: Jason Wang Tested-by: Jason Wang Tested-by: Peter Lieven --- hw/virtio/virtio-bus.c | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c index 131376027b..a85b7c8abe 100644 --- a/hw/virtio/virtio-bus.c +++ b/hw/virtio/virtio-bus.c @@ -176,8 +176,8 @@ static int set_host_notifier_internal(DeviceState *proxy, VirtioBusState *bus, return r; } } else { - virtio_queue_set_host_notifier_fd_handler(vq, false, false); k->ioeventfd_assign(proxy, notifier, n, assign); + virtio_queue_set_host_notifier_fd_handler(vq, false, false); event_notifier_cleanup(notifier); } return r; @@ -251,31 +251,25 @@ int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign) { VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(bus); DeviceState *proxy = DEVICE(BUS(bus)->parent); - VirtIODevice *vdev = virtio_bus_get_device(bus); - VirtQueue *vq = virtio_get_queue(vdev, n); if (!k->ioeventfd_started) { return -ENOSYS; } + k->ioeventfd_set_disabled(proxy, assign); if (assign) { /* * Stop using the generic ioeventfd, we are doing eventfd handling * ourselves below + * + * FIXME: We should just switch the handler and not deassign the + * ioeventfd. + * Otherwise, there's a window where we don't have an + * ioeventfd and we may end up with a notification where + * we don't expect one. */ - k->ioeventfd_set_disabled(proxy, true); + virtio_bus_stop_ioeventfd(bus); } - /* - * Just switch the handler, don't deassign the ioeventfd. - * Otherwise, there's a window where we don't have an - * ioeventfd and we may end up with a notification where - * we don't expect one. - */ - virtio_queue_set_host_notifier_fd_handler(vq, assign, !assign); - if (!assign) { - /* Use generic ioeventfd handler again. */ - k->ioeventfd_set_disabled(proxy, false); - } - return 0; + return set_host_notifier_internal(proxy, bus, n, assign, false); } static char *virtio_bus_get_dev_path(DeviceState *dev) From 62cee1a28aada2cce4b0e1fb835d8fc830aed7ac Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Mon, 4 Jul 2016 14:39:10 +0300 Subject: [PATCH 10/30] virtio: set low features early on load virtio migrates the low 32 feature bits twice, the first copy is there for compatibility but ever since 019a3edbb25f1571e876f8af1ce4c55412939e5d: ("virtio: make features 64bit wide") it's ignored on load. This is wrong since virtio_net_load tests self announcement and guest offloads before the second copy including high feature bits is loaded. This means that self announcement, control vq and guest offloads are all broken after migration. Fix it up by loading low feature bits: somewhat ugly since high and low bits become out of sync temporarily, but seems unavoidable for compatibility. The right thing to do for new features is probably to test the host features, anyway. Fixes: 019a3edbb25f1571e876f8af1ce4c55412939e5d ("virtio: make features 64bit wide") Cc: qemu-stable@nongnu.org Reported-by: Robin Geuze Tested-by: Robin Geuze Signed-off-by: Michael S. Tsirkin --- hw/virtio/virtio.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 7ed06eafa6..18153d5a39 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -1499,6 +1499,16 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id) } qemu_get_be32s(f, &features); + /* + * Temporarily set guest_features low bits - needed by + * virtio net load code testing for VIRTIO_NET_F_CTRL_GUEST_OFFLOADS + * VIRTIO_NET_F_GUEST_ANNOUNCE and VIRTIO_NET_F_CTRL_VQ. + * + * Note: devices should always test host features in future - don't create + * new dependencies like this. + */ + vdev->guest_features = features; + config_len = qemu_get_be32(f); /* From 6c6668232e71b7cf7ff39fa1a7abf660c40f9cea Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Mon, 4 Jul 2016 14:47:37 +0300 Subject: [PATCH 11/30] Revert "virtio-net: unbreak self announcement and guest offloads after migration" This reverts commit 1f8828ef573c83365b4a87a776daf8bcef1caa21. Cc: qemu-stable@nongnu.org Reported-by: Robin Geuze Tested-by: Robin Geuze Signed-off-by: Michael S. Tsirkin --- hw/net/virtio-net.c | 40 +++++++++++++++++----------------------- 1 file changed, 17 insertions(+), 23 deletions(-) diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index 7e6a60aa12..999989934e 100644 --- a/hw/net/virtio-net.c +++ b/hw/net/virtio-net.c @@ -1542,33 +1542,11 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id) { VirtIONet *n = opaque; VirtIODevice *vdev = VIRTIO_DEVICE(n); - int ret; if (version_id < 2 || version_id > VIRTIO_NET_VM_VERSION) return -EINVAL; - ret = virtio_load(vdev, f, version_id); - if (ret) { - return ret; - } - - if (virtio_vdev_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS)) { - n->curr_guest_offloads = qemu_get_be64(f); - } else { - n->curr_guest_offloads = virtio_net_supported_guest_offloads(n); - } - - if (peer_has_vnet_hdr(n)) { - virtio_net_apply_guest_offloads(n); - } - - if (virtio_vdev_has_feature(vdev, VIRTIO_NET_F_GUEST_ANNOUNCE) && - virtio_vdev_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ)) { - n->announce_counter = SELF_ANNOUNCE_ROUNDS; - timer_mod(n->announce_timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL)); - } - - return 0; + return virtio_load(vdev, f, version_id); } static int virtio_net_load_device(VirtIODevice *vdev, QEMUFile *f, @@ -1665,6 +1643,16 @@ static int virtio_net_load_device(VirtIODevice *vdev, QEMUFile *f, } } + if (virtio_vdev_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS)) { + n->curr_guest_offloads = qemu_get_be64(f); + } else { + n->curr_guest_offloads = virtio_net_supported_guest_offloads(n); + } + + if (peer_has_vnet_hdr(n)) { + virtio_net_apply_guest_offloads(n); + } + virtio_net_set_queues(n); /* Find the first multicast entry in the saved MAC filter */ @@ -1682,6 +1670,12 @@ static int virtio_net_load_device(VirtIODevice *vdev, QEMUFile *f, qemu_get_subqueue(n->nic, i)->link_down = link_down; } + if (virtio_vdev_has_feature(vdev, VIRTIO_NET_F_GUEST_ANNOUNCE) && + virtio_vdev_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ)) { + n->announce_counter = SELF_ANNOUNCE_ROUNDS; + timer_mod(n->announce_timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL)); + } + return 0; } From 5178ecd86367090a59af0e15e67cee6f673aaa08 Mon Sep 17 00:00:00 2001 From: Cao jin Date: Fri, 25 Mar 2016 14:49:37 +0800 Subject: [PATCH 12/30] pci_register_bar: cleanup place relevant code tegother, make the code easier to read Signed-off-by: Cao jin Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Reviewed-by: Paolo Bonzini --- hw/pci/pci.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/hw/pci/pci.c b/hw/pci/pci.c index ee385eba91..8c2f6ed605 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -1074,7 +1074,7 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num, uint8_t type, MemoryRegion *memory) { PCIIORegion *r; - uint32_t addr; + uint32_t addr; /* offset in pci config space */ uint64_t wmask; pcibus_t size = memory_region_size(memory); @@ -1090,15 +1090,20 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num, r->addr = PCI_BAR_UNMAPPED; r->size = size; r->type = type; - r->memory = NULL; + r->memory = memory; + r->address_space = type & PCI_BASE_ADDRESS_SPACE_IO + ? pci_dev->bus->address_space_io + : pci_dev->bus->address_space_mem; wmask = ~(size - 1); - addr = pci_bar(pci_dev, region_num); if (region_num == PCI_ROM_SLOT) { /* ROM enable bit is writable */ wmask |= PCI_ROM_ADDRESS_ENABLE; } + + addr = pci_bar(pci_dev, region_num); pci_set_long(pci_dev->config + addr, type); + if (!(r->type & PCI_BASE_ADDRESS_SPACE_IO) && r->type & PCI_BASE_ADDRESS_MEM_TYPE_64) { pci_set_quad(pci_dev->wmask + addr, wmask); @@ -1107,11 +1112,6 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num, pci_set_long(pci_dev->wmask + addr, wmask & 0xffffffff); pci_set_long(pci_dev->cmask + addr, 0xffffffff); } - pci_dev->io_regions[region_num].memory = memory; - pci_dev->io_regions[region_num].address_space - = type & PCI_BASE_ADDRESS_SPACE_IO - ? pci_dev->bus->address_space_io - : pci_dev->bus->address_space_mem; } static void pci_update_vga(PCIDevice *pci_dev) From 58e19e6e7914354242a67442d0006f9e31684d1a Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 1 Jul 2016 13:47:46 +0200 Subject: [PATCH 13/30] log: Clean up misuse of Range for -dfilter Range encodes an integer interval [a,b] as { begin = a, end = b + 1 }, where a \in [0,2^64-1] and b \in [1,2^64]. Thus, zero end is to be interpreted as 2^64. The implementation of -dfilter (commit 3514552) uses Range differently: it encodes [a,b] as { begin = a, end = b }. The code works, but it contradicts the specification of Range in range.h. Switch to the specified representation. Since it can't represent [0,UINT64_MAX], we have to reject that now. Add a test for it. While we're rejecting anyway: observe that we reject -dfilter LOB..UPB where LOB > UPB when UPB is zero, but happily create an empty Range when it isn't. Reject it then, too, and add a test for it. While there, add a positive test for the problematic upper bound UINT64_MAX. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Reviewed-by: Michael S. Tsirkin Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- tests/test-logging.c | 10 ++++++++++ util/log.c | 28 +++++++++++++++------------- 2 files changed, 25 insertions(+), 13 deletions(-) diff --git a/tests/test-logging.c b/tests/test-logging.c index 440e75f5db..b6fa94ee35 100644 --- a/tests/test-logging.c +++ b/tests/test-logging.c @@ -68,6 +68,16 @@ static void test_parse_range(void) g_assert(qemu_log_in_addr_range(0x2050)); g_assert(qemu_log_in_addr_range(0x3050)); + qemu_set_dfilter_ranges("0xffffffffffffffff-1", &error_abort); + g_assert(qemu_log_in_addr_range(UINT64_MAX)); + g_assert_false(qemu_log_in_addr_range(UINT64_MAX - 1)); + + qemu_set_dfilter_ranges("0..0xffffffffffffffff", &err); + error_free_or_abort(&err); + + qemu_set_dfilter_ranges("2..1", &err); + error_free_or_abort(&err); + qemu_set_dfilter_ranges("0x1000+onehundred", &err); error_free_or_abort(&err); diff --git a/util/log.c b/util/log.c index 32e416051c..f811d6163b 100644 --- a/util/log.c +++ b/util/log.c @@ -131,8 +131,8 @@ bool qemu_log_in_addr_range(uint64_t addr) if (debug_regions) { int i = 0; for (i = 0; i < debug_regions->len; i++) { - struct Range *range = &g_array_index(debug_regions, Range, i); - if (addr >= range->begin && addr <= range->end) { + Range *range = &g_array_index(debug_regions, Range, i); + if (addr >= range->begin && addr <= range->end - 1) { return true; } } @@ -158,7 +158,7 @@ void qemu_set_dfilter_ranges(const char *filter_spec, Error **errp) for (i = 0; ranges[i]; i++) { const char *r = ranges[i]; const char *range_op, *r2, *e; - uint64_t r1val, r2val; + uint64_t r1val, r2val, lob, upb; struct Range range; range_op = strstr(r, "-"); @@ -187,27 +187,29 @@ void qemu_set_dfilter_ranges(const char *filter_spec, Error **errp) (int)(r2 - range_op), range_op); goto out; } - if (r2val == 0) { - error_setg(errp, "Invalid range"); - goto out; - } switch (*range_op) { case '+': - range.begin = r1val; - range.end = r1val + (r2val - 1); + lob = r1val; + upb = r1val + r2val - 1; break; case '-': - range.end = r1val; - range.begin = r1val - (r2val - 1); + upb = r1val; + lob = r1val - (r2val - 1); break; case '.': - range.begin = r1val; - range.end = r2val; + lob = r1val; + upb = r2val; break; default: g_assert_not_reached(); } + if (lob > upb || (lob == 0 && upb == UINT64_MAX)) { + error_setg(errp, "Invalid range"); + goto out; + } + range.begin = lob; + range.end = upb + 1; g_array_append_val(debug_regions, range); } out: From a0efbf16604770b9d805bcf210ec29942321134f Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 1 Jul 2016 13:47:47 +0200 Subject: [PATCH 14/30] range: Eliminate direct Range member access Users of struct Range mess liberally with its members, which makes refactoring hard. Create a set of methods, and convert all users to call them instead of accessing members. The methods have carefully worded contracts, and use assertions to check them. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Reviewed-by: Michael S. Tsirkin Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/i386/acpi-build.c | 35 ++++++++------- hw/pci-host/piix.c | 26 +++++++---- hw/pci-host/q35.c | 41 +++++++++++------ hw/pci/pci.c | 17 ++++---- include/qemu/range.h | 85 +++++++++++++++++++++++++++++++++++- qapi/string-input-visitor.c | 20 ++++----- qapi/string-output-visitor.c | 18 ++++---- util/log.c | 5 +-- util/range.c | 3 +- 9 files changed, 176 insertions(+), 74 deletions(-) diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index 576c7af657..fbba461a87 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -236,18 +236,20 @@ static void acpi_get_pci_holes(Range *hole, Range *hole64) pci_host = acpi_get_i386_pci_host(); g_assert(pci_host); - hole->begin = object_property_get_int(pci_host, - PCI_HOST_PROP_PCI_HOLE_START, - NULL); - hole->end = object_property_get_int(pci_host, - PCI_HOST_PROP_PCI_HOLE_END, - NULL); - hole64->begin = object_property_get_int(pci_host, - PCI_HOST_PROP_PCI_HOLE64_START, - NULL); - hole64->end = object_property_get_int(pci_host, - PCI_HOST_PROP_PCI_HOLE64_END, - NULL); + range_set_bounds1(hole, + object_property_get_int(pci_host, + PCI_HOST_PROP_PCI_HOLE_START, + NULL), + object_property_get_int(pci_host, + PCI_HOST_PROP_PCI_HOLE_END, + NULL)); + range_set_bounds1(hole64, + object_property_get_int(pci_host, + PCI_HOST_PROP_PCI_HOLE64_START, + NULL), + object_property_get_int(pci_host, + PCI_HOST_PROP_PCI_HOLE64_END, + NULL)); } #define ACPI_PORT_SMI_CMD 0x00b2 /* TODO: this is APM_CNT_IOPORT */ @@ -2047,7 +2049,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, 0, 0x000A0000, 0x000BFFFF, 0, 0x00020000)); crs_replace_with_free_ranges(mem_ranges, - pci_hole->begin, pci_hole->end - 1); + range_lob(pci_hole), + range_upb(pci_hole)); for (i = 0; i < mem_ranges->len; i++) { entry = g_ptr_array_index(mem_ranges, i); aml_append(crs, @@ -2057,12 +2060,12 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, 0, entry->limit - entry->base + 1)); } - if (pci_hole64->begin) { + if (!range_is_empty(pci_hole64)) { aml_append(crs, aml_qword_memory(AML_POS_DECODE, AML_MIN_FIXED, AML_MAX_FIXED, AML_CACHEABLE, AML_READ_WRITE, - 0, pci_hole64->begin, pci_hole64->end - 1, 0, - pci_hole64->end - pci_hole64->begin)); + 0, range_lob(pci_hole64), range_upb(pci_hole64), 0, + range_upb(pci_hole64) + 1 - range_lob(pci_hole64))); } if (misc->tpm_version != TPM_VERSION_UNSPEC) { diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c index b1bfc59379..f9218aa952 100644 --- a/hw/pci-host/piix.c +++ b/hw/pci-host/piix.c @@ -221,8 +221,12 @@ static void i440fx_pcihost_get_pci_hole_start(Object *obj, Visitor *v, Error **errp) { I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj); - uint32_t value = s->pci_hole.begin; + uint64_t val64; + uint32_t value; + val64 = range_is_empty(&s->pci_hole) ? 0 : range_lob(&s->pci_hole); + value = val64; + assert(value == val64); visit_type_uint32(v, name, &value, errp); } @@ -231,8 +235,12 @@ static void i440fx_pcihost_get_pci_hole_end(Object *obj, Visitor *v, Error **errp) { I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj); - uint32_t value = s->pci_hole.end; + uint64_t val64; + uint32_t value; + val64 = range_is_empty(&s->pci_hole) ? 0 : range_upb(&s->pci_hole) + 1; + value = val64; + assert(value == val64); visit_type_uint32(v, name, &value, errp); } @@ -242,10 +250,11 @@ static void i440fx_pcihost_get_pci_hole64_start(Object *obj, Visitor *v, { PCIHostState *h = PCI_HOST_BRIDGE(obj); Range w64; + uint64_t value; pci_bus_get_w64_range(h->bus, &w64); - - visit_type_uint64(v, name, &w64.begin, errp); + value = range_is_empty(&w64) ? 0 : range_lob(&w64); + visit_type_uint64(v, name, &value, errp); } static void i440fx_pcihost_get_pci_hole64_end(Object *obj, Visitor *v, @@ -254,10 +263,11 @@ static void i440fx_pcihost_get_pci_hole64_end(Object *obj, Visitor *v, { PCIHostState *h = PCI_HOST_BRIDGE(obj); Range w64; + uint64_t value; pci_bus_get_w64_range(h->bus, &w64); - - visit_type_uint64(v, name, &w64.end, errp); + value = range_is_empty(&w64) ? 0 : range_upb(&w64) + 1; + visit_type_uint64(v, name, &value, errp); } static void i440fx_pcihost_initfn(Object *obj) @@ -344,8 +354,8 @@ PCIBus *i440fx_init(const char *host_type, const char *pci_type, f->ram_memory = ram_memory; i440fx = I440FX_PCI_HOST_BRIDGE(dev); - i440fx->pci_hole.begin = below_4g_mem_size; - i440fx->pci_hole.end = IO_APIC_DEFAULT_ADDRESS; + range_set_bounds(&i440fx->pci_hole, below_4g_mem_size, + IO_APIC_DEFAULT_ADDRESS - 1); /* setup pci memory mapping */ pc_pci_as_mapping_init(OBJECT(f), f->system_memory, diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c index f908ba36be..344f77b10c 100644 --- a/hw/pci-host/q35.c +++ b/hw/pci-host/q35.c @@ -74,8 +74,13 @@ static void q35_host_get_pci_hole_start(Object *obj, Visitor *v, Error **errp) { Q35PCIHost *s = Q35_HOST_DEVICE(obj); - uint32_t value = s->mch.pci_hole.begin; + uint64_t val64; + uint32_t value; + val64 = range_is_empty(&s->mch.pci_hole) + ? 0 : range_lob(&s->mch.pci_hole); + value = val64; + assert(value == val64); visit_type_uint32(v, name, &value, errp); } @@ -84,8 +89,13 @@ static void q35_host_get_pci_hole_end(Object *obj, Visitor *v, Error **errp) { Q35PCIHost *s = Q35_HOST_DEVICE(obj); - uint32_t value = s->mch.pci_hole.end; + uint64_t val64; + uint32_t value; + val64 = range_is_empty(&s->mch.pci_hole) + ? 0 : range_upb(&s->mch.pci_hole) + 1; + value = val64; + assert(value == val64); visit_type_uint32(v, name, &value, errp); } @@ -95,10 +105,11 @@ static void q35_host_get_pci_hole64_start(Object *obj, Visitor *v, { PCIHostState *h = PCI_HOST_BRIDGE(obj); Range w64; + uint64_t value; pci_bus_get_w64_range(h->bus, &w64); - - visit_type_uint64(v, name, &w64.begin, errp); + value = range_is_empty(&w64) ? 0 : range_lob(&w64); + visit_type_uint64(v, name, &value, errp); } static void q35_host_get_pci_hole64_end(Object *obj, Visitor *v, @@ -107,10 +118,11 @@ static void q35_host_get_pci_hole64_end(Object *obj, Visitor *v, { PCIHostState *h = PCI_HOST_BRIDGE(obj); Range w64; + uint64_t value; pci_bus_get_w64_range(h->bus, &w64); - - visit_type_uint64(v, name, &w64.end, errp); + value = range_is_empty(&w64) ? 0 : range_upb(&w64) + 1; + visit_type_uint64(v, name, &value, errp); } static void q35_host_get_mmcfg_size(Object *obj, Visitor *v, const char *name, @@ -205,9 +217,9 @@ static void q35_host_initfn(Object *obj) * it's not a power of two, which means an MTRR * can't cover it exactly. */ - s->mch.pci_hole.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT + - MCH_HOST_BRIDGE_PCIEXBAR_MAX; - s->mch.pci_hole.end = IO_APIC_DEFAULT_ADDRESS; + range_set_bounds(&s->mch.pci_hole, + MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT + MCH_HOST_BRIDGE_PCIEXBAR_MAX, + IO_APIC_DEFAULT_ADDRESS - 1); } static const TypeInfo q35_host_info = { @@ -275,10 +287,7 @@ static void mch_update_pciexbar(MCHPCIState *mch) break; case MCH_HOST_BRIDGE_PCIEXBAR_LENGTH_RVD: default: - enable = 0; - length = 0; abort(); - break; } addr = pciexbar & addr_mask; pcie_host_mmcfg_update(pehb, enable, addr, length); @@ -288,9 +297,13 @@ static void mch_update_pciexbar(MCHPCIState *mch) * which means an MTRR can't cover it exactly. */ if (enable) { - mch->pci_hole.begin = addr + length; + range_set_bounds(&mch->pci_hole, + addr + length, + IO_APIC_DEFAULT_ADDRESS - 1); } else { - mch->pci_hole.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT; + range_set_bounds(&mch->pci_hole, + MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT, + IO_APIC_DEFAULT_ADDRESS - 1); } } diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 8c2f6ed605..149994b815 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -2533,13 +2533,13 @@ static void pci_dev_get_w64(PCIBus *b, PCIDevice *dev, void *opaque) if (limit >= base) { Range pref_range; - pref_range.begin = base; - pref_range.end = limit + 1; + range_set_bounds(&pref_range, base, limit); range_extend(range, &pref_range); } } for (i = 0; i < PCI_NUM_REGIONS; ++i) { PCIIORegion *r = &dev->io_regions[i]; + pcibus_t lob, upb; Range region_range; if (!r->size || @@ -2547,16 +2547,17 @@ static void pci_dev_get_w64(PCIBus *b, PCIDevice *dev, void *opaque) !(r->type & PCI_BASE_ADDRESS_MEM_TYPE_64)) { continue; } - region_range.begin = pci_bar_address(dev, i, r->type, r->size); - region_range.end = region_range.begin + r->size; - if (region_range.begin == PCI_BAR_UNMAPPED) { + lob = pci_bar_address(dev, i, r->type, r->size); + upb = lob + r->size - 1; + if (lob == PCI_BAR_UNMAPPED) { continue; } - region_range.begin = MAX(region_range.begin, 0x1ULL << 32); + lob = MAX(lob, 0x1ULL << 32); - if (region_range.end - 1 >= region_range.begin) { + if (upb >= lob) { + range_set_bounds(®ion_range, lob, upb); range_extend(range, ®ion_range); } } @@ -2564,7 +2565,7 @@ static void pci_dev_get_w64(PCIBus *b, PCIDevice *dev, void *opaque) void pci_bus_get_w64_range(PCIBus *bus, Range *range) { - range->begin = range->end = 0; + range_make_empty(range); pci_for_each_device_under_bus(bus, pci_dev_get_w64, range); } diff --git a/include/qemu/range.h b/include/qemu/range.h index 3970f00089..c8c46a97cd 100644 --- a/include/qemu/range.h +++ b/include/qemu/range.h @@ -36,12 +36,91 @@ struct Range { uint64_t end; /* 1 + the last byte. 0 if range empty or ends at ~0x0LL. */ }; +static inline void range_invariant(Range *range) +{ + assert((!range->begin && !range->end) /* empty */ + || range->begin <= range->end - 1); /* non-empty */ +} + +/* Compound literal encoding the empty range */ +#define range_empty ((Range){ .begin = 0, .end = 0 }) + +/* Is @range empty? */ +static inline bool range_is_empty(Range *range) +{ + range_invariant(range); + return !range->begin && !range->end; +} + +/* Does @range contain @val? */ +static inline bool range_contains(Range *range, uint64_t val) +{ + return !range_is_empty(range) + && val >= range->begin && val <= range->end - 1; +} + +/* Initialize @range to the empty range */ +static inline void range_make_empty(Range *range) +{ + *range = range_empty; + assert(range_is_empty(range)); +} + +/* + * Initialize @range to span the interval [@lob,@upb]. + * Both bounds are inclusive. + * The interval must not be empty, i.e. @lob must be less than or + * equal @upb. + * The interval must not be [0,UINT64_MAX], because Range can't + * represent that. + */ +static inline void range_set_bounds(Range *range, uint64_t lob, uint64_t upb) +{ + assert(lob <= upb); + range->begin = lob; + range->end = upb + 1; /* may wrap to zero, that's okay */ + assert(!range_is_empty(range)); +} + +/* + * Initialize @range to span the interval [@lob,@upb_plus1). + * The lower bound is inclusive, the upper bound is exclusive. + * Zero @upb_plus1 is special: if @lob is also zero, set @range to the + * empty range. Else, set @range to [@lob,UINT64_MAX]. + */ +static inline void range_set_bounds1(Range *range, + uint64_t lob, uint64_t upb_plus1) +{ + range->begin = lob; + range->end = upb_plus1; + range_invariant(range); +} + +/* Return @range's lower bound. @range must not be empty. */ +static inline uint64_t range_lob(Range *range) +{ + assert(!range_is_empty(range)); + return range->begin; +} + +/* Return @range's upper bound. @range must not be empty. */ +static inline uint64_t range_upb(Range *range) +{ + assert(!range_is_empty(range)); + return range->end - 1; +} + +/* + * Extend @range to the smallest interval that includes @extend_by, too. + * This must not extend @range to cover the interval [0,UINT64_MAX], + * because Range can't represent that. + */ static inline void range_extend(Range *range, Range *extend_by) { - if (!extend_by->begin && !extend_by->end) { + if (range_is_empty(extend_by)) { return; } - if (!range->begin && !range->end) { + if (range_is_empty(range)) { *range = *extend_by; return; } @@ -52,6 +131,8 @@ static inline void range_extend(Range *range, Range *extend_by) if (range->end - 1 < extend_by->end - 1) { range->end = extend_by->end; } + /* Must not extend to { .begin = 0, .end = 0 }: */ + assert(!range_is_empty(range)); } /* Get last byte of a range from offset + length. diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c index b546e5f76a..0690abb7de 100644 --- a/qapi/string-input-visitor.c +++ b/qapi/string-input-visitor.c @@ -59,8 +59,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp) if (errno == 0 && endptr > str) { if (*endptr == '\0') { cur = g_malloc0(sizeof(*cur)); - cur->begin = start; - cur->end = start + 1; + range_set_bounds(cur, start, start); siv->ranges = range_list_insert(siv->ranges, cur); cur = NULL; str = NULL; @@ -73,16 +72,14 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp) end < start + 65536)) { if (*endptr == '\0') { cur = g_malloc0(sizeof(*cur)); - cur->begin = start; - cur->end = end + 1; + range_set_bounds(cur, start, end); siv->ranges = range_list_insert(siv->ranges, cur); cur = NULL; str = NULL; } else if (*endptr == ',') { str = endptr + 1; cur = g_malloc0(sizeof(*cur)); - cur->begin = start; - cur->end = end + 1; + range_set_bounds(cur, start, end); siv->ranges = range_list_insert(siv->ranges, cur); cur = NULL; } else { @@ -94,8 +91,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp) } else if (*endptr == ',') { str = endptr + 1; cur = g_malloc0(sizeof(*cur)); - cur->begin = start; - cur->end = start + 1; + range_set_bounds(cur, start, start); siv->ranges = range_list_insert(siv->ranges, cur); cur = NULL; } else { @@ -134,7 +130,7 @@ start_list(Visitor *v, const char *name, GenericList **list, size_t size, if (siv->cur_range) { Range *r = siv->cur_range->data; if (r) { - siv->cur = r->begin; + siv->cur = range_lob(r); } *list = g_malloc0(size); } else { @@ -156,7 +152,7 @@ static GenericList *next_list(Visitor *v, GenericList *tail, size_t size) return NULL; } - if (siv->cur < r->begin || siv->cur >= r->end) { + if (!range_contains(r, siv->cur)) { siv->cur_range = g_list_next(siv->cur_range); if (!siv->cur_range) { return NULL; @@ -165,7 +161,7 @@ static GenericList *next_list(Visitor *v, GenericList *tail, size_t size) if (!r) { return NULL; } - siv->cur = r->begin; + siv->cur = range_lob(r); } tail->next = g_malloc0(size); @@ -208,7 +204,7 @@ static void parse_type_int64(Visitor *v, const char *name, int64_t *obj, goto error; } - siv->cur = r->begin; + siv->cur = range_lob(r); } *obj = siv->cur; diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c index 5ea395ab98..c6f01f9f9c 100644 --- a/qapi/string-output-visitor.c +++ b/qapi/string-output-visitor.c @@ -83,8 +83,8 @@ static void string_output_set(StringOutputVisitor *sov, char *string) static void string_output_append(StringOutputVisitor *sov, int64_t a) { Range *r = g_malloc0(sizeof(*r)); - r->begin = a; - r->end = a + 1; + + range_set_bounds(r, a, a); sov->ranges = range_list_insert(sov->ranges, r); } @@ -92,28 +92,28 @@ static void string_output_append_range(StringOutputVisitor *sov, int64_t s, int64_t e) { Range *r = g_malloc0(sizeof(*r)); - r->begin = s; - r->end = e + 1; + + range_set_bounds(r, s, e); sov->ranges = range_list_insert(sov->ranges, r); } static void format_string(StringOutputVisitor *sov, Range *r, bool next, bool human) { - if (r->end - r->begin > 1) { + if (range_lob(r) != range_upb(r)) { if (human) { g_string_append_printf(sov->string, "0x%" PRIx64 "-0x%" PRIx64, - r->begin, r->end - 1); + range_lob(r), range_upb(r)); } else { g_string_append_printf(sov->string, "%" PRId64 "-%" PRId64, - r->begin, r->end - 1); + range_lob(r), range_upb(r)); } } else { if (human) { - g_string_append_printf(sov->string, "0x%" PRIx64, r->begin); + g_string_append_printf(sov->string, "0x%" PRIx64, range_lob(r)); } else { - g_string_append_printf(sov->string, "%" PRId64, r->begin); + g_string_append_printf(sov->string, "%" PRId64, range_lob(r)); } } if (next) { diff --git a/util/log.c b/util/log.c index f811d6163b..4da635c8c9 100644 --- a/util/log.c +++ b/util/log.c @@ -132,7 +132,7 @@ bool qemu_log_in_addr_range(uint64_t addr) int i = 0; for (i = 0; i < debug_regions->len; i++) { Range *range = &g_array_index(debug_regions, Range, i); - if (addr >= range->begin && addr <= range->end - 1) { + if (range_contains(range, addr)) { return true; } } @@ -208,8 +208,7 @@ void qemu_set_dfilter_ranges(const char *filter_spec, Error **errp) error_setg(errp, "Invalid range"); goto out; } - range.begin = lob; - range.end = upb + 1; + range_set_bounds(&range, lob, upb); g_array_append_val(debug_regions, range); } out: diff --git a/util/range.c b/util/range.c index e90c988dbf..e5f2e71142 100644 --- a/util/range.c +++ b/util/range.c @@ -46,8 +46,7 @@ GList *range_list_insert(GList *list, Range *data) { GList *l; - /* Range lists require no empty ranges */ - assert(data->begin < data->end || (data->begin && !data->end)); + assert(!range_is_empty(data)); /* Skip all list elements strictly less than data */ for (l = list; l && range_compare(l->data, data) < 0; l = l->next) { From 6dd726a2bf1b800289d90a84d5fcb5ce7b78a8e1 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 1 Jul 2016 13:47:48 +0200 Subject: [PATCH 15/30] range: Replace internal representation of Range Range represents a range as follows. Member @start is the inclusive lower bound, member @end is the exclusive upper bound. Zero @end is special: if @start is also zero, the range is empty, else @end is to be interpreted as 2^64. No other empty ranges may occur. The range [0,2^64-1] cannot be represented. If you try to create it with range_set_bounds1(), you get the empty range instead. If you try to create it with range_set_bounds() or range_extend(), assertions fail. Before range_set_bounds() existed, the open-coded creation usually got you the empty range instead. Open deathtrap. Moreover, the code dealing with the janus-faced @end is too clever by half. Dumb this down to a more pedestrian representation: members @lob and @upb are inclusive lower and upper bounds. The empty range is encoded as @lob = 1, @upb = 0. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- include/qemu/range.h | 56 +++++++++++++++++++++----------------------- util/range.c | 16 ++++++------- 2 files changed, 34 insertions(+), 38 deletions(-) diff --git a/include/qemu/range.h b/include/qemu/range.h index c8c46a97cd..f28f0c1825 100644 --- a/include/qemu/range.h +++ b/include/qemu/range.h @@ -26,37 +26,38 @@ /* * Operations on 64 bit address ranges. * Notes: - * - ranges must not wrap around 0, but can include the last byte ~0x0LL. - * - this can not represent a full 0 to ~0x0LL range. + * - Ranges must not wrap around 0, but can include UINT64_MAX. */ -/* A structure representing a range of addresses. */ struct Range { - uint64_t begin; /* First byte of the range, or 0 if empty. */ - uint64_t end; /* 1 + the last byte. 0 if range empty or ends at ~0x0LL. */ + /* + * Do not access members directly, use the functions! + * A non-empty range has @lob <= @upb. + * An empty range has @lob == @upb + 1. + */ + uint64_t lob; /* inclusive lower bound */ + uint64_t upb; /* inclusive upper bound */ }; static inline void range_invariant(Range *range) { - assert((!range->begin && !range->end) /* empty */ - || range->begin <= range->end - 1); /* non-empty */ + assert(range->lob <= range->upb || range->lob == range->upb + 1); } /* Compound literal encoding the empty range */ -#define range_empty ((Range){ .begin = 0, .end = 0 }) +#define range_empty ((Range){ .lob = 1, .upb = 0 }) /* Is @range empty? */ static inline bool range_is_empty(Range *range) { range_invariant(range); - return !range->begin && !range->end; + return range->lob > range->upb; } /* Does @range contain @val? */ static inline bool range_contains(Range *range, uint64_t val) { - return !range_is_empty(range) - && val >= range->begin && val <= range->end - 1; + return val >= range->lob && val <= range->upb; } /* Initialize @range to the empty range */ @@ -71,14 +72,11 @@ static inline void range_make_empty(Range *range) * Both bounds are inclusive. * The interval must not be empty, i.e. @lob must be less than or * equal @upb. - * The interval must not be [0,UINT64_MAX], because Range can't - * represent that. */ static inline void range_set_bounds(Range *range, uint64_t lob, uint64_t upb) { - assert(lob <= upb); - range->begin = lob; - range->end = upb + 1; /* may wrap to zero, that's okay */ + range->lob = lob; + range->upb = upb; assert(!range_is_empty(range)); } @@ -91,8 +89,12 @@ static inline void range_set_bounds(Range *range, uint64_t lob, uint64_t upb) static inline void range_set_bounds1(Range *range, uint64_t lob, uint64_t upb_plus1) { - range->begin = lob; - range->end = upb_plus1; + if (!lob && !upb_plus1) { + *range = range_empty; + } else { + range->lob = lob; + range->upb = upb_plus1 - 1; + } range_invariant(range); } @@ -100,20 +102,18 @@ static inline void range_set_bounds1(Range *range, static inline uint64_t range_lob(Range *range) { assert(!range_is_empty(range)); - return range->begin; + return range->lob; } /* Return @range's upper bound. @range must not be empty. */ static inline uint64_t range_upb(Range *range) { assert(!range_is_empty(range)); - return range->end - 1; + return range->upb; } /* * Extend @range to the smallest interval that includes @extend_by, too. - * This must not extend @range to cover the interval [0,UINT64_MAX], - * because Range can't represent that. */ static inline void range_extend(Range *range, Range *extend_by) { @@ -124,15 +124,13 @@ static inline void range_extend(Range *range, Range *extend_by) *range = *extend_by; return; } - if (range->begin > extend_by->begin) { - range->begin = extend_by->begin; + if (range->lob > extend_by->lob) { + range->lob = extend_by->lob; } - /* Compare last byte in case region ends at ~0x0LL */ - if (range->end - 1 < extend_by->end - 1) { - range->end = extend_by->end; + if (range->upb < extend_by->upb) { + range->upb = extend_by->upb; } - /* Must not extend to { .begin = 0, .end = 0 }: */ - assert(!range_is_empty(range)); + range_invariant(range); } /* Get last byte of a range from offset + length. diff --git a/util/range.c b/util/range.c index e5f2e71142..416df7cdae 100644 --- a/util/range.c +++ b/util/range.c @@ -22,20 +22,18 @@ #include "qemu/range.h" /* - * Operations on 64 bit address ranges. - * Notes: - * - ranges must not wrap around 0, but can include the last byte ~0x0LL. - * - this can not represent a full 0 to ~0x0LL range. + * Return -1 if @a < @b, 1 @a > @b, and 0 if they touch or overlap. + * Both @a and @b must not be empty. */ - -/* Return -1 if @a < @b, 1 if greater, and 0 if they touch or overlap. */ static inline int range_compare(Range *a, Range *b) { - /* Zero a->end is 2**64, and therefore not less than any b->begin */ - if (a->end && a->end < b->begin) { + assert(!range_is_empty(a) && !range_is_empty(b)); + + /* Careful, avoid wraparound */ + if (b->lob && b->lob - 1 > a->upb) { return -1; } - if (b->end && a->begin > b->end) { + if (a->lob && a->lob - 1 > b->upb) { return 1; } return 0; From 58eeb83cc7c445f167c951493deddf45621d66bb Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 1 Jul 2016 13:47:49 +0200 Subject: [PATCH 16/30] log: Permit -dfilter 0..0xffffffffffffffff Works fine since the previous commit fixed the underlying range data type. Of course it filters out nothing, but so does 0..1,2..0xffffffffffffffff, and we don't bother rejecting that either. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- tests/test-logging.c | 5 +++-- util/log.c | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/test-logging.c b/tests/test-logging.c index b6fa94ee35..cdf13c6ba5 100644 --- a/tests/test-logging.c +++ b/tests/test-logging.c @@ -73,8 +73,9 @@ static void test_parse_range(void) g_assert_false(qemu_log_in_addr_range(UINT64_MAX - 1)); qemu_set_dfilter_ranges("0..0xffffffffffffffff", &err); - error_free_or_abort(&err); - + g_assert(qemu_log_in_addr_range(0)); + g_assert(qemu_log_in_addr_range(UINT64_MAX)); + qemu_set_dfilter_ranges("2..1", &err); error_free_or_abort(&err); diff --git a/util/log.c b/util/log.c index 4da635c8c9..b6c75b1102 100644 --- a/util/log.c +++ b/util/log.c @@ -204,7 +204,7 @@ void qemu_set_dfilter_ranges(const char *filter_spec, Error **errp) default: g_assert_not_reached(); } - if (lob > upb || (lob == 0 && upb == UINT64_MAX)) { + if (lob > upb) { error_setg(errp, "Invalid range"); goto out; } From 6b9c1dd2cd4264a5a71efd7c3a7ef81f3c0a4c5a Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Fri, 1 Jul 2016 13:50:22 +0200 Subject: [PATCH 17/30] tests: acpi: add CPU hotplug testcase Test with: -smp 2,cores=3,sockets=2,maxcpus=6 to capture sparse APIC ID values that default AMD CPU has in above configuration. Signed-off-by: Igor Mammedov Reviewed-by: Marcel Apfelbaum Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- tests/bios-tables-test.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c index 92c90dd194..de4019e57d 100644 --- a/tests/bios-tables-test.c +++ b/tests/bios-tables-test.c @@ -801,6 +801,32 @@ static void test_acpi_q35_tcg_bridge(void) free_test_data(&data); } +static void test_acpi_piix4_tcg_cphp(void) +{ + test_data data; + + memset(&data, 0, sizeof(data)); + data.machine = MACHINE_PC; + data.variant = ".cphp"; + test_acpi_one("-machine accel=tcg" + " -smp 2,cores=3,sockets=2,maxcpus=6", + &data); + free_test_data(&data); +} + +static void test_acpi_q35_tcg_cphp(void) +{ + test_data data; + + memset(&data, 0, sizeof(data)); + data.machine = MACHINE_Q35; + data.variant = ".cphp"; + test_acpi_one("-machine q35,accel=tcg" + " -smp 2,cores=3,sockets=2,maxcpus=6", + &data); + free_test_data(&data); +} + static uint8_t ipmi_required_struct_types[] = { 0, 1, 3, 4, 16, 17, 19, 32, 38, 127 }; @@ -856,6 +882,8 @@ int main(int argc, char *argv[]) qtest_add_func("acpi/q35/tcg/bridge", test_acpi_q35_tcg_bridge); qtest_add_func("acpi/piix4/tcg/ipmi", test_acpi_piix4_tcg_ipmi); qtest_add_func("acpi/q35/tcg/ipmi", test_acpi_q35_tcg_ipmi); + qtest_add_func("acpi/piix4/tcg/cpuhp", test_acpi_piix4_tcg_cphp); + qtest_add_func("acpi/q35/tcg/cpuhp", test_acpi_q35_tcg_cphp); } ret = g_test_run(); boot_sector_cleanup(disk); From 600426f2df5242fe6fd32c93b150eeea40cde1fa Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Fri, 1 Jul 2016 13:50:23 +0200 Subject: [PATCH 18/30] tests: add APIC.cphp and DSDT.cphp blobs Signed-off-by: Igor Mammedov Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- tests/acpi-test-data/pc/APIC.cphp | Bin 0 -> 160 bytes tests/acpi-test-data/pc/DSDT.cphp | Bin 0 -> 6435 bytes tests/acpi-test-data/q35/APIC.cphp | Bin 0 -> 160 bytes tests/acpi-test-data/q35/DSDT.cphp | Bin 0 -> 9197 bytes 4 files changed, 0 insertions(+), 0 deletions(-) create mode 100644 tests/acpi-test-data/pc/APIC.cphp create mode 100644 tests/acpi-test-data/pc/DSDT.cphp create mode 100644 tests/acpi-test-data/q35/APIC.cphp create mode 100644 tests/acpi-test-data/q35/DSDT.cphp diff --git a/tests/acpi-test-data/pc/APIC.cphp b/tests/acpi-test-data/pc/APIC.cphp new file mode 100644 index 0000000000000000000000000000000000000000..1bf8a0a63bc1c9b716d937b96eb34b05016b9366 GIT binary patch literal 160 zcmXwx!3}^Q3`JW6f}%UP3UJYrBwpObWgNv(oJ8%n@zB24pP!~WmxG9S&r6xsF>kdb z$yhQtNOavFgY<9)W~DJWDKu7Tozi)bd+hVZHk}Lv=1?18ZTnj%1d=%RQTrg}%kWi^)!*oq zjaB;huKSJaKKjC?9gl22SDtQmyw9Jwn*>3RH$BEsP!=7l;@G_fQ>*7?r&ia~Yfm7rjAu?cO=_ZlUD+nidHKSIk0569_w2ZYIWHnpC&SPJn}nMch(e6PU}u z-M9YqF0x=xLTcB^WW%gBDS4lWS{VgVtH3`+yL4UT10$Q=yVh!JKpIS03MLEvoo8oO zsYg7b2#bWS(jBrxgo#~Z_ugBp=pkGb)ucZwVW56Tm$-yNuPw3#{}%;_*Y3S-tZ#%J zr)Q%bWtLbZ3IfaWimru=I63rafz7Yd@5S#$BCXON#UEj!7H^WPlFwaOX_#fc*eiN{ zCZ`aVVCyVT*-Iv{H{oxFEwE$u5&MBnGg)?4^lJ7jQ!x$4KLRLr@AnO}9r`K}bv{^n zoKkl%0n2?vo=IWM3d^k0PsC3|Szg@t{i#aYx>4YhnxH`javEHaIGR`DE0M^HichnG zG{p!F6G9$X(O4egl>j_4@F+ETltlZcX0>UGykIhdidJcE7&)6(8rm9B-!!%AEy2Ew+VQd1MWeS%w+VK)-^S)6 zqBLO(RUBoFVcM%YbIewocr(Jj>ygg$O7dxk?R%egm_RnYy`9b`VIsLdQ2O@)l!R^5 zXs+pGYjCB1pANG94wJ%Wi)=m1gjyLu+5UYdge{d}ix{?OWXt<(catduHZFOxMToc8 zf$^SfQQ~bqaXaL3=g74Wu3Q({Wi?%Ai2l(yRhk# zM=Yf-*KcdBBmi3Z>=a9VIYE+svh9+uu#F|)yFN%g?Ly35l#j64?lmSMOi1QnL#CmC zV0n^ZuB_}FoBeW%B*g?|DTBWh{OuBTI@p8g1iGhY9ldUm&roLje#*Q3M30r9h=FO3af@`o=)hA+hoU$T4a5=3uBhnIrkc?#hv0!z-z zZc3f-7h6pQbBwM+6RxhJw}JytWA{cy-)vRGA=reUTp7*W$kiS`@;-X}=iJVRA3uD& zbN|DSiA^=Lu{JEf8OByAc}ZTP?GkE#nS_f{>>~ z(lkSdQZs`fQM0Oz93b^_JEx|ddb2Kj1RL$%>gqkeN`Wtdf0?po*7Ny79z6)o^Mq!hrR=;+zkR~eSUYl6BZ0H=%LbxRDquL3U#(4Pme!Qx z!l3T+a;opbaSmlRN(!qpSd~r$@rtXCY#5`;@pmCPZ5i`XJf}Q*f$x_UCZwLq@{>gb!plq?UYy z2?hyll-t=9lZlM?Hn64~+#Q${M4fUVs1!yW7S863@!?+{?(F8eCn~5q>mSU6WZ%%6Ex0toy}+i1e`6|7 z7%&7G*y zPshQBHxJcgxa43*HU-W$0&xb!S|GmFsPfjUAP!sSjPl(f_B@C+&uCR@*a?LO5`oaD zVFwf%NV0>?C}3Yyd^7eQs86vC?K`MbzcK4K(nnznN)5C%2Kruio(K&^J`{sn{aAykHT@#^8bUn>Jxk*!3~m_Aa;2IL4tKms2M^53B>U@=3=!bH zjdO}$@L+tEewC&&w9_EfegyNYbf{s5rw*VKu~5!B$)S^R&?U|azgPsU zax9dyCOI@22fe{r8Hci1C})M;5IB&EgD!K{G>4{Rp`10tp_w@7dz^KOL#JY)oE6^8 zfv?kX(Dym3!l6nml(S|zG#dx~fV0kU=u9k>v(9qpY#g-ASt0X-u+&&6XFbEAXX2n0 z&N|1TbFom)dX_`a#z8gCI?ti=u~5!>jziDIL3Pf0o> z2?|uCz_O1DS~7dx6udtUEhsBPO+YQQNuWV-7}{{G8=(ycgDpO^;b_aD4Tpn`I<(<< z@1bpauM5=`jLLf~~{t@J0fCWLHp!O~CCrUmw|Tq7LcI?fbqy zvilK3VsbkiCWn?bX2+-@#X>vAt&iC;a!8iYokdb z$yhQtNOavFgY<9)W~DJWDKu7Tozi)bd+hVZHk}Lv=1?18ZTnj%1k`3ZtwG6@7{+!>rO20e&Ky~_fGu&N4>j(KyTfRU6&f(Q{7gn+qvHeT5T&Q zzI5~4E?K$!my{O$rJIeQR&u=UJVOsFdBg>$Tdjrp;@7U@bOYH+JKbW~6i)Y6Ewr50 ztwuvQLAzNOemL3PZUy#(*F_M%xH{OF=y=lZwHmu`Ok;=STmzxu~-AH43` z0IcEL!S{Mh0p+2_I;DD-KHSUnIq*L1?^*BR$SR{(2aBKf6;5`0bTB3`^*_wZUMjJA z^tu;0Qcu~bHp*?K$ASusA7{7PXh$M1#Mj^DgxxvtD4u_zycMoAnqhavztL^Aiz23; zUQAtg{?v25-XQ-;zbE>=-0|^|7)*cCza#!~Colf>zs!+1a%XV1nyuMcclv`#Tu3Ar zwh-?K@8-laG#om$ox>noYZbeEIx&D{45m?Q?xfrvU7kAbhm?EYO?3{=Q(FYvQ86tn ze3kH3Z?wY{qsl4wkWdRil|@i2Z&^VJAN2-4yqo8qO{Gh=rHBBCLwFFZM+$`;O=w{&cexj^OFEKgs7~B$0_d(GwO}uZUOheI*5@ox>-i z?OP+_%zTpQxS1=$BjEGUG6LGdUy^5>#@`!cah8w7Lwi)vbEhiS+v&H{j&tQc7b@F0 zC#y>xoH&3ZXv)!26eDnTX&c@v%>RX#-A=?((8)7a`{cZ|DMFnXDRWUbZ z=Z}xEG)UYqA{Kzt@)+{~RUt8vpRp-s0y~U|sh}yrOhB25keC<^W7Eu3BcS__vobU- znSiR0n5qiydx_;dHZv8}mP|Exgu2d*p)XV%b}Wu}5=O`QmJofC%6 z2}9?EsS}~D(=l{9hEB)SiBQ*x6+%1HlZMVoQzt@Qr)%hR4V|v36QQm%XXwlsI&-E@ zgt|_wRND2-8#?o*PK3J7DMRO!p>xXAiBQ)$ZRngfbWWQ(5$ZZ;44pHE&KXlDLS3h4 z==2Poo~aX|u5;GVIcw;gHFYA?bW;n!NFqjt%<^_|9P-k8=m=_J^MU#n8XI?Uxmkj15lZjAg zUN)GQ4d!K&iBM-g!A#{cPcT!h@lQymTDTDsQ#r}9QFGa-xop-%C~G<}(4uCbl~!$J zplGA;&_EHOtPIi!R8bhH#IYq=*zYhiRY*)F4F)Q)%0M+J8K{IZlMECgMxzW= zVuumcR9;I4Dxo6-MTns@76vM@!-$ybE$G3{-=Xfl4Sd$v_b* zoiI>|WhM+%gOY(tC^N}G5h|T9P>E$G3{-=Xfl4Sd$v_b*oiI>|WhM+%gOY(tC^N}G z5h|T9P>E$G3{-=Xfl4Sd$v_b*oiI>|WhM+%gOY(tC^N}G5h|T9P>E$G3{-=Xfl4Sd z$v_b*oiI>|WhM+%gOY(tC^N}G5h|T9P>E$G3{-=Xfl4Sd$v_b*oiI>|WhM+%gOY(t zC^N}G5h|T9P>E$G3{-=Xfl4Sd$v_b*oiI>|WhM+%gOY(tC^N}G5h|T9P>E$G3{-=X zfl4Sd$v_b*oiI>|WhM+%gOY(tC^N}G5h|T9P>E$G3{-=Xfl4Sd$v_b*oiI>|WhM+% zgOY(tC^N}G5h|T9P>E$G3{-=Xfl4Sd$v_b*oiI>|WhM+%gOY(tC^N}G5h|T9P>E$G z3{-=Xfl4Sd$v_b*oiI>|WhM+%gOY(tC^N}G5h|T9P(+%6BGL^Mp>CiEH3QX{Fi?$2 z2C6a1Ks6=|RAa(GH6|IT#v}vPm@rU{2?N!bWS|<83{+#nKs6=|RAZ8XYD_XvjR^xq zq^>Ru6cMf%pG-1Pgt!bUB&IsIFi=G5+`>Q+sdGyPicp>p<?)=BoVloYP6Ij0mR$^rHXXJAU8UjXB^k=(attH;0 zwtJs@X6Q+WIHCb@e6HJSwyX!c!!b~K>K|4wC931G!uPIT(yEuZdI{sxtC#%KtCu>5 z5or$+)!o!%ln=D>0hbRF<%2WI2gCAVvOFG_eQ))lRzBqNVWNC^M)`19zML#?KSueo zR=&*T%Zc*kGs>5TP_#X(>yV_hfmscwk#WkJv2?QU)O2I>#y8Vpt1^~O z_MMt;Y#uC_>9BtpODFqIO*eL5FPiDFsToTr`%XAbKh-w*`u z?UQHajb^!}?nD<85dJe2G;Xa_-?$h5{;l7w?7#Zv8*lCZ=G8Yv#|j$t&EXw6<+>H? zoBTI3->`~LA=IY)c*WYzh)LxVAG_}`d+otw)+0Ib=xjny{#cQMDWasMbXKnQ^fzA)< zK0c0jlie6-EP8_r{p0~s9=kKW&XZ$D>YGPBKg!VAc)UVZG8HnMn%}QZ2d!YsIZKAs z`=XfZ_wW{^7mH+4T%AG19uve@5obCH$Az=vv5^5CTfK*y_MkeRitmNfQ@QE!T!16j z=|ZE)V7t}MpC$Cv>oqo5D313|(G|O?WG_KqOLpQEoI1MB`*>f2I})=Kf4IAGYdJ@B zgW(2_itX8)>j*pxpk1P$>(cC?n?m^0c)8flkutHn_u4ScDsLu@G}x$((pXj^jisfr zI8BvCvxWydRB!5JG_>OyPDPkuBUyce1ojmJ%$WLn3Yl4K@qo<^C{b~001&y+sN zbR7?Vl9{j%8fCIG$xODM$v(;S9Bq{o>ae95Wy)lenKF8&3^Mr_vFubDf8RczV(_U&8uSL~{^tGwRR-;fX+`xsUJC>dM)&;UQiOdxmMA#{g>|Cl zjq9`PZsEa?g~-c$fKR&o`0iN#sjgvoj9)=Sbf+=cx@^v&S(VKn*2}8*X*7qui&62V zA@%~A)i`RFszjnjof4r@uM&Y8bxN{N-P9m11$>y;1OGGXqvQJ&TiiLzoz}fk54Gxa ztUd@Q`qV8Td3|km*}e23T=hgUNhO&ik>#zgxh~Rop$W}taEHxB45%AZWjY*Fayl5( zIO)*v3K4D8ym8(CLDYCDvXU1dMsHVL)SmyN2E~zn~ zh3?@1JihUHFEI34+|mAd%!GB_Cg`rGTh*)Or9;bqpP-lPr3v?sA5DoAp8U~?p%1() zf0Tcew;c3a^5lJs!;kW#c#2QsfAy2=;-u2AUHVH2K0*_h{=Pas{`+^s=JWJYJ+Lub z<-czvrXlrf+ZX(MM7!wbo~Pjj&5PCUn6v)ikI;+v`O-U>D&BvJaV+{7q20_8fXK< z&wW=c(Y~tMh?@967S~MzGZu{|B7Hnt1>K literal 0 HcmV?d00001 From b2e1fffb5a9fe35bfcde0db2bf1e63c062e1d0d4 Mon Sep 17 00:00:00 2001 From: Cao jin Date: Mon, 20 Jun 2016 14:13:32 +0800 Subject: [PATCH 19/30] change pvscsi_init_msi() type to void Nobody use its return value, so change the type to void. cc: Michael S. Tsirkin cc: Paolo Bonzini cc: Markus Armbruster cc: Marcel Apfelbaum Reviewed-by: Markus Armbruster Acked-by: Dmitry Fleytman Reviewed-by: Marcel Apfelbaum Signed-off-by: Cao jin Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/scsi/vmw_pvscsi.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c index 2d7528d1dd..e035fcea34 100644 --- a/hw/scsi/vmw_pvscsi.c +++ b/hw/scsi/vmw_pvscsi.c @@ -1056,7 +1056,7 @@ pvscsi_io_read(void *opaque, hwaddr addr, unsigned size) } -static bool +static void pvscsi_init_msi(PVSCSIState *s) { int res; @@ -1070,8 +1070,6 @@ pvscsi_init_msi(PVSCSIState *s) } else { s->msi_used = true; } - - return s->msi_used; } static void From 290fd20db6e0d739d92ee08f43bff8d3885cd283 Mon Sep 17 00:00:00 2001 From: Cao jin Date: Mon, 20 Jun 2016 14:13:34 +0800 Subject: [PATCH 20/30] usb xhci: change msi/msix property type >From bit to enum OnOffAuto cc: Gerd Hoffmann cc: Michael S. Tsirkin cc: Markus Armbruster cc: Marcel Apfelbaum Reviewed-by: Markus Armbruster Signed-off-by: Cao jin Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/usb/hcd-xhci.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c index 43ba61599a..0a5510fbc2 100644 --- a/hw/usb/hcd-xhci.c +++ b/hw/usb/hcd-xhci.c @@ -461,6 +461,8 @@ struct XHCIState { uint32_t numslots; uint32_t flags; uint32_t max_pstreams_mask; + OnOffAuto msi; + OnOffAuto msix; /* Operational Registers */ uint32_t usbcmd; @@ -498,9 +500,7 @@ typedef struct XHCIEvRingSeg { } XHCIEvRingSeg; enum xhci_flags { - XHCI_FLAG_USE_MSI = 1, - XHCI_FLAG_USE_MSI_X, - XHCI_FLAG_SS_FIRST, + XHCI_FLAG_SS_FIRST = 1, XHCI_FLAG_FORCE_PCIE_ENDCAP, XHCI_FLAG_ENABLE_STREAMS, }; @@ -3648,10 +3648,12 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp) assert(ret >= 0); } - if (xhci_get_flag(xhci, XHCI_FLAG_USE_MSI)) { + if (xhci->msi != ON_OFF_AUTO_OFF) { + /* TODO check for errors */ msi_init(dev, 0x70, xhci->numintrs, true, false); } - if (xhci_get_flag(xhci, XHCI_FLAG_USE_MSI_X)) { + if (xhci->msix != ON_OFF_AUTO_OFF) { + /* TODO check for errors */ msix_init(dev, xhci->numintrs, &xhci->mem, 0, OFF_MSIX_TABLE, &xhci->mem, 0, OFF_MSIX_PBA, @@ -3872,8 +3874,8 @@ static const VMStateDescription vmstate_xhci = { }; static Property xhci_properties[] = { - DEFINE_PROP_BIT("msi", XHCIState, flags, XHCI_FLAG_USE_MSI, true), - DEFINE_PROP_BIT("msix", XHCIState, flags, XHCI_FLAG_USE_MSI_X, true), + DEFINE_PROP_ON_OFF_AUTO("msi", XHCIState, msi, ON_OFF_AUTO_AUTO), + DEFINE_PROP_ON_OFF_AUTO("msix", XHCIState, msix, ON_OFF_AUTO_AUTO), DEFINE_PROP_BIT("superspeed-ports-first", XHCIState, flags, XHCI_FLAG_SS_FIRST, true), DEFINE_PROP_BIT("force-pcie-endcap", XHCIState, flags, From c0f2abff738ab83bd60da55b043a28c5ee664078 Mon Sep 17 00:00:00 2001 From: Cao jin Date: Mon, 20 Jun 2016 14:13:35 +0800 Subject: [PATCH 21/30] intel-hda: change msi property type >From uint32 to enum OnOffAuto. cc: Gerd Hoffmann cc: Michael S. Tsirkin cc: Markus Armbruster cc: Marcel Apfelbaum Reviewed-by: Markus Armbruster Signed-off-by: Cao jin Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/audio/intel-hda.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c index 098b17d020..4e19e8b0f2 100644 --- a/hw/audio/intel-hda.c +++ b/hw/audio/intel-hda.c @@ -191,7 +191,7 @@ struct IntelHDAState { /* properties */ uint32_t debug; - uint32_t msi; + OnOffAuto msi; bool old_msi_addr; }; @@ -256,7 +256,7 @@ static void intel_hda_update_int_sts(IntelHDAState *d) static void intel_hda_update_irq(IntelHDAState *d) { - int msi = d->msi && msi_enabled(&d->pci); + bool msi = msi_enabled(&d->pci); int level; intel_hda_update_int_sts(d); @@ -1143,7 +1143,8 @@ static void intel_hda_realize(PCIDevice *pci, Error **errp) memory_region_init_io(&d->mmio, OBJECT(d), &intel_hda_mmio_ops, d, "intel-hda", 0x4000); pci_register_bar(&d->pci, 0, 0, &d->mmio); - if (d->msi) { + if (d->msi != ON_OFF_AUTO_OFF) { + /* TODO check for errors */ msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60, 1, true, false); } @@ -1235,7 +1236,7 @@ static const VMStateDescription vmstate_intel_hda = { static Property intel_hda_properties[] = { DEFINE_PROP_UINT32("debug", IntelHDAState, debug, 0), - DEFINE_PROP_UINT32("msi", IntelHDAState, msi, 1), + DEFINE_PROP_ON_OFF_AUTO("msi", IntelHDAState, msi, ON_OFF_AUTO_AUTO), DEFINE_PROP_BOOL("old_msi_addr", IntelHDAState, old_msi_addr, false), DEFINE_PROP_END_OF_LIST(), }; From 444dd1af1cdf430a437d2c9f06192b0bc94d1e10 Mon Sep 17 00:00:00 2001 From: Cao jin Date: Mon, 20 Jun 2016 14:13:36 +0800 Subject: [PATCH 22/30] mptsas: change msi property type >From uint32 to enum OnOffAuto, and give it a shorter name. cc: Paolo Bonzini cc: Michael S. Tsirkin cc: Markus Armbruster cc: Marcel Apfelbaum Reviewed-by: Markus Armbruster Signed-off-by: Cao jin Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/scsi/mptsas.c | 5 +++-- hw/scsi/mptsas.h | 3 ++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c index be88e161a9..40033c4399 100644 --- a/hw/scsi/mptsas.c +++ b/hw/scsi/mptsas.c @@ -1284,8 +1284,9 @@ static void mptsas_scsi_init(PCIDevice *dev, Error **errp) memory_region_init_io(&s->diag_io, OBJECT(s), &mptsas_diag_ops, s, "mptsas-diag", 0x10000); - if (s->msi_available && + if (s->msi != ON_OFF_AUTO_OFF && msi_init(dev, 0, 1, true, false) >= 0) { + /* TODO check for errors */ s->msi_in_use = true; } @@ -1403,7 +1404,7 @@ static const VMStateDescription vmstate_mptsas = { static Property mptsas_properties[] = { DEFINE_PROP_UINT64("sas_address", MPTSASState, sas_addr, 0), /* TODO: test MSI support under Windows */ - DEFINE_PROP_BIT("msi", MPTSASState, msi_available, 0, true), + DEFINE_PROP_ON_OFF_AUTO("msi", MPTSASState, msi, ON_OFF_AUTO_AUTO), DEFINE_PROP_END_OF_LIST(), }; diff --git a/hw/scsi/mptsas.h b/hw/scsi/mptsas.h index 595f81fb5b..0436a33911 100644 --- a/hw/scsi/mptsas.h +++ b/hw/scsi/mptsas.h @@ -27,7 +27,8 @@ struct MPTSASState { MemoryRegion diag_io; QEMUBH *request_bh; - uint32_t msi_available; + /* properties */ + OnOffAuto msi; uint64_t sas_addr; bool msi_in_use; From b4b4a57fa6cfa4bf9005b212f01c31e915686813 Mon Sep 17 00:00:00 2001 From: Cao jin Date: Mon, 20 Jun 2016 14:13:37 +0800 Subject: [PATCH 23/30] megasas: change msi/msix property type >From bit to enum OnOffAuto. cc: Hannes Reinecke cc: Paolo Bonzini cc: Michael S. Tsirkin cc: Markus Armbruster cc: Marcel Apfelbaum Reviewed-by: Markus Armbruster Signed-off-by: Cao jin Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Reviewed-by: Hannes Reinecke --- hw/scsi/megasas.c | 28 +++++++++++----------------- 1 file changed, 11 insertions(+), 17 deletions(-) diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c index d1772183cf..636ea73adb 100644 --- a/hw/scsi/megasas.c +++ b/hw/scsi/megasas.c @@ -48,11 +48,7 @@ #define MEGASAS_FLAG_USE_JBOD 0 #define MEGASAS_MASK_USE_JBOD (1 << MEGASAS_FLAG_USE_JBOD) -#define MEGASAS_FLAG_USE_MSI 1 -#define MEGASAS_MASK_USE_MSI (1 << MEGASAS_FLAG_USE_MSI) -#define MEGASAS_FLAG_USE_MSIX 2 -#define MEGASAS_MASK_USE_MSIX (1 << MEGASAS_FLAG_USE_MSIX) -#define MEGASAS_FLAG_USE_QUEUE64 3 +#define MEGASAS_FLAG_USE_QUEUE64 1 #define MEGASAS_MASK_USE_QUEUE64 (1 << MEGASAS_FLAG_USE_QUEUE64) static const char *mfi_frame_desc[] = { @@ -96,6 +92,8 @@ typedef struct MegasasState { int busy; int diag; int adp_reset; + OnOffAuto msi; + OnOffAuto msix; MegasasCmd *event_cmd; int event_locale; @@ -159,12 +157,12 @@ static bool megasas_use_queue64(MegasasState *s) static bool megasas_use_msi(MegasasState *s) { - return s->flags & MEGASAS_MASK_USE_MSI; + return s->msi != ON_OFF_AUTO_OFF; } static bool megasas_use_msix(MegasasState *s) { - return s->flags & MEGASAS_MASK_USE_MSIX; + return s->msix != ON_OFF_AUTO_OFF; } static bool megasas_is_jbod(MegasasState *s) @@ -2349,12 +2347,12 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp) if (megasas_use_msi(s) && msi_init(dev, 0x50, 1, true, false)) { - s->flags &= ~MEGASAS_MASK_USE_MSI; + s->msi = ON_OFF_AUTO_OFF; } if (megasas_use_msix(s) && msix_init(dev, 15, &s->mmio_io, b->mmio_bar, 0x2000, &s->mmio_io, b->mmio_bar, 0x3800, 0x68)) { - s->flags &= ~MEGASAS_MASK_USE_MSIX; + s->msix = ON_OFF_AUTO_OFF; } if (pci_is_express(dev)) { pcie_endpoint_cap_init(dev, 0xa0); @@ -2422,10 +2420,8 @@ static Property megasas_properties_gen1[] = { MEGASAS_DEFAULT_FRAMES), DEFINE_PROP_STRING("hba_serial", MegasasState, hba_serial), DEFINE_PROP_UINT64("sas_address", MegasasState, sas_addr, 0), - DEFINE_PROP_BIT("use_msi", MegasasState, flags, - MEGASAS_FLAG_USE_MSI, false), - DEFINE_PROP_BIT("use_msix", MegasasState, flags, - MEGASAS_FLAG_USE_MSIX, false), + DEFINE_PROP_ON_OFF_AUTO("msi", MegasasState, msi, ON_OFF_AUTO_AUTO), + DEFINE_PROP_ON_OFF_AUTO("msix", MegasasState, msix, ON_OFF_AUTO_AUTO), DEFINE_PROP_BIT("use_jbod", MegasasState, flags, MEGASAS_FLAG_USE_JBOD, false), DEFINE_PROP_END_OF_LIST(), @@ -2438,10 +2434,8 @@ static Property megasas_properties_gen2[] = { MEGASAS_GEN2_DEFAULT_FRAMES), DEFINE_PROP_STRING("hba_serial", MegasasState, hba_serial), DEFINE_PROP_UINT64("sas_address", MegasasState, sas_addr, 0), - DEFINE_PROP_BIT("use_msi", MegasasState, flags, - MEGASAS_FLAG_USE_MSI, true), - DEFINE_PROP_BIT("use_msix", MegasasState, flags, - MEGASAS_FLAG_USE_MSIX, true), + DEFINE_PROP_ON_OFF_AUTO("msi", MegasasState, msi, ON_OFF_AUTO_AUTO), + DEFINE_PROP_ON_OFF_AUTO("msix", MegasasState, msix, ON_OFF_AUTO_AUTO), DEFINE_PROP_BIT("use_jbod", MegasasState, flags, MEGASAS_FLAG_USE_JBOD, false), DEFINE_PROP_END_OF_LIST(), From 69b205bb0b9ecf65b0ded7b9219ef9a58ef322ad Mon Sep 17 00:00:00 2001 From: Cao jin Date: Mon, 20 Jun 2016 14:13:38 +0800 Subject: [PATCH 24/30] pci bridge dev: change msi property type >From bit to enum OnOffAuto. cc: Michael S. Tsirkin cc: Markus Armbruster cc: Marcel Apfelbaum Reviewed-by: Markus Armbruster Signed-off-by: Cao jin Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/pci-bridge/pci_bridge_dev.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c index 41ca47b15a..0fbecc4bbd 100644 --- a/hw/pci-bridge/pci_bridge_dev.c +++ b/hw/pci-bridge/pci_bridge_dev.c @@ -42,9 +42,10 @@ struct PCIBridgeDev { MemoryRegion bar; uint8_t chassis_nr; -#define PCI_BRIDGE_DEV_F_MSI_REQ 0 -#define PCI_BRIDGE_DEV_F_SHPC_REQ 1 +#define PCI_BRIDGE_DEV_F_SHPC_REQ 0 uint32_t flags; + + OnOffAuto msi; }; typedef struct PCIBridgeDev PCIBridgeDev; @@ -66,7 +67,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev) } } else { /* MSI is not applicable without SHPC */ - bridge_dev->flags &= ~(1 << PCI_BRIDGE_DEV_F_MSI_REQ); + bridge_dev->msi = ON_OFF_AUTO_OFF; } err = slotid_cap_init(dev, 0, bridge_dev->chassis_nr, 0); @@ -74,7 +75,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev) goto slotid_error; } - if ((bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_MSI_REQ)) && + if (bridge_dev->msi != ON_OFF_AUTO_OFF && msi_nonbroken) { err = msi_init(dev, 0, 1, true, true); if (err < 0) { @@ -147,8 +148,8 @@ static Property pci_bridge_dev_properties[] = { /* Note: 0 is not a legal chassis number. */ DEFINE_PROP_UINT8(PCI_BRIDGE_DEV_PROP_CHASSIS_NR, PCIBridgeDev, chassis_nr, 0), - DEFINE_PROP_BIT(PCI_BRIDGE_DEV_PROP_MSI, PCIBridgeDev, flags, - PCI_BRIDGE_DEV_F_MSI_REQ, true), + DEFINE_PROP_ON_OFF_AUTO(PCI_BRIDGE_DEV_PROP_MSI, PCIBridgeDev, msi, + ON_OFF_AUTO_AUTO), DEFINE_PROP_BIT(PCI_BRIDGE_DEV_PROP_SHPC, PCIBridgeDev, flags, PCI_BRIDGE_DEV_F_SHPC_REQ, true), DEFINE_PROP_END_OF_LIST(), From 1108b2f8a939fb5778d384149e2f1b99062a72da Mon Sep 17 00:00:00 2001 From: Cao jin Date: Mon, 20 Jun 2016 14:13:39 +0800 Subject: [PATCH 25/30] pci: Convert msi_init() to Error and fix callers to check it msi_init() reports errors with error_report(), which is wrong when it's used in realize(). Fix by converting it to Error. Fix its callers to handle failure instead of ignoring it. For those callers who don't handle the failure, it might happen: when user want msi on, but he doesn't get what he want because of msi_init fails silently. cc: Gerd Hoffmann cc: John Snow cc: Dmitry Fleytman cc: Jason Wang cc: Michael S. Tsirkin cc: Hannes Reinecke cc: Paolo Bonzini cc: Alex Williamson cc: Markus Armbruster cc: Marcel Apfelbaum Reviewed-by: Markus Armbruster Signed-off-by: Cao jin Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Reviewed-by: Hannes Reinecke --- hw/audio/intel-hda.c | 24 +++++++++++++++---- hw/ide/ich.c | 7 ++++-- hw/net/e1000e.c | 8 ++----- hw/net/vmxnet3.c | 37 ++++++++++-------------------- hw/pci-bridge/ioh3420.c | 6 ++++- hw/pci-bridge/pci_bridge_dev.c | 20 ++++++++++++---- hw/pci-bridge/xio3130_downstream.c | 6 ++++- hw/pci-bridge/xio3130_upstream.c | 6 ++++- hw/pci/msi.c | 11 ++++++--- hw/scsi/megasas.c | 26 +++++++++++++++++---- hw/scsi/mptsas.c | 31 +++++++++++++++++++------ hw/scsi/vmw_pvscsi.c | 2 +- hw/usb/hcd-xhci.c | 23 +++++++++++++++---- hw/vfio/pci.c | 7 ++++-- include/hw/pci/msi.h | 3 ++- 15 files changed, 150 insertions(+), 67 deletions(-) diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c index 4e19e8b0f2..cd95340cd9 100644 --- a/hw/audio/intel-hda.c +++ b/hw/audio/intel-hda.c @@ -1132,6 +1132,8 @@ static void intel_hda_realize(PCIDevice *pci, Error **errp) { IntelHDAState *d = INTEL_HDA(pci); uint8_t *conf = d->pci.config; + Error *err = NULL; + int ret; d->name = object_get_typename(OBJECT(d)); @@ -1140,13 +1142,27 @@ static void intel_hda_realize(PCIDevice *pci, Error **errp) /* HDCTL off 0x40 bit 0 selects signaling mode (1-HDA, 0 - Ac97) 18.1.19 */ conf[0x40] = 0x01; + if (d->msi != ON_OFF_AUTO_OFF) { + ret = msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60, + 1, true, false, &err); + /* Any error other than -ENOTSUP(board's MSI support is broken) + * is a programming error */ + assert(!ret || ret == -ENOTSUP); + if (ret && d->msi == ON_OFF_AUTO_ON) { + /* Can't satisfy user's explicit msi=on request, fail */ + error_append_hint(&err, "You have to use msi=auto (default) or " + "msi=off with this machine type.\n"); + error_propagate(errp, err); + return; + } + assert(!err || d->msi == ON_OFF_AUTO_AUTO); + /* With msi=auto, we fall back to MSI off silently */ + error_free(err); + } + memory_region_init_io(&d->mmio, OBJECT(d), &intel_hda_mmio_ops, d, "intel-hda", 0x4000); pci_register_bar(&d->pci, 0, 0, &d->mmio); - if (d->msi != ON_OFF_AUTO_OFF) { - /* TODO check for errors */ - msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60, 1, true, false); - } hda_codec_bus_init(DEVICE(pci), &d->codecs, sizeof(d->codecs), intel_hda_response, intel_hda_xfer); diff --git a/hw/ide/ich.c b/hw/ide/ich.c index 0a13334baa..920ec276ed 100644 --- a/hw/ide/ich.c +++ b/hw/ide/ich.c @@ -68,7 +68,6 @@ #include #include "sysemu/block-backend.h" #include "sysemu/dma.h" - #include #include @@ -111,6 +110,7 @@ static void pci_ich9_ahci_realize(PCIDevice *dev, Error **errp) int sata_cap_offset; uint8_t *sata_cap; d = ICH_AHCI(dev); + int ret; ahci_realize(&d->ahci, DEVICE(dev), pci_get_address_space(dev), 6); @@ -146,7 +146,10 @@ static void pci_ich9_ahci_realize(PCIDevice *dev, Error **errp) /* Although the AHCI 1.3 specification states that the first capability * should be PMCAP, the Intel ICH9 data sheet specifies that the ICH9 * AHCI device puts the MSI capability first, pointing to 0x80. */ - msi_init(dev, ICH9_MSI_CAP_OFFSET, 1, true, false); + ret = msi_init(dev, ICH9_MSI_CAP_OFFSET, 1, true, false, NULL); + /* Any error other than -ENOTSUP(board's MSI support is broken) + * is a programming error. Fall back to INTx silently on -ENOTSUP */ + assert(!ret || ret == -ENOTSUP); } static void pci_ich9_uninit(PCIDevice *dev) diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c index 692283fdd7..a06d1848d2 100644 --- a/hw/net/e1000e.c +++ b/hw/net/e1000e.c @@ -268,13 +268,9 @@ e1000e_init_msi(E1000EState *s) { int res; - res = msi_init(PCI_DEVICE(s), - 0xD0, /* MSI capability offset */ - 1, /* MAC MSI interrupts */ - true, /* 64-bit message addresses supported */ - false); /* Per vector mask supported */ + res = msi_init(PCI_DEVICE(s), 0xD0, 1, true, false, NULL); - if (res > 0) { + if (!res) { s->intr_state |= E1000E_USE_MSI; } else { trace_e1000e_msi_init_fail(res); diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c index 92236d3919..f24298cc49 100644 --- a/hw/net/vmxnet3.c +++ b/hw/net/vmxnet3.c @@ -2216,27 +2216,6 @@ vmxnet3_cleanup_msix(VMXNET3State *s) } } -#define VMXNET3_USE_64BIT (true) -#define VMXNET3_PER_VECTOR_MASK (false) - -static bool -vmxnet3_init_msi(VMXNET3State *s) -{ - PCIDevice *d = PCI_DEVICE(s); - int res; - - res = msi_init(d, VMXNET3_MSI_OFFSET(s), VMXNET3_MAX_NMSIX_INTRS, - VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK); - if (0 > res) { - VMW_WRPRN("Failed to initialize MSI, error %d", res); - s->msi_used = false; - } else { - s->msi_used = true; - } - - return s->msi_used; -} - static void vmxnet3_cleanup_msi(VMXNET3State *s) { @@ -2298,10 +2277,15 @@ static uint64_t vmxnet3_device_serial_num(VMXNET3State *s) return dsn_payload; } + +#define VMXNET3_USE_64BIT (true) +#define VMXNET3_PER_VECTOR_MASK (false) + static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp) { DeviceState *dev = DEVICE(pci_dev); VMXNET3State *s = VMXNET3(pci_dev); + int ret; VMW_CBPRN("Starting init..."); @@ -2325,14 +2309,17 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp) /* Interrupt pin A */ pci_dev->config[PCI_INTERRUPT_PIN] = 0x01; + ret = msi_init(pci_dev, VMXNET3_MSI_OFFSET(s), VMXNET3_MAX_NMSIX_INTRS, + VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK, NULL); + /* Any error other than -ENOTSUP(board's MSI support is broken) + * is a programming error. Fall back to INTx silently on -ENOTSUP */ + assert(!ret || ret == -ENOTSUP); + s->msi_used = !ret; + if (!vmxnet3_init_msix(s)) { VMW_WRPRN("Failed to initialize MSI-X, configuration is inconsistent."); } - if (!vmxnet3_init_msi(s)) { - VMW_WRPRN("Failed to initialize MSI, configuration is inconsistent."); - } - vmxnet3_net_init(s); if (pci_is_express(pci_dev)) { diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c index b4a7806e2e..93c6f0b7a2 100644 --- a/hw/pci-bridge/ioh3420.c +++ b/hw/pci-bridge/ioh3420.c @@ -25,6 +25,7 @@ #include "hw/pci/msi.h" #include "hw/pci/pcie.h" #include "ioh3420.h" +#include "qapi/error.h" #define PCI_DEVICE_ID_IOH_EPORT 0x3420 /* D0:F0 express mode */ #define PCI_DEVICE_ID_IOH_REV 0x2 @@ -97,6 +98,7 @@ static int ioh3420_initfn(PCIDevice *d) PCIEPort *p = PCIE_PORT(d); PCIESlot *s = PCIE_SLOT(d); int rc; + Error *err = NULL; pci_bridge_initfn(d, TYPE_PCIE_BUS); pcie_port_init_reg(d); @@ -109,8 +111,10 @@ static int ioh3420_initfn(PCIDevice *d) rc = msi_init(d, IOH_EP_MSI_OFFSET, IOH_EP_MSI_NR_VECTOR, IOH_EP_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_64BIT, - IOH_EP_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT); + IOH_EP_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT, &err); if (rc < 0) { + assert(rc == -ENOTSUP); + error_report_err(err); goto err_bridge; } diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c index 0fbecc4bbd..5dbd933cc1 100644 --- a/hw/pci-bridge/pci_bridge_dev.c +++ b/hw/pci-bridge/pci_bridge_dev.c @@ -54,6 +54,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev) PCIBridge *br = PCI_BRIDGE(dev); PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev); int err; + Error *local_err = NULL; pci_bridge_initfn(dev, TYPE_PCI_BUS); @@ -75,12 +76,23 @@ static int pci_bridge_dev_initfn(PCIDevice *dev) goto slotid_error; } - if (bridge_dev->msi != ON_OFF_AUTO_OFF && - msi_nonbroken) { - err = msi_init(dev, 0, 1, true, true); - if (err < 0) { + if (bridge_dev->msi != ON_OFF_AUTO_OFF) { + /* it means SHPC exists, because MSI is needed by SHPC */ + + err = msi_init(dev, 0, 1, true, true, &local_err); + /* Any error other than -ENOTSUP(board's MSI support is broken) + * is a programming error */ + assert(!err || err == -ENOTSUP); + if (err && bridge_dev->msi == ON_OFF_AUTO_ON) { + /* Can't satisfy user's explicit msi=on request, fail */ + error_append_hint(&local_err, "You have to use msi=auto (default) " + "or msi=off with this machine type.\n"); + error_report_err(local_err); goto msi_error; } + assert(!local_err || bridge_dev->msi == ON_OFF_AUTO_AUTO); + /* With msi=auto, we fall back to MSI off silently */ + error_free(local_err); } if (shpc_present(dev)) { diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c index e6d653de4f..f6149a302d 100644 --- a/hw/pci-bridge/xio3130_downstream.c +++ b/hw/pci-bridge/xio3130_downstream.c @@ -24,6 +24,7 @@ #include "hw/pci/msi.h" #include "hw/pci/pcie.h" #include "xio3130_downstream.h" +#include "qapi/error.h" #define PCI_DEVICE_ID_TI_XIO3130D 0x8233 /* downstream port */ #define XIO3130_REVISION 0x1 @@ -60,14 +61,17 @@ static int xio3130_downstream_initfn(PCIDevice *d) PCIEPort *p = PCIE_PORT(d); PCIESlot *s = PCIE_SLOT(d); int rc; + Error *err = NULL; pci_bridge_initfn(d, TYPE_PCIE_BUS); pcie_port_init_reg(d); rc = msi_init(d, XIO3130_MSI_OFFSET, XIO3130_MSI_NR_VECTOR, XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_64BIT, - XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT); + XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT, &err); if (rc < 0) { + assert(rc == -ENOTSUP); + error_report_err(err); goto err_bridge; } diff --git a/hw/pci-bridge/xio3130_upstream.c b/hw/pci-bridge/xio3130_upstream.c index d97684474f..487edacc1d 100644 --- a/hw/pci-bridge/xio3130_upstream.c +++ b/hw/pci-bridge/xio3130_upstream.c @@ -24,6 +24,7 @@ #include "hw/pci/msi.h" #include "hw/pci/pcie.h" #include "xio3130_upstream.h" +#include "qapi/error.h" #define PCI_DEVICE_ID_TI_XIO3130U 0x8232 /* upstream port */ #define XIO3130_REVISION 0x2 @@ -56,14 +57,17 @@ static int xio3130_upstream_initfn(PCIDevice *d) { PCIEPort *p = PCIE_PORT(d); int rc; + Error *err = NULL; pci_bridge_initfn(d, TYPE_PCIE_BUS); pcie_port_init_reg(d); rc = msi_init(d, XIO3130_MSI_OFFSET, XIO3130_MSI_NR_VECTOR, XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_64BIT, - XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT); + XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT, &err); if (rc < 0) { + assert(rc == -ENOTSUP); + error_report_err(err); goto err_bridge; } diff --git a/hw/pci/msi.c b/hw/pci/msi.c index ed792251dd..a87b2278a3 100644 --- a/hw/pci/msi.c +++ b/hw/pci/msi.c @@ -22,6 +22,7 @@ #include "hw/pci/msi.h" #include "hw/xen/xen.h" #include "qemu/range.h" +#include "qapi/error.h" /* PCI_MSI_ADDRESS_LO */ #define PCI_MSI_ADDRESS_LO_MASK (~0x3) @@ -173,7 +174,8 @@ bool msi_enabled(const PCIDevice *dev) * If @msi64bit, make the device capable of sending a 64-bit message * address. * If @msi_per_vector_mask, make the device support per-vector masking. - * Return 0 on success, return -errno on error. + * @errp is for returning errors. + * Return 0 on success; set @errp and return -errno on error. * * -ENOTSUP means lacking msi support for a msi-capable platform. * -EINVAL means capability overlap, happens when @offset is non-zero, @@ -181,7 +183,8 @@ bool msi_enabled(const PCIDevice *dev) * if a real HW is broken. */ int msi_init(struct PCIDevice *dev, uint8_t offset, - unsigned int nr_vectors, bool msi64bit, bool msi_per_vector_mask) + unsigned int nr_vectors, bool msi64bit, + bool msi_per_vector_mask, Error **errp) { unsigned int vectors_order; uint16_t flags; @@ -189,6 +192,7 @@ int msi_init(struct PCIDevice *dev, uint8_t offset, int config_offset; if (!msi_nonbroken) { + error_setg(errp, "MSI is not supported by interrupt controller"); return -ENOTSUP; } @@ -212,7 +216,8 @@ int msi_init(struct PCIDevice *dev, uint8_t offset, } cap_size = msi_cap_sizeof(flags); - config_offset = pci_add_capability(dev, PCI_CAP_ID_MSI, offset, cap_size); + config_offset = pci_add_capability2(dev, PCI_CAP_ID_MSI, offset, + cap_size, errp); if (config_offset < 0) { return config_offset; } diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c index 636ea73adb..6eb57ff61b 100644 --- a/hw/scsi/megasas.c +++ b/hw/scsi/megasas.c @@ -29,7 +29,7 @@ #include "hw/scsi/scsi.h" #include "block/scsi.h" #include "trace.h" - +#include "qapi/error.h" #include "mfi.h" #define MEGASAS_VERSION_GEN1 "1.70" @@ -2330,6 +2330,8 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp) MegasasBaseClass *b = MEGASAS_DEVICE_GET_CLASS(s); uint8_t *pci_conf; int i, bar_type; + Error *err = NULL; + int ret; pci_conf = dev->config; @@ -2338,6 +2340,24 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp) /* Interrupt pin 1 */ pci_conf[PCI_INTERRUPT_PIN] = 0x01; + if (megasas_use_msi(s)) { + ret = msi_init(dev, 0x50, 1, true, false, &err); + /* Any error other than -ENOTSUP(board's MSI support is broken) + * is a programming error */ + assert(!ret || ret == -ENOTSUP); + if (ret && s->msi == ON_OFF_AUTO_ON) { + /* Can't satisfy user's explicit msi=on request, fail */ + error_append_hint(&err, "You have to use msi=auto (default) or " + "msi=off with this machine type.\n"); + error_propagate(errp, err); + return; + } else if (ret) { + /* With msi=auto, we fall back to MSI off silently */ + s->msi = ON_OFF_AUTO_OFF; + error_free(err); + } + } + memory_region_init_io(&s->mmio_io, OBJECT(s), &megasas_mmio_ops, s, "megasas-mmio", 0x4000); memory_region_init_io(&s->port_io, OBJECT(s), &megasas_port_ops, s, @@ -2345,10 +2365,6 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp) memory_region_init_io(&s->queue_io, OBJECT(s), &megasas_queue_ops, s, "megasas-queue", 0x40000); - if (megasas_use_msi(s) && - msi_init(dev, 0x50, 1, true, false)) { - s->msi = ON_OFF_AUTO_OFF; - } if (megasas_use_msix(s) && msix_init(dev, 15, &s->mmio_io, b->mmio_bar, 0x2000, &s->mmio_io, b->mmio_bar, 0x3800, 0x68)) { diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c index 40033c4399..bb056c4e41 100644 --- a/hw/scsi/mptsas.c +++ b/hw/scsi/mptsas.c @@ -32,7 +32,7 @@ #include "hw/scsi/scsi.h" #include "block/scsi.h" #include "trace.h" - +#include "qapi/error.h" #include "mptsas.h" #include "mpi.h" @@ -1273,10 +1273,33 @@ static void mptsas_scsi_init(PCIDevice *dev, Error **errp) { DeviceState *d = DEVICE(dev); MPTSASState *s = MPT_SAS(dev); + Error *err = NULL; + int ret; dev->config[PCI_LATENCY_TIMER] = 0; dev->config[PCI_INTERRUPT_PIN] = 0x01; + if (s->msi != ON_OFF_AUTO_OFF) { + ret = msi_init(dev, 0, 1, true, false, &err); + /* Any error other than -ENOTSUP(board's MSI support is broken) + * is a programming error */ + assert(!ret || ret == -ENOTSUP); + if (ret && s->msi == ON_OFF_AUTO_ON) { + /* Can't satisfy user's explicit msi=on request, fail */ + error_append_hint(&err, "You have to use msi=auto (default) or " + "msi=off with this machine type.\n"); + error_propagate(errp, err); + s->msi_in_use = false; + return; + } else if (ret) { + /* With msi=auto, we fall back to MSI off silently */ + error_free(err); + s->msi_in_use = false; + } else { + s->msi_in_use = true; + } + } + memory_region_init_io(&s->mmio_io, OBJECT(s), &mptsas_mmio_ops, s, "mptsas-mmio", 0x4000); memory_region_init_io(&s->port_io, OBJECT(s), &mptsas_port_ops, s, @@ -1284,12 +1307,6 @@ static void mptsas_scsi_init(PCIDevice *dev, Error **errp) memory_region_init_io(&s->diag_io, OBJECT(s), &mptsas_diag_ops, s, "mptsas-diag", 0x10000); - if (s->msi != ON_OFF_AUTO_OFF && - msi_init(dev, 0, 1, true, false) >= 0) { - /* TODO check for errors */ - s->msi_in_use = true; - } - pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &s->port_io); pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32, &s->mmio_io); diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c index e035fcea34..ecd6077f4b 100644 --- a/hw/scsi/vmw_pvscsi.c +++ b/hw/scsi/vmw_pvscsi.c @@ -1063,7 +1063,7 @@ pvscsi_init_msi(PVSCSIState *s) PCIDevice *d = PCI_DEVICE(s); res = msi_init(d, PVSCSI_MSI_OFFSET(s), PVSCSI_MSIX_NUM_VECTORS, - PVSCSI_USE_64BIT, PVSCSI_PER_VECTOR_MASK); + PVSCSI_USE_64BIT, PVSCSI_PER_VECTOR_MASK, NULL); if (res < 0) { trace_pvscsi_init_msi_fail(res); s->msi_used = false; diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c index 0a5510fbc2..1a3377f038 100644 --- a/hw/usb/hcd-xhci.c +++ b/hw/usb/hcd-xhci.c @@ -26,6 +26,7 @@ #include "hw/pci/msi.h" #include "hw/pci/msix.h" #include "trace.h" +#include "qapi/error.h" //#define DEBUG_XHCI //#define DEBUG_DATA @@ -3581,6 +3582,7 @@ static void usb_xhci_init(XHCIState *xhci) static void usb_xhci_realize(struct PCIDevice *dev, Error **errp) { int i, ret; + Error *err = NULL; XHCIState *xhci = XHCI(dev); @@ -3591,6 +3593,23 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp) usb_xhci_init(xhci); + if (xhci->msi != ON_OFF_AUTO_OFF) { + ret = msi_init(dev, 0x70, xhci->numintrs, true, false, &err); + /* Any error other than -ENOTSUP(board's MSI support is broken) + * is a programming error */ + assert(!ret || ret == -ENOTSUP); + if (ret && xhci->msi == ON_OFF_AUTO_ON) { + /* Can't satisfy user's explicit msi=on request, fail */ + error_append_hint(&err, "You have to use msi=auto (default) or " + "msi=off with this machine type.\n"); + error_propagate(errp, err); + return; + } + assert(!err || xhci->msi == ON_OFF_AUTO_AUTO); + /* With msi=auto, we fall back to MSI off silently */ + error_free(err); + } + if (xhci->numintrs > MAXINTRS) { xhci->numintrs = MAXINTRS; } @@ -3648,10 +3667,6 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp) assert(ret >= 0); } - if (xhci->msi != ON_OFF_AUTO_OFF) { - /* TODO check for errors */ - msi_init(dev, 0x70, xhci->numintrs, true, false); - } if (xhci->msix != ON_OFF_AUTO_OFF) { /* TODO check for errors */ msix_init(dev, xhci->numintrs, diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index f2c679e47c..44783c50ab 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -31,6 +31,7 @@ #include "sysemu/sysemu.h" #include "pci.h" #include "trace.h" +#include "qapi/error.h" #define MSIX_CAP_LENGTH 12 @@ -1170,6 +1171,7 @@ static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos) uint16_t ctrl; bool msi_64bit, msi_maskbit; int ret, entries; + Error *err = NULL; if (pread(vdev->vbasedev.fd, &ctrl, sizeof(ctrl), vdev->config_offset + pos + PCI_CAP_FLAGS) != sizeof(ctrl)) { @@ -1183,12 +1185,13 @@ static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos) trace_vfio_msi_setup(vdev->vbasedev.name, pos); - ret = msi_init(&vdev->pdev, pos, entries, msi_64bit, msi_maskbit); + ret = msi_init(&vdev->pdev, pos, entries, msi_64bit, msi_maskbit, &err); if (ret < 0) { if (ret == -ENOTSUP) { return 0; } - error_report("vfio: msi_init failed"); + error_prepend(&err, "vfio: msi_init failed: "); + error_report_err(err); return ret; } vdev->msi_cap_size = 0xa + (msi_maskbit ? 0xa : 0) + (msi_64bit ? 0x4 : 0); diff --git a/include/hw/pci/msi.h b/include/hw/pci/msi.h index 8124908abd..4837bcf490 100644 --- a/include/hw/pci/msi.h +++ b/include/hw/pci/msi.h @@ -35,7 +35,8 @@ void msi_set_message(PCIDevice *dev, MSIMessage msg); MSIMessage msi_get_message(PCIDevice *dev, unsigned int vector); bool msi_enabled(const PCIDevice *dev); int msi_init(struct PCIDevice *dev, uint8_t offset, - unsigned int nr_vectors, bool msi64bit, bool msi_per_vector_mask); + unsigned int nr_vectors, bool msi64bit, + bool msi_per_vector_mask, Error **errp); void msi_uninit(struct PCIDevice *dev); void msi_reset(PCIDevice *dev); void msi_notify(PCIDevice *dev, unsigned int vector); From afea4e1410654154018587dd35c1b250ba4d8ec4 Mon Sep 17 00:00:00 2001 From: Cao jin Date: Mon, 20 Jun 2016 14:13:40 +0800 Subject: [PATCH 26/30] megasas: remove unnecessary megasas_use_msi() megasas overwrites user configuration when msi_init fail to flag internal msi state, which is unsuitable. megasa_use_msi() is unnecessary, we can call msi_uninit() directly when unrealize, even no need to call msi_enabled() first. cc: Hannes Reinecke cc: Paolo Bonzini cc: Markus Armbruster cc: Marcel Apfelbaum cc: Michael S. Tsirkin Acked-by: Hannes Reinecke Reviewed-by: Markus Armbruster Signed-off-by: Cao jin Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/scsi/megasas.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c index 6eb57ff61b..52a41239cf 100644 --- a/hw/scsi/megasas.c +++ b/hw/scsi/megasas.c @@ -155,11 +155,6 @@ static bool megasas_use_queue64(MegasasState *s) return s->flags & MEGASAS_MASK_USE_QUEUE64; } -static bool megasas_use_msi(MegasasState *s) -{ - return s->msi != ON_OFF_AUTO_OFF; -} - static bool megasas_use_msix(MegasasState *s) { return s->msix != ON_OFF_AUTO_OFF; @@ -2307,9 +2302,7 @@ static void megasas_scsi_uninit(PCIDevice *d) if (megasas_use_msix(s)) { msix_uninit(d, &s->mmio_io, &s->mmio_io); } - if (megasas_use_msi(s)) { - msi_uninit(d); - } + msi_uninit(d); } static const struct SCSIBusInfo megasas_scsi_info = { @@ -2340,7 +2333,7 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp) /* Interrupt pin 1 */ pci_conf[PCI_INTERRUPT_PIN] = 0x01; - if (megasas_use_msi(s)) { + if (s->msi != ON_OFF_AUTO_OFF) { ret = msi_init(dev, 0x50, 1, true, false, &err); /* Any error other than -ENOTSUP(board's MSI support is broken) * is a programming error */ From 2e2aa31674444b61e79536a90d63a90572e695c8 Mon Sep 17 00:00:00 2001 From: Cao jin Date: Mon, 20 Jun 2016 14:13:41 +0800 Subject: [PATCH 27/30] mptsas: remove unnecessary internal msi state flag internal flag msi_in_use in unnecessary, msi_uninit() could be called directly, and msi_enabled() is enough to check device msi state. cc: Markus Armbruster cc: Marcel Apfelbaum cc: Paolo Bonzini cc: Michael S. Tsirkin Reviewed-by: Markus Armbruster Signed-off-by: Cao jin Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/scsi/mptsas.c | 18 ++++++------------ hw/scsi/mptsas.h | 2 -- 2 files changed, 6 insertions(+), 14 deletions(-) diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c index bb056c4e41..1ae32fb7d3 100644 --- a/hw/scsi/mptsas.c +++ b/hw/scsi/mptsas.c @@ -63,7 +63,7 @@ static void mptsas_update_interrupt(MPTSASState *s) PCIDevice *pci = (PCIDevice *) s; uint32_t state = s->intr_status & ~(s->intr_mask | MPI_HIS_IOP_DOORBELL_STATUS); - if (s->msi_in_use && msi_enabled(pci)) { + if (msi_enabled(pci)) { if (state) { trace_mptsas_irq_msi(s); msi_notify(pci, 0); @@ -1289,15 +1289,12 @@ static void mptsas_scsi_init(PCIDevice *dev, Error **errp) error_append_hint(&err, "You have to use msi=auto (default) or " "msi=off with this machine type.\n"); error_propagate(errp, err); - s->msi_in_use = false; return; - } else if (ret) { - /* With msi=auto, we fall back to MSI off silently */ - error_free(err); - s->msi_in_use = false; - } else { - s->msi_in_use = true; } + assert(!err || s->msi == ON_OFF_AUTO_AUTO); + /* With msi=auto, we fall back to MSI off silently */ + error_free(err); + } memory_region_init_io(&s->mmio_io, OBJECT(s), &mptsas_mmio_ops, s, @@ -1337,9 +1334,7 @@ static void mptsas_scsi_uninit(PCIDevice *dev) MPTSASState *s = MPT_SAS(dev); qemu_bh_delete(s->request_bh); - if (s->msi_in_use) { - msi_uninit(dev); - } + msi_uninit(dev); } static void mptsas_reset(DeviceState *dev) @@ -1375,7 +1370,6 @@ static const VMStateDescription vmstate_mptsas = { .post_load = mptsas_post_load, .fields = (VMStateField[]) { VMSTATE_PCI_DEVICE(dev, MPTSASState), - VMSTATE_BOOL(msi_in_use, MPTSASState), VMSTATE_UINT32(state, MPTSASState), VMSTATE_UINT8(who_init, MPTSASState), diff --git a/hw/scsi/mptsas.h b/hw/scsi/mptsas.h index 0436a33911..da014a397e 100644 --- a/hw/scsi/mptsas.h +++ b/hw/scsi/mptsas.h @@ -31,8 +31,6 @@ struct MPTSASState { OnOffAuto msi; uint64_t sas_addr; - bool msi_in_use; - /* Doorbell register */ uint32_t state; uint8_t who_init; From 1070048eae6d1adb971af749c92ef39a3168cfb4 Mon Sep 17 00:00:00 2001 From: Cao jin Date: Mon, 20 Jun 2016 14:13:42 +0800 Subject: [PATCH 28/30] vmxnet3: remove unnecessary internal msi state flag Internal flag msi_used is unnecessary, it has the same effect as msi_enabled(). msi_uninit() could be called directly without risk. cc: Paolo Bonzini cc: Dmitry Fleytman cc: Markus Armbruster cc: Marcel Apfelbaum cc: Michael S. Tsirkin Reviewed-by: Markus Armbruster Signed-off-by: Cao jin Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/net/vmxnet3.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c index f24298cc49..5994890691 100644 --- a/hw/net/vmxnet3.c +++ b/hw/net/vmxnet3.c @@ -283,8 +283,6 @@ typedef struct { /* Whether MSI-X support was installed successfully */ bool msix_used; - /* Whether MSI support was installed successfully */ - bool msi_used; hwaddr drv_shmem; hwaddr temp_shared_guest_driver_memory; @@ -366,7 +364,7 @@ static bool _vmxnet3_assert_interrupt_line(VMXNET3State *s, uint32_t int_idx) msix_notify(d, int_idx); return false; } - if (s->msi_used && msi_enabled(d)) { + if (msi_enabled(d)) { VMW_IRPRN("Sending MSI notification for vector %u", int_idx); msi_notify(d, int_idx); return false; @@ -390,7 +388,7 @@ static void _vmxnet3_deassert_interrupt_line(VMXNET3State *s, int lidx) * This function should never be called for MSI(X) interrupts * because deassertion never required for message interrupts */ - assert(!s->msi_used || !msi_enabled(d)); + assert(!msi_enabled(d)); VMW_IRPRN("Deasserting line for interrupt %u", lidx); pci_irq_deassert(d); @@ -427,7 +425,7 @@ static void vmxnet3_trigger_interrupt(VMXNET3State *s, int lidx) goto do_automask; } - if (s->msi_used && msi_enabled(d) && s->auto_int_masking) { + if (msi_enabled(d) && s->auto_int_masking) { goto do_automask; } @@ -1425,8 +1423,8 @@ static void vmxnet3_update_features(VMXNET3State *s) static bool vmxnet3_verify_intx(VMXNET3State *s, int intx) { - return s->msix_used || s->msi_used || (intx == - (pci_get_byte(s->parent_obj.config + PCI_INTERRUPT_PIN) - 1)); + return s->msix_used || msi_enabled(PCI_DEVICE(s)) + || intx == pci_get_byte(s->parent_obj.config + PCI_INTERRUPT_PIN) - 1; } static void vmxnet3_validate_interrupt_idx(bool is_msix, int idx) @@ -2221,9 +2219,7 @@ vmxnet3_cleanup_msi(VMXNET3State *s) { PCIDevice *d = PCI_DEVICE(s); - if (s->msi_used) { - msi_uninit(d); - } + msi_uninit(d); } static void @@ -2314,7 +2310,6 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp) /* Any error other than -ENOTSUP(board's MSI support is broken) * is a programming error. Fall back to INTx silently on -ENOTSUP */ assert(!ret || ret == -ENOTSUP); - s->msi_used = !ret; if (!vmxnet3_init_msix(s)) { VMW_WRPRN("Failed to initialize MSI-X, configuration is inconsistent."); From 66bf7d58d830e6370895e4f1bb1257d135661872 Mon Sep 17 00:00:00 2001 From: Cao jin Date: Mon, 20 Jun 2016 14:13:43 +0800 Subject: [PATCH 29/30] e1000e: remove unnecessary internal msi state flag Internal big flag E1000E_USE_MSI is unnecessary, also is the helper function: e1000e_init_msi(), e1000e_cleanup_msi(), so, remove them all. cc: Dmitry Fleytman cc: Jason Wang cc: Markus Armbruster cc: Marcel Apfelbaum cc: Michael S. Tsirkin Signed-off-by: Cao jin Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Reviewed-by: Markus Armbruster --- hw/net/e1000e.c | 33 +++++++-------------------------- 1 file changed, 7 insertions(+), 26 deletions(-) diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c index a06d1848d2..c7d33ee395 100644 --- a/hw/net/e1000e.c +++ b/hw/net/e1000e.c @@ -89,8 +89,7 @@ typedef struct E1000EState { #define E1000E_MSIX_TABLE (0x0000) #define E1000E_MSIX_PBA (0x2000) -#define E1000E_USE_MSI BIT(0) -#define E1000E_USE_MSIX BIT(1) +#define E1000E_USE_MSIX BIT(0) static uint64_t e1000e_mmio_read(void *opaque, hwaddr addr, unsigned size) @@ -263,28 +262,6 @@ static void e1000e_core_realize(E1000EState *s) s->core.owner_nic = s->nic; } -static void -e1000e_init_msi(E1000EState *s) -{ - int res; - - res = msi_init(PCI_DEVICE(s), 0xD0, 1, true, false, NULL); - - if (!res) { - s->intr_state |= E1000E_USE_MSI; - } else { - trace_e1000e_msi_init_fail(res); - } -} - -static void -e1000e_cleanup_msi(E1000EState *s) -{ - if (s->intr_state & E1000E_USE_MSI) { - msi_uninit(PCI_DEVICE(s)); - } -} - static void e1000e_unuse_msix_vectors(E1000EState *s, int num_vectors) { @@ -440,6 +417,7 @@ static void e1000e_pci_realize(PCIDevice *pci_dev, Error **errp) static const uint16_t e1000e_dsn_offset = 0x140; E1000EState *s = E1000E(pci_dev); uint8_t *macaddr; + int ret; trace_e1000e_cb_pci_realize(); @@ -489,7 +467,10 @@ static void e1000e_pci_realize(PCIDevice *pci_dev, Error **errp) hw_error("Failed to initialize PCIe capability"); } - e1000e_init_msi(s); + ret = msi_init(PCI_DEVICE(s), 0xD0, 1, true, false, NULL); + if (ret) { + trace_e1000e_msi_init_fail(ret); + } if (e1000e_add_pm_capability(pci_dev, e1000e_pmrb_offset, PCI_PM_CAP_DSI) < 0) { @@ -528,7 +509,7 @@ static void e1000e_pci_uninit(PCIDevice *pci_dev) qemu_del_nic(s->nic); e1000e_cleanup_msix(s); - e1000e_cleanup_msi(s); + msi_uninit(pci_dev); } static void e1000e_qdev_reset(DeviceState *dev) From 269fe4c3ab0cf29329317eb868f8ec90ac761b41 Mon Sep 17 00:00:00 2001 From: Cao jin Date: Mon, 20 Jun 2016 14:13:44 +0800 Subject: [PATCH 30/30] vmw_pvscsi: remove unnecessary internal msi state flag Internal flag msi_used is uncesessary, msi_uninit() could be called directly, msi_enabled() is enough to check device msi state. But for migration compatibility, keep the field in structure. cc: Paolo Bonzini cc: Dmitry Fleytman cc: Markus Armbruster cc: Marcel Apfelbaum cc: Michael S. Tsirkin Signed-off-by: Cao jin Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Reviewed-by: Markus Armbruster --- hw/scsi/vmw_pvscsi.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c index ecd6077f4b..da71c8c8a5 100644 --- a/hw/scsi/vmw_pvscsi.c +++ b/hw/scsi/vmw_pvscsi.c @@ -121,8 +121,7 @@ typedef struct { uint8_t msg_ring_info_valid; /* Whether message ring initialized */ uint8_t use_msg; /* Whether to use message ring */ - uint8_t msi_used; /* Whether MSI support was installed successfully */ - + uint8_t msi_used; /* For migration compatibility */ PVSCSIRingInfo rings; /* Data transfer rings manager */ uint32_t resetting; /* Reset in progress */ @@ -362,7 +361,7 @@ pvscsi_update_irq_status(PVSCSIState *s) trace_pvscsi_update_irq_level(should_raise, s->reg_interrupt_enabled, s->reg_interrupt_status); - if (s->msi_used && msi_enabled(d)) { + if (msi_enabled(d)) { if (should_raise) { trace_pvscsi_update_irq_msi(); msi_notify(d, PVSCSI_VECTOR_COMPLETION); @@ -1077,9 +1076,7 @@ pvscsi_cleanup_msi(PVSCSIState *s) { PCIDevice *d = PCI_DEVICE(s); - if (s->msi_used) { - msi_uninit(d); - } + msi_uninit(d); } static const MemoryRegionOps pvscsi_ops = {