[PATCH 14/31] KVM: arm64: vgic-v3: Add ICV_EOIR1_EL1 handler

[Marc Zyngier]

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 at redhat.com> 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 at arm.com>
>>>>>> ---
>>>>>>  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...