RE: [PATCH 2/3] xen/arm: Defer GICv2 CPU interface mapping until the first access

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Michal,

> -----Original Message-----
> From: Michal Orzel <michal.orzel@xxxxxxx>
> Subject: Re: [PATCH 2/3] xen/arm: Defer GICv2 CPU interface mapping until
> the first access
> 
> Hi Henry,
> 
> On 16/01/2023 02:58, Henry Wang wrote:
> > Note that GICv2 changes introduced by this patch is not applied to the
> > "New vGIC" implementation, as the "New vGIC" is not used. Also since
> The fact that "New vGIC" is not used very often and its work is in-progress
> does not mean that we can implement changes resulting in a build failure
> when enabling CONFIG_NEW_VGIC, which is the case with your patch.
> So this needs to be fixed.

I will add the "New vGIC" changes in v2.

> 
> > @@ -153,6 +153,8 @@ struct vgic_dist {
> >      /* Base address for guest GIC */
> >      paddr_t dbase; /* Distributor base address */
> >      paddr_t cbase; /* CPU interface base address */
> > +    paddr_t csize; /* CPU interface size */
> > +    paddr_t vbase; /* virtual CPU interface base address */
> Could you swap them so that base address variables are grouped?

Sure, my original thought was grouping the CPU interface related fields but
since you prefer grouping the base address, I will follow your suggestion.

> 
> >  #ifdef CONFIG_GICV3
> >      /* GIC V3 addressing */
> >      /* List of contiguous occupied by the redistributors */
> > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> > index 061c92acbd..d98f166050 100644
> > --- a/xen/arch/arm/traps.c
> > +++ b/xen/arch/arm/traps.c
> > @@ -1787,9 +1787,12 @@ static inline bool hpfar_is_valid(bool s1ptw,
> uint8_t fsc)
> >  }
> >
> >  /*
> > - * When using ACPI, most of the MMIO regions will be mapped on-
> demand
> > - * in stage-2 page tables for the hardware domain because Xen is not
> > - * able to know from the EFI memory map the MMIO regions.
> > + * Try to map the MMIO regions for some special cases:
> > + * 1. When using ACPI, most of the MMIO regions will be mapped on-
> demand
> > + *    in stage-2 page tables for the hardware domain because Xen is not
> > + *    able to know from the EFI memory map the MMIO regions.
> > + * 2. For guests using GICv2, the GICv2 CPU interface mapping is created
> > + *    on the first access of the MMIO region.
> >   */
> >  static bool try_map_mmio(gfn_t gfn)
> >  {
> > @@ -1798,6 +1801,16 @@ static bool try_map_mmio(gfn_t gfn)
> >      /* For the hardware domain, all MMIOs are mapped with GFN == MFN
> */
> >      mfn_t mfn = _mfn(gfn_x(gfn));
> >
> > +    /*
> > +     * Map the GICv2 virtual cpu interface in the gic cpu interface
> NIT: in all the other places you use CPU (capital letters)

Oh good catch, thank you. I think this part is the same as the original in-code
comment, but since I am touching this part anyway, it would be good to
correct them.

> 
> > +     * region of the guest on the first access of the MMIO region.
> > +     */
> > +    if ( d->arch.vgic.version == GIC_V2 &&
> > +         gfn_x(gfn) == gfn_x(gaddr_to_gfn(d->arch.vgic.cbase)) )
> There is a macro gnf_eq that you can use to avoid opencoding it.

Thanks! I will fix in v2.

> 
> > +        return !map_mmio_regions(d, gaddr_to_gfn(d->arch.vgic.cbase),
> At this point you can use just gfn instead of gaddr_to_gfn(d->arch.vgic.cbase)

Will fix in v2.

> 
> > +                                 d->arch.vgic.csize / PAGE_SIZE,
> > +                                 maddr_to_mfn(d->arch.vgic.vbase));
> > +
> >      /*
> >       * Device-Tree should already have everything mapped when building
> >       * the hardware domain.
> > diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> > index 0026cb4360..21e14a5a6f 100644
> > --- a/xen/arch/arm/vgic-v2.c
> > +++ b/xen/arch/arm/vgic-v2.c
> > @@ -644,10 +644,6 @@ static int vgic_v2_vcpu_init(struct vcpu *v)
> >
> >  static int vgic_v2_domain_init(struct domain *d)
> >  {
> > -    int ret;
> > -    paddr_t csize;
> > -    paddr_t vbase;
> > -
> >      /*
> >       * The hardware domain and direct-mapped domain both get the
> hardware
> >       * address.
> > @@ -667,8 +663,8 @@ static int vgic_v2_domain_init(struct domain *d)
> >           * aligned to PAGE_SIZE.
> >           */
> >          d->arch.vgic.cbase = vgic_v2_hw.cbase;
> > -        csize = vgic_v2_hw.csize;
> > -        vbase = vgic_v2_hw.vbase;
> > +        d->arch.vgic.csize = vgic_v2_hw.csize;
> > +        d->arch.vgic.vbase = vgic_v2_hw.vbase;
> >      }
> >      else if ( is_domain_direct_mapped(d) )
> >      {
> > @@ -683,8 +679,8 @@ static int vgic_v2_domain_init(struct domain *d)
> >           */
> >          d->arch.vgic.dbase = vgic_v2_hw.dbase;
> >          d->arch.vgic.cbase = vgic_v2_hw.cbase;
> > -        csize = GUEST_GICC_SIZE;
> > -        vbase = vgic_v2_hw.vbase + vgic_v2_hw.aliased_offset;
> > +        d->arch.vgic.csize = GUEST_GICC_SIZE;
> > +        d->arch.vgic.vbase = vgic_v2_hw.vbase + vgic_v2_hw.aliased_offset;
> >      }
> >      else
> >      {
> > @@ -697,19 +693,10 @@ static int vgic_v2_domain_init(struct domain *d)
> >           */
> >          BUILD_BUG_ON(GUEST_GICC_SIZE != SZ_8K);
> >          d->arch.vgic.cbase = GUEST_GICC_BASE;
> > -        csize = GUEST_GICC_SIZE;
> > -        vbase = vgic_v2_hw.vbase + vgic_v2_hw.aliased_offset;
> > +        d->arch.vgic.csize = GUEST_GICC_SIZE;
> > +        d->arch.vgic.vbase = vgic_v2_hw.vbase + vgic_v2_hw.aliased_offset;
> >      }
> >
> > -    /*
> > -     * Map the gic virtual cpu interface in the gic cpu interface
> > -     * region of the guest.
> > -     */
> > -    ret = map_mmio_regions(d, gaddr_to_gfn(d->arch.vgic.cbase),
> > -                           csize / PAGE_SIZE, maddr_to_mfn(vbase));
> > -    if ( ret )
> > -        return ret;
> > -
> Maybe adding some comment like "Mapping of the virtual CPU interface is
> deferred until first access"
> would be helpful.

Sure, I will add the comment in v2.

Kind regards,
Henry

> 
> ~Michal