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

[Prem Mallappa]

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.


-- 
Cheers,
/Prem