[v5,12/24] hw: acpi: Export the MCFG getter

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.

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(-)

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)

Hi Igor,

On Thu, Nov 15, 2018 at 01:36:58PM +0100, Igor Mammedov wrote:
> 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,
It's property based, and it's using a generic PCIE property afaict.
So it's up to each machine type to define those properties.
I'm curious here: What's the idiomatic way to define a machine
setting/attribute/property, let each instance define it or not, and
make it available at run time?
Would you be getting the PCI host pointer from the ACPI build state and
getting that information back from there?

Cheers,
Samuel.

On Thu, 22 Nov 2018 00:21:06 +0100
Samuel Ortiz <sameo@linux.intel.com> wrote:

> Hi Igor,
> 
> On Thu, Nov 15, 2018 at 01:36:58PM +0100, Igor Mammedov wrote:
> > 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,  
> It's property based, and it's using a generic PCIE property afaict.
> So it's up to each machine type to define those properties.
> I'm curious here: What's the idiomatic way to define a machine
> setting/attribute/property, let each instance define it or not, and
> make it available at run time?
> Would you be getting the PCI host pointer from the ACPI build state and
> getting that information back from there?

Cleaner way would be make arm/virt board set PCIE_HOST_MCFG_BASE/
PCIE_HOST_MCFG_SIZE properties and then use common build_mcfg()(in aml-build.c).
Something like this:
  acpi_setup_reduced()
     AcpiMcfgInfo mcfg_info = {
       .base = object_property_get_uint(pcie, PCIE_HOST_MCFG_BASE, NULL),
       .size = object_property_get_uint(pcie, PCIE_HOST_MCFG_SIZE, NULL)
     };
     acpi_build() {
         build_mcfg("MCFG", &info);
     }
  }
and for legacy q35
  acpi_build() {
     if (pcie) {
        AcpiMcfgInfo mcfg_info = {
          .base = object_property_get_uint(pcie, PCIE_HOST_MCFG_BASE, NULL),
          .size = object_property_get_uint(pcie, PCIE_HOST_MCFG_SIZE, NULL)
        };
        if (mcfg_info.base != PCIE_BASE_ADDR_UNMAPPED)
            build_mcfg("MCFG", &info);
        else
            /* move comment here why we are doing it */
            build_mcfg("QEMU", &info);
     }
  }

The thing I don't like about acpi_get_mcfg() is that it
does lookup acpi_get_i386_pci_host() each time it's called
and judges if it's PCI-E host by presence of properties.

I'd rather be explicit where PCI host be fetched once somewhere
in acpi_setup() or possibly passed down from the board as an argument
and board telling to i386/acpi_setup() if it's PCI or PCI-E host
so we don't have to guess it in acpi code.


> Cheers,
> Samuel.