| Message ID | 20170327101823.99368-6-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 27.03.17 at 12:18, <roger.pau@citrix.com> wrote: > Changes since v1: > - Constify some parameters. Considering this I'm surprised the first thing I have to ask is ... > --- a/xen/arch/x86/hvm/vioapic.c > +++ b/xen/arch/x86/hvm/vioapic.c > @@ -44,6 +44,54 @@ > > static void vioapic_deliver(struct hvm_vioapic *vioapic, int irq); > > +static struct hvm_vioapic *addr_vioapic(struct domain *d, unsigned long addr) ... const? The more that you have it like this ... > +struct hvm_vioapic *gsi_vioapic(const struct domain *d, unsigned int gsi, ... here. > + unsigned int *base_gsi) All callers of the function really want the pin number rather than the base GSI - why don't you make the function provide that instead? > +{ > + unsigned int i; > + *base_gsi = 0; Blank line between declaration(s) and statement(s) please. > +static unsigned int pin_gsi(const struct domain *d, > + const struct hvm_vioapic *vioapic, > + unsigned int pin) > +{ > + const struct hvm_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; > +} Didn't we settle on having a function base_gsi(), with callers adding in the pin number as needed? The only reason the function here takes pin as a parameter is this final addition. Also shouldn't this function somehow guard itself against overrunning the array? > @@ -275,16 +327,17 @@ static inline int pit_channel0_enabled(void) > return pt_active(¤t->domain->arch.vpit.pt0); > } > > -static void vioapic_deliver(struct hvm_vioapic *vioapic, int irq) > +static void vioapic_deliver(struct hvm_vioapic *vioapic, int pin) Occasionally we pass around negative IRQ numbers, but if this really is a pin number, I think the type wants changing to unsigned int - the more that it is being used as an array index below. > @@ -454,33 +517,70 @@ HVM_REGISTER_SAVE_RESTORE(IOAPIC, ioapic_save, ioapic_load, 1, HVMSR_PER_DOM); > > void vioapic_reset(struct domain *d) > { > - struct hvm_vioapic *vioapic = domain_vioapic(d); > - uint32_t nr_pins = vioapic->nr_pins; > - int i; > + unsigned int i; > > if ( !has_vioapic(d) ) > + { > + ASSERT(!d->arch.hvm_domain.nr_vioapics); > return; > + } > > - memset(vioapic, 0, hvm_vioapic_size(nr_pins)); > - vioapic->domain = d; > - vioapic->nr_pins = nr_pins; > - for ( i = 0; i < nr_pins; i++ ) > - vioapic->redirtbl[i].fields.mask = 1; > - vioapic->base_address = VIOAPIC_DEFAULT_BASE_ADDRESS; > + for ( i = 0; i < d->arch.hvm_domain.nr_vioapics; i++ ) > + { > + struct hvm_vioapic *vioapic = domain_vioapic(d, i); > + unsigned int j, nr_pins = vioapic->nr_pins; > + > + memset(vioapic, 0, hvm_vioapic_size(nr_pins)); > + for ( j = 0; j < nr_pins; j++ ) > + vioapic->redirtbl[j].fields.mask = 1; > + vioapic->base_address = VIOAPIC_DEFAULT_BASE_ADDRESS + > + VIOAPIC_MEM_LENGTH * i; > + vioapic->id = i; > + vioapic->nr_pins = nr_pins; > + vioapic->domain = d; > + } > +} Is this function validly reachable for Dom0? If not, better leave it alone (adding a nr_vioapics == 1 check), as at least the base address calculation looks rather suspicious (there shouldn't be multiple IO-APICs on a single page). If so, that same memory address calculation is actively wrong in the Dom0 case. > +static void vioapic_free(const struct domain *d, unsigned int nr_vioapics) > +{ > + unsigned int i; > + > + for ( i = 0; i < nr_vioapics; i++) > + { > + if ( domain_vioapic(d, i) == NULL ) > + break; Is this if() really needed? > int vioapic_init(struct domain *d) > { > + unsigned int i, nr_vioapics = 1; > + > if ( !has_vioapic(d) ) > + { > + ASSERT(!d->arch.hvm_domain.nr_vioapics); > return 0; > + } > > if ( (d->arch.hvm_domain.vioapic == NULL) && > ((d->arch.hvm_domain.vioapic = > - xmalloc_bytes(hvm_vioapic_size(VIOAPIC_NUM_PINS))) == NULL) ) > + xzalloc_array(struct hvm_vioapic *, nr_vioapics)) == NULL) ) > return -ENOMEM; > > - d->arch.hvm_domain.vioapic->domain = d; > - d->arch.hvm_domain.vioapic->nr_pins = VIOAPIC_NUM_PINS; > + for ( i = 0; i < nr_vioapics; i++ ) > + { > + if ( (domain_vioapic(d, i) = > + xmalloc_bytes(hvm_vioapic_size(VIOAPIC_NUM_PINS))) == NULL ) > + { > + vioapic_free(d, nr_vioapics); > + return -ENOMEM; > + } > + domain_vioapic(d, i)->nr_pins = VIOAPIC_NUM_PINS; > + } > + > + d->arch.hvm_domain.nr_vioapics = nr_vioapics; > vioapic_reset(d); These adjustments again don't look right for nr_vioapics > 1, so I wonder whether again this function wouldn't better be left alone. Alternatively, if Dom0 needs to come here, a comment is going to be needed to explain the (temporary) wrong setting of nr_pins. > @@ -91,13 +92,22 @@ 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, &base_gsi); > + if ( !vioapic ) > + { > + gdprintk(XENLOG_WARNING, "Invalid GSI (%u) for platform timer\n", gsi); > + domain_crash(v->domain); > + return -1; > + } Now that you have this check, ... > @@ -110,9 +120,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, &base_gsi); > > return (((pic_imr & (1 << (isa_irq & 7))) || !vlapic_accept_pic_intr(v)) && > - domain_vioapic(v->domain)->redirtbl[gsi].fields.mask); > + vioapic->redirtbl[gsi - base_gsi].fields.mask); > } ... why is there still none here? Jan
On Tue, Mar 28, 2017 at 04:32:05AM -0600, Jan Beulich wrote: > >>> On 27.03.17 at 12:18, <roger.pau@citrix.com> wrote: > > Changes since v1: > > - Constify some parameters. > > Considering this I'm surprised the first thing I have to ask is ... > > > --- a/xen/arch/x86/hvm/vioapic.c > > +++ b/xen/arch/x86/hvm/vioapic.c > > @@ -44,6 +44,54 @@ > > > > static void vioapic_deliver(struct hvm_vioapic *vioapic, int irq); > > > > +static struct hvm_vioapic *addr_vioapic(struct domain *d, unsigned long addr) > > ... const? The more that you have it like this ... Shame, I guess I missed this one, sorry. > > +struct hvm_vioapic *gsi_vioapic(const struct domain *d, unsigned int gsi, > > ... here. > > > + unsigned int *base_gsi) > > All callers of the function really want the pin number rather than > the base GSI - why don't you make the function provide that > instead? Done. > > +static unsigned int pin_gsi(const struct domain *d, > > + const struct hvm_vioapic *vioapic, > > + unsigned int pin) > > +{ > > + const struct hvm_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; > > +} > > Didn't we settle on having a function base_gsi(), with callers > adding in the pin number as needed? The only reason the function > here takes pin as a parameter is this final addition. > > Also shouldn't this function somehow guard itself against > overrunning the array? Done for both. > > @@ -275,16 +327,17 @@ static inline int pit_channel0_enabled(void) > > return pt_active(¤t->domain->arch.vpit.pt0); > > } > > > > -static void vioapic_deliver(struct hvm_vioapic *vioapic, int irq) > > +static void vioapic_deliver(struct hvm_vioapic *vioapic, int pin) > > Occasionally we pass around negative IRQ numbers, but if this > really is a pin number, I think the type wants changing to unsigned > int - the more that it is being used as an array index below. Fixed. > > @@ -454,33 +517,70 @@ HVM_REGISTER_SAVE_RESTORE(IOAPIC, ioapic_save, ioapic_load, 1, HVMSR_PER_DOM); > > > > void vioapic_reset(struct domain *d) > > { > > - struct hvm_vioapic *vioapic = domain_vioapic(d); > > - uint32_t nr_pins = vioapic->nr_pins; > > - int i; > > + unsigned int i; > > > > if ( !has_vioapic(d) ) > > + { > > + ASSERT(!d->arch.hvm_domain.nr_vioapics); > > return; > > + } > > > > - memset(vioapic, 0, hvm_vioapic_size(nr_pins)); > > - vioapic->domain = d; > > - vioapic->nr_pins = nr_pins; > > - for ( i = 0; i < nr_pins; i++ ) > > - vioapic->redirtbl[i].fields.mask = 1; > > - vioapic->base_address = VIOAPIC_DEFAULT_BASE_ADDRESS; > > + for ( i = 0; i < d->arch.hvm_domain.nr_vioapics; i++ ) > > + { > > + struct hvm_vioapic *vioapic = domain_vioapic(d, i); > > + unsigned int j, nr_pins = vioapic->nr_pins; > > + > > + memset(vioapic, 0, hvm_vioapic_size(nr_pins)); > > + for ( j = 0; j < nr_pins; j++ ) > > + vioapic->redirtbl[j].fields.mask = 1; > > + vioapic->base_address = VIOAPIC_DEFAULT_BASE_ADDRESS + > > + VIOAPIC_MEM_LENGTH * i; > > + vioapic->id = i; > > + vioapic->nr_pins = nr_pins; > > + vioapic->domain = d; > > + } > > +} > > Is this function validly reachable for Dom0? If not, better leave it > alone (adding a nr_vioapics == 1 check), as at least the base > address calculation looks rather suspicious (there shouldn't be > multiple IO-APICs on a single page). If so, that same memory > address calculation is actively wrong in the Dom0 case. Yes, this is called from vioapic_init in order to set the initial vIO APIC state and mask all the redir entries. For the Dom0 case we use what's found on the native MADT tables. For now I could fix that by adding: BUILD_BUG_ON(VIOAPIC_MEM_LENGTH > PAGE_SIZE); And then use PAGE_SIZE above instead of VIOAPIC_MEM_LENGTH. > > +static void vioapic_free(const struct domain *d, unsigned int nr_vioapics) > > +{ > > + unsigned int i; > > + > > + for ( i = 0; i < nr_vioapics; i++) > > + { > > + if ( domain_vioapic(d, i) == NULL ) > > + break; > > Is this if() really needed? Not at all, thanks for pointing it out. > > int vioapic_init(struct domain *d) > > { > > + unsigned int i, nr_vioapics = 1; > > + > > if ( !has_vioapic(d) ) > > + { > > + ASSERT(!d->arch.hvm_domain.nr_vioapics); > > return 0; > > + } > > > > if ( (d->arch.hvm_domain.vioapic == NULL) && > > ((d->arch.hvm_domain.vioapic = > > - xmalloc_bytes(hvm_vioapic_size(VIOAPIC_NUM_PINS))) == NULL) ) > > + xzalloc_array(struct hvm_vioapic *, nr_vioapics)) == NULL) ) > > return -ENOMEM; > > > > - d->arch.hvm_domain.vioapic->domain = d; > > - d->arch.hvm_domain.vioapic->nr_pins = VIOAPIC_NUM_PINS; > > + for ( i = 0; i < nr_vioapics; i++ ) > > + { > > + if ( (domain_vioapic(d, i) = > > + xmalloc_bytes(hvm_vioapic_size(VIOAPIC_NUM_PINS))) == NULL ) > > + { > > + vioapic_free(d, nr_vioapics); > > + return -ENOMEM; > > + } > > + domain_vioapic(d, i)->nr_pins = VIOAPIC_NUM_PINS; > > + } > > + > > + d->arch.hvm_domain.nr_vioapics = nr_vioapics; > > vioapic_reset(d); > > These adjustments again don't look right for nr_vioapics > 1, so I > wonder whether again this function wouldn't better be left alone. > Alternatively, if Dom0 needs to come here, a comment is going to > be needed to explain the (temporary) wrong setting of nr_pins. Yes, Dom0 will need to come here. If it's cleaner I could have two different init functions, one for DomU and one for Dom0, and call them from vioapic_init. Or add something like: ASSERT(is_hardware_domain(d) || nr_vioapics == 1); (the same could be added to vioapic_reset). > > @@ -91,13 +92,22 @@ 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, &base_gsi); > > + if ( !vioapic ) > > + { > > + gdprintk(XENLOG_WARNING, "Invalid GSI (%u) for platform timer\n", gsi); > > + domain_crash(v->domain); > > + return -1; > > + } > > Now that you have this check, ... > > > @@ -110,9 +120,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, &base_gsi); > > > > return (((pic_imr & (1 << (isa_irq & 7))) || !vlapic_accept_pic_intr(v)) && > > - domain_vioapic(v->domain)->redirtbl[gsi].fields.mask); > > + vioapic->redirtbl[gsi - base_gsi].fields.mask); > > } > > ... why is there still none here? Fixed. Thanks, Roger.
>>> On 28.03.17 at 13:40, <roger.pau@citrix.com> wrote: > On Tue, Mar 28, 2017 at 04:32:05AM -0600, Jan Beulich wrote: >> >>> On 27.03.17 at 12:18, <roger.pau@citrix.com> wrote: >> > @@ -454,33 +517,70 @@ HVM_REGISTER_SAVE_RESTORE(IOAPIC, ioapic_save, ioapic_load, 1, HVMSR_PER_DOM); >> > >> > void vioapic_reset(struct domain *d) >> > { >> > - struct hvm_vioapic *vioapic = domain_vioapic(d); >> > - uint32_t nr_pins = vioapic->nr_pins; >> > - int i; >> > + unsigned int i; >> > >> > if ( !has_vioapic(d) ) >> > + { >> > + ASSERT(!d->arch.hvm_domain.nr_vioapics); >> > return; >> > + } >> > >> > - memset(vioapic, 0, hvm_vioapic_size(nr_pins)); >> > - vioapic->domain = d; >> > - vioapic->nr_pins = nr_pins; >> > - for ( i = 0; i < nr_pins; i++ ) >> > - vioapic->redirtbl[i].fields.mask = 1; >> > - vioapic->base_address = VIOAPIC_DEFAULT_BASE_ADDRESS; >> > + for ( i = 0; i < d->arch.hvm_domain.nr_vioapics; i++ ) >> > + { >> > + struct hvm_vioapic *vioapic = domain_vioapic(d, i); >> > + unsigned int j, nr_pins = vioapic->nr_pins; >> > + >> > + memset(vioapic, 0, hvm_vioapic_size(nr_pins)); >> > + for ( j = 0; j < nr_pins; j++ ) >> > + vioapic->redirtbl[j].fields.mask = 1; >> > + vioapic->base_address = VIOAPIC_DEFAULT_BASE_ADDRESS + >> > + VIOAPIC_MEM_LENGTH * i; >> > + vioapic->id = i; >> > + vioapic->nr_pins = nr_pins; >> > + vioapic->domain = d; >> > + } >> > +} >> >> Is this function validly reachable for Dom0? If not, better leave it >> alone (adding a nr_vioapics == 1 check), as at least the base >> address calculation looks rather suspicious (there shouldn't be >> multiple IO-APICs on a single page). If so, that same memory >> address calculation is actively wrong in the Dom0 case. > > Yes, this is called from vioapic_init in order to set the initial vIO APIC > state and mask all the redir entries. For the Dom0 case we use what's found on > the native MADT tables. > > For now I could fix that by adding: > > BUILD_BUG_ON(VIOAPIC_MEM_LENGTH > PAGE_SIZE); > > And then use PAGE_SIZE above instead of VIOAPIC_MEM_LENGTH. Hmm - see my reply to patch 7. >> > int vioapic_init(struct domain *d) >> > { >> > + unsigned int i, nr_vioapics = 1; >> > + >> > if ( !has_vioapic(d) ) >> > + { >> > + ASSERT(!d->arch.hvm_domain.nr_vioapics); >> > return 0; >> > + } >> > >> > if ( (d->arch.hvm_domain.vioapic == NULL) && >> > ((d->arch.hvm_domain.vioapic = >> > - xmalloc_bytes(hvm_vioapic_size(VIOAPIC_NUM_PINS))) == NULL) ) >> > + xzalloc_array(struct hvm_vioapic *, nr_vioapics)) == NULL) ) >> > return -ENOMEM; >> > >> > - d->arch.hvm_domain.vioapic->domain = d; >> > - d->arch.hvm_domain.vioapic->nr_pins = VIOAPIC_NUM_PINS; >> > + for ( i = 0; i < nr_vioapics; i++ ) >> > + { >> > + if ( (domain_vioapic(d, i) = >> > + xmalloc_bytes(hvm_vioapic_size(VIOAPIC_NUM_PINS))) == NULL ) >> > + { >> > + vioapic_free(d, nr_vioapics); >> > + return -ENOMEM; >> > + } >> > + domain_vioapic(d, i)->nr_pins = VIOAPIC_NUM_PINS; >> > + } >> > + >> > + d->arch.hvm_domain.nr_vioapics = nr_vioapics; >> > vioapic_reset(d); >> >> These adjustments again don't look right for nr_vioapics > 1, so I >> wonder whether again this function wouldn't better be left alone. >> Alternatively, if Dom0 needs to come here, a comment is going to >> be needed to explain the (temporary) wrong setting of nr_pins. > > Yes, Dom0 will need to come here. If it's cleaner I could have two different > init functions, one for DomU and one for Dom0, and call them from > vioapic_init. > > Or add something like: > > ASSERT(is_hardware_domain(d) || nr_vioapics == 1); > > (the same could be added to vioapic_reset). Which is basically in line with what I've said in reply to patch 7. Jan
diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c index 5576db4ee8..daa791d3f4 100644 --- a/xen/arch/x86/hvm/dom0_build.c +++ b/xen/arch/x86/hvm/dom0_build.c @@ -729,7 +729,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 00048ad65d..327a9758e0 100644 --- a/xen/arch/x86/hvm/vioapic.c +++ b/xen/arch/x86/hvm/vioapic.c @@ -44,6 +44,54 @@ static void vioapic_deliver(struct hvm_vioapic *vioapic, int irq); +static struct hvm_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_vioapic *vioapic = domain_vioapic(d, i); + + if ( addr >= vioapic->base_address && + addr < vioapic->base_address + VIOAPIC_MEM_LENGTH ) + return vioapic; + } + + return NULL; +} + +struct hvm_vioapic *gsi_vioapic(const struct domain *d, unsigned int gsi, + unsigned int *base_gsi) +{ + unsigned int i; + *base_gsi = 0; + + 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 ) + return vioapic; + + *base_gsi += vioapic->nr_pins; + } + + return NULL; +} + +static unsigned int pin_gsi(const struct domain *d, + const struct hvm_vioapic *vioapic, + unsigned int pin) +{ + const struct hvm_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_vioapic *vioapic) { uint32_t result = 0; @@ -94,11 +142,14 @@ static int vioapic_read( struct vcpu *v, unsigned long addr, unsigned int length, unsigned long *pval) { - const struct hvm_vioapic *vioapic = domain_vioapic(v->domain); + const struct hvm_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: @@ -126,6 +177,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 = pin_gsi(d, vioapic, idx); spin_lock(&d->arch.hvm_domain.irq_lock); @@ -149,7 +201,7 @@ static void vioapic_write_redirent( *pent = ent; - if ( idx == 0 ) + if ( gsi == 0 ) { vlapic_adjust_i8259_target(d); } @@ -165,7 +217,7 @@ static void vioapic_write_redirent( spin_unlock(&d->arch.hvm_domain.irq_lock); - if ( idx == 0 || unmasked ) + if ( gsi == 0 || unmasked ) pt_may_unmask_irq(d, NULL); } @@ -215,7 +267,10 @@ static int vioapic_write( struct vcpu *v, unsigned long addr, unsigned int length, unsigned long val) { - struct hvm_vioapic *vioapic = domain_vioapic(v->domain); + struct hvm_vioapic *vioapic; + + vioapic = addr_vioapic(v->domain, addr); + ASSERT(vioapic); switch ( addr & 0xff ) { @@ -242,10 +297,7 @@ static int vioapic_write( static int vioapic_range(struct vcpu *v, unsigned long addr) { - struct hvm_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 = { @@ -275,16 +327,17 @@ static inline int pit_channel0_enabled(void) return pt_active(¤t->domain->arch.vpit.pt0); } -static void vioapic_deliver(struct hvm_vioapic *vioapic, int irq) +static void vioapic_deliver(struct hvm_vioapic *vioapic, int pin) { - 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; + 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 domain *d = vioapic_domain(vioapic); struct vlapic *target; struct vcpu *v; + unsigned int irq = pin_gsi(d, vioapic, pin); ASSERT(spin_is_locked(&d->arch.hvm_domain.irq_lock)); @@ -361,64 +414,72 @@ static void vioapic_deliver(struct hvm_vioapic *vioapic, int irq) void vioapic_irq_positive_edge(struct domain *d, unsigned int irq) { - struct hvm_vioapic *vioapic = domain_vioapic(d); + unsigned int base_gsi; + struct hvm_vioapic *vioapic = gsi_vioapic(d, irq, &base_gsi); + unsigned int pin = irq - base_gsi; 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; if ( ent->fields.trig_mode == VIOAPIC_EDGE_TRIG ) { - vioapic_deliver(vioapic, irq); + vioapic_deliver(vioapic, pin); } else if ( !ent->fields.remote_irr ) { ent->fields.remote_irr = 1; - vioapic_deliver(vioapic, irq); + vioapic_deliver(vioapic, pin); } } void vioapic_update_EOI(struct domain *d, u8 vector) { - struct hvm_vioapic *vioapic = domain_vioapic(d); struct hvm_irq *hvm_irq = hvm_domain_irq(d); 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_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(vioapic, 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(vioapic, j); + } } + base_gsi += vioapic->nr_pins; } spin_unlock(&d->arch.hvm_domain.irq_lock); @@ -426,12 +487,13 @@ void vioapic_update_EOI(struct domain *d, u8 vector) static int ioapic_save(struct domain *d, hvm_domain_context_t *h) { - struct hvm_vioapic *s = domain_vioapic(d); + struct hvm_vioapic *s = domain_vioapic(d, 0); if ( !has_vioapic(d) ) return 0; - if ( s->nr_pins != VIOAPIC_NUM_PINS ) + if ( s->nr_pins != VIOAPIC_NUM_PINS || + d->arch.hvm_domain.nr_vioapics != 1 ) return -EOPNOTSUPP; return hvm_save_entry(IOAPIC, 0, h, &s->base_address); @@ -439,12 +501,13 @@ static int ioapic_save(struct domain *d, hvm_domain_context_t *h) static int ioapic_load(struct domain *d, hvm_domain_context_t *h) { - struct hvm_vioapic *s = domain_vioapic(d); + struct hvm_vioapic *s = domain_vioapic(d, 0); if ( !has_vioapic(d) ) return -ENODEV; - if ( s->nr_pins != VIOAPIC_NUM_PINS ) + if ( s->nr_pins != VIOAPIC_NUM_PINS || + d->arch.hvm_domain.nr_vioapics != 1 ) return -EOPNOTSUPP; return hvm_load_entry(IOAPIC, h, &s->base_address); @@ -454,33 +517,70 @@ HVM_REGISTER_SAVE_RESTORE(IOAPIC, ioapic_save, ioapic_load, 1, HVMSR_PER_DOM); void vioapic_reset(struct domain *d) { - struct hvm_vioapic *vioapic = domain_vioapic(d); - uint32_t nr_pins = vioapic->nr_pins; - int i; + unsigned int i; if ( !has_vioapic(d) ) + { + ASSERT(!d->arch.hvm_domain.nr_vioapics); return; + } - memset(vioapic, 0, hvm_vioapic_size(nr_pins)); - vioapic->domain = d; - vioapic->nr_pins = nr_pins; - for ( i = 0; i < nr_pins; i++ ) - vioapic->redirtbl[i].fields.mask = 1; - vioapic->base_address = VIOAPIC_DEFAULT_BASE_ADDRESS; + for ( i = 0; i < d->arch.hvm_domain.nr_vioapics; i++ ) + { + struct hvm_vioapic *vioapic = domain_vioapic(d, i); + unsigned int j, nr_pins = vioapic->nr_pins; + + memset(vioapic, 0, hvm_vioapic_size(nr_pins)); + for ( j = 0; j < nr_pins; j++ ) + vioapic->redirtbl[j].fields.mask = 1; + vioapic->base_address = VIOAPIC_DEFAULT_BASE_ADDRESS + + VIOAPIC_MEM_LENGTH * i; + vioapic->id = i; + vioapic->nr_pins = nr_pins; + vioapic->domain = d; + } +} + +static void vioapic_free(const struct domain *d, unsigned int nr_vioapics) +{ + unsigned int i; + + for ( i = 0; i < nr_vioapics; i++) + { + if ( domain_vioapic(d, i) == NULL ) + break; + xfree(domain_vioapic(d, i)); + } + xfree(d->arch.hvm_domain.vioapic); } int vioapic_init(struct domain *d) { + unsigned int i, nr_vioapics = 1; + if ( !has_vioapic(d) ) + { + ASSERT(!d->arch.hvm_domain.nr_vioapics); return 0; + } if ( (d->arch.hvm_domain.vioapic == NULL) && ((d->arch.hvm_domain.vioapic = - xmalloc_bytes(hvm_vioapic_size(VIOAPIC_NUM_PINS))) == NULL) ) + xzalloc_array(struct hvm_vioapic *, nr_vioapics)) == NULL) ) return -ENOMEM; - d->arch.hvm_domain.vioapic->domain = d; - d->arch.hvm_domain.vioapic->nr_pins = VIOAPIC_NUM_PINS; + for ( i = 0; i < nr_vioapics; i++ ) + { + if ( (domain_vioapic(d, i) = + xmalloc_bytes(hvm_vioapic_size(VIOAPIC_NUM_PINS))) == NULL ) + { + vioapic_free(d, nr_vioapics); + return -ENOMEM; + } + domain_vioapic(d, i)->nr_pins = VIOAPIC_NUM_PINS; + } + + d->arch.hvm_domain.nr_vioapics = nr_vioapics; vioapic_reset(d); register_mmio_handler(d, &vioapic_mmio_ops); @@ -491,8 +591,10 @@ int vioapic_init(struct domain *d) void vioapic_deinit(struct domain *d) { if ( !has_vioapic(d) ) + { + ASSERT(!d->arch.hvm_domain.nr_vioapics); return; + } - xfree(d->arch.hvm_domain.vioapic); - d->arch.hvm_domain.vioapic = NULL; + vioapic_free(d, d->arch.hvm_domain.nr_vioapics); } diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c index 0590d6c69d..2653ba80fd 100644 --- a/xen/arch/x86/hvm/vlapic.c +++ b/xen/arch/x86/hvm/vlapic.c @@ -1107,7 +1107,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 5c48fdb4b5..97c5a13748 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_vioapic *vioapic; + unsigned int gsi, isa_irq, base_gsi; if ( pt->source == PTSRC_lapic ) return pt->irq; @@ -91,13 +92,22 @@ 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, &base_gsi); + if ( !vioapic ) + { + gdprintk(XENLOG_WARNING, "Invalid GSI (%u) for platform timer\n", gsi); + domain_crash(v->domain); + return -1; + } + + return vioapic->redirtbl[gsi - base_gsi].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, base_gsi; + struct hvm_vioapic *vioapic; uint8_t pic_imr; if ( pt->source == PTSRC_lapic ) @@ -110,9 +120,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, &base_gsi); return (((pic_imr & (1 << (isa_irq & 7))) || !vlapic_accept_pic_intr(v)) && - domain_vioapic(v->domain)->redirtbl[gsi].fields.mask); + vioapic->redirtbl[gsi - base_gsi].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 c3cca94a97..63b0d927ea 100644 --- a/xen/include/asm-x86/hvm/domain.h +++ b/xen/include/asm-x86/hvm/domain.h @@ -127,7 +127,8 @@ struct hvm_domain { spinlock_t irq_lock; struct hvm_irq *irq; struct hvm_hw_vpic vpic[2]; /* 0=master; 1=slave */ - struct hvm_vioapic *vioapic; + struct hvm_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 df8154390f..07455e2b61 100644 --- a/xen/include/asm-x86/hvm/vioapic.h +++ b/xen/include/asm-x86/hvm/vioapic.h @@ -58,8 +58,10 @@ struct hvm_vioapic { }; #define hvm_vioapic_size(cnt) offsetof(struct hvm_vioapic, redirtbl[cnt]) -#define domain_vioapic(d) ((d)->arch.hvm_domain.vioapic) +#define domain_vioapic(d, i) ((d)->arch.hvm_domain.vioapic[i]) #define vioapic_domain(v) ((v)->domain) +struct hvm_vioapic *gsi_vioapic(const struct domain *d, unsigned int gsi, + unsigned int *base_gsi); 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> --- Changes since v1: - Constify some parameters. - Make gsi_vioapic return the base GSI of the IO APIC. - Add checks to pt_irq_vector in order to prevent dereferencing NULL. --- xen/arch/x86/hvm/dom0_build.c | 2 +- xen/arch/x86/hvm/vioapic.c | 216 ++++++++++++++++++++++++++++---------- xen/arch/x86/hvm/vlapic.c | 2 +- xen/arch/x86/hvm/vpt.c | 19 +++- xen/include/asm-x86/hvm/domain.h | 3 +- xen/include/asm-x86/hvm/vioapic.h | 4 +- 6 files changed, 181 insertions(+), 65 deletions(-)