* [Qemu-devel] [PATCH v4 3/5] pcihp: make pci_read() mmio calback compatible with legacy ACPI hotplug
2014-02-03 10:44 [Qemu-devel] [PATCH v4 0/5] pc: make ACPI pcihp more reusable Igor Mammedov
2014-02-03 10:44 ` [Qemu-devel] [PATCH v4 1/5] pcihp: replace enable|disable_device() with oneliners Igor Mammedov
2014-02-03 10:44 ` [Qemu-devel] [PATCH v4 2/5] pcihp: make PCI hotplug mmio handlers indifferent to PCI_HOTPLUG_ADDR Igor Mammedov
@ 2014-02-03 10:44 ` Igor Mammedov
2014-02-03 10:45 ` [Qemu-devel] [PATCH v4 4/5] pcihp: remove unused AcpiPciHpPciStatus.device_present field Igor Mammedov
` (2 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Igor Mammedov @ 2014-02-03 10:44 UTC (permalink / raw)
To: qemu-devel; +Cc: aliguori, mst
due to recent change introduced by:
"pcihp: reduce number of device check events"
'up' field is cleared right after it's read.
This is incompatible with legacy BIOS ACPI code
where PCNF ACPI method reads this field 32 times.
To make pci_read mmio callback compatible with legacy
'up' behavior, pcihp code will need to know in which
mode it runs add 'legacy_piix' field to AcpiPciHpState
structure and alter register behavior accordingly.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
- keep "acpi-pci-hotplug-with-bridge-support" property
and tweek pcihp init API to initialize .legacy_piix field
---
hw/acpi/pcihp.c | 7 +++++--
hw/acpi/piix4.c | 3 ++-
include/hw/acpi/pcihp.h | 3 ++-
3 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index 64c8cf2..974f01c 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -215,7 +215,9 @@ static uint64_t pci_read(void *opaque, hwaddr addr, unsigned int size)
switch (addr) {
case PCI_UP_BASE:
val = s->acpi_pcihp_pci_status[bsel].up;
- s->acpi_pcihp_pci_status[bsel].up = 0;
+ if (!s->legacy_piix) {
+ s->acpi_pcihp_pci_status[bsel].up = 0;
+ }
ACPI_PCIHP_DPRINTF("pci_up_read %" PRIu32 "\n", val);
break;
case PCI_DOWN_BASE:
@@ -273,9 +275,10 @@ static const MemoryRegionOps acpi_pcihp_io_ops = {
};
void acpi_pcihp_init(AcpiPciHpState *s, PCIBus *root_bus,
- MemoryRegion *address_space_io)
+ MemoryRegion *address_space_io, bool bridges_enabled)
{
s->root= root_bus;
+ s->legacy_piix = !bridges_enabled;
memory_region_init_io(&s->io, NULL, &acpi_pcihp_io_ops, s,
"acpi-pci-hotplug",
PCI_HOTPLUG_SIZE);
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 5d55a3c..2aedfe5 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -695,7 +695,8 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe);
if (s->use_acpi_pci_hotplug) {
- acpi_pcihp_init(&s->acpi_pci_hotplug, bus, parent);
+ acpi_pcihp_init(&s->acpi_pci_hotplug, bus, parent,
+ s->use_acpi_pci_hotplug);
} else {
memory_region_init_io(&s->io_pci, OBJECT(s), &piix4_pci_ops, s,
"acpi-pci-hotplug", PCI_HOTPLUG_SIZE);
diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
index aa297c2..02d3ce3 100644
--- a/include/hw/acpi/pcihp.h
+++ b/include/hw/acpi/pcihp.h
@@ -46,10 +46,11 @@ typedef struct AcpiPciHpState {
uint32_t hotplug_select;
PCIBus *root;
MemoryRegion io;
+ bool legacy_piix;
} AcpiPciHpState;
void acpi_pcihp_init(AcpiPciHpState *, PCIBus *root,
- MemoryRegion *address_space_io);
+ MemoryRegion *address_space_io, bool bridges_enabled);
/* Invoke on device hotplug */
int acpi_pcihp_device_hotplug(AcpiPciHpState *, PCIDevice *,
--
1.7.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* [Qemu-devel] [PATCH v4 5/5] hw:piix4:acpi: reuse pcihp code for legacy PCI hotplug
2014-02-03 10:44 [Qemu-devel] [PATCH v4 0/5] pc: make ACPI pcihp more reusable Igor Mammedov
` (3 preceding siblings ...)
2014-02-03 10:45 ` [Qemu-devel] [PATCH v4 4/5] pcihp: remove unused AcpiPciHpPciStatus.device_present field Igor Mammedov
@ 2014-02-03 10:45 ` Igor Mammedov
2014-02-03 13:58 ` [Qemu-devel] [PATCH v4 0/5] pc: make ACPI pcihp more reusable Michael S. Tsirkin
5 siblings, 0 replies; 7+ messages in thread
From: Igor Mammedov @ 2014-02-03 10:45 UTC (permalink / raw)
To: qemu-devel; +Cc: aliguori, mst
reduces acpi PCI hotplug code duplication by ~200LOC
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v3:
- remove exessive checks for compat mode
- replace up/down fields of pci_status with corresponding fields
form AcpiPciHpPciStatus in vmstate wich allow to remove extra
50 LOC of legacy code
- move legacy initialization logic to pcihp.c
v2:
- replace obsolete 'device_present' with 'up' field
- add/set ACPI_PCIHP_PROP_BSEL to 0 when running in compatibility
mode with old machine types.
---
hw/acpi/pcihp.c | 23 ++++-
hw/acpi/piix4.c | 211 ++++-------------------------------------------
include/hw/acpi/pcihp.h | 1 +
3 files changed, 34 insertions(+), 201 deletions(-)
diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index 974f01c..1ce6fc2 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -46,8 +46,9 @@
# define ACPI_PCIHP_DPRINTF(format, ...) do { } while (0)
#endif
-#define PCI_HOTPLUG_ADDR 0xae00
-#define PCI_HOTPLUG_SIZE 0x0014
+#define ACPI_PCIHP_ADDR 0xae00
+#define ACPI_PCIHP_SIZE 0x0014
+#define ACPI_PCIHP_LEGACY_SIZE 0x000f
#define PCI_UP_BASE 0x0000
#define PCI_DOWN_BASE 0x0004
#define PCI_EJ_BASE 0x0008
@@ -277,12 +278,24 @@ static const MemoryRegionOps acpi_pcihp_io_ops = {
void acpi_pcihp_init(AcpiPciHpState *s, PCIBus *root_bus,
MemoryRegion *address_space_io, bool bridges_enabled)
{
+ uint16_t io_size = ACPI_PCIHP_SIZE;
+
s->root= root_bus;
s->legacy_piix = !bridges_enabled;
+
+ if (s->legacy_piix) {
+ unsigned *bus_bsel = g_malloc(sizeof *bus_bsel);
+
+ io_size = ACPI_PCIHP_LEGACY_SIZE;
+
+ *bus_bsel = ACPI_PCIHP_BSEL_DEFAULT;
+ object_property_add_uint32_ptr(OBJECT(root_bus), ACPI_PCIHP_PROP_BSEL,
+ bus_bsel, NULL);
+ }
+
memory_region_init_io(&s->io, NULL, &acpi_pcihp_io_ops, s,
- "acpi-pci-hotplug",
- PCI_HOTPLUG_SIZE);
- memory_region_add_subregion(address_space_io, PCI_HOTPLUG_ADDR, &s->io);
+ "acpi-pci-hotplug", io_size);
+ memory_region_add_subregion(address_space_io, ACPI_PCIHP_ADDR, &s->io);
}
const VMStateDescription vmstate_acpi_pcihp_pci_status = {
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 2aedfe5..7a0efcb 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -44,13 +44,6 @@
#define GPE_BASE 0xafe0
#define GPE_LEN 4
-#define PCI_HOTPLUG_ADDR 0xae00
-#define PCI_HOTPLUG_SIZE 0x000f
-#define PCI_UP_BASE 0xae00
-#define PCI_DOWN_BASE 0xae04
-#define PCI_EJ_BASE 0xae08
-#define PCI_RMV_BASE 0xae0c
-
#define PIIX4_PCI_HOTPLUG_STATUS 2
struct pci_status {
@@ -80,13 +73,6 @@ typedef struct PIIX4PMState {
Notifier machine_ready;
Notifier powerdown_notifier;
- /* for legacy pci hotplug (compatible with qemu 1.6 and older) */
- MemoryRegion io_pci;
- struct pci_status pci0_status;
- uint32_t pci0_hotplug_enable;
- uint32_t pci0_slot_device_present;
-
- /* for new pci hotplug (with PCI2PCI bridge support) */
AcpiPciHpState acpi_pci_hotplug;
bool use_acpi_pci_hotplug;
@@ -170,17 +156,6 @@ static void pm_write_config(PCIDevice *d,
}
}
-static void vmstate_pci_status_pre_save(void *opaque)
-{
- struct pci_status *pci0_status = opaque;
- PIIX4PMState *s = container_of(pci0_status, PIIX4PMState, pci0_status);
-
- /* We no longer track up, so build a safe value for migrating
- * to a version that still does... of course these might get lost
- * by an old buggy implementation, but we try. */
- pci0_status->up = s->pci0_slot_device_present & s->pci0_hotplug_enable;
-}
-
static int vmstate_acpi_post_load(void *opaque, int version_id)
{
PIIX4PMState *s = opaque;
@@ -216,10 +191,9 @@ static const VMStateDescription vmstate_pci_status = {
.version_id = 1,
.minimum_version_id = 1,
.minimum_version_id_old = 1,
- .pre_save = vmstate_pci_status_pre_save,
.fields = (VMStateField []) {
- VMSTATE_UINT32(up, struct pci_status),
- VMSTATE_UINT32(down, struct pci_status),
+ VMSTATE_UINT32(up, struct AcpiPciHpPciStatus),
+ VMSTATE_UINT32(down, struct AcpiPciHpPciStatus),
VMSTATE_END_OF_LIST()
}
};
@@ -256,7 +230,8 @@ static int acpi_load_old(QEMUFile *f, void *opaque, int version_id)
qemu_get_be16s(f, &temp);
}
- ret = vmstate_load_state(f, &vmstate_pci_status, &s->pci0_status, 1);
+ ret = vmstate_load_state(f, &vmstate_pci_status,
+ &s->acpi_pci_hotplug.acpi_pcihp_pci_status[ACPI_PCIHP_BSEL_DEFAULT], 1);
return ret;
}
@@ -294,70 +269,18 @@ static const VMStateDescription vmstate_acpi = {
VMSTATE_TIMER(ar.tmr.timer, PIIX4PMState),
VMSTATE_INT64(ar.tmr.overflow_time, PIIX4PMState),
VMSTATE_STRUCT(ar.gpe, PIIX4PMState, 2, vmstate_gpe, ACPIGPE),
- VMSTATE_STRUCT_TEST(pci0_status, PIIX4PMState,
- vmstate_test_no_use_acpi_pci_hotplug,
- 2, vmstate_pci_status,
- struct pci_status),
+ VMSTATE_STRUCT_TEST(
+ acpi_pci_hotplug.acpi_pcihp_pci_status[ACPI_PCIHP_BSEL_DEFAULT],
+ PIIX4PMState,
+ vmstate_test_no_use_acpi_pci_hotplug,
+ 2, vmstate_pci_status,
+ struct AcpiPciHpPciStatus),
VMSTATE_PCI_HOTPLUG(acpi_pci_hotplug, PIIX4PMState,
vmstate_test_use_acpi_pci_hotplug),
VMSTATE_END_OF_LIST()
}
};
-static void acpi_piix_eject_slot(PIIX4PMState *s, unsigned slots)
-{
- BusChild *kid, *next;
- BusState *bus = qdev_get_parent_bus(DEVICE(s));
- int slot = ffs(slots) - 1;
- bool slot_free = true;
-
- /* Mark request as complete */
- s->pci0_status.down &= ~(1U << slot);
-
- QTAILQ_FOREACH_SAFE(kid, &bus->children, sibling, next) {
- DeviceState *qdev = kid->child;
- PCIDevice *dev = PCI_DEVICE(qdev);
- PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
- if (PCI_SLOT(dev->devfn) == slot) {
- if (pc->no_hotplug) {
- slot_free = false;
- } else {
- object_unparent(OBJECT(qdev));
- }
- }
- }
- if (slot_free) {
- s->pci0_slot_device_present &= ~(1U << slot);
- }
-}
-
-static void piix4_update_hotplug(PIIX4PMState *s)
-{
- BusState *bus = qdev_get_parent_bus(DEVICE(s));
- BusChild *kid, *next;
-
- /* Execute any pending removes during reset */
- while (s->pci0_status.down) {
- acpi_piix_eject_slot(s, s->pci0_status.down);
- }
-
- s->pci0_hotplug_enable = ~0;
- s->pci0_slot_device_present = 0;
-
- QTAILQ_FOREACH_SAFE(kid, &bus->children, sibling, next) {
- DeviceState *qdev = kid->child;
- PCIDevice *pdev = PCI_DEVICE(qdev);
- PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(pdev);
- int slot = PCI_SLOT(pdev->devfn);
-
- if (pc->no_hotplug) {
- s->pci0_hotplug_enable &= ~(1U << slot);
- }
-
- s->pci0_slot_device_present |= (1U << slot);
- }
-}
-
static void piix4_reset(void *opaque)
{
PIIX4PMState *s = opaque;
@@ -377,11 +300,7 @@ static void piix4_reset(void *opaque)
pci_conf[0x5B] = 0x02;
}
pm_io_space_update(s);
- if (s->use_acpi_pci_hotplug) {
- acpi_pcihp_reset(&s->acpi_pci_hotplug);
- } else {
- piix4_update_hotplug(s);
- }
+ acpi_pcihp_reset(&s->acpi_pci_hotplug);
}
static void piix4_pm_powerdown_req(Notifier *n, void *opaque)
@@ -428,6 +347,8 @@ static void piix4_pm_machine_ready(Notifier *n, void *opaque)
if (s->use_acpi_pci_hotplug) {
pci_for_each_bus(d->bus, piix4_update_bus_hotplug, s);
+ } else {
+ piix4_update_bus_hotplug(d->bus, s);
}
}
@@ -621,60 +542,6 @@ static const MemoryRegionOps piix4_gpe_ops = {
.endianness = DEVICE_LITTLE_ENDIAN,
};
-static uint64_t pci_read(void *opaque, hwaddr addr, unsigned int size)
-{
- PIIX4PMState *s = opaque;
- uint32_t val = 0;
-
- switch (addr) {
- case PCI_UP_BASE - PCI_HOTPLUG_ADDR:
- /* Manufacture an "up" value to cause a device check on any hotplug
- * slot with a device. Extra device checks are harmless. */
- val = s->pci0_slot_device_present & s->pci0_hotplug_enable;
- PIIX4_DPRINTF("pci_up_read %" PRIu32 "\n", val);
- break;
- case PCI_DOWN_BASE - PCI_HOTPLUG_ADDR:
- val = s->pci0_status.down;
- PIIX4_DPRINTF("pci_down_read %" PRIu32 "\n", val);
- break;
- case PCI_EJ_BASE - PCI_HOTPLUG_ADDR:
- /* No feature defined yet */
- PIIX4_DPRINTF("pci_features_read %" PRIu32 "\n", val);
- break;
- case PCI_RMV_BASE - PCI_HOTPLUG_ADDR:
- val = s->pci0_hotplug_enable;
- break;
- default:
- break;
- }
-
- return val;
-}
-
-static void pci_write(void *opaque, hwaddr addr, uint64_t data,
- unsigned int size)
-{
- switch (addr) {
- case PCI_EJ_BASE - PCI_HOTPLUG_ADDR:
- acpi_piix_eject_slot(opaque, (uint32_t)data);
- PIIX4_DPRINTF("pciej write %" HWADDR_PRIx " <== %" PRIu64 "\n",
- addr, data);
- break;
- default:
- break;
- }
-}
-
-static const MemoryRegionOps piix4_pci_ops = {
- .read = pci_read,
- .write = pci_write,
- .endianness = DEVICE_LITTLE_ENDIAN,
- .valid = {
- .min_access_size = 4,
- .max_access_size = 4,
- },
-};
-
static void piix4_cpu_added_req(Notifier *n, void *opaque)
{
PIIX4PMState *s = container_of(n, PIIX4PMState, cpu_added_notifier);
@@ -684,9 +551,6 @@ static void piix4_cpu_added_req(Notifier *n, void *opaque)
acpi_update_sci(&s->ar, s->irq);
}
-static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
- PCIHotplugState state);
-
static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
PCIBus *bus, PIIX4PMState *s)
{
@@ -694,56 +558,11 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
"acpi-gpe0", GPE_LEN);
memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe);
- if (s->use_acpi_pci_hotplug) {
- acpi_pcihp_init(&s->acpi_pci_hotplug, bus, parent,
- s->use_acpi_pci_hotplug);
- } else {
- memory_region_init_io(&s->io_pci, OBJECT(s), &piix4_pci_ops, s,
- "acpi-pci-hotplug", PCI_HOTPLUG_SIZE);
- memory_region_add_subregion(parent, PCI_HOTPLUG_ADDR,
- &s->io_pci);
- pci_bus_hotplug(bus, piix4_device_hotplug, DEVICE(s));
- }
+ acpi_pcihp_init(&s->acpi_pci_hotplug, bus, parent,
+ s->use_acpi_pci_hotplug);
AcpiCpuHotplug_init(parent, OBJECT(s), &s->gpe_cpu,
PIIX4_CPU_HOTPLUG_IO_BASE);
s->cpu_added_notifier.notify = piix4_cpu_added_req;
qemu_register_cpu_added_notifier(&s->cpu_added_notifier);
}
-
-static void enable_device(PIIX4PMState *s, int slot)
-{
- s->ar.gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS;
- s->pci0_slot_device_present |= (1U << slot);
-}
-
-static void disable_device(PIIX4PMState *s, int slot)
-{
- s->ar.gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS;
- s->pci0_status.down |= (1U << slot);
-}
-
-static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
- PCIHotplugState state)
-{
- int slot = PCI_SLOT(dev->devfn);
- PIIX4PMState *s = PIIX4_PM(qdev);
-
- /* Don't send event when device is enabled during qemu machine creation:
- * it is present on boot, no hotplug event is necessary. We do send an
- * event when the device is disabled later. */
- if (state == PCI_COLDPLUG_ENABLED) {
- s->pci0_slot_device_present |= (1U << slot);
- return 0;
- }
-
- if (state == PCI_HOTPLUG_ENABLED) {
- enable_device(s, slot);
- } else {
- disable_device(s, slot);
- }
-
- acpi_update_sci(&s->ar, s->irq);
-
- return 0;
-}
diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
index 1fd90e1..0a90e4a 100644
--- a/include/hw/acpi/pcihp.h
+++ b/include/hw/acpi/pcihp.h
@@ -39,6 +39,7 @@ typedef struct AcpiPciHpPciStatus {
#define ACPI_PCIHP_PROP_BSEL "acpi-pcihp-bsel"
#define ACPI_PCIHP_MAX_HOTPLUG_BUS 256
+#define ACPI_PCIHP_BSEL_DEFAULT 0x0
typedef struct AcpiPciHpState {
AcpiPciHpPciStatus acpi_pcihp_pci_status[ACPI_PCIHP_MAX_HOTPLUG_BUS];
--
1.7.1
^ permalink raw reply related [flat|nested] 7+ messages in thread