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

[Julien Grall <julien.grall@xxxxxxx>]

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.
This would also avoid to introduce a fake ID for DT as you currently do 
in patch #2.
> 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.

> 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.

> 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
--
Julien Grall

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