On 24/05/16 13:52, Christoffer Dall wrote:
> On Mon, May 23, 2016 at 01:37:03PM +0100, Marc Zyngier wrote:
>> The GICv3 backend of the vgic is quite barrier heavy, in order
>> to ensure synchronization of the system registers and the
>> memory mapped view for a potential GICv2 guest.
>>
>> But when the guest is using a GICv3 model, there is absolutely
>> no need to execute all these heavy barriers, and it is actually
>> beneficial to avoid them altogether.
>>
>> This patch makes the synchonization conditional, and ensures
>> that we do not change the EL1 SRE settings if we do not need to.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
>> ---
>> arch/arm64/kvm/hyp/vgic-v3-sr.c | 23 ++++++++++++++++-------
>> 1 file changed, 16 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/hyp/vgic-v3-sr.c b/arch/arm64/kvm/hyp/vgic-v3-sr.c
>> index 40c3b4c..5f8f80b 100644
>> --- a/arch/arm64/kvm/hyp/vgic-v3-sr.c
>> +++ b/arch/arm64/kvm/hyp/vgic-v3-sr.c
>> @@ -169,7 +169,8 @@ 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.
>> */
>> - dsb(st);
>> + if (!cpu_if->vgic_sre)
>> + dsb(st);
>>
>> cpu_if->vgic_vmcr = read_gicreg(ICH_VMCR_EL2);
>>
>> @@ -235,8 +236,12 @@ void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu)
>>
>> val = read_gicreg(ICC_SRE_EL2);
>> write_gicreg(val | ICC_SRE_EL2_ENABLE, ICC_SRE_EL2);
>> - isb(); /* Make sure ENABLE is set at EL2 before setting SRE at EL1 */
>> - write_gicreg(1, ICC_SRE_EL1);
>> +
>> + if (!cpu_if->vgic_sre) {
>> + /* Make sure ENABLE is set at EL2 before setting SRE at EL1 */
>> + isb();
>> + write_gicreg(1, ICC_SRE_EL1);
>
> why the change in behavior to only write 1 to the register when
> (!cpu_if->vgic_sre) ?
If we're on a GICv3 host and emulating a GICv3 guest, then we never
changed the SRE mode (i.e. it is still set to 1). The only case we
change it is when we when have a GICv2 guest (i.e. when vgic_sre is zero).
So there is no need to restore the host configuration unless we actually
changed it...
>
>> + }
>> }
>>
>> void __hyp_text __vgic_v3_restore_state(struct kvm_vcpu *vcpu)
>> @@ -255,8 +260,10 @@ void __hyp_text __vgic_v3_restore_state(struct kvm_vcpu *vcpu)
>> * been actually programmed with the value we want before
>> * starting to mess with the rest of the GIC.
>> */
>> - write_gicreg(cpu_if->vgic_sre, ICC_SRE_EL1);
>> - isb();
>> + if (!cpu_if->vgic_sre) {
>> + write_gicreg(0, ICC_SRE_EL1);
>> + isb();
>> + }
... here.
Does it make sense? I could also split this as part of another patch if
you think that's clearer.
M.
--
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
