Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 30 Jun 2023 18:27:53 +0800
From:   Chao Gao <chao.gao@intel.com>
To:     "Yang, Weijiang" <weijiang.yang@intel.com>
CC:     Sean Christopherson <seanjc@google.com>, <pbonzini@redhat.com>,
        <kvm@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <peterz@infradead.org>, <rppt@kernel.org>,
        <binbin.wu@linux.intel.com>, <rick.p.edgecombe@intel.com>,
        <john.allen@amd.com>, <gil.neiger@intel.com>
Subject: Re: [PATCH v3 10/21] KVM:x86: Add #CP support in guest exception
 classification
Message-ID: <ZJ6uKZToMPfwoXW6@chao-email>
References: <20230511040857.6094-1-weijiang.yang@intel.com>
 <20230511040857.6094-11-weijiang.yang@intel.com>
 <ZH73kDx6VCaBFiyh@chao-email>
 <21568052-eb0f-a8d6-5225-3b422e9470e9@intel.com>
 <ZIulniryqlj0hLnt@google.com>
 <dfdf6d93-a68c-bb07-e59e-8d888dd6ebb6@intel.com>
 <ZIywqx6xTAMFyDPT@google.com>
 <0a98683f-3e60-1f1b-55df-f2a781929fdf@intel.com>
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <0a98683f-3e60-1f1b-55df-f2a781929fdf@intel.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?SDFjaGNsUXo2bndaYkxoVUFUTWtUN2xXOG5NWDUwakh2UXhrTjJjaUhlZHRy?=
 =?utf-8?B?MGFLRVBRN2lMQXlqcmFjV0Rac0dVN0VKQjNvNFNrRnpCYmhIcnp6MmI3Ujlr?=
 =?utf-8?B?a0x6UU0zLzVaV2Z6a3pGQ1F5MDlCN25vM1M1T2VCTVhBcjhYRzBDS1ZqWWJ2?=
 =?utf-8?B?UEdQSWNrMXJ3SzJMaStoRmRtSTJPVG80VDROS3BLREh1NVU2TTdHbkE2K1U1?=
 =?utf-8?B?Y2pDMm9yY0hRZ2ptdHBPOGUzMjNNc0JtbDljSFFGaE5jUDZVVkZwTU5FUjFI?=
 =?utf-8?B?S3kwUzhmYlZUNG16dUU4S3hLSldQV3pEREcwalNycm1NTGZUbDNmN1FLcFRI?=
 =?utf-8?B?bkJVU29Pa3NQRkRrMXN1cEJDeTBOMFJOSGc2ZU9ZaEtiRk5KTGMwN0YzYURl?=
 =?utf-8?B?bHNPOFpET01Vci9BMDhCQW9mYzgweUVwdHpRRm1na3dFV0FHUUpRRmhIY1B1?=
 =?utf-8?B?Vi95UzdDRFdyUXFuSytjUGduWGUwZkw4SjNoQ1BRQkNXT0NjSjJHc3JLeFZK?=
 =?utf-8?B?K0Mzb0NWL3JyTzZGUS80TUErOGF0YUpBNFAxRnVrU0R0Ni9YVjQ5OWRhZndl?=
 =?utf-8?B?UFB3YTRuVjNpaFBTcklucG9DcEQvK1RCTFZDRDlpb0d3bzJEL3Y2d0R0VjY3?=
 =?utf-8?B?VVZhNUVrSTc5Z21GSUlsemdueFhXL1cvUnVSaERoZTJra0dqbGt2Z2ZoVzlu?=
 =?utf-8?B?WkxSdHVESXMrZVRpL05ORXh6SEs1dnJCMWZQODB2RElySzZ6bjRueWVYc3JC?=
 =?utf-8?B?aWg0S0ptSkFoWXJLam0zakRqTlBqKzZ6MWtkc1RZcFpXUDc2cWJsdHluNWVR?=
 =?utf-8?B?MlNoRjRKZk5rcHd1am1QUXpBRTljTDVzUURFVzNPYzhlaFNwUnBMdjZCS0s2?=
 =?utf-8?B?TDNWeWVKWXJkZ0FCeEhNR2x0M2taQk1kZDY5dWpucmo3RVRIOEhmUDNhemZB?=
 =?utf-8?B?bmNxZDIveHV3Y3ptQ1h5cXl2bUUzTTNacVQvS1RLR1N5Q0kvSklOKzZHRGhi?=
 =?utf-8?B?TWl2NmdKcVhYUzJ1bUFiMVN1bmlQcVpRdGh5eHJiYUZ1SjVBMnRhSEU5Rm1y?=
 =?utf-8?B?L0tpWEJ6QlBVQVNPdWZ6S0U3b05xVlB0NGMvMUNDWFpuaytrRmhlNjhGcndu?=
 =?utf-8?B?VjVWZ051dCtpbHVvVzRBR2ZibzVPYjNybTAzVExTZ3NwZ0VpbzVLY3VvSStY?=
 =?utf-8?B?akxpVWNKMjhERlBTZUdlSlhzZVdGSll1ZkpyRjdERURNM0JhVFcybUVPVW9a?=
 =?utf-8?B?NzNOUVY4NVdGNEhKeHZZMWpMMzAyUmxzY25yTW9yaXhNMmhvemlJOVlWTGov?=
 =?utf-8?B?R2QyWHdhRHBvcDU1anlxTmxLODYvN1QyQzAyY0t1cXRxK3pDcFdCMlFMQkZZ?=
 =?utf-8?B?VFJrQkxDdUFuekdqYjNGNUhuSC9MMWZNSERWcWJHZzB0WllBQUdEaXE1RmpS?=
 =?utf-8?B?bTgxUmppSm1zK2UrMlFFZm9QVlVRdjBxRVhWYzNjNVpzZFRYUDI1TG1ITHYy?=
 =?utf-8?B?Vkd6T0ZUSTBiNC96bjhrbTdZZndSZ0xSd08rYnZISUFDTFl1VUtNVUxFRUN6?=
 =?utf-8?B?NkM1dGlHRVUyaEpIVTJtSEFCRThrWnRrL0VmcDEyanlMeklaSXpSUHdEY2ZJ?=
 =?utf-8?B?QityQUJLb1hSYmtEU05zOXErcHIvMHpEQXFscENZYTZNQzlRWWJ6QXdneFJE?=
 =?utf-8?B?Wk9OT0ljL1dyTUduZmVORTV5WEc4VnpvVmdRSnlZVW1odXI2M0VoQ2VIb3Vj?=
 =?utf-8?B?RGl2Y1Y1emZlWkxSQzVGdzNhMm4vZlVLcUxyNDNZOFBpK2g5SkNQQ2NLbTJL?=
 =?utf-8?B?QTFaZTY5amNYWnlyTU8wWnU0UVZKL0FWU2xkZDl5N0tOcU1TaEdjT2RoZ3h4?=
 =?utf-8?B?MTNwa0I1V0hJeUZRcUlMNGhHNXNSRHkwa3R3TjBXQmFWVjM0djd6bHhCM0Zq?=
 =?utf-8?B?NzBtd1BXRHhwbkxUbGxJYkhBa2JGQlNUN0NsdUdydGR5cGJabHp5UW1tdFpF?=
 =?utf-8?B?S3BuTHFFSGhsM1V5MXNGSUpDcnpUNFRLRVVxaUFjQW1SMU11THdhM3JZc0l6?=
 =?utf-8?B?bzRkeGtZVDR6Y0JiMHJQYndXMTVVbG9DaHNPVU9CaEk1Vm0rKzJTcDRmRE1i?=
 =?utf-8?Q?yRNxIhlfrcGqG7ZYUVNCgZ8ZL?=
