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

[Manish Jaggi <mjaggi@caviumnetworks.com>]

HI Julien,

On 6/9/2017 2:53 PM, Julien Grall wrote:
>
>
> 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.
I will follow this structure.
>
>>>>  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.
>
ok, I will pass it as a parameter.
>>>
>>>>
>>>>  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.
Sure. I will add more description in that case.
>
> Describing the RC is relevant in my example to show a case that your 
> solution will not handle.
I will add my rationale here. Hiding smmu from IORT table would require 
setting device ID in the pci_rc id_array for RID and output reference as 
ITS group.
For the RC idarray elements which don't have an output reference as smmu 
but a ITS group, there is no need to touch them.
Based on this rationale I said this is not relevant.
>
>>> 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.
ok, I will add that as well.
The current code parses all entries in pci_rc id_array and patches 
output reference and output_base.
The only assumption was on Number of ids in pci_rc id_array element and 
the matching smmu id_array element be same.
I can remove the assumption, will that be ok?

So,One ID Mapping element (Input_base, num_ids, out_base) can translate 
into two or more id_array entries of smmu node.

>
>>> // 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.
>
See above
> 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,
>


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