Re: [PATCH] x86/svm: require VMSAVEvirt for nested virt

[Stephen Cheng <stephen.cheng@xxxxxxxxxx>]

On 27/07/2026 10:50, Ross Lagerwall wrote:
> > @@ -837,7 +837,8 @@ static void __init calculate_hvm_max_policy(void)
> > -                               (1u << SVM_FEATURE_DECODEASSISTS));
> > +                               (1u << SVM_FEATURE_DECODEASSISTS) |
> > +                               (1u << SVM_FEATURE_VLOADSAVE));
>
> Exposing this feature to L1 is separate from requiring it in L0 and IMO should
> be a separate commit. If it's exposed to L1, there is a lot more to think
> about. E.g. whether L2 should follow L1's setting or L0's for vloadsave_enable
> and if it is not enabled what happens if L1 doesn't intercept VMLOAD/VMSAVE.

Agreed.  I'll drop the cpu-policy.c hunk from this patch and do the L1
side as a follow-up.

Worth noting that dropping it only removes the CPUID advertisement, not
the functionality: vmcb02 picks up vloadsave_enable from the virt_ext
merge in nsvm_vmcb_prepare4vmrun() regardless of what we advertise,
which is your third point below.

> Also note we discovered previously that contrary to what the APM documents:
>
> If the VMSAVE/VMLOAD intercept bits are clear, and virtual
> VMLOAD/VMSAVE is not enabled, then a VMSAVE instruction executed in
> guest mode executes as if it were in non-root mode, and operates on host
> physical addresses.

Thanks, I hadn't come across that.  It makes "vloadsave_enable clear
implies the intercepts are set" a hard requirement rather than a
tidiness thing, so I'll treat it as an invariant in the follow-up.

> I think there is also a separate bug that what Xen advertises doesn't
> necessarily control what the guest can use. E.g. previously we didn't 
> advertise
> SVM_FEATURE_VLOADSAVE but Xen would always merge in tthe virt_ext settings 
> from
> VMCB(L1-L2).

Agreed, and it's the same virt_ext merge.  I'll fix it as part of the
follow-up.

> > +        /*
> > +         * VMSAVEvirt is a hard requirement for nested virt (see
> > +         * start_nested_svm()), so it is guaranteed present here.
> > +         */
> > +        ASSERT(cpu_has_svm_vloadsave);
>
> Maybe just personal taste but I think this assert is unnecessary.
> cpu_has_svm_vloadsave is constant since boot and also checked at boot so
> further checks are unnecessary.

I'll drop it.

> >      case VMEXIT_VMLOAD:
> > -        svm_vmexit_do_vmload(vmcb, regs, v, regs->rax);
> > -        break;
> >      case VMEXIT_VMSAVE:
> > -        svm_vmexit_do_vmsave(vmcb, regs, v, regs->rax);
> > +        svm_vmexit_do_vmload_vmsave(v);
> >          break;
>
> I think it would be simpler to just move these case statements up to the
> VMEXIT_RDPRU section.

Will do.

Thanks for the review.

Stephen