Received-SPF: pass (google.com: domain of linux-kernel+bounces-25586-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: Mon, 15 Jan 2024 09:55:43 +0800
From: Chao Gao <chao.gao@intel.com>
To: "Yang, Weijiang" <weijiang.yang@intel.com>
CC: Sean Christopherson <seanjc@google.com>, Rick P Edgecombe
	<rick.p.edgecombe@intel.com>, Dave Hansen <dave.hansen@intel.com>,
	"peterz@infradead.org" <peterz@infradead.org>, "john.allen@amd.com"
	<john.allen@amd.com>, "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>, "pbonzini@redhat.com" <pbonzini@redhat.com>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>, "mlevitsk@redhat.com"
	<mlevitsk@redhat.com>
Subject: Re: [PATCH v8 00/26] Enable CET Virtualization
Message-ID: <ZaSQn7RCRTaBK1bc@chao-email>
References: <5f57ce03-9568-4739-b02d-e9fac6ed381a@intel.com>
 <6179ddcb25c683bd178e74e7e2455cee63ba74de.camel@intel.com>
 <ZZdLG5W5u19PsnTo@google.com>
 <a2344e2143ef2b9eca0d153c86091e58e596709d.camel@intel.com>
 <ZZdSSzCqvd-3sdBL@google.com>
 <8f070910-2b2e-425d-995e-dfa03a7695de@intel.com>
 <ZZgsipXoXTKyvCZT@google.com>
 <06fdd362-cb7f-47df-9d1a-9b85d2ed05b5@intel.com>
 <ZZ1h9GW93ckc3FlE@google.com>
 <cf043809-430a-4072-b0fd-201cd469b602@intel.com>
Content-Type: text/plain; charset="iso-8859-1"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <cf043809-430a-4072-b0fd-201cd469b602@intel.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?iso-8859-1?Q?GpxsuWMU8L9UFbRmwTtvmxSaMx3PruJlPQhFZyKoJq/A699WLAtiyHJYxQ?=
 =?iso-8859-1?Q?3CTOC5h4+kvXu+q2OUBOe5jjaLTs4NNH2kN5qCIjhb/9LApzXJSl81n4HQ?=
 =?iso-8859-1?Q?8iiFKaTERq1FcnwLRptpmBfAqNNtISoZcNchaNkk54f6UBEicavypPilQT?=
 =?iso-8859-1?Q?/ZyfdjMwtHil+C9ZP0V/LMjHJWIMvJ2nQYGtvlc/zspdnlEVgVm9Pz78fv?=
 =?iso-8859-1?Q?crE1Xx+mcgfOfoj8iJROWzOMXJFrplpx9CqN1lQf7dqyN0y81U2Rl+Friy?=
 =?iso-8859-1?Q?0WYETOJOFwE+mR7pT2ljYnZVhpmGlGQEN6dSm19CkkKXxMaMqfBmlFeHel?=
 =?iso-8859-1?Q?WY7MtA/r48+1a8oYZl0HeNQtX9XK3c/EmFmkE4G/dziqGZMQI1LFVOB7hc?=
 =?iso-8859-1?Q?5XCfKAvvRzxmP7myGmdBL6WI/f8Z6cOoFAsD6jbAMdN0ZWkqHyN74QCC48?=
 =?iso-8859-1?Q?ztOQCisTrQZ3IfFXDR9psvUSbD/iBGeE7m39Lzlz+QsJqJCmrVqTd+gT+j?=
 =?iso-8859-1?Q?0O6bOipUkueVN5/Z0pfL5P+GTMg3knjRbzh4+N9yqHHgaL90h1H3ntdaiS?=
 =?iso-8859-1?Q?DCRHGxKM07sp+q+PTOJC5eQU7QZ7y6ppXKsiQNyD0ilEbIwxcdBuTliajo?=
 =?iso-8859-1?Q?lf/Hpy5RTqNWjyPnWKH91Ckn61v9BhpDEBbf4kGgp0MLLe0sqcsEIk555y?=
 =?iso-8859-1?Q?N0HNJbV1KnThNg2TgiInf8EBN/tLywrzVa1n4YOshArli/qhAL3oPYHIpn?=
 =?iso-8859-1?Q?D69GFJWnd2wXcpJjUVF8WxqzTuJIwYykNDw4fEhESLC0cv8+TskxRKQhfq?=
 =?iso-8859-1?Q?0PV4ic6Z7yWug7m5YTWuSnZISglDJUWjvMwofKWzh2iF+qwzX3+RQJCTjO?=
 =?iso-8859-1?Q?rkGTjY87T9tizR9e72AUD9QmVd43XawYh1tXI1RARcDhmxcl3gK6LaC85t?=
 =?iso-8859-1?Q?qCoe9tdeGTmloCyYND5Zo0JlEhczE4JVvmULbaSRaY1J+ru5F/inQo5z1F?=
 =?iso-8859-1?Q?RjsEgm4g7fk2KZ7Qzt1Bej9UBHR6tTPc6RnOZ7hR+mPLxlynCjgaKKf5oG?=
 =?iso-8859-1?Q?XXSWWsmc9BiUPrclAIec2+5y/mNN/+ZjVskbj/XU5pF9YCy+rwjHUt1WC8?=
 =?iso-8859-1?Q?zpxlEr1A+OhkXKWJV0d8CvCDNqMCuxbT5dek3YpG5aEERzAhP+5ECFvZ5J?=
 =?iso-8859-1?Q?TR8VTAUQsRq1VRFg+ut7euDbt0QXRZ+86KIHKs9zwWctUbF8U3exUD1hti?=
 =?iso-8859-1?Q?ohVaNKYaGVzvKoWeKnAKbRHouW0TD1Bw6LcFyRUtb+tlqIxAQPs4xuoV2K?=
 =?iso-8859-1?Q?aMnqyOkkckFuI6TQwwdbMVGP2Bwi7nLO4lmL1CHU9DDI1Wo0mZcje31tUP?=
 =?iso-8859-1?Q?0EJk87HlV2/AF+6VZ3tWrn8XAaDYryyXJMoRxadRIL/zlFuBbf1l4g/tB1?=
 =?iso-8859-1?Q?78VlsvbL0ksQxKK9djUl9VOjfHq1YqBsxozcTVo+ykrlpf+KpF82GtS17q?=
 =?iso-8859-1?Q?1RBxYfmdfRjI0j8NiJzkwYf0yMlp9U1SToddGseBvBnPJPKbRHC0sd4Vaa?=
 =?iso-8859-1?Q?Jbc17Ewlequ6Uu5afh8atPLLKpY9xjwErk1XRd9nZaKT71un/q+W4CUzK1?=
 =?iso-8859-1?Q?9gmgHc48QSJD1wM+g8ahjIbDtWDWHLYwfV?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 0a8f98ac-a8b6-4240-0278-08dc156d1b12
X-MS-Exchange-CrossTenant-AuthSource: DS0PR11MB8665.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Jan 2024 01:55:53.2624
 (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: bGRMPd/4rwzVaS1JuWalLKC2cTjWTioKdNOt8L83wYSKGYczRkvzzwkMAkCOREilN7hhRqnnhTe7ezu/qgKZQQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH8PR11MB6855
X-OriginatorOrg: intel.com

On Thu, Jan 11, 2024 at 10:56:55PM +0800, Yang, Weijiang wrote:
>On 1/9/2024 11:10 PM, Sean Christopherson wrote:
>> On Mon, Jan 08, 2024, Weijiang Yang wrote:
>> > On 1/6/2024 12:21 AM, Sean Christopherson wrote:
>> > > On Fri, Jan 05, 2024, Weijiang Yang wrote:
>> > > > On 1/5/2024 8:54 AM, Sean Christopherson wrote:
>> > > > > On Fri, Jan 05, 2024, Rick P Edgecombe wrote:
>> > > > > > > For CALL/RET (and presumably any branch instructions with IBT?) other
>> > > > > > > instructions that are directly affected by CET, the simplest thing would
>> > > > > > > probably be to disable those in KVM's emulator if shadow stacks and/or IBT
>> > > > > > > are enabled, and let KVM's failure paths take it from there.
>> > > > > > Right, that is what I was wondering might be the normal solution for
>> > > > > > situations like this.
>> > > > > If KVM can't emulate something, it either retries the instruction (with some
>> > > > > decent logic to guard against infinite retries) or punts to userspace.
>> > > > What kind of error is proper if KVM has to punt to userspace?
>> > > KVM_INTERNAL_ERROR_EMULATION.  See prepare_emulation_failure_exit().
>> > > 
>> > > > Or just inject #UD into guest on detecting this case?
>> > > No, do not inject #UD or do anything else that deviates from architecturally
>> > > defined behavior.
>> > Thanks!
>> > But based on current KVM implementation and patch 24, seems that if CET is exposed
>> > to guest, the emulation code or shadow paging mode couldn't be activated at the same time:
>> No, requiring unrestricted guest only disables the paths where KVM *delibeately*
>> emulates the entire guest code stream.  In no way, shape, or form does it prevent
>> KVM from attempting to emulate arbitrary instructions.
>
>Yes, also need to prevent sporadic emulation, how about adding below patch in emulator?
>
>
>diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
>index e223043ef5b2..e817d8560ceb 100644
>--- a/arch/x86/kvm/emulate.c
>+++ b/arch/x86/kvm/emulate.c
>@@ -178,6 +178,7 @@
>?#define IncSP?????? ((u64)1 << 54)? /* SP is incremented before ModRM calc */
>?#define TwoMemOp??? ((u64)1 << 55)? /* Instruction has two memory operand */
>?#define IsBranch??? ((u64)1 << 56)? /* Instruction is considered a branch. */
>+#define IsProtected ((u64)1 << 57)? /* Instruction is protected by CET. */
>
>?#define DstXacc???? (DstAccLo | SrcAccHi | SrcWrite)
>
>@@ -4098,9 +4099,9 @@ static const struct opcode group4[] = {
>?static const struct opcode group5[] = {
>??????? F(DstMem | SrcNone | Lock,????????????? em_inc),
>??????? F(DstMem | SrcNone | Lock,????????????? em_dec),
>-?????? I(SrcMem | NearBranch | IsBranch,?????? em_call_near_abs),
>-?????? I(SrcMemFAddr | ImplicitOps | IsBranch, em_call_far),
>-?????? I(SrcMem | NearBranch | IsBranch,?????? em_jmp_abs),
>+?????? I(SrcMem | NearBranch | IsBranch | IsProtected, em_call_near_abs),
>+?????? I(SrcMemFAddr | ImplicitOps | IsBranch | IsProtected, em_call_far),
>+?????? I(SrcMem | NearBranch | IsBranch | IsProtected, em_jmp_abs),
>??????? I(SrcMemFAddr | ImplicitOps | IsBranch, em_jmp_far),
>??????? I(SrcMem | Stack | TwoMemOp,??????????? em_push), D(Undefined),
>?};
>@@ -4362,11 +4363,11 @@ static const struct opcode opcode_table[256] = {
>??????? /* 0xC8 - 0xCF */
>??????? I(Stack | SrcImmU16 | Src2ImmByte | IsBranch, em_enter),
>??????? I(Stack | IsBranch, em_leave),
>-?????? I(ImplicitOps | SrcImmU16 | IsBranch, em_ret_far_imm),
>-?????? I(ImplicitOps | IsBranch, em_ret_far),
>-?????? D(ImplicitOps | IsBranch), DI(SrcImmByte | IsBranch, intn),
>+?????? I(ImplicitOps | SrcImmU16 | IsBranch | IsProtected, em_ret_far_imm),
>+?????? I(ImplicitOps | IsBranch | IsProtected, em_ret_far),
>+?????? D(ImplicitOps | IsBranch), DI(SrcImmByte | IsBranch | IsProtected, intn),
>??????? D(ImplicitOps | No64 | IsBranch),
>-?????? II(ImplicitOps | IsBranch, em_iret, iret),
>+?????? II(ImplicitOps | IsBranch | IsProtected, em_iret, iret),
>??????? /* 0xD0 - 0xD7 */
>??????? G(Src2One | ByteOp, group2), G(Src2One, group2),
>??????? G(Src2CL | ByteOp, group2), G(Src2CL, group2),
>@@ -4382,7 +4383,7 @@ static const struct opcode opcode_table[256] = {
>??????? I2bvIP(SrcImmUByte | DstAcc, em_in,? in,? check_perm_in),
>??????? I2bvIP(SrcAcc | DstImmUByte, em_out, out, check_perm_out),
>??????? /* 0xE8 - 0xEF */
>-?????? I(SrcImm | NearBranch | IsBranch, em_call),
>+?????? I(SrcImm | NearBranch | IsBranch | IsProtected, em_call),
>??????? D(SrcImm | ImplicitOps | NearBranch | IsBranch),
>??????? I(SrcImmFAddr | No64 | IsBranch, em_jmp_far),
>??????? D(SrcImmByte | ImplicitOps | NearBranch | IsBranch),
>@@ -4401,7 +4402,7 @@ static const struct opcode opcode_table[256] = {
>?static const struct opcode twobyte_table[256] = {
>??????? /* 0x00 - 0x0F */
>??????? G(0, group6), GD(0, &group7), N, N,
>-?????? N, I(ImplicitOps | EmulateOnUD | IsBranch, em_syscall),
>+?????? N, I(ImplicitOps | EmulateOnUD | IsBranch | IsProtected, em_syscall),
>??????? II(ImplicitOps | Priv, em_clts, clts), N,
>??????? DI(ImplicitOps | Priv, invd), DI(ImplicitOps | Priv, wbinvd), N, N,
>??????? N, D(ImplicitOps | ModRM | SrcMem | NoAccess), N, N,
>@@ -4432,8 +4433,8 @@ static const struct opcode twobyte_table[256] = {
>??????? IIP(ImplicitOps, em_rdtsc, rdtsc, check_rdtsc),
>??????? II(ImplicitOps | Priv, em_rdmsr, rdmsr),
>??????? IIP(ImplicitOps, em_rdpmc, rdpmc, check_rdpmc),
>-?????? I(ImplicitOps | EmulateOnUD | IsBranch, em_sysenter),
>-?????? I(ImplicitOps | Priv | EmulateOnUD | IsBranch, em_sysexit),
>+?????? I(ImplicitOps | EmulateOnUD | IsBranch | IsProtected, em_sysenter),
>+?????? I(ImplicitOps | Priv | EmulateOnUD | IsBranch | IsProtected, em_sysexit),
>??????? N, N,
>??????? N, N, N, N, N, N, N, N,
>??????? /* 0x40 - 0x4F */
>@@ -4971,6 +4972,12 @@ int x86_decode_insn(struct x86_emulate_ctxt *ctxt, void *insn, int insn_len, int
>??????? if (ctxt->d == 0)
>??????????????? return EMULATION_FAILED;
>+?????? if ((opcode.flags & IsProtected) &&
>+?????????? (ctxt->ops->get_cr(ctxt, 4) & X86_CR4_CET)) {

CR4.CET doesn't necessarily mean IBT or shadow stack is enabled. why not check
CPL and IA32_S/U_CET?

>+?????????????? WARN_ONCE(1, "CET is active, emulation aborted.\n");

remove this WARN_ONCE(). Guest can trigger this at will and overflow host dmesg.

if you really want to tell usespace the emulation_failure is due to CET, maybe
you can add a new flag like KVM_INTERNAL_ERROR_EMULATION_FLAG_INSTRUCTION_BYTES.
for now, I won't bother to add this because probably userspace just terminates
the VM on any instruction failure (i.e., won't try to figure out the reason of
the instruction failure and fix it).