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

[Wang, Wei W]

On Friday, April 19, 2024 11:58 PM, Sean Christopherson wrote:
> 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.

I think determining whether a hook is optional is easy, but classifying a hook as
mandatory might be challenging due to the multiple options available to achieve
functional correctness.

Take the vm_destroy() example as you mentioned, it could be debatable to say
it's mandatory, e.g. the VMX code could be adjusted by incorporating vmx_vm_destroy()
into the vcpu_free() hook, and being invoked upon the first vcpu to be freed.
It could be even harder at the time when the first user (e.g. SVM) adds the hook
and classifies that vm_destroy() is mandatory.
(not try to argue for anything, just want to gain a comprehensive understanding of the rules)