Re: [Qemu-devel] [PATCH-for-4.0 v2 2/2] hw/arm/virt-acpi-build: IORT Update for revision D

[Auger Eric]

Hi Drew,

On 12/17/18 5:27 PM, Andrew Jones wrote:
> On Thu, Dec 06, 2018 at 06:07:33PM +0100, Eric Auger wrote:
>> Let's update the structs according to revision D of the IORT
>> specification and set the IORT node revision fields.
>>
>> In IORT smmuv3 node: the new proximity field is not used as
>> the proximity domain valid flag is not set. The new DeviceId
>> mapping index is not used either as all the SMMU control
>> interrupts are GSIV based.
>>
>> In IORT RC node: the new memory address size limit field is
>> set to 64 bits.
>>
>> Signed-off-by: Eric Auger <address@hidden>
>>
>> ---
>>
>> v1 -> v2:
>> - separate patches for SMMUv3 DMA coherency issue and struct
>>   update to revision D.
>> - revision fields set
>> ---
>>  hw/arm/virt-acpi-build.c    |  4 ++++
>>  include/hw/acpi/acpi-defs.h | 10 +++++++++-
>>  2 files changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index aa177ba64d..a34a0958a5 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -423,6 +423,7 @@ build_iort(GArray *table_data, BIOSLinker *linker, 
>> VirtMachineState *vms)
>>       */
>>      iort_node_offset = sizeof(*iort);
>>      iort->node_offset = cpu_to_le32(iort_node_offset);
>> +    iort->revision = 0;
>>  
>>      /* ITS group node */
>>      node_size =  sizeof(*its) + sizeof(uint32_t);
>> @@ -445,6 +446,7 @@ build_iort(GArray *table_data, BIOSLinker *linker, 
>> VirtMachineState *vms)
>>  
>>          smmu->type = ACPI_IORT_NODE_SMMU_V3;
>>          smmu->length = cpu_to_le16(node_size);
>> +        smmu->revision = cpu_to_le32(2);
>>          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);
>> @@ -470,6 +472,7 @@ build_iort(GArray *table_data, BIOSLinker *linker, 
>> VirtMachineState *vms)
>>  
>>      rc->type = ACPI_IORT_NODE_PCI_ROOT_COMPLEX;
>>      rc->length = cpu_to_le16(node_size);
>> +    rc->revision = cpu_to_le32(1);
>>      rc->mapping_count = cpu_to_le32(1);
>>      rc->mapping_offset = cpu_to_le32(sizeof(*rc));
>>  
>> @@ -477,6 +480,7 @@ build_iort(GArray *table_data, BIOSLinker *linker, 
>> VirtMachineState *vms)
>>      rc->memory_properties.cache_coherency = cpu_to_le32(1);
>>      rc->memory_properties.memory_flags = 0x3; /* CCA = CPM = DCAS = 1 */
>>      rc->pci_segment_number = 0; /* MCFG pci_segment */
>> +    rc->memory_address_limit = 64;
> 
> Hmmm... IIUC this provides the size of the IOVA space. When it's 64 the
> size of the space is U64_MAX, which gets added to the DMA base (also 64
> bits) when calculating things like the last PFN. It seems strange to use
> a value that will surely overflow those calculations. Is it common to
> put 64 here? Can you elaborate on this?

I looked at drivers/acpi/arm64/iort.c nc_dma_get_range.
There you can find

        *size = ncomp->memory_address_limit >= 64 ? U64_MAX :
                        1ULL<<ncomp->memory_address_limit;

So I was expecting the value "64" to be properly handled by the kernel.
If one decides to select something less than the whole range, which
value would you suggest?
> 
>>  
>>      /* Identity RID mapping covering the whole input RID range */
>>      idmap = &rc->id_mapping_array[0];
>> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
>> index 532eaf79bd..b4a5104367 100644
>> --- a/include/hw/acpi/acpi-defs.h
>> +++ b/include/hw/acpi/acpi-defs.h
>> @@ -628,7 +628,11 @@ struct AcpiIortItsGroup {
>>  } QEMU_PACKED;
>>  typedef struct AcpiIortItsGroup AcpiIortItsGroup;
>>  
>> -#define ACPI_IORT_SMMU_V3_COHACC_OVERRIDE 1
>> +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
>> +};
> 
> We don't usually add flag definitions until we need them. Indeed it'll
> just be more code to delete when switching to the aml_append* API.

The rationale was that those flags becomes usable with this revision so
I decided to expose them. Now I don't have a strong opinion here.

Thanks

Eric
> 
>>  
>>  struct AcpiIortSmmu3 {
>>      ACPI_IORT_NODE_HEADER_DEF
>> @@ -641,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;
>> @@ -650,6 +656,8 @@ struct AcpiIortRC {
>>      AcpiIortMemoryAccess memory_properties;
>>      uint32_t ats_attribute;
>>      uint32_t pci_segment_number;
>> +    uint8_t memory_address_limit;
>> +    uint8_t reserved2[3];
>>      AcpiIortIdMapping id_mapping_array[0];
>>  } QEMU_PACKED;
>>  typedef struct AcpiIortRC AcpiIortRC;
>> -- 
>> 2.17.2
>>
>>
> 
> Thanks,
> drew
>