Received-SPF: pass (google.com: domain of linux-kernel+bounces-72241-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Tue, 20 Feb 2024 11:04:27 +0800
From: Chao Gao <chao.gao@intel.com>
To: Yang Weijiang <weijiang.yang@intel.com>
CC: <seanjc@google.com>, <pbonzini@redhat.com>, <dave.hansen@intel.com>,
	<x86@kernel.org>, <kvm@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<peterz@infradead.org>, <rick.p.edgecombe@intel.com>, <mlevitsk@redhat.com>,
	<john.allen@amd.com>
Subject: Re: [PATCH v10 10/27] KVM: x86: Refine xsave-managed guest
 register/MSR reset handling
Message-ID: <ZdQWu3D3Jku1iAvd@chao-email>
References: <20240219074733.122080-1-weijiang.yang@intel.com>
 <20240219074733.122080-11-weijiang.yang@intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240219074733.122080-11-weijiang.yang@intel.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?GBqF/FsjWZbMWkZPZ8EC8n9jqD8cUs5FzK+Pt+6Jfl8BuQWuq+8kRcCaHACa?=
 =?us-ascii?Q?E15xnzYWCgCLlDTOnOR0et4CgAkW9nONNfLrFtR9gVXLiQK1FIhGGdlQOwkf?=
 =?us-ascii?Q?phox+NvwcOqxJwbHT1WlNEDvTpHDlJzMiFDZegwllPUtNX9/9xlttYlOTqb7?=
 =?us-ascii?Q?UBiEaHRjsRO9+iMW4pGy6pI1gPjqYL7irrGNKFkqgrQUrCjXl4TNl8Hy072l?=
 =?us-ascii?Q?nF5XhbzFan9nmlb/UOSWO8UFcIPVmL+VEZXy2p9W0RUwhJ/TqkSzU04VA11D?=
 =?us-ascii?Q?YYAJqxnHwYqh/m93HWL8DyMqH4x5mmmkinud4zzzu73MgPidejxoAgGKO+Ap?=
 =?us-ascii?Q?ut8ewpfCJzKc2qhGTZ0ziYhAhj54ByldLez4JI86QW+eoEgg0FcpLgrEEI2J?=
 =?us-ascii?Q?ZnoVXX6a8lb4Qnhbfqon4CwI1UdtE0Tjtgp0LdZvc4mi0rclBSNpD1C5cWez?=
 =?us-ascii?Q?uJFtoGYHFSpwiiq82gNE6koNhCVIdZF0cWG25h92dLNDIR3uvwFb5nQVPw5U?=
 =?us-ascii?Q?Pv0MzjGjeRhC2hjJhZvUgCANio+VPeoMyvOSE8KzoJ9SUiEqEAZmTX5g2vrx?=
 =?us-ascii?Q?eudEMUBM0LuIjFKAYyGonTpakqLDbioRMpSKsmVlIamyuKrYvPtiobtvYYYH?=
 =?us-ascii?Q?rvjmB+UyPIn1jG/PaZubo7uyzXa9UTmmak9A8eYmpbX9UGffFaez93GmQYSj?=
 =?us-ascii?Q?jomOWomht6DILpYDdqPfIOXQ/U6u5AmLeQtgq4IQa56KoBz6JGoI9AdzuiqU?=
 =?us-ascii?Q?V7LcVteOydjT+MP3ngtIBByNQflLDUt2BYE22o5XzFqzu5aIQljk/fi3GS82?=
 =?us-ascii?Q?y4LI9PLlnD5GFz0H9fY8MuahZk+zrbYLqQcOe3V3klkRPXzQP4I55g8iOe6W?=
 =?us-ascii?Q?yFqdoU789vkiJLYx203ZVpJDoQO96VtBPaASFQmumXxY2Sd3w/J+dZO4qHtO?=
 =?us-ascii?Q?fDfu1ughf940BhYtu180ZdhAyfQOaYB8FqW5ABCT0V0pKA04S98jNYBbljI6?=
 =?us-ascii?Q?4mZ/yqh3G3xnM8BaPK9Spzvjg1d6xg6T8elBUPsv9AX9/jkrprnILGskoqCG?=
 =?us-ascii?Q?lIItK9sVempTdBtqgnLZsA00jaziz8dCB5VnJueEQbyhx2lhnKuNiaxkuQP2?=
 =?us-ascii?Q?g92trwKBBpxl8QSG/2vkrmnwUCSEvx/m2sDW1fPKd5gfNbHYPjNIAjtqLCNn?=
 =?us-ascii?Q?vFki8Xxp3eGhMjW0p8ZgJyIYR6MiFzjkKV32rqE3+ouL+gvEX1BgPZdUow29?=
 =?us-ascii?Q?XM9ySCLtAKcPao2JqDSBLStLLJm99gtqie8gWShvSk8Cb89dZnsc2swtk0gh?=
 =?us-ascii?Q?HQr5ivZFb/AcAVPUv/HV0gXk80sy8SqeWxZXEeXgxrFm0ms39Ttup40XHL2e?=
 =?us-ascii?Q?vP/LztXHwafxH2YoZ3oCHNERB1QnZ2BHYAgy8+1VVxHETUvf8wezjYYgh9i0?=
 =?us-ascii?Q?+SnSiVsotLWSRGzGfDUFnGMjU6thTzar82EK36v4hPLlkZA2DToukARHO7JO?=
 =?us-ascii?Q?7zz+/XF7Cy8jlWJK5vSex6XRQvRyxFKhisiuuus38buR8VEOFrCUkaUqcRi5?=
 =?us-ascii?Q?wU4KBi0qDEvT0lQauCTuLHkiy9SxeKDUnDbt5Pif?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 07f743db-61f6-476c-fa0b-08dc31c0abba
X-MS-Exchange-CrossTenant-AuthSource: CH3PR11MB8660.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Feb 2024 03:04:36.7743
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: T7WVEpzgj1WTEl7b2USL+D9N9qXBr0gpSxhDmfDswe3i3W0GZZK4Nhngnk9rCKURKkF7FgQlx5QjPmdaNAzhRg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR11MB5408
X-OriginatorOrg: intel.com

On Sun, Feb 18, 2024 at 11:47:16PM -0800, Yang Weijiang wrote:
>Tweak the code a bit to facilitate resetting more xstate components in
>the future, e.g., CET's xstate-managed MSRs.
>

>No functional change intended.

Strictly speaking, there is a functional change. in the previous logic, if
either of BNDCSR or BNDREGS state is not supported (kvm_mpx_supported() will
return false), KVM won't reset either of them. Since this gets changed, I vote
to drop 'No functional change ..'

>
>Suggested-by: Chao Gao <chao.gao@intel.com>
>Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
>---
> arch/x86/kvm/x86.c | 30 +++++++++++++++++++++++++++---
> 1 file changed, 27 insertions(+), 3 deletions(-)
>
>diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>index 10847e1cc413..5a9c07751c0e 100644
>--- a/arch/x86/kvm/x86.c
>+++ b/arch/x86/kvm/x86.c
>@@ -12217,11 +12217,27 @@ void kvm_arch_vcpu_destroy(struct kvm_vcpu *vcpu)
> 		static_branch_dec(&kvm_has_noapic_vcpu);
> }
> 
>+#define XSTATE_NEED_RESET_MASK	(XFEATURE_MASK_BNDREGS | \
>+				 XFEATURE_MASK_BNDCSR)
>+
>+static bool kvm_vcpu_has_xstate(unsigned long xfeature)

