[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [RFC PATCH 05/10] ARM: vGIC: move priority from irq_rank to struct pending_irq
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@xxxxxxx>
---
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,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
