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

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

[Cao jin]

to match structure MemoryRegionOps definition

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
And there seems are some out of date content in this file, like
"cpu_register_io_memory()" mentioned in the last line, which is removed several
years ago in commit 97161e177. I am sure how to modify it, So I take the
liberty to CC the guys in the cc-list(get list from get_maintainer.pl).

 docs/memory.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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.
  - .impl.min_access_size, .impl.max_access_size define the access sizes
    (in bytes) supported by the *implementation*; other access sizes will be
-- 
2.1.0

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

[Peter Maydell]

On 25 February 2016 at 09:32, Cao jin <caoj.fnst@cn.fujitsu.com> wrote:
> to match structure MemoryRegionOps definition
>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
> And there seems are some out of date content in this file, like
> "cpu_register_io_memory()" mentioned in the last line, which is removed several
> years ago in commit 97161e177. I am sure how to modify it, So I take the
> liberty to CC the guys in the cc-list(get list from get_maintainer.pl).k

The part that says
# - .old_mmio can be used to ease porting from code using
#   cpu_register_io_memory(). It should not be used in new code.

could be changed to say "has been used to ease porting from code that
used to use cpu_register_io_memory()" if you like. The information
here is not wrong, really, just the tense.

(There are just 32 uses of old_mmio left in the codebase, which
hopefully we may eventually get rid of.)

>  docs/memory.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> 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.

>   - .impl.min_access_size, .impl.max_access_size define the access sizes
>     (in bytes) supported by the *implementation*; other access sizes will be

We also seem to be missing documentation of valid.accepts.

thanks
-- PMM

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

[Cao jin]

On 02/25/2016 06:00 PM, Peter Maydell wrote:
> On 25 February 2016 at 09:32, Cao jin <caoj.fnst@cn.fujitsu.com> wrote:
>> to match structure MemoryRegionOps definition
>>
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>> ---
>> And there seems are some out of date content in this file, like
>> "cpu_register_io_memory()" mentioned in the last line, which is removed several
>> years ago in commit 97161e177. I am sure how to modify it, So I take the
>> liberty to CC the guys in the cc-list(get list from get_maintainer.pl).k
>

Hi peter,
Thanks for your quick response.
Can`t believe I miss a "not" in my word..."I am not sure how to modify 
it" so I CC you guys:)


-- 
Yours Sincerely,

Cao jin

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

[Cao jin]

On 02/25/2016 06:00 PM, Peter Maydell wrote:
> On 25 February 2016 at 09:32, Cao jin <caoj.fnst@cn.fujitsu.com> 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/

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/


-- 
Yours Sincerely,

Cao jin

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

[Peter Maydell]

On 25 February 2016 at 13:30, Cao jin <caoj.fnst@cn.fujitsu.com> wrote:
>
>
> On 02/25/2016 06:00 PM, Peter Maydell wrote:
>>
>> On 25 February 2016 at 09:32, Cao jin <caoj.fnst@cn.fujitsu.com> 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

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

[Cao jin]

On 02/25/2016 10:24 PM, Peter Maydell wrote:
> On 25 February 2016 at 13:30, Cao jin <caoj.fnst@cn.fujitsu.com> wrote:

>
> 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.
>

Totally agree with the point.

>> 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.
>

Does better. I want to pack this into a trivial patch first, because the 
original is not easy to read & understand

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

-- 
Yours Sincerely,

Cao jin

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

[Paolo Bonzini]

On 25/02/2016 15:24, Peter Maydell wrote:
> 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.

Peter, are you okay with adding your S-o-b and authorship to
this patch?

I'll add a Reported-by for Cao jin, too.

Paolo

diff --git a/docs/memory.txt b/docs/memory.txt
index d0aca05..e44d081 100644
--- a/docs/memory.txt
+++ b/docs/memory.txt
@@ -297,8 +297,9 @@ 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
-   accesses.  Unaligned accesses invoke device and bus specific behaviour.
+ - .valid.unaligned specifies that the *device being modelled* supports
+    unaligned accesses; if false, unaligned accesses will invoke the
+    appropriate bus or CPU specific behaviour.
  - .impl.min_access_size, .impl.max_access_size define the access sizes
    (in bytes) supported by the *implementation*; other access sizes will be
    emulated using the ones available.  For example a 4-byte write will be
@@ -307,4 +308,4 @@ various constraints can be supplied to control how these callbacks are called:
    accesses; if false, unaligned accesses will be emulated by two aligned
    accesses.
  - .old_mmio can be used to ease porting from code using
-   cpu_register_io_memory(). It should not be used in new code.
+   cpu_register_io_memory() and the like. It should not be used in new code.

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

[Peter Maydell]

On 3 March 2016 at 14:45, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 25/02/2016 15:24, Peter Maydell wrote:
>> 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.
>
> Peter, are you okay with adding your S-o-b and authorship to
> this patch?

Yes.

thanks
-- PMM