| Message ID | 20170504153123.1204-6-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Andre, On 04/05/17 16:31, Andre Przywara wrote: > Currently we store the priority for newly triggered IRQs in the rank > structure. To get eventually rid of this structure, move this value > into the struct pending_irq. This one already contains a priority value, > but we have to keep the two apart, as the priority for guest visible > IRQs must not change while they are in a VCPU. > This patch introduces a framework to get some per-IRQ information for a > number of interrupts and collate them into one register. Similarily s/Similarily/Similarly/ > there is the opposite function to spread values from one register into > multiple pending_irq's. Also, the last paragraph is a call to split the patch in two: 1) Introducing the framework 2) Move priority from irq_rank to struct pending_irq > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > xen/arch/arm/vgic-v2.c | 33 +++++++++-------------------- > xen/arch/arm/vgic-v3.c | 33 ++++++++++------------------- > xen/arch/arm/vgic.c | 53 ++++++++++++++++++++++++++++++++++------------ > xen/include/asm-arm/vgic.h | 17 ++++++--------- > 4 files changed, 67 insertions(+), 69 deletions(-) > > diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c > index dc9f95b..5c59fb4 100644 > --- a/xen/arch/arm/vgic-v2.c > +++ b/xen/arch/arm/vgic-v2.c > @@ -171,6 +171,7 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info, > struct vgic_irq_rank *rank; > int gicd_reg = (int)(info->gpa - v->domain->arch.vgic.dbase); > unsigned long flags; > + unsigned int irq; s/irq/virq/ > > perfc_incr(vgicd_reads); > > @@ -250,22 +251,10 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info, > goto read_as_zero; > > case VRANGE32(GICD_IPRIORITYR, GICD_IPRIORITYRN): > - { > - uint32_t ipriorityr; > - > if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width; > - rank = vgic_rank_offset(v, 8, gicd_reg - GICD_IPRIORITYR, DABT_WORD); > - if ( rank == NULL ) goto read_as_zero; > - > - vgic_lock_rank(v, rank, flags); > - ipriorityr = rank->ipriorityr[REG_RANK_INDEX(8, > - gicd_reg - GICD_IPRIORITYR, > - DABT_WORD)]; > - vgic_unlock_rank(v, rank, flags); > - *r = vgic_reg32_extract(ipriorityr, info); > - > + irq = gicd_reg - GICD_IPRIORITYR; This variable name does not make sense. This is not rely an irq but an offset. > + *r = vgic_reg32_extract(gather_irq_info_priority(v, irq), info); > return 1; > - } > > case VREG32(0x7FC): > goto read_reserved; > @@ -415,6 +404,7 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info, > int gicd_reg = (int)(info->gpa - v->domain->arch.vgic.dbase); > uint32_t tr; > unsigned long flags; > + unsigned int irq; s/irq/virq/ > > perfc_incr(vgicd_writes); > > @@ -499,17 +489,14 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info, > > case VRANGE32(GICD_IPRIORITYR, GICD_IPRIORITYRN): > { > - uint32_t *ipriorityr; > + uint32_t ipriorityr; > > if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width; > - rank = vgic_rank_offset(v, 8, gicd_reg - GICD_IPRIORITYR, DABT_WORD); > - if ( rank == NULL) goto write_ignore; > - vgic_lock_rank(v, rank, flags); > - ipriorityr = &rank->ipriorityr[REG_RANK_INDEX(8, > - gicd_reg - GICD_IPRIORITYR, > - DABT_WORD)]; > - vgic_reg32_update(ipriorityr, r, info); > - vgic_unlock_rank(v, rank, flags); > + irq = gicd_reg - GICD_IPRIORITYR; > + > + ipriorityr = gather_irq_info_priority(v, irq); > + vgic_reg32_update(&ipriorityr, r, info); > + scatter_irq_info_priority(v, irq, ipriorityr); > return 1; > } > > diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c > index d10757a..10db939 100644 > --- a/xen/arch/arm/vgic-v3.c > +++ b/xen/arch/arm/vgic-v3.c > @@ -476,6 +476,7 @@ static int __vgic_v3_distr_common_mmio_read(const char *name, struct vcpu *v, > struct hsr_dabt dabt = info->dabt; > struct vgic_irq_rank *rank; > unsigned long flags; > + unsigned int irq; s/irq/virq/ > > switch ( reg ) > { > @@ -513,22 +514,11 @@ static int __vgic_v3_distr_common_mmio_read(const char *name, struct vcpu *v, > goto read_as_zero; > > case VRANGE32(GICD_IPRIORITYR, GICD_IPRIORITYRN): > - { > - uint32_t ipriorityr; > - > if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width; > - rank = vgic_rank_offset(v, 8, reg - GICD_IPRIORITYR, DABT_WORD); > - if ( rank == NULL ) goto read_as_zero; > - > - vgic_lock_rank(v, rank, flags); > - ipriorityr = rank->ipriorityr[REG_RANK_INDEX(8, reg - GICD_IPRIORITYR, > - DABT_WORD)]; > - vgic_unlock_rank(v, rank, flags); > - > - *r = vgic_reg32_extract(ipriorityr, info); > - > + irq = reg - GICD_IPRIORITYR; > + if ( irq >= v->domain->arch.vgic.nr_spis + 32 ) goto read_as_zero; This check will likely belong to an helper. > + *r = vgic_reg32_extract(gather_irq_info_priority(v, irq), info); > return 1; > - } > > case VRANGE32(GICD_ICFGR, GICD_ICFGRN): > { > @@ -572,6 +562,7 @@ static int __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v, > struct vgic_irq_rank *rank; > uint32_t tr; > unsigned long flags; > + unsigned int irq; s/irq/virq/ > > switch ( reg ) > { > @@ -630,16 +621,14 @@ static int __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v, > > case VRANGE32(GICD_IPRIORITYR, GICD_IPRIORITYRN): > { > - uint32_t *ipriorityr; > + uint32_t ipriorityr; > > if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width; > - rank = vgic_rank_offset(v, 8, reg - GICD_IPRIORITYR, DABT_WORD); > - if ( rank == NULL ) goto write_ignore; > - vgic_lock_rank(v, rank, flags); > - ipriorityr = &rank->ipriorityr[REG_RANK_INDEX(8, reg - GICD_IPRIORITYR, > - DABT_WORD)]; > - vgic_reg32_update(ipriorityr, r, info); > - vgic_unlock_rank(v, rank, flags); > + irq = reg - GICD_IPRIORITYR; > + if ( irq >= v->domain->arch.vgic.nr_spis + 32 ) goto write_ignore; Ditto. > + ipriorityr = gather_irq_info_priority(v, irq); > + vgic_reg32_update(&ipriorityr, r, info); > + scatter_irq_info_priority(v, irq, ipriorityr); > return 1; > } > > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c > index 44363bb..68eef47 100644 > --- a/xen/arch/arm/vgic.c > +++ b/xen/arch/arm/vgic.c > @@ -225,19 +225,49 @@ struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int virq) > return v->domain->vcpu[target]; > } > > -static int vgic_get_virq_priority(struct vcpu *v, unsigned int virq) > +static uint8_t extract_priority(struct pending_irq *p) Please append vgic_ > { > - struct vgic_irq_rank *rank = vgic_rank_irq(v, virq); > - unsigned long flags; > - int priority; > + return p->new_priority; > +} > + > +static void set_priority(struct pending_irq *p, uint8_t prio) Ditto. > +{ > + p->new_priority = prio; > +} > + > > - vgic_lock_rank(v, rank, flags); > - priority = rank->priority[virq & INTERRUPT_RANK_MASK]; > - vgic_unlock_rank(v, rank, flags); > +#define DEFINE_GATHER_IRQ_INFO(name, get_val, shift) \ > +uint32_t gather_irq_info_##name(struct vcpu *v, unsigned int irq) \ The name of this function are too generic. This should at least contain vgic. Also I would like to keep the naming consistent with what we did with irouter and itargetr. I.e fetch/store. Lastly, irq is confusing? How many irqs will it gather/scatter? > +{ \ > + uint32_t ret = 0, i; \ newline here. > + for ( i = 0; i < (32 / shift); i++ ) \ Why 32? > + { \ > + struct pending_irq *p = irq_to_pending(v, irq + i); \ > + spin_lock(&p->lock); \ I am fairly surprised that you don't need to disable the interrupts here. the pending_irq lock will be used in vgic_inject_irq which will be called in the interrupt context. > + ret |= get_val(p) << (shift * i); \ > + spin_unlock(&p->lock); \ > + } \ > + return ret; \ > +} > > - return priority; > +#define DEFINE_SCATTER_IRQ_INFO(name, set_val, shift) \ Why do you need to define two separate macros? Both could be done at the same time. > +void scatter_irq_info_##name(struct vcpu *v, unsigned int irq, \ Same remarks as above. > + unsigned int value) \ > +{ \ > + unsigned int i; \ newline here. > + for ( i = 0; i < (32 / shift); i++ ) \ > + { \ > + struct pending_irq *p = irq_to_pending(v, irq + i); \ > + spin_lock(&p->lock); \ > + set_val(p, (value >> (shift * i)) & ((1 << shift) - 1)); \ > + spin_unlock(&p->lock); \ > + } \ > } I do think those functions have to be introduced in a separate patch. > > +/* grep fodder: gather_irq_info_priority, scatter_irq_info_priority below */ > +DEFINE_GATHER_IRQ_INFO(priority, extract_priority, 8) > +DEFINE_SCATTER_IRQ_INFO(priority, set_priority, 8) > + > bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq) > { > unsigned long flags; > @@ -471,13 +501,10 @@ void vgic_clear_pending_irqs(struct vcpu *v) > > void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq) > { > - uint8_t priority; > struct pending_irq *iter, *n = irq_to_pending(v, virq); > unsigned long flags; > bool running; > > - priority = vgic_get_virq_priority(v, virq); > - > spin_lock_irqsave(&v->arch.vgic.lock, flags); > > /* vcpu offline */ > @@ -497,7 +524,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq) > goto out; > } > > - n->priority = priority; > + n->priority = n->new_priority; > > /* the irq is enabled */ > if ( test_bit(GIC_IRQ_GUEST_ENABLED, &n->status) ) > @@ -505,7 +532,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq) > > list_for_each_entry ( iter, &v->arch.vgic.inflight_irqs, inflight ) > { > - if ( iter->priority > priority ) > + if ( iter->priority > n->priority ) > { > list_add_tail(&n->inflight, &iter->inflight); > spin_unlock(&n->lock); > diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h > index e7322fc..38a5e76 100644 > --- a/xen/include/asm-arm/vgic.h > +++ b/xen/include/asm-arm/vgic.h > @@ -71,7 +71,8 @@ struct pending_irq > unsigned int irq; > #define GIC_INVALID_LR (uint8_t)~0 > uint8_t lr; > - uint8_t priority; > + uint8_t priority; /* the priority of the currently inflight IRQ */ > + uint8_t new_priority; /* the priority of newly triggered IRQs */ > /* inflight is used to append instances of pending_irq to > * vgic.inflight_irqs */ > struct list_head inflight; > @@ -104,16 +105,6 @@ struct vgic_irq_rank { > uint32_t icfg[2]; > > /* > - * Provide efficient access to the priority of an vIRQ while keeping > - * the emulation simple. > - * Note, this is working fine as long as Xen is using little endian. > - */ > - union { > - uint8_t priority[32]; > - uint32_t ipriorityr[8]; > - }; > - > - /* > * It's more convenient to store a target VCPU per vIRQ > * than the register ITARGETSR/IROUTER itself. > * Use atomic operations to read/write the vcpu fields to avoid > @@ -179,6 +170,10 @@ static inline int REG_RANK_NR(int b, uint32_t n) > } > } > > +uint32_t gather_irq_info_priority(struct vcpu *v, unsigned int irq); > +void scatter_irq_info_priority(struct vcpu *v, unsigned int irq, > + unsigned int value); > + > #define VGIC_REG_MASK(size) ((~0UL) >> (BITS_PER_LONG - ((1 << (size)) * 8))) > > /* > Cheers,
On 04/05/17 17:39, Julien Grall wrote: > Hi Andre, > > On 04/05/17 16:31, Andre Przywara wrote: >> Currently we store the priority for newly triggered IRQs in the rank >> structure. To get eventually rid of this structure, move this value >> into the struct pending_irq. This one already contains a priority value, >> but we have to keep the two apart, as the priority for guest visible >> IRQs must not change while they are in a VCPU. >> This patch introduces a framework to get some per-IRQ information for a >> number of interrupts and collate them into one register. Similarily > > s/Similarily/Similarly/ > >> there is the opposite function to spread values from one register into >> multiple pending_irq's. > > Also, the last paragraph is a call to split the patch in two: > 1) Introducing the framework > 2) Move priority from irq_rank to struct pending_irq > >> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >> --- >> xen/arch/arm/vgic-v2.c | 33 +++++++++-------------------- >> xen/arch/arm/vgic-v3.c | 33 ++++++++++------------------- >> xen/arch/arm/vgic.c | 53 >> ++++++++++++++++++++++++++++++++++------------ >> xen/include/asm-arm/vgic.h | 17 ++++++--------- >> 4 files changed, 67 insertions(+), 69 deletions(-) >> >> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c >> index dc9f95b..5c59fb4 100644 >> --- a/xen/arch/arm/vgic-v2.c >> +++ b/xen/arch/arm/vgic-v2.c >> @@ -171,6 +171,7 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, >> mmio_info_t *info, >> struct vgic_irq_rank *rank; >> int gicd_reg = (int)(info->gpa - v->domain->arch.vgic.dbase); >> unsigned long flags; >> + unsigned int irq; > > s/irq/virq/ > >> >> perfc_incr(vgicd_reads); >> >> @@ -250,22 +251,10 @@ static int vgic_v2_distr_mmio_read(struct vcpu >> *v, mmio_info_t *info, >> goto read_as_zero; >> >> case VRANGE32(GICD_IPRIORITYR, GICD_IPRIORITYRN): >> - { >> - uint32_t ipriorityr; >> - >> if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto >> bad_width; >> - rank = vgic_rank_offset(v, 8, gicd_reg - GICD_IPRIORITYR, >> DABT_WORD); >> - if ( rank == NULL ) goto read_as_zero; >> - >> - vgic_lock_rank(v, rank, flags); >> - ipriorityr = rank->ipriorityr[REG_RANK_INDEX(8, >> - gicd_reg - >> GICD_IPRIORITYR, >> - DABT_WORD)]; >> - vgic_unlock_rank(v, rank, flags); >> - *r = vgic_reg32_extract(ipriorityr, info); >> - >> + irq = gicd_reg - GICD_IPRIORITYR; Actually there are an issue with this code. You don't handle correctly byte access and vgic_reg32_extract expects the interrupt IHMO, this kind of problem could be handled directly in gather_irq_info_priority by passing the offset. See how we deal in vgic_fetch_itarger. The behavior of those functions should really be explained the commit message. Also I don't see any check to prevent reading interrupts outside of the one supported by the vGIC for this domain.
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c index dc9f95b..5c59fb4 100644 --- a/xen/arch/arm/vgic-v2.c +++ b/xen/arch/arm/vgic-v2.c @@ -171,6 +171,7 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info, struct vgic_irq_rank *rank; int gicd_reg = (int)(info->gpa - v->domain->arch.vgic.dbase); unsigned long flags; + unsigned int irq; perfc_incr(vgicd_reads); @@ -250,22 +251,10 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info, goto read_as_zero; case VRANGE32(GICD_IPRIORITYR, GICD_IPRIORITYRN): - { - uint32_t ipriorityr; - if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width; - rank = vgic_rank_offset(v, 8, gicd_reg - GICD_IPRIORITYR, DABT_WORD); - if ( rank == NULL ) goto read_as_zero; - - vgic_lock_rank(v, rank, flags); - ipriorityr = rank->ipriorityr[REG_RANK_INDEX(8, - gicd_reg - GICD_IPRIORITYR, - DABT_WORD)]; - vgic_unlock_rank(v, rank, flags); - *r = vgic_reg32_extract(ipriorityr, info); - + irq = gicd_reg - GICD_IPRIORITYR; + *r = vgic_reg32_extract(gather_irq_info_priority(v, irq), info); return 1; - } case VREG32(0x7FC): goto read_reserved; @@ -415,6 +404,7 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info, int gicd_reg = (int)(info->gpa - v->domain->arch.vgic.dbase); uint32_t tr; unsigned long flags; + unsigned int irq; perfc_incr(vgicd_writes); @@ -499,17 +489,14 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info, case VRANGE32(GICD_IPRIORITYR, GICD_IPRIORITYRN): { - uint32_t *ipriorityr; + uint32_t ipriorityr; if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width; - rank = vgic_rank_offset(v, 8, gicd_reg - GICD_IPRIORITYR, DABT_WORD); - if ( rank == NULL) goto write_ignore; - vgic_lock_rank(v, rank, flags); - ipriorityr = &rank->ipriorityr[REG_RANK_INDEX(8, - gicd_reg - GICD_IPRIORITYR, - DABT_WORD)]; - vgic_reg32_update(ipriorityr, r, info); - vgic_unlock_rank(v, rank, flags); + irq = gicd_reg - GICD_IPRIORITYR; + + ipriorityr = gather_irq_info_priority(v, irq); + vgic_reg32_update(&ipriorityr, r, info); + scatter_irq_info_priority(v, irq, ipriorityr); return 1; } diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c index d10757a..10db939 100644 --- a/xen/arch/arm/vgic-v3.c +++ b/xen/arch/arm/vgic-v3.c @@ -476,6 +476,7 @@ static int __vgic_v3_distr_common_mmio_read(const char *name, struct vcpu *v, struct hsr_dabt dabt = info->dabt; struct vgic_irq_rank *rank; unsigned long flags; + unsigned int irq; switch ( reg ) { @@ -513,22 +514,11 @@ static int __vgic_v3_distr_common_mmio_read(const char *name, struct vcpu *v, goto read_as_zero; case VRANGE32(GICD_IPRIORITYR, GICD_IPRIORITYRN): - { - uint32_t ipriorityr; - if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width; - rank = vgic_rank_offset(v, 8, reg - GICD_IPRIORITYR, DABT_WORD); - if ( rank == NULL ) goto read_as_zero; - - vgic_lock_rank(v, rank, flags); - ipriorityr = rank->ipriorityr[REG_RANK_INDEX(8, reg - GICD_IPRIORITYR, - DABT_WORD)]; - vgic_unlock_rank(v, rank, flags); - - *r = vgic_reg32_extract(ipriorityr, info); - + irq = reg - GICD_IPRIORITYR; + if ( irq >= v->domain->arch.vgic.nr_spis + 32 ) goto read_as_zero; + *r = vgic_reg32_extract(gather_irq_info_priority(v, irq), info); return 1; - } case VRANGE32(GICD_ICFGR, GICD_ICFGRN): { @@ -572,6 +562,7 @@ static int __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v, struct vgic_irq_rank *rank; uint32_t tr; unsigned long flags; + unsigned int irq; switch ( reg ) { @@ -630,16 +621,14 @@ static int __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v, case VRANGE32(GICD_IPRIORITYR, GICD_IPRIORITYRN): { - uint32_t *ipriorityr; + uint32_t ipriorityr; if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width; - rank = vgic_rank_offset(v, 8, reg - GICD_IPRIORITYR, DABT_WORD); - if ( rank == NULL ) goto write_ignore; - vgic_lock_rank(v, rank, flags); - ipriorityr = &rank->ipriorityr[REG_RANK_INDEX(8, reg - GICD_IPRIORITYR, - DABT_WORD)]; - vgic_reg32_update(ipriorityr, r, info); - vgic_unlock_rank(v, rank, flags); + irq = reg - GICD_IPRIORITYR; + if ( irq >= v->domain->arch.vgic.nr_spis + 32 ) goto write_ignore; + ipriorityr = gather_irq_info_priority(v, irq); + vgic_reg32_update(&ipriorityr, r, info); + scatter_irq_info_priority(v, irq, ipriorityr); return 1; } diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c index 44363bb..68eef47 100644 --- a/xen/arch/arm/vgic.c +++ b/xen/arch/arm/vgic.c @@ -225,19 +225,49 @@ struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int virq) return v->domain->vcpu[target]; } -static int vgic_get_virq_priority(struct vcpu *v, unsigned int virq) +static uint8_t extract_priority(struct pending_irq *p) { - struct vgic_irq_rank *rank = vgic_rank_irq(v, virq); - unsigned long flags; - int priority; + return p->new_priority; +} + +static void set_priority(struct pending_irq *p, uint8_t prio) +{ + p->new_priority = prio; +} + - vgic_lock_rank(v, rank, flags); - priority = rank->priority[virq & INTERRUPT_RANK_MASK]; - vgic_unlock_rank(v, rank, flags); +#define DEFINE_GATHER_IRQ_INFO(name, get_val, shift) \ +uint32_t gather_irq_info_##name(struct vcpu *v, unsigned int irq) \ +{ \ + uint32_t ret = 0, i; \ + for ( i = 0; i < (32 / shift); i++ ) \ + { \ + struct pending_irq *p = irq_to_pending(v, irq + i); \ + spin_lock(&p->lock); \ + ret |= get_val(p) << (shift * i); \ + spin_unlock(&p->lock); \ + } \ + return ret; \ +} - return priority; +#define DEFINE_SCATTER_IRQ_INFO(name, set_val, shift) \ +void scatter_irq_info_##name(struct vcpu *v, unsigned int irq, \ + unsigned int value) \ +{ \ + unsigned int i; \ + for ( i = 0; i < (32 / shift); i++ ) \ + { \ + struct pending_irq *p = irq_to_pending(v, irq + i); \ + spin_lock(&p->lock); \ + set_val(p, (value >> (shift * i)) & ((1 << shift) - 1)); \ + spin_unlock(&p->lock); \ + } \ } +/* grep fodder: gather_irq_info_priority, scatter_irq_info_priority below */ +DEFINE_GATHER_IRQ_INFO(priority, extract_priority, 8) +DEFINE_SCATTER_IRQ_INFO(priority, set_priority, 8) + bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq) { unsigned long flags; @@ -471,13 +501,10 @@ void vgic_clear_pending_irqs(struct vcpu *v) void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq) { - uint8_t priority; struct pending_irq *iter, *n = irq_to_pending(v, virq); unsigned long flags; bool running; - priority = vgic_get_virq_priority(v, virq); - spin_lock_irqsave(&v->arch.vgic.lock, flags); /* vcpu offline */ @@ -497,7 +524,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq) goto out; } - n->priority = priority; + n->priority = n->new_priority; /* the irq is enabled */ if ( test_bit(GIC_IRQ_GUEST_ENABLED, &n->status) ) @@ -505,7 +532,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq) list_for_each_entry ( iter, &v->arch.vgic.inflight_irqs, inflight ) { - if ( iter->priority > priority ) + if ( iter->priority > n->priority ) { list_add_tail(&n->inflight, &iter->inflight); spin_unlock(&n->lock); diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h index e7322fc..38a5e76 100644 --- a/xen/include/asm-arm/vgic.h +++ b/xen/include/asm-arm/vgic.h @@ -71,7 +71,8 @@ struct pending_irq unsigned int irq; #define GIC_INVALID_LR (uint8_t)~0 uint8_t lr; - uint8_t priority; + uint8_t priority; /* the priority of the currently inflight IRQ */ + uint8_t new_priority; /* the priority of newly triggered IRQs */ /* inflight is used to append instances of pending_irq to * vgic.inflight_irqs */ struct list_head inflight; @@ -104,16 +105,6 @@ struct vgic_irq_rank { uint32_t icfg[2]; /* - * Provide efficient access to the priority of an vIRQ while keeping - * the emulation simple. - * Note, this is working fine as long as Xen is using little endian. - */ - union { - uint8_t priority[32]; - uint32_t ipriorityr[8]; - }; - - /* * It's more convenient to store a target VCPU per vIRQ * than the register ITARGETSR/IROUTER itself. * Use atomic operations to read/write the vcpu fields to avoid @@ -179,6 +170,10 @@ static inline int REG_RANK_NR(int b, uint32_t n) } } +uint32_t gather_irq_info_priority(struct vcpu *v, unsigned int irq); +void scatter_irq_info_priority(struct vcpu *v, unsigned int irq, + unsigned int value); + #define VGIC_REG_MASK(size) ((~0UL) >> (BITS_PER_LONG - ((1 << (size)) * 8))) /*
Currently we store the priority for newly triggered IRQs in the rank structure. To get eventually rid of this structure, move this value into the struct pending_irq. This one already contains a priority value, but we have to keep the two apart, as the priority for guest visible IRQs must not change while they are in a VCPU. This patch introduces a framework to get some per-IRQ information for a number of interrupts and collate them into one register. Similarily there is the opposite function to spread values from one register into multiple pending_irq's. Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- xen/arch/arm/vgic-v2.c | 33 +++++++++-------------------- xen/arch/arm/vgic-v3.c | 33 ++++++++++------------------- xen/arch/arm/vgic.c | 53 ++++++++++++++++++++++++++++++++++------------ xen/include/asm-arm/vgic.h | 17 ++++++--------- 4 files changed, 67 insertions(+), 69 deletions(-)