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

[Cédric Le Goater]

Andrew,

On 07/14/2017 08:20 AM, Andrew Jeffery wrote:
> Hi Paolo,
> 
> Thanks for taking a look!
> 
> On Thu, 2017-07-13 at 14:05 +0200, Paolo Bonzini wrote:
>> 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.
> 
> Okay.
> 
>>
>>> +            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?
> 
> I haven't looked at the patch since I sent it. Given you think the idea
> of the patch is okay I'll get back to working on it and think about
> this.

We have been using successfully this patch for over a year now and it 
would be nice to get it merged along with the ADC model. Do you have 
a new version addressing Paolo's remarks ?

Thanks,

C.