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

[David Gibson]

On Mon, Dec 10, 2018 at 10:52:18AM -0200, Fabiano Rosas wrote:
> 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.

Ok, great.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature