[Qemu-devel] [PATCH v2 0/3] hw: edu: some fixes

[Qemu-devel] [PATCH v2 0/3] hw: edu: some fixes

[Li Qiang]

Recently I am considering write a driver for edu device.
After reading the spec, I found these three small issue.
Two first two related the MMIO access and the third is
related the DMA operation.

Change since v1:
Fix format compile error on Windows

Li Qiang (3):
  edu: mmio: set 'max_access_size' to 8
  edu: mmio: allow mmio read dispatch accept 8 bytes
  edu: uses uint64_t in dma operation

 hw/misc/edu.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH v2 0/3] hw: edu: some fixes

[Li Qiang]

Recently I am considering write a driver for edu device.
After reading the spec, I found these three small issue.
Two first two related the MMIO access and the third is
related the DMA operation.

Change since v1:
Fix format compile error on Windows

Li Qiang (3):
  edu: mmio: set 'max_access_size' to 8
  edu: mmio: allow mmio read dispatch accept 8 bytes
  edu: uses uint64_t in dma operation

 hw/misc/edu.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

-- 
2.17.1

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

[Li Qiang]

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

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

[Li Qiang]

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

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

[Philippe Mathieu-Daudé]

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 <liq3ea@163.com>
> ---
>  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é <philmd@redhat.com>

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

[Philippe Mathieu-Daudé]

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 <liq3ea@163.com>
> ---
>  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é <philmd@redhat.com>

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

[Li Qiang]

Philippe Mathieu-Daudé <philmd@redhat.com> 于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 <liq3ea@163.com>
> > ---
> >  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é <philmd@redhat.com>
>
>

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

[Li Qiang]

Philippe Mathieu-Daudé <philmd@redhat.com> 于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 <liq3ea@163.com>
> > ---
> >  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é <philmd@redhat.com>
>
>

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é <philmd@redhat.com <mailto:philmd@redhat.com>> 于
> 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 <liq3ea@163.com <mailto:liq3ea@163.com>>
>     > ---
>     >  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é <philmd@redhat.com
>     <mailto:philmd@redhat.com>>
> 

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é <philmd@redhat.com <mailto:philmd@redhat.com>> 于
> 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 <liq3ea@163.com <mailto:liq3ea@163.com>>
>     > ---
>     >  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é <philmd@redhat.com
>     <mailto:philmd@redhat.com>>
> 

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

[Li Qiang]

Philippe Mathieu-Daudé <philmd@redhat.com> 于2019年4月23日周二 上午12:28写道：

> On 4/22/19 3:17 AM, Li Qiang wrote:
> >
> >
> > Philippe Mathieu-Daudé <philmd@redhat.com <mailto:philmd@redhat.com>> 于
> > 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 <liq3ea@163.com <mailto:liq3ea@163.com>>
> >     > ---
> >     >  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é <philmd@redhat.com
> >     <mailto:philmd@redhat.com>>
> >
>

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

[Li Qiang]

Philippe Mathieu-Daudé <philmd@redhat.com> 于2019年4月23日周二 上午12:28写道：

> On 4/22/19 3:17 AM, Li Qiang wrote:
> >
> >
> > Philippe Mathieu-Daudé <philmd@redhat.com <mailto:philmd@redhat.com>> 于
> > 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 <liq3ea@163.com <mailto:liq3ea@163.com>>
> >     > ---
> >     >  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é <philmd@redhat.com
> >     <mailto:philmd@redhat.com>>
> >
>

[Qemu-devel] [PATCH v2 2/3] edu: mmio: allow mmio read dispatch accept 8 bytes

[Li Qiang]

The edu spec said when address >= 0x80, the MMIO area can
be accessed by 8 bytes.

Signed-off-by: Li Qiang <liq3ea@163.com>
---
 hw/misc/edu.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/misc/edu.c b/hw/misc/edu.c
index 65fc32b928..4018dddcb8 100644
--- a/hw/misc/edu.c
+++ b/hw/misc/edu.c
@@ -189,6 +189,10 @@ static uint64_t edu_mmio_read(void *opaque, hwaddr addr, unsigned size)
         return val;
     }
 
+    if (addr >= 0x80 && size != 4 && size != 8) {
+        return val;
+    }
+
     switch (addr) {
     case 0x00:
         val = 0x010000edu;
-- 
2.17.1

[Qemu-devel] [PATCH v2 2/3] edu: mmio: allow mmio read dispatch accept 8 bytes

[Li Qiang]

The edu spec said when address >= 0x80, the MMIO area can
be accessed by 8 bytes.

Signed-off-by: Li Qiang <liq3ea@163.com>
---
 hw/misc/edu.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/misc/edu.c b/hw/misc/edu.c
index 65fc32b928..4018dddcb8 100644
--- a/hw/misc/edu.c
+++ b/hw/misc/edu.c
@@ -189,6 +189,10 @@ static uint64_t edu_mmio_read(void *opaque, hwaddr addr, unsigned size)
         return val;
     }
 
