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.
