Re: [PATCH v2 2/6] KVM: x86: Reject clearing EFER.SVME via KVM_SYNC_X86_SREGS if L2 is active

[Yosry Ahmed]

On Tue, Oct 27, 2026 at 3:49 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> Disallow clearing EFER.SVME via KVM_SYNC_X86_SREGS if L2 is active so that
> svm_set_efer() can treat yanking the vCPU out of L2 during KVM_RUN as a KVM
> bug.  Ideally, kvm_is_valid_sregs() itself would reject attempts to clear
> SVME while L2 is active, but the risk of breaking userspace, e.g. across
> save/restore, is too high.

I am confused. This says we shouldn't disallow clearing SVME while L2
is active because we may break userspace..

> On the other hand, yanking the vCPU out of L2
> during save/restore is already "broken" as doing so would queue a triple
> fault for L1.

..and this says it's already broken.

I suspect you in the first paragraph were referring to userspace
restoring a vCPU not in guest mode on top of a vCPU in guest mode or
similar? Might be worth clarifying what exactly we don't wanna break.

>
> Link: https://lore.kernel.org/all/sashiko-outbox-182127@xxxxxxxxxx
> Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> ---
>  arch/x86/kvm/regs.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/arch/x86/kvm/regs.c b/arch/x86/kvm/regs.c
> index 3621f1a47d37..e53496fdeb82 100644
> --- a/arch/x86/kvm/regs.c
> +++ b/arch/x86/kvm/regs.c
> @@ -738,6 +738,10 @@ int kvm_run_sync_regs_from_user(struct kvm_vcpu *vcpu)
>         if (vcpu->run->kvm_dirty_regs & KVM_SYNC_X86_SREGS) {
>                 struct kvm_sregs sregs = vcpu->run->s.regs.sregs;
>
> +               if (is_guest_mode(vcpu) &&
> +                   (vcpu->arch.efer & EFER_SVME) && !(sregs.efer & EFER_SVME))
> +                       return -EINVAL;

Do we need to check (vcpu->arch.efer & EFER_SVME)?

If that's not set, we already lost, right?

IIUC, we can't be in the middle of save/restore here. Is this intended
to check for the situation where userspace cleared SVME while L2 is
running? Or maybe L1 cleared EFER_SVME from under L2 but the shutdown
didn't happen yet?

I suppose it makes sense for the sync to only fail if it's actually
changing something. Mostly just thinking out loud.

> +
>                 if (__set_sregs(vcpu, &sregs))
>                         return -EINVAL;
>
> --
> 2.56.0.307.g5fe7bd680c-goog
>
>