Re: [Qemu-devel] [RFC PATCH 1/1] memory: Support unaligned accesses on aligned-only models

[Paolo Bonzini]

On 30/06/2017 05:00, Andrew Jeffery wrote:
> This RFC patch stems from a discussion on a patch for an ADC model[1] where it
> was pointed out that I should be able to use the .impl member of
> MemoryRegionOps to constrain how my read() and write() callbacks where 
> invoked.
> 
> I tried Phil's suggested approach and found I got reads of size 4, but with an
> address that was not 4-byte aligned.
> 
> Looking at the source for access_with_adjusted_size() lead to the comment
> 
>      /* FIXME: support unaligned access? */
> 
> which at least suggests that the implementation isn't complete.
> 
> So, this patch is a quick and incomplete attempt at resolving the issue to see
> whether I'm on the right track or way off in the weeds.
> 
> I've lightly tested it with the ADC model mentioned above, and it appears to 
> do
> the right thing (I changed the values generated by the ADC to distinguish
> between the lower and upper 16 bits).

I think the idea is okay.

> +    access_addr[0] = align_down(addr, access_size);
> +    access_addr[1] = align_up(addr + size, access_size);
> +
> +        for (cur = access_addr[0]; cur < access_addr[1]; cur += access_size) 
> {
> +            uint64_t mask_bounds[2];
> +            mask_bounds[0] = MAX(addr, cur) - cur;
> +            mask_bounds[1] =
> +                MIN(addr + size, align_up(cur + 1, access_size)) - cur;
> +
> +            access_mask = (-1ULL << mask_bounds[0] * 8) &
> +                (-1ULL >> (64 - mask_bounds[1] * 8));

Please use MAKE_64BIT_MASK.

> +            r |= access(mr, cur, &access_value, access_size,
> +                  (MAX(addr, cur) - addr), access_mask, attrs);
> +
> +            /* XXX: Can't do this hack for writes */
> +            access_value >>= mask_bounds[0] * 8;
> +        }

Can you subtract access_addr[0] from mask_bounds[0] and mask_bounds[1]
(instead of cur) to remove the need for this right shift?

Thanks,

Paolo