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

[Manish Jaggi <mjaggi@caviumnetworks.com>]

On 6/8/2017 6:39 PM, Julien Grall wrote:
> Hi Manish,
>
Hi Julien,
> 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.
>
I will add it.

>>
>> Signed-off-by: Manish Jaggi <mjaggi@cavium.com>
>> ---
>>  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.
>
You are right, I also thought that
How about 3 files
domain_build.c
acpi_domain_build.c
dt_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.
If you look a bit closer this is used at multiple places
see fixup_pcirc_node, hide_smmu_iort.
>
>>
>>  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.
Sure.
>
>> +    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.
>
I will add detail comments.
>> +        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:
>
The  assumption is based on the ACPI tables on two platforms ThunderX 
and ThunderX2.
While the spec does not deny it but would there be a use case as such 
where a PCI node id array would split the
range into the same smmu.


> 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
>
This is not relevant as this code wont touch RC A.
> 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
>
It can be from 2 different RC's and not from same RC.
> // 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*
>
Spec does not prevent that, but we need to see IMHO what all cases are 
practically possible and current platforms support it.
Is there any platform which supports that ? I can add code for the 
combinations but how I will test it.
> [...]

>
>> 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.
>
ok.
>>  };
>>
>>  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,
>


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