David Gibson <da...@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
