Received-SPF: pass (google.com: domain of linux-kernel+bounces-161810-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Mon, 29 Apr 2024 14:31:51 +0800
From: Chao Gao <chao.gao@intel.com>
To: Xin Li <xin3.li@intel.com>
CC: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>, "linux-doc@vger.kernel.org"
	<linux-doc@vger.kernel.org>, "linux-kselftest@vger.kernel.org"
	<linux-kselftest@vger.kernel.org>, "seanjc@google.com" <seanjc@google.com>,
	"pbonzini@redhat.com" <pbonzini@redhat.com>, "corbet@lwn.net"
	<corbet@lwn.net>, "tglx@linutronix.de" <tglx@linutronix.de>,
	"mingo@redhat.com" <mingo@redhat.com>, "bp@alien8.de" <bp@alien8.de>,
	"dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>, "x86@kernel.org"
	<x86@kernel.org>, "hpa@zytor.com" <hpa@zytor.com>, "shuah@kernel.org"
	<shuah@kernel.org>, "vkuznets@redhat.com" <vkuznets@redhat.com>,
	"peterz@infradead.org" <peterz@infradead.org>, "Shankar, Ravi V"
	<ravi.v.shankar@intel.com>, "xin@zytor.com" <xin@zytor.com>
Subject: Re: [PATCH v2 10/25] KVM: VMX: Add support for FRED context
 save/restore
Message-ID: <Zi8+1wpOWAXDABbm@chao-email>
References: <20240207172646.3981-1-xin3.li@intel.com>
 <20240207172646.3981-11-xin3.li@intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240207172646.3981-11-xin3.li@intel.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?2JhdFyEs/M2DgdEJuunxS2D+30JaqgPws5S2rSnDqbE+lR0IGXeH0uILJJYl?=
 =?us-ascii?Q?4qK5ZlAtF1hV3VpvsMB36JojFd94fusHMO6UuTgikFs8v+Ortr+ry5M2cQ7X?=
 =?us-ascii?Q?HFQDL3CEAqTfGr/t9Td0SuDmIzc1BXOt/Zw4+QAu008Uuf8SP3clk9+bcj7l?=
 =?us-ascii?Q?58eWacnBkWLHK1vaZdvArko8kL3MJqQM8qaMLpJmrA7Jb5cz9RWp6yyJUO8F?=
 =?us-ascii?Q?KbqPny9BnfqLU8+mtDkSyiJFvtIcHAj5v0ZSXCPeoFOq86z7Pp11OwNyWdCY?=
 =?us-ascii?Q?KjR0r3Yp330GKmDdH1MKr02VnOTK9yz1EgDVb1YjVoTyQyOsQyZXWwSLSGij?=
 =?us-ascii?Q?hfpLylNlt8Dmjx3aPzeLplbnBvMlxozq+UwEtbN6d7uNWpfgVINw0k1fevNm?=
 =?us-ascii?Q?5Gm/r+CkkJXwFk/5537jyg46mLnv/uzmUMFEuum8opUlgZqTKBoR/xfxp4/X?=
 =?us-ascii?Q?2v8YQICgGoM4wrKgZc9clH2nUtifDM9hh+9PF1Mivpo4gMenFRElVKTt969G?=
 =?us-ascii?Q?UmqheGdjXdkEk2R1zArsFwpcXqo4+Y5TbtZiLaSZ0V6mMPCPNc4b0nW8wKJw?=
 =?us-ascii?Q?gF4Gk8/45/bihLg/4J1Vk15iT8uNUq+N406KG5BVnmSAloALhoiHK7vrbYlF?=
 =?us-ascii?Q?0J5XKkW1ysgiALfWsbakpzGOCQzpMler+7FQSmxkSpMqBqa+nqGLIhnpCDlr?=
 =?us-ascii?Q?0LE84h0DB3nO5Ir2lHj2OL6HzzGAIhtNJg+5oJ/xfy0f6uOIl8nTJLVd3wTv?=
 =?us-ascii?Q?IK5lnxsmUOFfHxFyLGLa+4ZdgtPuq0+3oLV1/q4FK14WzW/OQ+SNNQcz2NDf?=
 =?us-ascii?Q?7XqGLowpqscC/wrcqBFjodwglSd42RIhg+l01xBF6Ec31fIZfg01X/53/N9y?=
 =?us-ascii?Q?pYFH16/JTgoHxaZYgctnLm6gnzV9q2IuT0nv9jSEtEMgNCdLnXCfV4ScNrfN?=
 =?us-ascii?Q?KvZutyoXTtpnTnDVBLWTaHta6c9QqjBmgQdTPlmhBuHzqf7p23CQmeutCk45?=
 =?us-ascii?Q?FAAaaQvq43eDVOmoSdeTXMMVZ2DbMqaYxIy+3P15Bmo0nmRW/n0tqSDuReZw?=
 =?us-ascii?Q?rkldIMsI5Q9/rc072tztAh65bcSBP0xHTCkW/egi9levB1XioTzId8uGOyPI?=
 =?us-ascii?Q?HdeyaidHya/6OjZZmcCMVDrNxbtuuRBYkHUXEelANpyAzup9bQPiLCzKwKZs?=
 =?us-ascii?Q?l6SjHpW7h6OtRhNxoGKyS1wGx7wEwo9BNBPvQ8vIb08jdQjFxNCR7CC/rlZ9?=
 =?us-ascii?Q?EA8d6T73qM33s5OUGfhBKZdIgi+HcAOsGk/uIUIgXNcBCD5cLgGouI09r72q?=
 =?us-ascii?Q?ORhWUBQHnhttBq41ycGU3Wxl5CGDTkkVu+zYM7Jpw2KMJKAOLjbySXNVdQtg?=
 =?us-ascii?Q?hEwYZ1PJ+4CtOSj/Hf60ltWs5FFL3/Mrj2ebK83FbotQ8lvmSSOWx+GjdoFr?=
 =?us-ascii?Q?VkJrgXKVSIiSWpNv31B/sXpGF7+Bv+5kfccs9if2C8G/yGKYMlOhsIMfSKmx?=
 =?us-ascii?Q?WEu4Li66fRiMpMTO/wNv2bfCEfhI8lnYWy4/CuDBZ7P3c+4TDr6WfvBGr0OP?=
 =?us-ascii?Q?W4eKwkHIS9U53giYyHqXPMUvEuUHD3M3dB1OtLGw?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 4a99fa9b-f4d7-448a-9e0b-08dc681614d3
X-MS-Exchange-CrossTenant-AuthSource: CH3PR11MB8660.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Apr 2024 06:32:03.1264
 (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: N6dWaqLhsFy1TRB9h6M4iDt5/qdNjY+byHzBPF4zu9Gtg08kvMpN9jaqN6DAuKbVmKFKjBqmLa4UVeGhuGl+Ug==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA2PR11MB5036
X-OriginatorOrg: intel.com

On Thu, Feb 08, 2024 at 01:26:30AM +0800, Xin Li wrote:
>Handle host initiated FRED MSR access requests to allow FRED context
>to be set/get from user level.
>

The changelog isn't accurate because guest accesses are also handled
by this patch, specifically in the "else" branch.

	>+		if (host_initiated) {
	>+			if (!kvm_cpu_cap_has(X86_FEATURE_FRED))
	>+				return 1;
	>+		} else {


> void vmx_vcpu_load_vmcs(struct kvm_vcpu *vcpu, int cpu,
>@@ -2019,6 +2037,33 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> 	case MSR_KERNEL_GS_BASE:
> 		msr_info->data = vmx_read_guest_kernel_gs_base(vmx);
> 		break;
>+	case MSR_IA32_FRED_RSP0:
>+		msr_info->data = vmx_read_guest_fred_rsp0(vmx);
>+		break;
>+	case MSR_IA32_FRED_RSP1:
>+		msr_info->data = vmcs_read64(GUEST_IA32_FRED_RSP1);
>+		break;
>+	case MSR_IA32_FRED_RSP2:
>+		msr_info->data = vmcs_read64(GUEST_IA32_FRED_RSP2);
>+		break;
>+	case MSR_IA32_FRED_RSP3:
>+		msr_info->data = vmcs_read64(GUEST_IA32_FRED_RSP3);
>+		break;
>+	case MSR_IA32_FRED_STKLVLS:
>+		msr_info->data = vmcs_read64(GUEST_IA32_FRED_STKLVLS);
>+		break;
>+	case MSR_IA32_FRED_SSP1:
>+		msr_info->data = vmcs_read64(GUEST_IA32_FRED_SSP1);
>+		break;
>+	case MSR_IA32_FRED_SSP2:
>+		msr_info->data = vmcs_read64(GUEST_IA32_FRED_SSP2);
>+		break;
>+	case MSR_IA32_FRED_SSP3:
>+		msr_info->data = vmcs_read64(GUEST_IA32_FRED_SSP3);
>+		break;
>+	case MSR_IA32_FRED_CONFIG:
>+		msr_info->data = vmcs_read64(GUEST_IA32_FRED_CONFIG);
>+		break;

how about adding a helper function to convert MSR index to the VMCS field id?
Then do:

	case MSR_IA32_FRED_RSP1 ... MSR_IA32_FRED_STKLVLS:
	case MSR_IA32_FRED_SSP1 ... MSR_IA32_FRED_CONFIG:
		msr_info->data = vmcs_read64(msr_to_vmcs(index));
		break;

and ...

> #endif
> 	case MSR_EFER:
> 		return kvm_get_msr_common(vcpu, msr_info);
>@@ -2226,6 +2271,33 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> 			vmx_update_exception_bitmap(vcpu);
> 		}
> 		break;
>+	case MSR_IA32_FRED_RSP0:
>+		vmx_write_guest_fred_rsp0(vmx, data);
>+		break;
>+	case MSR_IA32_FRED_RSP1:
>+		vmcs_write64(GUEST_IA32_FRED_RSP1, data);
>+		break;
>+	case MSR_IA32_FRED_RSP2:
>+		vmcs_write64(GUEST_IA32_FRED_RSP2, data);
>+		break;
>+	case MSR_IA32_FRED_RSP3:
>+		vmcs_write64(GUEST_IA32_FRED_RSP3, data);
>+		break;
>+	case MSR_IA32_FRED_STKLVLS:
>+		vmcs_write64(GUEST_IA32_FRED_STKLVLS, data);
>+		break;
>+	case MSR_IA32_FRED_SSP1:
>+		vmcs_write64(GUEST_IA32_FRED_SSP1, data);
>+		break;
>+	case MSR_IA32_FRED_SSP2:
>+		vmcs_write64(GUEST_IA32_FRED_SSP2, data);
>+		break;
>+	case MSR_IA32_FRED_SSP3:
>+		vmcs_write64(GUEST_IA32_FRED_SSP3, data);
>+		break;
>+	case MSR_IA32_FRED_CONFIG:
>+		vmcs_write64(GUEST_IA32_FRED_CONFIG, data);
>+		break;

	case MSR_IA32_FRED_RSP1 ... MSR_IA32_FRED_STKLVLS:
	case MSR_IA32_FRED_SSP1 ... MSR_IA32_FRED_CONFIG:
		vmcs_write64(msr_to_vmcs(index), data);
		break;

The code will be more compact and generate less instructions.  I believe CET
series can do the same change [*]. Performance here isn't critical. I just
think it looks cumbersome to repeat the same pattern for 8 (and more with
CET considered) MSRs.

[*]: https://lore.kernel.org/kvm/20240219074733.122080-21-weijiang.yang@intel.com/

> #endif
> 	case MSR_IA32_SYSENTER_CS:
> 		if (is_guest_mode(vcpu))
>diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>index 363b1c080205..4e8d60f248e3 100644
>--- a/arch/x86/kvm/x86.c
>+++ b/arch/x86/kvm/x86.c
>@@ -1451,6 +1451,9 @@ static const u32 msrs_to_save_base[] = {
> 	MSR_STAR,
> #ifdef CONFIG_X86_64
> 	MSR_CSTAR, MSR_KERNEL_GS_BASE, MSR_SYSCALL_MASK, MSR_LSTAR,
>+	MSR_IA32_FRED_RSP0, MSR_IA32_FRED_RSP1, MSR_IA32_FRED_RSP2,
>+	MSR_IA32_FRED_RSP3, MSR_IA32_FRED_STKLVLS, MSR_IA32_FRED_SSP1,
>+	MSR_IA32_FRED_SSP2, MSR_IA32_FRED_SSP3, MSR_IA32_FRED_CONFIG,
> #endif
> 	MSR_IA32_TSC, MSR_IA32_CR_PAT, MSR_VM_HSAVE_PA,
> 	MSR_IA32_FEAT_CTL, MSR_IA32_BNDCFGS, MSR_TSC_AUX,
>@@ -1892,6 +1895,30 @@ static int __kvm_set_msr(struct kvm_vcpu *vcpu, u32 index, u64 data,
> 			return 1;
> 
> 		data = (u32)data;
>+		break;
>+	case MSR_IA32_FRED_RSP0 ... MSR_IA32_FRED_CONFIG:
>+		if (index != MSR_IA32_FRED_STKLVLS && is_noncanonical_address(data, vcpu))
>+			return 1;
>+		if ((index >= MSR_IA32_FRED_RSP0 && index <= MSR_IA32_FRED_RSP3) &&
>+		    (data & GENMASK_ULL(5, 0)))
>+			return 1;
>+		if ((index >= MSR_IA32_FRED_SSP1 && index <= MSR_IA32_FRED_SSP3) &&
>+		    (data & GENMASK_ULL(2, 0)))
>+			return 1;
>+
>+		if (host_initiated) {
>+			if (!kvm_cpu_cap_has(X86_FEATURE_FRED))
>+				return 1;

Should be:
			if (!kvm_cpu_cap_has(X86_FEATURE_FRED) && data)

KVM ABI allows userspace to write only 0 if guests cannot enumerate the
feature. And even better, your next version can be on top of Sean's series

https://lore.kernel.org/kvm/20240425181422.3250947-1-seanjc@google.com/T/#md00be687770e1e658fc9fe0eac20b5f0bd230e4c

this way, you can get rid of the "host_initiated" check.