| Message ID | 20180904192153.17210-3-julien.grall@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | xen/arm: vgic-v3: Bug fixes | expand |
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@huawei.com> > Signed-off-by: Julien Grall <julien.grall@arm.com> > --- > 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
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@huawei.com> >> Signed-off-by: Julien Grall <julien.grall@arm.com> >> --- >> 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,
On Wed, 26 Sep 2018, Julien Grall wrote: > 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. OK > > > > > 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. We don't want to or we can't? Because it looks like we would want to fix vgic_v3_rdist_count if we could, right? It is called from domain specific initialization functions, so theoretically it should return domain specific vgic information, not physical information. > 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). Yeah, this is nasty, but it is clear that until we rework the code to set d->max_vcpus earlier it won't get fixed. Nothing we can do here. So, I think ideally we would want to fix vgic_v3_rdist_count, but today we can't. Maybe we could rename vgic_v3_rdist_count to vgic_v3_hw_rdist_count, and add a short TODO comment somewhere in the file?
On 09/28/2018 12:34 AM, Stefano Stabellini wrote: > On Wed, 26 Sep 2018, Julien Grall wrote: >> 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. > > OK > > >>> > >>> 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. > > We don't want to or we can't? Because it looks like we would want to fix > vgic_v3_rdist_count if we could, right? It is called from domain > specific initialization functions, so theoretically it should return > domain specific vgic information, not physical information. We don't want to fix in the current design. > > >> 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). > > Yeah, this is nasty, but it is clear that until we rework the code to > set d->max_vcpus earlier it won't get fixed. Nothing we can do here. > > So, I think ideally we would want to fix vgic_v3_rdist_count, but today > we can't. Maybe we could rename vgic_v3_rdist_count to > vgic_v3_hw_rdist_count, and add a short TODO comment somewhere in the > file? > Which would be wrong as the function don't always return the number of rdist for the HW. A better name would be vgic_v3_max_rdist_count(struct domain *d). Cheers,
On Fri, 28 Sep 2018, Julien Grall wrote: > On 09/28/2018 12:34 AM, Stefano Stabellini wrote: > > On Wed, 26 Sep 2018, Julien Grall wrote: > > > 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. > > > > OK > > > > > > > > > > > > > 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. > > > > We don't want to or we can't? Because it looks like we would want to fix > > vgic_v3_rdist_count if we could, right? It is called from domain > > specific initialization functions, so theoretically it should return > > domain specific vgic information, not physical information. > > We don't want to fix in the current design. > > > > > > > > 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). > > > > Yeah, this is nasty, but it is clear that until we rework the code to > > set d->max_vcpus earlier it won't get fixed. Nothing we can do here. > > > > So, I think ideally we would want to fix vgic_v3_rdist_count, but today > > we can't. Maybe we could rename vgic_v3_rdist_count to > > vgic_v3_hw_rdist_count, and add a short TODO comment somewhere in the > > file? > > > > Which would be wrong as the function don't always return the number of rdist > for the HW. > > A better name would be vgic_v3_max_rdist_count(struct domain *d). I am OK with that
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); 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 + * 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; } + /* + * The hardware domain may not used 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
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. Reported-by: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> Signed-off-by: Julien Grall <julien.grall@arm.com> --- xen/arch/arm/gic-v3.c | 10 ++++++++-- xen/arch/arm/vgic-v3.c | 11 +++++++++++ 2 files changed, 19 insertions(+), 2 deletions(-)