[PATCH] KVM: Clean up kvm_vm_ioctl_create_vcpu()

[PATCH] KVM: Clean up kvm_vm_ioctl_create_vcpu()

[Michal Luczaj]

Since c9d601548603 ("KVM: allow KVM_BUG/KVM_BUG_ON to handle 64-bit cond")
'cond' is internally converted to boolean, so caller's explicit conversion
from void* is unnecessary.

Remove the double bang.

Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 virt/kvm/kvm_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 6a658f30af91..64dd940c549e 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -3975,7 +3975,7 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id)
 	if (r < 0)
 		goto kvm_put_xa_release;
 
-	if (KVM_BUG_ON(!!xa_store(&kvm->vcpu_array, vcpu->vcpu_idx, vcpu, 0), kvm)) {
+	if (KVM_BUG_ON(xa_store(&kvm->vcpu_array, vcpu->vcpu_idx, vcpu, 0), kvm)) {
 		r = -EINVAL;
 		goto kvm_put_xa_release;
 	}

base-commit: 31b4fc3bc64aadd660c5bfa5178c86a7ba61e0f7
-- 
2.41.0

Re: [PATCH] KVM: Clean up kvm_vm_ioctl_create_vcpu()

[Yuan Yao]

On Mon, Jun 05, 2023 at 01:44:19PM +0200, Michal Luczaj wrote:
> Since c9d601548603 ("KVM: allow KVM_BUG/KVM_BUG_ON to handle 64-bit cond")
> 'cond' is internally converted to boolean, so caller's explicit conversion
> from void* is unnecessary.
>
> Remove the double bang.
>
> Signed-off-by: Michal Luczaj <mhal@rbox.co>
> ---
>  virt/kvm/kvm_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 6a658f30af91..64dd940c549e 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -3975,7 +3975,7 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id)
>  	if (r < 0)
>  		goto kvm_put_xa_release;
>
> -	if (KVM_BUG_ON(!!xa_store(&kvm->vcpu_array, vcpu->vcpu_idx, vcpu, 0), kvm)) {
> +	if (KVM_BUG_ON(xa_store(&kvm->vcpu_array, vcpu->vcpu_idx, vcpu, 0), kvm)) {

Looks the only one place for KVM_BUG_ON().

Reviewed-by: Yuan Yao <yuan.yao@intel.com>

BTW: svm_get_lbr_msr() is using KVM_BUG(false, ...) and
handle_cr() is using KVM_BUG(1, ...), a chance to
change them to same style ?

>  		r = -EINVAL;
>  		goto kvm_put_xa_release;
>  	}
>
> base-commit: 31b4fc3bc64aadd660c5bfa5178c86a7ba61e0f7
> --
> 2.41.0
>

Re: [PATCH] KVM: Clean up kvm_vm_ioctl_create_vcpu()

[Michal Luczaj]

On 6/5/23 15:03, Yuan Yao wrote:
> On Mon, Jun 05, 2023 at 01:44:19PM +0200, Michal Luczaj wrote:
>> Since c9d601548603 ("KVM: allow KVM_BUG/KVM_BUG_ON to handle 64-bit cond")
>> 'cond' is internally converted to boolean, so caller's explicit conversion
>> from void* is unnecessary.
>>
>> Remove the double bang.
>> ...
>> -	if (KVM_BUG_ON(!!xa_store(&kvm->vcpu_array, vcpu->vcpu_idx, vcpu, 0), kvm)) {
>> +	if (KVM_BUG_ON(xa_store(&kvm->vcpu_array, vcpu->vcpu_idx, vcpu, 0), kvm)) {
> 
> Looks the only one place for KVM_BUG_ON().
> 
> Reviewed-by: Yuan Yao <yuan.yao@intel.com>
> 
> BTW: svm_get_lbr_msr() is using KVM_BUG(false, ...) and
> handle_cr() is using KVM_BUG(1, ...), a chance to
> change them to same style ?

Sure, but KVM_BUG(false, ...) is a no-op, right? Would you like me to fix it
separately with KVM_BUG(1, ...) as a (hardly significant) functional change?

Also, am I correct to assume that (1, ) is the preferred style?
arch/powerpc/kvm/book3s_64_mmu_host.c:kvmppc_mmu_map_page() seems to be the only
exception (within KVM) with a `WARN_ON(true)`.

Re: [PATCH] KVM: Clean up kvm_vm_ioctl_create_vcpu()

[Sean Christopherson]

On Mon, Jun 05, 2023, Michal Luczaj wrote:
> On 6/5/23 15:03, Yuan Yao wrote:
> > On Mon, Jun 05, 2023 at 01:44:19PM +0200, Michal Luczaj wrote:
> >> Since c9d601548603 ("KVM: allow KVM_BUG/KVM_BUG_ON to handle 64-bit cond")
> >> 'cond' is internally converted to boolean, so caller's explicit conversion
> >> from void* is unnecessary.
> >>
> >> Remove the double bang.
> >> ...
> >> -	if (KVM_BUG_ON(!!xa_store(&kvm->vcpu_array, vcpu->vcpu_idx, vcpu, 0), kvm)) {
> >> +	if (KVM_BUG_ON(xa_store(&kvm->vcpu_array, vcpu->vcpu_idx, vcpu, 0), kvm)) {
> > 
> > Looks the only one place for KVM_BUG_ON().
> > 
> > Reviewed-by: Yuan Yao <yuan.yao@intel.com>
> > 
> > BTW: svm_get_lbr_msr() is using KVM_BUG(false, ...) and
> > handle_cr() is using KVM_BUG(1, ...), a chance to
> > change them to same style ?
> 
> Sure, but KVM_BUG(false, ...) is a no-op, right? Would you like me to fix it
> separately with KVM_BUG(1, ...) as a (hardly significant) functional change?

Heh, yeah, that's dead code and should be fixed separately.  I think that should
just be a WARN_ON_ONCE(1) though, there's no reason to bug and kill the VM.
Actually, there's really no reason for double switch, KVM can simply provide a
helper to get the correct VMCB.  That'd provide an excuse to clean up a few other
uglies in svm_update_lbrv() too.  Tentative patch at the bottom.

> Also, am I correct to assume that (1, ) is the preferred style?

I don't think there's a preferred style, though (1, ...) is more prevalent, and
has the advantage of saving three chars for the message :-)

> arch/powerpc/kvm/book3s_64_mmu_host.c:kvmppc_mmu_map_page() seems to be the only
> exception (within KVM) with a `WARN_ON(true)`.

Yeah, I wouldn't worry about that one.

---
 arch/x86/kvm/svm/svm.c | 56 ++++++++++++++----------------------------
 1 file changed, 19 insertions(+), 37 deletions(-)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index ff48cdea1fbf..406b318f2f0d 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -960,50 +960,24 @@ static void svm_disable_lbrv(struct kvm_vcpu *vcpu)
 		svm_copy_lbrs(svm->vmcb01.ptr, svm->vmcb);
 }
 
-static int svm_get_lbr_msr(struct vcpu_svm *svm, u32 index)
+static struct vmcb *svm_get_lbr_vmcb(struct vcpu_svm *svm)
 {
 	/*
-	 * If the LBR virtualization is disabled, the LBR msrs are always
-	 * kept in the vmcb01 to avoid copying them on nested guest entries.
-	 *
-	 * If nested, and the LBR virtualization is enabled/disabled, the msrs
-	 * are moved between the vmcb01 and vmcb02 as needed.
+	 * If LBR virtualization is disabled, the LBR MSRs are always kept in
+	 * vmcb01.  If LBR virtualization is enabled and L1 is running VMs of
+	 * its own, the MSRs are moved between vmcb01 and vmcb02 as needed.
 	 */
-	struct vmcb *vmcb =
-		(svm->vmcb->control.virt_ext & LBR_CTL_ENABLE_MASK) ?
-			svm->vmcb : svm->vmcb01.ptr;
-
-	switch (index) {
-	case MSR_IA32_DEBUGCTLMSR:
-		return vmcb->save.dbgctl;
-	case MSR_IA32_LASTBRANCHFROMIP:
-		return vmcb->save.br_from;
-	case MSR_IA32_LASTBRANCHTOIP:
-		return vmcb->save.br_to;
-	case MSR_IA32_LASTINTFROMIP:
-		return vmcb->save.last_excp_from;
-	case MSR_IA32_LASTINTTOIP:
-		return vmcb->save.last_excp_to;
-	default:
-		KVM_BUG(false, svm->vcpu.kvm,
-			"%s: Unknown MSR 0x%x", __func__, index);
-		return 0;
-	}
+	return svm->vmcb->control.virt_ext & LBR_CTL_ENABLE_MASK ? svm->vmcb :
+								   svm->vmcb01.ptr;
 }
 
 void svm_update_lbrv(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
-
-	bool enable_lbrv = svm_get_lbr_msr(svm, MSR_IA32_DEBUGCTLMSR) &
-					   DEBUGCTLMSR_LBR;
-
-	bool current_enable_lbrv = !!(svm->vmcb->control.virt_ext &
-				      LBR_CTL_ENABLE_MASK);
-
-	if (unlikely(is_guest_mode(vcpu) && svm->lbrv_enabled))
-		if (unlikely(svm->nested.ctl.virt_ext & LBR_CTL_ENABLE_MASK))
-			enable_lbrv = true;
+	bool current_enable_lbrv = svm->vmcb->control.virt_ext & LBR_CTL_ENABLE_MASK;
+	bool enable_lbrv = (svm_get_lbr_vmcb(svm)->save.dbgctl & DEBUGCTLMSR_LBR) ||
+			   (is_guest_mode(vcpu) && svm->lbrv_enabled &&
+			    (svm->nested.ctl.virt_ext & LBR_CTL_ENABLE_MASK));
 
 	if (enable_lbrv == current_enable_lbrv)
 		return;
@@ -2808,11 +2782,19 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		msr_info->data = svm->tsc_aux;
 		break;
 	case MSR_IA32_DEBUGCTLMSR:
+		msr_info->data = svm_get_lbr_vmcb(svm)->save.dbgctl;
+		break;
 	case MSR_IA32_LASTBRANCHFROMIP:
+		msr_info->data = svm_get_lbr_vmcb(svm)->save.br_from;
+		break;
 	case MSR_IA32_LASTBRANCHTOIP:
+		msr_info->data = svm_get_lbr_vmcb(svm)->save.br_to;
+		break;
 	case MSR_IA32_LASTINTFROMIP:
+		msr_info->data = svm_get_lbr_vmcb(svm)->save.last_excp_from;
+		break;
 	case MSR_IA32_LASTINTTOIP:
-		msr_info->data = svm_get_lbr_msr(svm, msr_info->index);
+		msr_info->data = svm_get_lbr_vmcb(svm)->save.last_excp_to;
 		break;
 	case MSR_VM_HSAVE_PA:
 		msr_info->data = svm->nested.hsave_msr;

base-commit: 76a17bf03a268bc342e08c05d8ddbe607d294eb4
-- 

Re: [PATCH] KVM: Clean up kvm_vm_ioctl_create_vcpu()

[Sean Christopherson]

On Mon, 05 Jun 2023 13:44:19 +0200, Michal Luczaj wrote:
> Since c9d601548603 ("KVM: allow KVM_BUG/KVM_BUG_ON to handle 64-bit cond")
> 'cond' is internally converted to boolean, so caller's explicit conversion
> from void* is unnecessary.
> 
> Remove the double bang.
> 
> 
> [...]

Applied to kvm-x86 generic, thanks!

[1/1] KVM: Clean up kvm_vm_ioctl_create_vcpu()
      https://github.com/kvm-x86/linux/commit/5f643e460ab1

--
https://github.com/kvm-x86/linux/tree/next
https://github.com/kvm-x86/linux/tree/fixes