Re: [Qemu-devel] [PATCH v2 1/3] edu: mmio: set 'max_access_size' to 8

[Philippe Mathieu-Daudé]

On 4/22/19 3:17 AM, Li Qiang wrote:
> 
> 
> Philippe Mathieu-Daudé <address@hidden <mailto:address@hidden>> 于
> 2019年4月21日周日 下午6:28写道：
> 
>     Hi Li,
> 
>     The patch title is not very descriptive, maybe "allow 64-bit access"
> 
> 
>     On 4/20/19 6:14 PM, Li Qiang wrote:
>     > The edu spec said, the MMIO area can be accessed by 8 bytes.
> 
>     or 64-bit...
> 
>     > However currently the 'max_access_size' is not so the MMIO
>     > access dispatch can only access 4 bytes one time. This patch
> 
>     32-bit
> 
>     > fixes this to respect the spec.
>     >
>     > Notice: here the 'min_access_size' is not a must, I set this
>     > for completement.
> 
>     Which one? valid/impl? I think you can drop this comment from the commit
>     description.
> 
> 
> Both needed. from memory_access_size, if we has no valid.max_access_size,
> this function will set it to 4.
> 
> static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr)
> {
>     unsigned access_size_max = mr->ops->valid.max_access_size;
> 
>     /* Regions are assumed to support 1-4 byte accesses unless
>        otherwise specified.  */
>     if (access_size_max == 0) {
>         access_size_max = 4;
>     }
>    ...
> } 
> 
> From access_with_adjusted_size, if we has no impl.max_access_size,
> this function will set it to 4.
> 
> ps: I will appreciate if anyone can explain what's the meaning of valid
> and impl's min/max_access_size
> and how it affects the behavior.

"valid" describes the valid access from the bus to the device.

Indeed in the EDU case those are 4 and 8.

"impl" describes the accesses implemented by the QEMU device model.
The developper who writes the device is free to choose the accesses he
will model.

If valid/impl accesses don't match, the function
access_with_adjusted_size() from memory.c will adjust the bus access to
the device implementation.

For example, if the device only implements 1 and 2 bytes accesses, with
a 1-4 valid access, if the CPU executes a 32-bit access, this function
will do 2x 16-bit access to the device (incrementing the address by 2)
and returns a 32-bit result.

Similarly, if the CPU does a 8-bit access on a 32-bit impl device,
access_with_adjusted_size() will execute a single 32-bit access to the
device, then mask/shift the returned value and returns a 8-bit result to
the caller.

> Thanks,
> Li Qiang
> 
>     >
>     > Signed-off-by: Li Qiang <address@hidden <mailto:address@hidden>>
>     > ---
>     >  hw/misc/edu.c | 9 +++++++++
>     >  1 file changed, 9 insertions(+)
>     >
>     > diff --git a/hw/misc/edu.c b/hw/misc/edu.c
>     > index 91af452c9e..65fc32b928 100644
>     > --- a/hw/misc/edu.c
>     > +++ b/hw/misc/edu.c
>     > @@ -289,6 +289,15 @@ static const MemoryRegionOps edu_mmio_ops = {
>     >      .read = edu_mmio_read,
>     >      .write = edu_mmio_write,
>     >      .endianness = DEVICE_NATIVE_ENDIAN,
>     > +    .valid = {
>     > +        .min_access_size = 4,
> 
>     Per the spec, this is correct.
> 
>     > +        .max_access_size = 8,
> 
>     Correct.
> 
>     > +    },
>     > +    .impl = {
>     > +        .min_access_size = 4,
> 
>     OK.
> 
>     > +        .max_access_size = 8,
> 
>     Correct.
> 
>     > +    },
>     > +
>     >  };
>     > 
>     >  /*
>     >
> 
>     With title/description updated:
>     Reviewed-by: Philippe Mathieu-Daudé <address@hidden
>     <mailto:address@hidden>>
>