Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752638AbdIFJvD (ORCPT ); Wed, 6 Sep 2017 05:51:03 -0400 Received: from szxga05-in.huawei.com ([45.249.212.191]:5561 "EHLO szxga05-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752220AbdIFJu5 (ORCPT ); Wed, 6 Sep 2017 05:50:57 -0400 Subject: Re: [PATCH] arm64: KVM: VHE: save and restore some PSTATE bits To: Marc Zyngier , "christoffer.dall@linaro.org" , "pbonzini@redhat.com" , "rkrcmar@redhat.com" , "vladimir.murzin@arm.com" , "linux-arm-kernel@lists.infradead.org" , "kvmarm@lists.cs.columbia.edu" , "kvm@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "suzuki.poulose@arm.com" , , References: <0184EA26B2509940AA629AE1405DD7F2015DF717@DGGEMA503-MBX.china.huawei.com> <2a5d4299-2523-aef5-7db1-f351ca66b562@arm.com> CC: James Morse , , Huangshaoyu From: gengdongjiu Message-ID: <9396c8ff-98e3-af57-742a-bbe2ac54ad34@huawei.com> Date: Wed, 6 Sep 2017 17:49:56 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.7.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.142.68.147] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A020204.59AFC4D7.0024,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2014-11-16 11:51:01, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: 570afe614323726a0ec56b533c2c6037 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5068 Lines: 140 For UAO, if not save/restore PSTATE.UAO, we can use below fixing. diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c index 9341376..c3dd761 100644 --- a/arch/arm64/kvm/hyp/sysreg-sr.c +++ b/arch/arm64/kvm/hyp/sysreg-sr.c @@ -21,6 +21,8 @@ #include #include +#include + /* Yes, this does nothing, on purpose */ static void __hyp_text __sysreg_do_nothing(struct kvm_cpu_context *ctxt) { } @@ -121,8 +123,13 @@ static void __hyp_text __sysreg_restore_state(struct kvm_cpu_context *ctxt) write_sysreg_el1(ctxt->gp_regs.spsr[KVM_SPSR_EL1],spsr); } +static void __hyp_text __sysreg_restore_state_vhe(struct kvm_cpu_context *ctxt) +{ + uao_thread_switch(current); +} + static hyp_alternate_select(__sysreg_call_restore_host_state, - __sysreg_restore_state, __sysreg_do_nothing, + __sysreg_restore_state, __sysreg_restore_state_vhe, ARM64_HAS_VIRT_HOST_EXTN); void __hyp_text __sysreg_restore_host_state(struct kvm_cpu_context *ctxt) On 2017/9/6 17:32, gengdongjiu wrote: > Hi Marc, > > On 2017/9/6 16:17, Marc Zyngier wrote: >> On 05/09/17 19:58, gengdongjiu wrote: >>> when exit from guest, some host PSTATE bits may be lost, such as >>> PSTATE.PAN or PSTATE.UAO. It is because host and hypervisor all run >>> in the EL2, host PSTATE value cannot be saved and restored via >>> SPSR_EL2. So if guest has changed the PSTATE, host continues with >>> a wrong value guest has set. >>> >>> Signed-off-by: Dongjiu Geng >>> Signed-off-by: Haibin Zhang >>> --- >>> arch/arm64/include/asm/kvm_host.h | 8 +++++++ >>> arch/arm64/include/asm/kvm_hyp.h | 2 ++ >>> arch/arm64/include/asm/sysreg.h | 23 +++++++++++++++++++ >>> arch/arm64/kvm/hyp/entry.S | 2 -- >>> arch/arm64/kvm/hyp/switch.c | 24 ++++++++++++++++++-- >>> arch/arm64/kvm/hyp/sysreg-sr.c | 48 ++++++++++++++++++++++++++++++++++++--- >>> 6 files changed, 100 insertions(+), 7 deletions(-) >>> >>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h >>> index e923b58..cba7d3e 100644 >>> --- a/arch/arm64/include/asm/kvm_host.h >>> +++ b/arch/arm64/include/asm/kvm_host.h >>> @@ -193,6 +193,12 @@ struct kvm_cpu_context { >>> }; >>> }; >>> >>> +struct kvm_cpu_host_pstate { >>> + u64 daif; >>> + u64 uao; >>> + u64 pan; >>> +}; >> >> I love it. This is the most expensive way of saving/restoring a single >> 32bit value. >> >> More seriously, please see the discussion between James and Christoffer >> there[1]. I expect James to address the PAN/UAO states together with the >> debug state in the next iteration of his patch. > > I roughly see the discussion between James and Christoffer, Seems Christoffer does not suggest save and > restore it, and suggest to do below, and UAO/PAN may not use the same ways. > > __kvm_vcpu_run(struct kvm_vcpu *vcpu) > { > if (has_vhe()) > asm("msr daifset, #0xf"); > > ... > exit_code = __guest_enter(vcpu, host_ctxt); > ... > > if (has_vhe()) > asm("msr daifclr, #0xd"); > } > > > If not save/restore them, the KVM will set them according to the CPU capability. For example below fixing, it will check CPU capability, If CPU supports PAN, > the kvm will always enable the PAN for the host. But in fact, the host may be not enable the PAN. > Of course for the UAO, we can use the similar fixing if Marc or Christoffer is agreed. but seems not make sense. > > commit cb96408da4e11698674abd04aeac941c1bed2038 > Author: Vladimir Murzin > Date: Thu Sep 1 15:29:03 2016 +0100 > > arm64: KVM: VHE: reset PSTATE.PAN on entry to EL2 > > SCTLR_EL2.SPAN bit controls what happens with the PSTATE.PAN bit on an > exception. However, this bit has no effect on the PSTATE.PAN when > HCR_EL2.E2H or HCR_EL2.TGE is unset. Thus when VHE is used and > exception taken from a guest PSTATE.PAN bit left unchanged and we > continue with a value guest has set. > > To address that always reset PSTATE.PAN on entry from EL1. > > Fixes: 1f364c8c48a0 ("arm64: VHE: Add support for running Linux in EL2 mode") > > Signed-off-by: Vladimir Murzin > Reviewed-by: James Morse > Acked-by: Marc Zyngier > Cc: # v4.6+ > Signed-off-by: Christoffer Dall > > diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S > index 3967c231..b5926ee 100644 > --- a/arch/arm64/kvm/hyp/entry.S > +++ b/arch/arm64/kvm/hyp/entry.S > @@ -96,6 +96,8 @@ ENTRY(__guest_exit) > > add x1, x1, #VCPU_CONTEXT > > + ALTERNATIVE(nop, SET_PSTATE_PAN(1), ARM64_HAS_PAN, CONFIG_ARM64_PAN) > + > // Store the guest regs x2 and x3 > stp x2, x3, [x1, #CPU_XREG_OFFSET(2)] > > >> >> Thanks, >> >> M. >> >> [1] https://www.spinics.net/lists/arm-kernel/msg599798.html >>