| Message ID | 20170223115217.32764-4-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 23.02.17 at 12:52, <roger.pau@citrix.com> wrote: > +struct hvm_hw_vioapic *gsi_vioapic(struct domain *d, unsigned int gsi, const struct domain *d ? > +static unsigned int pin_gsi(struct domain *d, struct hvm_hw_vioapic *vioapic, const for both? > + unsigned int pin) > +{ > + struct hvm_hw_vioapic *tmp; And here. Also wouldn't this function more naturally (i.e. more generally useful) simply return the base GSI, leaving it to the caller to add the pin number? > @@ -157,21 +210,22 @@ static void vioapic_write_redirent( > pent->fields.remote_irr = 0; > else if ( !ent.fields.mask && > !ent.fields.remote_irr && > - hvm_irq->gsi_assert_count[idx] ) > + hvm_irq->gsi_assert_count[gsi] ) Neither this nor any earlier patch modified the size of this array, afaics. > -static void vioapic_write_indirect(struct domain *d, uint32_t val) > +static void vioapic_write_indirect(struct domain *d, unsigned long addr, > + uint32_t val) > { > - struct hvm_hw_vioapic *vioapic = domain_vioapic(d); > + struct hvm_hw_vioapic *vioapic = addr_vioapic(d, addr); I think it would be better for the caller to pass the vioapic it already established (and ASSERT()ed). > @@ -430,11 +493,14 @@ void vioapic_update_EOI(struct domain *d, u8 vector) > > static int vioapic_save(struct domain *d, hvm_domain_context_t *h) > { > - struct hvm_hw_vioapic *vioapic = domain_vioapic(d); > + struct hvm_hw_vioapic *vioapic = domain_vioapic(d, 0); > > if ( !has_vioapic(d) ) > return 0; > > + if ( d->arch.hvm_domain.nr_vioapics != 1 ) > + return -ENOSYS; > + > if ( vioapic->nr_pins != VIOAPIC_NUM_PINS ) > return -ENOSYS; Perhaps merge the two if()s? Otherwise -EOPNOTSUPP again. > int vioapic_init(struct domain *d) > { > if ( !has_vioapic(d) ) > + { > + d->arch.hvm_domain.nr_vioapics = 0; I don't think this is needed - if anything you may want to again ASSERT() it being zero. > --- a/xen/arch/x86/hvm/vlapic.c > +++ b/xen/arch/x86/hvm/vlapic.c > @@ -1120,7 +1120,7 @@ static int __vlapic_accept_pic_intr(struct vcpu *v) > if ( !has_vioapic(d) ) > return 0; > > - redir0 = domain_vioapic(d)->redirtbl[0]; > + redir0 = domain_vioapic(d, 0)->redirtbl[0]; What if the first IO-APIC has less than 16 pins? > --- a/xen/arch/x86/hvm/vpt.c > +++ b/xen/arch/x86/hvm/vpt.c > @@ -78,7 +78,8 @@ void hvm_set_guest_time(struct vcpu *v, u64 guest_time) > static int pt_irq_vector(struct periodic_time *pt, enum hvm_intsrc src) > { > struct vcpu *v = pt->vcpu; > - unsigned int gsi, isa_irq; > + struct hvm_hw_vioapic *vioapic; > + unsigned int gsi, isa_irq, pin; > > if ( pt->source == PTSRC_lapic ) > return pt->irq; > @@ -91,13 +92,16 @@ static int pt_irq_vector(struct periodic_time *pt, enum hvm_intsrc src) > + (isa_irq & 7)); > > ASSERT(src == hvm_intsrc_lapic); > - return domain_vioapic(v->domain)->redirtbl[gsi].fields.vector; > + vioapic = gsi_vioapic(v->domain, gsi, &pin); > + > + return vioapic->redirtbl[pin].fields.vector; Please don't chance de-referencing NULL here and below. Jan
On Fri, Mar 03, 2017 at 10:06:03AM -0700, Jan Beulich wrote: > >>> On 23.02.17 at 12:52, <roger.pau@citrix.com> wrote: > > +struct hvm_hw_vioapic *gsi_vioapic(struct domain *d, unsigned int gsi, > > const struct domain *d ? > > > +static unsigned int pin_gsi(struct domain *d, struct hvm_hw_vioapic *vioapic, > > const for both? > > > + unsigned int pin) > > +{ > > + struct hvm_hw_vioapic *tmp; > > And here. Done for all the above. > Also wouldn't this function more naturally (i.e. more generally useful) > simply return the base GSI, leaving it to the caller to add the pin > number? I don't have a strong opinion. Most (if not all callers) seem to want the pin, but you can obtain that by just doing gsi - base_gsi, so I changed it to return base_gsi (which also simplifies the code). > > @@ -157,21 +210,22 @@ static void vioapic_write_redirent( > > pent->fields.remote_irr = 0; > > else if ( !ent.fields.mask && > > !ent.fields.remote_irr && > > - hvm_irq->gsi_assert_count[idx] ) > > + hvm_irq->gsi_assert_count[gsi] ) > > Neither this nor any earlier patch modified the size of this array, afaics. Hm, right. This is still hardcoded to VIOAPIC_NUM_PINS. I should change it in the earlier patch also. > > -static void vioapic_write_indirect(struct domain *d, uint32_t val) > > +static void vioapic_write_indirect(struct domain *d, unsigned long addr, > > + uint32_t val) > > { > > - struct hvm_hw_vioapic *vioapic = domain_vioapic(d); > > + struct hvm_hw_vioapic *vioapic = addr_vioapic(d, addr); > > I think it would be better for the caller to pass the vioapic it > already established (and ASSERT()ed). Done. Now that hvm_vioapic has a reference to struct domain I no longer need to pass the domain around. > > @@ -430,11 +493,14 @@ void vioapic_update_EOI(struct domain *d, u8 vector) > > > > static int vioapic_save(struct domain *d, hvm_domain_context_t *h) > > { > > - struct hvm_hw_vioapic *vioapic = domain_vioapic(d); > > + struct hvm_hw_vioapic *vioapic = domain_vioapic(d, 0); > > > > if ( !has_vioapic(d) ) > > return 0; > > > > + if ( d->arch.hvm_domain.nr_vioapics != 1 ) > > + return -ENOSYS; > > + > > if ( vioapic->nr_pins != VIOAPIC_NUM_PINS ) > > return -ENOSYS; > > Perhaps merge the two if()s? Otherwise -EOPNOTSUPP again. OK. > > int vioapic_init(struct domain *d) > > { > > if ( !has_vioapic(d) ) > > + { > > + d->arch.hvm_domain.nr_vioapics = 0; > > I don't think this is needed - if anything you may want to again > ASSERT() it being zero. Done. > > --- a/xen/arch/x86/hvm/vlapic.c > > +++ b/xen/arch/x86/hvm/vlapic.c > > @@ -1120,7 +1120,7 @@ static int __vlapic_accept_pic_intr(struct vcpu *v) > > if ( !has_vioapic(d) ) > > return 0; > > > > - redir0 = domain_vioapic(d)->redirtbl[0]; > > + redir0 = domain_vioapic(d, 0)->redirtbl[0]; > > What if the first IO-APIC has less than 16 pins? I'm not sure I understand what this piece of code is trying to do. Why is PIC relying on the value of the first redirection entry of the IO APIC? Aren't the PIC and the IO APIC modes mutually exclusive? I would appreciate if you could provide some reference here. > > --- a/xen/arch/x86/hvm/vpt.c > > +++ b/xen/arch/x86/hvm/vpt.c > > @@ -78,7 +78,8 @@ void hvm_set_guest_time(struct vcpu *v, u64 guest_time) > > static int pt_irq_vector(struct periodic_time *pt, enum hvm_intsrc src) > > { > > struct vcpu *v = pt->vcpu; > > - unsigned int gsi, isa_irq; > > + struct hvm_hw_vioapic *vioapic; > > + unsigned int gsi, isa_irq, pin; > > > > if ( pt->source == PTSRC_lapic ) > > return pt->irq; > > @@ -91,13 +92,16 @@ static int pt_irq_vector(struct periodic_time *pt, enum hvm_intsrc src) > > + (isa_irq & 7)); > > > > ASSERT(src == hvm_intsrc_lapic); > > - return domain_vioapic(v->domain)->redirtbl[gsi].fields.vector; > > + vioapic = gsi_vioapic(v->domain, gsi, &pin); > > + > > + return vioapic->redirtbl[pin].fields.vector; > > Please don't chance de-referencing NULL here and below. Done, I've added an ASSERT. Roger.
>>> On 20.03.17 at 19:27, <roger.pau@citrix.com> wrote: > On Fri, Mar 03, 2017 at 10:06:03AM -0700, Jan Beulich wrote: >> >>> On 23.02.17 at 12:52, <roger.pau@citrix.com> wrote: >> > --- a/xen/arch/x86/hvm/vlapic.c >> > +++ b/xen/arch/x86/hvm/vlapic.c >> > @@ -1120,7 +1120,7 @@ static int __vlapic_accept_pic_intr(struct vcpu *v) >> > if ( !has_vioapic(d) ) >> > return 0; >> > >> > - redir0 = domain_vioapic(d)->redirtbl[0]; >> > + redir0 = domain_vioapic(d, 0)->redirtbl[0]; >> >> What if the first IO-APIC has less than 16 pins? > > I'm not sure I understand what this piece of code is trying to do. Why is PIC > relying on the value of the first redirection entry of the IO APIC? Aren't the > PIC and the IO APIC modes mutually exclusive? > > I would appreciate if you could provide some reference here. Well, we're both in the same position here: All there is as explanation is the code plus its history in git. Looking at the code I see that it uses RTE 0 for ExtINT handling, which is reasonable. After all that's the main connection between PIC and IO-APIC (so called Virtual Wire Mode B iirc). See also our own code dealing with this (just look for ExtINT in io_apic.c). >> > @@ -91,13 +92,16 @@ static int pt_irq_vector(struct periodic_time *pt, enum hvm_intsrc src) >> > + (isa_irq & 7)); >> > >> > ASSERT(src == hvm_intsrc_lapic); >> > - return domain_vioapic(v->domain)->redirtbl[gsi].fields.vector; >> > + vioapic = gsi_vioapic(v->domain, gsi, &pin); >> > + >> > + return vioapic->redirtbl[pin].fields.vector; >> >> Please don't chance de-referencing NULL here and below. > > Done, I've added an ASSERT. How about release builds then? Jan
On Tue, Mar 21, 2017 at 01:56:51AM -0600, Jan Beulich wrote: > >>> On 20.03.17 at 19:27, <roger.pau@citrix.com> wrote: > > On Fri, Mar 03, 2017 at 10:06:03AM -0700, Jan Beulich wrote: > >> >>> On 23.02.17 at 12:52, <roger.pau@citrix.com> wrote: > >> > --- a/xen/arch/x86/hvm/vlapic.c > >> > +++ b/xen/arch/x86/hvm/vlapic.c > >> > @@ -1120,7 +1120,7 @@ static int __vlapic_accept_pic_intr(struct vcpu *v) > >> > if ( !has_vioapic(d) ) > >> > return 0; > >> > > >> > - redir0 = domain_vioapic(d)->redirtbl[0]; > >> > + redir0 = domain_vioapic(d, 0)->redirtbl[0]; > >> > >> What if the first IO-APIC has less than 16 pins? > > > > I'm not sure I understand what this piece of code is trying to do. Why is PIC > > relying on the value of the first redirection entry of the IO APIC? Aren't the > > PIC and the IO APIC modes mutually exclusive? > > > > I would appreciate if you could provide some reference here. > > Well, we're both in the same position here: All there is as explanation > is the code plus its history in git. Looking at the code I see that it > uses RTE 0 for ExtINT handling, which is reasonable. After all that's > the main connection between PIC and IO-APIC (so called Virtual Wire > Mode B iirc). See also our own code dealing with this (just look for > ExtINT in io_apic.c). Oh, so that's hardcoded to pin 0 of the first IO APIC (that's where the 8259A-master output pin is hooked up AFAICT), in which case the code above is right, and doesn't depend on whether IO APIC #0 has less than 16 pins. > >> > @@ -91,13 +92,16 @@ static int pt_irq_vector(struct periodic_time *pt, enum hvm_intsrc src) > >> > + (isa_irq & 7)); > >> > > >> > ASSERT(src == hvm_intsrc_lapic); > >> > - return domain_vioapic(v->domain)->redirtbl[gsi].fields.vector; > >> > + vioapic = gsi_vioapic(v->domain, gsi, &pin); > >> > + > >> > + return vioapic->redirtbl[pin].fields.vector; > >> > >> Please don't chance de-referencing NULL here and below. > > > > Done, I've added an ASSERT. > > How about release builds then? OK, I can add a BUG_ON, but maybe it would be better to add a domain_crash and suitable printk in case this triggers. AFAICT if Xen happens to trigger this it would mean that the gsi of the platform timer is higher than the maximum gsi, which at the moment it's impossible. Roger.
>>> On 21.03.17 at 11:52, <roger.pau@citrix.com> wrote: > On Tue, Mar 21, 2017 at 01:56:51AM -0600, Jan Beulich wrote: >> >>> On 20.03.17 at 19:27, <roger.pau@citrix.com> wrote: >> > On Fri, Mar 03, 2017 at 10:06:03AM -0700, Jan Beulich wrote: >> >> >>> On 23.02.17 at 12:52, <roger.pau@citrix.com> wrote: >> >> > @@ -91,13 +92,16 @@ static int pt_irq_vector(struct periodic_time *pt, enum hvm_intsrc src) >> >> > + (isa_irq & 7)); >> >> > >> >> > ASSERT(src == hvm_intsrc_lapic); >> >> > - return domain_vioapic(v->domain)->redirtbl[gsi].fields.vector; >> >> > + vioapic = gsi_vioapic(v->domain, gsi, &pin); >> >> > + >> >> > + return vioapic->redirtbl[pin].fields.vector; >> >> >> >> Please don't chance de-referencing NULL here and below. >> > >> > Done, I've added an ASSERT. >> >> How about release builds then? > > OK, I can add a BUG_ON, but maybe it would be better to add a domain_crash and > suitable printk in case this triggers. The latter, please, plus returning the spurious vector (as you still need to return something). Jan
On Tue, Mar 21, 2017 at 07:45:13AM -0600, Jan Beulich wrote: > >>> On 21.03.17 at 11:52, <roger.pau@citrix.com> wrote: > > On Tue, Mar 21, 2017 at 01:56:51AM -0600, Jan Beulich wrote: > >> >>> On 20.03.17 at 19:27, <roger.pau@citrix.com> wrote: > >> > On Fri, Mar 03, 2017 at 10:06:03AM -0700, Jan Beulich wrote: > >> >> >>> On 23.02.17 at 12:52, <roger.pau@citrix.com> wrote: > >> >> > @@ -91,13 +92,16 @@ static int pt_irq_vector(struct periodic_time *pt, enum hvm_intsrc src) > >> >> > + (isa_irq & 7)); > >> >> > > >> >> > ASSERT(src == hvm_intsrc_lapic); > >> >> > - return domain_vioapic(v->domain)->redirtbl[gsi].fields.vector; > >> >> > + vioapic = gsi_vioapic(v->domain, gsi, &pin); > >> >> > + > >> >> > + return vioapic->redirtbl[pin].fields.vector; > >> >> > >> >> Please don't chance de-referencing NULL here and below. > >> > > >> > Done, I've added an ASSERT. > >> > >> How about release builds then? > > > > OK, I can add a BUG_ON, but maybe it would be better to add a domain_crash and > > suitable printk in case this triggers. > > The latter, please, plus returning the spurious vector (as you still > need to return something). Do we really need the domain_crash? I've added a gdprintk and returned -1. Roger.
>>> On 21.03.17 at 14:59, <roger.pau@citrix.com> wrote: > On Tue, Mar 21, 2017 at 07:45:13AM -0600, Jan Beulich wrote: >> >>> On 21.03.17 at 11:52, <roger.pau@citrix.com> wrote: >> > On Tue, Mar 21, 2017 at 01:56:51AM -0600, Jan Beulich wrote: >> >> >>> On 20.03.17 at 19:27, <roger.pau@citrix.com> wrote: >> >> > On Fri, Mar 03, 2017 at 10:06:03AM -0700, Jan Beulich wrote: >> >> >> >>> On 23.02.17 at 12:52, <roger.pau@citrix.com> wrote: >> >> >> > @@ -91,13 +92,16 @@ static int pt_irq_vector(struct periodic_time *pt, > enum hvm_intsrc src) >> >> >> > + (isa_irq & 7)); >> >> >> > >> >> >> > ASSERT(src == hvm_intsrc_lapic); >> >> >> > - return domain_vioapic(v->domain)->redirtbl[gsi].fields.vector; >> >> >> > + vioapic = gsi_vioapic(v->domain, gsi, &pin); >> >> >> > + >> >> >> > + return vioapic->redirtbl[pin].fields.vector; >> >> >> >> >> >> Please don't chance de-referencing NULL here and below. >> >> > >> >> > Done, I've added an ASSERT. >> >> >> >> How about release builds then? >> > >> > OK, I can add a BUG_ON, but maybe it would be better to add a domain_crash and >> > suitable printk in case this triggers. >> >> The latter, please, plus returning the spurious vector (as you still >> need to return something). > > Do we really need the domain_crash? It's not strictly needed, but allowing the guest to continue to run may make the (highly theoretical) issue harder to debug, the more that ... > I've added a gdprintk and returned -1. ... gdprintk() expands to nothing in release builds. Returning -1, otoh, seems fine for all (two) present callers. Jan
diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c index 932fa4e..dad3b4e 100644 --- a/xen/arch/x86/domain_build.c +++ b/xen/arch/x86/domain_build.c @@ -2317,7 +2317,7 @@ static int __init pvh_setup_acpi_madt(struct domain *d, paddr_t *addr) io_apic = (void *)(madt + 1); io_apic->header.type = ACPI_MADT_TYPE_IO_APIC; io_apic->header.length = sizeof(*io_apic); - io_apic->id = domain_vioapic(d)->id; + io_apic->id = domain_vioapic(d, 0)->id; io_apic->address = VIOAPIC_DEFAULT_BASE_ADDRESS; x2apic = (void *)(io_apic + 1); diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c index f469cbf..6421971 100644 --- a/xen/arch/x86/hvm/vioapic.c +++ b/xen/arch/x86/hvm/vioapic.c @@ -44,6 +44,56 @@ static void vioapic_deliver(struct domain *d, int irq); +static struct hvm_hw_vioapic *addr_vioapic(struct domain *d, + unsigned long addr) +{ + unsigned int i; + + for ( i = 0; i < d->arch.hvm_domain.nr_vioapics; i++ ) + { + struct hvm_hw_vioapic *vioapic = domain_vioapic(d, i); + + if ( addr >= vioapic->base_address && + addr < vioapic->base_address + VIOAPIC_MEM_LENGTH ) + return vioapic; + } + + return NULL; +} + +struct hvm_hw_vioapic *gsi_vioapic(struct domain *d, unsigned int gsi, + unsigned int *pin) +{ + unsigned int i, base_gsi = 0; + + for ( i = 0; i < d->arch.hvm_domain.nr_vioapics; i++ ) + { + struct hvm_hw_vioapic *vioapic = domain_vioapic(d, i); + + if ( gsi >= base_gsi && gsi < base_gsi + vioapic->nr_pins ) + { + if ( pin != NULL ) + *pin = gsi - base_gsi; + return vioapic; + } + base_gsi += vioapic->nr_pins; + } + + return NULL; +} + +static unsigned int pin_gsi(struct domain *d, struct hvm_hw_vioapic *vioapic, + unsigned int pin) +{ + struct hvm_hw_vioapic *tmp; + unsigned int base_gsi = 0; + + for ( tmp = domain_vioapic(d, 0); tmp != vioapic; tmp++ ) + base_gsi += tmp->nr_pins; + + return base_gsi + pin; +} + static uint32_t vioapic_read_indirect(const struct hvm_hw_vioapic *vioapic) { uint32_t result = 0; @@ -94,11 +144,14 @@ static int vioapic_read( struct vcpu *v, unsigned long addr, unsigned int length, unsigned long *pval) { - const struct hvm_hw_vioapic *vioapic = domain_vioapic(v->domain); + const struct hvm_hw_vioapic *vioapic; uint32_t result; HVM_DBG_LOG(DBG_LEVEL_IOAPIC, "addr %lx", addr); + vioapic = addr_vioapic(v->domain, addr); + ASSERT(vioapic); + switch ( addr & 0xff ) { case VIOAPIC_REG_SELECT: @@ -119,13 +172,13 @@ static int vioapic_read( } static void vioapic_write_redirent( - struct domain *d, unsigned int idx, + struct domain *d, struct hvm_hw_vioapic *vioapic, unsigned int idx, int top_word, uint32_t val) { - struct hvm_hw_vioapic *vioapic = domain_vioapic(d); struct hvm_irq *hvm_irq = &d->arch.hvm_domain.irq; union vioapic_redir_entry *pent, ent; int unmasked = 0; + unsigned int gsi = pin_gsi(d, vioapic, idx); spin_lock(&d->arch.hvm_domain.irq_lock); @@ -149,7 +202,7 @@ static void vioapic_write_redirent( *pent = ent; - if ( idx == 0 ) + if ( gsi == 0 ) { vlapic_adjust_i8259_target(d); } @@ -157,21 +210,22 @@ static void vioapic_write_redirent( pent->fields.remote_irr = 0; else if ( !ent.fields.mask && !ent.fields.remote_irr && - hvm_irq->gsi_assert_count[idx] ) + hvm_irq->gsi_assert_count[gsi] ) { pent->fields.remote_irr = 1; - vioapic_deliver(d, idx); + vioapic_deliver(d, gsi); } spin_unlock(&d->arch.hvm_domain.irq_lock); - if ( idx == 0 || unmasked ) + if ( gsi == 0 || unmasked ) pt_may_unmask_irq(d, NULL); } -static void vioapic_write_indirect(struct domain *d, uint32_t val) +static void vioapic_write_indirect(struct domain *d, unsigned long addr, + uint32_t val) { - struct hvm_hw_vioapic *vioapic = domain_vioapic(d); + struct hvm_hw_vioapic *vioapic = addr_vioapic(d, addr); switch ( vioapic->ioregsel ) { @@ -205,7 +259,8 @@ static void vioapic_write_indirect(struct domain *d, uint32_t val) break; } - vioapic_write_redirent(d, redir_index, vioapic->ioregsel&1, val); + vioapic_write_redirent(d, vioapic, redir_index, vioapic->ioregsel&1, + val); break; } } @@ -215,7 +270,10 @@ static int vioapic_write( struct vcpu *v, unsigned long addr, unsigned int length, unsigned long val) { - struct hvm_hw_vioapic *vioapic = domain_vioapic(v->domain); + struct hvm_hw_vioapic *vioapic; + + vioapic = addr_vioapic(v->domain, addr); + ASSERT(vioapic); switch ( addr & 0xff ) { @@ -224,7 +282,7 @@ static int vioapic_write( break; case VIOAPIC_REG_WINDOW: - vioapic_write_indirect(v->domain, val); + vioapic_write_indirect(v->domain, addr, val); break; #if VIOAPIC_VERSION_ID >= 0x20 @@ -242,10 +300,7 @@ static int vioapic_write( static int vioapic_range(struct vcpu *v, unsigned long addr) { - struct hvm_hw_vioapic *vioapic = domain_vioapic(v->domain); - - return ((addr >= vioapic->base_address && - (addr < vioapic->base_address + VIOAPIC_MEM_LENGTH))); + return !!addr_vioapic(v->domain, addr); } static const struct hvm_mmio_ops vioapic_mmio_ops = { @@ -277,12 +332,13 @@ static inline int pit_channel0_enabled(void) static void vioapic_deliver(struct domain *d, int irq) { - struct hvm_hw_vioapic *vioapic = domain_vioapic(d); - uint16_t dest = vioapic->redirtbl[irq].fields.dest_id; - uint8_t dest_mode = vioapic->redirtbl[irq].fields.dest_mode; - uint8_t delivery_mode = vioapic->redirtbl[irq].fields.delivery_mode; - uint8_t vector = vioapic->redirtbl[irq].fields.vector; - uint8_t trig_mode = vioapic->redirtbl[irq].fields.trig_mode; + unsigned int pin; + struct hvm_hw_vioapic *vioapic = gsi_vioapic(d, irq, &pin); + uint16_t dest = vioapic->redirtbl[pin].fields.dest_id; + uint8_t dest_mode = vioapic->redirtbl[pin].fields.dest_mode; + uint8_t delivery_mode = vioapic->redirtbl[pin].fields.delivery_mode; + uint8_t vector = vioapic->redirtbl[pin].fields.vector; + uint8_t trig_mode = vioapic->redirtbl[pin].fields.trig_mode; struct vlapic *target; struct vcpu *v; @@ -361,17 +417,18 @@ static void vioapic_deliver(struct domain *d, int irq) void vioapic_irq_positive_edge(struct domain *d, unsigned int irq) { - struct hvm_hw_vioapic *vioapic = domain_vioapic(d); + unsigned int pin; + struct hvm_hw_vioapic *vioapic = gsi_vioapic(d, irq, &pin); union vioapic_redir_entry *ent; ASSERT(has_vioapic(d)); HVM_DBG_LOG(DBG_LEVEL_IOAPIC, "irq %x", irq); - ASSERT(irq < vioapic->nr_pins); + ASSERT(pin < vioapic->nr_pins); ASSERT(spin_is_locked(&d->arch.hvm_domain.irq_lock)); - ent = &vioapic->redirtbl[irq]; + ent = &vioapic->redirtbl[pin]; if ( ent->fields.mask ) return; @@ -388,37 +445,43 @@ void vioapic_irq_positive_edge(struct domain *d, unsigned int irq) void vioapic_update_EOI(struct domain *d, u8 vector) { - struct hvm_hw_vioapic *vioapic = domain_vioapic(d); struct hvm_irq *hvm_irq = &d->arch.hvm_domain.irq; union vioapic_redir_entry *ent; - int gsi; + unsigned int i, base_gsi = 0; ASSERT(has_vioapic(d)); spin_lock(&d->arch.hvm_domain.irq_lock); - for ( gsi = 0; gsi < vioapic->nr_pins; gsi++ ) + for ( i = 0; i < d->arch.hvm_domain.nr_vioapics; i++ ) { - ent = &vioapic->redirtbl[gsi]; - if ( ent->fields.vector != vector ) - continue; - - ent->fields.remote_irr = 0; + struct hvm_hw_vioapic *vioapic = domain_vioapic(d, i); + unsigned int j; - if ( iommu_enabled ) + for ( j = 0; j < vioapic->nr_pins; j++ ) { - spin_unlock(&d->arch.hvm_domain.irq_lock); - hvm_dpci_eoi(d, gsi, 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[gsi] ) - { - ent->fields.remote_irr = 1; - vioapic_deliver(d, gsi); + ent = &vioapic->redirtbl[j]; + if ( ent->fields.vector != vector ) + continue; + + ent->fields.remote_irr = 0; + + if ( iommu_enabled ) + { + spin_unlock(&d->arch.hvm_domain.irq_lock); + hvm_dpci_eoi(d, base_gsi + j, 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 + j] ) + { + ent->fields.remote_irr = 1; + vioapic_deliver(d, base_gsi + j); + } } + base_gsi += vioapic->nr_pins; } spin_unlock(&d->arch.hvm_domain.irq_lock); @@ -430,11 +493,14 @@ void vioapic_update_EOI(struct domain *d, u8 vector) static int vioapic_save(struct domain *d, hvm_domain_context_t *h) { - struct hvm_hw_vioapic *vioapic = domain_vioapic(d); + struct hvm_hw_vioapic *vioapic = domain_vioapic(d, 0); if ( !has_vioapic(d) ) return 0; + if ( d->arch.hvm_domain.nr_vioapics != 1 ) + return -ENOSYS; + if ( vioapic->nr_pins != VIOAPIC_NUM_PINS ) return -ENOSYS; @@ -456,7 +522,7 @@ static int vioapic_load(struct domain *d, hvm_domain_context_t *h) unsigned int ioapic_nr = hvm_load_instance(h); const struct hvm_save_descriptor *desc; struct hvm_hw_vioapic_compat *ioapic_compat; - struct hvm_hw_vioapic *ioapic = domain_vioapic(d); + struct hvm_hw_vioapic *ioapic = domain_vioapic(d, 0); if ( !has_vioapic(d) ) return -ENODEV; @@ -546,40 +612,53 @@ __initcall(vioapic_register_save_and_restore); void vioapic_reset(struct domain *d) { - struct hvm_hw_vioapic *vioapic = domain_vioapic(d); unsigned int i; if ( !has_vioapic(d) ) + { + ASSERT(!d->arch.hvm_domain.nr_vioapics); return; + } - memset(vioapic->redirtbl, 0, - sizeof(*vioapic->redirtbl) * vioapic->nr_pins); - for ( i = 0; i < vioapic->nr_pins; i++ ) - vioapic->redirtbl[i].fields.mask = 1; - vioapic->base_address = VIOAPIC_DEFAULT_BASE_ADDRESS; - vioapic->id = 0; - vioapic->ioregsel = 0; + for ( i = 0; i < d->arch.hvm_domain.nr_vioapics; i++ ) + { + struct hvm_hw_vioapic *vioapic = domain_vioapic(d, i); + unsigned int j; + + memset(vioapic->redirtbl, 0, + sizeof(*vioapic->redirtbl) * vioapic->nr_pins); + for ( j = 0; j < vioapic->nr_pins; j++ ) + vioapic->redirtbl[j].fields.mask = 1; + vioapic->base_address = VIOAPIC_DEFAULT_BASE_ADDRESS + + VIOAPIC_MEM_LENGTH * i; + vioapic->id = i; + vioapic->ioregsel = 0; + } } int vioapic_init(struct domain *d) { if ( !has_vioapic(d) ) + { + d->arch.hvm_domain.nr_vioapics = 0; return 0; + } if ( (d->arch.hvm_domain.vioapic == NULL) && ((d->arch.hvm_domain.vioapic = xmalloc(struct hvm_hw_vioapic)) == NULL) ) return -ENOMEM; - domain_vioapic(d)->redirtbl = xmalloc_array(union vioapic_redir_entry, - VIOAPIC_NUM_PINS); - if ( !domain_vioapic(d)->redirtbl ) + domain_vioapic(d, 0)->redirtbl = xmalloc_array(union vioapic_redir_entry, + VIOAPIC_NUM_PINS); + if ( !domain_vioapic(d, 0)->redirtbl ) { xfree(d->arch.hvm_domain.vioapic); return -ENOMEM; } - domain_vioapic(d)->nr_pins = VIOAPIC_NUM_PINS; + domain_vioapic(d, 0)->nr_pins = VIOAPIC_NUM_PINS; + d->arch.hvm_domain.nr_vioapics = 1; vioapic_reset(d); register_mmio_handler(d, &vioapic_mmio_ops); @@ -589,9 +668,16 @@ int vioapic_init(struct domain *d) void vioapic_deinit(struct domain *d) { + unsigned int i; + if ( !has_vioapic(d) ) + { + ASSERT(!d->arch.hvm_domain.nr_vioapics); return; + } + for ( i = 0; i < d->arch.hvm_domain.nr_vioapics; i++ ) + xfree(domain_vioapic(d, i)->redirtbl); xfree(d->arch.hvm_domain.vioapic); d->arch.hvm_domain.vioapic = NULL; } diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c index 3fa3727..187f106 100644 --- a/xen/arch/x86/hvm/vlapic.c +++ b/xen/arch/x86/hvm/vlapic.c @@ -1120,7 +1120,7 @@ static int __vlapic_accept_pic_intr(struct vcpu *v) if ( !has_vioapic(d) ) return 0; - redir0 = domain_vioapic(d)->redirtbl[0]; + redir0 = domain_vioapic(d, 0)->redirtbl[0]; /* We deliver 8259 interrupts to the appropriate CPU as follows. */ return ((/* IOAPIC pin0 is unmasked and routing to this LAPIC? */ diff --git a/xen/arch/x86/hvm/vpt.c b/xen/arch/x86/hvm/vpt.c index 5c48fdb..fd30dfb 100644 --- a/xen/arch/x86/hvm/vpt.c +++ b/xen/arch/x86/hvm/vpt.c @@ -78,7 +78,8 @@ void hvm_set_guest_time(struct vcpu *v, u64 guest_time) static int pt_irq_vector(struct periodic_time *pt, enum hvm_intsrc src) { struct vcpu *v = pt->vcpu; - unsigned int gsi, isa_irq; + struct hvm_hw_vioapic *vioapic; + unsigned int gsi, isa_irq, pin; if ( pt->source == PTSRC_lapic ) return pt->irq; @@ -91,13 +92,16 @@ static int pt_irq_vector(struct periodic_time *pt, enum hvm_intsrc src) + (isa_irq & 7)); ASSERT(src == hvm_intsrc_lapic); - return domain_vioapic(v->domain)->redirtbl[gsi].fields.vector; + vioapic = gsi_vioapic(v->domain, gsi, &pin); + + return vioapic->redirtbl[pin].fields.vector; } static int pt_irq_masked(struct periodic_time *pt) { struct vcpu *v = pt->vcpu; - unsigned int gsi, isa_irq; + unsigned int gsi, isa_irq, pin; + struct hvm_hw_vioapic *vioapic; uint8_t pic_imr; if ( pt->source == PTSRC_lapic ) @@ -110,9 +114,10 @@ static int pt_irq_masked(struct periodic_time *pt) isa_irq = pt->irq; gsi = hvm_isa_irq_to_gsi(isa_irq); pic_imr = v->domain->arch.hvm_domain.vpic[isa_irq >> 3].imr; + vioapic = gsi_vioapic(v->domain, gsi, &pin); return (((pic_imr & (1 << (isa_irq & 7))) || !vlapic_accept_pic_intr(v)) && - domain_vioapic(v->domain)->redirtbl[gsi].fields.mask); + vioapic->redirtbl[pin].fields.mask); } static void pt_lock(struct periodic_time *pt) diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-x86/hvm/domain.h index 5cf3398..cf1be37 100644 --- a/xen/include/asm-x86/hvm/domain.h +++ b/xen/include/asm-x86/hvm/domain.h @@ -128,6 +128,7 @@ struct hvm_domain { struct hvm_irq irq; struct hvm_hw_vpic vpic[2]; /* 0=master; 1=slave */ struct hvm_hw_vioapic *vioapic; + unsigned int nr_vioapics; struct hvm_hw_stdvga stdvga; /* diff --git a/xen/include/asm-x86/hvm/vioapic.h b/xen/include/asm-x86/hvm/vioapic.h index 6762835..6889b57 100644 --- a/xen/include/asm-x86/hvm/vioapic.h +++ b/xen/include/asm-x86/hvm/vioapic.h @@ -47,7 +47,9 @@ #define VIOAPIC_REG_ARB_ID 0x02 /* x86 IOAPIC only */ #define VIOAPIC_REG_RTE0 0x10 -#define domain_vioapic(d) ((d)->arch.hvm_domain.vioapic) +#define domain_vioapic(d, i) (&(d)->arch.hvm_domain.vioapic[i]) +struct hvm_hw_vioapic *gsi_vioapic(struct domain *d, unsigned int gsi, + unsigned int *pin); int vioapic_init(struct domain *d); void vioapic_deinit(struct domain *d);
Add support for multiple vIO APICs on the same domain, thus turning d->arch.hvm_domain.vioapic into an array of vIO APIC control structures. Note that this functionality is not exposed to unprivileged guests, and will only be used by PVHv2 Dom0. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> --- xen/arch/x86/domain_build.c | 2 +- xen/arch/x86/hvm/vioapic.c | 204 +++++++++++++++++++++++++++----------- xen/arch/x86/hvm/vlapic.c | 2 +- xen/arch/x86/hvm/vpt.c | 13 ++- xen/include/asm-x86/hvm/domain.h | 1 + xen/include/asm-x86/hvm/vioapic.h | 4 +- 6 files changed, 160 insertions(+), 66 deletions(-)