Re: [RFC PATCH v2 2/2] cputlb: implement load_helper_unaligned() for unaligned loads

[Mark Cave-Ayland]

On 10/06/2021 22:41, Richard Henderson wrote:

> On 6/9/21 7:10 AM, Philippe Mathieu-Daudé wrote:
> > +    oi = make_memop_idx(MO_UB, mmu_idx);
> > +    if (memop_big_endian(op)) {
> > +        for (i = 0; i < size; ++i) {
> > +            /* Big-endian load.  */
> > +            uint8_t val8 = helper_ret_ldub_mmu(env, addr + i, oi, retaddr);
> > +            val |= val8 << (((size - 1) * 8) - (i * 8));
> > +        }
> > +    } else {
> > +        for (i = 0; i < size; ++i) {
> > +            /* Little-endian load.  */
> > +            uint8_t val8 = helper_ret_ldub_mmu(env, addr + i, oi, retaddr);
> > +            val |= val8 << (i * 8);
> > +        }
> > +    }
>
> This doesn't quite work.  You can't just call helper_ret_ldub_mmu, as the other 
> option is full_ldub_code.  So, at present you've broken unaligned code loads.
>
> We also need noinline markup for clang, like we do for helper_ret_stb_mmu. I've no 
> proof of that, but it certainly makes sense to record how we expect the inline loop 
> to be resolved.
>
> Finally, you have to use uint64_t for val8, otherwise the shift fails for size 
> == 8.
>
> I'll fix these up and see how things go.

Ah that makes sense - I wasn't quite sure whether the previous full_load() would 
already be handled correctly by the code_read logic in load_helper_unaligned().

Sleeping on the problem, I do wonder if the role of load_helper() and store_helper() 
should be just to ensure that if an access crosses a page then both entries are in 
the softtlb before passing MMIO accesses down through the memory API with Andrew 
Jeffrey's proposed unaligned access patch (see 
https://gitlab.com/qemu-project/qemu/-/issues/360#note_597130838 for a summary of the 
IRC discussion).

However as I suspect this may cause some breakage then I would still be fine with 
load_helper_unaligned() in the meantime.


ATB,

Mark.