Re: [Qemu-devel] [PATCH 4/5] tcg: reorder removal from lists in tb_phys_invalidate

[Paolo Bonzini <pbonzini@redhat.com>]

On 14/04/2016 16:45, Sergey Fedorov wrote:
> So what would you suggest to use for x86? I can't think of something
> that looks like a really compelling combination when I look at
> cpu_get_tb_cpu_state() in target-i386/cpu.h.

On x86 I think we should define HF_INVALID_TB to an invalid flag
combination. I can think of several solutions:

- #defining HF_INVALID_TB to an invalid combination (e.g. HF_CS64_MASK,
because it always appears together with HF_LMA_MASK; see code that
updates those hflags in cpu_x86_load_seg_cache, cpu_x86_update_cr0,
kvm_get_sregs). Advantage: doesn't waste a bit, reasonably self
documenting. Disadvantage: a bit tricky, but still my favorite.

- rename HF_SOFTMMU_MASK to HF_INVALID_MASK (it's always the same as
CONFIG_SOFTMMU so we can remove it), then #define HF_INVALID_TB
HF_INVALID_MASK. Advantage: obviously correct. Disadvantage: wastes a
bit. My second favorite.

- #defining HF_INVALID_TB to -1. Advantage: ?!? Disadvantage:
everything. Looks tame, actually a huge hack

- #defining HF_INVALID_TB to the "wrong" direction HF_SOFTMMU_MASK (i.e.
to 0 if CONFIG_SOFTMMU, . Advantage: obviously correct. Disadvantage:
huge hack, HF_SOFTMMU_MASK is unused anyway.

Choose your own favorite. :)

(Setting cs_base to -1 actually would work on 64-bit x86, but not on
qemu-system-i386).

> Personally, I'm not so
> happy trying to use pc/cs_base/flags to mark an invalid TB. Are my
> worries unreasonable? :)

Can you explain your worries?

The advantages are that it's O(1) and it obviously doesn't affect other
TBs than the invalidated one.

> Anyway, I am wondering if there is still a way to clear tb_phys_hash and
> tb_jmp_cache safely.
>
> Maybe something like this:
>  * Remove the TB from physical hash list

So at this point tb_find_slow cannot find it.

>  * Memory barrier
>  * Remove the TB from each vCPU's virtual address hash cache

tb_find_fast then cannot find it either.

> Would that work?

This is very similar to the current code.  From 10,000 feet, because
tb_find_fast calls tb_find_slow, this could indeed work, but I'm a bit
concerned about how to order the removal of the jump lists.  The usage
of "tcg_ctx.tb_ctx.tb_invalidated_flag = 1" in the existing code was
what worries me.  Indeed the motivation of this patch was removing that
single line of code to prepare for the move of tb_invalidated_flag to
CPUState.

Also, this loop will not be thread-safe anymore as soon as Fred's
"tb_jmp_cache lookup outside tb_lock" goes in:

    CPU_FOREACH(cpu) {
        if (cpu->tb_jmp_cache[h] == tb) {
            cpu->tb_jmp_cache[h] = NULL;
        }
    }

It should use atomic_cmpxchg (slow!) or to unconditionally NULL out
cpu->tb_jmp_cache (a bit hacky).  Preparing for that change is an added
bonus of the tb-hacking approach.

Paolo