Hi Stewart,
On 04/10/2023 15:55, Stewart Hildebrand wrote:
From: Rahul Singh <[email protected]>
This wants an explanation why this is needed.
Signed-off-by: Rahul Singh <[email protected]>
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
