Re: [Qemu-devel] [PATCH v3 3/5] pcihp: make pci_read() mmio calback compatible with legacy ACPI hotplug

[Michael S. Tsirkin]

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 <address@hidden>
> ---
>  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