Re: [Qemu-devel] [PATCH] softmmu: fixing usage of cpu_st/ld* from helpers

[Richard Henderson]

On 09/15/2014 03:50 AM, Pavel Dovgalyuk wrote:
> +/* inline helper ld function */
> +
> +static inline DATA_TYPE
> +glue(glue(helper_inline_ld, SUFFIX), MEMSUFFIX)(CPUArchState *env,
> +                                                target_ulong addr,
> +                                                int mmu_idx)
> +{
> +    return glue(glue(helper_call_ld, SUFFIX), MMUSUFFIX)(env, addr, mmu_idx,
> +                                                         GETRA());
> +}

You'd have to mark this always_inline to make absolutely sure that the caller's
GETRA value is used.  That said...

> @@ -76,7 +87,8 @@ glue(glue(cpu_ld, USUFFIX), MEMSUFFIX)(CPUArchState *env, 
> target_ulong ptr)
>      mmu_idx = CPU_MMU_INDEX;
>      if (unlikely(env->tlb_table[mmu_idx][page_index].ADDR_READ !=
>                   (addr & (TARGET_PAGE_MASK | (DATA_SIZE - 1))))) {
> -        res = glue(glue(helper_ld, SUFFIX), MMUSUFFIX)(env, addr, mmu_idx);
> +        res = glue(glue(helper_inline_ld, SUFFIX), MEMSUFFIX)(env, addr,
> +                                                              mmu_idx);
>      } else {
>          uintptr_t hostaddr = addr + 
> env->tlb_table[mmu_idx][page_index].addend;
>          res = glue(glue(ld, USUFFIX), _raw)(hostaddr);

... this is also the wrong context.

The only GETRA value that helps you at all is the one from the *top level*
helper -- the one that's directly called from TCG code.  So, in the case of
maskmov, helper_maskmov_xmm.  Anything else and you aren't getting the call
site address from the TCG code, and so can't be used to detect the PC of the
MMU fault.

I guess there are only two real possibilities:

(1) Have the cpu_ldst_template helpers all be marked always_inline so that they
could use GETRA.  I'm not too fond of this because we'd still get the wrong
results if these are not used from top-level helpers.

(2) Add helpers that accept the GETRA value from the top-level helper.  And not
hidden within a macro or always_inline function.  This helps us see what
portions of the code have been audited for the new interface.  This will
involve quite a bit more code churn, but shouldn't been too difficult for any
single function.


r~