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

[Manish Jaggi <mjaggi@xxxxxxxxxxxxxxxxxx>]

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