Re: [Qemu-trivial] [Qemu-devel] [PATCH] doc/memory.txt: fix typo

[Peter Maydell]

On 25 February 2016 at 13:30, Cao jin <address@hidden> wrote:
>
>
> On 02/25/2016 06:00 PM, Peter Maydell wrote:
>>
>> On 25 February 2016 at 09:32, Cao jin <address@hidden> wrote:
>
>
>>> diff --git a/docs/memory.txt b/docs/memory.txt
>>> index 8745f76..1a3ad622 100644
>>> --- a/docs/memory.txt
>>> +++ b/docs/memory.txt
>>> @@ -297,7 +297,7 @@ various constraints can be supplied to control how
>>> these callbacks are called:
>>>    - .valid.min_access_size, .valid.max_access_size define the access
>>> sizes
>>>      (in bytes) which the device accepts; accesses outside this range
>>> will
>>>      have device and bus specific behaviour (ignored, or machine check)
>>> - - .valid.aligned specifies that the device only accepts naturally
>>> aligned
>>> + - .valid.unaligned specifies that the device only accepts naturally
>>> aligned
>>>      accesses.  Unaligned accesses invoke device and bus specific
>>> behaviour.
>>
>>
>> This doesn't look like the right change, because (a) a field named
>> unaligned which you set true to specifiy that unaligned accesses
>> are invalid would be very confusing and (b) the comment in the
>> header file says that 'valid.unaligned' means that the device does
>> support unaligned accesses.
>>
>
> the reason that I think it is a typo is: from the pattern ".xxx.yyy", this
> section looks like a explanation(or detailed comment) of struct
> MemoryRegionOps`s fields, isn`t it? If yes, all the others match with the
> structure, except this /.valid.aligned/

You're certainly right that the documentation as it stands is wrong.
I don't think this is a simple typo though. Probably what happened
was that when the API was being designed it started off with an
'unaligned' field, and then later the field name and semantics
were changed but the docs weren't updated to match. My point was
that just changing the name without looking at the behaviour we're
actually implementing isn't correct.

> I admit that, the description like ".valid.unaligned specifies that the
> device only accepts naturally aligned accesses" looks very confusing. But
> from the only caller memory_region_access_valid(), I think the original
> maybe not quite good? how about this:

> /.valid.unaligned specifies that the device accepts unaligned accesses.  If
> false, Unaligned accesses invoke device and bus specific behaviour/

I would suggest:

  - .valid.unaligned specifies that the *device being modelled*
    supports unaligned accesses; if false, unaligned accesses will
    invoke the appropriate bus or CPU specific behaviour.

(the idea being to make the text parallel to the existing
.impl.unaligned docs so it's clear what the difference is.)

thanks
-- PMM