Re: [PATCH v6 3/3] target/ppc: support single stepping with KVM HV

[Fabiano Rosas <farosas@linux.ibm.com>]

David Gibson <david@gibson.dropbear.id.au> writes:

>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
>> index 103bfe9dc2..b69f8565aa 100644
>> --- a/target/ppc/cpu.h
>> +++ b/target/ppc/cpu.h
>> @@ -440,6 +440,10 @@ typedef struct ppc_v3_pate_t {
>>  #define msr_ts   ((env->msr >> MSR_TS1)  & 3)
>>  #define msr_tm   ((env->msr >> MSR_TM)   & 1)
>>  
>> +#define srr1_ir ((env->spr[SPR_SRR1] >> MSR_IR) & 1)
>> +#define srr1_dr ((env->spr[SPR_SRR1] >> MSR_DR) & 1)
>> +#define srr1_se ((env->spr[SPR_SRR1] >> MSR_SE) & 1)
>
> I'd prefer not to introduce these.  The msr_xx macros are already kind
> of dubious because they assume the meaning of 'env' in the context
> they're used.  I'm ok to use them because they're so useful, so
> often.  These srr1 variants however are used in far fewer situations.
> So, I'd prefer to just open code these as (env->spr[SPR_SRR1] &
> MSR_IR) in the relatively few places they're used for now.
>

Ok. No problem.

