[PATCH 08/11] xen/arm: if 1:1 direct-map domain use native addresses for GICv3

Penny Zheng posted 11 patches 3 years, 2 months ago
[PATCH 08/11] xen/arm: if 1:1 direct-map domain use native addresses for GICv3
Posted by Penny Zheng 3 years, 2 months ago
From: Stefano Stabellini <stefano.stabellini@xilinx.com>

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
all domains that are 1:1 direct-map(including Dom0).

Update the rdist accessor to use the struct vgic variables to provide
the updated addresses.

Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
 xen/arch/arm/domain_build.c | 32 ++++++++++++++++++++++++--------
 xen/arch/arm/vgic-v3.c      | 10 +++++-----
 xen/include/asm-arm/vgic.h  |  6 +++---
 3 files changed, 32 insertions(+), 16 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index d5f201f73e..120f1ae575 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1854,10 +1854,11 @@ static int __init make_gicv3_domU_node(struct kernel_info *kinfo)
 {
     void *fdt = kinfo->fdt;
     int res = 0;
-    __be32 reg[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) * 2];
+    __be32 *reg;
     __be32 *cells;
     struct domain *d = kinfo->d;
     char buf[38];
+    unsigned int i, len = 0;
 
     snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIx64,
              vgic_dist_base(&d->arch.vgic));
@@ -1881,27 +1882,42 @@ static int __init make_gicv3_domU_node(struct kernel_info *kinfo)
     if ( res )
         return res;
 
+    /* reg specifies all re-distributors and Distributor. */
+    len = (GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) *
+          (vgic_rdist_nr(&d->arch.vgic) + 1) * sizeof(__be32);
+    reg = xmalloc_bytes(len);
+    if ( reg == NULL )
+        return -ENOMEM;
+
     cells = &reg[0];
     dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
                        vgic_dist_base(&d->arch.vgic), GUEST_GICV3_GICD_SIZE);
-    dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
-                       vgic_rdist_base(&d->arch.vgic, 0),
-                       vgic_rdist_size(&d->arch.vgic, 0));
+    for ( i = 0;
+          i < vgic_rdist_nr(&d->arch.vgic);
+          i++, cells += (GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) )
+    {
+        dt_child_set_range(&cells,
+                           GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
+                           vgic_rdist_base(&d->arch.vgic, i),
+                           vgic_rdist_size(&d->arch.vgic, i));
+    }
 
-    res = fdt_property(fdt, "reg", reg, sizeof(reg));
+    res = fdt_property(fdt, "reg", reg, len);
     if (res)
-        return res;
+        goto out;
 
     res = fdt_property_cell(fdt, "linux,phandle", kinfo->phandle_gic);
     if (res)
-        return res;
+        goto out;
 
     res = fdt_property_cell(fdt, "phandle", kinfo->phandle_gic);
     if (res)
-        return res;
+        goto out;
 
     res = fdt_end_node(fdt);
 
+ out:
+    xfree(reg);
     return res;
 }
 
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index cb5a70c42e..83f996b46c 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -1647,8 +1647,8 @@ static inline unsigned int vgic_v3_max_rdist_count(struct domain *d)
      * However DomU get a constructed memory map, so we can go with
      * the architected single redistributor region.
      */
-    return is_hardware_domain(d) ? vgic_v3_hw.nr_rdist_regions :
-               GUEST_GICV3_RDIST_REGIONS;
+    return is_domain_direct_mapped(d) ? vgic_v3_hw.nr_rdist_regions :
+                                        GUEST_GICV3_RDIST_REGIONS;
 }
 
 static int vgic_v3_domain_init(struct domain *d)
@@ -1670,10 +1670,10 @@ static int vgic_v3_domain_init(struct domain *d)
     radix_tree_init(&d->arch.vgic.pend_lpi_tree);
 
     /*
-     * Domain 0 gets the hardware address.
-     * Guests get the virtual platform layout.
+     * 1:1 direct-map domain (including Dom0) gets the hardware address.
+     * Other guests get the virtual platform layout.
      */
