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

[Manish Jaggi <mjaggi@xxxxxxxxxxxxxxxxxx>]

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.
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?
> > Patch2: ARM: ITS: ACPI: Introduce gicv3_its_acpi_init
> >  Introduces function for its_acpi_init, which calls add_to_host_its_list
> >  which is a common function also called from _dt variant.
> Just reading at the description, there are a call for splitting this 
> patch... Looking at the code, you mix code movement and code addition.
> Have a look at [1] to see how to break patches.
Yes I will break into multiple patches patch 2 and 4.
> > Patch3: ARM: ITS: Deny hardware domain access to its
> >  Extends the gicv3_iomem_deny to include its regions as well
> >
> > Patch4: ARM: ACPI: Add ITS to hardware domain MADT
> >  This patch adds ITS information in hardware domain's MADT table.
> >  Also this patch interoduces .get_hwdom_madt_size in gic_hw_operations,
> >  to return the complete size of MADT table for hardware domain.
> Same here.
Yes.
> > Manish Jaggi (4):
> >   ARM: ITS: Add translation_id to host_its
> >   ARM: ITS: ACPI: Introduce gicv3_its_acpi_init
> >   ARM: ITS: Deny hardware domain access to its
> >   ARM: ACPI: Add ITS to hardware domain MADT
> >
> >  xen/arch/arm/domain_build.c      |   7 +--
> >  xen/arch/arm/gic-v2.c            |   6 +++
> >  xen/arch/arm/gic-v3-its.c        | 102 
> > +++++++++++++++++++++++++++++++++++----
> >  xen/arch/arm/gic-v3.c            |  31 ++++++++++++
> >  xen/arch/arm/gic.c               |  11 +++++
> >  xen/include/asm-arm/gic.h        |   3 ++
> >  xen/include/asm-arm/gic_v3_its.h |  36 ++++++++++++++
> >  7 files changed, 180 insertions(+), 16 deletions(-)
> Cheers,
>
> [1] 
> https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches#Making_good_patches

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