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

[Julien Grall <julien@xxxxxxx>]

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.

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