[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



Hi Stefano,

On 09/25/2018 09:38 PM, Stefano Stabellini wrote:
On Tue, 4 Sep 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.

I have a question: given that it is possible for a rdist_region to cover
more than 1 cpu, could we get into troubles if the last rdist_region of
the hardware_domain covers 2 cpus, but actually dom0 only has 1 vcpu?
get_vcpu_from_rdist would return NULL and vgic_v3_rdistr_mmio_read/write
would return 0.
This case seems to be handled correctly but I wanted to
double check.

0 means a data abort will be injected into the guest. However, the guest should never touch that because the last valid re-distributor of the regions will have GICR_TYPER.Last set.

So the guest OS will stop looking for more re-distributors in that region.

 >
I think we also need to fix vgic_v3_rdist_count? Today it just returns
vgic_v3_hw.nr_rdist_regions for dom0. It would be bad if we left it
unfixed? If we do that, we might be able to get rid of the modifications
to vgic_v3_real_domain_init.

We don't want to fix vgic_v3_rdist_count. The helper returns the maximum re-distributors regions. This is used to compute the number of IO handlers and allocating the array storing the regions.

I am pretty sure you will say we will waste memory. However, at the moment, we need to know the number of IO handlers much before we know the number of vCPUs. For the array, we would need to go through the regions twice (regions may not be the same size so we can't infer easily the number needed). Overall, the amount of memory used is the same as today (so not really a waste per-se).

It might be possible to limit that once we reworked the common code to know the number of vCPUs earlier on (see discussion on patch #1).

Reported-by: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@xxxxxxxxxx>
Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
---
  xen/arch/arm/gic-v3.c  | 10 ++++++++--
  xen/arch/arm/vgic-v3.c | 11 +++++++++++
  2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index b2ed0f8b55..4a984cfb12 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1274,8 +1274,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 used 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);

Do we also need to fix "#redistributor-regions" just above?
Hmm I missed that one. Not sure why it didn't show up in my test.

diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index df1bab3a35..9f729862da 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -1695,8 +1695,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;

This is just a matter of code style and preferences, but I would prefer
if the termination condition was at the top as part of the for
statement. Of course, it works regardless, so the patch would be
OK either way.

I thought about it when writing this patch. This would look like:

for ( i = 0;
     (i < vgic_v3_hw.nr_dist_regions) && (first_cpu < d->max_vcpus);
     i++ )

This is IHMO more difficult to understand (long condition) and slightly strange because for is not incrementing directly first_cpu.

I will stick with the current implementation unless you have a more readable solution.

Cheers,

--
Julien Grall

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.