Re: [PATCH] KVM: x86: Fix guest debug across vcpu INIT reset

[Avi Kivity <avi@redhat.com>]

On 09/19/2012 10:38 PM, Jan Kiszka wrote:
> If we reset a vcpu on INIT, make sure to not touch dr7 as stored in the
> VMCS/VMCB and also switch_db_regs if guest debugging is using hardware
> breakpoints. Otherwise, the vcpu will not trigger hardware breakpoints
> until userspace issues another KVM_SET_GUEST_DEBUG IOCTL for it.
> 
> Found while trying to stop on start_secondary.
> 
> @@ -1146,7 +1146,8 @@ static void init_vmcb(struct vcpu_svm *svm)
>  
>  	svm_set_efer(&svm->vcpu, 0);
>  	save->dr6 = 0xffff0ff0;
> -	save->dr7 = 0x400;
> +	if (!(svm->vcpu.guest_debug & KVM_GUESTDBG_USE_HW_BP))
> +		save->dr7 = 0x400;

Whenever we multiplex a resource foo for several users, we should have a
function update_foo() that recalculates foo from all its sources:

        vcpu->arch.dr7 = 0x400;
        update_dr7(&svm->vcpu);

(don't know if the first line really belongs here)

>  	kvm_set_rflags(&svm->vcpu, 2);
>  	save->rip = 0x0000fff0;
>  	svm->vcpu.arch.regs[VCPU_REGS_RIP] = save->rip;
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index d62b413..37f68f7 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -3959,7 +3959,8 @@ static int vmx_vcpu_reset(struct kvm_vcpu *vcpu)
>  		kvm_rip_write(vcpu, 0);
>  	kvm_register_write(vcpu, VCPU_REGS_RSP, 0);
>  
> -	vmcs_writel(GUEST_DR7, 0x400);
> +	if (!(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP))
> +		vmcs_writel(GUEST_DR7, 0x400);
>  

Ditto. update_dr7() could even be generic and use ->set_dr7().

>  	vmcs_writel(GUEST_GDTR_BASE, 0);
>  	vmcs_write32(GUEST_GDTR_LIMIT, 0xffff);
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index c4d451e..7c8c2b8 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6043,7 +6043,8 @@ int kvm_arch_vcpu_reset(struct kvm_vcpu *vcpu)
>  	vcpu->arch.nmi_pending = 0;
>  	vcpu->arch.nmi_injected = false;
>  
> -	vcpu->arch.switch_db_regs = 0;
> +	if (!(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP))
> +		vcpu->arch.switch_db_regs = 0;
>  	memset(vcpu->arch.db, 0, sizeof(vcpu->arch.db));
>  	vcpu->arch.dr6 = DR6_FIXED_1;
>  	vcpu->arch.dr7 = DR7_FIXED_1;
> 

You could move the switch_db_regs calculation into update_dr7(), and
move the update_dr7() call after assigning it here.  This fixes
everything neatly (and obviates the need for the first line in the
snippet above).


-- 
error compiling committee.c: too many arguments to function