Re: [PATCH v2 10/25] KVM: arm64: vgic-v3: Add ICV_HPPIR1_EL1 handler

[Marc Zyngier <marc.zyngier@arm.com>]

On 06/06/17 12:51, Christoffer Dall wrote:
> On Thu, Jun 01, 2017 at 11:21:02AM +0100, Marc Zyngier wrote:
>> Add a handler for reading the guest's view of the ICV_HPPIR1_EL1
>> register. This is a simple parsing of the available LRs, extracting the
>> highest available interrupt.
>>
>> Reviewed-by: Eric Auger <eric.auger@redhat.com>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  arch/arm64/include/asm/sysreg.h |  1 +
>>  virt/kvm/arm/hyp/vgic-v3-sr.c   | 23 +++++++++++++++++++++++
>>  2 files changed, 24 insertions(+)
>>
>> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
>> index aad46b8eea5e..bd000686194a 100644
>> --- a/arch/arm64/include/asm/sysreg.h
>> +++ b/arch/arm64/include/asm/sysreg.h
>> @@ -185,6 +185,7 @@
>>  #define SYS_ICC_SGI1R_EL1		sys_reg(3, 0, 12, 11, 5)
>>  #define SYS_ICC_IAR1_EL1		sys_reg(3, 0, 12, 12, 0)
>>  #define SYS_ICC_EOIR1_EL1		sys_reg(3, 0, 12, 12, 1)
>> +#define SYS_ICC_HPPIR1_EL1		sys_reg(3, 0, 12, 12, 2)
>>  #define SYS_ICC_BPR1_EL1		sys_reg(3, 0, 12, 12, 3)
>>  #define SYS_ICC_CTLR_EL1		sys_reg(3, 0, 12, 12, 4)
>>  #define SYS_ICC_SRE_EL1			sys_reg(3, 0, 12, 12, 5)
>> diff --git a/virt/kvm/arm/hyp/vgic-v3-sr.c b/virt/kvm/arm/hyp/vgic-v3-sr.c
>> index d4f07f84602d..f0bc711db258 100644
>> --- a/virt/kvm/arm/hyp/vgic-v3-sr.c
>> +++ b/virt/kvm/arm/hyp/vgic-v3-sr.c
>> @@ -772,6 +772,26 @@ static void __hyp_text __vgic_v3_write_apxr3(struct kvm_vcpu *vcpu,
>>  	__vgic_v3_write_apxrn(vcpu, rt, 3);
>>  }
>>  
>> +static void __hyp_text __vgic_v3_read_hppir(struct kvm_vcpu *vcpu,
>> +					    u32 vmcr, int rt)
>> +{
>> +	u64 lr_val;
>> +	int lr, lr_grp, grp;
>> +
>> +	grp = __vgic_v3_get_group(vcpu);
>> +
>> +	lr = __vgic_v3_highest_priority_lr(vcpu, vmcr, &lr_val);
>> +	if (lr == -1)
>> +		goto spurious;
>> +
>> +	lr_grp = !!(lr_val & ICH_LR_GROUP);
>> +	if (lr_grp != grp)
>> +		lr_val = ICC_IAR1_EL1_SPURIOUS;
> 
> I don't get this.  The spec says that the special INTID 1023
>   "...is returned in response to an interrupt acknowledge, if there is
>    no pending interrupt with sufficient priority for it to be signaled to
>    the PE, or if the highest priority pending interrupt is not
>    appropriate for the:
>     * Current Security state.  Interrupt group that is associated with
>     * the System register."
> 
> So do we just take this to imply that it also covers the HPPIRx_EL1
> registers (despite them being mentioned explicitly for the other special
> INTIDs) or is there some other piece of spec I'm missing here?

Here's what the ICV_HPPIRx_EL1 INTID description says:

"If the highest priority pending interrupt is not observable, this field
contains a special INTID to indicate the reason. This special INTID can
take the value 1023 only."

Yes, the disparity between ICC_HPPIRx_EL1 and ICV_HPPIRx_EL1 is very
confusing and makes me want to shout at people.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...