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

[Prem Mallappa]

On Fri, Sep 23, 2016 at 6:40 PM, Auger Eric <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>
> 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
> >
> >
>

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