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.
