Re: [Qemu-devel] [PATCH v4 6/6] acpi: pci: use build_append_foo() API to construct MCFG

[Wei Yang <richardw.yang@linux.intel.com>]

On Wed, May 15, 2019 at 07:29:17AM +0200, Philippe Mathieu-Daudé wrote:
>On 5/15/19 3:10 AM, Michael S. Tsirkin wrote:
>> On Fri, Apr 19, 2019 at 08:30:53AM +0800, Wei Yang wrote:
>>> build_append_foo() API doesn't need explicit endianness conversions
>>> which eliminates a source of errors and it makes build_mcfg() look like
>>> declarative definition of MCFG table in ACPI spec, which makes it easy
>>> to review.
>>>
>>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>>> Suggested-by: Igor Mammedov <imammedo@redhat.com>
>>> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> 
>> Causes a regression with an invalid MCFG produced.
>> Dropped.
>> 
>>> ---
>>>  hw/acpi/pci.c               | 33 +++++++++++++++++++++------------
>>>  include/hw/acpi/acpi-defs.h | 18 ------------------
>>>  2 files changed, 21 insertions(+), 30 deletions(-)
>>>
>>> diff --git a/hw/acpi/pci.c b/hw/acpi/pci.c
>>> index fa0fa30bb9..341805e786 100644
>>> --- a/hw/acpi/pci.c
>>> +++ b/hw/acpi/pci.c
>>> @@ -30,17 +30,26 @@
>>>  
>>>  void build_mcfg(GArray *table_data, BIOSLinker *linker, AcpiMcfgInfo *info)
>>>  {
>>> -    AcpiTableMcfg *mcfg;
>>> -    int len = sizeof(*mcfg) + sizeof(mcfg->allocation[0]);
>>> -
>>> -    mcfg = acpi_data_push(table_data, len);
>>> -    mcfg->allocation[0].address = cpu_to_le64(info->base);
>>> -
>>> -    /* Only a single allocation so no need to play with segments */
>>> -    mcfg->allocation[0].pci_segment = cpu_to_le16(0);
>>> -    mcfg->allocation[0].start_bus_number = 0;
>>> -    mcfg->allocation[0].end_bus_number = PCIE_MMCFG_BUS(info->size - 1);
>>> -
>>> -    build_header(linker, table_data, (void *)mcfg, "MCFG", len, 1, NULL, NULL);
>>> +    int mcfg_start = table_data->len;
>>> +
>>> +    acpi_data_push(table_data, sizeof(AcpiTableHeader));
>
>Ah, it seems we missed AcpiTableMcfg.reserved[8]...:
>
>        build_append_int_noprefix(table_data, 0, 8);
>

Hmm... you are right. Shame on me.

>>> +
>>> +    /*
>>> +     * PCI Firmware Specification, Revision 3.0
>>> +     * 4.1.2 MCFG Table Description.
>>> +     */
>>> +    /* Base address, processor-relative */
>>> +    build_append_int_noprefix(table_data, info->base, 8);
>>> +    /* PCI segment group number */
>>> +    build_append_int_noprefix(table_data, 0, 2);
>>> +    /* Starting PCI Bus number */
>>> +    build_append_int_noprefix(table_data, 0, 1);
>>> +    /* Final PCI Bus number */
>>> +    build_append_int_noprefix(table_data, PCIE_MMCFG_BUS(info->size - 1), 1);
>>> +    /* Reserved */
>>> +    build_append_int_noprefix(table_data, 0, 4);
>>> +
>>> +    build_header(linker, table_data, (void *)(table_data->data + mcfg_start),
>>> +                 "MCFG", table_data->len - mcfg_start, 1, NULL, NULL);
>>>  }
>>>  
>>> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
>>> index f9aa4bd398..57a3f58b0c 100644
>>> --- a/include/hw/acpi/acpi-defs.h
>>> +++ b/include/hw/acpi/acpi-defs.h
>>> @@ -449,24 +449,6 @@ struct AcpiSratProcessorGiccAffinity {
>>>  
>>>  typedef struct AcpiSratProcessorGiccAffinity AcpiSratProcessorGiccAffinity;
>>>  
>>> -/* PCI fw r3.0 MCFG table. */
>>> -/* Subtable */
>>> -struct AcpiMcfgAllocation {
>>> -    uint64_t address;                /* Base address, processor-relative */
>>> -    uint16_t pci_segment;            /* PCI segment group number */
>>> -    uint8_t start_bus_number;       /* Starting PCI Bus number */
>>> -    uint8_t end_bus_number;         /* Final PCI Bus number */
>>> -    uint32_t reserved;
>>> -} QEMU_PACKED;
>>> -typedef struct AcpiMcfgAllocation AcpiMcfgAllocation;
>>> -
>>> -struct AcpiTableMcfg {
>>> -    ACPI_TABLE_HEADER_DEF;
>>> -    uint8_t reserved[8];
>
>           --------^
>
>Thanks Michael for testing...
>
>Wei, can you add a MCFG test in tests/bios-tables-test.c?
>

Sure, let me prepare the test.

>Regards,
>
>Phil.
>
>>> -    AcpiMcfgAllocation allocation[0];
>>> -} QEMU_PACKED;
>>> -typedef struct AcpiTableMcfg AcpiTableMcfg;
>>> -
>>>  /*
>>>   * TCPA Description Table
>>>   *
>>> -- 
>>> 2.19.1

-- 
Wei Yang
Help you, Help me