Re: [PATCH 07/11] xen/arm: if 1:1 direct-map domain use native addresses for GICv2

[Julien Grall <julien@xxxxxxx>]

Hi,

On 23/09/2021 08:11, Penny Zheng wrote:
> From: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
>
> 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
> all domains that are 1:1 direct-map(including Dom0).
>
> Update the accessors to use the struct vgic variables to provide the
> updated addresses.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> ---
>   xen/arch/arm/vgic-v2.c         | 16 ++++++++++++++++
>   xen/arch/arm/vgic/vgic-v2.c    | 17 +++++++++++++++++
>   xen/include/asm-arm/new_vgic.h |  4 ++--
>   xen/include/asm-arm/vgic.h     |  4 ++--
>   4 files changed, 37 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> index b34ec88106..a8cf8173d0 100644
> --- a/xen/arch/arm/vgic-v2.c
> +++ b/xen/arch/arm/vgic-v2.c
> @@ -673,6 +673,22 @@ static int vgic_v2_domain_init(struct domain *d)
>           csize = vgic_v2_hw.csize;
>           vbase = vgic_v2_hw.vbase;
>       }
> +    else if ( is_domain_direct_mapped(d) )
> +    {
> +        /*
> +         * For non-dom0 direct_mapped guests we only map a 8kB CPU
> +         * interface but we make sure it is at a location occupied by
> +         * the physical GIC in the host device tree.
> +         *
> +         * We need to add an offset to the virtual CPU interface base
> +         * address when the GIC is aliased to get a 8kB contiguous
> +         * region.
> +         */
So I agree that we need to differentiate between dom0 and other direct 
mapped domains. However, I think it would be good to explainm why given 
that in other places you treat dom0 and direct mapped domain the same way.
> +        d->arch.vgic.dbase = vgic_v2_hw.dbase;
> +        d->arch.vgic.cbase = vgic_v2_hw.cbase + vgic_v2_hw.aliased_offset;
> +        csize = GUEST_GICC_SIZE;
> +        vbase = vgic_v2_hw.vbase + vgic_v2_hw.aliased_offset;
> +    }
>       else
>       {
>           d->arch.vgic.dbase = GUEST_GICD_BASE;
> diff --git a/xen/arch/arm/vgic/vgic-v2.c b/xen/arch/arm/vgic/vgic-v2.c
> index fd452fcb5a..ce1f6e4373 100644
> --- a/xen/arch/arm/vgic/vgic-v2.c
> +++ b/xen/arch/arm/vgic/vgic-v2.c
> @@ -280,6 +280,23 @@ int vgic_v2_map_resources(struct domain *d)
>           csize = gic_v2_hw_data.csize;
>           vbase = gic_v2_hw_data.vbase;
>       }
> +    else if ( is_domain_direct_mapped(d) )
> +    {
> +        d->arch.vgic.vgic_dist_base = gic_v2_hw_data.dbase;
> +        /*
> +         * For non-dom0 direct_mapped guests we only map a 8kB CPU
> +         * interface but we make sure it is at a location occupied by
> +         * the physical GIC in the host device tree.
> +         *
> +         * We need to add an offset to the virtual CPU interface base
> +         * address when the GIC is aliased to get a 8kB contiguous
> +         * region.
> +         */
> +        d->arch.vgic.vgic_cpu_base = gic_v2_hw_data.cbase +
> +                                     gic_v2_hw_data.aliased_offset;
> +        csize = GUEST_GICC_SIZE;
> +        vbase = gic_v2_hw_data.vbase + gic_v2_hw_data.aliased_offset;
> +    }
>       else
>       {
>           d->arch.vgic.vgic_dist_base = GUEST_GICD_BASE;
> diff --git a/xen/include/asm-arm/new_vgic.h b/xen/include/asm-arm/new_vgic.h
> index 9097522b27..0d16f878eb 100644
> --- a/xen/include/asm-arm/new_vgic.h
> +++ b/xen/include/asm-arm/new_vgic.h
> @@ -188,12 +188,12 @@ struct vgic_cpu {
>   
>   static inline paddr_t vgic_cpu_base(struct vgic_dist *vgic)
>   {
> -    return GUEST_GICC_BASE;
> +    return vgic->vgic_cpu_base;
>   }
>   
>   static inline paddr_t vgic_dist_base(struct vgic_dist *vgic)
>   {
> -    return GUEST_GICD_BASE;
> +    return vgic->vgic_dist_base;
>   }
>   
>   static inline unsigned int vgic_rdist_nr(struct vgic_dist *vgic)
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index d5ad1f387b..92f6a2d15d 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -280,12 +280,12 @@ enum gic_sgi_mode;
>   
>   static inline paddr_t vgic_cpu_base(struct vgic_dist *vgic)
>   {
> -    return GUEST_GICC_BASE;
> +    return vgic->cbase;
>   }
>   
>   static inline paddr_t vgic_dist_base(struct vgic_dist *vgic)
>   {
> -    return GUEST_GICD_BASE;
> +    return vgic->dbase;
>   }
>   
>   #ifdef CONFIG_GICV3 >
Cheers,

--
Julien Grall