Re: [Xen-devel] [PATCH for-4.9] x86/vioapic: allow holes in the GSI range for PVH Dom0
>>> On 18.04.17 at 10:49, wrote: > On Tue, Apr 18, 2017 at 02:39:34AM -0600, Jan Beulich wrote: >> >>> On 17.04.17 at 18:09, wrote: >> > @@ -601,7 +587,12 @@ int vioapic_init(struct domain *d) >> > nr_gsis += nr_pins; >> > } >> > >> > -ASSERT(hvm_domain_irq(d)->nr_gsis == nr_gsis); >> > +/* >> > + * NB: hvm_domain_irq(d)->nr_gsis is actually the highest GSI + 1, but >> > + * there might be holes in this range (ie: GSIs that don't belong to >> > any >> > + * vIO APIC). >> > + */ >> > +ASSERT(hvm_domain_irq(d)->nr_gsis >= nr_gsis); >> >> This becomes too weak then, as you want to index the array using >> the GSI (and not some compressed representation with the holes >> squashed). Which in turn means the nr_gsis calculation in this >> function is now wrong - you need to accumulate the maximum >> base_gsi + nr_pins value here instead. With that >= will actually >> be fine to use here. > > Is the array of IO APICs guaranteed to be ordered from lower GSI to highest > one? So far it seems like it is on all the machines I've tested, but I'm not > sure this is a guarantee, thus I'm going to use: > > nr_gsis = max(nr_gsis, base_gsi + nr_pins); Yes - as Andrew has already said, don't make assumptions here. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH for-4.9] x86/vioapic: allow holes in the GSI range for PVH Dom0
On 18/04/2017 09:49, Roger Pau Monne wrote: > On Tue, Apr 18, 2017 at 02:39:34AM -0600, Jan Beulich wrote: > On 17.04.17 at 18:09, wrote: >>> @@ -601,7 +587,12 @@ int vioapic_init(struct domain *d) >>> nr_gsis += nr_pins; >>> } >>> >>> -ASSERT(hvm_domain_irq(d)->nr_gsis == nr_gsis); >>> +/* >>> + * NB: hvm_domain_irq(d)->nr_gsis is actually the highest GSI + 1, but >>> + * there might be holes in this range (ie: GSIs that don't belong to >>> any >>> + * vIO APIC). >>> + */ >>> +ASSERT(hvm_domain_irq(d)->nr_gsis >= nr_gsis); >> This becomes too weak then, as you want to index the array using >> the GSI (and not some compressed representation with the holes >> squashed). Which in turn means the nr_gsis calculation in this >> function is now wrong - you need to accumulate the maximum >> base_gsi + nr_pins value here instead. With that >= will actually >> be fine to use here. > Is the array of IO APICs guaranteed to be ordered from lower GSI to highest > one? I would certainly not bet on it. > So far it seems like it is on all the machines I've tested, but I'm not > sure this is a guarantee, thus I'm going to use: > > nr_gsis = max(nr_gsis, base_gsi + nr_pins); This is indeed the right thing to do. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH for-4.9] x86/vioapic: allow holes in the GSI range for PVH Dom0
On Tue, Apr 18, 2017 at 02:39:34AM -0600, Jan Beulich wrote: > >>> On 17.04.17 at 18:09, wrote: > > @@ -601,7 +587,12 @@ int vioapic_init(struct domain *d) > > nr_gsis += nr_pins; > > } > > > > -ASSERT(hvm_domain_irq(d)->nr_gsis == nr_gsis); > > +/* > > + * NB: hvm_domain_irq(d)->nr_gsis is actually the highest GSI + 1, but > > + * there might be holes in this range (ie: GSIs that don't belong to > > any > > + * vIO APIC). > > + */ > > +ASSERT(hvm_domain_irq(d)->nr_gsis >= nr_gsis); > > This becomes too weak then, as you want to index the array using > the GSI (and not some compressed representation with the holes > squashed). Which in turn means the nr_gsis calculation in this > function is now wrong - you need to accumulate the maximum > base_gsi + nr_pins value here instead. With that >= will actually > be fine to use here. Is the array of IO APICs guaranteed to be ordered from lower GSI to highest one? So far it seems like it is on all the machines I've tested, but I'm not sure this is a guarantee, thus I'm going to use: nr_gsis = max(nr_gsis, base_gsi + nr_pins); Just in case. Thanks, Roger. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH for-4.9] x86/vioapic: allow holes in the GSI range for PVH Dom0
>>> On 17.04.17 at 18:09, wrote: > @@ -601,7 +587,12 @@ int vioapic_init(struct domain *d) > nr_gsis += nr_pins; > } > > -ASSERT(hvm_domain_irq(d)->nr_gsis == nr_gsis); > +/* > + * NB: hvm_domain_irq(d)->nr_gsis is actually the highest GSI + 1, but > + * there might be holes in this range (ie: GSIs that don't belong to any > + * vIO APIC). > + */ > +ASSERT(hvm_domain_irq(d)->nr_gsis >= nr_gsis); This becomes too weak then, as you want to index the array using the GSI (and not some compressed representation with the holes squashed). Which in turn means the nr_gsis calculation in this function is now wrong - you need to accumulate the maximum base_gsi + nr_pins value here instead. With that >= will actually be fine to use here. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH for-4.9] x86/vioapic: allow holes in the GSI range for PVH Dom0
On Mon, Apr 17, 2017 at 05:09:22PM +0100, Roger Pau Monne wrote: > The current vIO APIC for PVH Dom0 doesn't allow non-contiguous GSIs, which > means that all GSIs must belong to an IO APIC. This doesn't match reality, > where there are systems with non-contiguous GSIs. > > In order to fix this add a base_gsi field to each hvm_vioapic struct, in order > to store the base GSI for each emulated IO APIC. For PVH Dom0 those values are > populated based on the hardware ones. > > Signed-off-by: Roger Pau Monné > Reported-by: Chao Gao For consistency I think the following chunk should also be merged into it, now that Xen can get the base GSI from the vIO APIC struct itself. Roger. ---8<--- diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c index db9be87612..020c355faf 100644 --- a/xen/arch/x86/hvm/dom0_build.c +++ b/xen/arch/x86/hvm/dom0_build.c @@ -719,7 +719,7 @@ static int __init pvh_setup_acpi_madt(struct domain *d, paddr_t *addr) io_apic->header.length = sizeof(*io_apic); io_apic->id = domain_vioapic(d, i)->id; io_apic->address = domain_vioapic(d, i)->base_address; -io_apic->global_irq_base = io_apic_gsi_base(i); +io_apic->global_irq_base = domain_vioapic(d, i)->base_gsi; io_apic++; } ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH for-4.9] x86/vioapic: allow holes in the GSI range for PVH Dom0
On Mon, Apr 17, 2017 at 05:09:22PM +0100, Roger Pau Monne wrote: >The current vIO APIC for PVH Dom0 doesn't allow non-contiguous GSIs, which >means that all GSIs must belong to an IO APIC. This doesn't match reality, >where there are systems with non-contiguous GSIs. > >In order to fix this add a base_gsi field to each hvm_vioapic struct, in order >to store the base GSI for each emulated IO APIC. For PVH Dom0 those values are >populated based on the hardware ones. > >Signed-off-by: Roger Pau Monné >Reported-by: Chao Gao Tested-by: Chao Gao Thanks Chao ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH for-4.9] x86/vioapic: allow holes in the GSI range for PVH Dom0
The current vIO APIC for PVH Dom0 doesn't allow non-contiguous GSIs, which
means that all GSIs must belong to an IO APIC. This doesn't match reality,
where there are systems with non-contiguous GSIs.
In order to fix this add a base_gsi field to each hvm_vioapic struct, in order
to store the base GSI for each emulated IO APIC. For PVH Dom0 those values are
populated based on the hardware ones.
Signed-off-by: Roger Pau Monné
Reported-by: Chao Gao
---
Cc: Jan Beulich
Cc: Andrew Cooper
Cc: Julien Grall
Cc: Chao Gao
---
I think this is a good canditate to be included in 4.9, it's a bugfix, and it's
only used by PVH Dom0, so the risk is low IMHO.
---
xen/arch/x86/hvm/vioapic.c| 41 +++
xen/include/asm-x86/hvm/vioapic.h | 1 +
2 files changed, 17 insertions(+), 25 deletions(-)
diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c
index 5157db7a4e..ec87a97651 100644
--- a/xen/arch/x86/hvm/vioapic.c
+++ b/xen/arch/x86/hvm/vioapic.c
@@ -64,37 +64,23 @@ static struct hvm_vioapic *addr_vioapic(const struct domain
*d,
struct hvm_vioapic *gsi_vioapic(const struct domain *d, unsigned int gsi,
unsigned int *pin)
{
-unsigned int i, base_gsi = 0;
+unsigned int i;
for ( i = 0; i < d->arch.hvm_domain.nr_vioapics; i++ )
{
struct hvm_vioapic *vioapic = domain_vioapic(d, i);
-if ( gsi >= base_gsi && gsi < base_gsi + vioapic->nr_pins )
+if ( gsi >= vioapic->base_gsi &&
+ gsi < vioapic->base_gsi + vioapic->nr_pins )
{
-*pin = gsi - base_gsi;
+*pin = gsi - vioapic->base_gsi;
return vioapic;
}
-
-base_gsi += vioapic->nr_pins;
}
return NULL;
}
-static unsigned int base_gsi(const struct domain *d,
- const struct hvm_vioapic *vioapic)
-{
-unsigned int nr_vioapics = d->arch.hvm_domain.nr_vioapics;
-unsigned int base_gsi = 0, i = 0;
-const struct hvm_vioapic *tmp;
-
-while ( i < nr_vioapics && (tmp = domain_vioapic(d, i++)) != vioapic )
-base_gsi += tmp->nr_pins;
-
-return base_gsi;
-}
-
static uint32_t vioapic_read_indirect(const struct hvm_vioapic *vioapic)
{
uint32_t result = 0;
@@ -180,7 +166,7 @@ static void vioapic_write_redirent(
struct hvm_irq *hvm_irq = hvm_domain_irq(d);
union vioapic_redir_entry *pent, ent;
int unmasked = 0;
-unsigned int gsi = base_gsi(d, vioapic) + idx;
+unsigned int gsi = vioapic->base_gsi + idx;
spin_lock(&d->arch.hvm_domain.irq_lock);
@@ -340,7 +326,7 @@ static void vioapic_deliver(struct hvm_vioapic *vioapic,
unsigned int pin)
struct domain *d = vioapic_domain(vioapic);
struct vlapic *target;
struct vcpu *v;
-unsigned int irq = base_gsi(d, vioapic) + pin;
+unsigned int irq = vioapic->base_gsi + pin;
ASSERT(spin_is_locked(&d->arch.hvm_domain.irq_lock));
@@ -451,7 +437,7 @@ void vioapic_update_EOI(struct domain *d, u8 vector)
{
struct hvm_irq *hvm_irq = hvm_domain_irq(d);
union vioapic_redir_entry *ent;
-unsigned int i, base_gsi = 0;
+unsigned int i;
ASSERT(has_vioapic(d));
@@ -473,19 +459,18 @@ void vioapic_update_EOI(struct domain *d, u8 vector)
if ( iommu_enabled )
{
spin_unlock(&d->arch.hvm_domain.irq_lock);
-hvm_dpci_eoi(d, base_gsi + pin, ent);
+hvm_dpci_eoi(d, vioapic->base_gsi + pin, ent);
spin_lock(&d->arch.hvm_domain.irq_lock);
}
if ( (ent->fields.trig_mode == VIOAPIC_LEVEL_TRIG) &&
!ent->fields.mask &&
- hvm_irq->gsi_assert_count[base_gsi + pin] )
+ hvm_irq->gsi_assert_count[vioapic->base_gsi + pin] )
{
ent->fields.remote_irr = 1;
vioapic_deliver(vioapic, pin);
}
}
-base_gsi += vioapic->nr_pins;
}
spin_unlock(&d->arch.hvm_domain.irq_lock);
@@ -554,6 +539,7 @@ void vioapic_reset(struct domain *d)
{
vioapic->base_address = mp_ioapics[i].mpc_apicaddr;
vioapic->id = mp_ioapics[i].mpc_apicid;
+vioapic->base_gsi = io_apic_gsi_base(i);
}
vioapic->nr_pins = nr_pins;
vioapic->domain = d;
@@ -601,7 +587,12 @@ int vioapic_init(struct domain *d)
nr_gsis += nr_pins;
}
-ASSERT(hvm_domain_irq(d)->nr_gsis == nr_gsis);
+/*
+ * NB: hvm_domain_irq(d)->nr_gsis is actually the highest GSI + 1, but
+ * there might be holes in this range (ie: GSIs that don't belong to any
+ * vIO APIC).
+ */
+ASSERT(hvm_domain_irq(d)->nr_gsis >= nr_gsis);
d->arch.hvm_domain.nr_vioapics = nr_vioapics;
vioapic_reset(d);
diff --git a/xen/include/asm-x86/hvm/vioapic.h
b/xen/include/asm-x86/hvm/vioapic.h
index 8ec91d2bd1..2ceb60eaef 100644
--- a/xen/include/asm-x86/
