Re: [PATCH 09/12] xen/arm: if is_domain_direct_mapped use native addresses for GICv3

[Julien Grall <julien@xxxxxxx>]

Hi,

On 15/04/2020 02:02, Stefano Stabellini wrote:
> Today we use native addresses to map the GICv3 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. The patch has to introduce one
> #ifndef CONFIG_NEW_VGIC because the new vgic doesn't support GICv3.
Please no #ifdef. Instead move the creation of the DT node in vgic.c and 
introduce a stub for non-GICv3 platform.
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> ---
>   xen/arch/arm/domain_build.c | 12 +++++++++---
>   xen/arch/arm/vgic-v3.c      | 18 ++++++++++++++----
>   2 files changed, 23 insertions(+), 7 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 303bee60f6..beec0a144c 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1697,8 +1697,12 @@ static int __init make_gicv3_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_GICV3_GICD_BASE));
> +    snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIx64,
> +             d->arch.vgic.dbase);
> +    res = fdt_begin_node(fdt, buf);
>       if ( res )
>           return res;
>   
> @@ -1720,9 +1724,11 @@ static int __init make_gicv3_domU_node(struct kernel_info *kinfo)
>   
>       cells = &reg[0];
>       dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, 
> GUEST_ROOT_SIZE_CELLS,
> -                       GUEST_GICV3_GICD_BASE, GUEST_GICV3_GICD_SIZE);
> +                       d->arch.vgic.dbase, GUEST_GICV3_GICD_SIZE);
> +#if defined(CONFIG_GICV3) && !defined(CONFIG_NEW_VGIC)
CONFIG_GICV3 does not exist. Did you mean CONFIG_HAS_GICV3?

>       dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, 
> GUEST_ROOT_SIZE_CELLS,
> -                       GUEST_GICV3_GICR0_BASE, GUEST_GICV3_GICR0_SIZE);
> +                       d->arch.vgic.rdist_regions[0].base, 
> GUEST_GICV3_GICR0_SIZE);
> +#endif
>   
>       res = fdt_property(fdt, "reg", reg, sizeof(reg));
>       if (res)
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index 4e60ba15cc..4cf430f865 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -1677,13 +1677,25 @@ static int vgic_v3_domain_init(struct domain *d)

I think you also want to modify vgic_v3_max_rdist_count().

>        * Domain 0 gets the hardware address.
>        * Guests get the virtual platform layout.
This comment needs to be updated.

>        */
> -    if ( is_hardware_domain(d) )
> +    if ( is_domain_direct_mapped(d) )
>       {
>           unsigned int first_cpu = 0;
> +        unsigned int nr_rdist_regions;
>   
>           d->arch.vgic.dbase = vgic_v3_hw.dbase;
>   
> -        for ( i = 0; i < vgic_v3_hw.nr_rdist_regions; i++ )
> +        if ( is_hardware_domain(d) )
> +        {
> +            nr_rdist_regions = vgic_v3_hw.nr_rdist_regions;
> +            d->arch.vgic.intid_bits = vgic_v3_hw.intid_bits;
> +        }
> +        else
> +        {
> +            nr_rdist_regions = 1;
What does promise your the rdist region will be big enough to cater all 
the re-distributors for your domain?
Cheers,

--
Julien Grall