From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-14.7 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1092CC433B4 for ; Tue, 11 May 2021 11:53:13 +0000 (UTC) Received: from desiato.infradead.org (desiato.infradead.org [90.155.92.199]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 7FAF3611CE for ; Tue, 11 May 2021 11:53:12 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 7FAF3611CE Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=desiato.20200630; h=Sender:Content-Transfer-Encoding :Content-Type:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To:Subject:Cc:To: From:Message-ID:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=W5e1ZfsvRkwqvfzRiR4VYPTlA1Eq+dHSh6qvEnTzuFo=; b=fzXDkLXxxI+Z0nepUC41MojWj 6eyOzTFSpDXBMZF4xusnwQYaK0Kv3OqtC6d7tth/pOJeYvwCG1pzFgA0brIhBpH2/tbT//S0AAPR6 y9h1q3rIjUlZJMbSOYRfpsoVxAk7J0BiCs068KzrDZ+JYZL088cxtxOOOKDOuOGyo9JR02F38KrZv kmJiKo8QeO4OTmcmAx6/WPEwtoRrUqcaB/26gV9CVCZaQfxqGFmdOMotIoJ4bZra3NT2+4OTg+1Ri TNUD9vyI2ZMZKZF8kowX56mWLtBTF+0Y9kTj9wuW9UXBQAdIDIS2SU/5G4TSxC1IYU3ILaaJaMLuz PuvRblOXA==; Received: from localhost ([::1] helo=desiato.infradead.org) by desiato.infradead.org with esmtp (Exim 4.94 #2 (Red Hat Linux)) id 1lgQvm-00HDwz-Fz; Tue, 11 May 2021 11:52:06 +0000 Received: from [2607:7c80:54:e::133] (helo=bombadil.infradead.org) by desiato.infradead.org with esmtps (Exim 4.94 #2 (Red Hat Linux)) id 1lgN4b-00GXJV-86 for linux-arm-kernel@desiato.infradead.org; Tue, 11 May 2021 07:45:01 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=bombadil.20210309; h=Content-Type:MIME-Version:References: In-Reply-To:Subject:Cc:To:From:Message-ID:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=AudHRKhrr6+ClG1CA7+5DwAyowFCFGybck2F8gyybv8=; b=alRkEZRaoGzdEQ97Lsu4OdV7G4 ncK8R6C89ppdQPXewBBgk1qRevtwrGUVsFyfjJtJqImqOm1DIFMzXwH3XLw4ZoCTws7qmWsp/Kkc6 uNdIaDKtbw6sn5LM4SUqHdYil3ukiOd+F7MdisC2JCrF5xoRCt4OADRNuA2qemTfhrU+ICaRXHuwE xuZwu355USmDozVxBGgrVdKvPCrZ5l2L7G7vHYHklYXdHwv/NqBgHv/pkKbHvwU+i1E+Pka3DddNk aTy9Ycof5yBSe/g3ZJPiQeLAa51sgYVUw8PduNl7Fol4wG/WdS0gOXBaa3DSeZsPn6HVrpAM+MlxG UP+wRmRg==; Received: from mail.kernel.org ([198.145.29.99]) by bombadil.infradead.org with esmtps (Exim 4.94 #2 (Red Hat Linux)) id 1lgN4X-009N0J-Na for linux-arm-kernel@lists.infradead.org; Tue, 11 May 2021 07:44:55 +0000 Received: from disco-boy.misterjones.org (disco-boy.misterjones.org [51.254.78.96]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id A4E4861026; Tue, 11 May 2021 07:44:52 +0000 (UTC) Received: from 78.163-31-62.static.virginmediabusiness.co.uk ([62.31.163.78] helo=why.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1lgN4U-000cdO-F7; Tue, 11 May 2021 08:44:50 +0100 Date: Tue, 11 May 2021 08:44:49 +0100 Message-ID: <87o8dhogsu.wl-maz@kernel.org> From: Marc Zyngier To: Alexandru Elisei Cc: kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org, Zenghui Yu , James Morse , Suzuki K Poulose , kernel-team@android.com, stable@vger.kernel.org Subject: Re: [PATCH 2/2] KVM: arm64: Commit pending PC adjustemnts before returning to userspace In-Reply-To: <65b5cad7-13d8-13a9-9502-7c21e0b72761@arm.com> References: <20210510094915.1909484-1-maz@kernel.org> <20210510094915.1909484-3-maz@kernel.org> <7a0f43c8-cc36-810e-0b8e-ffe66672ca82@arm.com> <87v97qociy.wl-maz@kernel.org> <65b5cad7-13d8-13a9-9502-7c21e0b72761@arm.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") X-SA-Exim-Connect-IP: 62.31.163.78 X-SA-Exim-Rcpt-To: alexandru.elisei@arm.com, kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org, yuzenghui@huawei.com, james.morse@arm.com, suzuki.poulose@arm.com, kernel-team@android.com, stable@vger.kernel.org X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210511_004453_853984_9687C449 X-CRM114-Status: GOOD ( 44.33 ) /bin/ln: failed to access 'reaver_cache/texts/20210511_004453_853984_9687C449': No such file or directory X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210511_004453_853984_9687C449 X-CRM114-Status: GOOD ( 39.34 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Mon, 10 May 2021 16:14:37 +0100, Alexandru Elisei wrote: > > Hi Marc, > > On 5/10/21 4:04 PM, Marc Zyngier wrote: > > On Mon, 10 May 2021 15:55:28 +0100, > > Alexandru Elisei wrote: > >> Hi Marc, > >> > >> On 5/10/21 10:49 AM, Marc Zyngier wrote: > >>> KVM currently updates PC (and the corresponding exception state) > >>> using a two phase approach: first by setting a set of flags, > >>> then by converting these flags into a state update when the vcpu > >>> is about to enter the guest. > >>> > >>> However, this creates a disconnect with userspace if the vcpu thread > >>> returns there with any exception/PC flag set. In this case, the exposed > >> The code seems to handle only the KVM_ARM64_PENDING_EXCEPTION > >> flag. Is the "PC flag" a reference to the KVM_ARM64_INCREMENT_PC > >> flag? > > No, it does handle both exception and PC increment, unless I have > > completely bodged something (entirely possible). > > The message is correct, my bad. > > > > >>> context is wrong, as userpsace doesn't have access to these flags > >> s/userpsace/userspace > >> > >>> (they aren't architectural). It also means that these flags are > >>> preserved across a reset, which isn't expected. > >>> > >>> To solve this problem, force an explicit synchronisation of the > >>> exception state on vcpu exit to userspace. As an optimisation > >>> for nVHE systems, only perform this when there is something pending. > >>> > >>> Reported-by: Zenghui Yu > >>> Signed-off-by: Marc Zyngier > >>> Cc: stable@vger.kernel.org # 5.11 > >>> --- > >>> arch/arm64/include/asm/kvm_asm.h | 1 + > >>> arch/arm64/kvm/arm.c | 10 ++++++++++ > >>> arch/arm64/kvm/hyp/exception.c | 4 ++-- > >>> arch/arm64/kvm/hyp/nvhe/hyp-main.c | 8 ++++++++ > >>> 4 files changed, 21 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h > >>> index d5b11037401d..5e9b33cbac51 100644 > >>> --- a/arch/arm64/include/asm/kvm_asm.h > >>> +++ b/arch/arm64/include/asm/kvm_asm.h > >>> @@ -63,6 +63,7 @@ > >>> #define __KVM_HOST_SMCCC_FUNC___pkvm_cpu_set_vector 18 > >>> #define __KVM_HOST_SMCCC_FUNC___pkvm_prot_finalize 19 > >>> #define __KVM_HOST_SMCCC_FUNC___pkvm_mark_hyp 20 > >>> +#define __KVM_HOST_SMCCC_FUNC___kvm_adjust_pc 21 > >>> > >>> #ifndef __ASSEMBLY__ > >>> > >>> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c > >>> index 1cb39c0803a4..d62a7041ebd1 100644 > >>> --- a/arch/arm64/kvm/arm.c > >>> +++ b/arch/arm64/kvm/arm.c > >>> @@ -897,6 +897,16 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) > >>> > >>> kvm_sigset_deactivate(vcpu); > >>> > >>> + /* > >>> + * In the unlikely event that we are returning to userspace > >>> + * with pending exceptions or PC adjustment, commit these > >> I'm going to assume "PC adjustment" means the KVM_ARM64_INCREMENT_PC > >> flag. Please correct me if that's not true, but if that's the case, > >> then the flag isn't handled below. > >> > >>> + * adjustments in order to give userspace a consistent view of > >>> + * the vcpu state. > >>> + */ > >>> + if (unlikely(vcpu->arch.flags & (KVM_ARM64_PENDING_EXCEPTION | > >>> + KVM_ARM64_EXCEPT_MASK))) > >> The condition seems to suggest that it is valid to set > >> KVM_ARM64_EXCEPT_{AA32,AA64}_* without setting > >> KVM_ARM64_PENDING_EXCEPTION, which looks rather odd to me. > >> Is that a valid use of the KVM_ARM64_EXCEPT_MASK bits? If it's not > >> (the existing code always sets the exception type with the > >> KVM_ARM64_PENDING_EXCEPTION), that I was thinking that checking only > >> the KVM_ARM64_PENDING_EXCEPTION flag would make the intention > >> clearer. > > No, you are missing this (subtle) comment in kvm_host.h: > > > > > > /* > > * Overlaps with KVM_ARM64_EXCEPT_MASK on purpose so that it can't be > > * set together with an exception... > > */ > > #define KVM_ARM64_INCREMENT_PC (1 << 9) /* Increment PC */ > > > > > > So (KVM_ARM64_PENDING_EXCEPTION | KVM_ARM64_EXCEPT_MASK) checks for > > *both* an exception and a PC increment. > > Then how about explicitly checking for the > KVM_ARM64_PENDING_EXCEPTION and KVM_ARM64_INCREMENT_PC flags, like > it's done in __kvm_adjust_pc? That would certainly make the code > easier to understand, as it's not immediately obvious that the > EXCEPT mask includes the INCREMENT_PC flag. Fair enough. I'll fix that in v2. Another thing I wondered about: we now rely on __kvm_adjust_pc() to be preemption safe. That's always the case in nVHE (we're at EL2), but VHE can be preempted at any point. The code we call is preemption safe, but it takes some effort to be convinced of it. Do you have a good suggestion on how to express this requirement? I could throw a preempt_disable()/enable() at the call for the sake of being in the same context between VHE and nVHE, but that's not strictly necessary for now. Thanks, M. -- Without deviation from the norm, progress is not possible. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel