| Message ID | 20230217184814.1243046-3-burzalodowa@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/hvm: {svm,vmx}.{c,h} cleanup | expand |
On 17/02/2023 6:48 pm, Xenia Ragiadakou wrote: > Remove the forward declaration of struct vcpu because it is not used. Huh, turns out that was my fault in c/s b158e72abe, shortly after I introduced them in the first place. Also, looking into that, there's one legitimate use of svm.h from outside, which is svm_load_segs*() which means we can't make all the headers be local. But still, most of svm.h shouldn't be includable in the rest of Xen. Perhaps we can make a separate dedicated header for just this. [edit] And svm_{doman,vcpu}. Perhaps we want a brand new include/asm/hvm/svm.h with only the things needed elsewhere. > Move the forward declaration of struct cpu_user_regs just above the > function that needs it (to remind that it will need to be removed > along with the function). I'm not sure. This feels like churn. > Move the definitions of NPT_PFEC_with_gla and NPT_PFEC_in_gpt in svm.c > because they are used only in this file. IMO, these would better live in vmcb.h because that's where all the other decode information lives, not that there is much. I previously started trying to convert all the exit_into fields into a typed union, but I didn't get as far the NPT info. > Move the definitions of SVM_PAUSE{FILTER,THRESH}_INIT in vmcb.c because > they are used only in this file. Honestly, at this point you might as well just delete the defines, and opencode at their single usage site. They're pure obfuscation like this, given no statement of units / behaviour, etc. That said, we do need to stea^W borrow adaptive PLE, and make the settings hvm-common because right now we've got two different ways of configuring the same thing for VT-x and SVM. (This is definitely not cleanup work. Just an observation for anyone feeling at a loose end.) ~Andrew
On 2/21/23 01:08, Andrew Cooper wrote: > On 17/02/2023 6:48 pm, Xenia Ragiadakou wrote: >> Remove the forward declaration of struct vcpu because it is not used. > > Huh, turns out that was my fault in c/s b158e72abe, shortly after I > introduced them in the first place. > > Also, looking into that, there's one legitimate use of svm.h from > outside, which is svm_load_segs*() which means we can't make all the > headers be local. > > But still, most of svm.h shouldn't be includable in the rest of Xen. > Perhaps we can make a separate dedicated header for just this. > > [edit] And svm_{doman,vcpu}. Perhaps we want a brand new > include/asm/hvm/svm.h with only the things needed elsewhere. This can be done as part of the series aiming to make svm/vmx support configurable. > >> Move the forward declaration of struct cpu_user_regs just above the >> function that needs it (to remind that it will need to be removed >> along with the function). > > I'm not sure. This feels like churn. Ok I will leave it where it is in v2. > >> Move the definitions of NPT_PFEC_with_gla and NPT_PFEC_in_gpt in svm.c >> because they are used only in this file. > > IMO, these would better live in vmcb.h because that's where all the > other decode information lives, not that there is much. I previously > started trying to convert all the exit_into fields into a typed union, > but I didn't get as far the NPT info. Ok I will fix it in v2. > >> Move the definitions of SVM_PAUSE{FILTER,THRESH}_INIT in vmcb.c because >> they are used only in this file. > > Honestly, at this point you might as well just delete the defines, and > opencode at their single usage site. They're pure obfuscation like > this, given no statement of units / behaviour, etc. Ok I will do in v2. > > That said, we do need to stea^W borrow adaptive PLE, and make the I cannot understand what do you mean by "stea^W borrow adaptive PLE". > settings hvm-common because right now we've got two different ways of > configuring the same thing for VT-x and SVM. (This is definitely not > cleanup work. Just an observation for anyone feeling at a loose end.) I will create hvm_function_table stubs for the functions that are used in common code, do the same thing but are implemented differently by svm and vtx, as part of the series aiming to make svm/vmx support configurable. > > ~Andrew
On 21/02/2023 7:58 am, Xenia Ragiadakou wrote: > > On 2/21/23 01:08, Andrew Cooper wrote: >> On 17/02/2023 6:48 pm, Xenia Ragiadakou wrote: >>> Remove the forward declaration of struct vcpu because it is not used. >> >> Huh, turns out that was my fault in c/s b158e72abe, shortly after I >> introduced them in the first place. >> >> Also, looking into that, there's one legitimate use of svm.h from >> outside, which is svm_load_segs*() which means we can't make all the >> headers be local. >> >> But still, most of svm.h shouldn't be includable in the rest of Xen. >> Perhaps we can make a separate dedicated header for just this. >> >> [edit] And svm_{doman,vcpu}. Perhaps we want a brand new >> include/asm/hvm/svm.h with only the things needed elsewhere. > > This can be done as part of the series aiming to make svm/vmx support > configurable. Ok, that's fine. Honestly, there's a lot of cleanup which can be done. We probably want to end up making a number of $foo-types.h headers so we can construct struct vcpu/domain without xen/sched.h including the majority of Xen in one fell swoop. > >> >> That said, we do need to stea^W borrow adaptive PLE, and make the > > I cannot understand what do you mean by "stea^W borrow adaptive PLE". Pause Loop Exiting is tricky to get right. The common expectation is that PLE hits in a spinlock or other mutual exclusion primitive. It is generally good to let the vCPU spin for a bit, in the expectation that the other vCPU holding lock will release it in a timely fashion. Spinning for a few iterations (even a few hundred) is far lower overhead than taking a vmexit. But if the other vCPU isn't executing, it can never release the lock, and letting the current vCPU spin does double damage because it consumes the domain's scheduler credit, which in turn pushes out the reschedule of the vCPU that's actually holding the lock. (This is why paravirt spinlocks are so useful in virt. If you get them right, you end up with only the vcpus that can usefully do work to release a lock executing.) For overall system performance, there is a tradeoff between how long you let a vCPU spin, and when it's better to force such a vCPU to yield. This point depends on the typical spinlock contention inside the guest, and the overall system busyness, both of which vary over time. Picking fixed numbers for PLE is better than not having PLE in the first place, but only just. There is an algorithm called adaptive-PLE which tries to balance the PLE settings over time to optimise overall system performance. ~Andrew
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c index f0bc72c313..620c0d2e67 100644 --- a/xen/arch/x86/hvm/svm/svm.c +++ b/xen/arch/x86/hvm/svm/svm.c @@ -1634,6 +1634,12 @@ static int cf_check svm_cpu_up(void) return _svm_cpu_up(false); } +/* EXITINFO1 fields on NPT faults */ +#define _NPT_PFEC_with_gla 32 +#define NPT_PFEC_with_gla (1UL<<_NPT_PFEC_with_gla) +#define _NPT_PFEC_in_gpt 33 +#define NPT_PFEC_in_gpt (1UL<<_NPT_PFEC_in_gpt) + static void svm_do_nested_pgfault(struct vcpu *v, struct cpu_user_regs *regs, uint64_t pfec, paddr_t gpa) { diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c index 305d4767e3..151a04b60d 100644 --- a/xen/arch/x86/hvm/svm/vmcb.c +++ b/xen/arch/x86/hvm/svm/vmcb.c @@ -30,6 +30,9 @@ #include <asm/hvm/svm/svmdebug.h> #include <asm/spec_ctrl.h> +#define SVM_PAUSEFILTER_INIT 4000 +#define SVM_PAUSETHRESH_INIT 1000 + struct vmcb_struct *alloc_vmcb(void) { struct vmcb_struct *vmcb; diff --git a/xen/arch/x86/include/asm/hvm/svm/svm.h b/xen/arch/x86/include/asm/hvm/svm/svm.h index 65e35a4f59..2cd9cea4a0 100644 --- a/xen/arch/x86/include/asm/hvm/svm/svm.h +++ b/xen/arch/x86/include/asm/hvm/svm/svm.h @@ -45,10 +45,8 @@ static inline void svm_invlpga(unsigned long linear, uint32_t asid) "a" (linear), "c" (asid)); } -struct cpu_user_regs; -struct vcpu; - unsigned long *svm_msrbit(unsigned long *msr_bitmap, uint32_t msr); +struct cpu_user_regs; void __update_guest_eip(struct cpu_user_regs *regs, unsigned int inst_len); /* @@ -96,17 +94,8 @@ extern u32 svm_feature_flags; #define cpu_has_svm_sss cpu_has_svm_feature(SVM_FEATURE_SSS) #define cpu_has_svm_spec_ctrl cpu_has_svm_feature(SVM_FEATURE_SPEC_CTRL) -#define SVM_PAUSEFILTER_INIT 4000 -#define SVM_PAUSETHRESH_INIT 1000 - /* TSC rate */ #define DEFAULT_TSC_RATIO 0x0000000100000000ULL #define TSC_RATIO_RSVD_BITS 0xffffff0000000000ULL -/* EXITINFO1 fields on NPT faults */ -#define _NPT_PFEC_with_gla 32 -#define NPT_PFEC_with_gla (1UL<<_NPT_PFEC_with_gla) -#define _NPT_PFEC_in_gpt 33 -#define NPT_PFEC_in_gpt (1UL<<_NPT_PFEC_in_gpt) - #endif /* __ASM_X86_HVM_SVM_H__ */
Remove the forward declaration of struct vcpu because it is not used. Move the forward declaration of struct cpu_user_regs just above the function that needs it (to remind that it will need to be removed along with the function). Move the definitions of NPT_PFEC_with_gla and NPT_PFEC_in_gpt in svm.c because they are used only in this file. Move the definitions of SVM_PAUSE{FILTER,THRESH}_INIT in vmcb.c because they are used only in this file. No functional change intended. Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com> --- xen/arch/x86/hvm/svm/svm.c | 6 ++++++ xen/arch/x86/hvm/svm/vmcb.c | 3 +++ xen/arch/x86/include/asm/hvm/svm/svm.h | 13 +------------ 3 files changed, 10 insertions(+), 12 deletions(-)