| Message ID | 20180124143517.18469-3-andre.przywara@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ARM: vGICv3: clean up optional DT properties | expand |
Hi Andre, On 24/01/18 14:35, Andre Przywara wrote: > Architecturally there is only one GICv3 redistributor region. > Drop the symbol which suggested that was a delibarate choice for Xen > guests, instead hard code the "1" in the appropriate places, along with > a comment to explain the reasons. > > Signed-off-by: Andre Przywara <andre.przywara@linaro.org> > --- > xen/arch/arm/vgic-v3.c | 17 ++++++++++++----- > xen/include/public/arch-arm.h | 1 - > 2 files changed, 12 insertions(+), 6 deletions(-) > > diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c > index af16dfd005..7d3ea171b4 100644 > --- a/xen/arch/arm/vgic-v3.c > +++ b/xen/arch/arm/vgic-v3.c > @@ -1640,8 +1640,18 @@ static int vgic_v3_vcpu_init(struct vcpu *v) > > static inline unsigned int vgic_v3_rdist_count(struct domain *d) > { > - return is_hardware_domain(d) ? vgic_v3_hw.nr_rdist_regions : > - GUEST_GICV3_RDIST_REGIONS; > + /* > + * Architecturally there is only one GICv3 redistributor region. > + * The GICv3 DT binding provisions for multiple regions, since there are > + * platforms out there which break this architectural assumption. > + * ACPI does not support this workaround at all. This is not true. The ACPI spec supports multiple regions of redistributors. What ACPI does not support is a different stride. > + * For Dom0 we have to live with the MMIO layout the hardware provides, > + * so we have to copy the multiple regions - as the first region may not > + * provide enough space to hold all redistributors we need. > + * However DomU get a constructed memory map, so we can go with > + * the architected single redistributor region. > + */ > + return is_hardware_domain(d) ? vgic_v3_hw.nr_rdist_regions : 1; > } > > static int vgic_v3_domain_init(struct domain *d) > @@ -1700,9 +1710,6 @@ static int vgic_v3_domain_init(struct domain *d) > { > d->arch.vgic.dbase = GUEST_GICV3_GICD_BASE; > > - /* XXX: Only one Re-distributor region mapped for the guest */ > - BUILD_BUG_ON(GUEST_GICV3_RDIST_REGIONS != 1); > - > d->arch.vgic.rdist_stride = GUEST_GICV3_RDIST_STRIDE; > > /* The first redistributor should contain enough space for all CPUs */ > diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h > index 05fd11ca38..ca79ab6284 100644 > --- a/xen/include/public/arch-arm.h > +++ b/xen/include/public/arch-arm.h > @@ -402,7 +402,6 @@ typedef uint64_t xen_callback_t; > #define GUEST_GICV3_GICD_SIZE xen_mk_ullong(0x00010000) > > #define GUEST_GICV3_RDIST_STRIDE xen_mk_ullong(0x00020000) > -#define GUEST_GICV3_RDIST_REGIONS 1 > > #define GUEST_GICV3_GICR0_BASE xen_mk_ullong(0x03020000) /* vCPU0..127 */ > #define GUEST_GICV3_GICR0_SIZE xen_mk_ullong(0x01000000) > Cheers,
Hi, On 24/01/18 16:13, Julien Grall wrote: > Hi Andre, > > On 24/01/18 14:35, Andre Przywara wrote: >> Architecturally there is only one GICv3 redistributor region. >> Drop the symbol which suggested that was a delibarate choice for Xen >> guests, instead hard code the "1" in the appropriate places, along with >> a comment to explain the reasons. >> >> Signed-off-by: Andre Przywara <andre.przywara@linaro.org> >> --- >> xen/arch/arm/vgic-v3.c | 17 ++++++++++++----- >> xen/include/public/arch-arm.h | 1 - >> 2 files changed, 12 insertions(+), 6 deletions(-) >> >> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c >> index af16dfd005..7d3ea171b4 100644 >> --- a/xen/arch/arm/vgic-v3.c >> +++ b/xen/arch/arm/vgic-v3.c >> @@ -1640,8 +1640,18 @@ static int vgic_v3_vcpu_init(struct vcpu *v) >> static inline unsigned int vgic_v3_rdist_count(struct domain *d) >> { >> - return is_hardware_domain(d) ? vgic_v3_hw.nr_rdist_regions : >> - GUEST_GICV3_RDIST_REGIONS; >> + /* >> + * Architecturally there is only one GICv3 redistributor region. >> + * The GICv3 DT binding provisions for multiple regions, since >> there are >> + * platforms out there which break this architectural assumption. >> + * ACPI does not support this workaround at all. > > This is not true. The ACPI spec supports multiple regions of > redistributors. What ACPI does not support is a different stride. Ah, that's true. Sorry for that, I was a bit too enthusiastic here ;-) Will change the comment, definitely. However I would still be interested in dropping the GUEST_GICV3_RDIST_REGIONS symbol, possibly replacing it with DEFAULT_GICV3_RDIST_REGIONS, avoiding the misleading GUEST_ prefix. Cheers, Andre. >> + * For Dom0 we have to live with the MMIO layout the hardware >> provides, >> + * so we have to copy the multiple regions - as the first region >> may not >> + * provide enough space to hold all redistributors we need. >> + * However DomU get a constructed memory map, so we can go with >> + * the architected single redistributor region. >> + */ >> + return is_hardware_domain(d) ? vgic_v3_hw.nr_rdist_regions : 1; >> > } >> static int vgic_v3_domain_init(struct domain *d) >> @@ -1700,9 +1710,6 @@ static int vgic_v3_domain_init(struct domain *d) >> { >> d->arch.vgic.dbase = GUEST_GICV3_GICD_BASE; >> - /* XXX: Only one Re-distributor region mapped for the guest */ >> - BUILD_BUG_ON(GUEST_GICV3_RDIST_REGIONS != 1); >> - >> d->arch.vgic.rdist_stride = GUEST_GICV3_RDIST_STRIDE; >> /* The first redistributor should contain enough space for >> all CPUs */ >> diff --git a/xen/include/public/arch-arm.h >> b/xen/include/public/arch-arm.h >> index 05fd11ca38..ca79ab6284 100644 >> --- a/xen/include/public/arch-arm.h >> +++ b/xen/include/public/arch-arm.h >> @@ -402,7 +402,6 @@ typedef uint64_t xen_callback_t; >> #define GUEST_GICV3_GICD_SIZE xen_mk_ullong(0x00010000) >> #define GUEST_GICV3_RDIST_STRIDE xen_mk_ullong(0x00020000) >> -#define GUEST_GICV3_RDIST_REGIONS 1 >> #define GUEST_GICV3_GICR0_BASE xen_mk_ullong(0x03020000) /* >> vCPU0..127 */ >> #define GUEST_GICV3_GICR0_SIZE xen_mk_ullong(0x01000000) >> > > Cheers, >
Hi Andre, On 24/01/18 16:58, Andre Przywara wrote: > On 24/01/18 16:13, Julien Grall wrote: >> Hi Andre, >> >> On 24/01/18 14:35, Andre Przywara wrote: >>> Architecturally there is only one GICv3 redistributor region. >>> Drop the symbol which suggested that was a delibarate choice for Xen >>> guests, instead hard code the "1" in the appropriate places, along with >>> a comment to explain the reasons. >>> >>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org> >>> --- >>> xen/arch/arm/vgic-v3.c | 17 ++++++++++++----- >>> xen/include/public/arch-arm.h | 1 - >>> 2 files changed, 12 insertions(+), 6 deletions(-) >>> >>> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c >>> index af16dfd005..7d3ea171b4 100644 >>> --- a/xen/arch/arm/vgic-v3.c >>> +++ b/xen/arch/arm/vgic-v3.c >>> @@ -1640,8 +1640,18 @@ static int vgic_v3_vcpu_init(struct vcpu *v) >>> static inline unsigned int vgic_v3_rdist_count(struct domain *d) >>> { >>> - return is_hardware_domain(d) ? vgic_v3_hw.nr_rdist_regions : >>> - GUEST_GICV3_RDIST_REGIONS; >>> + /* >>> + * Architecturally there is only one GICv3 redistributor region. >>> + * The GICv3 DT binding provisions for multiple regions, since >>> there are >>> + * platforms out there which break this architectural assumption. >>> + * ACPI does not support this workaround at all. >> >> This is not true. The ACPI spec supports multiple regions of >> redistributors. What ACPI does not support is a different stride. > > Ah, that's true. Sorry for that, I was a bit too enthusiastic here ;-) > Will change the comment, definitely. > However I would still be interested in dropping the > GUEST_GICV3_RDIST_REGIONS symbol, possibly replacing it with > DEFAULT_GICV3_RDIST_REGIONS, avoiding the misleading GUEST_ prefix. Why GUEST_ is misleading? Bear in mind that the toolstack is in charge of the memory map. Not the hypervisor. Today, the memory map is static and so rather than implementing a bunch of hypercalls the hypervisor is directly using the value. Cheers,
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c index af16dfd005..7d3ea171b4 100644 --- a/xen/arch/arm/vgic-v3.c +++ b/xen/arch/arm/vgic-v3.c @@ -1640,8 +1640,18 @@ static int vgic_v3_vcpu_init(struct vcpu *v) static inline unsigned int vgic_v3_rdist_count(struct domain *d) { - return is_hardware_domain(d) ? vgic_v3_hw.nr_rdist_regions : - GUEST_GICV3_RDIST_REGIONS; + /* + * Architecturally there is only one GICv3 redistributor region. + * The GICv3 DT binding provisions for multiple regions, since there are + * platforms out there which break this architectural assumption. + * ACPI does not support this workaround at all. + * For Dom0 we have to live with the MMIO layout the hardware provides, + * so we have to copy the multiple regions - as the first region may not + * provide enough space to hold all redistributors we need. + * However DomU get a constructed memory map, so we can go with + * the architected single redistributor region. + */ + return is_hardware_domain(d) ? vgic_v3_hw.nr_rdist_regions : 1; } static int vgic_v3_domain_init(struct domain *d) @@ -1700,9 +1710,6 @@ static int vgic_v3_domain_init(struct domain *d) { d->arch.vgic.dbase = GUEST_GICV3_GICD_BASE; - /* XXX: Only one Re-distributor region mapped for the guest */ - BUILD_BUG_ON(GUEST_GICV3_RDIST_REGIONS != 1); - d->arch.vgic.rdist_stride = GUEST_GICV3_RDIST_STRIDE; /* The first redistributor should contain enough space for all CPUs */ diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h index 05fd11ca38..ca79ab6284 100644 --- a/xen/include/public/arch-arm.h +++ b/xen/include/public/arch-arm.h @@ -402,7 +402,6 @@ typedef uint64_t xen_callback_t; #define GUEST_GICV3_GICD_SIZE xen_mk_ullong(0x00010000) #define GUEST_GICV3_RDIST_STRIDE xen_mk_ullong(0x00020000) -#define GUEST_GICV3_RDIST_REGIONS 1 #define GUEST_GICV3_GICR0_BASE xen_mk_ullong(0x03020000) /* vCPU0..127 */ #define GUEST_GICV3_GICR0_SIZE xen_mk_ullong(0x01000000)
Architecturally there is only one GICv3 redistributor region. Drop the symbol which suggested that was a delibarate choice for Xen guests, instead hard code the "1" in the appropriate places, along with a comment to explain the reasons. Signed-off-by: Andre Przywara <andre.przywara@linaro.org> --- xen/arch/arm/vgic-v3.c | 17 ++++++++++++----- xen/include/public/arch-arm.h | 1 - 2 files changed, 12 insertions(+), 6 deletions(-)