[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-arm] [PATCH v4 12/20] intc/arm_gic: Implement virtualization e
From: |
Luc Michel |
Subject: |
Re: [Qemu-arm] [PATCH v4 12/20] intc/arm_gic: Implement virtualization extensions in gic_(deactivate|complete_irq) |
Date: |
Wed, 18 Jul 2018 15:22:35 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 |
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.
>
>> +
>> 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.
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?
Thanks
Luc
>
>> +
>> if (irq >= s->num_irq) {
>> /* This handles two cases:
>> * 1. If software writes the ID of a spurious interrupt [ie 1023]
>> --
>> 2.18.0
>>
>
> thanks
> -- PMM
>
signature.asc
Description: OpenPGP digital signature
- [Qemu-arm] [PATCH v4 00/20] arm_gic: add virtualization extensions support, Luc Michel, 2018/07/14
- [Qemu-arm] [PATCH v4 04/20] vmstate.h: Provide VMSTATE_UINT16_SUB_ARRAY, Luc Michel, 2018/07/14
- [Qemu-arm] [PATCH v4 06/20] intc/arm_gic: Add virtual interface register definitions, Luc Michel, 2018/07/14
- [Qemu-arm] [PATCH v4 02/20] intc/arm_gic: Implement GICD_ISACTIVERn and GICD_ICACTIVERn registers, Luc Michel, 2018/07/14
- [Qemu-arm] [PATCH v4 08/20] intc/arm_gic: Refactor secure/ns access check in the CPU interface, Luc Michel, 2018/07/14
- [Qemu-arm] [PATCH v4 12/20] intc/arm_gic: Implement virtualization extensions in gic_(deactivate|complete_irq), Luc Michel, 2018/07/14
- [Qemu-arm] [PATCH v4 03/20] intc/arm_gic: Remove some dead code and put some functions static, Luc Michel, 2018/07/14
- [Qemu-arm] [PATCH v4 10/20] intc/arm_gic: Implement virtualization extensions in gic_(activate_irq|drop_prio), Luc Michel, 2018/07/14
- [Qemu-arm] [PATCH v4 13/20] intc/arm_gic: Implement virtualization extensions in gic_cpu_(read|write), Luc Michel, 2018/07/14
- [Qemu-arm] [PATCH v4 09/20] intc/arm_gic: Add virtualization enabled IRQ helper functions, Luc Michel, 2018/07/14
- [Qemu-arm] [PATCH v4 11/20] intc/arm_gic: Implement virtualization extensions in gic_acknowledge_irq, Luc Michel, 2018/07/14
- [Qemu-arm] [PATCH v4 01/20] intc/arm_gic: Refactor operations on the distributor, Luc Michel, 2018/07/14
- [Qemu-arm] [PATCH v4 16/20] intc/arm_gic: Implement gic_update_virt() function, Luc Michel, 2018/07/14