-    if ( is_hardware_domain(d) )
+    if ( is_domain_direct_mapped(d) )
     {
         unsigned int first_cpu = 0;
 
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 92f6a2d15d..0f7cb32c58 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -291,17 +291,17 @@ static inline paddr_t vgic_dist_base(struct vgic_dist *vgic)
 #ifdef CONFIG_GICV3
 static inline unsigned int vgic_rdist_nr(struct vgic_dist *vgic)
 {
-    return GUEST_GICV3_RDIST_REGIONS;
+    return vgic->nr_regions;
 }
 
 static inline paddr_t vgic_rdist_base(struct vgic_dist *vgic, unsigned int i)
 {
-    return GUEST_GICV3_GICR0_BASE;
+    return vgic->rdist_regions[i].base;
 }
 
 static inline paddr_t vgic_rdist_size(struct vgic_dist *vgic, unsigned int i)
 {
-    return GUEST_GICV3_GICR0_SIZE;
+    return vgic->rdist_regions[i].size;
 }
 #else
 static inline unsigned int vgic_rdist_nr(struct vgic_dist *vgic)
-- 
2.25.1


Re: [PATCH 08/11] xen/arm: if 1:1 direct-map domain use native addresses for GICv3
Posted by Julien Grall 3 years, 2 months ago
Hi,

On 23/09/2021 08:11, Penny Zheng wrote:
> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
> 
> 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
> all domains that are 1:1 direct-map(including Dom0).
> 
> Update the rdist accessor to use the struct vgic variables to provide
> the updated addresses.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
>   xen/arch/arm/domain_build.c | 32 ++++++++++++++++++++++++--------
>   xen/arch/arm/vgic-v3.c      | 10 +++++-----
>   xen/include/asm-arm/vgic.h  |  6 +++---
>   3 files changed, 32 insertions(+), 16 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index d5f201f73e..120f1ae575 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1854,10 +1854,11 @@ static int __init make_gicv3_domU_node(struct kernel_info *kinfo)
>   {
>       void *fdt = kinfo->fdt;
>       int res = 0;
> -    __be32 reg[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) * 2];
> +    __be32 *reg;
>       __be32 *cells;
>       struct domain *d = kinfo->d;
>       char buf[38];
> +    unsigned int i, len = 0;
>   
>       snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIx64,
>                vgic_dist_base(&d->arch.vgic));
> @@ -1881,27 +1882,42 @@ static int __init make_gicv3_domU_node(struct kernel_info *kinfo)
>       if ( res )
>           return res;
>   
> +    /* reg specifies all re-distributors and Distributor. */
> +    len = (GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) *
> +          (vgic_rdist_nr(&d->arch.vgic) + 1) * sizeof(__be32);
> +    reg = xmalloc_bytes(len);
> +    if ( reg == NULL )
> +        return -ENOMEM;
> +
>       cells = &reg[0];
>       dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
>                          vgic_dist_base(&d->arch.vgic), GUEST_GICV3_GICD_SIZE);
> -    dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
> -                       vgic_rdist_base(&d->arch.vgic, 0),
> -                       vgic_rdist_size(&d->arch.vgic, 0));
> +    for ( i = 0;
> +          i < vgic_rdist_nr(&d->arch.vgic);
> +          i++, cells += (GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) )
> +    {
> +        dt_child_set_range(&cells,
> +                           GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
> +                           vgic_rdist_base(&d->arch.vgic, i),
> +                           vgic_rdist_size(&d->arch.vgic, i));
> +    }
>   
> -    res = fdt_property(fdt, "reg", reg, sizeof(reg));
> +    res = fdt_property(fdt, "reg", reg, len);
>       if (res)
> -        return res;
> +        goto out;
>   
>       res = fdt_property_cell(fdt, "linux,phandle", kinfo->phandle_gic);
>       if (res)
> -        return res;
> +        goto out;
>   
>       res = fdt_property_cell(fdt, "phandle", kinfo->phandle_gic);
>       if (res)
> -        return res;
> +        goto out;
>   
>       res = fdt_end_node(fdt);
>   
> + out:
> +    xfree(reg);
>       return res;
>   }
>   
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index cb5a70c42e..83f996b46c 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -1647,8 +1647,8 @@ static inline unsigned int vgic_v3_max_rdist_count(struct domain *d)
>        * However DomU get a constructed memory map, so we can go with
>        * the architected single redistributor region.
>        */

The comment needs to be updated.

> -    return is_hardware_domain(d) ? vgic_v3_hw.nr_rdist_regions :
> -               GUEST_GICV3_RDIST_REGIONS;
> +    return is_domain_direct_mapped(d) ? vgic_v3_hw.nr_rdist_regions :
> +                                        GUEST_GICV3_RDIST_REGIONS;

Let's avoid to make the assumption that dom0 is always direct mapped. So 
this should be "is_domain_direct_mapped(d) || is_hardware_domain(d)".

I would actually suggest to introduce a new helper 
"is_domain_use_host_layout()" (or similar) that wraps the two check. 
This would make the code a bit more flexible.

>   }
>   
>   static int vgic_v3_domain_init(struct domain *d)
> @@ -1670,10 +1670,10 @@ static int vgic_v3_domain_init(struct domain *d)
>       radix_tree_init(&d->arch.vgic.pend_lpi_tree);
>   
>       /*
> -     * Domain 0 gets the hardware address.
> -     * Guests get the virtual platform layout.
> +     * 1:1 direct-map domain (including Dom0) gets the hardware address.
> +     * Other guests get the virtual platform layout.
>        */
> -    if ( is_hardware_domain(d) )
> +    if ( is_domain_direct_mapped(d) )

Same here.

>       {
>           unsigned int first_cpu = 0;
>   
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index 92f6a2d15d..0f7cb32c58 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -291,17 +291,17 @@ static inline paddr_t vgic_dist_base(struct vgic_dist *vgic)
>   #ifdef CONFIG_GICV3
>   static inline unsigned int vgic_rdist_nr(struct vgic_dist *vgic)
>   {
> -    return GUEST_GICV3_RDIST_REGIONS;
> +    return vgic->nr_regions;
>   }
>   
>   static inline paddr_t vgic_rdist_base(struct vgic_dist *vgic, unsigned int i)
>   {
> -    return GUEST_GICV3_GICR0_BASE;
> +    return vgic->rdist_regions[i].base;
>   }
>   
>   static inline paddr_t vgic_rdist_size(struct vgic_dist *vgic, unsigned int i)
>   {
> -    return GUEST_GICV3_GICR0_SIZE;
> +    return vgic->rdist_regions[i].size;
>   }

I think this change in vgic.h should have belong to the patch 
introducing the helpers (if we still plan to use them).

>   #else
>   static inline unsigned int vgic_rdist_nr(struct vgic_dist *vgic)
> 

Cheers,

-- 
Julien Grall