[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





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@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;

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.

--
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®.