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

[Auger Eric]

Hi Prem,
On 23/09/2016 16:07, Prem Mallappa wrote:
> On Fri, Sep 23, 2016 at 6:40 PM, Auger Eric <address@hidden
> <mailto:address@hidden>> wrote:
> 
>     Hi Prem,
> 
>     On 12/09/2016 22:42, Prem Mallappa wrote:
>     > On Fri, Sep 9, 2016 at 8:54 PM, Auger Eric <address@hidden
>     <mailto: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
>     <mailto: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.

OK thanks.

Looking forward to reviewing your patch

Best Regards

Eric
> 
>     Thanks
> 
>     Eric
>     >
>     >
> 
> 
> Hi Eric,
> I have been busy with something else, however I have a wokring patch set
> (unclean version)
> which creates the IORT tables, with SMMU->RC->ITS with ITS id mapping
> (routing all interrupts to single ITS).
> I'll push them by to unstable branch by this Sunday.
> 
> -- 
> Cheers,
> /Prem