On 31/05/17 08:26, Auger Eric wrote:
> Hi Marc,
>
> On 31/05/2017 08:46, Marc Zyngier wrote:
>> On Wed, 31 May 2017 08:33:05 +0200
>> Auger Eric <eric.auger@xxxxxxxxxx> wrote:
>>
>>> Hi Marc,
>>>
>>> On 30/05/2017 16:24, Marc Zyngier wrote:
>>>> On 30/05/17 08:48, Auger Eric wrote:
>>>>> Hi Marc
>>>>>
>>>>> On 03/05/2017 12:45, Marc Zyngier wrote:
>>>>>> Add a handler for writing the guest's view of the ICC_EOIR1_EL1
>>>>>> register. This involves dropping the priority of the interrupt,
>>>>>> and deactivating it if required (EOImode == 0).
>>>>>>
>>>>>> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
>>>>>> ---
>>>>>> include/linux/irqchip/arm-gic-v3.h | 2 +
>>>>>> virt/kvm/arm/hyp/vgic-v3-sr.c | 119 +++++++++++++++++++++++++++++++++++++
>>>>>> 2 files changed, 121 insertions(+)
>>>>>>
>>>>>> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
>>>>>> index 7610ea4e8337..c56d9bc2c904 100644
>>>>>> --- a/include/linux/irqchip/arm-gic-v3.h
>>>>>> +++ b/include/linux/irqchip/arm-gic-v3.h
>>>>>> @@ -403,6 +403,8 @@
>>>>>>
>>>>>> #define ICH_HCR_EN (1 << 0)
>>>>>> #define ICH_HCR_UIE (1 << 1)
>>>>>> +#define ICH_HCR_EOIcount_SHIFT 27
>>>>>> +#define ICH_HCR_EOIcount_MASK (0x1f << ICH_HCR_EOIcount_SHIFT)
>>>>>>
>>>>>> #define ICH_VMCR_CBPR_SHIFT 4
>>>>>> #define ICH_VMCR_CBPR_MASK (1 << ICH_VMCR_CBPR_SHIFT)
>>>>>> diff --git a/virt/kvm/arm/hyp/vgic-v3-sr.c b/virt/kvm/arm/hyp/vgic-v3-sr.c
>>>>>> index 49aad1de3ac8..a76351b3ad66 100644
>>>>>> --- a/virt/kvm/arm/hyp/vgic-v3-sr.c
>>>>>> +++ b/virt/kvm/arm/hyp/vgic-v3-sr.c
>>>>>> @@ -425,6 +425,26 @@ static int __hyp_text __vgic_v3_highest_priority_lr(struct kvm_vcpu *vcpu,
>>>>>> return lr;
>>>>>> }
>>>>>>
>>>>>> +static int __hyp_text __vgic_v3_find_active_lr(struct kvm_vcpu *vcpu,
>>>>>> + int intid, u64 *lr_val)
>>>>>> +{
>>>>>> + unsigned int used_lrs = vcpu->arch.vgic_cpu.used_lrs;
>>>>>> + int i;
>>>>>> +
>>>>>> + for (i = 0; i < used_lrs; i++) {
>>>>>> + u64 val = __gic_v3_get_lr(i);
>>>>>> +
>>>>>> + if ((val & ICH_LR_VIRTUAL_ID_MASK) == intid &&
>>>>>> + (val & ICH_LR_ACTIVE_BIT)) {
>>>>> I guess it is safe because we don't have yet virtual interrupts directly
>>>>> mapped to phys IRQs, besides timer one?
>>>>
>>>> What would that change? I don't see how having a HW interrupt here would
>>>> be unsafe... Am I missing something?
>>> I was thinking about the case of an HW mapped interrupt whose active
>>> state must be observed at distributor level - as I understood the spec -
>>> and not at LR level.
>>
>> What part of the spec are you referring to?
>>
>> A virtual interrupt, even backed by a HW interrupt, does have an active
>> state (unless it is an LPI). The only state we cannot observe in the LR
>> when the HW bit is set is the Active+Pending state because the pending
>> bit is then at the physical distributor level.
>>
>> For all intent and purposes, the active state in the LR behaves the
>> same, irrespective of the HW state, until the guest issues a
>> deactivation (either using EOI or DIR, depending on EOImode).
>
> Hum OK. I was referring to the note in table 5-9 of IHI0048B2 but
> effectively I added a "s" where there is none :-(
>
> "
> For hardware interrupts, the pending and active state is held in the
> physical Distributor rather than the virtual
> CPU interface. A hypervisor must only use the pending and active state
> for software originated interrupts
> associated with virtual devices, or SGIs.
> "
>
> Sorry for the noise.
No worries. I've pestered against this verbiage multiple times because
it is incredibly misleading for non native speakers (I misinterpreted
this "pending and active state" a couple of times myself). This is why I
wrote Active+Pending above, because it is less ambiguous.
The GICv3 spec contains the same subtlety in multiple places. Man, how I
hate this thing, it is unbelievable.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
