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:   Thu, 15 Jun 2023 16:30:55 -0700
In-Reply-To: <20230511040857.6094-1-weijiang.yang@intel.com>
Mime-Version: 1.0
References: <20230511040857.6094-1-weijiang.yang@intel.com>
Message-ID: <ZIufL7p/ZvxjXwK5@google.com>
Subject: Re: [PATCH v3 00/21] Enable CET Virtualization
From:   Sean Christopherson <seanjc@google.com>
To:     Yang Weijiang <weijiang.yang@intel.com>
Cc:     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
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, May 11, 2023, Yang Weijiang wrote:
> The last patch is introduced to support supervisor SHSTK but the feature =
is
> not enabled on Intel platform for now, the main purpose of this patch is =
to
> facilitate AMD folks to enable the feature.

I am beyond confused by the SDM's wording of CET_SSS.

First, it says that CET_SSS says the CPU isn't buggy (or maybe "less buggy"=
 is
more appropriate phrasing).

  Bit 18: CET_SSS. If 1, indicates that an operating system can enable supe=
rvisor
  shadow stacks as long as it ensures that certain supervisor shadow-stack =
pushes
  will not cause page faults (see Section 17.2.3 of the Intel=C2=AE 64 and =
IA-32
  Architectures Software Developer=E2=80=99s Manual, Volume 1).

But then it says says VMMs shouldn't set the bit.

  When emulating the CPUID instruction, a virtual-machine monitor should re=
turn
  this bit as 0 if those pushes can cause VM exits.

Based on the Xen code (which is sadly a far better source of information th=
an the
SDM), I *think* that what the SDM is trying to say is that VMMs should not =
set
CET_SS if VM-Exits can occur ***and*** the bit is not set in the host CPU. =
 Because
if the SDM really means "VMMs should never set the bit", then what on earth=
 is the
point of the bit.

> In summary, this new series enables CET user SHSTK/IBT and kernel IBT, bu=
t
> doesn't fully support CET supervisor SHSTK, the enabling work is left for
> the future.

Why?  If my interpretation of the SDM is correct, then all the pieces are t=
here.

> Executed all KVM-unit-test cases and KVM selftests against this series, a=
ll
> test cases passed except the vmx test, the failure is due to CR4_CET bit
> testing in test_vmxon_bad_cr(). After add CR4_CET bit to skip list, the t=
est
> passed. I'll send a patch to fix this issue later.

Your cover letter from v2 back in April said the same thing.  Why hasn't th=
e patch
been posted?  And what exactly is the issue?  IIUC, setting CR4.CET with
MSR_IA32_S_CET=3D0 and MSR_IA32_U_CET=3D0 should be a nop, which suggests t=
hat there's
a KVM bug.  And if that's the case, the next obvious questions is, why are =
you
posting known buggy code?