Re: [PATCH 2/3] KVM: arm64: Stop save/restoring ACTLR_EL1

From: Marc Zyngier <maz@kernel.org>
To: James Morse <james.morse@arm.com>
Cc: kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 2/3] KVM: arm64: Stop save/restoring ACTLR_EL1
Date: Thu, 28 May 2020 13:38:51 +0100	[thread overview]
Message-ID: <07d09551c456c6be326473e003def3ab@kernel.org> (raw)
In-Reply-To: <4d42a5db0b573c7a184aea654829a06c@kernel.org>

On 2020-05-28 13:36, Marc Zyngier wrote:
> On 2020-05-26 17:18, James Morse wrote:
>> KVM sets HCR_EL2.TACR (which it calls HCR_TAC) via HCR_GUEST_FLAGS.
> 
> TAC is a leftover from 32bit.
> 
>> This means ACTLR* accesses from the guest are always trapped, and
>> always return the value in the sys_regs array.
>> 
>> The guest can't change the value of these registers, so we are
>> save restoring the reset value, which came from the host.
>> 
>> Stop save/restoring this register.
>> 
>> This also stops this register being affected by sysregs_loaded_on_cpu,
>> so we can provide 32 bit accessors that always use the in-memory copy.
>> 
>> Signed-off-by: James Morse <james.morse@arm.com>
>> ---
>>  arch/arm64/kvm/hyp/sysreg-sr.c | 2 --
>>  arch/arm64/kvm/sys_regs.c      | 2 --
>>  2 files changed, 4 deletions(-)
>> 
>> diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c 
>> b/arch/arm64/kvm/hyp/sysreg-sr.c
>> index 75b1925763f1..57116cf3a1a5 100644
>> --- a/arch/arm64/kvm/hyp/sysreg-sr.c
>> +++ b/arch/arm64/kvm/hyp/sysreg-sr.c
>> @@ -44,7 +44,6 @@ static void __hyp_text
>> __sysreg_save_el1_state(struct kvm_cpu_context *ctxt)
>>  {
>>  	ctxt->sys_regs[CSSELR_EL1]	= read_sysreg(csselr_el1);
>>  	ctxt->sys_regs[SCTLR_EL1]	= read_sysreg_el1(SYS_SCTLR);
>> -	ctxt->sys_regs[ACTLR_EL1]	= read_sysreg(actlr_el1);
>>  	ctxt->sys_regs[CPACR_EL1]	= read_sysreg_el1(SYS_CPACR);
>>  	ctxt->sys_regs[TTBR0_EL1]	= read_sysreg_el1(SYS_TTBR0);
>>  	ctxt->sys_regs[TTBR1_EL1]	= read_sysreg_el1(SYS_TTBR1);
>> @@ -133,7 +132,6 @@ static void __hyp_text
>> __sysreg_restore_el1_state(struct kvm_cpu_context *ctxt)
>>  		isb();
>>  	}
>> 
>> -	write_sysreg(ctxt->sys_regs[ACTLR_EL1],		actlr_el1);
> 
> If we don't need to save/restore it, we can also drop its presence
> in the sys_regs array.
> 
>>  	write_sysreg_el1(ctxt->sys_regs[CPACR_EL1],	SYS_CPACR);
>>  	write_sysreg_el1(ctxt->sys_regs[TTBR0_EL1],	SYS_TTBR0);
>>  	write_sysreg_el1(ctxt->sys_regs[TTBR1_EL1],	SYS_TTBR1);
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index 2eda539f3281..aae58513025c 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -81,7 +81,6 @@ u64 vcpu_read_sys_reg(const struct kvm_vcpu *vcpu, 
>> int reg)
>>  	switch (reg) {
>>  	case CSSELR_EL1:	return read_sysreg_s(SYS_CSSELR_EL1);
>>  	case SCTLR_EL1:		return read_sysreg_s(SYS_SCTLR_EL12);
>> -	case ACTLR_EL1:		return read_sysreg_s(SYS_ACTLR_EL1);
>>  	case CPACR_EL1:		return read_sysreg_s(SYS_CPACR_EL12);
>>  	case TTBR0_EL1:		return read_sysreg_s(SYS_TTBR0_EL12);
>>  	case TTBR1_EL1:		return read_sysreg_s(SYS_TTBR1_EL12);
>> @@ -124,7 +123,6 @@ void vcpu_write_sys_reg(struct kvm_vcpu *vcpu, u64
>> val, int reg)
>>  	switch (reg) {
>>  	case CSSELR_EL1:	write_sysreg_s(val, SYS_CSSELR_EL1);	return;
>>  	case SCTLR_EL1:		write_sysreg_s(val, SYS_SCTLR_EL12);	return;
>> -	case ACTLR_EL1:		write_sysreg_s(val, SYS_ACTLR_EL1);	return;
>>  	case CPACR_EL1:		write_sysreg_s(val, SYS_CPACR_EL12);	return;
>>  	case TTBR0_EL1:		write_sysreg_s(val, SYS_TTBR0_EL12);	return;
>>  	case TTBR1_EL1:		write_sysreg_s(val, SYS_TTBR1_EL12);	return;
> 
> It strikes me that we don't even have a trap handler for this sysreg,
> whether it is 32 or 64bit... That's a bit unfortunate, to say the
> least...

Ah, no. the sucker is hidden away in "generic_v8"...

         M.
-- 
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm