[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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





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.

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.