Re: [RFC PATCH v2 4/5] KVM: x86: Remove KVM_X86_OP_OPTIONAL

[Sean Christopherson <seanjc@google.com>]

On Fri, Apr 19, 2024, Wei W Wang wrote:
> On Friday, April 19, 2024 9:42 PM, Sean Christopherson wrote:
> > On Fri, Apr 19, 2024, Wei Wang wrote:
> > > KVM_X86_OP and KVM_X86_OP_OPTIONAL were utilized to define and
> > execute
> > > static_call_update() calls on mandatory and optional hooks, respectively.
> > > Mandatory hooks were invoked via static_call() and necessitated
> > > definition due to the presumption that an undefined hook (i.e., NULL)
> > > would cause
> > > static_call() to fail. This assumption no longer holds true as
> > > static_call() has been updated to treat a "NULL" hook as a NOP on x86.
> > > Consequently, the so-called mandatory hooks are no longer required to
> > > be defined, rendering them non-mandatory.
> > 
> > This is wrong.  They absolutely are mandatory.  The fact that static_call()
> > doesn't blow up doesn't make them optional.  If a vendor neglects to
> > implement a mandatory hook, KVM *will* break, just not immediately on the
> > static_call().
> > 
> > The static_call() behavior is actually unfortunate, as KVM at least would prefer
> > that it does explode on a NULL point.  I.e. better to crash the kernel (hopefully
> > before getting to production) then to have a lurking bug just waiting to cause
> > problems.
> > 
> > > This eliminates the need to differentiate between mandatory and
> > > optional hooks, allowing a single KVM_X86_OP to suffice.
> > >
> > > So KVM_X86_OP_OPTIONAL and the WARN_ON() associated with
> > KVM_X86_OP
> > > are removed to simplify usage,
> > 
> > Just in case it isn't clear, I am very strongly opposed to removing
> > KVM_X86_OP_OPTIONAL() and the WARN_ON() protection to ensure
> > mandatory ops are implemented.
> 
> OK, we can drop patch 4 and 5.
> 
> Btw, may I know what is the boundary between mandatory and optional hooks?
> For example, when adding a new hook, what criteria should we use to determine
> whether it's mandatory, thereby requiring both SVM and VMX to implement it (and
> seems need to be merged them together?)
> (I searched a bit, but didn't find it)

It's a fairly simple rule: is the hook required for functional correctness, at
all times?

E.g. post_set_cr3() is unique to SEV-ES+ guests, and so it's optional for both
VMX and SVM (because SEV-ES might not be enabled).

All of the APICv related hooks are optional, because APICv support isn't guaranteed.

set_tss_addr() and set_identity_map_addr() are unique to old Intel hardware.

The mem_enc ops are unique to SEV+ (and at some point TDX), which again isn't
guaranteed to be supported and enabled.

For something like vcpu_precreate(), it's an arbitrary judgment call: is it
cleaner to make the hook optional, or to have SVM implement a nop?  Thankfully,
there are very few of these.

Heh, vm_destroy() should be non-optional, we should clean that up.