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

[Shannon Zhao <zhaoshenglong@huawei.com>]

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.

Thanks,
-- 
Shannon