Re: [PATCH 3/5] KVM: VMX: Ensure interruptibility when single-stepping

[Jan Kiszka <jan.kiszka@siemens.com>]

Avi Kivity wrote:
> Jan Kiszka wrote:
>> When single-stepping, we have to ensure that the INT1 can make it
>> through even if the guest itself is uninterruptible due to MOV SS or
>> STI. VMENTRY will fail otherwise.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>
>>  arch/x86/kvm/vmx.c |   10 ++++++++--
>>  1 files changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index 3a422dc..8e83102 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -1010,6 +1010,7 @@ static void vmx_cache_reg(struct kvm_vcpu *vcpu,
>> enum kvm_reg reg)
>>  static int set_guest_debug(struct kvm_vcpu *vcpu, struct
>> kvm_guest_debug *dbg)
>>  {
>>      int old_debug = vcpu->guest_debug;
>> +    u32 interruptibility;
>>      unsigned long flags;
>>  
>>      vcpu->guest_debug = dbg->control;
>> @@ -1017,9 +1018,14 @@ static int set_guest_debug(struct kvm_vcpu
>> *vcpu, struct kvm_guest_debug *dbg)
>>          vcpu->guest_debug = 0;
>>  
>>      flags = vmcs_readl(GUEST_RFLAGS);
>> -    if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)
>> +    if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
>>          flags |= X86_EFLAGS_TF | X86_EFLAGS_RF;
>> -    else if (old_debug & KVM_GUESTDBG_SINGLESTEP)
>> +        /* We must be interruptible when single-stepping */
>> +        interruptibility = vmcs_read32(GUEST_INTERRUPTIBILITY_INFO);
>> +        if (interruptibility & 3)
>> +            vmcs_write32(GUEST_INTERRUPTIBILITY_INFO,
>> +                     interruptibility & ~3);
>>   
> 
> Could just write unconditionally - it's not like the write has any
> effect on speed.  vmcs_clear_bits() will do it cleanly.

OK.

> 
> But I'm worried about correctness.  Suppose we're singlestepping a sti;
> hlt sequence.  While we'll clear interruptibility, probably receive the
> debug trap (since that's a high priority exception), but then inject the
> interrupt before the hlt, hanging the guest.  So we probably need to
> restore interruptibility on exit.
> 
> This looks like a good candidate for a test case.

You're concern might be valid. Will dig into the test infrastructure.
Maybe it is a good idea to have something like the kgdb testsuite as
part of the kvm tests to check debugger regressions, not just this
special case.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 26
Corporate Competence Center Embedded Linux