* [Qemu-devel] [PATCH v2 0/4] pc: make ACPI pcihp more reusable
@ 2014-01-27 15:39 Igor Mammedov
2014-01-27 15:39 ` [Qemu-devel] [PATCH v2 1/4] hw:piix4:acpi: replace enable|disable_device() with oneliners Igor Mammedov
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Igor Mammedov @ 2014-01-27 15:39 UTC (permalink / raw)
To: qemu-devel; +Cc: aliguori, mst
changes since v1:
- add BSEL property to root bus when in compatibility mode
as result dropped "hw:acpi:pcihp: assume root PCI bus if bus has no ACPI_PCIHP_PROP_BSEL property"
- rebasing on top of "pcihp: reduce number of device check events"
added patch 3/4.
Reuses new pcihp code for handling legacy PCI hotplug in
acpi/piix4_pm, which significantly reduces code duplication
between piix4_pm and pcihp amd makes pcihp less dependend
on piix4 specific details.
It also allows cleaner rebase of generic hotplug refactoring,
by not increasing code duplication since it will need to be
done only for pcihp.
Git tree for testing based on mst's PCI tree:
https://github.com/imammedo/qemu/commits/pcihp_cleanup_v2
Igor Mammedov (4):
hw:piix4:acpi: replace enable|disable_device() with oneliners
hw:piix4:acpi: make PCI hotplug mmio handlers indifferent to
PCI_HOTPLUG_ADDR
pcihp: make pci_read() mmio calback compatible with legacy ACPI
hotplug
hw:piix4:acpi: reuse pcihp code for legacy PCI hotplug
hw/acpi/pcihp.c | 52 +++++-------
hw/acpi/piix4.c | 211 ++++++++---------------------------------------
include/hw/acpi/pcihp.h | 9 ++-
3 files changed, 61 insertions(+), 211 deletions(-)
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH v2 1/4] hw:piix4:acpi: replace enable|disable_device() with oneliners
2014-01-27 15:39 [Qemu-devel] [PATCH v2 0/4] pc: make ACPI pcihp more reusable Igor Mammedov
@ 2014-01-27 15:39 ` Igor Mammedov
2014-01-27 15:39 ` [Qemu-devel] [PATCH v2 2/4] hw:piix4:acpi: make PCI hotplug mmio handlers indifferent to PCI_HOTPLUG_ADDR Igor Mammedov
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: Igor Mammedov @ 2014-01-27 15:39 UTC (permalink / raw)
To: qemu-devel; +Cc: aliguori, mst
enable_device() and disable_device() functions aren't reused anywere,
so replace them with respective oneliners at call sites.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
hw/acpi/pcihp.c | 14 ++------------
1 files changed, 2 insertions(+), 12 deletions(-)
diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index 4345f5d..464739a 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -177,16 +177,6 @@ void acpi_pcihp_reset(AcpiPciHpState *s)
acpi_pcihp_update(s);
}
-static void enable_device(AcpiPciHpState *s, unsigned bsel, int slot)
-{
- s->acpi_pcihp_pci_status[bsel].up |= (1U << slot);
-}
-
-static void disable_device(AcpiPciHpState *s, unsigned bsel, int slot)
-{
- s->acpi_pcihp_pci_status[bsel].down |= (1U << slot);
-}
-
int acpi_pcihp_device_hotplug(AcpiPciHpState *s, PCIDevice *dev,
PCIHotplugState state)
{
@@ -204,9 +194,9 @@ int acpi_pcihp_device_hotplug(AcpiPciHpState *s, PCIDevice *dev,
}
if (state == PCI_HOTPLUG_ENABLED) {
- enable_device(s, bsel, slot);
+ s->acpi_pcihp_pci_status[bsel].up |= (1U << slot);
} else {
- disable_device(s, bsel, slot);
+ s->acpi_pcihp_pci_status[bsel].down |= (1U << slot);
}
return 0;
--
1.7.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH v2 2/4] hw:piix4:acpi: make PCI hotplug mmio handlers indifferent to PCI_HOTPLUG_ADDR
2014-01-27 15:39 [Qemu-devel] [PATCH v2 0/4] pc: make ACPI pcihp more reusable Igor Mammedov
2014-01-27 15:39 ` [Qemu-devel] [PATCH v2 1/4] hw:piix4:acpi: replace enable|disable_device() with oneliners Igor Mammedov
@ 2014-01-27 15:39 ` Igor Mammedov
2014-01-27 15:39 ` [Qemu-devel] [PATCH v2 3/4] pcihp: make pci_read() mmio calback compatible with legacy ACPI hotplug Igor Mammedov
2014-01-27 15:39 ` [Qemu-devel] [PATCH v2 4/4] hw:piix4:acpi: reuse pcihp code for legacy PCI hotplug Igor Mammedov
3 siblings, 0 replies; 9+ messages in thread
From: Igor Mammedov @ 2014-01-27 15:39 UTC (permalink / raw)
To: qemu-devel; +Cc: aliguori, mst
... removes dependency of mmio handler on PCI_HOTPLUG_ADDR.
It will be needed in case of Q35 where base could be different.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
hw/acpi/pcihp.c | 24 ++++++++++++------------
1 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index 464739a..64c8cf2 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -48,11 +48,11 @@
#define PCI_HOTPLUG_ADDR 0xae00
#define PCI_HOTPLUG_SIZE 0x0014
-#define PCI_UP_BASE 0xae00
-#define PCI_DOWN_BASE 0xae04
-#define PCI_EJ_BASE 0xae08
-#define PCI_RMV_BASE 0xae0c
-#define PCI_SEL_BASE 0xae10
+#define PCI_UP_BASE 0x0000
+#define PCI_DOWN_BASE 0x0004
+#define PCI_EJ_BASE 0x0008
+#define PCI_RMV_BASE 0x000c
+#define PCI_SEL_BASE 0x0010
typedef struct AcpiPciHpFind {
int bsel;
@@ -213,24 +213,24 @@ static uint64_t pci_read(void *opaque, hwaddr addr, unsigned int size)
}
switch (addr) {
- case PCI_UP_BASE - PCI_HOTPLUG_ADDR:
+ case PCI_UP_BASE:
val = s->acpi_pcihp_pci_status[bsel].up;
s->acpi_pcihp_pci_status[bsel].up = 0;
ACPI_PCIHP_DPRINTF("pci_up_read %" PRIu32 "\n", val);
break;
- case PCI_DOWN_BASE - PCI_HOTPLUG_ADDR:
+ case PCI_DOWN_BASE:
val = s->acpi_pcihp_pci_status[bsel].down;
ACPI_PCIHP_DPRINTF("pci_down_read %" PRIu32 "\n", val);
break;
- case PCI_EJ_BASE - PCI_HOTPLUG_ADDR:
+ case PCI_EJ_BASE:
/* No feature defined yet */
ACPI_PCIHP_DPRINTF("pci_features_read %" PRIu32 "\n", val);
break;
- case PCI_RMV_BASE - PCI_HOTPLUG_ADDR:
+ case PCI_RMV_BASE:
val = s->acpi_pcihp_pci_status[bsel].hotplug_enable;
ACPI_PCIHP_DPRINTF("pci_rmv_read %" PRIu32 "\n", val);
break;
- case PCI_SEL_BASE - PCI_HOTPLUG_ADDR:
+ case PCI_SEL_BASE:
val = s->hotplug_select;
ACPI_PCIHP_DPRINTF("pci_sel_read %" PRIu32 "\n", val);
default:
@@ -245,7 +245,7 @@ static void pci_write(void *opaque, hwaddr addr, uint64_t data,
{
AcpiPciHpState *s = opaque;
switch (addr) {
- case PCI_EJ_BASE - PCI_HOTPLUG_ADDR:
+ case PCI_EJ_BASE:
if (s->hotplug_select >= ACPI_PCIHP_MAX_HOTPLUG_BUS) {
break;
}
@@ -253,7 +253,7 @@ static void pci_write(void *opaque, hwaddr addr, uint64_t data,
ACPI_PCIHP_DPRINTF("pciej write %" HWADDR_PRIx " <== %" PRIu64 "\n",
addr, data);
break;
- case PCI_SEL_BASE - PCI_HOTPLUG_ADDR:
+ case PCI_SEL_BASE:
s->hotplug_select = data;
ACPI_PCIHP_DPRINTF("pcisel write %" HWADDR_PRIx " <== %" PRIu64 "\n",
addr, data);
--
1.7.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH v2 3/4] pcihp: make pci_read() mmio calback compatible with legacy ACPI hotplug
2014-01-27 15:39 [Qemu-devel] [PATCH v2 0/4] pc: make ACPI pcihp more reusable Igor Mammedov
2014-01-27 15:39 ` [Qemu-devel] [PATCH v2 1/4] hw:piix4:acpi: replace enable|disable_device() with oneliners Igor Mammedov
2014-01-27 15:39 ` [Qemu-devel] [PATCH v2 2/4] hw:piix4:acpi: make PCI hotplug mmio handlers indifferent to PCI_HOTPLUG_ADDR Igor Mammedov
@ 2014-01-27 15:39 ` Igor Mammedov
2014-01-30 11:28 ` Michael S. Tsirkin
2014-01-27 15:39 ` [Qemu-devel] [PATCH v2 4/4] hw:piix4:acpi: reuse pcihp code for legacy PCI hotplug Igor Mammedov
3 siblings, 1 reply; 9+ messages in thread
From: Igor Mammedov @ 2014-01-27 15:39 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 so move 'use_acpi_pci_hotplug' into
AcpiPciHpState structure and alter register behavior
accordingly.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
hw/acpi/pcihp.c | 4 +++-
hw/acpi/piix4.c | 13 ++++++-------
include/hw/acpi/pcihp.h | 1 +
3 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index 64c8cf2..e48b758 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->use_acpi_pci_hotplug) {
+ s->acpi_pcihp_pci_status[bsel].up = 0;
+ }
ACPI_PCIHP_DPRINTF("pci_up_read %" PRIu32 "\n", val);
break;
case PCI_DOWN_BASE:
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 5d55a3c..a20453d 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -88,7 +88,6 @@ typedef struct PIIX4PMState {
/* for new pci hotplug (with PCI2PCI bridge support) */
AcpiPciHpState acpi_pci_hotplug;
- bool use_acpi_pci_hotplug;
uint8_t disable_s3;
uint8_t disable_s4;
@@ -263,13 +262,13 @@ static int acpi_load_old(QEMUFile *f, void *opaque, int version_id)
static bool vmstate_test_use_acpi_pci_hotplug(void *opaque, int version_id)
{
PIIX4PMState *s = opaque;
- return s->use_acpi_pci_hotplug;
+ return s->acpi_pci_hotplug.use_acpi_pci_hotplug;
}
static bool vmstate_test_no_use_acpi_pci_hotplug(void *opaque, int version_id)
{
PIIX4PMState *s = opaque;
- return !s->use_acpi_pci_hotplug;
+ return !s->acpi_pci_hotplug.use_acpi_pci_hotplug;
}
/* qemu-kvm 1.2 uses version 3 but advertised as 2
@@ -377,7 +376,7 @@ static void piix4_reset(void *opaque)
pci_conf[0x5B] = 0x02;
}
pm_io_space_update(s);
- if (s->use_acpi_pci_hotplug) {
+ if (s->acpi_pci_hotplug.use_acpi_pci_hotplug) {
acpi_pcihp_reset(&s->acpi_pci_hotplug);
} else {
piix4_update_hotplug(s);
@@ -426,7 +425,7 @@ static void piix4_pm_machine_ready(Notifier *n, void *opaque)
pci_conf[0x67] = (memory_region_present(io_as, 0x3f8) ? 0x08 : 0) |
(memory_region_present(io_as, 0x2f8) ? 0x90 : 0);
- if (s->use_acpi_pci_hotplug) {
+ if (s->acpi_pci_hotplug.use_acpi_pci_hotplug) {
pci_for_each_bus(d->bus, piix4_update_bus_hotplug, s);
}
}
@@ -551,7 +550,7 @@ static Property piix4_pm_properties[] = {
DEFINE_PROP_UINT8(ACPI_PM_PROP_S4_DISABLED, PIIX4PMState, disable_s4, 0),
DEFINE_PROP_UINT8(ACPI_PM_PROP_S4_VAL, PIIX4PMState, s4_val, 2),
DEFINE_PROP_BOOL("acpi-pci-hotplug-with-bridge-support", PIIX4PMState,
- use_acpi_pci_hotplug, true),
+ acpi_pci_hotplug.use_acpi_pci_hotplug, true),
DEFINE_PROP_END_OF_LIST(),
};
@@ -694,7 +693,7 @@ 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) {
+ if (s->acpi_pci_hotplug.use_acpi_pci_hotplug) {
acpi_pcihp_init(&s->acpi_pci_hotplug, bus, parent);
} else {
memory_region_init_io(&s->io_pci, OBJECT(s), &piix4_pci_ops, s,
diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
index aa297c2..cff5270 100644
--- a/include/hw/acpi/pcihp.h
+++ b/include/hw/acpi/pcihp.h
@@ -46,6 +46,7 @@ typedef struct AcpiPciHpState {
uint32_t hotplug_select;
PCIBus *root;
MemoryRegion io;
+ bool use_acpi_pci_hotplug;
} AcpiPciHpState;
void acpi_pcihp_init(AcpiPciHpState *, PCIBus *root,
--
1.7.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH v2 4/4] hw:piix4:acpi: reuse pcihp code for legacy PCI hotplug
2014-01-27 15:39 [Qemu-devel] [PATCH v2 0/4] pc: make ACPI pcihp more reusable Igor Mammedov
` (2 preceding siblings ...)
2014-01-27 15:39 ` [Qemu-devel] [PATCH v2 3/4] pcihp: make pci_read() mmio calback compatible with legacy ACPI hotplug Igor Mammedov
@ 2014-01-27 15:39 ` Igor Mammedov
2014-01-30 11:25 ` Michael S. Tsirkin
3 siblings, 1 reply; 9+ messages in thread
From: Igor Mammedov @ 2014-01-27 15:39 UTC (permalink / raw)
To: qemu-devel; +Cc: aliguori, mst
reduces acpi PCI hotplug code duplication by ~150LOC,
and makes pcihp less dependend on piix specific code.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
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 | 10 +--
hw/acpi/piix4.c | 204 +++++++----------------------------------------
include/hw/acpi/pcihp.h | 8 ++-
3 files changed, 40 insertions(+), 182 deletions(-)
diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index e48b758..d17040c 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -46,8 +46,6 @@
# define ACPI_PCIHP_DPRINTF(format, ...) do { } while (0)
#endif
-#define PCI_HOTPLUG_ADDR 0xae00
-#define PCI_HOTPLUG_SIZE 0x0014
#define PCI_UP_BASE 0x0000
#define PCI_DOWN_BASE 0x0004
#define PCI_EJ_BASE 0x0008
@@ -274,14 +272,14 @@ static const MemoryRegionOps acpi_pcihp_io_ops = {
},
};
-void acpi_pcihp_init(AcpiPciHpState *s, PCIBus *root_bus,
- MemoryRegion *address_space_io)
+void acpi_pcihp_init(AcpiPciHpState *s, PCIBus *root_bus, uint16_t io_base,
+ uint16_t io_size, MemoryRegion *address_space_io)
{
s->root= root_bus;
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);
+ io_size);
+ memory_region_add_subregion(address_space_io, io_base, &s->io);
}
const VMStateDescription vmstate_acpi_pcihp_pci_status = {
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index a20453d..d04ac2f 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,8 +73,8 @@ typedef struct PIIX4PMState {
Notifier machine_ready;
Notifier powerdown_notifier;
- /* for legacy pci hotplug (compatible with qemu 1.6 and older) */
- MemoryRegion io_pci;
+ /* for legacy pci hotplug (compatible with qemu 1.6 and older)
+ * used only for migration and updated in pre_save() */
struct pci_status pci0_status;
uint32_t pci0_hotplug_enable;
uint32_t pci0_slot_device_present;
@@ -174,16 +167,24 @@ static void vmstate_pci_status_pre_save(void *opaque)
struct pci_status *pci0_status = opaque;
PIIX4PMState *s = container_of(pci0_status, PIIX4PMState, pci0_status);
+ pci0_status->down = s->acpi_pci_hotplug.acpi_pcihp_pci_status[0].down;
/* 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;
+ pci0_status->up = s->acpi_pci_hotplug.acpi_pcihp_pci_status[0].up;
}
static int vmstate_acpi_post_load(void *opaque, int version_id)
{
PIIX4PMState *s = opaque;
+ if (!s->acpi_pci_hotplug.use_acpi_pci_hotplug) {
+ s->acpi_pci_hotplug.acpi_pcihp_pci_status[0].down =
+ s->pci0_status.down;
+ s->acpi_pci_hotplug.acpi_pcihp_pci_status[0].up =
+ s->pci0_status.up;
+ }
+
pm_io_space_update(s);
return 0;
}
@@ -303,60 +304,6 @@ static const VMStateDescription vmstate_acpi = {
}
};
-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;
@@ -376,11 +323,7 @@ static void piix4_reset(void *opaque)
pci_conf[0x5B] = 0x02;
}
pm_io_space_update(s);
- if (s->acpi_pci_hotplug.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)
@@ -408,6 +351,11 @@ static int piix4_acpi_pci_hotplug(DeviceState *qdev, PCIDevice *dev,
static void piix4_update_bus_hotplug(PCIBus *bus, void *opaque)
{
PIIX4PMState *s = opaque;
+
+ if (!s->acpi_pci_hotplug.use_acpi_pci_hotplug &&
+ (bus != s->acpi_pci_hotplug.root)) {
+ return;
+ }
pci_bus_hotplug(bus, piix4_acpi_pci_hotplug, DEVICE(s));
}
@@ -425,9 +373,7 @@ static void piix4_pm_machine_ready(Notifier *n, void *opaque)
pci_conf[0x67] = (memory_region_present(io_as, 0x3f8) ? 0x08 : 0) |
(memory_region_present(io_as, 0x2f8) ? 0x90 : 0);
- if (s->acpi_pci_hotplug.use_acpi_pci_hotplug) {
- pci_for_each_bus(d->bus, piix4_update_bus_hotplug, s);
- }
+ pci_for_each_bus(d->bus, piix4_update_bus_hotplug, s);
}
static void piix4_pm_add_propeties(PIIX4PMState *s)
@@ -620,60 +566,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);
@@ -683,65 +575,29 @@ 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)
{
+ uint16_t pcihp_io_size = PIIX_PCI_HOTPLUG_SIZE;
+
memory_region_init_io(&s->io_gpe, OBJECT(s), &piix4_gpe_ops, s,
"acpi-gpe0", GPE_LEN);
memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe);
- if (s->acpi_pci_hotplug.use_acpi_pci_hotplug) {
- acpi_pcihp_init(&s->acpi_pci_hotplug, bus, parent);
- } 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));
+ if (!s->acpi_pci_hotplug.use_acpi_pci_hotplug) {
+ unsigned *bus_bsel = g_malloc(sizeof *bus_bsel);
+
+ pcihp_io_size = PIIX_PCI_HOTPLUG_LEGACY_SIZE;
+
+ *bus_bsel = 0;
+ object_property_add_uint32_ptr(OBJECT(bus), ACPI_PCIHP_PROP_BSEL,
+ bus_bsel, NULL);
}
+ acpi_pcihp_init(&s->acpi_pci_hotplug, bus, PIIX_PCI_HOTPLUG_ADDR,
+ pcihp_io_size, parent);
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 cff5270..afe1e8c 100644
--- a/include/hw/acpi/pcihp.h
+++ b/include/hw/acpi/pcihp.h
@@ -31,6 +31,10 @@
#include <qemu/typedefs.h>
#include "hw/pci/pci.h" /* for PCIHotplugState */
+#define PIIX_PCI_HOTPLUG_ADDR 0xae00
+#define PIIX_PCI_HOTPLUG_SIZE 0x0014
+#define PIIX_PCI_HOTPLUG_LEGACY_SIZE 0x000f
+
typedef struct AcpiPciHpPciStatus {
uint32_t up;
uint32_t down;
@@ -49,8 +53,8 @@ typedef struct AcpiPciHpState {
bool use_acpi_pci_hotplug;
} AcpiPciHpState;
-void acpi_pcihp_init(AcpiPciHpState *, PCIBus *root,
- MemoryRegion *address_space_io);
+void acpi_pcihp_init(AcpiPciHpState *, PCIBus *root, uint16_t io_base,
+ uint16_t io_size, MemoryRegion *address_space_io);
/* Invoke on device hotplug */
int acpi_pcihp_device_hotplug(AcpiPciHpState *, PCIDevice *,
--
1.7.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/4] hw:piix4:acpi: reuse pcihp code for legacy PCI hotplug
2014-01-27 15:39 ` [Qemu-devel] [PATCH v2 4/4] hw:piix4:acpi: reuse pcihp code for legacy PCI hotplug Igor Mammedov
@ 2014-01-30 11:25 ` Michael S. Tsirkin
2014-01-30 12:20 ` Igor Mammedov
0 siblings, 1 reply; 9+ messages in thread
From: Michael S. Tsirkin @ 2014-01-30 11:25 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel, aliguori
On Mon, Jan 27, 2014 at 04:39:55PM +0100, Igor Mammedov wrote:
> reduces acpi PCI hotplug code duplication by ~150LOC,
> and makes pcihp less dependend on piix specific code.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> 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 | 10 +--
> hw/acpi/piix4.c | 204 +++++++----------------------------------------
> include/hw/acpi/pcihp.h | 8 ++-
> 3 files changed, 40 insertions(+), 182 deletions(-)
>
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index e48b758..d17040c 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -46,8 +46,6 @@
> # define ACPI_PCIHP_DPRINTF(format, ...) do { } while (0)
> #endif
>
> -#define PCI_HOTPLUG_ADDR 0xae00
> -#define PCI_HOTPLUG_SIZE 0x0014
> #define PCI_UP_BASE 0x0000
> #define PCI_DOWN_BASE 0x0004
> #define PCI_EJ_BASE 0x0008
> @@ -274,14 +272,14 @@ static const MemoryRegionOps acpi_pcihp_io_ops = {
> },
> };
>
> -void acpi_pcihp_init(AcpiPciHpState *s, PCIBus *root_bus,
> - MemoryRegion *address_space_io)
> +void acpi_pcihp_init(AcpiPciHpState *s, PCIBus *root_bus, uint16_t io_base,
> + uint16_t io_size, MemoryRegion *address_space_io)
OK this is a trick: io_size can be smaller than PIIX_PCI_HOTPLUG_SIZE.
If it is smaller, then only first X registers are enabled.
I think it's not a great API.
Just add a flag legacy_piix to acpi_pcihp_init, set size
accordingly.
> {
> s->root= root_bus;
> 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);
> + io_size);
> + memory_region_add_subregion(address_space_io, io_base, &s->io);
> }
>
> const VMStateDescription vmstate_acpi_pcihp_pci_status = {
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index a20453d..d04ac2f 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,8 +73,8 @@ typedef struct PIIX4PMState {
> Notifier machine_ready;
> Notifier powerdown_notifier;
>
> - /* for legacy pci hotplug (compatible with qemu 1.6 and older) */
> - MemoryRegion io_pci;
> + /* for legacy pci hotplug (compatible with qemu 1.6 and older)
> + * used only for migration and updated in pre_save() */
Pls make it looks like this:
+ /* for legacy pci hotplug (compatible with qemu 1.6 and older)
+ * used only for migration and updated in pre_save()
+ */
or drop it completely.
> struct pci_status pci0_status;
> uint32_t pci0_hotplug_enable;
> uint32_t pci0_slot_device_present;
> @@ -174,16 +167,24 @@ static void vmstate_pci_status_pre_save(void *opaque)
> struct pci_status *pci0_status = opaque;
> PIIX4PMState *s = container_of(pci0_status, PIIX4PMState, pci0_status);
>
> + pci0_status->down = s->acpi_pci_hotplug.acpi_pcihp_pci_status[0].down;
> /* 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. */
This comment is really wrong now, isn't it?
> - pci0_status->up = s->pci0_slot_device_present & s->pci0_hotplug_enable;
> + pci0_status->up = s->acpi_pci_hotplug.acpi_pcihp_pci_status[0].up;
> }
>
> static int vmstate_acpi_post_load(void *opaque, int version_id)
> {
> PIIX4PMState *s = opaque;
>
> + if (!s->acpi_pci_hotplug.use_acpi_pci_hotplug) {
> + s->acpi_pci_hotplug.acpi_pcihp_pci_status[0].down =
> + s->pci0_status.down;
> + s->acpi_pci_hotplug.acpi_pcihp_pci_status[0].up =
> + s->pci0_status.up;
> + }
> +
> pm_io_space_update(s);
> return 0;
> }
OK if all we do is this, how about just giving
s->acpi_pci_hotplug.acpi_pcihp_pci_status[0]
to vmstate?
> @@ -303,60 +304,6 @@ static const VMStateDescription vmstate_acpi = {
> }
> };
>
> -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;
> @@ -376,11 +323,7 @@ static void piix4_reset(void *opaque)
> pci_conf[0x5B] = 0x02;
> }
> pm_io_space_update(s);
> - if (s->acpi_pci_hotplug.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)
> @@ -408,6 +351,11 @@ static int piix4_acpi_pci_hotplug(DeviceState *qdev, PCIDevice *dev,
> static void piix4_update_bus_hotplug(PCIBus *bus, void *opaque)
> {
> PIIX4PMState *s = opaque;
> +
> + if (!s->acpi_pci_hotplug.use_acpi_pci_hotplug &&
> + (bus != s->acpi_pci_hotplug.root)) {
> + return;
> + }
That's an unnecessary complication.
Just call pci_bus_hotplug(d->bus, piix4_acpi_pci_hotplug, s);
in compat mode like we always did.
> pci_bus_hotplug(bus, piix4_acpi_pci_hotplug, DEVICE(s));
> }
>
> @@ -425,9 +373,7 @@ static void piix4_pm_machine_ready(Notifier *n, void *opaque)
> pci_conf[0x67] = (memory_region_present(io_as, 0x3f8) ? 0x08 : 0) |
> (memory_region_present(io_as, 0x2f8) ? 0x90 : 0);
>
> - if (s->acpi_pci_hotplug.use_acpi_pci_hotplug) {
> - pci_for_each_bus(d->bus, piix4_update_bus_hotplug, s);
> - }
> + pci_for_each_bus(d->bus, piix4_update_bus_hotplug, s);
> }
>
> static void piix4_pm_add_propeties(PIIX4PMState *s)
same as above.
> @@ -620,60 +566,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);
> @@ -683,65 +575,29 @@ 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)
> {
> + uint16_t pcihp_io_size = PIIX_PCI_HOTPLUG_SIZE;
> +
> memory_region_init_io(&s->io_gpe, OBJECT(s), &piix4_gpe_ops, s,
> "acpi-gpe0", GPE_LEN);
> memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe);
>
> - if (s->acpi_pci_hotplug.use_acpi_pci_hotplug) {
> - acpi_pcihp_init(&s->acpi_pci_hotplug, bus, parent);
> - } 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));
> + if (!s->acpi_pci_hotplug.use_acpi_pci_hotplug) {
> + unsigned *bus_bsel = g_malloc(sizeof *bus_bsel);
> +
> + pcihp_io_size = PIIX_PCI_HOTPLUG_LEGACY_SIZE;
> +
> + *bus_bsel = 0;
> + object_property_add_uint32_ptr(OBJECT(bus), ACPI_PCIHP_PROP_BSEL,
> + bus_bsel, NULL);
> }
Okay, but please add a comment here.
Also how about
#define ACPI_PCIHP_BSEL_DEFAULT 0
and use this in pcihp.c on reset?
> + acpi_pcihp_init(&s->acpi_pci_hotplug, bus, PIIX_PCI_HOTPLUG_ADDR,
> + pcihp_io_size, parent);
>
> 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 cff5270..afe1e8c 100644
> --- a/include/hw/acpi/pcihp.h
> +++ b/include/hw/acpi/pcihp.h
> @@ -31,6 +31,10 @@
> #include <qemu/typedefs.h>
> #include "hw/pci/pci.h" /* for PCIHotplugState */
>
> +#define PIIX_PCI_HOTPLUG_ADDR 0xae00
> +#define PIIX_PCI_HOTPLUG_SIZE 0x0014
Pls change prefix to ACPI_PCIHP_
Also - required in header?
> +#define PIIX_PCI_HOTPLUG_LEGACY_SIZE 0x000f
> +
> typedef struct AcpiPciHpPciStatus {
> uint32_t up;
> uint32_t down;
> @@ -49,8 +53,8 @@ typedef struct AcpiPciHpState {
> bool use_acpi_pci_hotplug;
> } AcpiPciHpState;
>
> -void acpi_pcihp_init(AcpiPciHpState *, PCIBus *root,
> - MemoryRegion *address_space_io);
> +void acpi_pcihp_init(AcpiPciHpState *, PCIBus *root, uint16_t io_base,
> + uint16_t io_size, MemoryRegion *address_space_io);
>
> /* Invoke on device hotplug */
> int acpi_pcihp_device_hotplug(AcpiPciHpState *, PCIDevice *,
> --
> 1.7.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/4] pcihp: make pci_read() mmio calback compatible with legacy ACPI hotplug
2014-01-27 15:39 ` [Qemu-devel] [PATCH v2 3/4] pcihp: make pci_read() mmio calback compatible with legacy ACPI hotplug Igor Mammedov
@ 2014-01-30 11:28 ` Michael S. Tsirkin
0 siblings, 0 replies; 9+ messages in thread
From: Michael S. Tsirkin @ 2014-01-30 11:28 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel, aliguori
On Mon, Jan 27, 2014 at 04:39:54PM +0100, Igor Mammedov wrote:
> 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 so move 'use_acpi_pci_hotplug' into
> AcpiPciHpState structure and alter register behavior
> accordingly.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
OK but the field name is really ugly.
It should be legacy_piix.
If you don't want to change property names, simply
check property and set it in init.
> ---
> hw/acpi/pcihp.c | 4 +++-
> hw/acpi/piix4.c | 13 ++++++-------
> include/hw/acpi/pcihp.h | 1 +
> 3 files changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index 64c8cf2..e48b758 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->use_acpi_pci_hotplug) {
> + s->acpi_pcihp_pci_status[bsel].up = 0;
> + }
> ACPI_PCIHP_DPRINTF("pci_up_read %" PRIu32 "\n", val);
> break;
> case PCI_DOWN_BASE:
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index 5d55a3c..a20453d 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -88,7 +88,6 @@ typedef struct PIIX4PMState {
>
> /* for new pci hotplug (with PCI2PCI bridge support) */
> AcpiPciHpState acpi_pci_hotplug;
> - bool use_acpi_pci_hotplug;
>
> uint8_t disable_s3;
> uint8_t disable_s4;
> @@ -263,13 +262,13 @@ static int acpi_load_old(QEMUFile *f, void *opaque, int version_id)
> static bool vmstate_test_use_acpi_pci_hotplug(void *opaque, int version_id)
> {
> PIIX4PMState *s = opaque;
> - return s->use_acpi_pci_hotplug;
> + return s->acpi_pci_hotplug.use_acpi_pci_hotplug;
> }
>
> static bool vmstate_test_no_use_acpi_pci_hotplug(void *opaque, int version_id)
> {
> PIIX4PMState *s = opaque;
> - return !s->use_acpi_pci_hotplug;
> + return !s->acpi_pci_hotplug.use_acpi_pci_hotplug;
> }
>
> /* qemu-kvm 1.2 uses version 3 but advertised as 2
> @@ -377,7 +376,7 @@ static void piix4_reset(void *opaque)
> pci_conf[0x5B] = 0x02;
> }
> pm_io_space_update(s);
> - if (s->use_acpi_pci_hotplug) {
> + if (s->acpi_pci_hotplug.use_acpi_pci_hotplug) {
> acpi_pcihp_reset(&s->acpi_pci_hotplug);
> } else {
> piix4_update_hotplug(s);
> @@ -426,7 +425,7 @@ static void piix4_pm_machine_ready(Notifier *n, void *opaque)
> pci_conf[0x67] = (memory_region_present(io_as, 0x3f8) ? 0x08 : 0) |
> (memory_region_present(io_as, 0x2f8) ? 0x90 : 0);
>
> - if (s->use_acpi_pci_hotplug) {
> + if (s->acpi_pci_hotplug.use_acpi_pci_hotplug) {
> pci_for_each_bus(d->bus, piix4_update_bus_hotplug, s);
> }
> }
> @@ -551,7 +550,7 @@ static Property piix4_pm_properties[] = {
> DEFINE_PROP_UINT8(ACPI_PM_PROP_S4_DISABLED, PIIX4PMState, disable_s4, 0),
> DEFINE_PROP_UINT8(ACPI_PM_PROP_S4_VAL, PIIX4PMState, s4_val, 2),
> DEFINE_PROP_BOOL("acpi-pci-hotplug-with-bridge-support", PIIX4PMState,
> - use_acpi_pci_hotplug, true),
> + acpi_pci_hotplug.use_acpi_pci_hotplug, true),
> DEFINE_PROP_END_OF_LIST(),
> };
>
> @@ -694,7 +693,7 @@ 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) {
> + if (s->acpi_pci_hotplug.use_acpi_pci_hotplug) {
> acpi_pcihp_init(&s->acpi_pci_hotplug, bus, parent);
> } else {
> memory_region_init_io(&s->io_pci, OBJECT(s), &piix4_pci_ops, s,
> diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
> index aa297c2..cff5270 100644
> --- a/include/hw/acpi/pcihp.h
> +++ b/include/hw/acpi/pcihp.h
> @@ -46,6 +46,7 @@ typedef struct AcpiPciHpState {
> uint32_t hotplug_select;
> PCIBus *root;
> MemoryRegion io;
> + bool use_acpi_pci_hotplug;
> } AcpiPciHpState;
>
> void acpi_pcihp_init(AcpiPciHpState *, PCIBus *root,
> --
> 1.7.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/4] hw:piix4:acpi: reuse pcihp code for legacy PCI hotplug
2014-01-30 11:25 ` Michael S. Tsirkin
@ 2014-01-30 12:20 ` Igor Mammedov
2014-01-30 13:08 ` Michael S. Tsirkin
0 siblings, 1 reply; 9+ messages in thread
From: Igor Mammedov @ 2014-01-30 12:20 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel, aliguori
On Thu, 30 Jan 2014 13:25:39 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Mon, Jan 27, 2014 at 04:39:55PM +0100, Igor Mammedov wrote:
> > reduces acpi PCI hotplug code duplication by ~150LOC,
> > and makes pcihp less dependend on piix specific code.
> >
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > 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 | 10 +--
> > hw/acpi/piix4.c | 204 +++++++----------------------------------------
> > include/hw/acpi/pcihp.h | 8 ++-
> > 3 files changed, 40 insertions(+), 182 deletions(-)
> >
> > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > index e48b758..d17040c 100644
> > --- a/hw/acpi/pcihp.c
> > +++ b/hw/acpi/pcihp.c
> > @@ -46,8 +46,6 @@
> > # define ACPI_PCIHP_DPRINTF(format, ...) do { } while (0)
> > #endif
> >
> > -#define PCI_HOTPLUG_ADDR 0xae00
> > -#define PCI_HOTPLUG_SIZE 0x0014
> > #define PCI_UP_BASE 0x0000
> > #define PCI_DOWN_BASE 0x0004
> > #define PCI_EJ_BASE 0x0008
> > @@ -274,14 +272,14 @@ static const MemoryRegionOps acpi_pcihp_io_ops = {
> > },
> > };
> >
> > -void acpi_pcihp_init(AcpiPciHpState *s, PCIBus *root_bus,
> > - MemoryRegion *address_space_io)
> > +void acpi_pcihp_init(AcpiPciHpState *s, PCIBus *root_bus, uint16_t io_base,
> > + uint16_t io_size, MemoryRegion *address_space_io)
>
> OK this is a trick: io_size can be smaller than PIIX_PCI_HOTPLUG_SIZE.
> If it is smaller, then only first X registers are enabled.
> I think it's not a great API.
> Just add a flag legacy_piix to acpi_pcihp_init, set size
> accordingly.
Size set at init time in piix4_acpi_system_hot_add_init() according to
machine type. Why it will be smaller than PIIX_PCI_HOTPLUG_SIZE?
It will be programming error to pass an incorrect size.
I think it's not good to hardcode piix4 specific constants in code
that will be shared with q35 unless there is compelling reason to do so.
Passing io_base & io_size to acpi_pcihp_init() is the same as
passing region address & size to memory_region_* API.
So I think isolating device specific code from common one is better
API in this case than what you suggest.
>
>
> > {
> > s->root= root_bus;
> > 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);
> > + io_size);
> > + memory_region_add_subregion(address_space_io, io_base, &s->io);
> > }
> >
> > const VMStateDescription vmstate_acpi_pcihp_pci_status = {
> > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> > index a20453d..d04ac2f 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,8 +73,8 @@ typedef struct PIIX4PMState {
> > Notifier machine_ready;
> > Notifier powerdown_notifier;
> >
> > - /* for legacy pci hotplug (compatible with qemu 1.6 and older) */
> > - MemoryRegion io_pci;
> > + /* for legacy pci hotplug (compatible with qemu 1.6 and older)
> > + * used only for migration and updated in pre_save() */
>
> Pls make it looks like this:
sure
>
> + /* for legacy pci hotplug (compatible with qemu 1.6 and older)
> + * used only for migration and updated in pre_save()
> + */
>
> or drop it completely.
>
> > struct pci_status pci0_status;
> > uint32_t pci0_hotplug_enable;
> > uint32_t pci0_slot_device_present;
> > @@ -174,16 +167,24 @@ static void vmstate_pci_status_pre_save(void *opaque)
> > struct pci_status *pci0_status = opaque;
> > PIIX4PMState *s = container_of(pci0_status, PIIX4PMState, pci0_status);
> >
> > + pci0_status->down = s->acpi_pci_hotplug.acpi_pcihp_pci_status[0].down;
> > /* 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. */
>
> This comment is really wrong now, isn't it?
yep it should have been dropped in
"pcihp: reduce number of device check events" patch
I'll make a cleanup patch that would take care of it and device+present field.
>
> > - pci0_status->up = s->pci0_slot_device_present & s->pci0_hotplug_enable;
> > + pci0_status->up = s->acpi_pci_hotplug.acpi_pcihp_pci_status[0].up;
> > }
> >
> > static int vmstate_acpi_post_load(void *opaque, int version_id)
> > {
> > PIIX4PMState *s = opaque;
> >
> > + if (!s->acpi_pci_hotplug.use_acpi_pci_hotplug) {
> > + s->acpi_pci_hotplug.acpi_pcihp_pci_status[0].down =
> > + s->pci0_status.down;
> > + s->acpi_pci_hotplug.acpi_pcihp_pci_status[0].up =
> > + s->pci0_status.up;
> > + }
> > +
> > pm_io_space_update(s);
> > return 0;
> > }
>
> OK if all we do is this, how about just giving
> s->acpi_pci_hotplug.acpi_pcihp_pci_status[0]
> to vmstate?
not sure if it would work with old vmstate but, I'll try it.
>
> > @@ -303,60 +304,6 @@ static const VMStateDescription vmstate_acpi = {
> > }
> > };
> >
> > -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;
> > @@ -376,11 +323,7 @@ static void piix4_reset(void *opaque)
> > pci_conf[0x5B] = 0x02;
> > }
> > pm_io_space_update(s);
> > - if (s->acpi_pci_hotplug.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)
> > @@ -408,6 +351,11 @@ static int piix4_acpi_pci_hotplug(DeviceState *qdev, PCIDevice *dev,
> > static void piix4_update_bus_hotplug(PCIBus *bus, void *opaque)
> > {
> > PIIX4PMState *s = opaque;
> > +
> > + if (!s->acpi_pci_hotplug.use_acpi_pci_hotplug &&
> > + (bus != s->acpi_pci_hotplug.root)) {
> > + return;
> > + }
>
> That's an unnecessary complication.
> Just call pci_bus_hotplug(d->bus, piix4_acpi_pci_hotplug, s);
> in compat mode like we always did.
>
> > pci_bus_hotplug(bus, piix4_acpi_pci_hotplug, DEVICE(s));
> > }
> >
> > @@ -425,9 +373,7 @@ static void piix4_pm_machine_ready(Notifier *n, void *opaque)
> > pci_conf[0x67] = (memory_region_present(io_as, 0x3f8) ? 0x08 : 0) |
> > (memory_region_present(io_as, 0x2f8) ? 0x90 : 0);
> >
> > - if (s->acpi_pci_hotplug.use_acpi_pci_hotplug) {
> > - pci_for_each_bus(d->bus, piix4_update_bus_hotplug, s);
> > - }
> > + pci_for_each_bus(d->bus, piix4_update_bus_hotplug, s);
> > }
> >
> > static void piix4_pm_add_propeties(PIIX4PMState *s)
>
> same as above.
>
> > @@ -620,60 +566,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);
> > @@ -683,65 +575,29 @@ 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)
> > {
> > + uint16_t pcihp_io_size = PIIX_PCI_HOTPLUG_SIZE;
> > +
> > memory_region_init_io(&s->io_gpe, OBJECT(s), &piix4_gpe_ops, s,
> > "acpi-gpe0", GPE_LEN);
> > memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe);
> >
> > - if (s->acpi_pci_hotplug.use_acpi_pci_hotplug) {
> > - acpi_pcihp_init(&s->acpi_pci_hotplug, bus, parent);
> > - } 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));
> > + if (!s->acpi_pci_hotplug.use_acpi_pci_hotplug) {
> > + unsigned *bus_bsel = g_malloc(sizeof *bus_bsel);
> > +
> > + pcihp_io_size = PIIX_PCI_HOTPLUG_LEGACY_SIZE;
> > +
> > + *bus_bsel = 0;
> > + object_property_add_uint32_ptr(OBJECT(bus), ACPI_PCIHP_PROP_BSEL,
> > + bus_bsel, NULL);
> > }
>
> Okay, but please add a comment here.
> Also how about
> #define ACPI_PCIHP_BSEL_DEFAULT 0
> and use this in pcihp.c on reset?
>
> > + acpi_pcihp_init(&s->acpi_pci_hotplug, bus, PIIX_PCI_HOTPLUG_ADDR,
> > + pcihp_io_size, parent);
> >
> > 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 cff5270..afe1e8c 100644
> > --- a/include/hw/acpi/pcihp.h
> > +++ b/include/hw/acpi/pcihp.h
> > @@ -31,6 +31,10 @@
> > #include <qemu/typedefs.h>
> > #include "hw/pci/pci.h" /* for PCIHotplugState */
> >
> > +#define PIIX_PCI_HOTPLUG_ADDR 0xae00
> > +#define PIIX_PCI_HOTPLUG_SIZE 0x0014
>
> Pls change prefix to ACPI_PCIHP_
>
> Also - required in header?
not really, I'll put them into piix4.c and keep
PIIX_PCI_ prefix to reflect that it's piix4 only defines.
>
> > +#define PIIX_PCI_HOTPLUG_LEGACY_SIZE 0x000f
> > +
> > typedef struct AcpiPciHpPciStatus {
> > uint32_t up;
> > uint32_t down;
> > @@ -49,8 +53,8 @@ typedef struct AcpiPciHpState {
> > bool use_acpi_pci_hotplug;
> > } AcpiPciHpState;
> >
> > -void acpi_pcihp_init(AcpiPciHpState *, PCIBus *root,
> > - MemoryRegion *address_space_io);
> > +void acpi_pcihp_init(AcpiPciHpState *, PCIBus *root, uint16_t io_base,
> > + uint16_t io_size, MemoryRegion *address_space_io);
> >
> > /* Invoke on device hotplug */
> > int acpi_pcihp_device_hotplug(AcpiPciHpState *, PCIDevice *,
> > --
> > 1.7.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/4] hw:piix4:acpi: reuse pcihp code for legacy PCI hotplug
2014-01-30 12:20 ` Igor Mammedov
@ 2014-01-30 13:08 ` Michael S. Tsirkin
0 siblings, 0 replies; 9+ messages in thread
From: Michael S. Tsirkin @ 2014-01-30 13:08 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel, aliguori
On Thu, Jan 30, 2014 at 01:20:12PM +0100, Igor Mammedov wrote:
> On Thu, 30 Jan 2014 13:25:39 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > On Mon, Jan 27, 2014 at 04:39:55PM +0100, Igor Mammedov wrote:
> > > reduces acpi PCI hotplug code duplication by ~150LOC,
> > > and makes pcihp less dependend on piix specific code.
> > >
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > > 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 | 10 +--
> > > hw/acpi/piix4.c | 204 +++++++----------------------------------------
> > > include/hw/acpi/pcihp.h | 8 ++-
> > > 3 files changed, 40 insertions(+), 182 deletions(-)
> > >
> > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > > index e48b758..d17040c 100644
> > > --- a/hw/acpi/pcihp.c
> > > +++ b/hw/acpi/pcihp.c
> > > @@ -46,8 +46,6 @@
> > > # define ACPI_PCIHP_DPRINTF(format, ...) do { } while (0)
> > > #endif
> > >
> > > -#define PCI_HOTPLUG_ADDR 0xae00
> > > -#define PCI_HOTPLUG_SIZE 0x0014
> > > #define PCI_UP_BASE 0x0000
> > > #define PCI_DOWN_BASE 0x0004
> > > #define PCI_EJ_BASE 0x0008
> > > @@ -274,14 +272,14 @@ static const MemoryRegionOps acpi_pcihp_io_ops = {
> > > },
> > > };
> > >
> > > -void acpi_pcihp_init(AcpiPciHpState *s, PCIBus *root_bus,
> > > - MemoryRegion *address_space_io)
> > > +void acpi_pcihp_init(AcpiPciHpState *s, PCIBus *root_bus, uint16_t io_base,
> > > + uint16_t io_size, MemoryRegion *address_space_io)
> >
> > OK this is a trick: io_size can be smaller than PIIX_PCI_HOTPLUG_SIZE.
> > If it is smaller, then only first X registers are enabled.
> > I think it's not a great API.
> > Just add a flag legacy_piix to acpi_pcihp_init, set size
> > accordingly.
>
> Size set at init time in piix4_acpi_system_hot_add_init() according to
> machine type. Why it will be smaller than PIIX_PCI_HOTPLUG_SIZE?
that's what you do for legacy. why? because new registers
happen to be at the end. It's a trick and that's okay
since it saves lines of code, but let's keep this
trick local in acpi/pcihp.c. Don't leak it out to piix.
> It will be programming error to pass an incorrect size.
>
> I think it's not good to hardcode piix4 specific constants in code
> that will be shared with q35 unless there is compelling reason to do so.
Well the legacy mode is piix specific.
There's no way around it.
Let's just make it explicit, keep code in one place.
> Passing io_base & io_size to acpi_pcihp_init() is the same as
> passing region address & size to memory_region_* API.
> So I think isolating device specific code from common one is better
> API in this case than what you suggest.
Problem is it's not well isolated.
The idea of keeping all acpi based pci hotplug in one file
is appealing.
But we should not leak out the register layout otherwise
what's the point.
And size is part of layout.
> >
> >
> > > {
> > > s->root= root_bus;
> > > 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);
> > > + io_size);
> > > + memory_region_add_subregion(address_space_io, io_base, &s->io);
> > > }
> > >
> > > const VMStateDescription vmstate_acpi_pcihp_pci_status = {
> > > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> > > index a20453d..d04ac2f 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,8 +73,8 @@ typedef struct PIIX4PMState {
> > > Notifier machine_ready;
> > > Notifier powerdown_notifier;
> > >
> > > - /* for legacy pci hotplug (compatible with qemu 1.6 and older) */
> > > - MemoryRegion io_pci;
> > > + /* for legacy pci hotplug (compatible with qemu 1.6 and older)
> > > + * used only for migration and updated in pre_save() */
> >
> > Pls make it looks like this:
> sure
>
> >
> > + /* for legacy pci hotplug (compatible with qemu 1.6 and older)
> > + * used only for migration and updated in pre_save()
> > + */
> >
> > or drop it completely.
> >
> > > struct pci_status pci0_status;
> > > uint32_t pci0_hotplug_enable;
> > > uint32_t pci0_slot_device_present;
> > > @@ -174,16 +167,24 @@ static void vmstate_pci_status_pre_save(void *opaque)
> > > struct pci_status *pci0_status = opaque;
> > > PIIX4PMState *s = container_of(pci0_status, PIIX4PMState, pci0_status);
> > >
> > > + pci0_status->down = s->acpi_pci_hotplug.acpi_pcihp_pci_status[0].down;
> > > /* 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. */
> >
> > This comment is really wrong now, isn't it?
> yep it should have been dropped in
> "pcihp: reduce number of device check events" patch
> I'll make a cleanup patch that would take care of it and device+present field.
>
> >
> > > - pci0_status->up = s->pci0_slot_device_present & s->pci0_hotplug_enable;
> > > + pci0_status->up = s->acpi_pci_hotplug.acpi_pcihp_pci_status[0].up;
> > > }
> > >
> > > static int vmstate_acpi_post_load(void *opaque, int version_id)
> > > {
> > > PIIX4PMState *s = opaque;
> > >
> > > + if (!s->acpi_pci_hotplug.use_acpi_pci_hotplug) {
> > > + s->acpi_pci_hotplug.acpi_pcihp_pci_status[0].down =
> > > + s->pci0_status.down;
> > > + s->acpi_pci_hotplug.acpi_pcihp_pci_status[0].up =
> > > + s->pci0_status.up;
> > > + }
> > > +
> > > pm_io_space_update(s);
> > > return 0;
> > > }
> >
> > OK if all we do is this, how about just giving
> > s->acpi_pci_hotplug.acpi_pcihp_pci_status[0]
> > to vmstate?
> not sure if it would work with old vmstate but, I'll try it.
>
> >
> > > @@ -303,60 +304,6 @@ static const VMStateDescription vmstate_acpi = {
> > > }
> > > };
> > >
> > > -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;
> > > @@ -376,11 +323,7 @@ static void piix4_reset(void *opaque)
> > > pci_conf[0x5B] = 0x02;
> > > }
> > > pm_io_space_update(s);
> > > - if (s->acpi_pci_hotplug.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)
> > > @@ -408,6 +351,11 @@ static int piix4_acpi_pci_hotplug(DeviceState *qdev, PCIDevice *dev,
> > > static void piix4_update_bus_hotplug(PCIBus *bus, void *opaque)
> > > {
> > > PIIX4PMState *s = opaque;
> > > +
> > > + if (!s->acpi_pci_hotplug.use_acpi_pci_hotplug &&
> > > + (bus != s->acpi_pci_hotplug.root)) {
> > > + return;
> > > + }
> >
> > That's an unnecessary complication.
> > Just call pci_bus_hotplug(d->bus, piix4_acpi_pci_hotplug, s);
> > in compat mode like we always did.
> >
> > > pci_bus_hotplug(bus, piix4_acpi_pci_hotplug, DEVICE(s));
> > > }
> > >
> > > @@ -425,9 +373,7 @@ static void piix4_pm_machine_ready(Notifier *n, void *opaque)
> > > pci_conf[0x67] = (memory_region_present(io_as, 0x3f8) ? 0x08 : 0) |
> > > (memory_region_present(io_as, 0x2f8) ? 0x90 : 0);
> > >
> > > - if (s->acpi_pci_hotplug.use_acpi_pci_hotplug) {
> > > - pci_for_each_bus(d->bus, piix4_update_bus_hotplug, s);
> > > - }
> > > + pci_for_each_bus(d->bus, piix4_update_bus_hotplug, s);
> > > }
> > >
> > > static void piix4_pm_add_propeties(PIIX4PMState *s)
> >
> > same as above.
> >
> > > @@ -620,60 +566,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);
> > > @@ -683,65 +575,29 @@ 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)
> > > {
> > > + uint16_t pcihp_io_size = PIIX_PCI_HOTPLUG_SIZE;
> > > +
> > > memory_region_init_io(&s->io_gpe, OBJECT(s), &piix4_gpe_ops, s,
> > > "acpi-gpe0", GPE_LEN);
> > > memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe);
> > >
> > > - if (s->acpi_pci_hotplug.use_acpi_pci_hotplug) {
> > > - acpi_pcihp_init(&s->acpi_pci_hotplug, bus, parent);
> > > - } 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));
> > > + if (!s->acpi_pci_hotplug.use_acpi_pci_hotplug) {
> > > + unsigned *bus_bsel = g_malloc(sizeof *bus_bsel);
> > > +
> > > + pcihp_io_size = PIIX_PCI_HOTPLUG_LEGACY_SIZE;
> > > +
> > > + *bus_bsel = 0;
> > > + object_property_add_uint32_ptr(OBJECT(bus), ACPI_PCIHP_PROP_BSEL,
> > > + bus_bsel, NULL);
> > > }
> >
> > Okay, but please add a comment here.
> > Also how about
> > #define ACPI_PCIHP_BSEL_DEFAULT 0
> > and use this in pcihp.c on reset?
> >
> > > + acpi_pcihp_init(&s->acpi_pci_hotplug, bus, PIIX_PCI_HOTPLUG_ADDR,
> > > + pcihp_io_size, parent);
> > >
> > > 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 cff5270..afe1e8c 100644
> > > --- a/include/hw/acpi/pcihp.h
> > > +++ b/include/hw/acpi/pcihp.h
> > > @@ -31,6 +31,10 @@
> > > #include <qemu/typedefs.h>
> > > #include "hw/pci/pci.h" /* for PCIHotplugState */
> > >
> > > +#define PIIX_PCI_HOTPLUG_ADDR 0xae00
> > > +#define PIIX_PCI_HOTPLUG_SIZE 0x0014
> >
> > Pls change prefix to ACPI_PCIHP_
> >
> > Also - required in header?
> not really, I'll put them into piix4.c and keep
> PIIX_PCI_ prefix to reflect that it's piix4 only defines.
>
> >
> > > +#define PIIX_PCI_HOTPLUG_LEGACY_SIZE 0x000f
> > > +
> > > typedef struct AcpiPciHpPciStatus {
> > > uint32_t up;
> > > uint32_t down;
> > > @@ -49,8 +53,8 @@ typedef struct AcpiPciHpState {
> > > bool use_acpi_pci_hotplug;
> > > } AcpiPciHpState;
> > >
> > > -void acpi_pcihp_init(AcpiPciHpState *, PCIBus *root,
> > > - MemoryRegion *address_space_io);
> > > +void acpi_pcihp_init(AcpiPciHpState *, PCIBus *root, uint16_t io_base,
> > > + uint16_t io_size, MemoryRegion *address_space_io);
> > >
> > > /* Invoke on device hotplug */
> > > int acpi_pcihp_device_hotplug(AcpiPciHpState *, PCIDevice *,
> > > --
> > > 1.7.1
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2014-01-30 13:08 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-01-27 15:39 [Qemu-devel] [PATCH v2 0/4] pc: make ACPI pcihp more reusable Igor Mammedov
2014-01-27 15:39 ` [Qemu-devel] [PATCH v2 1/4] hw:piix4:acpi: replace enable|disable_device() with oneliners Igor Mammedov
2014-01-27 15:39 ` [Qemu-devel] [PATCH v2 2/4] hw:piix4:acpi: make PCI hotplug mmio handlers indifferent to PCI_HOTPLUG_ADDR Igor Mammedov
2014-01-27 15:39 ` [Qemu-devel] [PATCH v2 3/4] pcihp: make pci_read() mmio calback compatible with legacy ACPI hotplug Igor Mammedov
2014-01-30 11:28 ` Michael S. Tsirkin
2014-01-27 15:39 ` [Qemu-devel] [PATCH v2 4/4] hw:piix4:acpi: reuse pcihp code for legacy PCI hotplug Igor Mammedov
2014-01-30 11:25 ` Michael S. Tsirkin
2014-01-30 12:20 ` Igor Mammedov
2014-01-30 13:08 ` Michael S. Tsirkin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).