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 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.
Could you please detail 3rd approach and how different it is from 
approach 2.
> > Which of the two you find cleaner?
> > > This would also avoid to introduce a fake ID for DT as you currently
> > > do in patch #2.
> > This can be avoided by storing translator_id only for acpi.
> >
> > +static int add_to_host_its_list(u64 addr, u64 size,
> > +                      u32 translation_id, const void *node)
> > +{
> > +    struct host_its *its_data;
> > +    its_data = xzalloc(struct host_its);
> > +
> > +    if ( !its_data )
> > +        return -1;
> > +
> > +    if ( node )
> > +        its_data->dt_node = node;
> > +    else
> > +        its_data->translation_id = translation_id;
> > +
> > +    its_data->addr = addr;
> > +    its_data->size = size;
> > +    printk("GICv3: Found ITS @0x%lx\n", addr);
> > +
> > +    list_add_tail(&its_data->entry, &host_its_list);
> > +
> > +    return 0;
> >
> > What do you think?
> I don't want to see the translation_id stored for no use at all but 
> creating the DOM0 ACPI tables. Is that clearer?
ok, I will remove it.
> Cheers,

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