On Mon, Jul 24, 2023, Dmitry Torokhov wrote:
> Hi Sean,
>
> On Fri, Jul 21, 2023 at 01:18:54PM -0700, Sean Christopherson wrote:
> > +static bool kvm_is_svm_supported(void)
> > +{
> > + bool supported;
> > +
> > + migrate_disable();
> > + supported = __kvm_is_svm_supported();
> > + migrate_enable();
>
> I am typically very wary of the constructs like this, as the value
> returned is obsolete the moment migrate_enable() happens.
Yeah, I don't like this code, but there's no great solution in this case. Or at
least, none that I've found. At some point KVM has to enable migration/preemption
before "is KVM supported?" is ultimately consumed, as the real consumer is
userspace.
> Is value of "svm was supported at some time in the past but may or may not be
> supported right now" useful and if it is then could you add comment why?
No, because barring fatal silicon/ucode/kernel bugs, SVM support isn't expected
to disappear or (re)appear).
KVM defends against the "disappear" case as much as can be reasonably expected.
It's ugly, but functionally ok (not perfect, but ok). KVM doesn't actually care
which CPU does the initial support check, because KVM will do fully protected support
checks on all CPUs before actually letting userspace create VMs. This is why the
changelog states that ensuring a stable CPU is a non-goal, and also why the inner
helpers don't use the raw accessors.
The "(re)appear" case doesn't need to be handled, because userspace could simply
retry if it really wanted to (but that would be quite insane/nonsensical, and
just asking for problems).
I didn't add a comment because VMX uses the exact same pattern, and I didn't
want to copy+paste a non-trivial comment. And this is a single use local helper,
so I'm not terribly concerned about it being misused.
That said, I'll see if I can find a common, intuitive location to document this.