+    if (addr >= 0x80 && size != 4 && size != 8) {
+        return val;
+    }
+
     switch (addr) {
     case 0x00:
         val = 0x010000edu;
-- 
2.17.1

Re: [Qemu-devel] [PATCH v2 2/3] edu: mmio: allow mmio read dispatch accept 8 bytes

[Philippe Mathieu-Daudé]

Hi Li,

On 4/20/19 6:14 PM, Li Qiang wrote:
> The edu spec said when address >= 0x80, the MMIO area can

"says"

> be accessed by 8 bytes.
> 
> Signed-off-by: Li Qiang <liq3ea@163.com>
> ---
>  hw/misc/edu.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/misc/edu.c b/hw/misc/edu.c
> index 65fc32b928..4018dddcb8 100644
> --- a/hw/misc/edu.c
> +++ b/hw/misc/edu.c
> @@ -189,6 +189,10 @@ static uint64_t edu_mmio_read(void *opaque, hwaddr addr, unsigned size)

Completing the diff ...:

       if (size != 4) {

>          return val;
>      }
>  
> +    if (addr >= 0x80 && size != 4 && size != 8) {

... to show this code is unreachable for size == 8.

> +        return val;
> +    }
> +
>      switch (addr) {
>      case 0x00:
>          val = 0x010000edu;
> 

I think the change you wanted is:

-- >8 --
@@ -185,7 +185,11 @@ static uint64_t edu_mmio_read(void *opaque, hwaddr
addr, unsigned size)
     EduState *edu = opaque;
     uint64_t val = ~0ULL;

-    if (size != 4) {
+    if (addr < 0x80 && size != 4) {
+        return val;
+    }
+
+    if (addr >= 0x80 && size != 4 && size != 8) {
         return val;
     }

---

Another cleaner way to solve this is use 2 MemoryRegionOps, one for the
first 0x80 addresses and another for the rest.

Regards,

Phil.

Re: [Qemu-devel] [PATCH v2 2/3] edu: mmio: allow mmio read dispatch accept 8 bytes

[Philippe Mathieu-Daudé]

Hi Li,

On 4/20/19 6:14 PM, Li Qiang wrote:
> The edu spec said when address >= 0x80, the MMIO area can

"says"

> be accessed by 8 bytes.
> 
> Signed-off-by: Li Qiang <liq3ea@163.com>
> ---
>  hw/misc/edu.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/misc/edu.c b/hw/misc/edu.c
> index 65fc32b928..4018dddcb8 100644
> --- a/hw/misc/edu.c
> +++ b/hw/misc/edu.c
> @@ -189,6 +189,10 @@ static uint64_t edu_mmio_read(void *opaque, hwaddr addr, unsigned size)

Completing the diff ...:

       if (size != 4) {

>          return val;
>      }
>  
> +    if (addr >= 0x80 && size != 4 && size != 8) {

... to show this code is unreachable for size == 8.

> +        return val;
> +    }
> +
>      switch (addr) {
>      case 0x00:
>          val = 0x010000edu;
> 

I think the change you wanted is:

-- >8 --
@@ -185,7 +185,11 @@ static uint64_t edu_mmio_read(void *opaque, hwaddr
addr, unsigned size)
     EduState *edu = opaque;
     uint64_t val = ~0ULL;

-    if (size != 4) {
+    if (addr < 0x80 && size != 4) {
+        return val;
+    }
+
+    if (addr >= 0x80 && size != 4 && size != 8) {
         return val;
     }

---

Another cleaner way to solve this is use 2 MemoryRegionOps, one for the
first 0x80 addresses and another for the rest.

Regards,

Phil.

Re: [Qemu-devel] [PATCH v2 2/3] edu: mmio: allow mmio read dispatch accept 8 bytes

[Li Qiang]

Philippe Mathieu-Daudé <philmd@redhat.com> 于2019年4月21日周日 下午6:44写道：

> Hi Li,
>
> On 4/20/19 6:14 PM, Li Qiang wrote:
> > The edu spec said when address >= 0x80, the MMIO area can
>
> "says"
>
> > be accessed by 8 bytes.
> >
> > Signed-off-by: Li Qiang <liq3ea@163.com>
> > ---
> >  hw/misc/edu.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/hw/misc/edu.c b/hw/misc/edu.c
> > index 65fc32b928..4018dddcb8 100644
> > --- a/hw/misc/edu.c
> > +++ b/hw/misc/edu.c
> > @@ -189,6 +189,10 @@ static uint64_t edu_mmio_read(void *opaque, hwaddr
> addr, unsigned size)
>
> Completing the diff ...:
>
>        if (size != 4) {
>
> >          return val;
> >      }
> >
> > +    if (addr >= 0x80 && size != 4 && size != 8) {
>
> ... to show this code is unreachable for size == 8.
>
>
Ohhh, yes



> > +        return val;
> > +    }
> > +
> >      switch (addr) {
> >      case 0x00:
> >          val = 0x010000edu;
> >
>
> I think the change you wanted is:
>
> -- >8 --
> @@ -185,7 +185,11 @@ static uint64_t edu_mmio_read(void *opaque, hwaddr
> addr, unsigned size)
>      EduState *edu = opaque;
>      uint64_t val = ~0ULL;
>
> -    if (size != 4) {
> +    if (addr < 0x80 && size != 4) {
> +        return val;
> +    }
> +
> +    if (addr >= 0x80 && size != 4 && size != 8) {
>          return val;
>      }
>
>

Yes, this is what I want. Thanks, will change to this in next revision.

Thanks,
Li Qiang



> ---
>
> Another cleaner way to solve this is use 2 MemoryRegionOps, one for the
> first 0x80 addresses and another for the rest.
>
> Regards,
>
> Phil.
>

Re: [Qemu-devel] [PATCH v2 2/3] edu: mmio: allow mmio read dispatch accept 8 bytes

[Li Qiang]

Philippe Mathieu-Daudé <philmd@redhat.com> 于2019年4月21日周日 下午6:44写道：

> Hi Li,
>
> On 4/20/19 6:14 PM, Li Qiang wrote:
> > The edu spec said when address >= 0x80, the MMIO area can
>
> "says"
>
> > be accessed by 8 bytes.
> >
> > Signed-off-by: Li Qiang <liq3ea@163.com>
> > ---
> >  hw/misc/edu.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/hw/misc/edu.c b/hw/misc/edu.c
> > index 65fc32b928..4018dddcb8 100644
> > --- a/hw/misc/edu.c
> > +++ b/hw/misc/edu.c
> > @@ -189,6 +189,10 @@ static uint64_t edu_mmio_read(void *opaque, hwaddr
> addr, unsigned size)
>
> Completing the diff ...:
>
>        if (size != 4) {
>
> >          return val;
> >      }
> >
> > +    if (addr >= 0x80 && size != 4 && size != 8) {
>
> ... to show this code is unreachable for size == 8.
>
>
Ohhh, yes



> > +        return val;
> > +    }
> > +
> >      switch (addr) {
> >      case 0x00:
> >          val = 0x010000edu;
> >
>
> I think the change you wanted is:
>
> -- >8 --
> @@ -185,7 +185,11 @@ static uint64_t edu_mmio_read(void *opaque, hwaddr
> addr, unsigned size)
>      EduState *edu = opaque;
>      uint64_t val = ~0ULL;
>
> -    if (size != 4) {
> +    if (addr < 0x80 && size != 4) {
> +        return val;
> +    }
> +
> +    if (addr >= 0x80 && size != 4 && size != 8) {
>          return val;
>      }
>
>

Yes, this is what I want. Thanks, will change to this in next revision.

Thanks,
Li Qiang



> ---
>
> Another cleaner way to solve this is use 2 MemoryRegionOps, one for the
> first 0x80 addresses and another for the rest.
>
> Regards,
>
> Phil.
>

[Qemu-devel] [PATCH v2 3/3] edu: uses uint64_t in dma operation

[Li Qiang]

The dma related variable is dma_addr_t, it is uint64_t in
x64 platform. Change these usage from uint32_to uint64_t to
avoid trancation.

Signed-off-by: Li Qiang <liq3ea@163.com>
---
Change since v1:
Fix format compile error on Windows

 hw/misc/edu.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/hw/misc/edu.c b/hw/misc/edu.c
index 4018dddcb8..f4a6d5f1c5 100644
--- a/hw/misc/edu.c
+++ b/hw/misc/edu.c
@@ -98,23 +98,24 @@ static void edu_lower_irq(EduState *edu, uint32_t val)
     }
 }
 
-static bool within(uint32_t addr, uint32_t start, uint32_t end)
+static bool within(uint64_t addr, uint64_t start, uint64_t end)
 {
     return start <= addr && addr < end;
 }
 
-static void edu_check_range(uint32_t addr, uint32_t size1, uint32_t start,
+static void edu_check_range(uint64_t addr, uint64_t size1, uint64_t start,
                 uint32_t size2)
 {
-    uint32_t end1 = addr + size1;
-    uint32_t end2 = start + size2;
+    uint64_t end1 = addr + size1;
+    uint64_t end2 = start + size2;
 
     if (within(addr, start, end2) &&
             end1 > addr && within(end1, start, end2)) {
         return;
     }
 
-    hw_error("EDU: DMA range 0x%.8x-0x%.8x out of bounds (0x%.8x-0x%.8x)!",
+    hw_error("EDU: DMA range 0x%016"PRIx64"-0x%016"PRIx64
+             " out of bounds (0x%016"PRIx64"-0x%016"PRIx64")!",
             addr, end1 - 1, start, end2 - 1);
 }
 
@@ -139,13 +140,13 @@ static void edu_dma_timer(void *opaque)
     }
 
     if (EDU_DMA_DIR(edu->dma.cmd) == EDU_DMA_FROM_PCI) {
-        uint32_t dst = edu->dma.dst;
+        uint64_t dst = edu->dma.dst;
         edu_check_range(dst, edu->dma.cnt, DMA_START, DMA_SIZE);
         dst -= DMA_START;
         pci_dma_read(&edu->pdev, edu_clamp_addr(edu, edu->dma.src),
                 edu->dma_buf + dst, edu->dma.cnt);
     } else {
-        uint32_t src = edu->dma.src;
+        uint64_t src = edu->dma.src;
         edu_check_range(src, edu->dma.cnt, DMA_START, DMA_SIZE);
         src -= DMA_START;
         pci_dma_write(&edu->pdev, edu_clamp_addr(edu, edu->dma.dst),
-- 
2.17.1

[Qemu-devel] [PATCH v2 3/3] edu: uses uint64_t in dma operation

[Li Qiang]

The dma related variable is dma_addr_t, it is uint64_t in
x64 platform. Change these usage from uint32_to uint64_t to
avoid trancation.

Signed-off-by: Li Qiang <liq3ea@163.com>
---
Change since v1:
Fix format compile error on Windows

 hw/misc/edu.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/hw/misc/edu.c b/hw/misc/edu.c
index 4018dddcb8..f4a6d5f1c5 100644
--- a/hw/misc/edu.c
+++ b/hw/misc/edu.c
@@ -98,23 +98,24 @@ static void edu_lower_irq(EduState *edu, uint32_t val)
     }
 }
 
-static bool within(uint32_t addr, uint32_t start, uint32_t end)
+static bool within(uint64_t addr, uint64_t start, uint64_t end)
 {
     return start <= addr && addr < end;
 }
 
-static void edu_check_range(uint32_t addr, uint32_t size1, uint32_t start,
+static void edu_check_range(uint64_t addr, uint64_t size1, uint64_t start,
                 uint32_t size2)
 {
-    uint32_t end1 = addr + size1;
-    uint32_t end2 = start + size2;
+    uint64_t end1 = addr + size1;
+    uint64_t end2 = start + size2;
 
     if (within(addr, start, end2) &&
             end1 > addr && within(end1, start, end2)) {
         return;
     }
 
-    hw_error("EDU: DMA range 0x%.8x-0x%.8x out of bounds (0x%.8x-0x%.8x)!",
+    hw_error("EDU: DMA range 0x%016"PRIx64"-0x%016"PRIx64
+             " out of bounds (0x%016"PRIx64"-0x%016"PRIx64")!",
             addr, end1 - 1, start, end2 - 1);
 }
 
@@ -139,13 +140,13 @@ static void edu_dma_timer(void *opaque)
     }
 
     if (EDU_DMA_DIR(edu->dma.cmd) == EDU_DMA_FROM_PCI) {
-        uint32_t dst = edu->dma.dst;
+        uint64_t dst = edu->dma.dst;
         edu_check_range(dst, edu->dma.cnt, DMA_START, DMA_SIZE);
         dst -= DMA_START;
         pci_dma_read(&edu->pdev, edu_clamp_addr(edu, edu->dma.src),
                 edu->dma_buf + dst, edu->dma.cnt);
     } else {
-        uint32_t src = edu->dma.src;
+        uint64_t src = edu->dma.src;
         edu_check_range(src, edu->dma.cnt, DMA_START, DMA_SIZE);
         src -= DMA_START;
         pci_dma_write(&edu->pdev, edu_clamp_addr(edu, edu->dma.dst),
-- 
2.17.1

Re: [Qemu-devel] [PATCH v2 3/3] edu: uses uint64_t in dma operation

[Philippe Mathieu-Daudé]

On 4/20/19 6:14 PM, Li Qiang wrote:
> The dma related variable is dma_addr_t, it is uint64_t in
> x64 platform. Change these usage from uint32_to uint64_t to
> avoid trancation.

"to avoid address truncation"?

> 
> Signed-off-by: Li Qiang <liq3ea@163.com>
> ---
> Change since v1:
> Fix format compile error on Windows
> 
>  hw/misc/edu.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/misc/edu.c b/hw/misc/edu.c
> index 4018dddcb8..f4a6d5f1c5 100644
> --- a/hw/misc/edu.c
> +++ b/hw/misc/edu.c
> @@ -98,23 +98,24 @@ static void edu_lower_irq(EduState *edu, uint32_t val)
>      }
>  }
>  
> -static bool within(uint32_t addr, uint32_t start, uint32_t end)
> +static bool within(uint64_t addr, uint64_t start, uint64_t end)

OK.

>  {
>      return start <= addr && addr < end;
>  }
>  
> -static void edu_check_range(uint32_t addr, uint32_t size1, uint32_t start,
> +static void edu_check_range(uint64_t addr, uint64_t size1, uint64_t start,
>                  uint32_t size2)

OK for addr. MMIO range is 1MiB so you can keep uint32_t for
size1/size2. Up to the maintainer (personally I'd prefer keep u32).

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>  {
> -    uint32_t end1 = addr + size1;
> -    uint32_t end2 = start + size2;
> +    uint64_t end1 = addr + size1;
> +    uint64_t end2 = start + size2;
>  
>      if (within(addr, start, end2) &&
>              end1 > addr && within(end1, start, end2)) {
>          return;
>      }
>  
> -    hw_error("EDU: DMA range 0x%.8x-0x%.8x out of bounds (0x%.8x-0x%.8x)!",
> +    hw_error("EDU: DMA range 0x%016"PRIx64"-0x%016"PRIx64
> +             " out of bounds (0x%016"PRIx64"-0x%016"PRIx64")!",
>              addr, end1 - 1, start, end2 - 1);
>  }
>  
> @@ -139,13 +140,13 @@ static void edu_dma_timer(void *opaque)
>      }
>  
>      if (EDU_DMA_DIR(edu->dma.cmd) == EDU_DMA_FROM_PCI) {
> -        uint32_t dst = edu->dma.dst;
> +        uint64_t dst = edu->dma.dst;
>          edu_check_range(dst, edu->dma.cnt, DMA_START, DMA_SIZE);
>          dst -= DMA_START;
>          pci_dma_read(&edu->pdev, edu_clamp_addr(edu, edu->dma.src),
>                  edu->dma_buf + dst, edu->dma.cnt);
>      } else {
> -        uint32_t src = edu->dma.src;
> +        uint64_t src = edu->dma.src;
>          edu_check_range(src, edu->dma.cnt, DMA_START, DMA_SIZE);
>          src -= DMA_START;
>          pci_dma_write(&edu->pdev, edu_clamp_addr(edu, edu->dma.dst),
> 

Re: [Qemu-devel] [PATCH v2 3/3] edu: uses uint64_t in dma operation

[Philippe Mathieu-Daudé]

On 4/20/19 6:14 PM, Li Qiang wrote:
> The dma related variable is dma_addr_t, it is uint64_t in
> x64 platform. Change these usage from uint32_to uint64_t to
> avoid trancation.

"to avoid address truncation"?

> 
> Signed-off-by: Li Qiang <liq3ea@163.com>
> ---
> Change since v1:
> Fix format compile error on Windows
> 
>  hw/misc/edu.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/misc/edu.c b/hw/misc/edu.c
> index 4018dddcb8..f4a6d5f1c5 100644
> --- a/hw/misc/edu.c
> +++ b/hw/misc/edu.c
> @@ -98,23 +98,24 @@ static void edu_lower_irq(EduState *edu, uint32_t val)
>      }
>  }
>  
> -static bool within(uint32_t addr, uint32_t start, uint32_t end)
> +static bool within(uint64_t addr, uint64_t start, uint64_t end)

OK.

>  {
>      return start <= addr && addr < end;
>  }
>  
> -static void edu_check_range(uint32_t addr, uint32_t size1, uint32_t start,
> +static void edu_check_range(uint64_t addr, uint64_t size1, uint64_t start,
>                  uint32_t size2)

OK for addr. MMIO range is 1MiB so you can keep uint32_t for
size1/size2. Up to the maintainer (personally I'd prefer keep u32).

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>  {
> -    uint32_t end1 = addr + size1;
> -    uint32_t end2 = start + size2;
> +    uint64_t end1 = addr + size1;
> +    uint64_t end2 = start + size2;
>  
>      if (within(addr, start, end2) &&
>              end1 > addr && within(end1, start, end2)) {
>          return;
>      }
>  
> -    hw_error("EDU: DMA range 0x%.8x-0x%.8x out of bounds (0x%.8x-0x%.8x)!",
> +    hw_error("EDU: DMA range 0x%016"PRIx64"-0x%016"PRIx64
> +             " out of bounds (0x%016"PRIx64"-0x%016"PRIx64")!",
>              addr, end1 - 1, start, end2 - 1);
>  }
>  
> @@ -139,13 +140,13 @@ static void edu_dma_timer(void *opaque)
>      }
>  
>      if (EDU_DMA_DIR(edu->dma.cmd) == EDU_DMA_FROM_PCI) {
> -        uint32_t dst = edu->dma.dst;
> +        uint64_t dst = edu->dma.dst;
>          edu_check_range(dst, edu->dma.cnt, DMA_START, DMA_SIZE);
>          dst -= DMA_START;
>          pci_dma_read(&edu->pdev, edu_clamp_addr(edu, edu->dma.src),
>                  edu->dma_buf + dst, edu->dma.cnt);
>      } else {
> -        uint32_t src = edu->dma.src;
> +        uint64_t src = edu->dma.src;
>          edu_check_range(src, edu->dma.cnt, DMA_START, DMA_SIZE);
>          src -= DMA_START;
>          pci_dma_write(&edu->pdev, edu_clamp_addr(edu, edu->dma.dst),
> 

Re: [Qemu-devel] [PATCH v2 3/3] edu: uses uint64_t in dma operation

[Li Qiang]

Philippe Mathieu-Daudé <philmd@redhat.com> 于2019年4月21日周日 下午6:32写道：

> On 4/20/19 6:14 PM, Li Qiang wrote:
> > The dma related variable is dma_addr_t, it is uint64_t in
> > x64 platform. Change these usage from uint32_to uint64_t to
> > avoid trancation.
>
> "to avoid address truncation"?
>
>

The dma.dst/src/cnt..is from guest and is 64-bits. But in 'edu_dma_timer',
it is assigned to uint32_t, If it is 0xffffffff 00000000, it will be ok by
the check
but it is of course not allowed.

Though this is just an edu device, I think we should avoid this.

Thanks,
Li Qiang



> >
> > Signed-off-by: Li Qiang <liq3ea@163.com>
> > ---
> > Change since v1:
> > Fix format compile error on Windows
> >
> >  hw/misc/edu.c | 15 ++++++++-------
> >  1 file changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/misc/edu.c b/hw/misc/edu.c
> > index 4018dddcb8..f4a6d5f1c5 100644
> > --- a/hw/misc/edu.c
> > +++ b/hw/misc/edu.c
> > @@ -98,23 +98,24 @@ static void edu_lower_irq(EduState *edu, uint32_t
> val)
> >      }
> >  }
> >
> > -static bool within(uint32_t addr, uint32_t start, uint32_t end)
> > +static bool within(uint64_t addr, uint64_t start, uint64_t end)
>
> OK.
>
> >  {
> >      return start <= addr && addr < end;
> >  }
> >
> > -static void edu_check_range(uint32_t addr, uint32_t size1, uint32_t
> start,
> > +static void edu_check_range(uint64_t addr, uint64_t size1, uint64_t
> start,
> >                  uint32_t size2)
>
> OK for addr. MMIO range is 1MiB so you can keep uint32_t for
> size1/size2. Up to the maintainer (personally I'd prefer keep u32).
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>
> >  {
> > -    uint32_t end1 = addr + size1;
> > -    uint32_t end2 = start + size2;
> > +    uint64_t end1 = addr + size1;
> > +    uint64_t end2 = start + size2;
> >
> >      if (within(addr, start, end2) &&
> >              end1 > addr && within(end1, start, end2)) {
> >          return;
> >      }
> >
> > -    hw_error("EDU: DMA range 0x%.8x-0x%.8x out of bounds
> (0x%.8x-0x%.8x)!",
> > +    hw_error("EDU: DMA range 0x%016"PRIx64"-0x%016"PRIx64
> > +             " out of bounds (0x%016"PRIx64"-0x%016"PRIx64")!",
> >              addr, end1 - 1, start, end2 - 1);
> >  }
> >
> > @@ -139,13 +140,13 @@ static void edu_dma_timer(void *opaque)
> >      }
> >
> >      if (EDU_DMA_DIR(edu->dma.cmd) == EDU_DMA_FROM_PCI) {
> > -        uint32_t dst = edu->dma.dst;
> > +        uint64_t dst = edu->dma.dst;
> >          edu_check_range(dst, edu->dma.cnt, DMA_START, DMA_SIZE);
> >          dst -= DMA_START;
> >          pci_dma_read(&edu->pdev, edu_clamp_addr(edu, edu->dma.src),
> >                  edu->dma_buf + dst, edu->dma.cnt);
> >      } else {
> > -        uint32_t src = edu->dma.src;
> > +        uint64_t src = edu->dma.src;
> >          edu_check_range(src, edu->dma.cnt, DMA_START, DMA_SIZE);
> >          src -= DMA_START;
> >          pci_dma_write(&edu->pdev, edu_clamp_addr(edu, edu->dma.dst),
> >
>

Re: [Qemu-devel] [PATCH v2 3/3] edu: uses uint64_t in dma operation

[Li Qiang]

Philippe Mathieu-Daudé <philmd@redhat.com> 于2019年4月21日周日 下午6:32写道：

> On 4/20/19 6:14 PM, Li Qiang wrote:
> > The dma related variable is dma_addr_t, it is uint64_t in
> > x64 platform. Change these usage from uint32_to uint64_t to
> > avoid trancation.
>
> "to avoid address truncation"?
>
>

The dma.dst/src/cnt..is from guest and is 64-bits. But in 'edu_dma_timer',
it is assigned to uint32_t, If it is 0xffffffff 00000000, it will be ok by
the check
but it is of course not allowed.

Though this is just an edu device, I think we should avoid this.

Thanks,
Li Qiang



> >
> > Signed-off-by: Li Qiang <liq3ea@163.com>
> > ---
> > Change since v1:
> > Fix format compile error on Windows
> >
> >  hw/misc/edu.c | 15 ++++++++-------
> >  1 file changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/misc/edu.c b/hw/misc/edu.c
> > index 4018dddcb8..f4a6d5f1c5 100644
> > --- a/hw/misc/edu.c
> > +++ b/hw/misc/edu.c
> > @@ -98,23 +98,24 @@ static void edu_lower_irq(EduState *edu, uint32_t
> val)
> >      }
> >  }
> >
> > -static bool within(uint32_t addr, uint32_t start, uint32_t end)
> > +static bool within(uint64_t addr, uint64_t start, uint64_t end)
>
> OK.
>
> >  {
> >      return start <= addr && addr < end;
> >  }
> >
> > -static void edu_check_range(uint32_t addr, uint32_t size1, uint32_t
> start,
> > +static void edu_check_range(uint64_t addr, uint64_t size1, uint64_t
> start,
> >                  uint32_t size2)
>
> OK for addr. MMIO range is 1MiB so you can keep uint32_t for
> size1/size2. Up to the maintainer (personally I'd prefer keep u32).
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>
> >  {
> > -    uint32_t end1 = addr + size1;
> > -    uint32_t end2 = start + size2;
> > +    uint64_t end1 = addr + size1;
> > +    uint64_t end2 = start + size2;
> >
> >      if (within(addr, start, end2) &&
> >              end1 > addr && within(end1, start, end2)) {
> >          return;
> >      }
> >
> > -    hw_error("EDU: DMA range 0x%.8x-0x%.8x out of bounds
> (0x%.8x-0x%.8x)!",
> > +    hw_error("EDU: DMA range 0x%016"PRIx64"-0x%016"PRIx64
> > +             " out of bounds (0x%016"PRIx64"-0x%016"PRIx64")!",
> >              addr, end1 - 1, start, end2 - 1);
> >  }
> >
> > @@ -139,13 +140,13 @@ static void edu_dma_timer(void *opaque)
> >      }
> >
> >      if (EDU_DMA_DIR(edu->dma.cmd) == EDU_DMA_FROM_PCI) {
> > -        uint32_t dst = edu->dma.dst;
> > +        uint64_t dst = edu->dma.dst;
> >          edu_check_range(dst, edu->dma.cnt, DMA_START, DMA_SIZE);
> >          dst -= DMA_START;
> >          pci_dma_read(&edu->pdev, edu_clamp_addr(edu, edu->dma.src),
> >                  edu->dma_buf + dst, edu->dma.cnt);
> >      } else {
> > -        uint32_t src = edu->dma.src;
> > +        uint64_t src = edu->dma.src;
> >          edu_check_range(src, edu->dma.cnt, DMA_START, DMA_SIZE);
> >          src -= DMA_START;
> >          pci_dma_write(&edu->pdev, edu_clamp_addr(edu, edu->dma.dst),
> >
>

Re: [Qemu-devel] [PATCH v2 3/3] edu: uses uint64_t in dma operation

[Philippe Mathieu-Daudé]

On 4/22/19 3:21 AM, Li Qiang wrote:
> 
> 
> Philippe Mathieu-Daudé <philmd@redhat.com <mailto:philmd@redhat.com>> 于
> 2019年4月21日周日 下午6:32写道：
> 
>     On 4/20/19 6:14 PM, Li Qiang wrote:
>     > The dma related variable is dma_addr_t, it is uint64_t in
>     > x64 platform. Change these usage from uint32_to uint64_t to
>     > avoid trancation.
> 
>     "to avoid address truncation"?
> 
> 
> 
> The dma.dst/src/cnt..is from guest and is 64-bits. But in 'edu_dma_timer',
> it is assigned to uint32_t, If it is 0xffffffff 00000000, it will be ok
> by the check
> but it is of course not allowed. 
> 
> Though this is just an edu device, I think we should avoid this. 

I agree with you, I was just trying to correct your English ;)

> Thanks,
> Li Qiang
> 
>  
> 
>     >
>     > Signed-off-by: Li Qiang <liq3ea@163.com <mailto:liq3ea@163.com>>
>     > ---
>     > Change since v1:
>     > Fix format compile error on Windows
>     >
>     >  hw/misc/edu.c | 15 ++++++++-------
>     >  1 file changed, 8 insertions(+), 7 deletions(-)
>     >
>     > diff --git a/hw/misc/edu.c b/hw/misc/edu.c
>     > index 4018dddcb8..f4a6d5f1c5 100644
>     > --- a/hw/misc/edu.c
>     > +++ b/hw/misc/edu.c
>     > @@ -98,23 +98,24 @@ static void edu_lower_irq(EduState *edu,
>     uint32_t val)
>     >      }
>     >  }
>     > 
>     > -static bool within(uint32_t addr, uint32_t start, uint32_t end)
>     > +static bool within(uint64_t addr, uint64_t start, uint64_t end)
> 
>     OK.
> 
>     >  {
>     >      return start <= addr && addr < end;
>     >  }
>     > 
>     > -static void edu_check_range(uint32_t addr, uint32_t size1,
>     uint32_t start,
>     > +static void edu_check_range(uint64_t addr, uint64_t size1,
>     uint64_t start,
>     >                  uint32_t size2)
> 
>     OK for addr. MMIO range is 1MiB so you can keep uint32_t for
>     size1/size2. Up to the maintainer (personally I'd prefer keep u32).
> 
>     Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com
>     <mailto:philmd@redhat.com>>
> 
>     >  {
>     > -    uint32_t end1 = addr + size1;
>     > -    uint32_t end2 = start + size2;
>     > +    uint64_t end1 = addr + size1;
>     > +    uint64_t end2 = start + size2;
>     > 
>     >      if (within(addr, start, end2) &&
>     >              end1 > addr && within(end1, start, end2)) {
>     >          return;
>     >      }
>     > 
>     > -    hw_error("EDU: DMA range 0x%.8x-0x%.8x out of bounds
>     (0x%.8x-0x%.8x)!",
>     > +    hw_error("EDU: DMA range 0x%016"PRIx64"-0x%016"PRIx64
>     > +             " out of bounds (0x%016"PRIx64"-0x%016"PRIx64")!",
>     >              addr, end1 - 1, start, end2 - 1);
>     >  }
>     > 
>     > @@ -139,13 +140,13 @@ static void edu_dma_timer(void *opaque)
>     >      }
>     > 
>     >      if (EDU_DMA_DIR(edu->dma.cmd) == EDU_DMA_FROM_PCI) {
>     > -        uint32_t dst = edu->dma.dst;
>     > +        uint64_t dst = edu->dma.dst;
>     >          edu_check_range(dst, edu->dma.cnt, DMA_START, DMA_SIZE);
>     >          dst -= DMA_START;
>     >          pci_dma_read(&edu->pdev, edu_clamp_addr(edu, edu->dma.src),
>     >                  edu->dma_buf + dst, edu->dma.cnt);
>     >      } else {
>     > -        uint32_t src = edu->dma.src;
>     > +        uint64_t src = edu->dma.src;
>     >          edu_check_range(src, edu->dma.cnt, DMA_START, DMA_SIZE);
>     >          src -= DMA_START;
>     >          pci_dma_write(&edu->pdev, edu_clamp_addr(edu, edu->dma.dst),
>     >
> 

Re: [Qemu-devel] [PATCH v2 3/3] edu: uses uint64_t in dma operation

[Philippe Mathieu-Daudé]

On 4/22/19 3:21 AM, Li Qiang wrote:
> 
> 
> Philippe Mathieu-Daudé <philmd@redhat.com <mailto:philmd@redhat.com>> 于
> 2019年4月21日周日 下午6:32写道：
> 
>     On 4/20/19 6:14 PM, Li Qiang wrote:
>     > The dma related variable is dma_addr_t, it is uint64_t in
>     > x64 platform. Change these usage from uint32_to uint64_t to
>     > avoid trancation.
> 
>     "to avoid address truncation"?
> 
> 
> 
> The dma.dst/src/cnt..is from guest and is 64-bits. But in 'edu_dma_timer',
> it is assigned to uint32_t, If it is 0xffffffff 00000000, it will be ok
> by the check
> but it is of course not allowed. 
> 
> Though this is just an edu device, I think we should avoid this. 

I agree with you, I was just trying to correct your English ;)

> Thanks,
> Li Qiang
> 
>  
> 
>     >
>     > Signed-off-by: Li Qiang <liq3ea@163.com <mailto:liq3ea@163.com>>
>     > ---
>     > Change since v1:
>     > Fix format compile error on Windows
>     >
>     >  hw/misc/edu.c | 15 ++++++++-------
>     >  1 file changed, 8 insertions(+), 7 deletions(-)
>     >
>     > diff --git a/hw/misc/edu.c b/hw/misc/edu.c
>     > index 4018dddcb8..f4a6d5f1c5 100644
>     > --- a/hw/misc/edu.c
>     > +++ b/hw/misc/edu.c
>     > @@ -98,23 +98,24 @@ static void edu_lower_irq(EduState *edu,
>     uint32_t val)
>     >      }
>     >  }
>     > 
>     > -static bool within(uint32_t addr, uint32_t start, uint32_t end)
>     > +static bool within(uint64_t addr, uint64_t start, uint64_t end)
> 
>     OK.
> 
>     >  {
>     >      return start <= addr && addr < end;
>     >  }
>     > 
>     > -static void edu_check_range(uint32_t addr, uint32_t size1,
>     uint32_t start,
>     > +static void edu_check_range(uint64_t addr, uint64_t size1,
>     uint64_t start,
>     >                  uint32_t size2)
> 
>     OK for addr. MMIO range is 1MiB so you can keep uint32_t for
>     size1/size2. Up to the maintainer (personally I'd prefer keep u32).
> 
>     Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com
>     <mailto:philmd@redhat.com>>
> 
>     >  {
>     > -    uint32_t end1 = addr + size1;
>     > -    uint32_t end2 = start + size2;
>     > +    uint64_t end1 = addr + size1;
>     > +    uint64_t end2 = start + size2;
>     > 
>     >      if (within(addr, start, end2) &&
>     >              end1 > addr && within(end1, start, end2)) {
>     >          return;
>     >      }
>     > 
>     > -    hw_error("EDU: DMA range 0x%.8x-0x%.8x out of bounds
>     (0x%.8x-0x%.8x)!",
>     > +    hw_error("EDU: DMA range 0x%016"PRIx64"-0x%016"PRIx64
>     > +             " out of bounds (0x%016"PRIx64"-0x%016"PRIx64")!",
>     >              addr, end1 - 1, start, end2 - 1);
>     >  }
>     > 
>     > @@ -139,13 +140,13 @@ static void edu_dma_timer(void *opaque)
>     >      }
>     > 
>     >      if (EDU_DMA_DIR(edu->dma.cmd) == EDU_DMA_FROM_PCI) {
>     > -        uint32_t dst = edu->dma.dst;
>     > +        uint64_t dst = edu->dma.dst;
>     >          edu_check_range(dst, edu->dma.cnt, DMA_START, DMA_SIZE);
>     >          dst -= DMA_START;
>     >          pci_dma_read(&edu->pdev, edu_clamp_addr(edu, edu->dma.src),
>     >                  edu->dma_buf + dst, edu->dma.cnt);
>     >      } else {
>     > -        uint32_t src = edu->dma.src;
>     > +        uint64_t src = edu->dma.src;
>     >          edu_check_range(src, edu->dma.cnt, DMA_START, DMA_SIZE);
>     >          src -= DMA_START;
>     >          pci_dma_write(&edu->pdev, edu_clamp_addr(edu, edu->dma.dst),
>     >
>