Re: [PATCH v9 09/13] Adding info [tb-list|tb] commands to HMP (WIP)

["Alex Bennée" <alex.bennee@linaro.org>]

Richard Henderson <richard.henderson@linaro.org> writes:

> On 10/7/19 11:28 AM, Alex Bennée wrote:
>> From: "Vanderson M. do Rosario" <vandersonmr2@gmail.com>
>>
>> These commands allow the exploration of TBs generated by the TCG.
>> Understand which one hotter, with more guest/host instructions... and
>> examine their guest, host and IR code.
>>
>> The goal of this command is to allow the dynamic exploration of TCG
>> behavior and code quality. Therefore, for now, a corresponding QMP
>> command is not worthwhile.
>>
>> [AJB: WIP - we still can't be safely sure a translation will succeed]

I'll fix up all the other points...

>
>> +/*
>> + * We cannot always re-generate the code even if we know there are
>> + * valid translations still in the cache. The reason being the guest
>> + * may have un-mapped the page code.
>
> Um... unless I mistake what's being described here, that wouldn't be a valid
> translation.  Or do you just mean that the page mapping isn't present within
> the TLB?  Which is not quite the same thing as "unmapping".

A page entry can get dropped without invalidating the TB (for example
the guest OS sets a page as non-exec). The TLB flush takes care of the
jump cache and there are no inter-page hardwired links so if a tb_find
goes there we would get the fault.

>> + * TODO: can we do this safely? We need to
>> + *  a) somehow recover the mmu_idx for this translation
>
> We could add an interface for this, yes.  The value *must* be able to be
> derived from tb->flags, but of course in a target-dependent way.
>
>> + *  b) probe MMU_INST_FETCH to know it will succeed
>
> We *do* have this now, sort of: tlb_vaddr_to_host.
>
> So far all use of this function originates from target/foo/,
> and so some targets have not been updated to work with this.
> I've marked these with asserts within foo_cpu_tlb_fill.
>
> Notable targets for which it won't work: i386, sparc.
>
>
>> +static GString *get_code_string(TBStatistics *tbs, int log_flags)
>> +{
>> +    int old_log_flags = qemu_loglevel;
>> +
>> +    CPUState *cpu = first_cpu;
>> +    uint32_t cflags = curr_cflags() | CF_NOCACHE;
>> +    TranslationBlock *tb = NULL;
>> +
>> +    GString *code_s = g_string_new(NULL);
>> +    qemu_log_to_string(true, code_s);
>> +
>> +    qemu_set_log(log_flags);
>> +
>> +    if (sigsetjmp(cpu->jmp_env, 0) == 0) {
>> +        mmap_lock();
>> +        tb = tb_gen_code(cpu, tbs->pc, tbs->cs_base, tbs->flags, cflags);
>> +        tb_phys_invalidate(tb, -1);
>> +        mmap_unlock();
>
> Ew.  No.
>
> Let us not go through tb_gen_code, just to get logging output from the
> translator.  What are we really after here?  Input assembly?

All of it - in_asm, op, op_opt, out_asm potentially. But I agree it's
far too hacky - c.f. above because we end up potentially delivering a
fault to the guest when we fail.

It would be nice if we could run the translation phase independently of
the CPU environment. Maybe the monitor could have it's own TCGContext
and call tcg_gen_code directly just for debug output? It would avoid
having to use safe work and all the rest of the stuff we don't actually
care about in tb_gen_code. I guess you could still use the sigsetjmp to
catch the inevitable exceptions from the code load?

--
Alex Bennée