[v5] SMMU handling for PCIe Passthrough on ARM

[PATCH v5 9/9] xen/arm: Map ITS doorbell register to IOMMU page tables.

Posted by Stewart Hildebrand 2 years, 4 months ago

From: Rahul Singh <rahul.singh@arm.com>

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
v4->v5:
* new patch
---
 xen/arch/arm/vgic-v3-its.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
index 05429030b539..df8f045198a3 100644
--- a/xen/arch/arm/vgic-v3-its.c
+++ b/xen/arch/arm/vgic-v3-its.c
@@ -682,6 +682,18 @@ static int its_handle_mapd(struct virt_its *its, uint64_t *cmdptr)
                                          BIT(size, UL), valid);
         if ( ret && valid )
             return ret;
+
+        if ( is_iommu_enabled(its->d) ) {
+            ret = map_mmio_regions(its->d, gaddr_to_gfn(its->doorbell_address),
+                           PFN_UP(ITS_DOORBELL_OFFSET),
+                           maddr_to_mfn(its->doorbell_address));
+            if ( ret < 0 )
+            {
+                printk(XENLOG_ERR "GICv3: Map ITS translation register d%d failed.\n",
+                        its->d->domain_id);
+                return ret;
+            }
+        }
     }
 
     spin_lock(&its->its_lock);
-- 
2.42.0

Re: [PATCH v5 9/9] xen/arm: Map ITS doorbell register to IOMMU page tables.

Posted by Julien Grall 2 years, 3 months ago

Hi Stewart,

On 04/10/2023 15:55, Stewart Hildebrand wrote:
> From: Rahul Singh <rahul.singh@arm.com>

This wants an explanation why this is needed.

> Signed-off-by: Rahul Singh <rahul.singh@arm.com>

Your signed-off-by is missing.

