Re: [Xen-devel] [PATCH 0/4] ARM: ACPI: ITS: Add ITS Support for ACPI hardware domain

[Manish Jaggi <mjaggi@xxxxxxxxxxxxxxxxxx>]

HI Julien,

On 8/10/2017 5:43 PM, Julien Grall wrote:
> On 10/08/17 13:00, Manish Jaggi wrote:
> > Hi Julien,
> >
> > On 8/10/2017 4:58 PM, Julien Grall wrote:
> > > On 10/08/17 12:21, Manish Jaggi wrote:
> > > > Hi Julien,
> > > >
> > > > On 6/21/2017 6:53 PM, Julien Grall wrote:
> > > > > Hi Manish,
> > > > >
> > > > > On 21/06/17 02:01, Manish Jaggi wrote:
> > > > > > This patch series adds the support of ITS for ACPI hardware domain.
> > > > > > It is tested on staging branch with has ITS v12 patchset by Andre.
> > > > > >
> > > > > > I have tried to incorporate the review comments on the RFC v1/v2
> > > > > > patch.
> > > > > > The single patch in RFC is now split into 4 patches.
> > > > > I will comment here rather than on each patches.
> > > > >
> > > > > > Patch1: ARM: ITS: Add translation_id to host_its
> > > > > >  Adds translation_id in host_its data structure, which is populated
> > > > > > from
> > > > > >  translation_id read from firmwar MADT. This value is then 
> > > > > > programmed
> > > > > > into
> > > > > >  local MADT created for hardware domain in patch 4.
> > > > > I don't see any reason to store value that will only be used for
> > > > > generating the MADT which BTW is just a copy for the ITS. Instead we
> > > > > should copy over the MADT entries.
> > > > There are two approaches,
> > > >
> > > > If I use the standard API  acpi_table_parse_madt which would iterate
> > > > over ACPI_MADT_TYPE_GENERIC_TRANSLATOR entries, I have to maintain the
> > > > addr and translation_id in some data structure, to be filled later in
> > > > the hwdomain copy of madt generic translator.
> > > >
> > > > If I don't use the standard API I have to add code to manually 
> > > > parse all
> > > > the translator entries.
> > > There are a 3rd approach I suggested and ignored... The ITS entries
> > > for Dom0 is exactly the same as the host entries.
> > Yes, and if not passed properly dom0 wont get device interrupts...
> > > So you only need to do a verbatim copy of the entry...
> > Can you please check patch 4/2, the translation_id and address are
> > passed verbatim, the other values are reserved in
> > acpi_madt_generic_translator.
> For ACPI, we took the approach to only rewrite what's necessary and 
> give the rest to Dom0 as it is. If newer version of ACPI re-used those 
> fields, then they will be copied over to Dom0. I don't consider it as 
> an issue because the problem would be the same if those fields have an 
> important meaning for the platform.
Few thoughts...
If we follow this approach, few points needs to be considered
- If ACPI may use the reserved information later it could be equally 
important for dom0 and Xen,
 so it might be useful to keep reserved in xen as well.

- For platforms which use dt, translation_id is not required to be 
stored in struct host_its, similarly for platforms which use acpi
dt_node pointer might be of no use.

So we can have struct host_its having a union with dt_device_node * for 
dt and acpi_madt_generic_translator * for acpi.
IMHO this could be an approach we can take.

struct host_its {
     struct list_head entry;
-    const struct dt_device_node *dt_node;
+   union {
+    const struct dt_device_node *dt_node;
+    const struct acpi_madt_generic_translator *acpi_its_entry;
+    };
    paddr_t addr;


> > Could you please detail 3rd approach and how different it is from
> > approach 2.
> ACPI_MEMCPY(its, host_its, size);
>
> Cheers,

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel