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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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.


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.


> 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?


>      hw_reg = dt_get_property(gic, "reg", &len);
>      if ( !hw_reg )
> @@ -1503,7 +1505,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
                                      ^ use


> +     * 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..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.



>          }
>  
> +        /*
> +         * The hardware domain may not used all the re-distributors
                                          ^ use


> +         * 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.
                               ^ regions


> +         */
> +        d->arch.vgic.nr_regions = i + 1;
>          d->arch.vgic.intid_bits = vgic_v3_hw.intid_bits;
>      }
>      else

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