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

[Julien Grall <julien.grall@arm.com>]

On 09/06/2017 08:13, Manish Jaggi wrote:
> On 6/8/2017 6:39 PM, Julien Grall wrote:
>> Hi Manish,
>>
> Hi Julien,

Hello,

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

If you want to split the current code, then fine. But it is not strictly 
mandatory for this code. What I want is adding new code in separate 
files. But in this case they should be named:

domain_build.c
acpi/domain_build.c
dt/domain_build.c

This would keep the ACPI and DT firmware code separated and not 
polluting the arch/arm.

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

My point stands... you could have passed iort_base_ptr as an extra 
parameter of the functions. Or even use kinfo.

Anyway, at the moment I don't see any reason to have this global variable.

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

May I remind you that the goal of Xen is to run on *all* the current and 
future platforms. If the spec says it is allowed, then we should do it 
unless there is a strong reason not to do it.

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

Can you avoid to dismiss any example that don't fit your solution? This 
is not helpful.

Describing the RC is relevant in my example to show a case that your 
solution will not handle.

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

It is not my point in this example. My point is same RC with split 
DeviceID mapping.

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

See above.

> Is there any platform which supports that ? I can add code for the
> combinations but how I will test it.

The only thing I can tell you is the spec allows it and your suggestion 
would have to be fully rewritten if someone decide to not follow your 
assumptions.

On the previous thread "xen/arm: Hiding SMMUs from Dom0 when using ACPI 
on Xen", I made 2 suggestions which, I believe, is spec-proof:

1) Resolve all the RID (or platform device ID) to a DeviceID one by one 
and generating the a new IORT for DOM0 with that
2) Generating new DeviceID mapping for each RID mapping

Solution 1 would be the easiest to do and could be tested on any 
platform as the algo would be based on the IORT parsing.

So I don't see why we should have a limiting solution at the moment.

Regards,

-- 
Julien Grall

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