Received: by 2002:ac0:b08d:0:0:0:0:0 with SMTP id l13csp4717636imc; Mon, 25 Feb 2019 09:42:10 -0800 (PST) X-Google-Smtp-Source: AHgI3IYJ6X9qOGcuV8M6W2k2c1TNdgxymWbYeX9uhysXjSe07azV1dYojiP21OnmJRSN63ceDvME X-Received: by 2002:a63:2a82:: with SMTP id q124mr19955606pgq.402.1551116530572; Mon, 25 Feb 2019 09:42:10 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1551116530; cv=none; d=google.com; s=arc-20160816; b=dfG/MUDrforin0ot+DWofQ0wwNzr02ujlBml5/Ij+tGGFmMUFXUgS8GyVEUu5i6frs ZO1SiADactV6aT74Rly7ggxPVTUb2YEWyBaq1lFGILjwtyrKx8+tBDXTuZ3SNc8S40S8 Y6DfFiTEZbuW56XA1b6/EXeys+dt+LWA+X7vEQ7d5HQisDcAf8KrpKVQh6WIQOxMNAaa hNiVJRzvcj1n6WeSTcABi4zMet8scyf7SBMRN3e1qUC+AyWDG3hrqvDMAzFZ2fl3cTIb cxXYVQwAS/M8F8uvGqGt3YOjgxAhVyxM/rFRDrOwAVb9biZ8O9DczG+qcq0O23PjNN6l fIJA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding :content-language:in-reply-to:mime-version:user-agent:date :message-id:from:references:cc:to:subject; bh=CVn9gUltKOmIIcoH/wtz1legriKKnRFPQ5MpsJs4VnU=; b=lZg0WXHlJSsTCTM3DLZMXtcWcRiOmtaUKTQ2ZxxMTBYvDUL5XOgpHqttD/FVwYVDHF aibjgeUM32fdTcuQhqczd1xxWiO8cHlii0gDHduyouylguOlbRcJJT21Pabe5/19fnNp Qm3KNWvRnPzMcZUgRnU9bmKonRA+sKthjjeWuhNyMJ4xRm4SCv7wdYY8UfECDnnu+UGx +kJJ63k1XviyNHZc9P/JGwhUlPwbpor0KkVwdgTG8yMU8NJBnKz9m+nRx/P58mof4eIh Uf9CRXd9vIJJOzHD39DOSOvGg7rxMNEtts16us9QnAZoWl88U0gsta4/T2i8jYPxBOFU cc7w== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id p12si9909474pls.111.2019.02.25.09.41.55; Mon, 25 Feb 2019 09:42:10 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728778AbfBYRkD (ORCPT + 99 others); Mon, 25 Feb 2019 12:40:03 -0500 Received: from foss.arm.com ([217.140.101.70]:35556 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726352AbfBYRkD (ORCPT ); Mon, 25 Feb 2019 12:40:03 -0500 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id A4F07374; Mon, 25 Feb 2019 09:40:02 -0800 (PST) Received: from [10.1.196.105] (eglon.cambridge.arm.com [10.1.196.105]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 16BD33F703; Mon, 25 Feb 2019 09:39:59 -0800 (PST) Subject: Re: [PATCH v6 1/6] arm64/kvm: preserve host HCR_EL2 value To: Amit Daniel Kachhap , linux-arm-kernel@lists.infradead.org Cc: Christoffer Dall , Marc Zyngier , Catalin Marinas , Will Deacon , Andrew Jones , Dave Martin , Ramana Radhakrishnan , kvmarm@lists.cs.columbia.edu, Kristina Martsenko , linux-kernel@vger.kernel.org, Mark Rutland , Julien Thierry References: <1550568271-5319-1-git-send-email-amit.kachhap@arm.com> <1550568271-5319-2-git-send-email-amit.kachhap@arm.com> From: James Morse Message-ID: Date: Mon, 25 Feb 2019 17:39:58 +0000 User-Agent: Mozilla/5.0 (X11; Linux aarch64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 MIME-Version: 1.0 In-Reply-To: <1550568271-5319-2-git-send-email-amit.kachhap@arm.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-GB Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Amit, On 19/02/2019 09:24, Amit Daniel Kachhap wrote: > From: Mark Rutland > > When restoring HCR_EL2 for the host, KVM uses HCR_HOST_VHE_FLAGS, which > is a constant value. This works today, as the host HCR_EL2 value is > always the same, but this will get in the way of supporting extensions > that require HCR_EL2 bits to be set conditionally for the host. > > To allow such features to work without KVM having to explicitly handle > every possible host feature combination, this patch has KVM save/restore > for the host HCR when switching to/from a guest HCR. The saving of the > register is done once during cpu hypervisor initialization state and is > just restored after switch from guest. > > For fetching HCR_EL2 during kvm initialisation, a hyp call is made using > kvm_call_hyp and is helpful in NHVE case. > > For the hyp TLB maintenance code, __tlb_switch_to_host_vhe() is updated > to toggle the TGE bit with a RMW sequence, as we already do in > __tlb_switch_to_guest_vhe(). > > The value of hcr_el2 is now stored in struct kvm_cpu_context as both host > and guest can now use this field in a common way. > diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h > index ca56537..05706b4 100644 > --- a/arch/arm/include/asm/kvm_host.h > +++ b/arch/arm/include/asm/kvm_host.h > @@ -273,6 +273,8 @@ static inline void __cpu_init_stage2(void) > kvm_call_hyp(__init_stage2_translation); > } > > +static inline void __cpu_copy_hyp_conf(void) {} > + I agree Mark's suggestion of adding 'host_ctxt' in here makes it clearer what it is. > diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h > index 506386a..0dbe795 100644 > --- a/arch/arm64/include/asm/kvm_emulate.h > +++ b/arch/arm64/include/asm/kvm_emulate.h Hmmm, there is still a fair amount of churn due to moving the struct definition, but its easy enough to ignore as its mechanical. A preparatory patch that switched as may as possible to '*vcpu_hcr() = ' would cut the churn down some more, but I don't think its worth the extra effort. > diff --git a/arch/arm64/include/asm/kvm_hyp.h b/arch/arm64/include/asm/kvm_hyp.h > index a80a7ef..6e65cad 100644 > --- a/arch/arm64/include/asm/kvm_hyp.h > +++ b/arch/arm64/include/asm/kvm_hyp.h > @@ -151,7 +151,7 @@ void __fpsimd_restore_state(struct user_fpsimd_state *fp_regs); > bool __fpsimd_enabled(void); > > void activate_traps_vhe_load(struct kvm_vcpu *vcpu); > -void deactivate_traps_vhe_put(void); > +void deactivate_traps_vhe_put(struct kvm_vcpu *vcpu); I've forgotten why this is needed. You don't add a user of vcpu to deactivate_traps_vhe_put() in this patch. > diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c > index b0b1478..006bd33 100644 > --- a/arch/arm64/kvm/hyp/switch.c > +++ b/arch/arm64/kvm/hyp/switch.c > @@ -191,7 +194,7 @@ void activate_traps_vhe_load(struct kvm_vcpu *vcpu) > -void deactivate_traps_vhe_put(void) > +void deactivate_traps_vhe_put(struct kvm_vcpu *vcpu) > { > u64 mdcr_el2 = read_sysreg(mdcr_el2); > Why does deactivate_traps_vhe_put() need the vcpu? > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index 7732d0b..1b2e05b 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -458,6 +459,16 @@ int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu, > > static inline void __cpu_init_stage2(void) {} > > +/** > + * __cpu_copy_hyp_conf - copy the boot hyp configuration registers > + * > + * It is called once per-cpu during CPU hyp initialisation. > + */ Is it just the boot cpu? > +static inline void __cpu_copy_hyp_conf(void) > +{ > + kvm_call_hyp(__kvm_populate_host_regs); > +} > + > diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c > index 68d6f7c..68ddc0f 100644 > --- a/arch/arm64/kvm/hyp/sysreg-sr.c > +++ b/arch/arm64/kvm/hyp/sysreg-sr.c > @@ -21,6 +21,7 @@ > #include > #include > #include > +#include ... what's kvm_mmu.h needed for? The __hyp_this_cpu_ptr() you add comes from kvm_asm.h. /me tries it. Heh, hyp_symbol_addr(). kvm_asm.h should include this, but can't because the kvm_ksym_ref() dependency is the other-way round. This is just going to bite us somewhere else later! If we want to fix it now, moving hyp_symbol_addr() to kvm_asm.h would fix it. It's generating adrp/add so the 'asm' label is fair, and it really should live with its EL1 counterpart kvm_ksym_ref(). > @@ -294,7 +295,7 @@ void kvm_vcpu_put_sysregs(struct kvm_vcpu *vcpu) > if (!has_vhe()) > return; > > - deactivate_traps_vhe_put(); > + deactivate_traps_vhe_put(vcpu); > > __sysreg_save_el1_state(guest_ctxt); > __sysreg_save_user_state(guest_ctxt); > @@ -316,3 +317,21 @@ void __hyp_text __kvm_enable_ssbs(void) > "msr sctlr_el2, %0" > : "=&r" (tmp) : "L" (SCTLR_ELx_DSSBS)); > } > + > +/** > + * __kvm_populate_host_regs - Stores host register values > + * > + * This function acts as a function handler parameter for kvm_call_hyp and > + * may be called from EL1 exception level to fetch the register value. > + */ > +void __hyp_text __kvm_populate_host_regs(void) > +{ > + struct kvm_cpu_context *host_ctxt; > + if (has_vhe()) > + host_ctxt = this_cpu_ptr(&kvm_host_cpu_state); > + else > + host_ctxt = __hyp_this_cpu_ptr(kvm_host_cpu_state); You can use __hyp_this_cpu_ptr() here, even on VHE. For VHE the guts are the same and its simpler to use the same version in both cases. __hyp_this_cpu_ptr(sym) == hyp_symbol_addr(sym) + tpidr_el2; hyp_symbol_addr() here is just to guarantee the address is generated based on where we're executing from, not loaded from a literal pool which would give us the link-time address. (or whenever kaslr applied the relocations). This matters for non-VHE because the compiler can't know the code has an EL2 address as well as its link-time address. This doesn't matter for VHE, as there is no additional different address. (the other trickery is on non-VHE the tpidr_el2 value isn't actually the same as the hosts.. but on VHE it is) > + host_ctxt->hcr_el2 = read_sysreg(hcr_el2); > +} > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c > index 9e350fd3..8e18f7f 100644 > --- a/virt/kvm/arm/arm.c > +++ b/virt/kvm/arm/arm.c > @@ -1328,6 +1328,7 @@ static void cpu_hyp_reinit(void) > cpu_init_hyp_mode(NULL); > > kvm_arm_init_debug(); > + __cpu_copy_hyp_conf(); Your commit message says: | The saving of the register is done once during cpu hypervisor initialization state But cpu_hyp_reinit() is called each time secondary CPUs come online. Its also called as part of the cpu-idle mechanism via hyp_init_cpu_pm_notifier(). cpu-idle can ask the firmware to power-off the CPU until an interrupt becomes pending for it. KVM's EL2 state disappears when this happens, these calls take care of setting it back up again. On Juno, this can happen tens of times a second, and this adds an extra call to EL2. init_subsystems() would be the alternative place for this, but it wouldn't catch CPUs that came online after booting. I think you need something in cpu_hyp_reinit() or __cpu_copy_hyp_conf() to ensure it only happens once per CPU. I think you can test whether the HCR_EL2 value is zero, assuming zero means uninitialised. A VHE system would always set E2H, and a non-VHE system has to set RW. > if (vgic_present) > kvm_vgic_init_cpu_hardware(); > Thanks, James