[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [RFC PATCH v2 07/22] ARM: vGIC: introduce priority setter/getter



Hi Andre,

On 21/07/17 20:59, Andre Przywara wrote:
Since the GICs MMIO access always covers a number of IRQs at once,
introduce wrapper functions which loop over those IRQs, take their
locks and read or update the priority values.
This will be used in a later patch.

Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
---
 xen/arch/arm/vgic.c        | 37 +++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/vgic.h |  5 +++++
 2 files changed, 42 insertions(+)

diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 434b7e2..b2c9632 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -243,6 +243,43 @@ static int vgic_get_virq_priority(struct vcpu *v, unsigned 
int virq)
     return ACCESS_ONCE(rank->priority[virq & INTERRUPT_RANK_MASK]);
 }

+#define MAX_IRQS_PER_IPRIORITYR 4

The name gives the impression that you may have IPRIORITYR with only 1 IRQ. But this is not true. The registers is always 4. However, you are able to access using byte or word.

+uint32_t vgic_fetch_irq_priority(struct vcpu *v, unsigned int nrirqs,

I am well aware that the vgic code is mixing between virq and irq. Moving forward, we should use virq to avoid confusion.

+                                 unsigned int first_irq)

Please stay consistent, with the naming. Either nr_irqs/first_irq or nrirqs/firstirq. But not a mix.

Also, it makes more sense to describe first the start then number.

+{
+    struct pending_irq *pirqs[MAX_IRQS_PER_IPRIORITYR];
+    unsigned long flags;
+    uint32_t ret = 0, i;
+
+    local_irq_save(flags);
+    vgic_lock_irqs(v, nrirqs, first_irq, pirqs);

I am not convinced on the usefulness of taking all the locks in one go. At one point in the time, you only need to lock a given pending_irq.

+
+    for ( i = 0; i < nrirqs; i++ )
+        ret |= pirqs[i]->priority << (i * 8);

Please avoid open-coding number.

+
+    vgic_unlock_irqs(pirqs, nrirqs);
+    local_irq_restore(flags);
+
+    return ret;
+}
+
+void vgic_store_irq_priority(struct vcpu *v, unsigned int nrirqs,
+                             unsigned int first_irq, uint32_t value)
+{
+    struct pending_irq *pirqs[MAX_IRQS_PER_IPRIORITYR];
+    unsigned long flags;
+    unsigned int i;
+
+    local_irq_save(flags);
+    vgic_lock_irqs(v, nrirqs, first_irq, pirqs);
+
+    for ( i = 0; i < nrirqs; i++, value >>= 8 )

Same here.

+        pirqs[i]->priority = value & 0xff;
+
+    vgic_unlock_irqs(pirqs, nrirqs);
+    local_irq_restore(flags);
+}
+
 bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)
 {
     unsigned long flags;
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index ecf4969..f3791c8 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -198,6 +198,11 @@ void vgic_lock_irqs(struct vcpu *v, unsigned int nrirqs, 
unsigned int first_irq,
                     struct pending_irq **pirqs);
 void vgic_unlock_irqs(struct pending_irq **pirqs, unsigned int nrirqs);

+uint32_t vgic_fetch_irq_priority(struct vcpu *v, unsigned int nrirqs,
+                                 unsigned int first_irq);
+void vgic_store_irq_priority(struct vcpu *v, unsigned int nrirqs,
+                             unsigned int first_irq, uint32_t reg);
+
 enum gic_sgi_mode;

 /*


Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.