Re: [Qemu-devel] [PATCH v3 09/19] hw: acpi: Export and generalize the PCI host AML API

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

Hi Samuel,

On 30/10/18 15:57, Samuel Ortiz wrote:
> Hi Philippe,
> 
> On Mon, Oct 29, 2018 at 08:29:38PM +0100, Philippe Mathieu-Daudé wrote:
>> On 29/10/18 20:23, Philippe Mathieu-Daudé wrote:
>>> On 29/10/18 18:01, Samuel Ortiz wrote:
>>>> From: Yang Zhong <yang.zhong@intel.com>
>>>>
>>>> The AML build routines for the PCI host bridge and the corresponding
>>>> DSDT addition are neither x86 nor PC machine type specific.
>>>> We can move them to the architecture agnostic hw/acpi folder, and by
>>>> carrying all the needed information through a new AcpiPciBus structure,
>>>> we can make them PC machine type independent.
>>>>
>>>> Signed-off-by: Yang Zhong <yang.zhong@intel.com>
>>>> Signed-off-by: Rob Bradford <robert.bradford@intel.com>
>>>> ---
>>>>    hw/acpi/aml-build.c         | 208 ++++++++++++++++++++++++++++++++++++
>>>>    hw/i386/acpi-build.c        | 167 ++---------------------------
>>>>    include/hw/acpi/aml-build.h |  10 ++
>>>>    3 files changed, 226 insertions(+), 159 deletions(-)
>>>>
>>>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>>>> index 52ac39acdb..aa72b5459c 100644
>>>> --- a/hw/acpi/aml-build.c
>>>> +++ b/hw/acpi/aml-build.c
>>>> @@ -29,6 +29,25 @@
>>>>    #include "hw/pci/pci_bus.h"
>>>>    #include "qemu/range.h"
>>>>    #include "hw/pci/pci_bridge.h"
>>>> +#include "hw/i386/pc.h"
>>
>> Err I just missed that, this include doesn't belong here, ...
>>
>>>> +#include "sysemu/tpm.h"
>>>> +#include "hw/acpi/tpm.h"
>>>> +
>>>> +#define PCI_HOST_BRIDGE_CONFIG_ADDR        0xcf8
>>>> +#define PCI_HOST_BRIDGE_IO_0_MIN_ADDR      0x0000
>>>> +#define PCI_HOST_BRIDGE_IO_0_MAX_ADDR      0x0cf7
>>>> +#define PCI_HOST_BRIDGE_IO_1_MIN_ADDR      0x0d00
>>>> +#define PCI_HOST_BRIDGE_IO_1_MAX_ADDR      0xffff
>>>> +#define PCI_VGA_MEM_BASE_ADDR              0x000a0000
>>>> +#define PCI_VGA_MEM_MAX_ADDR               0x000bffff
>>>> +#define IO_0_LEN                           0xcf8
>>>> +#define VGA_MEM_LEN                        0x20000
>>>> +
>>>> +static const char *pci_hosts[] = {
>>>
>>> This array should stay in hw/i386/acpi-build.c.
>>>
>>>> +   "/machine/i440fx",
>>>> +   "/machine/q35",
>>>> +   NULL,
>>>> +};
>>>>    static GArray *build_alloc_array(void)
>>>>    {
>>>> @@ -1601,6 +1620,51 @@ void
>>>> acpi_build_tables_cleanup(AcpiBuildTables *tables, bool mfre)
>>>>        g_array_free(tables->vmgenid, mfre);
>>>>    }
>>>> +/*
>>>> + * Because of the PXB hosts we cannot simply query TYPE_PCI_HOST_BRIDGE.
>>>> + */
>>>> +Object *acpi_get_pci_host(void)
>>>
>>> This function should take the machine_paths as argument.
>>>
>>>> +{
>>>> +    PCIHostState *host;
>>>> +    int i = 0;
>>>> +
>>>> +    while (pci_hosts[i]) {
>>>> +        host = OBJECT_CHECK(PCIHostState,
>>>> +                            object_resolve_path(pci_hosts[i], NULL),
>>>> +                            TYPE_PCI_HOST_BRIDGE);
>>>> +        if (host) {
>>>> +            return OBJECT(host);
>>>> +        }
>>>> +
>>>> +        i++;
>>>> +    }
>>>> +
>>>> +    return NULL;
>>>> +}
>>>> +
>>>> +void acpi_get_pci_holes(Range *hole, Range *hole64)
>>
>> ... and this function neither, it should stay in hw/i386/acpi-build.c, thus
>> you don't need to modify the prototype and can call
>> acpi_get_pci_host(x86_machine_paths) directly.
>>
> So the idea for those routines is that they're not x86 specific. As a
> matter of fact, they will eventually get called from architecture
> agnostic code like e.g. hw/acpi/reduced.c. So I don't think they should
> live under hw/i386/

But PCI_HOST_PROP_PCI_HOLE_START is only defined in "hw/i386/pc.h"... 
This file is no more arch agnostic.

I can understand/accept a acpi_get_pci_holes() function, but the ranges 
shouldn't be i386 based.

> 
> Moreover adding an argument to acpi_get_pci_host() means the caller should
> know which potential machines it needs to go through. And once both
> arm/virt and i386/virt calls into hw/acpi/reduced.c, we need to somehow
> pass a relevant set of machines paths to this API.
> So I think a more robust way to do so would be for each machine instance to
> be able to set its own PCI host pointer instead of having to maintain
> a per architecture set of potential PCI machine paths. I'm thinking
> about adding that to the AcpiConfiguration structure and let each
> machine fills it if/when it builds its PCI host.
> 
> Cheers,
> Samuel.
>