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

[Julien Grall <julien.grall@xxxxxxx>]

On 17/08/17 18:06, Andre Przywara wrote:
> Hi,
Hi Andre,

> On 11/08/17 15:10, Julien Grall wrote:
> > 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.
> I don't think so. The MMIO access a guest does is expected to be atomic,
> so it expects to read the priorities of the four interrupts as they were
> *at one point in time*.
> This issue is more obvious for the enabled bit, for instance, but also
> here a (32-bit) read and a write of some IPRIORITYR might race against
> each other. This was covered by the rank lock before, but now we have to
> bite the bullet and lock all involved IRQs.
A well-behaved guest would need a lock in order to modify the hardware 
as it can't predict in which order the write will happen. If the guest 
does not respect that I don't think you it is necessary to require 
atomicity of the modification.
This is making the code more complex for a little benefits and also 
increase the duration of the interrupt masked.
So as long as it does not affect the hypervisor, then I think it is fine 
to not handle more than the atomicity at the IRQ level.
Cheers,

--
Julien Grall

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