[Qemu-devel] [PATCH for-3.1] hw/arm/virt-acpi-build: Fix SMMUv3 ACPI integration

[Qemu-devel] [PATCH for-3.1] hw/arm/virt-acpi-build: Fix SMMUv3 ACPI integration

[Eric Auger]

The AcpiIortSmmu3 misses 2 32b fields corresponding to the
proximity domain and the device id mapping index.

Also let's report IO-coherent access is supported for
translation table walks, descriptor fetches and queues by
setting the COHACC override flag. Without that, we observe
wrong command opcodes. The DT description also advertises
the dma coherency.

Fixes a703b4f6c1ee ("hw/arm/virt-acpi-build: Add smmuv3 node in IORT table")

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Reported-by: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
---
 hw/arm/virt-acpi-build.c    | 1 +
 include/hw/acpi/acpi-defs.h | 8 ++++++++
 2 files changed, 9 insertions(+)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 5785fb697c..aa177ba64d 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -448,6 +448,7 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
         smmu->mapping_count = cpu_to_le32(1);
         smmu->mapping_offset = cpu_to_le32(sizeof(*smmu));
         smmu->base_address = cpu_to_le64(vms->memmap[VIRT_SMMU].base);
+        smmu->flags = ACPI_IORT_SMMU_V3_COHACC_OVERRIDE;
         smmu->event_gsiv = cpu_to_le32(irq);
         smmu->pri_gsiv = cpu_to_le32(irq + 1);
         smmu->gerr_gsiv = cpu_to_le32(irq + 2);
diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
index af8e023968..c3ee1f517b 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -628,6 +628,12 @@ struct AcpiIortItsGroup {
 } QEMU_PACKED;
 typedef struct AcpiIortItsGroup AcpiIortItsGroup;
 
