Re: [Qemu-devel] [RFC v5 7/7] hw/arm/virt-acpi-build: Add ITS description in ACPI MADT table

[Auger Eric <eric.auger@redhat.com>]

Hi Shannon,

On 03/08/2016 10:50, Shannon Zhao wrote:
> 
> 
> On 2016/8/3 15:22, Auger Eric wrote:
>> Hi Shannon,
>>
>> On 03/08/2016 02:56, Shannon Zhao wrote:
>>>> Hi Eric,
>>>>
>>>> On 2016/8/3 2:07, Eric Auger wrote:
>>>>>> This patch exposes the GICv3 ITS to the ACPI guest. The ITS structure
>>>>>> is added to the MADT table.
>>>>>>
>>>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>>>>>
>>>>>> ---
>>>>>>
>>>>>> v5: new
>>>>>>
>>>>>> Tested with Tomasz' kernel series on guest side:
>>>>>> - [PATCH V7 0/8] Introduce ACPI world to ITS,
>>>>>>   https://lkml.org/lkml/2016/6/20/321
>>>>>> - for running PCIe on the guest (virtio-pci-net or vhost-net)
>>>>>>   the following series is also needed, although not directly ITS:
>>>>>>   Support for ARM64 ACPI based PCI host controller,
>>>>>>   https://lwn.net/Articles/690995/
>>>>>> ---
>>>>>>  hw/arm/virt-acpi-build.c    |  7 +++++++
>>>>>>  include/hw/acpi/acpi-defs.h | 13 ++++++++++++-
>>>>>>  2 files changed, 19 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>>>>>> index 28fc59c..6cfedff 100644
>>>>>> --- a/hw/arm/virt-acpi-build.c
>>>>>> +++ b/hw/arm/virt-acpi-build.c
>>>>>> @@ -546,6 +546,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtGuestInfo *guest_info)
>>>>>>      }
>>>>>>  
>>>>>>      if (guest_info->gic_version == 3) {
>>>>>> +        AcpiMadtGicIts *gic_its;
>>>>>>          AcpiMadtGenericRedistributor *gicr = acpi_data_push(table_data,
>>>>>>                                                           sizeof *gicr);
>>>>>>  
>>>>>> @@ -553,6 +554,12 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtGuestInfo *guest_info)
>>>>>>          gicr->length = sizeof(*gicr);
>>>>>>          gicr->base_address = cpu_to_le64(memmap[VIRT_GIC_REDIST].base);
>>>>>>          gicr->range_length = cpu_to_le32(memmap[VIRT_GIC_REDIST].size);
>>>>>> +
>>>>>> +        gic_its = acpi_data_push(table_data, sizeof *gic_its);
>>>>>> +        gic_its->type = ACPI_APIC_ITS_STRUCTURE;
>>>>>> +        gic_its->length = sizeof(*gic_its);
>>>>>> +        gic_its->gic_its_id = 0;
>>>>>> +        gic_its->base_address = cpu_to_le64(memmap[VIRT_GIC_ITS].base);
>>>> Since for TCG it doesn't support ITS yet, it should check here using
>>>> its_class_name().
>>>>
>>>>>>      } else {
>>>>>>          gic_msi = acpi_data_push(table_data, sizeof *gic_msi);
>>>>>>          gic_msi->type = ACPI_APIC_GENERIC_MSI_FRAME;
>>>>>> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
>>>>>> index 41c1d95..ba3be1e 100644
>>>>>> --- a/include/hw/acpi/acpi-defs.h
>>>>>> +++ b/include/hw/acpi/acpi-defs.h
>>>>>> @@ -294,7 +294,8 @@ typedef struct AcpiMultipleApicTable AcpiMultipleApicTable;
>>>>>>  #define ACPI_APIC_GENERIC_DISTRIBUTOR   12
>>>>>>  #define ACPI_APIC_GENERIC_MSI_FRAME     13
>>>>>>  #define ACPI_APIC_GENERIC_REDISTRIBUTOR 14
>>>>>> -#define ACPI_APIC_RESERVED              15   /* 15 and greater are reserved */
>>>>>> +#define ACPI_APIC_ITS_STRUCTURE         15
>>>> use ACPI_APIC_GENERIC_TRANSLATOR instead.
>>>>
>>>>>> +#define ACPI_APIC_RESERVED              16   /* 16 and greater are reserved */
>>>>>>  
>>>>>>  /*
>>>>>>   * MADT sub-structures (Follow MULTIPLE_APIC_DESCRIPTION_TABLE)
>>>>>> @@ -386,6 +387,16 @@ struct AcpiMadtGenericMsiFrame {
>>>>>>  
>>>>>>  typedef struct AcpiMadtGenericMsiFrame AcpiMadtGenericMsiFrame;
>>>>>>  
>>>>>> +struct AcpiMadtGicIts {
>>>>>> +    ACPI_SUB_HEADER_DEF
>>>>>> +    uint16_t reserved;
>>>>>> +    uint32_t gic_its_id;
>>>>>> +    uint64_t base_address;
>>>>>> +    uint32_t reserved2;
>>>>>> +} QEMU_PACKED;
>>>>>> +
>>>>>> +typedef struct AcpiMadtGicIts AcpiMadtGicIts;
>>>>>> +
>>>> Define it like below to respect the name in linux kernel and also keep
>>>> consistent with other structures.
>>>>
>>>> +struct AcpiMadtGenericTranslator {
>>>> +    ACPI_SUB_HEADER_DEF
>>>> +    uint16_t reserved;
>>>> +    uint32_t translation_id;
>>>> +    uint64_t base_address;
>>>> +    uint32_t reserved2;
>>>> +} QEMU_PACKED;
>>>> +
>>>> +typedef struct AcpiMadtGenericTranslator AcpiMadtGenericTranslator;
>>>>
>>>> BTW, you could have a look at [1] which I sent before.
>>>>
>>>> [1] https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg06282.html
>> Hum sorry, please apologize. I was not aware of those. Do you want to
>> respin or shall I respin mine taking into account your comments and
>> changing the author. Besides, thanks for the review.
> Never mind. I think you can respin yours.
OK thanks.

Eric
> 
> Thanks,
>