>>  #define DBCR0_ICMP (1 << 27)
>>  #define DBCR0_BRT (1 << 26)
>>  #define DBSR_ICMP (1 << 27)
>> @@ -1158,6 +1162,16 @@ struct CPUPPCState {
>>      uint32_t tm_vscr;
>>      uint64_t tm_dscr;
>>      uint64_t tm_tar;
>> +
>> +    /* Used for software single step */
>> +    target_ulong sstep_srr0;
>> +    target_ulong sstep_srr1;
>> +    target_ulong sstep_insn;
>> +    target_ulong trace_handler_addr;
>> +    int sstep_kind;
>
> Won't you need to migrate this extra state, at least some of the time?
>

Ah. I haven't looked into this yet. Will do that for the next
version. Need to learn a bit about migration first.

>> +#define SSTEP_REGULAR 0
>> +#define SSTEP_PENDING 1
>> +#define SSTEP_GUEST   2
>
> Some comments on what these modes mean would be useful.
>

Ok.

>> +static uint32_t ppc_gdb_read_insn(CPUState *cs, target_ulong addr)
>> +{
>> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
>> +    CPUPPCState *env = &cpu->env;
>> +    uint32_t insn;
>> +
>> +    cpu_memory_rw_debug(cs, addr, (uint8_t *)&insn, sizeof(insn), 0);
>> +
>> +    if (msr_le) {
>> +        return ldl_le_p(&insn);
>> +    } else {
>> +        return ldl_be_p(&insn);
>> +    }
>
> I think you can just use cpu_ldl_code() for this.
>

I'll look into it.

>> +static void kvm_insert_singlestep_breakpoint(CPUState *cs, bool mmu_on)
>> +{
>> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
>> +    CPUPPCState *env = &cpu->env;
>> +    target_ulong bp_addr;
>> +    target_ulong saved_msr = env->msr;
>> +
>> +    bp_addr = ppc_get_trace_int_handler_addr(cs, mmu_on);
>> +    if (env->nip == bp_addr) {
>> +        /*
>> +         * We are trying to step the interrupt handler address itself;
>> +         * move the breakpoint to the next instruction.
>> +         */
>> +        bp_addr += 4;
>
> What if the first instruction of the interrupt handler is a branch?
>

Well, I need to displace the breakpoint somehow. I don't think I can
handle this particular case without having _some_ knowledge of the
handler's code. The ones I've seen so far don't have a branch as first
instruction.

>> +    }
>> +
>> +    /*
>> +     * The actual access by the guest might be made in a mode
>> +     * different than we are now (due to rfid) so temporarily set the
>> +     * MSR to reflect that during the breakpoint placement.
>> +     *
>> +     * Note: we need this because breakpoints currently use
>> +     * cpu_memory_rw_debug, which does the memory accesses from the
>> +     * guest point of view.
>> +     */
>> +    if ((msr_ir & msr_dr) != mmu_on) {
>
> Should be msr_ir && msr_dr - you only get away with bitwise and by
> accident.
>

Ack.

>> +        if (mmu_on) {
>> +            env->msr |= (1ULL << MSR_IR | 1ULL << MSR_DR);
>> +        } else {
>> +            env->msr &= ~(1ULL << MSR_IR | 1ULL << MSR_DR);
>> +        }
>> +    }
>
> Wouldn't it be simpler to unconditionally set env->msr based on
> mmu_on.
>

Yes.

>> +
>> +    kvm_insert_breakpoint(cs, bp_addr, 4, GDB_BREAKPOINT_SW);
>
> Hrm.... I don't actually see how changing env->msr helps you here.
> AFAICT if kvm_insert_breakpoint() resolves to kvm_arch_sw_breakpoint()
> it doesn't rely on the MSR value at all.  If it resolves to
> kvm_arch_hw_breakpoint(), then it looks like it just stashes
> information to be pushed into KVM when we re-enter the guest.  None of
> the information stashed appears to depend on the current MSR, and once
> we re-enter the MSR will already have been restored.
>

This is the call chain:

kvm_arch_insert_sw_breakpoint -> cpu_memory_rw_debug ->
cpu_get_phys_page_attrs_debug -> ppc_cpu_get_phys_page_debug ->
ppc64_v3_get_phys_page_debug -> ppc_radix64_get_phys_page_debug:

    /* Handle Real Mode */
    if (msr_dr == 0) {
        /* In real mode top 4 effective addr bits (mostly) ignored */
        return eaddr & 0x0FFFFFFFFFFFFFFFULL;
    }


Actually, I think there is a bug after ppc_cpu_get_phys_page_debug
somewhere. There are some cases where GDB wants to read/write to some
memory, but it gets denied access. Presumably because of one such
discrepancy as the one above. I need to spend more time looking at this
to define the problem properly, though.

>> +    /*
>> +     * MSR_SE = 1 will cause a Trace Interrupt in the guest after the
>> +     * next instruction executes. If this is a rfid, use SRR1 instead
>> +     * of MSR.
>> +     */
>> +    if (rfid) {
>> +        if ((env->spr[SPR_SRR1] >> MSR_SE) & 1) {
>> +            /*
>> +             * The guest is doing a single step itself. Make sure we
>> +             * restore it later.
>> +             */
>> +            env->sstep_kind = SSTEP_GUEST;
>> +        }
>> +
>> +        env->spr[SPR_SRR1] |= (1ULL << MSR_SE);
>> +        mmu_on = srr1_ir & srr1_dr;
>
> s/&/&&/
>

Ack.

>> +    } else {
>> +        env->msr |= (1ULL << MSR_SE);
>> +        mmu_on = msr_ir & msr_dr;
>
> s/&/&&/
>

Ack.

> Also, what happens if the guest is using MSR[DR] != MSR[IR]?  It's
> rare, but it is occasionally useful.
>

I understand from the ISA that for the purposes of AIL, both bits need
to be set. So mmu_on = 0 is correct here.

>> +    }
>> +
>> +    kvm_insert_singlestep_breakpoint(cs, mmu_on);
>> +}
>> +
>> +void kvm_singlestep_ail_change(CPUState *cs)
>> +{
>> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
>> +    CPUPPCState *env = &cpu->env;
>> +
>> +    if (kvm_arch_can_singlestep(cs)) {
>> +        return;
>> +    }
>> +
>> +    /*
>> +     * The instruction being stepped altered the interrupt vectors
>> +     * location (AIL). Re-insert the single step breakpoint at the new
>> +     * location
>> +     */
>> +    kvm_remove_breakpoint(cs, env->trace_handler_addr, 4, GDB_BREAKPOINT_SW);
>> +    kvm_insert_singlestep_breakpoint(cs, (msr_ir & msr_dr));
>
> s/&/&&/
>

Ack.

>> +}
>> +
>>  void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
>>  {
>>      int n;
>> @@ -1585,6 +1781,98 @@ void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
>>      }
>>  }
>>  
>> +/* Revert any side-effects caused during single step */
>> +static void restore_singlestep_env(CPUState *cs)
>> +{
>> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
>> +    CPUPPCState *env = &cpu->env;
>> +    uint32_t insn = env->sstep_insn;
>> +    int reg;
>> +    int spr;
>> +
>> +    env->spr[SPR_SRR0] = env->sstep_srr0;
>> +    env->spr[SPR_SRR1] = env->sstep_srr1;
>> +
>> +    if (ppc_gdb_get_op(insn) != OP_MOV) {
>> +        return;
>> +    }
>> +
>> +    reg = ppc_gdb_get_rt(insn);
>> +
>> +    switch (ppc_gdb_get_xop(insn)) {
>> +    case XOP_MTSPR:
>> +        /*
>> +         * mtspr: the guest altered the SRR, so do not use the
>> +         *        pre-step value.
>> +         */
>> +        spr = ppc_gdb_get_spr(insn);
>> +        if (spr == SPR_SRR0 || spr == SPR_SRR1) {
>> +            env->spr[spr] = env->gpr[reg];
>> +        }
>> +        break;
>> +    case XOP_MFMSR:
>> +        /*
>> +         * mfmsr: clear MSR_SE bit to avoid the guest knowing
>> +         *         that it is being single-stepped.
>> +         */
>> +        env->gpr[reg] &= ~(1ULL << MSR_SE);
>
> Don't you need to check for the case where the guest also thinks it is
> single stepping here?
>

Hm. I had this in some previous version but removed it for some
reason. I'll review it.


Thanks