Re: [PATCH v2 3/4] xen/arm: Defer GICv2 CPU interface mapping until the first access

[Julien Grall <julien@xxxxxxx>]

On 21/03/2023 03:57, Henry Wang wrote:
> Hi Julien,
Hi Henry,

> > -----Original Message-----
> > From: Julien Grall <julien@xxxxxxx>
> > Subject: Re: [PATCH v2 3/4] xen/arm: Defer GICv2 CPU interface mapping until
> > the first access
> >
> > Hi Henry,
> >
> > On 30/01/2023 04:06, Henry Wang wrote:
> > > Since the hardware domain (Dom0) has an unlimited size P2M pool, the
> > > gicv2_map_hwdom_extra_mappings() is also not touched by this patch.
> > I didn't notice this in the previous version. The fact that dom0 has
> > unlimited size P2M pool doesn't matter here (this could also change in
> > the future). Even if the P2M pool was limited, then it would be fine
> > because the extra mappings happen after domain_create(). So there is no
> > need to map them on-demand as the code could be order the way we want.
> >
> > So this paragraph needs to be reworded.
> Sure, I've reworded this paragraph to below:
> "Since gicv2_map_hwdom_extra_mappings() happens after domain_create(),
> so there is no need to map the extra mappings on-demand, and therefore
> keep the hwdom extra mappings as untouched."
Looks good to me.

> > > +    /*
> > > +     * Map the GICv2 virtual CPU interface in the GIC CPU interface
> > > +     * region of the guest on the first access of the MMIO region.
> > > +     */
> > > +    if ( d->arch.vgic.version == GIC_V2 &&
> > > +         gfn_eq(gfn, gaddr_to_gfn(d->arch.vgic.cbase)) )
> > The CPU interface size is 8KB (bigger in some cases) but here you only
> > check for the access to be in the first 4KB.
> Yeah indeed, gfn might fall into the range between 4KB and 8KB, sorry
> about that.
>
> Considering that the CPU interface is continuous (I suppose), I have two
> ways of rewriting the gfn check, we can do either:
>
> gfn_eq(gfn, gaddr_to_gfn(d->arch.vgic.cbase)) ||
> gfn_eq(gfn, gfn_add(gaddr_to_gfn(d->arch.vgic.cbase), 1))
This check is incorrect as you are making the assumption that the size 
is 8KB. As I hinted in the previous e-mail, the size could be bigger. 
For instance, for dom0, it could be up to 128KB when the GICC is aliased.
So you want to...

> or
>
> gfn_to_gaddr(gfn) >= d->arch.vgic.cbase ||
> gfn_to_gaddr(gfn) < d->arch.vgic.cbase + d->arch.vgic.csize
... use the size in the check. With the || switch to &&, my preference 
would be to use:
((d->arch.vgic.cbase - gfn_to_addr(gfn)) < d->arch.vgic.csize)

to avoid a potential overflow in the unlikely case the CPU interface is 
at the top of the address space.
Cheers,

--
Julien Grall