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

[Manish Jaggi <mjaggi@xxxxxxxxxxxxxxxxxx>]

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@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.
> > 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel