[PATCH v2 0/2] xen/arm: vgic-v3: bug fixes

[PATCH v2 0/2] xen/arm: vgic-v3: bug fixes

[Julien Grall]

Hi all,

This series is meant to address Dom0 boot failure when running on GICv4
platforms as well as when the number of vCPUs is not equal to the numbers of
pCPUs. They should be backported up Xen 4.8.

Cheers,

Julien Grall (2):
  xen/arm: vgic-v3: Delay the initialization of the domain information
  xen/arm: vgic-v3: Don't create empty re-distributor regions

 xen/arch/arm/gic-v3.c  | 14 +++++++++++---
 xen/arch/arm/vgic-v3.c | 50 +++++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 56 insertions(+), 8 deletions(-)

-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 1/2] xen/arm: vgic-v3: Delay the initialization of the domain information

[Julien Grall]

A follow-up patch will require to know the number of vCPUs when
initializating the vGICv3 domain structure. However this information is
not available at domain creation. This is only known once
XEN_DOMCTL_max_vpus is called for that domain.

In order to get the max vCPUs around, delay the domain part of the vGIC
v3 initialization until the first vCPU of the domain is initialized.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Tested-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
Acked-but-disliked-by: Stefano Stabellini <sstabellini@kernel.org>

---

Cc: Andrew Cooper <andrew.cooper3@citrix.com>

This is nasty but I can't find a better way for Xen 4.11 and older. We
still need it for unstable because the number of vCPUs is not known in
arch_domain_init. There are discussion to rework the domain creation a
bit further but I would hope to fix the bug first.

Andrew, I have CCed you to know whether you have a better idea where to
place this call on Xen 4.11 and older.

    Changes in v2:
        - The patch is also needed for the time being on unstable
        - Add Stefano's recently invented tag
        - Add Shameer's tested tag
---
 xen/arch/arm/vgic-v3.c | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 4b42739a52..df1bab3a35 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -1573,9 +1573,11 @@ static const struct mmio_handler_ops vgic_distr_mmio_handler = {
     .write = vgic_v3_distr_mmio_write,
 };
 
+static int vgic_v3_real_domain_init(struct domain *d);
+
 static int vgic_v3_vcpu_init(struct vcpu *v)
 {
-    int i;
+    int i, rc;
     paddr_t rdist_base;
     struct vgic_rdist_region *region;
     unsigned int last_cpu;
@@ -1584,6 +1586,19 @@ static int vgic_v3_vcpu_init(struct vcpu *v)
     struct domain *d = v->domain;
 
     /*
+     * This is the earliest place where the number of vCPUs is
+     * known. This is required to initialize correctly the vGIC v3
+     * domain structure. We only to do that when vCPU 0 is
+     * initilialized.
+     */
+    if ( v->vcpu_id == 0 )
+    {
+        rc = vgic_v3_real_domain_init(d);
+        if ( rc )
+            return rc;
+    }
+
+    /*
      * Find the region where the re-distributor lives. For this purpose,
      * we look one region ahead as we have only the first CPU in hand.
      */
@@ -1641,7 +1656,7 @@ static inline unsigned int vgic_v3_rdist_count(struct domain *d)
                GUEST_GICV3_RDIST_REGIONS;
 }
 
-static int vgic_v3_domain_init(struct domain *d)
+static int vgic_v3_real_domain_init(struct domain *d)
 {
     struct vgic_rdist_region *rdist_regions;
     int rdist_count, i, ret;
@@ -1733,6 +1748,16 @@ static int vgic_v3_domain_init(struct domain *d)
     return 0;
 }
 
+static int vgic_v3_domain_init(struct domain *d)
+{
+    /*
+     * The domain initialization for vGIC v3 is delayed until the first vCPU
+     * is created. This because the initialization may require to know the
+     * number of vCPUs that is not known when creating the domain.
+     */
+    return 0;
+}
+
 static void vgic_v3_domain_free(struct domain *d)
 {
     vgic_v3_its_free_domain(d);
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 2/2] xen/arm: vgic-v3: Don't create empty re-distributor regions

[Julien Grall]

At the moment, Xen is assuming the hardware domain will have the same
number of re-distributor regions as the host. However, as the
number of CPUs or the stride (e.g on GICv4) may be different we end up
exposing regions which does not contain any re-distributors.

When booting, Linux will go through all the re-distributor region to
check whether a property (e.g vPLIs) is available accross all the
re-distributors. This will result to a data abort on empty regions
because there are no underlying re-distributor.

So we need to limit the number of regions exposed to the hardware
domain. The code reworked to only expose the minimun number of regions
required by the hardware domain. It is assumed the regions will be
populated starting from the first one.

Lastly, rename vgic_v3_rdist_count to reflect the value return by the
helper.

Reported-by: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
Signed-off-by: Julien Grall <julien.grall@arm.com>
Tested-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

---
    Changes in v2:
        - Rename vgic_v3_rdist_count to vgic_v3_max_rdist_count
        - Fixup #re-distributors
        - Fix typoes
        - Add Shameer's tested tag
---
 xen/arch/arm/gic-v3.c  | 14 +++++++++++---
 xen/arch/arm/vgic-v3.c | 21 ++++++++++++++++++---
 2 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index c98a163ee7..2c1454f425 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1265,7 +1265,8 @@ static int gicv3_make_hwdom_dt_node(const struct domain *d,
     if ( res )
         return res;
 
-    res = fdt_property_cell(fdt, "#redistributor-regions", gicv3.rdist_count);
+    res = fdt_property_cell(fdt, "#redistributor-regions",
+                            d->arch.vgic.nr_regions);
     if ( res )
         return res;
 
@@ -1274,8 +1275,10 @@ static int gicv3_make_hwdom_dt_node(const struct domain *d,
      * GIC has two memory regions: Distributor + rdist regions
      * CPU interface and virtual cpu interfaces accessesed as System registers
      * So cells are created only for Distributor and rdist regions
+     * The hardware domain may not use all the regions. So only copy
+     * what is necessary.
      */
-    new_len = new_len * (gicv3.rdist_count + 1);
+    new_len = new_len * (d->arch.vgic.nr_regions + 1);
 
     hw_reg = dt_get_property(gic, "reg", &len);
     if ( !hw_reg )
@@ -1466,6 +1469,7 @@ static inline bool gic_dist_supports_dvis(void)
 }
 
 static int gicv3_make_hwdom_madt(const struct domain *d, u32 offset)
+
 {
     struct acpi_subtable_header *header;
     struct acpi_madt_generic_interrupt *host_gicc, *gicc;
@@ -1503,7 +1507,11 @@ static int gicv3_make_hwdom_madt(const struct domain *d, u32 offset)
 
     /* Add Generic Redistributor */
     size = sizeof(struct acpi_madt_generic_redistributor);
-    for ( i = 0; i < gicv3.rdist_count; i++ )
+    /*
+     * The hardware domain may not used all the regions. So only copy
+     * what is necessary.
+     */
+    for ( i = 0; i < d->arch.vgic.nr_regions; i++ )
     {
         gicr = (struct acpi_madt_generic_redistributor *)(base_ptr + table_len);
         gicr->header.type = ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR;
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index df1bab3a35..efe824c6fb 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -1640,7 +1640,11 @@ static int vgic_v3_vcpu_init(struct vcpu *v)
     return 0;
 }
 
-static inline unsigned int vgic_v3_rdist_count(struct domain *d)
+/*
+ * Return the maximum number possible of re-distributor regions for
+ * a given domain.
+ */
+static inline unsigned int vgic_v3_max_rdist_count(struct domain *d)
 {
     /*
      * Normally there is only one GICv3 redistributor region.
@@ -1662,7 +1666,7 @@ static int vgic_v3_real_domain_init(struct domain *d)
     int rdist_count, i, ret;
 
     /* Allocate memory for Re-distributor regions */
-    rdist_count = vgic_v3_rdist_count(d);
+    rdist_count = vgic_v3_max_rdist_count(d);
 
     rdist_regions = xzalloc_array(struct vgic_rdist_region, rdist_count);
     if ( !rdist_regions )
@@ -1695,8 +1699,19 @@ static int vgic_v3_real_domain_init(struct domain *d)
             d->arch.vgic.rdist_regions[i].first_cpu = first_cpu;
 
             first_cpu += size / GICV3_GICR_SIZE;
+
+            if ( first_cpu >= d->max_vcpus )
+                break;
         }
 
+        /*
+         * The hardware domain may not use all the re-distributors
+         * regions (e.g when the number of vCPUs does not match the
+         * number of pCPUs). Update the number of regions to avoid
+         * exposing unused region as they will not get emulated.
+         */
+        d->arch.vgic.nr_regions = i + 1;
+
         d->arch.vgic.intid_bits = vgic_v3_hw.intid_bits;
     }
     else
@@ -1825,7 +1840,7 @@ int vgic_v3_init(struct domain *d, int *mmio_count)
     }
 
     /* GICD region + number of Redistributors */
-    *mmio_count = vgic_v3_rdist_count(d) + 1;
+    *mmio_count = vgic_v3_max_rdist_count(d) + 1;
 
     /* one region per ITS */
     *mmio_count += vgic_v3_its_count(d);
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 2/2] xen/arm: vgic-v3: Don't create empty re-distributor regions

[Stefano Stabellini]

On Mon, 1 Oct 2018, Julien Grall wrote:
> At the moment, Xen is assuming the hardware domain will have the same
> number of re-distributor regions as the host. However, as the
> number of CPUs or the stride (e.g on GICv4) may be different we end up
> exposing regions which does not contain any re-distributors.
> 
> When booting, Linux will go through all the re-distributor region to
> check whether a property (e.g vPLIs) is available accross all the
> re-distributors. This will result to a data abort on empty regions
> because there are no underlying re-distributor.
> 
> So we need to limit the number of regions exposed to the hardware
> domain. The code reworked to only expose the minimun number of regions
> required by the hardware domain. It is assumed the regions will be
> populated starting from the first one.
> 
> Lastly, rename vgic_v3_rdist_count to reflect the value return by the
> helper.
> 
> Reported-by: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Tested-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> 
> ---
>     Changes in v2:
>         - Rename vgic_v3_rdist_count to vgic_v3_max_rdist_count
>         - Fixup #re-distributors
>         - Fix typoes
>         - Add Shameer's tested tag
> ---
>  xen/arch/arm/gic-v3.c  | 14 +++++++++++---
>  xen/arch/arm/vgic-v3.c | 21 ++++++++++++++++++---
>  2 files changed, 29 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index c98a163ee7..2c1454f425 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1265,7 +1265,8 @@ static int gicv3_make_hwdom_dt_node(const struct domain *d,
>      if ( res )
>          return res;
>  
> -    res = fdt_property_cell(fdt, "#redistributor-regions", gicv3.rdist_count);
> +    res = fdt_property_cell(fdt, "#redistributor-regions",
> +                            d->arch.vgic.nr_regions);
>      if ( res )
>          return res;
>  
> @@ -1274,8 +1275,10 @@ static int gicv3_make_hwdom_dt_node(const struct domain *d,
>       * GIC has two memory regions: Distributor + rdist regions
>       * CPU interface and virtual cpu interfaces accessesed as System registers
>       * So cells are created only for Distributor and rdist regions
> +     * The hardware domain may not use all the regions. So only copy
> +     * what is necessary.
>       */
> -    new_len = new_len * (gicv3.rdist_count + 1);
> +    new_len = new_len * (d->arch.vgic.nr_regions + 1);
>  
>      hw_reg = dt_get_property(gic, "reg", &len);
>      if ( !hw_reg )
> @@ -1466,6 +1469,7 @@ static inline bool gic_dist_supports_dvis(void)
>  }
>  
>  static int gicv3_make_hwdom_madt(const struct domain *d, u32 offset)
> +
>  {

Aside from this spurious change, the patch is OK. Provided you remove
the blank on commit:

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


>      struct acpi_subtable_header *header;
>      struct acpi_madt_generic_interrupt *host_gicc, *gicc;
> @@ -1503,7 +1507,11 @@ static int gicv3_make_hwdom_madt(const struct domain *d, u32 offset)
>  
>      /* Add Generic Redistributor */
>      size = sizeof(struct acpi_madt_generic_redistributor);
> -    for ( i = 0; i < gicv3.rdist_count; i++ )
> +    /*
> +     * The hardware domain may not used all the regions. So only copy
> +     * what is necessary.
> +     */
> +    for ( i = 0; i < d->arch.vgic.nr_regions; i++ )
>      {
>          gicr = (struct acpi_madt_generic_redistributor *)(base_ptr + table_len);
>          gicr->header.type = ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR;
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index df1bab3a35..efe824c6fb 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -1640,7 +1640,11 @@ static int vgic_v3_vcpu_init(struct vcpu *v)
>      return 0;
>  }
>  
> -static inline unsigned int vgic_v3_rdist_count(struct domain *d)
> +/*
> + * Return the maximum number possible of re-distributor regions for
> + * a given domain.
> + */
> +static inline unsigned int vgic_v3_max_rdist_count(struct domain *d)
>  {
>      /*
>       * Normally there is only one GICv3 redistributor region.
> @@ -1662,7 +1666,7 @@ static int vgic_v3_real_domain_init(struct domain *d)
>      int rdist_count, i, ret;
>  
>      /* Allocate memory for Re-distributor regions */
> -    rdist_count = vgic_v3_rdist_count(d);
> +    rdist_count = vgic_v3_max_rdist_count(d);
>  
>      rdist_regions = xzalloc_array(struct vgic_rdist_region, rdist_count);
>      if ( !rdist_regions )
> @@ -1695,8 +1699,19 @@ static int vgic_v3_real_domain_init(struct domain *d)
>              d->arch.vgic.rdist_regions[i].first_cpu = first_cpu;
>  
>              first_cpu += size / GICV3_GICR_SIZE;
> +
> +            if ( first_cpu >= d->max_vcpus )
> +                break;
>          }
>  
> +        /*
> +         * The hardware domain may not use all the re-distributors
> +         * regions (e.g when the number of vCPUs does not match the
> +         * number of pCPUs). Update the number of regions to avoid
> +         * exposing unused region as they will not get emulated.
> +         */
> +        d->arch.vgic.nr_regions = i + 1;
> +
>          d->arch.vgic.intid_bits = vgic_v3_hw.intid_bits;
>      }
>      else
> @@ -1825,7 +1840,7 @@ int vgic_v3_init(struct domain *d, int *mmio_count)
>      }
>  
>      /* GICD region + number of Redistributors */
> -    *mmio_count = vgic_v3_rdist_count(d) + 1;
> +    *mmio_count = vgic_v3_max_rdist_count(d) + 1;
>  
>      /* one region per ITS */
>      *mmio_count += vgic_v3_its_count(d);
> -- 
> 2.11.0
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 2/2] xen/arm: vgic-v3: Don't create empty re-distributor regions

[Julien Grall]

Hi,

On 01/10/2018 21:55, Stefano Stabellini wrote:
> On Mon, 1 Oct 2018, Julien Grall wrote:
  > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

Thank you! I have committed both patches. Can you queue them for backport?

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 2/2] xen/arm: vgic-v3: Don't create empty re-distributor regions

[Julien Grall]

Hi,

I forgot to remove patch from the previous series before sending the new 
one.

Please ignore that patch. Sorry for the noise.

Cheers,


On 10/01/2018 07:57 PM, Julien Grall wrote:
> At the moment, Xen is assuming the hardware domain will have the same
> number of re-distributor regions as the host. However, as the
> number of CPUs or the stride (e.g on GICv4) may be different we end up
> exposing regions which does not contain any re-distributors.
> 
> When booting, Linux will go through all the re-distributor region to
> check whether a property (e.g vPLIs) is available accross all the
> re-distributors. This will result to a data abort on empty regions
> because there are no underlying re-distributor.
> 
> So we need to limit the number of regions exposed to the hardware
> domain. The code reworked to only expose the minimun number of regions
> required by the hardware domain. It is assumed the regions will be
> populated starting from the first one.
> 
> Lastly, rename vgic_v3_rdist_count to reflect the value return by the
> helper.
> 
> Reported-by: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Tested-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> 
> ---
>      Changes in v2:
>          - Rename vgic_v3_rdist_count to vgic_v3_max_rdist_count
>          - Fixup #re-distributors
>          - Fix typoes
>          - Add Shameer's tested tag
> ---
>   xen/arch/arm/gic-v3.c  | 14 +++++++++++---
>   xen/arch/arm/vgic-v3.c | 21 ++++++++++++++++++---
>   2 files changed, 29 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index c98a163ee7..2c1454f425 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1265,7 +1265,8 @@ static int gicv3_make_hwdom_dt_node(const struct domain *d,
>       if ( res )
>           return res;
>   
> -    res = fdt_property_cell(fdt, "#redistributor-regions", gicv3.rdist_count);
> +    res = fdt_property_cell(fdt, "#redistributor-regions",
> +                            d->arch.vgic.nr_regions);
>       if ( res )
>           return res;
>   
> @@ -1274,8 +1275,10 @@ static int gicv3_make_hwdom_dt_node(const struct domain *d,
>        * GIC has two memory regions: Distributor + rdist regions
>        * CPU interface and virtual cpu interfaces accessesed as System registers
>        * So cells are created only for Distributor and rdist regions
> +     * The hardware domain may not use all the regions. So only copy
> +     * what is necessary.
>        */
> -    new_len = new_len * (gicv3.rdist_count + 1);
> +    new_len = new_len * (d->arch.vgic.nr_regions + 1);
>   
>       hw_reg = dt_get_property(gic, "reg", &len);
>       if ( !hw_reg )
> @@ -1466,6 +1469,7 @@ static inline bool gic_dist_supports_dvis(void)
>   }
>   
>   static int gicv3_make_hwdom_madt(const struct domain *d, u32 offset)
> +
>   {
>       struct acpi_subtable_header *header;
>       struct acpi_madt_generic_interrupt *host_gicc, *gicc;
> @@ -1503,7 +1507,11 @@ static int gicv3_make_hwdom_madt(const struct domain *d, u32 offset)
>   
>       /* Add Generic Redistributor */
>       size = sizeof(struct acpi_madt_generic_redistributor);
> -    for ( i = 0; i < gicv3.rdist_count; i++ )
> +    /*
> +     * The hardware domain may not used all the regions. So only copy
> +     * what is necessary.
> +     */
> +    for ( i = 0; i < d->arch.vgic.nr_regions; i++ )
>       {
>           gicr = (struct acpi_madt_generic_redistributor *)(base_ptr + table_len);
>           gicr->header.type = ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR;
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index df1bab3a35..efe824c6fb 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -1640,7 +1640,11 @@ static int vgic_v3_vcpu_init(struct vcpu *v)
>       return 0;
>   }
>   
> -static inline unsigned int vgic_v3_rdist_count(struct domain *d)
> +/*
> + * Return the maximum number possible of re-distributor regions for
> + * a given domain.
> + */
> +static inline unsigned int vgic_v3_max_rdist_count(struct domain *d)
>   {
>       /*
>        * Normally there is only one GICv3 redistributor region.
> @@ -1662,7 +1666,7 @@ static int vgic_v3_real_domain_init(struct domain *d)
>       int rdist_count, i, ret;
>   
>       /* Allocate memory for Re-distributor regions */
> -    rdist_count = vgic_v3_rdist_count(d);
> +    rdist_count = vgic_v3_max_rdist_count(d);
>   
>       rdist_regions = xzalloc_array(struct vgic_rdist_region, rdist_count);
>       if ( !rdist_regions )
> @@ -1695,8 +1699,19 @@ static int vgic_v3_real_domain_init(struct domain *d)
>               d->arch.vgic.rdist_regions[i].first_cpu = first_cpu;
>   
>               first_cpu += size / GICV3_GICR_SIZE;
> +
> +            if ( first_cpu >= d->max_vcpus )
> +                break;
>           }
>   
> +        /*
> +         * The hardware domain may not use all the re-distributors
> +         * regions (e.g when the number of vCPUs does not match the
> +         * number of pCPUs). Update the number of regions to avoid
> +         * exposing unused region as they will not get emulated.
> +         */
> +        d->arch.vgic.nr_regions = i + 1;
> +
>           d->arch.vgic.intid_bits = vgic_v3_hw.intid_bits;
>       }
>       else
> @@ -1825,7 +1840,7 @@ int vgic_v3_init(struct domain *d, int *mmio_count)
>       }
>   
>       /* GICD region + number of Redistributors */
> -    *mmio_count = vgic_v3_rdist_count(d) + 1;
> +    *mmio_count = vgic_v3_max_rdist_count(d) + 1;
>   
>       /* one region per ITS */
>       *mmio_count += vgic_v3_its_count(d);
> 

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 2/2] xen/arm: vgic-v3: Don't create empty re-distributor regions

