Re: [Qemu-devel] [PATCH v2 9/9] [optional] arm: smmu-v3: ACPI IORT initial support

[Auger Eric]

Hi Prem,

On 12/09/2016 22:42, Prem Mallappa wrote:
> On Fri, Sep 9, 2016 at 8:54 PM, Auger Eric <address@hidden> wrote:
> 
>> Hi Prem,
>>
>> On 22/08/2016 18:17, Prem Mallappa wrote:
>>> Added ACPI IORT tables, was needed for internal project purpose, but
>>> posting here for anyone looking for testing ACPI on ARM platforms.
>>> (P.S: Linux side IORT patches are WIP)
>> I am also interested in IORT ITS group and currently prototyping
>> something, hence my few comments/questions.
>>>
>>> Signed-off-by: Prem Mallappa <address@hidden>
>>> ---
>>>  hw/arm/virt-acpi-build.c    | 43 +++++++++++++++++++++++
>>>  include/hw/acpi/acpi-defs.h | 84 ++++++++++++++++++++++++++++++
>> +++++++++++++++
>>>  2 files changed, 127 insertions(+)
>>>
>>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>>> index 1fa0581..d5fb69e 100644
>>> --- a/hw/arm/virt-acpi-build.c
>>> +++ b/hw/arm/virt-acpi-build.c
>>> @@ -382,6 +382,45 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker,
>> unsigned rsdt_tbl_offset)
>>>      return rsdp_table;
>>>  }
>>>
>>> +/*
>>> + * TODO: Simple IORT for now, will add ID mappings as we go
>>> + * basic idea is to instantiate SMMU from ACPI
>>> + */
>>> +static void
>>> +build_iort(GArray *table_data, BIOSLinker *linker, VirtGuestInfo
>> *guest_info)
>>> +{
>>> +    int iort_start = table_data->len;
>>> +    AcpiIortTable *iort;
>>> +    AcpiIortNode *iort_node;
>>> +    AcpiIortSmmu3 *smmu;
>>> +    AcpiIortRC *rc;
>>> +    const MemMapEntry *memmap = guest_info->memmap;
>>> +
>>> +    iort = acpi_data_push(table_data, sizeof(*iort));
>>> +
>>> +    iort->length = sizeof(*iort);
>> Isn't is supposed to be the length of the whole IORT (including the node
>> cumulated sizes?)
>>> +    iort->node_offset = table_data->len - iort_start;
>>> +    iort->num_nodes++;
>>> +
>>> +    smmu = acpi_data_push(table_data, sizeof(*smmu));
>>> +    iort_node = &smmu->iort_node;
>>> +    iort_node->type = 0x04;          /* SMMUv3 */
>> To match existing code (include/hw/acpi/acpi-defs.h), maybe enum values
>> can be created (ACPI_IORT_NODE_SMMU_V3). This also matches kernel enum.
>>
>> I have made these changes, will send out ASAP.
> 
> 
>> More generally Shannon advised to use the same field names as the ones
>> used in the kernel header: acpi_iort_node_type in include/acpi/actbl2.h
>>
> 
> Will change this accordingly
> 
> 
>>> +    iort_node->length = sizeof(*smmu);
>>> +    smmu->base_addr = cpu_to_le64(memmap[VIRT_SMMU].base);
>>> +
>>> +    iort->num_nodes++;
>>> +
>>> +    rc = acpi_data_push(table_data, sizeof(*rc));
>>> +    iort_node = &rc->iort_node;
>>> +    iort_node->type = 0x02;          /* RC */
>>> +    iort_node->length = sizeof(*rc);
>> I think the mem_access_prop field should be set to 1 now the host
>> controller is assumed to be cache coherent.
>>> +    rc->ats_attr = 1;
>> no ATS support instead?
>>> +    rc->pci_seg_num = 0;
>> ID mappings are mandated for me to support MSIs with ITS.
>>
> 
> These changes are made as I write,
> 
> 
>> Shannon told me we should match the kernel datatypes & fields
>>
>> for instance in include/acpi/actbl2.h we have:
>>
>> struct acpi_iort_id_mapping {
>>         u32 input_base;         /* Lowest value in input range */
>>         u32 id_count;           /* Number of IDs */
>>         u32 output_base;        /* Lowest value in output range */
>>         u32 output_reference;   /* A reference to the output node */
>>         u32 flags;
>> };
>>
>> This also holds for other struct definitions.
>>
>>
> Sure will change this accordingly.

I currently have a series creating the IORT with an RC node and an ITS
node. It is needed to complete the integration of the virtual ITS (to
connect it with the PCI host controller). This originates from this
patch: I added the RC->ITS ID mapping + the ITS node and tested it.

I don't know how to proceed to get the 2 features (vSMMU and vITS)
progress separately. Do you plan to respin this patch quickly?
Otherwise, if you are busy with other things I propose you to respin
fixing the few things above, splitting it into 3 patches, header [1],
ITS node creation [2], RC node creation with RC->ITS mapping [3] while
keeping credit to you on [1] and [3].

Then we can have a 4th patch adding RC-> SMMU ID > ITS mapping?

Please let me know what are your plans and what do you think.

Thanks

Eric
> 
>