Re: [PATCH v10 24/27] KVM: x86: Enable CET virtualization for VMX and advertise to userspace

[Yang, Weijiang]

On 5/22/2024 4:41 PM, Yang, Weijiang wrote:
> On 5/21/2024 1:09 AM, Sean Christopherson wrote:
> > On Mon, May 20, 2024, Weijiang Yang wrote:
> > > On 5/17/2024 10:26 PM, Sean Christopherson wrote:
> > > > On Fri, May 17, 2024, Thomas Gleixner wrote:
> > > > > On Thu, May 16 2024 at 07:39, Sean Christopherson wrote:
> > > > > > On Thu, May 16, 2024, Weijiang Yang wrote:
> > > > > > > We synced the issue internally, and got conclusion that KVM should honor host
> > > > > > > IBT config.  In this case IBT bit in boot_cpu_data should be honored.  With
> > > > > > > this policy, it can avoid CPUID confusion to guest side due to host ibt=off
> > > > > > > config.
> > > > > > What was the reasoning?  CPUID confusion is a weak justification, e.g. it's not
> > > > > > like the guest has visibility into the host kernel, and raw CPUID will still show
> > > > > > IBT support in the host.
> > > > > >
> > > > > > On the other hand, I can definitely see folks wanting to expose IBT to guests
> > > > > > when running non-complaint host kernels, especially when live migration is in
> > > > > > play, i.e. when hiding IBT from the guest will actively cause problems.
> > > > > I have to disagree here violently.
> > > > >
> > > > > If the exposure of a CPUID bit to a guest requires host side support,
> > > > > e.g. in xstate handling, then exposing it to a guest is simply not
> > > > > possible.
> > > > Ya, I don't disagree, I just didn't realize that CET_USER would be cleared in the
> > > > supported xfeatures mask.
> > > For host side support, fortunately,  this patch already has some checks for
> > > that. But for userspace CPUID config, it allows IBT to be exposed alone.
> > >
> > > IIUC, this series tries to tie IBT to SHSTK feature, i.e., IBT cannot be
> > > exposed as an independent feature to guest without exposing SHSTK at the same
> > > time. If it is, then below patch is not needed anymore:
> > > https://lore.kernel.org/all/20240219074733.122080-3-weijiang.yang@xxxxxxxxx/
> > That's a question for the x86 maintainers.  Specifically, do they want to allow
> > enabling XFEATURE_CET_USER even if userspace shadow stack support is disabled.
> >
> > I don't think it impacts KVM, at least not directly.  Regardless of what decision
> > the kernel makes, KVM needs to disable IBT and SHSTK if CET_USER _or_ CET_KERNEL
> > is missing, which KVM already does via:
> >
> >     if ((kvm_caps.supported_xss & (XFEATURE_MASK_CET_USER |
> >          XFEATURE_MASK_CET_KERNEL)) !=
> >         (XFEATURE_MASK_CET_USER | XFEATURE_MASK_CET_KERNEL)) {
> >         kvm_cpu_cap_clear(X86_FEATURE_SHSTK);
> >         kvm_cpu_cap_clear(X86_FEATURE_IBT);
> >         kvm_caps.supported_xss &= ~(XFEATURE_MASK_CET_USER |
> >                         XFEATURE_MASK_CET_KERNEL);
> >     }
> >
> > > I'd check and clear IBT bit from CPUID when userspace enables only IBT via
> > > KVM_SET_CPUID2.
> > No.  It is userspace's responsibility to provide a sane CPUID model for the guest.
> > KVM needs to ensure that *KVM* doesn't treat IBT as supported if the kernel doesn't
> > allow XFEATURE_CET_USER, but userspace can advertise whatever it wants to the guest
> > (and gets to keep the pieces if it does something funky).
>
> OK, I think we can go ahead to keep KVM patches as-is given the fact user IBT is not enabled in Linux.
> I only hope other OSes can enforce both SHSTK and IBT dependency on XFEATURE_CET_USER so
> that user IBT can work well there.
>
> Then IBT can be exposed to guest alone because guest *kernel* IBT only relies on S_CET MSR  which is
> VMCS auto-saved/restored.
>
> What's your thoughts?

If there's objection I'll do below changes for this series:
1) Remove patch :
https://lore.kernel.org/all/20240219074733.122080-3-weijiang.yang@xxxxxxxxx/
2) Remove reference to raw CPUID for KVM IBT CPUID, handling it the same as SHSTK.

Meanwhile still allow userspace to enable IBT feature alone because user IBT is not enabled
in kernel now, leave enforcement of user IBT dependency on XFEATURE_CET_USER in future.

>