Re: [Qemu-devel] [PATCH v4 12/20] intc/arm_gic: Implement virtualization extensions in gic_(deactivate|complete_irq)

[Peter Maydell]

On 18 July 2018 at 14:22, Luc Michel <address@hidden> wrote:
>
>
> On 07/17/2018 03:32 PM, Peter Maydell wrote:
>> On 14 July 2018 at 18:15, Luc Michel <address@hidden> wrote:
>>> Implement virtualization extensions in the gic_deactivate_irq() and
>>> gic_complete_irq() functions.  When a guest tries to deactivat or end an
>>
>> "deactivate"
>>
>>> IRQ that does not exist in the LRs, the EOICount field of the virtual
>>> interface HCR register is incremented by one, and the request is
>>> ignored.
>>>
>>> Signed-off-by: Luc Michel <address@hidden>
>>> ---
>>>  hw/intc/arm_gic.c | 18 ++++++++++++++++++
>>>  1 file changed, 18 insertions(+)
>>>
>>> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
>>> index be9e2594d9..50cbbfbe24 100644
>>> --- a/hw/intc/arm_gic.c
>>> +++ b/hw/intc/arm_gic.c
>>> @@ -590,6 +590,15 @@ static void gic_deactivate_irq(GICState *s, int cpu, 
>>> int irq, MemTxAttrs attrs)
>>>          return;
>>>      }
>>>
>>> +    if (gic_is_vcpu(cpu) && !gic_virq_is_valid(s, irq, cpu)) {
>>> +        /* This vIRQ does not have an LR entry which is either active or
>>> +         * pending and active. Increment EOICount and ignore the write.
>>> +         */
>>> +        int rcpu = gic_get_vcpu_real_id(cpu);
>>> +        s->h_hcr[rcpu] += 1 << R_GICH_HCR_EOICount_SHIFT;
>>> +        return;
>>> +    }
>>
>> It's a bit hard to tell from the amount of context in the diff,
>> but I think this check is being done too late in this function.
>> It needs to happen before we do any operations on the irq we're
>> passed (eg checking which group it is).

> For operations on the IRQ, yes. But there is also the test on the
> EOIMode bit in C_CTLR before that. Writing to C_DIR when EOIMode is 0 is
> unpredictable. I was thinking of keeping the same behaviour we had until
> then, which is to completely ignore the write.

Yes, that's a reasonable choice.

>>> +
>>>      if (gic_cpu_ns_access(s, cpu, attrs) && !group) {
>>>          DPRINTF("Non-secure DI for Group0 interrupt %d ignored\n", irq);
>>>          return;
>>> @@ -604,6 +613,15 @@ static void gic_complete_irq(GICState *s, int cpu, int 
>>> irq, MemTxAttrs attrs)
>>>      int group;
>>>
>>>      DPRINTF("EOI %d\n", irq);
>>> +    if (gic_is_vcpu(cpu) && !gic_virq_is_valid(s, irq, cpu)) {
>>> +        /* This vIRQ does not have an LR entry which is either active or
>>> +         * pending and active. Increment EOICount and ignore the write.
>>> +         */
>>> +        int rcpu = gic_get_vcpu_real_id(cpu);
>>> +        s->h_hcr[rcpu] += 1 << R_GICH_HCR_EOICount_SHIFT;
>>> +        return;
>>> +    }
>>
>> This isn't consistent with the deactivate code about whether we
>> do this check before the "irq >= s->num_irq" check or after.
>>
>> I think the correct answer is "before", because the number of
>> physical interrupts in the GIC shouldn't affect the valid
>> range of virtual interrupts.
>>
>> There is also an edge case here: if the VIRQ written to the
>> EOI or DIR registers is a special interrupt number (1020..1023),
>> then should we increment the EOI count or not? The GICv2 spec
>> is not entirely clear on this point, but it does say in the
>> GICH_HCR.EOICount docs that "EOIs that do not clear a bit in
>> the Active Priorities register GICH_APR do not cause an increment",
>> and the GICv3 spec is clear so my recommendation is that for
>> 1020..1023 we should ignore the write and not increment EOICount.
>>
>> That bit about "EOIs that do not clear a bit in GICH_APR do
>> not increment EOICount" also suggests that our logic for DIR
>> and EOI needs to be something like:
>>
>>   if (vcpu) {
>>       if (irq > 1020) {
>>           return;
>>       }
>>       clear GICH_HCR bit;
>>       if (no bit cleared) {
>>           return;
>>       }
>>       if (!gic_virq_is_valid()) {
>>           increment EOICount;
>>           return;
>>       }
>>       clear active bit in LR;
>>   }
>>
>> ?
> I agree for EOIR, but for DIR, it seems weird. A write to DIR never
> causes a bit to be cleared in GICH_APR. It can only change the state of
> the LR corresponding to the given vIRQ. So if we read the specification
> this way, a write to DIR should never cause a EOICount increment.

Yes, I think you're right and I was misreading the spec here,
at least where it regards DIR.

> However the part you quoted:
>
> "EOIs that do not clear a bit in the Active Priorities register GICH_APR
> do not cause an increment"
>
> seems to apply to EOIs only, not to interrupt deactivations.
>
> And in the DIR specification:
> "If the specified Interrupt does not exist in the List registers, the
> GICH_HCR.EOIcount field is incremented"
>
> So I would suggest that we apply your reasoning for EOIR. For DIR, I
> think something like this is sufficient:
>
>    if (vcpu) {
>        if (irq > 1020) {
>            return;
>        }
>        if (!gic_virq_is_valid()) {
>            increment EOICount;
>            return;
>        }
>        clear active bit in LR;
>    }
>
>
> What do you think?

Yes, I think this is right.

thanks
-- PMM