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

[Andre Przywara <andre.przywara@xxxxxxx>]

Hi,

On 18/08/17 15:21, Julien Grall wrote:
> 
> 
> 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.

Fair enough, I can live with that. I didn't like the added complexity
for the tiny benefit either, just wanted to retain the behaviour we had
naturally with the rank lock before.
So this is definitely true for IPRIORITYR, ICFGR and friends, but I need
to double check on ISENABLER/ICENABLER, because of the OR/AND-NOT
semantics, which allows lockless accesses from the software side. I
believe this is fine, though.

Cheers,
Andre.

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