Re: [PATCH 4/6] hw-breakpoints: Rewrite the hw-breakpoints layer on top of perf events

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 2292 bytes --]

Frederic Weisbecker wrote:
> On Sun, Nov 01, 2009 at 11:09:03PM +0100, Jan Kiszka wrote:
>>> @@ -3643,14 +3644,8 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>>>  	trace_kvm_entry(vcpu->vcpu_id);
>>>  	kvm_x86_ops->run(vcpu, kvm_run);
>>>  
>>> -	if (unlikely(vcpu->arch.switch_db_regs || test_thread_flag(TIF_DEBUG))) {
>>> -		set_debugreg(current->thread.debugreg[0], 0);
>>> -		set_debugreg(current->thread.debugreg[1], 1);
>>> -		set_debugreg(current->thread.debugreg[2], 2);
>>> -		set_debugreg(current->thread.debugreg[3], 3);
>>> -		set_debugreg(current->thread.debugreg6, 6);
>>> -		set_debugreg(current->thread.debugreg7, 7);
>>> -	}
>>> +	if (unlikely(vcpu->arch.switch_db_regs || test_thread_flag(TIF_DEBUG)))
>>> +		hw_breakpoint_restore();
>> TIF_DEBUG is only set on active ptrace hw-breakpoints, thus we miss
>> other types here, right? (Note: arch.switch_db_regs is guest-related,
>> thus does not help in this regard.)
>>
>> Jan
>>
> 
> 
> About this. vcpu->arch.switch_db_regs is guest related but it looks
> like the only thing I need to check.
> 
> I'm not sure when it is activated. Is it always done once the guest
> changes its debug registers? I suspect there is a corner case.

It's set when we had to write to some debugreg[0..4], either for use by
the guest itself or for debugging it from the host. It used to be the
only condition for switching on exit as we saved the registers on entry
(under the same condition). This was reworked recently to avoid the
entry saving.

> 
> Because since I can't anymore assume TIF_DEBUG covers every
> breakpoints uses, it means I'll need to maintain a refcount of
> breakpoints in use.
> Well, I have one already, but it is splitted into several refcounts
> (per task events, per cpu, non-pinned, etc...). And since
> vcpu_enter_guest() is a fast path, I'll need to maintain another global
> per cpu one, without lock or further operations to know if we need
> to save the debug registers, just a simple check.
> 

I'm not 100% sure right now if we still need "switch_db_reg" in case we
have a reliable indicator that the host requires properly set registers.
ATM I would dare to say, we don't, but I need to think about this again.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]