[PATCH 1/2] KVM: arm/arm64: vgic-v3: De-optimize VMCR save/restore when emulating a GICv2

[Marc Zyngier]

On 19/04/17 11:08, Christoffer Dall wrote:
> On Mon, Apr 10, 2017 at 10:19:43AM +0100, Marc Zyngier wrote:
>> When emulating a GICv2-on-GICv3, special care must be taken to only
>> save/restore VMCR_EL2 when ICC_SRE_EL1.SRE is cleared. Otherwise,
>> all Group-0 interrupts end-up being delivered as FIQ, which is
>> probably not what the guest expects, as demonstrated here with
>> an unhappy EFI:
>>
>> 	FIQ Exception at 0x000000013BD21CC4
>>
>> This means that we cannot perform the load/put trick when dealing
>> with VMCR_EL2 (because the host has SRE set), and we have to deal
>> with it in the world-switch.
>>
>> Fortunately, this is not the most common case (modern guests should
>> be able to deal with GICv3 directly), and the performance is not worse
>> than what it was before the VMCR optimization.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier at arm.com>
>> ---
>>  virt/kvm/arm/hyp/vgic-v3-sr.c |  8 ++++++--
>>  virt/kvm/arm/vgic/vgic.c      | 26 ++++++++++++++++++++------
>>  2 files changed, 26 insertions(+), 8 deletions(-)
>>
>> diff --git a/virt/kvm/arm/hyp/vgic-v3-sr.c b/virt/kvm/arm/hyp/vgic-v3-sr.c
>> index 3d0b1ddb6929..91922c1eddc8 100644
>> --- a/virt/kvm/arm/hyp/vgic-v3-sr.c
>> +++ b/virt/kvm/arm/hyp/vgic-v3-sr.c
>> @@ -128,8 +128,10 @@ void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu)
>>  	 * Make sure stores to the GIC via the memory mapped interface
>>  	 * are now visible to the system register interface.
>>  	 */
>> -	if (!cpu_if->vgic_sre)
>> +	if (!cpu_if->vgic_sre) {
>>  		dsb(st);
>> +		cpu_if->vgic_vmcr = read_gicreg(ICH_VMCR_EL2);
>> +	}
>>  
>>  	if (used_lrs) {
>>  		int i;
>> @@ -205,11 +207,13 @@ void __hyp_text __vgic_v3_restore_state(struct kvm_vcpu *vcpu)
>>  	 * delivered as a FIQ to the guest, with potentially fatal
>>  	 * consequences. So we must make sure that ICC_SRE_EL1 has
>>  	 * been actually programmed with the value we want before
>> -	 * starting to mess with the rest of the GIC.
>> +	 * starting to mess with the rest of the GIC, and VMCR_EL2 in
>> +	 * particular.
>>  	 */
>>  	if (!cpu_if->vgic_sre) {
>>  		write_gicreg(0, ICC_SRE_EL1);
>>  		isb();
>> +		write_gicreg(cpu_if->vgic_vmcr, ICH_VMCR_EL2);
>>  	}
>>  
>>  	val = read_gicreg(ICH_VTR_EL2);
>> diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
>> index 3d0979c30721..32205f2ba6ff 100644
>> --- a/virt/kvm/arm/vgic/vgic.c
>> +++ b/virt/kvm/arm/vgic/vgic.c
>> @@ -667,10 +667,20 @@ void kvm_vgic_load(struct kvm_vcpu *vcpu)
>>  	if (unlikely(!vgic_initialized(vcpu->kvm)))
>>  		return;
>>  
>> -	if (kvm_vgic_global_state.type == VGIC_V2)
>> +	if (kvm_vgic_global_state.type == VGIC_V2) {
>>  		vgic_v2_load(vcpu);
>> -	else
>> -		vgic_v3_load(vcpu);
>> +	} else {
>> +		struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3;
>> +
>> +		/*
>> +		 * If dealing with a GICv2 emulation on GICv3,
>> +		 * VMCR_EL2.VFIQen is dependent on ICC_SRE_EL1.SRE,
>> +		 * and we have to perform the VMCR_EL2 save/restore in
>> +		 * the world switch.
>> +		 */
>> +		if (likely(cpu_if->vgic_sre))
>> +			vgic_v3_load(vcpu);
> 
> I wonder if this conditional should really be inside vgic_v3_load
> instead?

That's a good point. It will keep the wart out of the main vgic code and
make it an "implementation detail".

> 
>> +	}
>>  }
>>  
>>  void kvm_vgic_put(struct kvm_vcpu *vcpu)
>> @@ -678,10 +688,14 @@ void kvm_vgic_put(struct kvm_vcpu *vcpu)
>>  	if (unlikely(!vgic_initialized(vcpu->kvm)))
>>  		return;
>>  
>> -	if (kvm_vgic_global_state.type == VGIC_V2)
>> +	if (kvm_vgic_global_state.type == VGIC_V2) {
>>  		vgic_v2_put(vcpu);
>> -	else
>> -		vgic_v3_put(vcpu);
>> +	} else {
>> +		struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3;
>> +
>> +		if (likely(cpu_if->vgic_sre))
>> +			vgic_v3_put(vcpu);
> 
> And same here?
> 
>> +	}
>>  }
>>  
>>  int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu)
>> -- 
>> 2.11.0
>>
> 
> Otherwise looks good.

Thanks. I'll respin this patch shortly.

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