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

[Julien Grall <julien@xen.org>]

Hi Henry,

On 30/01/2023 04:06, Henry Wang wrote:
> Currently, the mapping of the GICv2 CPU interface is created in
> arch_domain_create(). This causes some troubles in populating and
> freeing of the domain P2M pages pool. For example, a default 16
> P2M pages are required in p2m_init() to cope with the P2M mapping
> of 8KB GICv2 CPU interface area, and these 16 P2M pages would cause
> the complexity of P2M destroy in the failure path of
> arch_domain_create().
> 
> As per discussion in [1], similarly as the MMIO access for ACPI, this
> patch defers the GICv2 CPU interface mapping until the first MMIO
> access. This is achieved by moving the GICv2 CPU interface mapping
> code from vgic_v2_domain_init()/vgic_v2_map_resources() to the
> stage-2 data abort trap handling code. The original CPU interface
> size and virtual CPU interface base address is now saved in
> `struct vgic_dist` instead of the local variable of
> vgic_v2_domain_init()/vgic_v2_map_resources().
> 
> Take the opportunity to unify the way of data access using the
> existing pointer to struct vgic_dist in vgic_v2_map_resources() for
> new GICv2.
> 
> 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.

>   #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..9dd703f661 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,15 @@ 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
> +     * 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.

Cheers,

-- 
Julien Grall