> ---
> v4->v5:
> * new patch
> ---
>   xen/arch/arm/vgic-v3-its.c | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> index 05429030b539..df8f045198a3 100644
> --- a/xen/arch/arm/vgic-v3-its.c
> +++ b/xen/arch/arm/vgic-v3-its.c
> @@ -682,6 +682,18 @@ static int its_handle_mapd(struct virt_its *its, uint64_t *cmdptr)
>                                            BIT(size, UL), valid);
>           if ( ret && valid )
>               return ret;
> +
> +        if ( is_iommu_enabled(its->d) ) {

Coding style.

> +            ret = map_mmio_regions(its->d, gaddr_to_gfn(its->doorbell_address),
> +                           PFN_UP(ITS_DOORBELL_OFFSET),
> +                           maddr_to_mfn(its->doorbell_address));

A couple of remarks. Firstly, we know the ITS doorbell at domain 
creation. So I think thish should be called from vgic_v3_its_init_virtual().

Regardless that, any code related to device initialization belongs to 
gicv3_its_map_guest_device().

Lastly, I know the IOMMU page-tables and CPU page-tables are currently 
shared. But strictly speaking, map_mmio_regions() is incorrect because 
the doorbell is only meant to be accessible by the device. So this 
should only be mapped in the IOMMU page-tables.

In fact I vaguely recall that on some platforms you may get a lockup if 
the CPU attempts to write to the doorbell. So we may want to unshare 
page-tables in the future.

For now, we want to use the correct interface (iommu_*) and write down 
the potential security impact (so we remember when exposing a virtual 
ITS to  guests).

> +            if ( ret < 0 )
> +            {
> +                printk(XENLOG_ERR "GICv3: Map ITS translation register d%d failed.\n",
> +                        its->d->domain_id);

XENLOG_ERR is not ratelimited and therefore should not be called from 
emulation path. If you want to print an error, then you should use 
XENLOG_G_ERR.

Also, for printing domain, the preferred is to using %pd with the domain 
as argument (here its->d.

But as this is emulation and therefore the current vCPU belongs to 
its->d, you could directly use gprintk(XENLOG_ERR, "...").

Cheers,

-- 
Julien Grall

Re: [PATCH v5 9/9] xen/arm: Map ITS doorbell register to IOMMU page tables.

Posted by Stewart Hildebrand 2 years, 3 months ago

On 10/20/23 14:02, Julien Grall wrote:
> Hi Stewart,
> 
> On 04/10/2023 15:55, Stewart Hildebrand wrote:
>> From: Rahul Singh <rahul.singh@arm.com>
> 
> This wants an explanation why this is needed.

I'll add an explanation

> 
>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> 
> Your signed-off-by is missing.

I'll add it

> 
>> ---
>> v4->v5:
>> * new patch
>> ---
>>   xen/arch/arm/vgic-v3-its.c | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
>> index 05429030b539..df8f045198a3 100644
>> --- a/xen/arch/arm/vgic-v3-its.c
>> +++ b/xen/arch/arm/vgic-v3-its.c
>> @@ -682,6 +682,18 @@ static int its_handle_mapd(struct virt_its *its, uint64_t *cmdptr)
>>                                            BIT(size, UL), valid);
>>           if ( ret && valid )
>>               return ret;
>> +
>> +        if ( is_iommu_enabled(its->d) ) {
> 
> Coding style.

I'll fix

> 
>> +            ret = map_mmio_regions(its->d, gaddr_to_gfn(its->doorbell_address),
>> +                           PFN_UP(ITS_DOORBELL_OFFSET),
>> +                           maddr_to_mfn(its->doorbell_address));
> 
> 
> A couple of remarks. Firstly, we know the ITS doorbell at domain
> creation. So I think thish should be called from vgic_v3_its_init_virtual().
> 
> Regardless that, any code related to device initialization belongs to
> gicv3_its_map_guest_device().

How about calling it from a map_hwdom_extra_mappings() hook?
For example see [1].

[1] https://gitlab.com/xen-project/people/bmarquis/xen-arm-poc/-/commit/6232a0d53377009bb7fbc3c3ab81d0153734be6b

> 
> Lastly, I know the IOMMU page-tables and CPU page-tables are currently
> shared. But strictly speaking, map_mmio_regions() is incorrect because
> the doorbell is only meant to be accessible by the device. So this
> should only be mapped in the IOMMU page-tables.
> 
> In fact I vaguely recall that on some platforms you may get a lockup if
> the CPU attempts to write to the doorbell. So we may want to unshare
> page-tables in the future.
> 
> For now, we want to use the correct interface (iommu_*) and write down
> the potential security impact (so we remember when exposing a virtual
> ITS to  guests).

OK, I will use iommu_map()

> 
>> +            if ( ret < 0 )
>> +            {
>> +                printk(XENLOG_ERR "GICv3: Map ITS translation register d%d failed.\n",
>> +                        its->d->domain_id);
> 
> XENLOG_ERR is not ratelimited and therefore should not be called from
> emulation path. If you want to print an error, then you should use
> XENLOG_G_ERR.
> 
> Also, for printing domain, the preferred is to using %pd with the domain
> as argument (here its->d.

I'll fix it

> 
> But as this is emulation and therefore the current vCPU belongs to
> its->d, you could directly use gprintk(XENLOG_ERR, "...").

Will do

> 
> Cheers,
> 
> -- 
> Julien Grall

Re: [PATCH v5 9/9] xen/arm: Map ITS doorbell register to IOMMU page tables.

Posted by Julien Grall 2 years, 3 months ago

Hi,

On 07/11/2023 20:10, Stewart Hildebrand wrote:
>>> +            ret = map_mmio_regions(its->d, gaddr_to_gfn(its->doorbell_address),
>>> +                           PFN_UP(ITS_DOORBELL_OFFSET),
>>> +                           maddr_to_mfn(its->doorbell_address));
>>
>>
>> A couple of remarks. Firstly, we know the ITS doorbell at domain
>> creation. So I think thish should be called from vgic_v3_its_init_virtual().
>>
>> Regardless that, any code related to device initialization belongs to
>> gicv3_its_map_guest_device().
> 
> How about calling it from a map_hwdom_extra_mappings() hook?
> For example see [1].

I don't think this is the correct place because sooner or later you will 
need the same mapping when enable the ITS for domUs. So I think the 
mapping should be part of vgic_v3_its_init_virtual().

Cheers,

-- 
Julien Grall