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