Re: [Qemu-devel] [PATCH v5 12/24] hw: acpi: Export the MCFG getter

[Igor Mammedov <imammedo@redhat.com>]

On Mon,  5 Nov 2018 02:40:35 +0100
Samuel Ortiz <sameo@linux.intel.com> wrote:

> From: Yang Zhong <yang.zhong@intel.com>
> 
> The ACPI MCFG getter is not x86 specific and could be called from
> anywhere within generic ACPI API, so let's export it.
So far it's x86 or more exactly q35 specific thing,
for example it won't work with arm/virt without refactoring
the later.

We probably over-engineered it and could get by without
properties here at all, but that's not related to this series.

So for now I'd suggest to move it to x86/acpi-pci.c for now
and we can generalize the way we get address/size later,
but if you feel like replacing all this property complications
and making it simpler go ahead.


 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Signed-off-by: Yang Zhong <yang.zhong@intel.com>
> ---
>  include/hw/acpi/aml-build.h |  1 +
>  hw/acpi/aml-build.c         | 24 ++++++++++++++++++++++++
>  hw/i386/acpi-build.c        | 22 ----------------------
>  3 files changed, 25 insertions(+), 22 deletions(-)
> 
> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> index 1861e37ebf..64ea371656 100644
> --- a/include/hw/acpi/aml-build.h
> +++ b/include/hw/acpi/aml-build.h
> @@ -408,6 +408,7 @@ void *acpi_data_push(GArray *table_data, unsigned size);
>  unsigned acpi_data_len(GArray *table);
>  Object *acpi_get_pci_host(void);
>  void acpi_get_pci_holes(Range *hole, Range *hole64);
> +bool acpi_get_mcfg(AcpiMcfgInfo *mcfg);
>  /* Align AML blob size to a multiple of 'align' */
>  void acpi_align_size(GArray *blob, unsigned align);
>  void acpi_add_table(GArray *table_offsets, GArray *table_data);
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index 869ed70db3..2c5446ab23 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -32,6 +32,8 @@
>  #include "hw/i386/pc.h"
>  #include "sysemu/tpm.h"
>  #include "hw/acpi/tpm.h"
> +#include "qom/qom-qobject.h"
> +#include "qapi/qmp/qnum.h"
>  
>  #define PCI_HOST_BRIDGE_CONFIG_ADDR        0xcf8
>  #define PCI_HOST_BRIDGE_IO_0_MIN_ADDR      0x0000
> @@ -1657,6 +1659,28 @@ void acpi_get_pci_holes(Range *hole, Range *hole64)
>                                                 NULL));
>  }
>  
> +bool acpi_get_mcfg(AcpiMcfgInfo *mcfg)
> +{
> +    Object *pci_host;
> +    QObject *o;
> +
> +    pci_host = acpi_get_pci_host();

it would be better to get bus from struct AcpiPciBus
instead of doing lookup again. (that's a separate patch from moving function around)

> +    g_assert(pci_host);
> +
> +    o = object_property_get_qobject(pci_host, PCIE_HOST_MCFG_BASE, NULL);
> +    if (!o) {
> +        return false;
> +    }
> +    mcfg->mcfg_base = qnum_get_uint(qobject_to(QNum, o));
> +    qobject_unref(o);
> +
> +    o = object_property_get_qobject(pci_host, PCIE_HOST_MCFG_SIZE, NULL);
> +    assert(o);
> +    mcfg->mcfg_size = qnum_get_uint(qobject_to(QNum, o));
> +    qobject_unref(o);
> +    return true;
> +}
> +
>  static void crs_range_insert(GPtrArray *ranges, uint64_t base, uint64_t limit)
>  {
>      CrsRangeEntry *entry;
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 14e2624d14..d8bba16776 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1856,28 +1856,6 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker)
>                   "IVRS", table_data->len - iommu_start, 1, NULL, NULL);
>  }
>  
> -static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg)
> -{
> -    Object *pci_host;
> -    QObject *o;
> -
> -    pci_host = acpi_get_pci_host();
> -    g_assert(pci_host);
> -
> -    o = object_property_get_qobject(pci_host, PCIE_HOST_MCFG_BASE, NULL);
> -    if (!o) {
> -        return false;
> -    }
> -    mcfg->mcfg_base = qnum_get_uint(qobject_to(QNum, o));
> -    qobject_unref(o);
> -
> -    o = object_property_get_qobject(pci_host, PCIE_HOST_MCFG_SIZE, NULL);
> -    assert(o);
> -    mcfg->mcfg_size = qnum_get_uint(qobject_to(QNum, o));
> -    qobject_unref(o);
> -    return true;
> -}
> -
>  static
>  void acpi_build(AcpiBuildTables *tables,
>                  MachineState *machine, AcpiConfiguration *acpi_conf)