[Julien Grall]

At the moment, Xen is assuming the hardware domain will have the same
number of re-distributor regions as the host. However, as the
number of CPUs or the stride (e.g on GICv4) may be different we end up
exposing regions which does not contain any re-distributors.

When booting, Linux will go through all the re-distributor region to
check whether a property (e.g vPLIs) is available accross all the
re-distributors. This will result to a data abort on empty regions
because there are no underlying re-distributor.

So we need to limit the number of regions exposed to the hardware
domain. The code reworked to only expose the minimun number of regions
required by the hardware domain. It is assumed the regions will be
populated starting from the first one.

Lastly, rename vgic_v3_rdist_count to reflect the value return by the
helper.

Reported-by: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
Signed-off-by: Julien Grall <julien.grall@arm.com>
Tested-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

---
    Changes in v2:
        - Rename vgic_v3_rdist_count to vgic_v3_max_rdist_count
        - Fixup #re-distributors
        - Fix typoes
        - Add Shameer's tested tag
---
 xen/arch/arm/gic-v3.c  | 14 +++++++++++---
 xen/arch/arm/vgic-v3.c | 21 ++++++++++++++++++---
 2 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index c98a163ee7..2c1454f425 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1265,7 +1265,8 @@ static int gicv3_make_hwdom_dt_node(const struct domain *d,
     if ( res )
         return res;
 
-    res = fdt_property_cell(fdt, "#redistributor-regions", gicv3.rdist_count);
+    res = fdt_property_cell(fdt, "#redistributor-regions",
+                            d->arch.vgic.nr_regions);
     if ( res )
         return res;
 
@@ -1274,8 +1275,10 @@ static int gicv3_make_hwdom_dt_node(const struct domain *d,
      * GIC has two memory regions: Distributor + rdist regions
      * CPU interface and virtual cpu interfaces accessesed as System registers
      * So cells are created only for Distributor and rdist regions
+     * The hardware domain may not use all the regions. So only copy
+     * what is necessary.
      */
-    new_len = new_len * (gicv3.rdist_count + 1);
+    new_len = new_len * (d->arch.vgic.nr_regions + 1);
 
     hw_reg = dt_get_property(gic, "reg", &len);
     if ( !hw_reg )
@@ -1466,6 +1469,7 @@ static inline bool gic_dist_supports_dvis(void)
 }
 
 static int gicv3_make_hwdom_madt(const struct domain *d, u32 offset)
+
 {
     struct acpi_subtable_header *header;
     struct acpi_madt_generic_interrupt *host_gicc, *gicc;
@@ -1503,7 +1507,11 @@ static int gicv3_make_hwdom_madt(const struct domain *d, u32 offset)
 
     /* Add Generic Redistributor */
     size = sizeof(struct acpi_madt_generic_redistributor);
-    for ( i = 0; i < gicv3.rdist_count; i++ )
+    /*
+     * The hardware domain may not used all the regions. So only copy
+     * what is necessary.
+     */
+    for ( i = 0; i < d->arch.vgic.nr_regions; i++ )
     {
         gicr = (struct acpi_madt_generic_redistributor *)(base_ptr + table_len);
         gicr->header.type = ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR;
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index df1bab3a35..efe824c6fb 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -1640,7 +1640,11 @@ static int vgic_v3_vcpu_init(struct vcpu *v)
     return 0;
 }
 
-static inline unsigned int vgic_v3_rdist_count(struct domain *d)
+/*
+ * Return the maximum number possible of re-distributor regions for
+ * a given domain.
+ */
+static inline unsigned int vgic_v3_max_rdist_count(struct domain *d)
 {
     /*
      * Normally there is only one GICv3 redistributor region.
@@ -1662,7 +1666,7 @@ static int vgic_v3_real_domain_init(struct domain *d)
     int rdist_count, i, ret;
 
     /* Allocate memory for Re-distributor regions */
-    rdist_count = vgic_v3_rdist_count(d);
+    rdist_count = vgic_v3_max_rdist_count(d);
 
     rdist_regions = xzalloc_array(struct vgic_rdist_region, rdist_count);
     if ( !rdist_regions )
@@ -1695,8 +1699,19 @@ static int vgic_v3_real_domain_init(struct domain *d)
             d->arch.vgic.rdist_regions[i].first_cpu = first_cpu;
 
             first_cpu += size / GICV3_GICR_SIZE;
+
+            if ( first_cpu >= d->max_vcpus )
+                break;
         }
 
+        /*
+         * The hardware domain may not use all the re-distributors
+         * regions (e.g when the number of vCPUs does not match the
+         * number of pCPUs). Update the number of regions to avoid
+         * exposing unused region as they will not get emulated.
+         */
+        d->arch.vgic.nr_regions = i + 1;
+
         d->arch.vgic.intid_bits = vgic_v3_hw.intid_bits;
     }
     else
@@ -1825,7 +1840,7 @@ int vgic_v3_init(struct domain *d, int *mmio_count)
     }
 
     /* GICD region + number of Redistributors */
-    *mmio_count = vgic_v3_rdist_count(d) + 1;
+    *mmio_count = vgic_v3_max_rdist_count(d) + 1;
 
     /* one region per ITS */
     *mmio_count += vgic_v3_its_count(d);
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel