Re: [Qemu-devel] [PATCH 18/20] tcg: Save insn data and use it in cpu_restore_state_from_tb

[Richard Henderson]

On 09/10/2015 06:49 AM, Peter Maydell wrote:
>> +    tcg_debug_assert(num_insns >= 0);
> 
> This is claiming that every TB will have at least one insn_start,
> right? I think that most targets will violate that in the breakpoint
> case, because the "if we have a bp for this insn then generate a
> debug insn and break out of the loop" code is before the call
> to tcg_gen_insn_start().
> 
> We should probably assert that num_insns < TCG_MAX_INSNS while
> we're here.

True.  I wonder if we shouldn't fix bp placement while I'm at it.  And the
assertion should really be num_insns == tb->icount.

>> +static target_long decode_sleb128(uint8_t **pp)
>> +{
>> +    uint8_t *p = *pp;
>> +    target_long val = 0;
>> +    int byte, shift = 0;
>> +
>> +    do {
>> +        byte = *p++;
>> +        val |= (target_ulong)(byte & 0x7f) << shift;
>> +        shift += 7;
>> +    } while (byte & 0x80);
>> +    if (shift < TARGET_LONG_BITS && (byte & 0x40)) {
>> +        val |= -(target_ulong)1 << shift;
>> +    }
>> +
>> +    *pp = p;
>> +    return val;
>> +}
> 
> Are the encode/decode sleb128 functions known-good ones
> borrowed from somewhere else?

Yes, from libgcc.

> (PS: checkpatch complains about missing braces.)

Ho hum...

>> +static int encode_search(TranslationBlock *tb, uint8_t *block)
>> +{
> 
> I think this function would benefit from a brief comment
> describing the compressed format we're creating here.

Yes.

>>      gen_code_size = tcg_gen_code(&tcg_ctx, gen_code_buf);
>> +    search_size = encode_search(tb, (void *)gen_code_buf + gen_code_size);
> 
> Now we're putting the encoded search info in the codegen buffer,
> don't we need to adjust the calculation of code_gen_buffer_max_size
> to avoid falling off the end if the last TB in the buffer has a very
> large set of generated TCG code and also a big encoded search buffer?

Dunno.  It's not that we've ever checked for this before; I'm not sure what
factor I would actually apply.

> It would also be nice to assert if we do fall off the end of the
> buffer somehow.

Given that we generally use a very large mmap to allocate it, perhaps simply
adding a guard page would be best.

> How much extra space does the encoded search typically take (as a
> % of the gen_code_size, say)?

Dunno; I'll have to have a look at that.  Probably easiest to just enhance info
jit...


r~