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 08:07:52 -0700
In-Reply-To: <0a98683f-3e60-1f1b-55df-f2a781929fdf@intel.com>
Mime-Version: 1.0
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>
Message-ID: <ZJ7vyBw1nbTBOfuf@google.com>
Subject: Re: [PATCH v3 10/21] KVM:x86: Add #CP support in guest exception classification
From:   Sean Christopherson <seanjc@google.com>
To:     Weijiang Yang <weijiang.yang@intel.com>
Cc:     Chao Gao <chao.gao@intel.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
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, Jun 30, 2023, Weijiang Yang wrote:
>=20
> On 6/17/2023 2:57 AM, Sean Christopherson wrote:
> > > Do you mean documentation for #CP as an generic exception or the beha=
vior in
> > > KVM as this patch shows?
> > As I pointed out two *years* ago, this entry in the SDM
> >=20
> >    =E2=80=94 The field's deliver-error-code bit (bit 11) is 1 if each o=
f 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-sta=
te area;
> >      (3) IA32_VMX_BASIC[56] is read as 0 (see Appendix A.1); and (4) th=
e vector
> >      indicates one of the following exceptions: #DF (vector 8), #TS (10=
),
> >      #NP (11), #SS (12), #GP (13), #PF (14), or #AC (17).
> >=20
> > needs to read something like
> >=20
> >    =E2=80=94 The field's deliver-error-code bit (bit 11) is 1 if each o=
f 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-sta=
te area;
> >      (3) IA32_VMX_BASIC[56] is read as 0 (see Appendix A.1); and (4) th=
e 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]
> >=20
> >      [1] #CP has an error code if and only if IA32_VMX_CR4_FIXED1 enume=
rates
> >          support for the 1-setting of CR4.CET.
>=20
> Hi, Sean,
>=20
> I sent above change request to Gil(added in cc), but he shared different
> opinion on this issue:

Heh, "opinion".

> =C2=A0It may make things clearer if we document the statement above (all
> CET-capable parts enumerate IA32_VMX_BASIC[56] as 1).
>=20
> I will see if we can update future revisions of the SDM to clarify this."

That would be helpful.  Though to be perfectly honest, I simply overlooked =
the
existence of IA32_VMX_BASIC[56].

Thanks!

> Then if this is the case,=C2=A0 kvm needs to check IA32_VMX_BASIC[56] bef=
ore
> inject exception to nested VM.
>=20
> And this patch could be removed, instead need another patch like below:
>=20
> 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 @@
> =C2=A0#define VMX_BASIC_MEM_TYPE_MASK=C2=A0=C2=A0=C2=A0 0x003c00000000000=
0LLU
> =C2=A0#define VMX_BASIC_MEM_TYPE_WB=C2=A0=C2=A0=C2=A0 6LLU
> =C2=A0#define VMX_BASIC_INOUT=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 0x0040=
000000000000LLU
> +#define VMX_BASIC_CHECK_ERRCODE=C2=A0=C2=A0=C2=A0 0x0140000000000000LLU

"Check Error Code" isn't a great description.  The flag enumerates that the=
re the
CPU does *not* perform consistency checks on the error code when injecting =
hardware
exceptions.

So something like this?

  VMX_BASIC_NO_HW_ERROR_CODE_CC

or maybe

  VMX_BASIC_PM_NO_HW_ERROR_CODE_CC

if we want to capture that only protected mode is exempt (I personally pref=
er
just VMX_BASIC_NO_HW_ERROR_CODE_CC as "PM" is a bit ambiguous).

> @@ -2873,7 +2873,8 @@ static int nested_check_vm_entry_controls(struct
> kvm_vcpu *vcpu,
> =C2=A0=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 should_have_error_code =3D
> =C2=A0=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 intr_type =
=3D=3D INTR_TYPE_HARD_EXCEPTION && prot_mode &&
> =C2=A0=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 x86_except=
ion_has_error_code(vector);
> -=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 if (CC(has_error_code !=3D should_=
have_error_code))
> +=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 if (!cpu_has_vmx_basic_check_errco=
de() &&
> +=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 CC(has_error_co=
de !=3D should_have_error_code))

This is wrong on mutiple fronts:

  1. The new feature flag only excempts hardware exceptions delivered to gu=
ests
     with CR0.PE=3D1.  The above will skip the consistency check for all ev=
ent injection.

  2. KVM needs to check the CPU model that is exposed to L1, not the capabi=
lities
     of the host CPU.

Highlighting the key phrases 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 vecto=
r indicates one of the following
  exceptions: #DF (vector 8), #TS (10), #NP (11), #SS (12), #GP (13), #PF (=
14), or #AC (17).
 =20
  The field's deliver-error-code bit is 0 if any of the following holds: (1=
) the interruption type is not hardware
                                             ^^^^^^
  exception; (2) bit 0 is clear in the CR0 field in the guest-state area; o=
r (3) IA32_VMX_BASIC[56] is read as
  0 and the vector is in one of the following ranges: 0=E2=80=937, 9, 15, 1=
6, or 18=E2=80=9331.

I think what we want is:

		/* VM-entry interruption-info field: deliver error code */
		if (!prot_mode || intr_type !=3D INTR_TYPE_HARD_EXCEPTION ||
		    !nested_cpu_has_no_hw_error_code_cc(vcpu)) {
			should_have_error_code =3D
				intr_type =3D=3D INTR_TYPE_HARD_EXCEPTION && prot_mode &&
				x86_exception_has_error_code(vector);
			if (CC(has_error_code !=3D should_have_error_code))
				return -EINVAL;
		}