On Mon, Dec 21, 2020, Uros Bizjak wrote: > On Mon, Dec 21, 2020 at 7:19 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote: > > > > On Sun, Dec 20, 2020, Uros Bizjak wrote: > > > Merge __kvm_handle_fault_on_reboot with its sole user > > > > There's also a comment in vmx.c above kvm_cpu_vmxoff() that should be updated. > > Alternatively, and probably preferably for me, what about keeping the long > > __kvm_handle_fault_on_reboot() name for the macro itself and simply moving the > > __ex() macro? > > > > That would also allow keeping kvm_spurious_fault() and > > __kvm_handle_fault_on_reboot() where they are (for no reason other than to avoid > > code churn). Though I'm also ok if folks would prefer to move everything to > > x86.h. > > The new patch is vaguely based on our correspondence on the prototype patch: > > --q-- > Moving this to asm/kvm_host.h is a bit sketchy as __ex() isn't exactly the > most unique name. arch/x86/kvm/x86.h would probably be a better > destination as it's "private". __ex() is only used in vmx.c, nested.c and > svm.c, all of which already include x86.h. > --/q-- > > where you mentioned that x86.h would be a better destination for > __ex(). Ya, thankfully I still agree with my past self on this one :-) > IMO, __kvm_handle_fault_on_reboot also belongs in x86.h, as it > deals with a low-level access to the processor, and there is really no > reason for this #define to be available for the whole x86 architecture > directory. I remember looking for the __kvm_handle_falult_on_reboot, > and was surprised to find it in a global x86 include directory. Works for me. If you have a strong preference for moving everything to x86.h, then let's do that. > I tried to keep __ex as a redefine to __kvm_hanlde_fault_on_reboot in > x86.h, but it just looked weird, since __ex is the only user and the > introductory document explains in detail, what > __kvm_hanlde_fault_on_reboot (aka __ex) does. I like the verbose name because it very quickly reminds what the macro does; I somehow manage to forget every few months. I agree it's a bit superfluous since the comment explains exactly what goes on. And I can see how __kvm_handle_fault_on_reboot() would be misleading as it also "handles" faults at all other times as well. What if we add a one-line synopsis in the comment to state the (very) high-level purpose of the function? We could also opportunistically clean up the formatting in the existing comment to save a line, e.g.: /* * Handle a fault on a hardware virtualization (VMX or SVM) instruction. * * Hardware virtualization extension instructions may fault if a reboot turns * off virtualization while processes are running. Usually after catching the * fault we just panic; during reboot instead the instruction is ignored. */
