Re: [Qemu-devel] [PATCH v3 2/2] target-mips: Misaligned memory accesses for MSA

[Richard Henderson <rth@twiddle.net>]

On 05/13/2015 08:37 AM, Yongbok Kim wrote:
> +static inline void ensure_atomic_msa_block_access(CPUMIPSState *env,
> +                                                  target_ulong addr,
> +                                                  int rw,
> +                                                  int mmu_idx)
>  {
> +#if !defined(CONFIG_USER_ONLY)
> +#define MSA_PAGESPAN(x) (unlikely((((x) & ~TARGET_PAGE_MASK)                \
> +                                   + MSA_WRLEN/8 - 1) >= TARGET_PAGE_SIZE))
> +    CPUState *cs = CPU(mips_env_get_cpu(env));
> +    target_ulong page_addr;
>  
> +    if (MSA_PAGESPAN(addr)) {
> +        /* first page */
> +        tlb_fill(cs, addr, rw, mmu_idx, 0);
> +        /* second page */
> +        page_addr = (addr & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE;
> +        tlb_fill(cs, page_addr, rw, mmu_idx, 0);
>      }
> +#endif
>  }

This doesn't do quite what you think it does.  It does trap if the page isn't
mapped at all, but it doesn't trap if e.g. rw is set and the page is read-only.
That requires a subsequent check for what permissions were installed by
tlb_set_page.

I had thought there was a way to look this up besides duplicating the code in
softmmu_template.h, but I suppose that's in a patch set that never made it in.


> +    if (unlikely(addr & ((1 << DF) - 1))) {                             \
> +        /* work-around for misaligned accesses */                       \
> +        for (i = 0; i < DF_ELEMENTS(DF_BYTE); i++) {                    \
> +            pwd->b[i] = do_lbu(env, addr + (i << DF_BYTE), mmu_idx);    \
> +        }                                                               \
> +        correct_vector_endianness_ ## TYPE(pwd, pwd);                   \

Why byte accesses?  The softmmu helpers are guaranteed to support misalignment.

As an aside, consider moving away from

#define HELPER_LD(name, insn, type)                                     \
static inline type do_##name(CPUMIPSState *env, target_ulong addr,      \
                             int mem_idx)                               \
{                                                                       \
    switch (mem_idx)                                                    \
    {                                                                   \
    case 0: return (type) cpu_##insn##_kernel(env, addr); break;        \
    case 1: return (type) cpu_##insn##_super(env, addr); break;         \
    default:                                                            \
    case 2: return (type) cpu_##insn##_user(env, addr); break;          \
    }                                                                   \
}

to using helper_ret_*_mmu directly.  Which allows you to specify the mmu_idx
directly rather than bouncing around different thunks.  It also allows you to
pass in GETRA(), which would allow these helpers to use cpu_restore_state on
faults.


r~