Re: [PATCH 08/12] xen/arm: if is_domain_direct_mapped use native addresses for GICv2

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 1 May 2020, Julien Grall wrote:
> On 01/05/2020 02:26, Stefano Stabellini wrote:
> > On Wed, 15 Apr 2020, Julien Grall wrote:
> > > On 15/04/2020 02:02, Stefano Stabellini wrote:
> > > > Today we use native addresses to map the GICv2 for Dom0 and fixed
> > > > addresses for DomUs.
> > > > 
> > > > This patch changes the behavior so that native addresses are used for
> > > > any domain that is_domain_direct_mapped.
> > > > 
> > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> > > > ---
> > > >    xen/arch/arm/domain_build.c | 10 +++++++---
> > > >    xen/arch/arm/vgic-v2.c      | 12 ++++++------
> > > >    xen/arch/arm/vgic/vgic-v2.c |  2 +-
> > > >    xen/include/asm-arm/vgic.h  |  1 +
> > > >    4 files changed, 15 insertions(+), 10 deletions(-)
> > > > 
> > > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > > > index 627e0c5e8e..303bee60f6 100644
> > > > --- a/xen/arch/arm/domain_build.c
> > > > +++ b/xen/arch/arm/domain_build.c
> > > > @@ -1643,8 +1643,12 @@ static int __init make_gicv2_domU_node(struct
> > > > kernel_info *kinfo)
> > > >        int res = 0;
> > > >        __be32 reg[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) *
> > > > 2];
> > > >        __be32 *cells;
> > > > +    struct domain *d = kinfo->d;
> > > > +    char buf[38];
> > > >    -    res = fdt_begin_node(fdt,
> > > > "interrupt-controller@"__stringify(GUEST_GICD_BASE));
> > > > +    snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIx64,
> > > > +             d->arch.vgic.dbase);
> > > > +    res = fdt_begin_node(fdt, buf);
> > > >        if ( res )
> > > >            return res;
> > > >    @@ -1666,9 +1670,9 @@ static int __init make_gicv2_domU_node(struct
> > > > kernel_info *kinfo)
> > > >          cells = &reg[0];
> > > >        dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS,
> > > > GUEST_ROOT_SIZE_CELLS,
> > > > -                       GUEST_GICD_BASE, GUEST_GICD_SIZE);
> > > > +                       d->arch.vgic.dbase, GUEST_GICD_SIZE);
> > > >        dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS,
> > > > GUEST_ROOT_SIZE_CELLS,
> > > > -                       GUEST_GICC_BASE, GUEST_GICC_SIZE);
> > > > +                       d->arch.vgic.cbase, GUEST_GICC_SIZE);
> > > 
> > > You can't use the native base address and not the native size.
> > > Particularly,
> > > this is going to screw any GIC using 8KB alias.
> > 
> > I don't follow why it could cause problems with a GIC using the 8KB
> > alias. Maybe an example (even a fake example) would help.
> 
> The GICC interface is composed of the two 4KB pages. In some of the
> implementation, each pages starts at a 64KB aligned address. Each page are
> also aliased every 4KB in the 64KB region.
> 
> For guest, we don't expose the full 128KB region but only part of it (8KB). So
> the guest interface is the same regardless the underlying implementation of
> the GIC.
> 
> vgic.cbase points to the beginning of the first region. So what you are
> mapping is the first 8KB of the first region. The second region is not mapped
> at all.
> 
> As the pages are aliased, the trick we use is to map from vgic.cbase + 60KB
> (vgic_v2.hw.aliased_offset). This means the 2 pages will now be contiguous in
> the guest physical memory.

I understood the issue and I fixed it by applying
vgic_v2.hw.aliased_offset to vbase and cbase. (Although only vbase is
actually necessary as far as I can tell.)