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

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

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@xxxxxxx>
> 
> This wants an explanation why this is needed.

I'll add an explanation

> 
>> Signed-off-by: Rahul Singh <rahul.singh@xxxxxxx>
> 
> 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