Re: [Qemu-arm] [Qemu-devel] [PATCH-for-4.0 v2 1/2] hw/arm/virt-acpi-build: Set COHACC override flag in IORT SMMUv3 node

[Auger Eric]

Hi Drew,

On 12/17/18 5:02 PM, Andrew Jones wrote:
> On Thu, Dec 06, 2018 at 06:07:32PM +0100, Eric Auger wrote:
>> 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 <address@hidden>
>> Reported-by: Shameerali Kolothum Thodi <address@hidden>
>> Tested-by: Shameer Kolothum <address@hidden>
>>
>> ---
>>
>> - change the commit title
>> - addition of new fields (pxm and id_mapping_index) done in a
>>   separate patch
>> ---
>>  hw/arm/virt-acpi-build.c    | 1 +
>>  include/hw/acpi/acpi-defs.h | 2 ++
>>  2 files changed, 3 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;
> 
> Flags are 4 bytes in length, so you need cpu_to_le32()
Hum right!
> 
>>          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..532eaf79bd 100644
>> --- a/include/hw/acpi/acpi-defs.h
>> +++ b/include/hw/acpi/acpi-defs.h
>> @@ -628,6 +628,8 @@ struct AcpiIortItsGroup {
>>  } QEMU_PACKED;
>>  typedef struct AcpiIortItsGroup AcpiIortItsGroup;
>>  
>> +#define ACPI_IORT_SMMU_V3_COHACC_OVERRIDE 1
>> +
>>  struct AcpiIortSmmu3 {
>>      ACPI_IORT_NODE_HEADER_DEF
>>      uint64_t base_address;
>> -- 
>> 2.17.2
>>
>>
> 
> Probably not necessary for this series, but we should really switch all
> table generators to the aml_append* API.
Agreed, I will prepare a separate series for that improvement.

> 
> Besides the missing cpu_to_le32(),
> 
> Reviewed-by: Andrew Jones <address@hidden>
Thanks

Eric
>