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

[Manish Jaggi <mjaggi@xxxxxxxxxxxxxxxxxx>]

On 8/10/2017 6:44 PM, Julien Grall wrote:
> On 08/10/2017 02:00 PM, Manish Jaggi wrote:
> > 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.
> I already covered that in my previous e-mail.
Yes, I am just stating it again for xen.
> > - 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;
> What don't you get in my previous e-mail? A no is a no, full stop.
This is not helping.
> Just do what we do in *_make_hwdom_madt. That will work here with no 
> need of a union or anything else.
The patchset provides two features
 (a) populates host_its list from ACPI tables, so ACPI xen can use ITS
 (b) provides a MADT with ITS information to dom0.

What I am focusing with union is for (a) ,
and (b) code would be simpler if we use the union in (a).

You seem to be discounting (a) in comments so far.

why union? as I have mentioned before...
 It will make the host_its structure accommodate dt node and 
acpi_madt_generic_translator, both has same purpose.
If one is valid why not other.

please provide a technical reason for not doing it.

> Even the DT code can be reworked to avoid storing the node.
we can have a separate patch for that.


> Cheers,
Cheers!
Sending next rev shortly.

-manish

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