On Fri, Jan 31, 2014 at 02:27:08PM +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.
>
> PS:
> And since field 'use_acpi_pci_hotplug' is touched
> anyway rename it to reflect its meaning more crearly
> to 'legacy_piix' and corresponding property to
> 'legacy-acpi-pci-hotplug'.
>
> Signed-off-by: Igor Mammedov <imamm...@redhat.com>
> ---
> hw/acpi/pcihp.c | 4 +++-
> hw/acpi/piix4.c | 15 +++++++--------
> include/hw/acpi/pcihp.h | 1 +
> include/hw/i386/pc.h | 4 ++--
> 4 files changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index 64c8cf2..a7353f6 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:
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index 5d55a3c..33c9516 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.legacy_piix;
> }
>
> 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.legacy_piix;
> }
>
> /* 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.legacy_piix) {
> 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.legacy_piix) {
> pci_for_each_bus(d->bus, piix4_update_bus_hotplug, s);
> }
> }
> @@ -550,8 +549,8 @@ static Property piix4_pm_properties[] = {
> DEFINE_PROP_UINT8(ACPI_PM_PROP_S3_DISABLED, PIIX4PMState, disable_s3, 0),
> 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),
> + DEFINE_PROP_BOOL("legacy-acpi-pci-hotplug", PIIX4PMState,
> + acpi_pci_hotplug.legacy_piix, false),
That's a worse less descriptive name for user property.
I also think it's not nice that piix pokes at
acpi_pci_hotplug structure internals
without an API, here and below.
Let's pass a bool flag to the init function, have
acpi maintain it internally.
> 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.legacy_piix) {
> 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..6877892 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 legacy_piix;
> } AcpiPciHpState;
>
> void acpi_pcihp_init(AcpiPciHpState *, PCIBus *root,
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 3e1e81b..68d75d8 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -268,8 +268,8 @@ int e820_add_entry(uint64_t, uint64_t, uint32_t);
> },\
> {\
> .driver = "PIIX4_PM",\
> - .property = "acpi-pci-hotplug-with-bridge-support",\
> - .value = "off",\
> + .property = "legacy-acpi-pci-hotplug",\
> + .value = "on",\
?
> }
>
> #define PC_COMPAT_1_6 \
> --
> 1.7.1
