Re: [Qemu-devel] [RFC PATCH v2 3/3] target/ppc: support single stepping with KVM HV

[Fabiano Rosas]

David Gibson <address@hidden> writes:

>> >> +    if (arch_info->address == trace_handler_addr) {
>> >> +        cpu_synchronize_state(cs);
>> >> +        kvm_remove_breakpoint(cs, trace_handler_addr, 4, 
>> >> GDB_BREAKPOINT_SW);
>> >> +
>> >> +        cpu_memory_rw_debug(cs, env->spr[SPR_SRR0] - 4, (uint8_t *)&insn,
>> >> +                            sizeof(insn), 0);
>> >> +
>> >> +        /* If the last instruction was a mfmsr, make sure that the
>> >> +         * MSR_SE bit is not set to avoid the guest kernel knowing
>> >> +         * that it is being single-stepped */
>> >> +        if (extract32(insn, 26, 6) == 31 && extract32(insn, 1, 10) == 
>> >> 83) {
>> >> +            reg = extract32(insn, 21, 5);
>> >> +            env->gpr[reg] &= ~(1ULL << MSR_SE);
>> >> +        }
>> >
>> > Hm.  What happens if both qemu and the guest itself attempt to single
>> > step at the same time?  How do you distinguish between the cases?
>> 
>> There is currently no distinction being made.
>
> This seems incorrect to me.  Basically you're restoring !MSR_SE
> unconditionaly when you finish the hypervisor side step, which might
> not be correct if the guest is also attempting to single step itself.
> AFAICT it should be possible to track what the guest thinks is the
> value of MSR_SE and restore that.

I was skeptical of being able to do both single steps at the same time
but I found a way to reproduce it by stepping over an rfid when
SRR1_SE is already 1.

>  If both hypervisor and guest
> attempt to single step, I'd expect to have the hypervisor trap first,
> then return to the guest's single step vector.

With the fix you suggest above, QEMU will be able to single step the
interrupt handler during the guest's single step. That means I'll have
to restore the SRRs as well so that the handler returns to the correct
place.

>> I could do the latter, if you prefer.
>
> I think that's better - I don't think it's safe to assume that you're
> *not* synchronized with KVM.

Ok, that's better indeed.

Thanks for the comments, I'll prepare another version with the
appropriate corrections.