Hi Michal,
> -----Original Message-----
> From: Michal Orzel <[email protected]>
> 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
