Re: [PATCH] arm/acpi: don't expose the ACPI IORT SMMUv3 entry to dom0

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hi Julien,

> On 27 Apr 2022, at 7:26 pm, Julien Grall <julien@xxxxxxx> 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@xxxxxxx>
>> ---
>> 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