[v2,1/3] edu: mmio: set 'max_access_size' to 8

Commit Message

Li Qiang April 20, 2019, 4:14 p.m. UTC

The edu spec said, the MMIO area can be accessed by 8 bytes.
However currently the 'max_access_size' is not so the MMIO
access dispatch can only access 4 bytes one time. This patch
fixes this to respect the spec.

Notice: here the 'min_access_size' is not a must, I set this
for completement.

Signed-off-by: Li Qiang <[e-mail]>
---
 hw/misc/edu.c | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Philippe Mathieu-Daudé April 21, 2019, 10:28 a.m. UTC | #1

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.

> 
> Signed-off-by: Li Qiang <[e-mail]>
> ---
>  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é <[e-mail]>

Li Qiang April 22, 2019, 1:17 a.m. UTC | #2

Philippe Mathieu-Daudé <[e-mail]> 于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.

Thanks,
Li Qiang

>
> > Signed-off-by: Li Qiang <[e-mail]>
> > ---
> >  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é <[e-mail]>
>
>

Philippe Mathieu-Daudé April 22, 2019, 4:27 p.m. UTC | #3

On 4/22/19 3:17 AM, Li Qiang wrote:
> 
> 
> Philippe Mathieu-Daudé <[e-mail] <mailto:[e-mail]>> 于
> 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 <[e-mail] <mailto:[e-mail]>>
>     > ---
>     >  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é <[e-mail]
>     <mailto:[e-mail]>>
>

Li Qiang April 23, 2019, 3:50 a.m. UTC | #4

Philippe Mathieu-Daudé <[e-mail]> 于2019年4月23日周二 上午12:28写道：

> On 4/22/19 3:17 AM, Li Qiang wrote:
> >
> >
> > Philippe Mathieu-Daudé <[e-mail] <mailto:[e-mail]>> 于
> > 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  Philippe,

I think I get the pointer.

Thanks,
Li Qiang



> > Thanks,
> > Li Qiang
> >
> >     >
> >     > Signed-off-by: Li Qiang <[e-mail] <mailto:[e-mail]>>
> >     > ---
> >     >  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é <[e-mail]
> >     <mailto:[e-mail]>>
> >
>

Patch

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,
+        .max_access_size = 8,
+    },
+    .impl = {
+        .min_access_size = 4,
+        .max_access_size = 8,
+    },
+
 };
 
 /*