X-MS-Exchange-CrossTenant-Network-Message-Id: f861412d-cbff-4ae5-4603-08db7954b07f
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB6780.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 Jun 2023 10:28:05.2846
 (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: TvRZif3JeNSZ+GkZXa65E/J9Ik/ip0P1dYW9L78UaPLf2dXE1lrZs0RIO7Vg9tCd7aDT2LFnGC4TRbzN9q3W0g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY5PR11MB6368
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Fri, Jun 30, 2023 at 05:34:28PM +0800, Yang, Weijiang wrote:
>
>On 6/17/2023 2:57 AM, Sean Christopherson wrote:
>> On Fri, Jun 16, 2023, Weijiang Yang wrote:
>> > On 6/16/2023 7:58 AM, Sean Christopherson wrote:
>> > > On Thu, Jun 08, 2023, Weijiang Yang wrote:
>> > > > On 6/6/2023 5:08 PM, Chao Gao wrote:
>> > > > > On Thu, May 11, 2023 at 12:08:46AM -0400, Yang Weijiang wrote:
>> > > > > > Add handling for Control Protection (#CP) exceptions(vector 21).
>> > > > > > The new vector is introduced for Intel's Control-Flow Enforcement
>> > > > > > Technology (CET) relevant violation cases.
>> > > > > > 
>> > > > > > Although #CP belongs contributory exception class, but the actual
>> > > > > > effect is conditional on CET being exposed to guest. If CET is not
>> > > > > > available to guest, #CP falls back to non-contributory and doesn't
>> > > > > > have an error code.
>> > > > > This sounds weird. is this the hardware behavior? If yes, could you
>> > > > > point us to where this behavior is documented?
>> > > > It's not SDM documented behavior.
>> > > The #CP behavior needs to be documented.  Please pester whoever you need to in
>> > > order to make that happen.
>> > Do you mean documentation for #CP as an generic exception or the behavior in
>> > KVM as this patch shows?
>> As I pointed out two *years* ago, this entry in the SDM
>> 
>>    — The field's deliver-error-code bit (bit 11) is 1 if each of the following
>>      holds: (1) the interruption type is hardware exception; (2) bit 0
>>      (corresponding to CR0.PE) is set in the CR0 field in the guest-state area;
>>      (3) IA32_VMX_BASIC[56] is read as 0 (see Appendix A.1); and (4) the vector
>>      indicates one of the following exceptions: #DF (vector 8), #TS (10),
>>      #NP (11), #SS (12), #GP (13), #PF (14), or #AC (17).
>> 
>> needs to read something like
>> 
>>    — The field's deliver-error-code bit (bit 11) is 1 if each of the following
>>      holds: (1) the interruption type is hardware exception; (2) bit 0
>>      (corresponding to CR0.PE) is set in the CR0 field in the guest-state area;
>>      (3) IA32_VMX_BASIC[56] is read as 0 (see Appendix A.1); and (4) the vector
>>      indicates one of the following exceptions: #DF (vector 8), #TS (10),
>>      #NP (11), #SS (12), #GP (13), #PF (14), #AC (17), or #CP (21)[1]
>> 
>>      [1] #CP has an error code if and only if IA32_VMX_CR4_FIXED1 enumerates
>>          support for the 1-setting of CR4.CET.
>
>Hi, Sean,
>
>I sent above change request to Gil(added in cc), but he shared different
>opinion on this issue:
>
>
>"It is the case that all CET-capable parts enumerate IA32_VMX_BASIC[56] as 1.
>
> However, there were earlier parts without CET that enumerated
>IA32_VMX_BASIC[56] as 0.
>
> On those parts, an attempt to inject an exception with vector 21 (#CP) with
>an error code would fail.
>
>(Injection of exception 21 with no error code would be allowed.)
>
> It may make things clearer if we document the statement above (all
>CET-capable parts enumerate IA32_VMX_BASIC[56] as 1).
>
>I will see if we can update future revisions of the SDM to clarify this."
>
>
>Then if this is the case,  kvm needs to check IA32_VMX_BASIC[56] before
>inject exception to nested VM.

And KVM can hide CET from guests if IA32_VMX_BASIC[56] is 0.

>
>And this patch could be removed, instead need another patch like below:
>
>diff --git a/arch/x86/include/asm/msr-index.h
>b/arch/x86/include/asm/msr-index.h
>index ad35355ee43e..6b33aacc8587 100644
>--- a/arch/x86/include/asm/msr-index.h
>+++ b/arch/x86/include/asm/msr-index.h
>@@ -1076,6 +1076,7 @@
> #define VMX_BASIC_MEM_TYPE_MASK    0x003c000000000000LLU
> #define VMX_BASIC_MEM_TYPE_WB    6LLU
> #define VMX_BASIC_INOUT        0x0040000000000000LLU
>+#define VMX_BASIC_CHECK_ERRCODE    0x0140000000000000LLU
>
> /* Resctrl MSRs: */
> /* - Intel: */
>diff --git a/arch/x86/kvm/vmx/capabilities.h
>b/arch/x86/kvm/vmx/capabilities.h
>index 85cffeae7f10..4b1ed4dc03bc 100644
>--- a/arch/x86/kvm/vmx/capabilities.h
>+++ b/arch/x86/kvm/vmx/capabilities.h
>@@ -79,6 +79,11 @@ static inline bool cpu_has_vmx_basic_inout(void)
>     return    (((u64)vmcs_config.basic_cap << 32) & VMX_BASIC_INOUT);
> }
>
>+static inline bool cpu_has_vmx_basic_check_errcode(void)
>+{
>+    return    (((u64)vmcs_config.basic_cap << 32) &
>VMX_BASIC_CHECK_ERRCODE);
>+}
>+
> static inline bool cpu_has_virtual_nmis(void)
> {
>     return vmcs_config.pin_based_exec_ctrl & PIN_BASED_VIRTUAL_NMIS &&
>diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
>index 78524daa2cb2..92aa4fc3d233 100644
>--- a/arch/x86/kvm/vmx/nested.c
>+++ b/arch/x86/kvm/vmx/nested.c
>@@ -1227,9 +1227,9 @@ static int vmx_restore_vmx_basic(struct vcpu_vmx *vmx,
>u64 data)
> {
>     const u64 feature_and_reserved =
>         /* feature (except bit 48; see below) */
>-        BIT_ULL(49) | BIT_ULL(54) | BIT_ULL(55) |
>+        BIT_ULL(49) | BIT_ULL(54) | BIT_ULL(55) | BIT_ULL(56) |
>         /* reserved */
>-        BIT_ULL(31) | GENMASK_ULL(47, 45) | GENMASK_ULL(63, 56);
>+        BIT_ULL(31) | GENMASK_ULL(47, 45) | GENMASK_ULL(63, 57);
>     u64 vmx_basic = vmcs_config.nested.basic;
>
>     if (!is_bitwise_subset(vmx_basic, data, feature_and_reserved))
>@@ -2873,7 +2873,8 @@ static int nested_check_vm_entry_controls(struct
>kvm_vcpu *vcpu,
>         should_have_error_code =
>             intr_type == INTR_TYPE_HARD_EXCEPTION && prot_mode &&
>             x86_exception_has_error_code(vector);
>-        if (CC(has_error_code != should_have_error_code))
>+        if (!cpu_has_vmx_basic_check_errcode() &&

We can skip computing should_have_error_code. and we should check if
IA32_VMX_BASIC[56] is set for this vCPU (i.e. in vmx->nested.msrs.basic)
rather than host/kvm capability.

>+            CC(has_error_code != should_have_error_code))
>             return -EINVAL;
>
>         /* VM-entry exception error code */
>@@ -6986,6 +6987,8 @@ static void nested_vmx_setup_basic(struct
>nested_vmx_msrs *msrs)
>
>     if (cpu_has_vmx_basic_inout())
>         msrs->basic |= VMX_BASIC_INOUT;
>+    if (cpu_has_vmx_basic_check_errcode())
>+        msrs->basic |= VMX_BASIC_CHECK_ERRCODE;
> }
>
> static void nested_vmx_setup_cr_fixed(struct nested_vmx_msrs *msrs)
>diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>index d70f2e94b187..95c0eab7805c 100644
>--- a/arch/x86/kvm/vmx/vmx.c
>+++ b/arch/x86/kvm/vmx/vmx.c
>@@ -2748,7 +2748,7 @@ static int setup_vmcs_config(struct vmcs_config
>*vmcs_conf,
>     rdmsrl(MSR_IA32_VMX_MISC, misc_msr);
>
>     vmcs_conf->size = vmx_msr_high & 0x1fff;
>-    vmcs_conf->basic_cap = vmx_msr_high & ~0x1fff;
>+    vmcs_conf->basic_cap = vmx_msr_high & ~0x7fff;
>
>     vmcs_conf->revision_id = vmx_msr_low;
>
>