Re: [RFC PATCH 1/2] KVM: x86: Add a framework for supporting MSR-based features
On 14/02/2018 17:44, Borislav Petkov wrote: > On Thu, Feb 08, 2018 at 04:58:46PM -0600, Tom Lendacky wrote: >> @@ -2681,11 +2731,15 @@ static int __msr_io(struct kvm_vcpu *vcpu, struct >> kvm_msrs *msrs, >> { >> int i, idx; >> >> -idx = srcu_read_lock(>kvm->srcu); >> +if (vcpu) >> +idx = srcu_read_lock(>kvm->srcu); >> + >> for (i = 0; i < msrs->nmsrs; ++i) >> if (do_msr(vcpu, entries[i].index, [i].data)) >> break; >> -srcu_read_unlock(>kvm->srcu, idx); >> + >> +if (vcpu) >> +srcu_read_unlock(>kvm->srcu, idx); > > > ./include/linux/srcu.h:175:2: warning: ‘idx’ may be used uninitialized in > this function [-Wmaybe-uninitialized] > __srcu_read_unlock(sp, idx); > ^~~ > arch/x86/kvm/x86.c:2739:9: note: ‘idx’ was declared here > int i, idx; > ^~~ > > I know, silly gcc. > Nice point---even better, just push srcu_read_lock/unlock to msr_io or even msr_io's callers. Thanks, Paolo
Re: [RFC PATCH 1/2] KVM: x86: Add a framework for supporting MSR-based features
On 14/02/2018 17:44, Borislav Petkov wrote: > On Thu, Feb 08, 2018 at 04:58:46PM -0600, Tom Lendacky wrote: >> @@ -2681,11 +2731,15 @@ static int __msr_io(struct kvm_vcpu *vcpu, struct >> kvm_msrs *msrs, >> { >> int i, idx; >> >> -idx = srcu_read_lock(>kvm->srcu); >> +if (vcpu) >> +idx = srcu_read_lock(>kvm->srcu); >> + >> for (i = 0; i < msrs->nmsrs; ++i) >> if (do_msr(vcpu, entries[i].index, [i].data)) >> break; >> -srcu_read_unlock(>kvm->srcu, idx); >> + >> +if (vcpu) >> +srcu_read_unlock(>kvm->srcu, idx); > > > ./include/linux/srcu.h:175:2: warning: ‘idx’ may be used uninitialized in > this function [-Wmaybe-uninitialized] > __srcu_read_unlock(sp, idx); > ^~~ > arch/x86/kvm/x86.c:2739:9: note: ‘idx’ was declared here > int i, idx; > ^~~ > > I know, silly gcc. > Nice point---even better, just push srcu_read_lock/unlock to msr_io or even msr_io's callers. Thanks, Paolo
Re: [RFC PATCH 1/2] KVM: x86: Add a framework for supporting MSR-based features
On Thu, Feb 08, 2018 at 04:58:46PM -0600, Tom Lendacky wrote: > @@ -2681,11 +2731,15 @@ static int __msr_io(struct kvm_vcpu *vcpu, struct > kvm_msrs *msrs, > { > int i, idx; > > - idx = srcu_read_lock(>kvm->srcu); > + if (vcpu) > + idx = srcu_read_lock(>kvm->srcu); > + > for (i = 0; i < msrs->nmsrs; ++i) > if (do_msr(vcpu, entries[i].index, [i].data)) > break; > - srcu_read_unlock(>kvm->srcu, idx); > + > + if (vcpu) > + srcu_read_unlock(>kvm->srcu, idx); ./include/linux/srcu.h:175:2: warning: ‘idx’ may be used uninitialized in this function [-Wmaybe-uninitialized] __srcu_read_unlock(sp, idx); ^~~ arch/x86/kvm/x86.c:2739:9: note: ‘idx’ was declared here int i, idx; ^~~ I know, silly gcc. -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.
Re: [RFC PATCH 1/2] KVM: x86: Add a framework for supporting MSR-based features
On Thu, Feb 08, 2018 at 04:58:46PM -0600, Tom Lendacky wrote: > @@ -2681,11 +2731,15 @@ static int __msr_io(struct kvm_vcpu *vcpu, struct > kvm_msrs *msrs, > { > int i, idx; > > - idx = srcu_read_lock(>kvm->srcu); > + if (vcpu) > + idx = srcu_read_lock(>kvm->srcu); > + > for (i = 0; i < msrs->nmsrs; ++i) > if (do_msr(vcpu, entries[i].index, [i].data)) > break; > - srcu_read_unlock(>kvm->srcu, idx); > + > + if (vcpu) > + srcu_read_unlock(>kvm->srcu, idx); ./include/linux/srcu.h:175:2: warning: ‘idx’ may be used uninitialized in this function [-Wmaybe-uninitialized] __srcu_read_unlock(sp, idx); ^~~ arch/x86/kvm/x86.c:2739:9: note: ‘idx’ was declared here int i, idx; ^~~ I know, silly gcc. -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.
Re: [RFC PATCH 1/2] KVM: x86: Add a framework for supporting MSR-based features
On 14/02/2018 05:42, Tom Lendacky wrote: >>> +bool kvm_valid_msr_feature(u32 msr, u64 data) >>> +{ >>> + unsigned int i; >>> + >>> + for (i = 0; i < num_msr_based_features; i++) { >>> + struct kvm_msr_based_features *m = msr_based_features + i; >>> + >>> + if (msr != m->msr) >>> + continue; >>> + >>> + /* Make sure not trying to change unsupported bits */ >>> + return (data & ~m->mask) ? false : true; >>> + } >>> + >>> + return false; >>> +} >>> +EXPORT_SYMBOL_GPL(kvm_valid_msr_feature); >>> + >> >> This is probably unnecessary too (the allowed values are a bit more >> complicated for, you just guessed it, VMX capability MSRs) and you can >> just check bits other than LFENCE in svm_set_msr. > > The whole routine or just the bit checking? I can see still needing the > check to be sure the "feature" is present. You can return the MSR unconditionally from KVM_GET_MSR_INDEX_LIST. Then KVM_GET_MSR would return 0 or 1 depending on whether the feature is present. Paolo
Re: [RFC PATCH 1/2] KVM: x86: Add a framework for supporting MSR-based features
On 14/02/2018 05:42, Tom Lendacky wrote: >>> +bool kvm_valid_msr_feature(u32 msr, u64 data) >>> +{ >>> + unsigned int i; >>> + >>> + for (i = 0; i < num_msr_based_features; i++) { >>> + struct kvm_msr_based_features *m = msr_based_features + i; >>> + >>> + if (msr != m->msr) >>> + continue; >>> + >>> + /* Make sure not trying to change unsupported bits */ >>> + return (data & ~m->mask) ? false : true; >>> + } >>> + >>> + return false; >>> +} >>> +EXPORT_SYMBOL_GPL(kvm_valid_msr_feature); >>> + >> >> This is probably unnecessary too (the allowed values are a bit more >> complicated for, you just guessed it, VMX capability MSRs) and you can >> just check bits other than LFENCE in svm_set_msr. > > The whole routine or just the bit checking? I can see still needing the > check to be sure the "feature" is present. You can return the MSR unconditionally from KVM_GET_MSR_INDEX_LIST. Then KVM_GET_MSR would return 0 or 1 depending on whether the feature is present. Paolo
Re: [RFC PATCH 1/2] KVM: x86: Add a framework for supporting MSR-based features
On 2/13/2018 10:25 AM, Paolo Bonzini wrote: > On 08/02/2018 23:58, Tom Lendacky wrote: >> +bool kvm_valid_msr_feature(u32 msr, u64 data) >> +{ >> +unsigned int i; >> + >> +for (i = 0; i < num_msr_based_features; i++) { >> +struct kvm_msr_based_features *m = msr_based_features + i; >> + >> +if (msr != m->msr) >> +continue; >> + >> +/* Make sure not trying to change unsupported bits */ >> +return (data & ~m->mask) ? false : true; >> +} >> + >> +return false; >> +} >> +EXPORT_SYMBOL_GPL(kvm_valid_msr_feature); >> + > > This is probably unnecessary too (the allowed values are a bit more > complicated for, you just guessed it, VMX capability MSRs) and you can > just check bits other than LFENCE in svm_set_msr. The whole routine or just the bit checking? I can see still needing the check to be sure the "feature" is present. Thanks, Tom > > Paolo >
Re: [RFC PATCH 1/2] KVM: x86: Add a framework for supporting MSR-based features
On 2/13/2018 10:25 AM, Paolo Bonzini wrote: > On 08/02/2018 23:58, Tom Lendacky wrote: >> +bool kvm_valid_msr_feature(u32 msr, u64 data) >> +{ >> +unsigned int i; >> + >> +for (i = 0; i < num_msr_based_features; i++) { >> +struct kvm_msr_based_features *m = msr_based_features + i; >> + >> +if (msr != m->msr) >> +continue; >> + >> +/* Make sure not trying to change unsupported bits */ >> +return (data & ~m->mask) ? false : true; >> +} >> + >> +return false; >> +} >> +EXPORT_SYMBOL_GPL(kvm_valid_msr_feature); >> + > > This is probably unnecessary too (the allowed values are a bit more > complicated for, you just guessed it, VMX capability MSRs) and you can > just check bits other than LFENCE in svm_set_msr. The whole routine or just the bit checking? I can see still needing the check to be sure the "feature" is present. Thanks, Tom > > Paolo >
Re: [RFC PATCH 1/2] KVM: x86: Add a framework for supporting MSR-based features
On 2/13/2018 10:21 AM, Paolo Bonzini wrote: > On 08/02/2018 23:58, Tom Lendacky wrote: >> Provide a new KVM capability that allows bits within MSRs to be recognized >> as features. Two new ioctls are added to the VM ioctl routine to retrieve >> the list of these MSRs and their values. The MSR features can optionally >> be exposed based on a CPU and/or a CPU feature. > > Yes, pretty much. Just two changes: > >> +struct kvm_msr_based_features { >> +u32 msr;/* MSR to query */ >> +u64 mask; /* MSR mask */ >> +const struct x86_cpu_id *match; /* Match criteria */ >> +u64 value; /* MSR value */ > > 1) These two should be replaced by a kvm_x86_ops callback, because > computing the value is sometimes a bit more complicated than just rdmsr > (for example, MSRs for VMX capabilities depend on the kvm_intel.ko > module parameters). Ok, I'll rework this. > > >> +case KVM_CAP_GET_MSR_FEATURES: > > This should be KVM_GET_MSR. Yup, not sure what I was thinking there. > >> +r = msr_io(NULL, argp, do_get_msr_features, 1); >> +break; > > > Bonus points for writing documentation :) and for moving the MSR> handling > code to arch/x86/kvm/msr.{c,h}. Yup, there will be documentation on it - I wanted to make sure the direction was correct first. Splitting out msr.c/msr.h might be best as a separate patchset, let me see what's involved. Thanks, Tom > > Thanks, > > Paolo >
Re: [RFC PATCH 1/2] KVM: x86: Add a framework for supporting MSR-based features
On 2/13/2018 10:21 AM, Paolo Bonzini wrote: > On 08/02/2018 23:58, Tom Lendacky wrote: >> Provide a new KVM capability that allows bits within MSRs to be recognized >> as features. Two new ioctls are added to the VM ioctl routine to retrieve >> the list of these MSRs and their values. The MSR features can optionally >> be exposed based on a CPU and/or a CPU feature. > > Yes, pretty much. Just two changes: > >> +struct kvm_msr_based_features { >> +u32 msr;/* MSR to query */ >> +u64 mask; /* MSR mask */ >> +const struct x86_cpu_id *match; /* Match criteria */ >> +u64 value; /* MSR value */ > > 1) These two should be replaced by a kvm_x86_ops callback, because > computing the value is sometimes a bit more complicated than just rdmsr > (for example, MSRs for VMX capabilities depend on the kvm_intel.ko > module parameters). Ok, I'll rework this. > > >> +case KVM_CAP_GET_MSR_FEATURES: > > This should be KVM_GET_MSR. Yup, not sure what I was thinking there. > >> +r = msr_io(NULL, argp, do_get_msr_features, 1); >> +break; > > > Bonus points for writing documentation :) and for moving the MSR> handling > code to arch/x86/kvm/msr.{c,h}. Yup, there will be documentation on it - I wanted to make sure the direction was correct first. Splitting out msr.c/msr.h might be best as a separate patchset, let me see what's involved. Thanks, Tom > > Thanks, > > Paolo >
Re: [RFC PATCH 1/2] KVM: x86: Add a framework for supporting MSR-based features
On 08/02/2018 23:58, Tom Lendacky wrote: > +bool kvm_valid_msr_feature(u32 msr, u64 data) > +{ > + unsigned int i; > + > + for (i = 0; i < num_msr_based_features; i++) { > + struct kvm_msr_based_features *m = msr_based_features + i; > + > + if (msr != m->msr) > + continue; > + > + /* Make sure not trying to change unsupported bits */ > + return (data & ~m->mask) ? false : true; > + } > + > + return false; > +} > +EXPORT_SYMBOL_GPL(kvm_valid_msr_feature); > + This is probably unnecessary too (the allowed values are a bit more complicated for, you just guessed it, VMX capability MSRs) and you can just check bits other than LFENCE in svm_set_msr. Paolo
Re: [RFC PATCH 1/2] KVM: x86: Add a framework for supporting MSR-based features
On 08/02/2018 23:58, Tom Lendacky wrote: > +bool kvm_valid_msr_feature(u32 msr, u64 data) > +{ > + unsigned int i; > + > + for (i = 0; i < num_msr_based_features; i++) { > + struct kvm_msr_based_features *m = msr_based_features + i; > + > + if (msr != m->msr) > + continue; > + > + /* Make sure not trying to change unsupported bits */ > + return (data & ~m->mask) ? false : true; > + } > + > + return false; > +} > +EXPORT_SYMBOL_GPL(kvm_valid_msr_feature); > + This is probably unnecessary too (the allowed values are a bit more complicated for, you just guessed it, VMX capability MSRs) and you can just check bits other than LFENCE in svm_set_msr. Paolo
Re: [RFC PATCH 1/2] KVM: x86: Add a framework for supporting MSR-based features
On 08/02/2018 23:58, Tom Lendacky wrote: > Provide a new KVM capability that allows bits within MSRs to be recognized > as features. Two new ioctls are added to the VM ioctl routine to retrieve > the list of these MSRs and their values. The MSR features can optionally > be exposed based on a CPU and/or a CPU feature. Yes, pretty much. Just two changes: > +struct kvm_msr_based_features { > + u32 msr;/* MSR to query */ > + u64 mask; /* MSR mask */ > + const struct x86_cpu_id *match; /* Match criteria */ > + u64 value; /* MSR value */ 1) These two should be replaced by a kvm_x86_ops callback, because computing the value is sometimes a bit more complicated than just rdmsr (for example, MSRs for VMX capabilities depend on the kvm_intel.ko module parameters). > + case KVM_CAP_GET_MSR_FEATURES: This should be KVM_GET_MSR. > + r = msr_io(NULL, argp, do_get_msr_features, 1); > + break; Bonus points for writing documentation :) and for moving the MSR handling code to arch/x86/kvm/msr.{c,h}. Thanks, Paolo
Re: [RFC PATCH 1/2] KVM: x86: Add a framework for supporting MSR-based features
On 08/02/2018 23:58, Tom Lendacky wrote: > Provide a new KVM capability that allows bits within MSRs to be recognized > as features. Two new ioctls are added to the VM ioctl routine to retrieve > the list of these MSRs and their values. The MSR features can optionally > be exposed based on a CPU and/or a CPU feature. Yes, pretty much. Just two changes: > +struct kvm_msr_based_features { > + u32 msr;/* MSR to query */ > + u64 mask; /* MSR mask */ > + const struct x86_cpu_id *match; /* Match criteria */ > + u64 value; /* MSR value */ 1) These two should be replaced by a kvm_x86_ops callback, because computing the value is sometimes a bit more complicated than just rdmsr (for example, MSRs for VMX capabilities depend on the kvm_intel.ko module parameters). > + case KVM_CAP_GET_MSR_FEATURES: This should be KVM_GET_MSR. > + r = msr_io(NULL, argp, do_get_msr_features, 1); > + break; Bonus points for writing documentation :) and for moving the MSR handling code to arch/x86/kvm/msr.{c,h}. Thanks, Paolo