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

[Michael S. Tsirkin]

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

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