Hi Julien,
> On 27 Apr 2022, at 7:26 pm, Julien Grall <jul...@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.si...@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
