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

[Julien Grall <julien.grall@xxxxxxx>]

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