Date:   Mon, 26 Jun 2023 14:46:20 +0800
From:   "Yang, Weijiang" <weijiang.yang@...el.com>
To:     Sean Christopherson <seanjc@...gle.com>
CC:     <pbonzini@...hat.com>, <kvm@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <peterz@...radead.org>,
        <rppt@...nel.org>, <binbin.wu@...ux.intel.com>,
        <rick.p.edgecombe@...el.com>, <john.allen@....com>
Subject: Re: [PATCH v3 00/21] Enable CET Virtualization


On 6/24/2023 4:51 AM, Sean Christopherson wrote:
> On Mon, Jun 19, 2023, Weijiang Yang wrote:
>> 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 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 supervisor
>>>>>      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® 64 and IA-32
>>>>>      Architectures Software Developer’s Manual, Volume 1).
>>>>>
>>>>> But then it says says VMMs shouldn't set the bit.
>>>>>
>>>>>      When emulating the CPUID instruction, a virtual-machine monitor should return
>>>>>      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 than 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.
>>>> I need to double check for the vague description.
>>>>
>>>>   From my understanding, on bare metal side, if the bit is 1, OS can enable
>>>> SSS if pushes won't cause page fault. But for VM case, it's not recommended
>>>> (regardless of the bit state) to set the bit as vm-exits caused by guest SSS
>>>> pushes cannot be fully excluded.
>>>>
>>>> In other word, the bit is mainly for bare metal guidance now.
>>>>
>>>>>> 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 pieces are there.
>>> ...
>>>
>>>> And also based on above SDM description, I don't want to add the support
>>>> blindly now.
>>> *sigh*
>>>
>>> I got filled in on the details offlist.
>>>
>>> 1) In the next version of this series, please rework it to reincorporate Supervisor
>>>      Shadow Stack support into the main series, i.e. pretend Intel's implemenation
>>>      isn't horribly flawed.
>> Let me make it clear, you want me to do two things:
>>
>> 1)Add Supervisor Shadow Stack  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 architecture,
> it's little more than a hint.  And even if the guest follows SDM's recommendation
> to not enable shadow stacks, a clever kernel can still utilize SSS assets, e.g. use
> the MSRs as scratch registers.

Understood, thanks!

Powered by blists - more mailing lists