| Message ID | 20201220211109.129946-1-ubizjak@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] KVM/x86: Move definition of __ex to x86.h | expand |
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. > and move the definition of __ex to a common include to be > shared between VMX and SVM.
On Mon, Dec 21, 2020 at 7:19 PM Sean Christopherson <seanjc@google.com> 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(). 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. 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. Uros.
On Mon, Dec 21, 2020 at 7:57 PM Uros Bizjak <ubizjak@gmail.com> wrote: > > On Mon, Dec 21, 2020 at 7:19 PM Sean Christopherson <seanjc@google.com> 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. IMO, this comment could read: /* Just like cpu_vmxoff(), but with the fault on reboot handling. */ static void kvm_cpu_vmxoff(void) Uros.
On Mon, Dec 21, 2020, Uros Bizjak wrote: > On Mon, Dec 21, 2020 at 7:19 PM Sean Christopherson <seanjc@google.com> 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. */
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index 39707e72b062..a78e4b1a5d77 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -1634,31 +1634,6 @@ enum { #define kvm_arch_vcpu_memslots_id(vcpu) ((vcpu)->arch.hflags & HF_SMM_MASK ? 1 : 0) #define kvm_memslots_for_spte_role(kvm, role) __kvm_memslots(kvm, (role).smm) -asmlinkage void kvm_spurious_fault(void); - -/* - * 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. - */ -#define __kvm_handle_fault_on_reboot(insn) \ - "666: \n\t" \ - insn "\n\t" \ - "jmp 668f \n\t" \ - "667: \n\t" \ - "1: \n\t" \ - ".pushsection .discard.instr_begin \n\t" \ - ".long 1b - . \n\t" \ - ".popsection \n\t" \ - "call kvm_spurious_fault \n\t" \ - "1: \n\t" \ - ".pushsection .discard.instr_end \n\t" \ - ".long 1b - . \n\t" \ - ".popsection \n\t" \ - "668: \n\t" \ - _ASM_EXTABLE(666b, 667b) - #define KVM_ARCH_WANT_MMU_NOTIFIER int kvm_unmap_hva_range(struct kvm *kvm, unsigned long start, unsigned long end, unsigned flags); diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c index e57847ff8bd2..ba492b6d37a0 100644 --- a/arch/x86/kvm/svm/sev.c +++ b/arch/x86/kvm/svm/sev.c @@ -25,8 +25,6 @@ #include "cpuid.h" #include "trace.h" -#define __ex(x) __kvm_handle_fault_on_reboot(x) - static u8 sev_enc_bit; static int sev_flush_asids(void); static DECLARE_RWSEM(sev_deactivate_lock); diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c index 941e5251e13f..733d9f98a121 100644 --- a/arch/x86/kvm/svm/svm.c +++ b/arch/x86/kvm/svm/svm.c @@ -42,8 +42,6 @@ #include "svm.h" -#define __ex(x) __kvm_handle_fault_on_reboot(x) - MODULE_AUTHOR("Qumranet"); MODULE_LICENSE("GPL"); diff --git a/arch/x86/kvm/vmx/vmx_ops.h b/arch/x86/kvm/vmx/vmx_ops.h index 692b0c31c9c8..7e3cb53c413f 100644 --- a/arch/x86/kvm/vmx/vmx_ops.h +++ b/arch/x86/kvm/vmx/vmx_ops.h @@ -4,13 +4,11 @@ #include <linux/nospec.h> -#include <asm/kvm_host.h> #include <asm/vmx.h> #include "evmcs.h" #include "vmcs.h" - -#define __ex(x) __kvm_handle_fault_on_reboot(x) +#include "x86.h" asmlinkage void vmread_error(unsigned long field, bool fault); __attribute__((regparm(0))) void vmread_error_trampoline(unsigned long field, diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h index c5ee0f5ce0f1..3cb12788ddc5 100644 --- a/arch/x86/kvm/x86.h +++ b/arch/x86/kvm/x86.h @@ -8,6 +8,29 @@ #include "kvm_cache_regs.h" #include "kvm_emulate.h" +asmlinkage void kvm_spurious_fault(void); + +/* + * 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. + */ +#define __ex(insn) \ + "666: " insn "\n" \ + " jmp 669f\n" \ + "667:\n" \ + " .pushsection .discard.instr_begin\n" \ + " .long 667b - .\n" \ + " .popsection\n" \ + " call kvm_spurious_fault\n" \ + "668:\n" \ + " .pushsection .discard.instr_end\n" \ + " .long 668b - .\n" \ + " .popsection\n" \ + "669:\n" \ + _ASM_EXTABLE(666b, 667b) + #define KVM_DEFAULT_PLE_GAP 128 #define KVM_VMX_DEFAULT_PLE_WINDOW 4096 #define KVM_DEFAULT_PLE_WINDOW_GROW 2