Re: [Xen-devel] [RFC] [PATCH] arm-acpi: Hide SMMU from IORT for hardware domain

[Julien Grall <julien.grall@xxxxxxx>]

Hi Manish,

On 08/06/17 13:38, Manish Jaggi wrote:
>
Spurious line.

> This patch disables the smmu node in IORT table for hardware domain.
> Also patches the output_base of pci_rc id_array with output_base of
> smmu node id_array.
I would have appreciated a bit more description in the commit message to 
explain your logic.
> Signed-off-by: Manish Jaggi <mjaggi@xxxxxxxxxx>
> ---
>  xen/arch/arm/domain_build.c | 142
> +++++++++++++++++++++++++++++++++++++++++++-
domain_build.c is starting to be really big. I think it is time to move 
some acpi bits outside domain_build.c.
>  xen/include/acpi/actbl2.h   |   3 +-
>  xen/include/asm-arm/acpi.h  |   1 +
>  3 files changed, 144 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index d6d6c94..9f41d0e 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -32,6 +32,7 @@ integer_param("dom0_max_vcpus", opt_dom0_max_vcpus);
>  int dom0_11_mapping = 1;
>
>  static u64 __initdata dom0_mem;
> +static u8 *iort_base_ptr;
Looking at the code, I don't see any reason to have this global.

>  static void __init parse_dom0_mem(const char *s)
>  {
> @@ -1336,6 +1337,96 @@ static int prepare_dtb(struct domain *d, struct
> kernel_info *kinfo)
>  #ifdef CONFIG_ACPI
>  #define ACPI_DOM0_FDT_MIN_SIZE 4096
>
> +static void patch_output_ref(struct acpi_iort_id_mapping *pci_idmap,
> +                      struct acpi_iort_node *smmu_node)
> +{
> +    struct acpi_iort_id_mapping *idmap = NULL;
> +    int i;
Newline.

> +    for (i=0; i < smmu_node->mapping_count; i++) {
Please respect Xen coding style... I expect you to fix *all* the place 
in the next version.
Also, there is a latent lack of comments within the patch to explain the 
logic.
> +        if(!idmap)
> +            idmap = (struct acpi_iort_id_mapping*)((u8*)smmu_node
> +                                          + smmu_node->mapping_offset);
> +        else
> +            idmap++;
> +
> +        if (pci_idmap->output_base == idmap->input_base) {
> +            pci_idmap->output_base = idmap->output_base;
> +            pci_idmap->output_reference = idmap->output_reference;
As I pointed out on the previous thread, you assume that one PCI ID 
mapping will end up to be translated to one Device ID mapping and not 
split across multiple one. For instance:
RC A
 // doesn't use SMMU 0 so just outputs DeviceIDs to ITS GROUP 0
 // Input ID --> Output reference: Output ID
0x0000-0xffff --> ITS GROUP 0 : 0x0000->0xffff

SMMU 0
// Note that range of StreamIDs that map to DeviceIDs excludes
// the NIC 0 DeviceID as it does not generate MSIs
 // Input ID --> Output reference: Output ID
0x0000-0x01ff --> ITS GROUP 0 : 0x10000->0x101ff
0x0200-0xffff --> ITS GROUP 0 : 0x20000->0x207ff

// SMMU 0 Control interrupt is MSI based
 // Input ID --> Output reference: Output ID
N/A --> ITS GROUP 0 : 0x200001

I still don't see anything in the spec preventing that. And I would like 
clarification from your side before going forward. *hint* The spec 
should be quoted *hint*
[...]

> diff --git a/xen/include/acpi/actbl2.h b/xen/include/acpi/actbl2.h
> index 42beac4..f180ea5 100644
> --- a/xen/include/acpi/actbl2.h
> +++ b/xen/include/acpi/actbl2.h
> @@ -591,7 +591,8 @@ enum acpi_iort_node_type {
>      ACPI_IORT_NODE_NAMED_COMPONENT = 0x01,
>      ACPI_IORT_NODE_PCI_ROOT_COMPLEX = 0x02,
>      ACPI_IORT_NODE_SMMU = 0x03,
> -    ACPI_IORT_NODE_SMMU_V3 = 0x04
> +    ACPI_IORT_NODE_SMMU_V3 = 0x04,
> +    ACPI_IORT_NODE_RESERVED = 0xff
This is likely a call to a separate patch.

>  };
>
>  struct acpi_iort_id_mapping {
> diff --git a/xen/include/asm-arm/acpi.h b/xen/include/asm-arm/acpi.h
> index 9f954d3..1cc0167 100644
> --- a/xen/include/asm-arm/acpi.h
> +++ b/xen/include/asm-arm/acpi.h
> @@ -36,6 +36,7 @@ typedef enum {
>      TBL_FADT,
>      TBL_MADT,
>      TBL_STAO,
> +    TBL_IORT,
>      TBL_XSDT,
>      TBL_RSDP,
>      TBL_EFIT,
Cheers,

--
Julien Grall

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