| Message ID | e11c57909782c60a6914d81e9c9893ff1712cc5b.1651075724.git.rahul.singh@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm/acpi: don't expose the ACPI IORT SMMUv3 entry to dom0 | expand |
Hi Rahul, On 27/04/2022 17:12, Rahul Singh wrote: > Xen should control the SMMUv3 devices therefore, don't expose the > SMMUv3 devices to dom0. Deny iomem access to SMMUv3 address space for > dom0 and also make ACPI IORT SMMUv3 node type to 0xff. Looking at the IORT spec (ARM DEN 0049E), 255 (0xff) is marked as reserved. So I don't think we can "allocate" 0xff to mean invalid without updating the spec. Did you engage with whoever own the spec? > > Signed-off-by: Rahul Singh <rahul.singh@arm.com> > --- > xen/arch/arm/acpi/domain_build.c | 40 ++++++++++++++++++++++++++++++++ > 1 file changed, 40 insertions(+) > > diff --git a/xen/arch/arm/acpi/domain_build.c b/xen/arch/arm/acpi/domain_build.c > index bbdc90f92c..ec0b5b261f 100644 > --- a/xen/arch/arm/acpi/domain_build.c > +++ b/xen/arch/arm/acpi/domain_build.c > @@ -14,6 +14,7 @@ > #include <xen/acpi.h> > #include <xen/event.h> > #include <xen/iocap.h> > +#include <xen/sizes.h> > #include <xen/device_tree.h> > #include <xen/libfdt/libfdt.h> > #include <acpi/actables.h> > @@ -30,6 +31,7 @@ static int __init acpi_iomem_deny_access(struct domain *d) > { > acpi_status status; > struct acpi_table_spcr *spcr = NULL; > + struct acpi_table_iort *iort; > unsigned long mfn; > int rc; > > @@ -55,6 +57,44 @@ static int __init acpi_iomem_deny_access(struct domain *d) > printk("Failed to get SPCR table, Xen console may be unavailable\n"); > } > > + status = acpi_get_table(ACPI_SIG_IORT, 0, > + (struct acpi_table_header **)&iort); At some point we will need to add support to hide the ARM SMMU device and possibly some devices. So I think it would be better to create a function that would deal with the IORT. > + > + if ( ACPI_SUCCESS(status) ) > + { > + int i; Please use unsigned int. > + struct acpi_iort_node *node, *end; Coding style: Please add a newline. > + node = ACPI_ADD_PTR(struct acpi_iort_node, iort, iort->node_offset); > + end = ACPI_ADD_PTR(struct acpi_iort_node, iort, iort->header.length); > + > + for ( i = 0; i < iort->node_count; i++ ) > + { > + if ( node >= end ) Wouldn't this only happen if the table is somehow corrupted? If so, I think we should print an error (or even panic). > + break; > + > + switch ( node->type ) > + { > + case ACPI_IORT_NODE_SMMU_V3: Coding style: The keyword "case" should be aligned the the start of the keyword "switch". > + { > + struct acpi_iort_smmu_v3 *smmu; Coding style: Newline. > + smmu = (struct acpi_iort_smmu_v3 *)node->node_data; > + mfn = paddr_to_pfn(smmu->base_address); > + rc = iomem_deny_access(d, mfn, mfn + PFN_UP(SZ_128K)); > + if ( rc ) > + printk("iomem_deny_access failed for SMMUv3\n"); > + node->type = 0xff; 'node' points to the Xen copy of the ACPI table. We should really not touch this copy. Instead, we should modify the version that will be used by dom0. Furthermore, if we go down the road to update node->type, we should 0 the node to avoid leaking the information to dom0. > + break; > + } > + } > + node = ACPI_ADD_PTR(struct acpi_iort_node, node, node->length); > + } > + } > + else > + { > + printk("Failed to get IORT table\n"); > + return -EINVAL; > + } The IORT is not yet parsed by Xen and AFAIK is optional. So I don't think we should return an error. > + > /* Deny MMIO access for GIC regions */ > return gic_iomem_deny_access(d); > } Cheers,
Hi Julien, > On 27 Apr 2022, at 7:26 pm, Julien Grall <julien@xen.org> wrote: > > Hi Rahul, > > On 27/04/2022 17:12, Rahul Singh wrote: >> Xen should control the SMMUv3 devices therefore, don't expose the >> SMMUv3 devices to dom0. Deny iomem access to SMMUv3 address space for >> dom0 and also make ACPI IORT SMMUv3 node type to 0xff. > > Looking at the IORT spec (ARM DEN 0049E), 255 (0xff) is marked as reserved. So I don't think we can "allocate" 0xff to mean invalid without updating the spec. Did you engage with whoever own the spec? Yes I agree with you 0xff is reserved for future use. I didn’t find any other value to make node invalid. Linux kernel is mostly using the node->type to process the SMMUv3 or other IORT node so I thought this is the only possible solution to hide SMMUv3 for dom0 If you have any other suggestion to hide the SMMUv3 node I am okay to use that. > >> Signed-off-by: Rahul Singh <rahul.singh@arm.com> >> --- >> xen/arch/arm/acpi/domain_build.c | 40 ++++++++++++++++++++++++++++++++ >> 1 file changed, 40 insertions(+) >> diff --git a/xen/arch/arm/acpi/domain_build.c b/xen/arch/arm/acpi/domain_build.c >> index bbdc90f92c..ec0b5b261f 100644 >> --- a/xen/arch/arm/acpi/domain_build.c >> +++ b/xen/arch/arm/acpi/domain_build.c >> @@ -14,6 +14,7 @@ >> #include <xen/acpi.h> >> #include <xen/event.h> >> #include <xen/iocap.h> >> +#include <xen/sizes.h> >> #include <xen/device_tree.h> >> #include <xen/libfdt/libfdt.h> >> #include <acpi/actables.h> >> @@ -30,6 +31,7 @@ static int __init acpi_iomem_deny_access(struct domain *d) >> { >> acpi_status status; >> struct acpi_table_spcr *spcr = NULL; >> + struct acpi_table_iort *iort; >> unsigned long mfn; >> int rc; >> @@ -55,6 +57,44 @@ static int __init acpi_iomem_deny_access(struct domain *d) >> printk("Failed to get SPCR table, Xen console may be unavailable\n"); >> } >> + status = acpi_get_table(ACPI_SIG_IORT, 0, >> + (struct acpi_table_header **)&iort); > > At some point we will need to add support to hide the ARM SMMU device and possibly some devices. So I think it would be better to create a function that would deal with the IORT. Ok. Let me add the function in next version. > >> + >> + if ( ACPI_SUCCESS(status) ) >> + { >> + int i; > > Please use unsigned int. Ack. > >> + struct acpi_iort_node *node, *end; > > Coding style: Please add a newline. Ack. > >> + node = ACPI_ADD_PTR(struct acpi_iort_node, iort, iort->node_offset); >> + end = ACPI_ADD_PTR(struct acpi_iort_node, iort, iort->header.length); >> + >> + for ( i = 0; i < iort->node_count; i++ ) >> + { >> + if ( node >= end ) > > Wouldn't this only happen if the table is somehow corrupted? If so, I think we should print an error (or even panic). Ok. > >> + break; >> + >> + switch ( node->type ) >> + { >> + case ACPI_IORT_NODE_SMMU_V3: > > Coding style: The keyword "case" should be aligned the the start of the keyword "switch”. Ack. > >> + { >> + struct acpi_iort_smmu_v3 *smmu; > > Coding style: Newline. Ack. >> + smmu = (struct acpi_iort_smmu_v3 *)node->node_data; >> + mfn = paddr_to_pfn(smmu->base_address); >> + rc = iomem_deny_access(d, mfn, mfn + PFN_UP(SZ_128K)); >> + if ( rc ) >> + printk("iomem_deny_access failed for SMMUv3\n"); >> + node->type = 0xff; > > 'node' points to the Xen copy of the ACPI table. We should really not touch this copy. Instead, we should modify the version that will be used by dom0. As of now IORT is untouched by Xen and mapped to dom0. I will create the IORT table for dom0 and modify the node SMMUv3 that will be used by dom0. > > Furthermore, if we go down the road to update node->type, we should 0 the node to avoid leaking the information to dom0. I am not sure if we can zero the node, let me check and come back to you. > >> + break; >> + } >> + } >> + node = ACPI_ADD_PTR(struct acpi_iort_node, node, node->length); >> + } >> + } >> + else >> + { >> + printk("Failed to get IORT table\n"); >> + return -EINVAL; >> + } > > The IORT is not yet parsed by Xen and AFAIK is optional. So I don't think we should return an error. Ack. Regards, Rahul
On 29/04/2022 19:18, Rahul Singh wrote: > Hi Julien, Hi Rahul, >> On 27 Apr 2022, at 7:26 pm, Julien Grall <julien@xen.org> wrote: >> >> Hi Rahul, >> >> On 27/04/2022 17:12, Rahul Singh wrote: >>> Xen should control the SMMUv3 devices therefore, don't expose the >>> SMMUv3 devices to dom0. Deny iomem access to SMMUv3 address space for >>> dom0 and also make ACPI IORT SMMUv3 node type to 0xff. >> >> Looking at the IORT spec (ARM DEN 0049E), 255 (0xff) is marked as reserved. So I don't think we can "allocate" 0xff to mean invalid without updating the spec. Did you engage with whoever own the spec? > > Yes I agree with you 0xff is reserved for future use. I didn’t find any other value to make node invalid. > Linux kernel is mostly using the node->type to process the SMMUv3 or other IORT node so I thought this is the only possible solution to hide SMMUv3 for dom0 > If you have any other suggestion to hide the SMMUv3 node I am okay to use that. The other solution is to remove completely the SMMUv3 node from the IORT. This would require more work as you would need to fully rewrite the IORT. Hence why I suggested to speak with the spec owner (it seems to be Arm) to reserve 0xff as "Invalid/Ignore". >>> + smmu = (struct acpi_iort_smmu_v3 *)node->node_data; >>> + mfn = paddr_to_pfn(smmu->base_address); >>> + rc = iomem_deny_access(d, mfn, mfn + PFN_UP(SZ_128K)); >>> + if ( rc ) >>> + printk("iomem_deny_access failed for SMMUv3\n"); >>> + node->type = 0xff; >> >> 'node' points to the Xen copy of the ACPI table. We should really not touch this copy. Instead, we should modify the version that will be used by dom0. > > As of now IORT is untouched by Xen and mapped to dom0. I will create the IORT table for dom0 and modify the node SMMUv3 that will be used by dom0. >> >> Furthermore, if we go down the road to update node->type, we should 0 the node to avoid leaking the information to dom0. > > I am not sure if we can zero the node, let me check and come back to you. By writing node->type, you already invalidate the content because the software cannot know how to interpret it. At which point, zeroing it should make no difference for software parsing the table afterwards. This may be a problem for software parsing before hand and keeping a pointer to the entry. But then, this is yet another reason to no updated the host IORT and create a copy for dom0. Cheers,
On 2022-04-27 17:12, Rahul Singh wrote: > Xen should control the SMMUv3 devices therefore, don't expose the > SMMUv3 devices to dom0. Deny iomem access to SMMUv3 address space for > dom0 and also make ACPI IORT SMMUv3 node type to 0xff. ...making the resulting IORT technically useless to consumers. ID mappings for all the Root Complex, Named Component and RMR nodes which were supposed to be translated through that SMMU node are now invalid, because ID mappings can only target an SMMU or ITS node. I can't guess at how other consumers may react, but Linux's IORT code is going to experience undefined behaviour when tries to translate any MSI DeviceID mappings through this invalid node, since it uses a bitmap of (1 << node->type) internally; beyond that we're not as strict as we could be and make some pretty loose assumptions about what we're parsing, so it might even appear to work, but that could easily change at any time in future and is absolutely not something Xen or any other software should try to rely on. Thanks, Robin. > Signed-off-by: Rahul Singh <rahul.singh@arm.com> > --- > xen/arch/arm/acpi/domain_build.c | 40 ++++++++++++++++++++++++++++++++ > 1 file changed, 40 insertions(+) > > diff --git a/xen/arch/arm/acpi/domain_build.c b/xen/arch/arm/acpi/domain_build.c > index bbdc90f92c..ec0b5b261f 100644 > --- a/xen/arch/arm/acpi/domain_build.c > +++ b/xen/arch/arm/acpi/domain_build.c > @@ -14,6 +14,7 @@ > #include <xen/acpi.h> > #include <xen/event.h> > #include <xen/iocap.h> > +#include <xen/sizes.h> > #include <xen/device_tree.h> > #include <xen/libfdt/libfdt.h> > #include <acpi/actables.h> > @@ -30,6 +31,7 @@ static int __init acpi_iomem_deny_access(struct domain *d) > { > acpi_status status; > struct acpi_table_spcr *spcr = NULL; > + struct acpi_table_iort *iort; > unsigned long mfn; > int rc; > > @@ -55,6 +57,44 @@ static int __init acpi_iomem_deny_access(struct domain *d) > printk("Failed to get SPCR table, Xen console may be unavailable\n"); > } > > + status = acpi_get_table(ACPI_SIG_IORT, 0, > + (struct acpi_table_header **)&iort); > + > + if ( ACPI_SUCCESS(status) ) > + { > + int i; > + struct acpi_iort_node *node, *end; > + node = ACPI_ADD_PTR(struct acpi_iort_node, iort, iort->node_offset); > + end = ACPI_ADD_PTR(struct acpi_iort_node, iort, iort->header.length); > + > + for ( i = 0; i < iort->node_count; i++ ) > + { > + if ( node >= end ) > + break; > + > + switch ( node->type ) > + { > + case ACPI_IORT_NODE_SMMU_V3: > + { > + struct acpi_iort_smmu_v3 *smmu; > + smmu = (struct acpi_iort_smmu_v3 *)node->node_data; > + mfn = paddr_to_pfn(smmu->base_address); > + rc = iomem_deny_access(d, mfn, mfn + PFN_UP(SZ_128K)); > + if ( rc ) > + printk("iomem_deny_access failed for SMMUv3\n"); > + node->type = 0xff; > + break; > + } > + } > + node = ACPI_ADD_PTR(struct acpi_iort_node, node, node->length); > + } > + } > + else > + { > + printk("Failed to get IORT table\n"); > + return -EINVAL; > + } > + > /* Deny MMIO access for GIC regions */ > return gic_iomem_deny_access(d); > }
diff --git a/xen/arch/arm/acpi/domain_build.c b/xen/arch/arm/acpi/domain_build.c index bbdc90f92c..ec0b5b261f 100644 --- a/xen/arch/arm/acpi/domain_build.c +++ b/xen/arch/arm/acpi/domain_build.c @@ -14,6 +14,7 @@ #include <xen/acpi.h> #include <xen/event.h> #include <xen/iocap.h> +#include <xen/sizes.h> #include <xen/device_tree.h> #include <xen/libfdt/libfdt.h> #include <acpi/actables.h> @@ -30,6 +31,7 @@ static int __init acpi_iomem_deny_access(struct domain *d) { acpi_status status; struct acpi_table_spcr *spcr = NULL; + struct acpi_table_iort *iort; unsigned long mfn; int rc; @@ -55,6 +57,44 @@ static int __init acpi_iomem_deny_access(struct domain *d) printk("Failed to get SPCR table, Xen console may be unavailable\n"); } + status = acpi_get_table(ACPI_SIG_IORT, 0, + (struct acpi_table_header **)&iort); + + if ( ACPI_SUCCESS(status) ) + { + int i; + struct acpi_iort_node *node, *end; + node = ACPI_ADD_PTR(struct acpi_iort_node, iort, iort->node_offset); + end = ACPI_ADD_PTR(struct acpi_iort_node, iort, iort->header.length); + + for ( i = 0; i < iort->node_count; i++ ) + { + if ( node >= end ) + break; + + switch ( node->type ) + { + case ACPI_IORT_NODE_SMMU_V3: + { + struct acpi_iort_smmu_v3 *smmu; + smmu = (struct acpi_iort_smmu_v3 *)node->node_data; + mfn = paddr_to_pfn(smmu->base_address); + rc = iomem_deny_access(d, mfn, mfn + PFN_UP(SZ_128K)); + if ( rc ) + printk("iomem_deny_access failed for SMMUv3\n"); + node->type = 0xff; + break; + } + } + node = ACPI_ADD_PTR(struct acpi_iort_node, node, node->length); + } + } + else + { + printk("Failed to get IORT table\n"); + return -EINVAL; + } + /* Deny MMIO access for GIC regions */ return gic_iomem_deny_access(d); }
Xen should control the SMMUv3 devices therefore, don't expose the SMMUv3 devices to dom0. Deny iomem access to SMMUv3 address space for dom0 and also make ACPI IORT SMMUv3 node type to 0xff. Signed-off-by: Rahul Singh <rahul.singh@arm.com> --- xen/arch/arm/acpi/domain_build.c | 40 ++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+)