Re: [Xen-devel] [RFC PATCH 05/10] ARM: vGIC: move priority from irq_rank to struct pending_irq

[Julien Grall <julien.grall@xxxxxxx>]

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