| Message ID | 20230914063325.85503-11-weijiang.yang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enable CET Virtualization | expand |
On Thu, 2023-09-14 at 02:33 -0400, Yang Weijiang wrote: > Wrap __kvm_{get,set}_msr() into two new helpers for KVM usage and use the > helpers to replace existing usage of the raw functions. > kvm_msr_{read,write}() are KVM-internal helpers, i.e. used when KVM needs > to get/set a MSR value for emulating CPU behavior. I am not sure if I like this patch or not. On one hand the code is cleaner this way, but on the other hand now it is easier to call kvm_msr_write() on behalf of the guest. For example we also have the 'kvm_set_msr()' which does actually set the msr on behalf of the guest. How about we call the new function kvm_msr_set_host() and rename kvm_set_msr() to kvm_msr_set_guest(), together with good comments explaning what they do? Also functions like kvm_set_msr_ignored_check(), kvm_set_msr_with_filter() and such, IMHO have names that are not very user friendly. A refactoring is very welcome in this area. At the very least they should gain thoughtful comments about what they do. For reading msrs API, I can suggest similar names and comments: /* * Read a value of a MSR. * Some MSRs exist in the KVM model even when the guest can't read them. */ int kvm_get_msr_value(struct kvm_vcpu *vcpu, u32 index, u64 *data); /* Read a value of a MSR on the behalf of the guest */ int kvm_get_guest_msr_value(struct kvm_vcpu *vcpu, u32 index, u64 *data); Although I am not going to argue over this, there are multiple ways to improve this, and also keeping things as is, or something similar to this patch is also fine with me. Best regards, Maxim Levitsky > > Suggested-by: Sean Christopherson <seanjc@google.com> > Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> > --- > arch/x86/include/asm/kvm_host.h | 4 +++- > arch/x86/kvm/cpuid.c | 2 +- > arch/x86/kvm/x86.c | 16 +++++++++++++--- > 3 files changed, 17 insertions(+), 5 deletions(-) > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index 1a4def36d5bb..0fc5e6312e93 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -1956,7 +1956,9 @@ void kvm_prepare_emulation_failure_exit(struct kvm_vcpu *vcpu); > > void kvm_enable_efer_bits(u64); > bool kvm_valid_efer(struct kvm_vcpu *vcpu, u64 efer); > -int __kvm_get_msr(struct kvm_vcpu *vcpu, u32 index, u64 *data, bool host_initiated); > + > +int kvm_msr_read(struct kvm_vcpu *vcpu, u32 index, u64 *data); > +int kvm_msr_write(struct kvm_vcpu *vcpu, u32 index, u64 data); > int kvm_get_msr(struct kvm_vcpu *vcpu, u32 index, u64 *data); > int kvm_set_msr(struct kvm_vcpu *vcpu, u32 index, u64 data); > int kvm_emulate_rdmsr(struct kvm_vcpu *vcpu); > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > index 7c3e4a550ca7..1f206caec559 100644 > --- a/arch/x86/kvm/cpuid.c > +++ b/arch/x86/kvm/cpuid.c > @@ -1531,7 +1531,7 @@ bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx, > *edx = entry->edx; > if (function == 7 && index == 0) { > u64 data; > - if (!__kvm_get_msr(vcpu, MSR_IA32_TSX_CTRL, &data, true) && > + if (!kvm_msr_read(vcpu, MSR_IA32_TSX_CTRL, &data) && > (data & TSX_CTRL_CPUID_CLEAR)) > *ebx &= ~(F(RTM) | F(HLE)); > } else if (function == 0x80000007) { > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 6c9c81e82e65..e0b55c043dab 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -1917,8 +1917,8 @@ static int kvm_set_msr_ignored_check(struct kvm_vcpu *vcpu, > * Returns 0 on success, non-0 otherwise. > * Assumes vcpu_load() was already called. > */ > -int __kvm_get_msr(struct kvm_vcpu *vcpu, u32 index, u64 *data, > - bool host_initiated) > +static int __kvm_get_msr(struct kvm_vcpu *vcpu, u32 index, u64 *data, > + bool host_initiated) > { > struct msr_data msr; > int ret; > @@ -1944,6 +1944,16 @@ int __kvm_get_msr(struct kvm_vcpu *vcpu, u32 index, u64 *data, > return ret; > } > > +int kvm_msr_write(struct kvm_vcpu *vcpu, u32 index, u64 data) > +{ > + return __kvm_set_msr(vcpu, index, data, true); > +} > + > +int kvm_msr_read(struct kvm_vcpu *vcpu, u32 index, u64 *data) > +{ > + return __kvm_get_msr(vcpu, index, data, true); > +} > + > static int kvm_get_msr_ignored_check(struct kvm_vcpu *vcpu, > u32 index, u64 *data, bool host_initiated) > { > @@ -12082,7 +12092,7 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event) > MSR_IA32_MISC_ENABLE_BTS_UNAVAIL; > > __kvm_set_xcr(vcpu, 0, XFEATURE_MASK_FP); > - __kvm_set_msr(vcpu, MSR_IA32_XSS, 0, true); > + kvm_msr_write(vcpu, MSR_IA32_XSS, 0); > } > > /* All GPRs except RDX (handled below) are zeroed on RESET/INIT. */
On Tue, Oct 31, 2023, Maxim Levitsky wrote: > On Thu, 2023-09-14 at 02:33 -0400, Yang Weijiang wrote: > > Wrap __kvm_{get,set}_msr() into two new helpers for KVM usage and use the > > helpers to replace existing usage of the raw functions. > > kvm_msr_{read,write}() are KVM-internal helpers, i.e. used when KVM needs > > to get/set a MSR value for emulating CPU behavior. > > I am not sure if I like this patch or not. On one hand the code is cleaner > this way, but on the other hand now it is easier to call kvm_msr_write() on > behalf of the guest. > > For example we also have the 'kvm_set_msr()' which does actually set the msr > on behalf of the guest. > > How about we call the new function kvm_msr_set_host() and rename > kvm_set_msr() to kvm_msr_set_guest(), together with good comments explaning > what they do? LOL, just call me Nostradamus[*] ;-) : > SSP save/load should go to enter_smm_save_state_64() and rsm_load_state_64(), : > where other fields of SMRAM are handled. : : +1. The right way to get/set MSRs like this is to use __kvm_get_msr() and pass : %true for @host_initiated. Though I would add a prep patch to provide wrappers : for __kvm_get_msr() and __kvm_set_msr(). Naming will be hard, but I think we ^^^^^^^^^^^^^^^^^^^ : can use kvm_{read,write}_msr() to go along with the KVM-initiated register : accessors/mutators, e.g. kvm_register_read(), kvm_pdptr_write(), etc. [*] https://lore.kernel.org/all/ZM0YZgFsYWuBFOze@google.com > Also functions like kvm_set_msr_ignored_check(), kvm_set_msr_with_filter() and such, > IMHO have names that are not very user friendly. I don't like the host/guest split because KVM always operates on guest values, e.g. kvm_msr_set_host() in particular could get confusing. IMO kvm_get_msr() and kvm_set_msr(), and to some extent the helpers you note below, are the real problem. What if we rename kvm_{g,s}et_msr() to kvm_emulate_msr_{read,write}() to make it more obvious that those are the "guest" helpers? And do that as a prep patch in this series (there aren't _that_ many users). I'm also in favor of renaming the "inner" helpers, but I think we should tackle those separately.separately
On Wed, 2023-11-01 at 12:32 -0700, Sean Christopherson wrote: > On Tue, Oct 31, 2023, Maxim Levitsky wrote: > > On Thu, 2023-09-14 at 02:33 -0400, Yang Weijiang wrote: > > > Wrap __kvm_{get,set}_msr() into two new helpers for KVM usage and use the > > > helpers to replace existing usage of the raw functions. > > > kvm_msr_{read,write}() are KVM-internal helpers, i.e. used when KVM needs > > > to get/set a MSR value for emulating CPU behavior. > > > > I am not sure if I like this patch or not. On one hand the code is cleaner > > this way, but on the other hand now it is easier to call kvm_msr_write() on > > behalf of the guest. > > > > For example we also have the 'kvm_set_msr()' which does actually set the msr > > on behalf of the guest. > > > > How about we call the new function kvm_msr_set_host() and rename > > kvm_set_msr() to kvm_msr_set_guest(), together with good comments explaning > > what they do? > > LOL, just call me Nostradamus[*] ;-) > > : > SSP save/load should go to enter_smm_save_state_64() and rsm_load_state_64(), > : > where other fields of SMRAM are handled. > : > : +1. The right way to get/set MSRs like this is to use __kvm_get_msr() and pass > : %true for @host_initiated. Though I would add a prep patch to provide wrappers > : for __kvm_get_msr() and __kvm_set_msr(). Naming will be hard, but I think we > ^^^^^^^^^^^^^^^^^^^ > : can use kvm_{read,write}_msr() to go along with the KVM-initiated register > : accessors/mutators, e.g. kvm_register_read(), kvm_pdptr_write(), etc. > > [*] https://lore.kernel.org/all/ZM0YZgFsYWuBFOze@google.com > > > Also functions like kvm_set_msr_ignored_check(), kvm_set_msr_with_filter() and such, > > IMHO have names that are not very user friendly. > > I don't like the host/guest split because KVM always operates on guest values, > e.g. kvm_msr_set_host() in particular could get confusing. That makes sense. > > IMO kvm_get_msr() and kvm_set_msr(), and to some extent the helpers you note below, > are the real problem. > > What if we rename kvm_{g,s}et_msr() to kvm_emulate_msr_{read,write}() to make it > more obvious that those are the "guest" helpers? And do that as a prep patch in > this series (there aren't _that_ many users). Makes sense. > > I'm also in favor of renaming the "inner" helpers, but I think we should tackle > those separately.separately OK. > Best regards, Maxim Levitsky
On 11/3/2023 2:26 AM, Maxim Levitsky wrote: > On Wed, 2023-11-01 at 12:32 -0700, Sean Christopherson wrote: >> On Tue, Oct 31, 2023, Maxim Levitsky wrote: >>> On Thu, 2023-09-14 at 02:33 -0400, Yang Weijiang wrote: >>>> Wrap __kvm_{get,set}_msr() into two new helpers for KVM usage and use the >>>> helpers to replace existing usage of the raw functions. >>>> kvm_msr_{read,write}() are KVM-internal helpers, i.e. used when KVM needs >>>> to get/set a MSR value for emulating CPU behavior. >>> I am not sure if I like this patch or not. On one hand the code is cleaner >>> this way, but on the other hand now it is easier to call kvm_msr_write() on >>> behalf of the guest. >>> >>> For example we also have the 'kvm_set_msr()' which does actually set the msr >>> on behalf of the guest. >>> >>> How about we call the new function kvm_msr_set_host() and rename >>> kvm_set_msr() to kvm_msr_set_guest(), together with good comments explaning >>> what they do? >> LOL, just call me Nostradamus[*] ;-) >> >> : > SSP save/load should go to enter_smm_save_state_64() and rsm_load_state_64(), >> : > where other fields of SMRAM are handled. >> : >> : +1. The right way to get/set MSRs like this is to use __kvm_get_msr() and pass >> : %true for @host_initiated. Though I would add a prep patch to provide wrappers >> : for __kvm_get_msr() and __kvm_set_msr(). Naming will be hard, but I think we >> ^^^^^^^^^^^^^^^^^^^ >> : can use kvm_{read,write}_msr() to go along with the KVM-initiated register >> : accessors/mutators, e.g. kvm_register_read(), kvm_pdptr_write(), etc. >> >> [*] https://lore.kernel.org/all/ZM0YZgFsYWuBFOze@google.com >> >>> Also functions like kvm_set_msr_ignored_check(), kvm_set_msr_with_filter() and such, >>> IMHO have names that are not very user friendly. >> I don't like the host/guest split because KVM always operates on guest values, >> e.g. kvm_msr_set_host() in particular could get confusing. > That makes sense. > >> IMO kvm_get_msr() and kvm_set_msr(), and to some extent the helpers you note below, >> are the real problem. >> >> What if we rename kvm_{g,s}et_msr() to kvm_emulate_msr_{read,write}() to make it >> more obvious that those are the "guest" helpers? And do that as a prep patch in >> this series (there aren't _that_ many users). > Makes sense. Then I'll modify related code and add the pre-patch in next version, thanks! >> I'm also in favor of renaming the "inner" helpers, but I think we should tackle >> those separately.separately > OK. > > Best regards, > Maxim Levitsky > >
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index 1a4def36d5bb..0fc5e6312e93 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -1956,7 +1956,9 @@ void kvm_prepare_emulation_failure_exit(struct kvm_vcpu *vcpu); void kvm_enable_efer_bits(u64); bool kvm_valid_efer(struct kvm_vcpu *vcpu, u64 efer); -int __kvm_get_msr(struct kvm_vcpu *vcpu, u32 index, u64 *data, bool host_initiated); + +int kvm_msr_read(struct kvm_vcpu *vcpu, u32 index, u64 *data); +int kvm_msr_write(struct kvm_vcpu *vcpu, u32 index, u64 data); int kvm_get_msr(struct kvm_vcpu *vcpu, u32 index, u64 *data); int kvm_set_msr(struct kvm_vcpu *vcpu, u32 index, u64 data); int kvm_emulate_rdmsr(struct kvm_vcpu *vcpu); diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index 7c3e4a550ca7..1f206caec559 100644 --- a/arch/x86/kvm/cpuid.c +++ b/arch/x86/kvm/cpuid.c @@ -1531,7 +1531,7 @@ bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx, *edx = entry->edx; if (function == 7 && index == 0) { u64 data; - if (!__kvm_get_msr(vcpu, MSR_IA32_TSX_CTRL, &data, true) && + if (!kvm_msr_read(vcpu, MSR_IA32_TSX_CTRL, &data) && (data & TSX_CTRL_CPUID_CLEAR)) *ebx &= ~(F(RTM) | F(HLE)); } else if (function == 0x80000007) { diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 6c9c81e82e65..e0b55c043dab 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -1917,8 +1917,8 @@ static int kvm_set_msr_ignored_check(struct kvm_vcpu *vcpu, * Returns 0 on success, non-0 otherwise. * Assumes vcpu_load() was already called. */ -int __kvm_get_msr(struct kvm_vcpu *vcpu, u32 index, u64 *data, - bool host_initiated) +static int __kvm_get_msr(struct kvm_vcpu *vcpu, u32 index, u64 *data, + bool host_initiated) { struct msr_data msr; int ret; @@ -1944,6 +1944,16 @@ int __kvm_get_msr(struct kvm_vcpu *vcpu, u32 index, u64 *data, return ret; } +int kvm_msr_write(struct kvm_vcpu *vcpu, u32 index, u64 data) +{ + return __kvm_set_msr(vcpu, index, data, true); +} + +int kvm_msr_read(struct kvm_vcpu *vcpu, u32 index, u64 *data) +{ + return __kvm_get_msr(vcpu, index, data, true); +} + static int kvm_get_msr_ignored_check(struct kvm_vcpu *vcpu, u32 index, u64 *data, bool host_initiated) { @@ -12082,7 +12092,7 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event) MSR_IA32_MISC_ENABLE_BTS_UNAVAIL; __kvm_set_xcr(vcpu, 0, XFEATURE_MASK_FP); - __kvm_set_msr(vcpu, MSR_IA32_XSS, 0, true); + kvm_msr_write(vcpu, MSR_IA32_XSS, 0); } /* All GPRs except RDX (handled below) are zeroed on RESET/INIT. */
Wrap __kvm_{get,set}_msr() into two new helpers for KVM usage and use the helpers to replace existing usage of the raw functions. kvm_msr_{read,write}() are KVM-internal helpers, i.e. used when KVM needs to get/set a MSR value for emulating CPU behavior. Suggested-by: Sean Christopherson <seanjc@google.com> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> --- arch/x86/include/asm/kvm_host.h | 4 +++- arch/x86/kvm/cpuid.c | 2 +- arch/x86/kvm/x86.c | 16 +++++++++++++--- 3 files changed, 17 insertions(+), 5 deletions(-)