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, 23 Jun 2023 13:51:58 -0700
In-Reply-To: <c438b5b1-b34d-3e77-d374-37053f4c14fa@intel.com>
Mime-Version: 1.0
References: <20230511040857.6094-1-weijiang.yang@intel.com>
 <ZIufL7p/ZvxjXwK5@google.com> <147246fc-79a2-3bb5-f51f-93dfc1cffcc0@intel.com>
 <ZIyiWr4sR+MqwmAo@google.com> <c438b5b1-b34d-3e77-d374-37053f4c14fa@intel.com>
Message-ID: <ZJYF7haMNRCbtLIh@google.com>
Subject: Re: [PATCH v3 00/21] Enable CET Virtualization
From:   Sean Christopherson <seanjc@google.com>
To:     Weijiang Yang <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 Mon, Jun 19, 2023, Weijiang Yang wrote:
>=20
> On 6/17/2023 1:56 AM, Sean Christopherson wrote:
> > On Fri, Jun 16, 2023, Weijiang Yang wrote:
> > > On 6/16/2023 7:30 AM, Sean Christopherson wrote:
> > > > 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 p=
atch is to
> > > > > facilitate AMD folks to enable the feature.
> > > > I am beyond confused by the SDM's wording of CET_SSS.
> > > >=20
> > > > First, it says that CET_SSS says the CPU isn't buggy (or maybe "les=
s buggy" is
> > > > more appropriate phrasing).
> > > >=20
> > > >     Bit 18: CET_SSS. If 1, indicates that an operating system can e=
nable supervisor
> > > >     shadow stacks as long as it ensures that certain supervisor sha=
dow-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).
> > > >=20
> > > > But then it says says VMMs shouldn't set the bit.
> > > >=20
> > > >     When emulating the CPUID instruction, a virtual-machine monitor=
 should return
> > > >     this bit as 0 if those pushes can cause VM exits.
> > > >=20
> > > > Based on the Xen code (which is sadly a far better source of inform=
ation than the
> > > > SDM), I *think* that what the SDM is trying to say is that VMMs sho=
uld not set
> > > > CET_SS if VM-Exits can occur ***and*** the bit is not set in the ho=
st CPU.  Because
> > > > if the SDM really means "VMMs should never set the bit", then what =
on earth is the
> > > > point of the bit.
> > > I need to double check for the vague description.
> > >=20
> > >  From my understanding, on bare metal side, if the bit is 1, OS can e=
nable
> > > SSS if pushes won't cause page fault. But for VM case, it's not recom=
mended
> > > (regardless of the bit state) to set the bit as vm-exits caused by gu=
est SSS
> > > pushes cannot be fully excluded.
> > >=20
> > > In other word, the bit is mainly for bare metal guidance now.
> > >=20
> > > > > In summary, this new series enables CET user SHSTK/IBT and kernel=
 IBT, but
> > > > > 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 piec=
es are there.
> > ...
> >=20
> > > And also based on above SDM description, I don't want to add the supp=
ort
> > > blindly now.
> > *sigh*
> >=20
> > I got filled in on the details offlist.
> >=20
> > 1) In the next version of this series, please rework it to reincorporat=
e Supervisor
> >     Shadow Stack support into the main series, i.e. pretend Intel's imp=
lemenation
> >     isn't horribly flawed.
>=20
> Let me make it clear, you want me to do two things:
>=20
> 1)Add Supervisor Shadow Stack=C2=A0 state support(i.e., XSS.bit12(CET_S))=
 into
> kernel so that host can support guest Supervisor Shadow Stack MSRs in g/h=
 FPU
> context switch.

If that's necessary for correct functionality, yes.

> 2) Add Supervisor Shadow stack support into KVM part so that guest OS is
> able to use SSS with risk.

Yes.  Architecturally, if KVM advertises X86_FEATURE_SHSTK, then KVM needs =
to
provide both User and Supervisor support.  CET_SSS doesn't change the archi=
tecture,
it's little more than a hint.  And even if the guest follows SDM's recommen=
dation
to not enable shadow stacks, a clever kernel can still utilize SSS assets, =
e.g. use
the MSRs as scratch registers.