+enum {
+    ACPI_IORT_SMMU_V3_COHACC_OVERRIDE = 1 << 0,
+    ACPI_IORT_SMMU_V3_HTTU_OVERRIDE   = 3 << 1,
+    ACPI_IORT_SMMU_V3_PXM_VALID       = 1 << 3
+};
+
 struct AcpiIortSmmu3 {
     ACPI_IORT_NODE_HEADER_DEF
     uint64_t base_address;
@@ -639,6 +645,8 @@ struct AcpiIortSmmu3 {
     uint32_t pri_gsiv;
     uint32_t gerr_gsiv;
     uint32_t sync_gsiv;
+    uint32_t pxm;
+    uint32_t id_mapping_index;
     AcpiIortIdMapping id_mapping_array[0];
 } QEMU_PACKED;
 typedef struct AcpiIortSmmu3 AcpiIortSmmu3;
-- 
2.17.2

Re: [Qemu-devel] [PATCH for-3.1] hw/arm/virt-acpi-build: Fix SMMUv3 ACPI integration

[Shameerali Kolothum Thodi]

Hi Eric,

> -----Original Message-----
> From: Eric Auger [mailto:eric.auger@redhat.com]
> Sent: 26 November 2018 15:46
> To: eric.auger.pro@gmail.com; eric.auger@redhat.com; qemu-
> devel@nongnu.org; qemu-arm@nongnu.org; peter.maydell@linaro.org;
> shannon.zhaosl@gmail.com
> Cc: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Subject: [PATCH for-3.1] hw/arm/virt-acpi-build: Fix SMMUv3 ACPI integration
> 
> The AcpiIortSmmu3 misses 2 32b fields corresponding to the
> proximity domain and the device id mapping index.
> 
> Also let's report IO-coherent access is supported for
> translation table walks, descriptor fetches and queues by
> setting the COHACC override flag. Without that, we observe
> wrong command opcodes. The DT description also advertises
> the dma coherency.

Ah..that explains the "IDR0.COHACC overridden" and "CMD_SYNC timeout "
entries in the boot log.  Thanks for the fix and I can confirm this fixes the issue
reported earlier[1]. 

FWIW:
Tested-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

Thanks,
Shameer

[1] https://patchwork.kernel.org/cover/10609261/

> Fixes a703b4f6c1ee ("hw/arm/virt-acpi-build: Add smmuv3 node in IORT
> table")
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Reported-by: Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>
> ---
>  hw/arm/virt-acpi-build.c    | 1 +
>  include/hw/acpi/acpi-defs.h | 8 ++++++++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 5785fb697c..aa177ba64d 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -448,6 +448,7 @@ build_iort(GArray *table_data, BIOSLinker *linker,
> VirtMachineState *vms)
>          smmu->mapping_count = cpu_to_le32(1);
>          smmu->mapping_offset = cpu_to_le32(sizeof(*smmu));
>          smmu->base_address = cpu_to_le64(vms->memmap[VIRT_SMMU].base);
> +        smmu->flags = ACPI_IORT_SMMU_V3_COHACC_OVERRIDE;
>          smmu->event_gsiv = cpu_to_le32(irq);
>          smmu->pri_gsiv = cpu_to_le32(irq + 1);
>          smmu->gerr_gsiv = cpu_to_le32(irq + 2);
> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> index af8e023968..c3ee1f517b 100644
> --- a/include/hw/acpi/acpi-defs.h
> +++ b/include/hw/acpi/acpi-defs.h
> @@ -628,6 +628,12 @@ struct AcpiIortItsGroup {
>  } QEMU_PACKED;
>  typedef struct AcpiIortItsGroup AcpiIortItsGroup;
> 
> +enum {
> +    ACPI_IORT_SMMU_V3_COHACC_OVERRIDE = 1 << 0,
> +    ACPI_IORT_SMMU_V3_HTTU_OVERRIDE   = 3 << 1,
> +    ACPI_IORT_SMMU_V3_PXM_VALID       = 1 << 3
> +};
> +
>  struct AcpiIortSmmu3 {
>      ACPI_IORT_NODE_HEADER_DEF
>      uint64_t base_address;
> @@ -639,6 +645,8 @@ struct AcpiIortSmmu3 {
>      uint32_t pri_gsiv;
>      uint32_t gerr_gsiv;
>      uint32_t sync_gsiv;
> +    uint32_t pxm;
> +    uint32_t id_mapping_index;
>      AcpiIortIdMapping id_mapping_array[0];
>  } QEMU_PACKED;
>  typedef struct AcpiIortSmmu3 AcpiIortSmmu3;
> --
> 2.17.2

Re: [Qemu-devel] [PATCH for-3.1] hw/arm/virt-acpi-build: Fix SMMUv3 ACPI integration

[Auger Eric]

Hi Shameer,
On 11/26/18 6:04 PM, Shameerali Kolothum Thodi wrote:
> Hi Eric,
> 
>> -----Original Message-----
>> From: Eric Auger [mailto:eric.auger@redhat.com]
>> Sent: 26 November 2018 15:46
>> To: eric.auger.pro@gmail.com; eric.auger@redhat.com; qemu-
>> devel@nongnu.org; qemu-arm@nongnu.org; peter.maydell@linaro.org;
>> shannon.zhaosl@gmail.com
>> Cc: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
>> Subject: [PATCH for-3.1] hw/arm/virt-acpi-build: Fix SMMUv3 ACPI integration
>>
>> The AcpiIortSmmu3 misses 2 32b fields corresponding to the
>> proximity domain and the device id mapping index.
>>
>> Also let's report IO-coherent access is supported for
>> translation table walks, descriptor fetches and queues by
>> setting the COHACC override flag. Without that, we observe
>> wrong command opcodes. The DT description also advertises
>> the dma coherency.
> 
> Ah..that explains the "IDR0.COHACC overridden" and "CMD_SYNC timeout "
> entries in the boot log.  Thanks for the fix and I can confirm this fixes the issue
> reported earlier[1]. 
> 
> FWIW:
> Tested-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
Many thanks!

Best Regards

Eric
> 
> Thanks,
> Shameer
> 
> [1] https://patchwork.kernel.org/cover/10609261/
> 
>> Fixes a703b4f6c1ee ("hw/arm/virt-acpi-build: Add smmuv3 node in IORT
>> table")
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> Reported-by: Shameerali Kolothum Thodi
>> <shameerali.kolothum.thodi@huawei.com>
>> ---
>>  hw/arm/virt-acpi-build.c    | 1 +
>>  include/hw/acpi/acpi-defs.h | 8 ++++++++
>>  2 files changed, 9 insertions(+)
>>
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index 5785fb697c..aa177ba64d 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -448,6 +448,7 @@ build_iort(GArray *table_data, BIOSLinker *linker,
>> VirtMachineState *vms)
>>          smmu->mapping_count = cpu_to_le32(1);
>>          smmu->mapping_offset = cpu_to_le32(sizeof(*smmu));
>>          smmu->base_address = cpu_to_le64(vms->memmap[VIRT_SMMU].base);
>> +        smmu->flags = ACPI_IORT_SMMU_V3_COHACC_OVERRIDE;
>>          smmu->event_gsiv = cpu_to_le32(irq);
>>          smmu->pri_gsiv = cpu_to_le32(irq + 1);
>>          smmu->gerr_gsiv = cpu_to_le32(irq + 2);
>> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
>> index af8e023968..c3ee1f517b 100644
>> --- a/include/hw/acpi/acpi-defs.h
>> +++ b/include/hw/acpi/acpi-defs.h
>> @@ -628,6 +628,12 @@ struct AcpiIortItsGroup {
>>  } QEMU_PACKED;
>>  typedef struct AcpiIortItsGroup AcpiIortItsGroup;
>>
>> +enum {
>> +    ACPI_IORT_SMMU_V3_COHACC_OVERRIDE = 1 << 0,
>> +    ACPI_IORT_SMMU_V3_HTTU_OVERRIDE   = 3 << 1,
>> +    ACPI_IORT_SMMU_V3_PXM_VALID       = 1 << 3
>> +};
>> +
>>  struct AcpiIortSmmu3 {
>>      ACPI_IORT_NODE_HEADER_DEF
>>      uint64_t base_address;
>> @@ -639,6 +645,8 @@ struct AcpiIortSmmu3 {
>>      uint32_t pri_gsiv;
>>      uint32_t gerr_gsiv;
>>      uint32_t sync_gsiv;
>> +    uint32_t pxm;
>> +    uint32_t id_mapping_index;
>>      AcpiIortIdMapping id_mapping_array[0];
>>  } QEMU_PACKED;
>>  typedef struct AcpiIortSmmu3 AcpiIortSmmu3;
>> --
>> 2.17.2
> 
> 

Re: [Qemu-devel] [PATCH for-3.1] hw/arm/virt-acpi-build: Fix SMMUv3 ACPI integration

[Auger Eric]

Hi Shannon,

On 11/26/18 4:46 PM, Eric Auger wrote:
> The AcpiIortSmmu3 misses 2 32b fields corresponding to the
> proximity domain and the device id mapping index.
I fail to understand how we currently track the evolutions of the IORT
structures:

Looking at the smmuv3 node in kernel include/acpi/actbl2.h

- the following fields were added in c944230064eb  ACPICA: iasl: Update
to IORT SMMUv3 disassembling (1 year, 4 months ago)
	u8 pxm;
	u8 reserved1;
	u16 reserved2;
- id_mapping_index was added in 4c106aa411ee  ACPICA: iasl: Add SMMUv3
device ID mapping index support (1 year ago)
- current u32 pxm was introduced in d87be0438e3d  ACPICA: IORT: Update
for revision D (6 months ago)

I would have expected each of those evolutions to be tagged by a
revision increment in the acpi_iort_node.revision field but this is not
done. Also I fail to see any enum for encoding the revision.

The smmuv3 struct that has been exposed until now corresponds to Rev C
https://lists.linuxfoundation.org/pipermail/iommu/2017-May/022000.html
(0c2021c047ba  ACPICA: IORT: Update SMMU models for revision C (1 year,
4 months ago)

Also I noticed that in rev D, new fields were added in struct
acpi_iort_root_complex. We miss them at the moment in the IORT description.

How does the guest kernel know which revision of the IORT is exposed?
What do I miss?

Thanks

Eric

> 
> Also let's report IO-coherent access is supported for
> translation table walks, descriptor fetches and queues by
> setting the COHACC override flag. Without that, we observe
> wrong command opcodes. The DT description also advertises
> the dma coherency.
> 
> Fixes a703b4f6c1ee ("hw/arm/virt-acpi-build: Add smmuv3 node in IORT table")
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Reported-by: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> ---
>  hw/arm/virt-acpi-build.c    | 1 +
>  include/hw/acpi/acpi-defs.h | 8 ++++++++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 5785fb697c..aa177ba64d 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -448,6 +448,7 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>          smmu->mapping_count = cpu_to_le32(1);
>          smmu->mapping_offset = cpu_to_le32(sizeof(*smmu));
>          smmu->base_address = cpu_to_le64(vms->memmap[VIRT_SMMU].base);
> +        smmu->flags = ACPI_IORT_SMMU_V3_COHACC_OVERRIDE;
>          smmu->event_gsiv = cpu_to_le32(irq);
>          smmu->pri_gsiv = cpu_to_le32(irq + 1);
>          smmu->gerr_gsiv = cpu_to_le32(irq + 2);
> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> index af8e023968..c3ee1f517b 100644
> --- a/include/hw/acpi/acpi-defs.h
> +++ b/include/hw/acpi/acpi-defs.h
> @@ -628,6 +628,12 @@ struct AcpiIortItsGroup {
>  } QEMU_PACKED;
>  typedef struct AcpiIortItsGroup AcpiIortItsGroup;
>  
> +enum {
> +    ACPI_IORT_SMMU_V3_COHACC_OVERRIDE = 1 << 0,
> +    ACPI_IORT_SMMU_V3_HTTU_OVERRIDE   = 3 << 1,
> +    ACPI_IORT_SMMU_V3_PXM_VALID       = 1 << 3
> +};
> +
>  struct AcpiIortSmmu3 {
>      ACPI_IORT_NODE_HEADER_DEF
>      uint64_t base_address;
> @@ -639,6 +645,8 @@ struct AcpiIortSmmu3 {
>      uint32_t pri_gsiv;
>      uint32_t gerr_gsiv;
>      uint32_t sync_gsiv;
> +    uint32_t pxm;
> +    uint32_t id_mapping_index;
>      AcpiIortIdMapping id_mapping_array[0];
>  } QEMU_PACKED;
>  typedef struct AcpiIortSmmu3 AcpiIortSmmu3;
> 

Re: [Qemu-devel] [PATCH for-3.1] hw/arm/virt-acpi-build: Fix SMMUv3 ACPI integration

[Shannon Zhao]

On 2018/11/27 13:53, Auger Eric wrote:
> Hi Shannon,
> 
> On 11/26/18 4:46 PM, Eric Auger wrote:
>> The AcpiIortSmmu3 misses 2 32b fields corresponding to the
>> proximity domain and the device id mapping index.
> I fail to understand how we currently track the evolutions of the IORT
> structures:
> 
> Looking at the smmuv3 node in kernel include/acpi/actbl2.h
> 
> - the following fields were added in c944230064eb  ACPICA: iasl: Update
> to IORT SMMUv3 disassembling (1 year, 4 months ago)
> 	u8 pxm;
> 	u8 reserved1;
> 	u16 reserved2;
> - id_mapping_index was added in 4c106aa411ee  ACPICA: iasl: Add SMMUv3
> device ID mapping index support (1 year ago)
> - current u32 pxm was introduced in d87be0438e3d  ACPICA: IORT: Update
> for revision D (6 months ago)
> 
> I would have expected each of those evolutions to be tagged by a
> revision increment in the acpi_iort_node.revision field but this is not
> done. Also I fail to see any enum for encoding the revision.
> 
> The smmuv3 struct that has been exposed until now corresponds to Rev C
> https://lists.linuxfoundation.org/pipermail/iommu/2017-May/022000.html
> (0c2021c047ba  ACPICA: IORT: Update SMMU models for revision C (1 year,
> 4 months ago)
> 
> Also I noticed that in rev D, new fields were added in struct
> acpi_iort_root_complex. We miss them at the moment in the IORT description.
> 
> How does the guest kernel know which revision of the IORT is exposed?
> What do I miss?
> 
Look at the kernel code in iort.c it checks if the flags field has set 
ACPI_IORT_SMMU_V3_PXM_VALID bit.

	if (smmu->flags & ACPI_IORT_SMMU_V3_PXM_VALID) {
		set_dev_node(dev, acpi_map_pxm_to_node(smmu->pxm));
		pr_info("SMMU-v3[%llx] Mapped to Proximity domain %d\n",
			smmu->base_address,
			smmu->pxm);
	}

But it doesn't check the revision I think the reason is that revision 1 
& 2 just use the next reserved fields. No matter u8 or u32, kernel 
driver get the same value.

The code handling id_mapping_index does check the revision.

static int iort_get_id_mapping_index(struct acpi_iort_node *node)
{
	struct acpi_iort_smmu_v3 *smmu;

	switch (node->type) {
	case ACPI_IORT_NODE_SMMU_V3:
		/*
		 * SMMUv3 dev ID mapping index was introduced in revision 1
		 * table, not available in revision 0
		 */
		if (node->revision < 1)
			return -EINVAL;

> Thanks
> 
> Eric
> 
>>
>> Also let's report IO-coherent access is supported for
>> translation table walks, descriptor fetches and queues by
>> setting the COHACC override flag. Without that, we observe
>> wrong command opcodes. The DT description also advertises
>> the dma coherency.
>>
>> Fixes a703b4f6c1ee ("hw/arm/virt-acpi-build: Add smmuv3 node in IORT table")
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> Reported-by: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
>> ---
>>   hw/arm/virt-acpi-build.c    | 1 +
>>   include/hw/acpi/acpi-defs.h | 8 ++++++++
>>   2 files changed, 9 insertions(+)
>>
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index 5785fb697c..aa177ba64d 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -448,6 +448,7 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>>           smmu->mapping_count = cpu_to_le32(1);
>>           smmu->mapping_offset = cpu_to_le32(sizeof(*smmu));
>>           smmu->base_address = cpu_to_le64(vms->memmap[VIRT_SMMU].base);
>> +        smmu->flags = ACPI_IORT_SMMU_V3_COHACC_OVERRIDE;
>>           smmu->event_gsiv = cpu_to_le32(irq);
>>           smmu->pri_gsiv = cpu_to_le32(irq + 1);
>>           smmu->gerr_gsiv = cpu_to_le32(irq + 2);
>> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
>> index af8e023968..c3ee1f517b 100644
>> --- a/include/hw/acpi/acpi-defs.h
>> +++ b/include/hw/acpi/acpi-defs.h
>> @@ -628,6 +628,12 @@ struct AcpiIortItsGroup {
>>   } QEMU_PACKED;
>>   typedef struct AcpiIortItsGroup AcpiIortItsGroup;
>>   
>> +enum {
>> +    ACPI_IORT_SMMU_V3_COHACC_OVERRIDE = 1 << 0,
>> +    ACPI_IORT_SMMU_V3_HTTU_OVERRIDE   = 3 << 1,
>> +    ACPI_IORT_SMMU_V3_PXM_VALID       = 1 << 3
>> +};
>> +
>>   struct AcpiIortSmmu3 {
>>       ACPI_IORT_NODE_HEADER_DEF
>>       uint64_t base_address;
>> @@ -639,6 +645,8 @@ struct AcpiIortSmmu3 {
>>       uint32_t pri_gsiv;
>>       uint32_t gerr_gsiv;
>>       uint32_t sync_gsiv;
>> +    uint32_t pxm;
So if we use this field ,we need to set the flags with 
ACPI_IORT_SMMU_V3_PXM_VALID.
>> +    uint32_t id_mapping_index;
And if we use this field, it needs to set the revision to at least 1.

Thanks,
Shannon
>>       AcpiIortIdMapping id_mapping_array[0];
>>   } QEMU_PACKED;
>>   typedef struct AcpiIortSmmu3 AcpiIortSmmu3;
>>

Re: [Qemu-devel] [PATCH for-3.1] hw/arm/virt-acpi-build: Fix SMMUv3 ACPI integration

[Auger Eric]

Hi Shannon,

On 11/28/18 5:39 PM, Shannon Zhao wrote:
> 
> 
> On 2018/11/27 13:53, Auger Eric wrote:
>> Hi Shannon,
>>
>> On 11/26/18 4:46 PM, Eric Auger wrote:
>>> The AcpiIortSmmu3 misses 2 32b fields corresponding to the
>>> proximity domain and the device id mapping index.
>> I fail to understand how we currently track the evolutions of the IORT
>> structures:
>>
>> Looking at the smmuv3 node in kernel include/acpi/actbl2.h
>>
>> - the following fields were added in c944230064eb  ACPICA: iasl: Update
>> to IORT SMMUv3 disassembling (1 year, 4 months ago)
>>     u8 pxm;
>>     u8 reserved1;
>>     u16 reserved2;
>> - id_mapping_index was added in 4c106aa411ee  ACPICA: iasl: Add SMMUv3
>> device ID mapping index support (1 year ago)
>> - current u32 pxm was introduced in d87be0438e3d  ACPICA: IORT: Update
>> for revision D (6 months ago)
>>
>> I would have expected each of those evolutions to be tagged by a
>> revision increment in the acpi_iort_node.revision field but this is not
>> done. Also I fail to see any enum for encoding the revision.
>>
>> The smmuv3 struct that has been exposed until now corresponds to Rev C
>> https://lists.linuxfoundation.org/pipermail/iommu/2017-May/022000.html
>> (0c2021c047ba  ACPICA: IORT: Update SMMU models for revision C (1 year,
>> 4 months ago)
>>
>> Also I noticed that in rev D, new fields were added in struct
>> acpi_iort_root_complex. We miss them at the moment in the IORT
>> description.
>>
>> How does the guest kernel know which revision of the IORT is exposed?
>> What do I miss?
>>
> Look at the kernel code in iort.c it checks if the flags field has set
> ACPI_IORT_SMMU_V3_PXM_VALID bit.
> 
>     if (smmu->flags & ACPI_IORT_SMMU_V3_PXM_VALID) {
>         set_dev_node(dev, acpi_map_pxm_to_node(smmu->pxm));
>         pr_info("SMMU-v3[%llx] Mapped to Proximity domain %d\n",
>             smmu->base_address,
>             smmu->pxm);
>     }
> 
> But it doesn't check the revision I think the reason is that revision 1
> & 2 just use the next reserved fields. No matter u8 or u32, kernel
> driver get the same value.
> 
> The code handling id_mapping_index does check the revision.
> 
> static int iort_get_id_mapping_index(struct acpi_iort_node *node)
> {
>     struct acpi_iort_smmu_v3 *smmu;
> 
>     switch (node->type) {
>     case ACPI_IORT_NODE_SMMU_V3:
>         /*
>          * SMMUv3 dev ID mapping index was introduced in revision 1
>          * table, not available in revision 0
>          */
>         if (node->revision < 1)
>             return -EINVAL;
> 
>> Thanks
>>
>> Eric
>>
>>>
>>> Also let's report IO-coherent access is supported for
>>> translation table walks, descriptor fetches and queues by
>>> setting the COHACC override flag. Without that, we observe
>>> wrong command opcodes. The DT description also advertises
>>> the dma coherency.
>>>
>>> Fixes a703b4f6c1ee ("hw/arm/virt-acpi-build: Add smmuv3 node in IORT
>>> table")
>>>
>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>> Reported-by: Shameerali Kolothum Thodi
>>> <shameerali.kolothum.thodi@huawei.com>
>>> ---
>>>   hw/arm/virt-acpi-build.c    | 1 +
>>>   include/hw/acpi/acpi-defs.h | 8 ++++++++
>>>   2 files changed, 9 insertions(+)
>>>
>>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>>> index 5785fb697c..aa177ba64d 100644
>>> --- a/hw/arm/virt-acpi-build.c
>>> +++ b/hw/arm/virt-acpi-build.c
>>> @@ -448,6 +448,7 @@ build_iort(GArray *table_data, BIOSLinker
>>> *linker, VirtMachineState *vms)
>>>           smmu->mapping_count = cpu_to_le32(1);
>>>           smmu->mapping_offset = cpu_to_le32(sizeof(*smmu));
>>>           smmu->base_address = cpu_to_le64(vms->memmap[VIRT_SMMU].base);
>>> +        smmu->flags = ACPI_IORT_SMMU_V3_COHACC_OVERRIDE;
>>>           smmu->event_gsiv = cpu_to_le32(irq);
>>>           smmu->pri_gsiv = cpu_to_le32(irq + 1);
>>>           smmu->gerr_gsiv = cpu_to_le32(irq + 2);
>>> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
>>> index af8e023968..c3ee1f517b 100644
>>> --- a/include/hw/acpi/acpi-defs.h
>>> +++ b/include/hw/acpi/acpi-defs.h
>>> @@ -628,6 +628,12 @@ struct AcpiIortItsGroup {
>>>   } QEMU_PACKED;
>>>   typedef struct AcpiIortItsGroup AcpiIortItsGroup;
>>>   +enum {
>>> +    ACPI_IORT_SMMU_V3_COHACC_OVERRIDE = 1 << 0,
>>> +    ACPI_IORT_SMMU_V3_HTTU_OVERRIDE   = 3 << 1,
>>> +    ACPI_IORT_SMMU_V3_PXM_VALID       = 1 << 3
>>> +};
>>> +
>>>   struct AcpiIortSmmu3 {
>>>       ACPI_IORT_NODE_HEADER_DEF
>>>       uint64_t base_address;
>>> @@ -639,6 +645,8 @@ struct AcpiIortSmmu3 {
>>>       uint32_t pri_gsiv;
>>>       uint32_t gerr_gsiv;
>>>       uint32_t sync_gsiv;
>>> +    uint32_t pxm;
> So if we use this field ,we need to set the flags with
> ACPI_IORT_SMMU_V3_PXM_VALID
>>> +    uint32_t id_mapping_index;
> And if we use this field, it needs to set the revision to at least 1.
But is it harmful to add those fields in the struct as this patch does?

- in our case ACPI_IORT_SMMU_V3_PXM_VALID flag is not set so the field
value is ignored according to the spec and arm_smmu_v3_set_proximity()
will not do anything.

- SMMU control interrupts are all GSIV based so spec says that deviceID
index is ignored.

So eventually the fact the struct size is changing over the revisions
does not look a problem because the node length is part of the struct
and the offset to the ID array also is part of the structure.

So I could have left the structure as before (because we don't use the
functionalities associated to those fields). But on the other hand it's
good to upgrade the struct according to Rev D now.

So I think the patch is correct, isn't?

Thanks

Eric
> 
> Thanks,
> Shannon
>>>       AcpiIortIdMapping id_mapping_array[0];
>>>   } QEMU_PACKED;
>>>   typedef struct AcpiIortSmmu3 AcpiIortSmmu3;
>>>

Re: [Qemu-devel] [PATCH for-3.1] hw/arm/virt-acpi-build: Fix SMMUv3 ACPI integration

[Shannon Zhao]

On 2018/11/29 1:26, Auger Eric wrote:
>>>>    struct AcpiIortSmmu3 {
>>>>        ACPI_IORT_NODE_HEADER_DEF
>>>>        uint64_t base_address;
>>>> @@ -639,6 +645,8 @@ struct AcpiIortSmmu3 {
>>>>        uint32_t pri_gsiv;
>>>>        uint32_t gerr_gsiv;
>>>>        uint32_t sync_gsiv;
>>>> +    uint32_t pxm;
>> So if we use this field ,we need to set the flags with
>> ACPI_IORT_SMMU_V3_PXM_VALID
>>>> +    uint32_t id_mapping_index;
>> And if we use this field, it needs to set the revision to at least 1.
> But is it harmful to add those fields in the struct as this patch does?
> 
> - in our case ACPI_IORT_SMMU_V3_PXM_VALID flag is not set so the field
> value is ignored according to the spec and arm_smmu_v3_set_proximity()
> will not do anything.
> 
> - SMMU control interrupts are all GSIV based so spec says that deviceID
> index is ignored.
> 
> So eventually the fact the struct size is changing over the revisions
> does not look a problem because the node length is part of the struct
> and the offset to the ID array also is part of the structure.
> 
> So I could have left the structure as before (because we don't use the
> functionalities associated to those fields). But on the other hand it's
> good to upgrade the struct according to Rev D now.
> 
> So I think the patch is correct, isn't?
Yes, I think it's not harmful but it would be better to add some 
comments to explain why we don't increase the revision number ATM.

Thanks,
Shannon

Re: [Qemu-devel] [PATCH for-3.1] hw/arm/virt-acpi-build: Fix SMMUv3 ACPI integration

[Auger Eric]

Hi Shannon,

On 11/29/18 3:24 AM, Shannon Zhao wrote:
> 
> 
> On 2018/11/29 1:26, Auger Eric wrote:
>>>>>    struct AcpiIortSmmu3 {
>>>>>        ACPI_IORT_NODE_HEADER_DEF
>>>>>        uint64_t base_address;
>>>>> @@ -639,6 +645,8 @@ struct AcpiIortSmmu3 {
>>>>>        uint32_t pri_gsiv;
>>>>>        uint32_t gerr_gsiv;
>>>>>        uint32_t sync_gsiv;
>>>>> +    uint32_t pxm;
>>> So if we use this field ,we need to set the flags with
>>> ACPI_IORT_SMMU_V3_PXM_VALID
>>>>> +    uint32_t id_mapping_index;
>>> And if we use this field, it needs to set the revision to at least 1.
>> But is it harmful to add those fields in the struct as this patch does?
>>
>> - in our case ACPI_IORT_SMMU_V3_PXM_VALID flag is not set so the field
>> value is ignored according to the spec and arm_smmu_v3_set_proximity()
>> will not do anything.
>>
>> - SMMU control interrupts are all GSIV based so spec says that deviceID
>> index is ignored.
>>
>> So eventually the fact the struct size is changing over the revisions
>> does not look a problem because the node length is part of the struct
>> and the offset to the ID array also is part of the structure.
>>
>> So I could have left the structure as before (because we don't use the
>> functionalities associated to those fields). But on the other hand it's
>> good to upgrade the struct according to Rev D now.
>>
>> So I think the patch is correct, isn't?
> Yes, I think it's not harmful but it would be better to add some
> comments to explain why we don't increase the revision number ATM.
OK thank you for your input. I will repost for 3.2 and will upgrade the
table structs and their associated revision fields.

Thanks

Eric
> 
> Thanks,
> Shannon
> 

Re: [Qemu-devel] [PATCH for-3.1] hw/arm/virt-acpi-build: Fix SMMUv3 ACPI integration

[Peter Maydell]

On Mon, 26 Nov 2018 at 15:46, Eric Auger <eric.auger@redhat.com> wrote:
>
> The AcpiIortSmmu3 misses 2 32b fields corresponding to the
> proximity domain and the device id mapping index.
>
> Also let's report IO-coherent access is supported for
> translation table walks, descriptor fetches and queues by
> setting the COHACC override flag. Without that, we observe
> wrong command opcodes. The DT description also advertises
> the dma coherency.
>
> Fixes a703b4f6c1ee ("hw/arm/virt-acpi-build: Add smmuv3 node in IORT table")
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Reported-by: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>

Hi; it looks (from looking at the code) like this isn't a
regression from 3.0 -- is that right?

I think this fix has missed rc3 (discussion seems to be ongoing
and it needs more time on-list for review), which means it's
likely to miss the 3.1 release. So at the moment I'm expecting
it to go into 4.0. (Maybe if we need an rc4 for some other reason
we might be able to put it in.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH for-3.1] hw/arm/virt-acpi-build: Fix SMMUv3 ACPI integration

[Auger Eric]

Hi Peter,

On 11/27/18 2:32 PM, Peter Maydell wrote:
> On Mon, 26 Nov 2018 at 15:46, Eric Auger <eric.auger@redhat.com> wrote:
>>
>> The AcpiIortSmmu3 misses 2 32b fields corresponding to the
>> proximity domain and the device id mapping index.
>>
>> Also let's report IO-coherent access is supported for
>> translation table walks, descriptor fetches and queues by
>> setting the COHACC override flag. Without that, we observe
>> wrong command opcodes. The DT description also advertises
>> the dma coherency.
>>
>> Fixes a703b4f6c1ee ("hw/arm/virt-acpi-build: Add smmuv3 node in IORT table")
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> Reported-by: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> 
> Hi; it looks (from looking at the code) like this isn't a
> regression from 3.0 -- is that right?
Correct, the lack of COHACC override flag has been there from the very
beginning.
> 
> I think this fix has missed rc3 (discussion seems to be ongoing
> and it needs more time on-list for review), which means it's
> likely to miss the 3.1 release. So at the moment I'm expecting
> it to go into 4.0. (Maybe if we need an rc4 for some other reason
> we might be able to put it in.)
OK

Thanks

Eric
> 
> thanks
> -- PMM
>