kvm_vcpu_has_xstate is a misnomer because it doesn't take a vCPU.

>+{
>+	switch (xfeature) {
>+	case XFEATURE_MASK_BNDREGS:
>+	case XFEATURE_MASK_BNDCSR:
>+		return kvm_cpu_cap_has(X86_FEATURE_MPX);
>+	default:
>+		return false;
>+	}
>+}
>+
> void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
> {
> 	struct kvm_cpuid_entry2 *cpuid_0x1;
> 	unsigned long old_cr0 = kvm_read_cr0(vcpu);
>+	DECLARE_BITMAP(reset_mask, 64);
> 	unsigned long new_cr0;
>+	unsigned int i;
> 
> 	/*
> 	 * Several of the "set" flows, e.g. ->set_cr0(), read other registers
>@@ -12274,7 +12290,12 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
> 	kvm_async_pf_hash_reset(vcpu);
> 	vcpu->arch.apf.halted = false;
> 
>-	if (vcpu->arch.guest_fpu.fpstate && kvm_mpx_supported()) {
>+	bitmap_from_u64(reset_mask, (kvm_caps.supported_xcr0 |
>+				     kvm_caps.supported_xss) &
>+				    XSTATE_NEED_RESET_MASK);
>+
>+	if (vcpu->arch.guest_fpu.fpstate &&
>+	    !bitmap_empty(reset_mask, XFEATURE_MAX)) {
> 		struct fpstate *fpstate = vcpu->arch.guest_fpu.fpstate;
> 
> 		/*
>@@ -12284,8 +12305,11 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
> 		if (init_event)
> 			kvm_put_guest_fpu(vcpu);
> 
>-		fpstate_clear_xstate_component(fpstate, XFEATURE_BNDREGS);
>-		fpstate_clear_xstate_component(fpstate, XFEATURE_BNDCSR);
>+		for_each_set_bit(i, reset_mask, XFEATURE_MAX) {
>+			if (!kvm_vcpu_has_xstate(i))
>+				continue;

The kvm_vcpu_has_xstate() check is superfluous because @i is derived from
kvm_caps.supported_xcr0/xss, which already guarantees that all unsupported
xfeatures are filtered out.

I recommend dropping this check. w/ this change,

Reviewed-by: Chao Gao <chao.gao@intel.com>

>+			fpstate_clear_xstate_component(fpstate, i);
>+		}
> 
> 		if (init_event)
> 			kvm_load_guest_fpu(vcpu);
>-- 
>2.43.0
>