[Qemu-devel] [PATCH] cputlb: Fix io_readx() to respect the access_type

[Qemu-devel] [PATCH] cputlb: Fix io_readx() to respect the access_type

[Shahab Vahedi]

This change adapts io_readx() to its input access_type. Currently
io_readx() treats any memory access as a read, although it has an
input argument "MMUAccessType access_type". This results in:

1) Calling the tlb_fill() only with MMU_DATA_LOAD
2) Considering only entry->addr_read as the tlb_addr

Buglink: https://bugs.launchpad.net/qemu/+bug/1825359

Signed-off-by: Shahab Vahedi <shahab.vahedi@gmail.com>
---
Changelog:
- Extra space before closing parenthesis is removed

 accel/tcg/cputlb.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 88cc8389e9..4a305ac942 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -878,10 +878,13 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
         CPUTLBEntry *entry;
         target_ulong tlb_addr;
 
-        tlb_fill(cpu, addr, size, MMU_DATA_LOAD, mmu_idx, retaddr);
+        tlb_fill(cpu, addr, size, access_type, mmu_idx, retaddr);
 
         entry = tlb_entry(env, mmu_idx, addr);
-        tlb_addr = entry->addr_read;
+        tlb_addr =
+            (access_type == MMU_DATA_LOAD)  ? entry->addr_read  :
+            (access_type == MMU_DATA_STORE) ? entry->addr_write :
+            entry->addr_code;
         if (!(tlb_addr & ~(TARGET_PAGE_MASK | TLB_RECHECK))) {
             /* RAM access */
             uintptr_t haddr = addr + entry->addend;
-- 
2.21.0

[Qemu-devel] [PATCH] cputlb: Fix io_readx() to respect the access_type

[Shahab Vahedi]

This change adapts io_readx() to its input access_type. Currently
io_readx() treats any memory access as a read, although it has an
input argument "MMUAccessType access_type". This results in:

1) Calling the tlb_fill() only with MMU_DATA_LOAD
2) Considering only entry->addr_read as the tlb_addr

Buglink: https://bugs.launchpad.net/qemu/+bug/1825359

Signed-off-by: Shahab Vahedi <shahab.vahedi@gmail.com>
---
Changelog:
- Extra space before closing parenthesis is removed

 accel/tcg/cputlb.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 88cc8389e9..4a305ac942 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -878,10 +878,13 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
         CPUTLBEntry *entry;
         target_ulong tlb_addr;
 
-        tlb_fill(cpu, addr, size, MMU_DATA_LOAD, mmu_idx, retaddr);
+        tlb_fill(cpu, addr, size, access_type, mmu_idx, retaddr);
 
         entry = tlb_entry(env, mmu_idx, addr);
-        tlb_addr = entry->addr_read;
+        tlb_addr =
+            (access_type == MMU_DATA_LOAD)  ? entry->addr_read  :
+            (access_type == MMU_DATA_STORE) ? entry->addr_write :
+            entry->addr_code;
         if (!(tlb_addr & ~(TARGET_PAGE_MASK | TLB_RECHECK))) {
             /* RAM access */
             uintptr_t haddr = addr + entry->addend;
-- 
2.21.0

Re: [Qemu-devel] [PATCH] cputlb: Fix io_readx() to respect the access_type

[Peter Maydell]

On Fri, 19 Apr 2019 at 12:46, Shahab Vahedi <shahab.vahedi@gmail.com> wrote:
>
> This change adapts io_readx() to its input access_type. Currently
> io_readx() treats any memory access as a read, although it has an
> input argument "MMUAccessType access_type". This results in:
>
> 1) Calling the tlb_fill() only with MMU_DATA_LOAD
> 2) Considering only entry->addr_read as the tlb_addr
>
> Buglink: https://bugs.launchpad.net/qemu/+bug/1825359
>
> Signed-off-by: Shahab Vahedi <shahab.vahedi@gmail.com>
> ---
> Changelog:
> - Extra space before closing parenthesis is removed
>
>  accel/tcg/cputlb.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)

Hi; this patch mostly looks good; thanks for submitting it.

> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index 88cc8389e9..4a305ac942 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -878,10 +878,13 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
>          CPUTLBEntry *entry;
>          target_ulong tlb_addr;
>
> -        tlb_fill(cpu, addr, size, MMU_DATA_LOAD, mmu_idx, retaddr);
> +        tlb_fill(cpu, addr, size, access_type, mmu_idx, retaddr);
>
>          entry = tlb_entry(env, mmu_idx, addr);
> -        tlb_addr = entry->addr_read;
> +        tlb_addr =
> +            (access_type == MMU_DATA_LOAD)  ? entry->addr_read  :
> +            (access_type == MMU_DATA_STORE) ? entry->addr_write :
> +            entry->addr_code;

Here you don't need to handle MMU_DATA_STORE, because
we're in io_readx -- stores will go to io_writex, not here.

Style-wise it's probably better just to use an
  if (...) {
      tlb_addr = ...;
  } else {
      tlb_addr = ...;
  }

rather than a multi-line ?: expression.

>          if (!(tlb_addr & ~(TARGET_PAGE_MASK | TLB_RECHECK))) {
>              /* RAM access */
>              uintptr_t haddr = addr + entry->addend;
> --
> 2.21.0
>

thanks
-- PMM

Re: [Qemu-devel] [PATCH] cputlb: Fix io_readx() to respect the access_type

[Peter Maydell]

On Fri, 19 Apr 2019 at 12:46, Shahab Vahedi <shahab.vahedi@gmail.com> wrote:
>
> This change adapts io_readx() to its input access_type. Currently
> io_readx() treats any memory access as a read, although it has an
> input argument "MMUAccessType access_type". This results in:
>
> 1) Calling the tlb_fill() only with MMU_DATA_LOAD
> 2) Considering only entry->addr_read as the tlb_addr
>
> Buglink: https://bugs.launchpad.net/qemu/+bug/1825359
>
> Signed-off-by: Shahab Vahedi <shahab.vahedi@gmail.com>
> ---
> Changelog:
> - Extra space before closing parenthesis is removed
>
>  accel/tcg/cputlb.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)

Hi; this patch mostly looks good; thanks for submitting it.

> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index 88cc8389e9..4a305ac942 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -878,10 +878,13 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
>          CPUTLBEntry *entry;
>          target_ulong tlb_addr;
>
> -        tlb_fill(cpu, addr, size, MMU_DATA_LOAD, mmu_idx, retaddr);
> +        tlb_fill(cpu, addr, size, access_type, mmu_idx, retaddr);
>
>          entry = tlb_entry(env, mmu_idx, addr);
> -        tlb_addr = entry->addr_read;
> +        tlb_addr =
> +            (access_type == MMU_DATA_LOAD)  ? entry->addr_read  :
> +            (access_type == MMU_DATA_STORE) ? entry->addr_write :
> +            entry->addr_code;

Here you don't need to handle MMU_DATA_STORE, because
we're in io_readx -- stores will go to io_writex, not here.

Style-wise it's probably better just to use an
  if (...) {
      tlb_addr = ...;
  } else {
      tlb_addr = ...;
  }

rather than a multi-line ?: expression.

>          if (!(tlb_addr & ~(TARGET_PAGE_MASK | TLB_RECHECK))) {
>              /* RAM access */
>              uintptr_t haddr = addr + entry->addend;
> --
> 2.21.0
>

thanks
-- PMM

Re: [Qemu-devel] [PATCH] cputlb: Fix io_readx() to respect the access_type

[Shahab Vahedi]

Hi Peter,

On Sat, Apr 20, 2019 at 07:57:31PM +0100, Peter Maydell wrote:
> On Fri, 19 Apr 2019 at 12:46, Shahab Vahedi <shahab.vahedi@gmail.com> wrote:
> >
> > This change adapts io_readx() to its input access_type. Currently
> > io_readx() treats any memory access as a read, although it has an
> > input argument "MMUAccessType access_type". This results in:
> >
> > 1) Calling the tlb_fill() only with MMU_DATA_LOAD
> > 2) Considering only entry->addr_read as the tlb_addr
> >
> > Buglink: https://bugs.launchpad.net/qemu/+bug/1825359
> >
> > Signed-off-by: Shahab Vahedi <shahab.vahedi@gmail.com>
> > ---
> > Changelog:
> > - Extra space before closing parenthesis is removed
> >
> >  accel/tcg/cputlb.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> Hi; this patch mostly looks good; thanks for submitting it.
> 
Please let me remind you that there is a newer [PATCH v3].

> > diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> > index 88cc8389e9..4a305ac942 100644
> > --- a/accel/tcg/cputlb.c
> > +++ b/accel/tcg/cputlb.c
> > @@ -878,10 +878,13 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
> >          CPUTLBEntry *entry;
> >          target_ulong tlb_addr;
> >
> > -        tlb_fill(cpu, addr, size, MMU_DATA_LOAD, mmu_idx, retaddr);
> > +        tlb_fill(cpu, addr, size, access_type, mmu_idx, retaddr);
> >
> >          entry = tlb_entry(env, mmu_idx, addr);
> > -        tlb_addr = entry->addr_read;
> > +        tlb_addr =
> > +            (access_type == MMU_DATA_LOAD)  ? entry->addr_read  :
> > +            (access_type == MMU_DATA_STORE) ? entry->addr_write :
> > +            entry->addr_code;
> 
> Here you don't need to handle MMU_DATA_STORE, because
> we're in io_readx -- stores will go to io_writex, not here.
> 
This concern was already raised by Alex (Bennée) and in [PATCH v3] I
addressed it the way he suggested: an _assert_ in the beginning to
verify that "access_type" is only for fetching and reading.
I must say, Philippe (Mathieu-Daudé) has his doubts about using an
_assert_ like this.

>
> Style-wise it's probably better just to use an
>   if (...) {
>       tlb_addr = ...;
>   } else {
>       tlb_addr = ...;
>   }
> 
> rather than a multi-line ?: expression.
> 
Sure Peter, I will do that, but please let me remind you that in
[PATCH v3] the conditional part is a two-liner (i.s.o the three-
liner here).

> >          if (!(tlb_addr & ~(TARGET_PAGE_MASK | TLB_RECHECK))) {
> >              /* RAM access */
> >              uintptr_t haddr = addr + entry->addend;
> > --
> > 2.21.0
> >
> 
> thanks
> -- PMM

I have a question though: Richard (Henderson) has already _reviewed_
[PATCH v3]. Is it OK if I change the code further and submit yet a
newer version?

Cheers,
Shahab

Re: [Qemu-devel] [PATCH] cputlb: Fix io_readx() to respect the access_type

[Shahab Vahedi]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="UTF-8", Size: 2774 bytes --]

Hi Peter,

On Sat, Apr 20, 2019 at 07:57:31PM +0100, Peter Maydell wrote:
> On Fri, 19 Apr 2019 at 12:46, Shahab Vahedi <shahab.vahedi@gmail.com> wrote:
> >
> > This change adapts io_readx() to its input access_type. Currently
> > io_readx() treats any memory access as a read, although it has an
> > input argument "MMUAccessType access_type". This results in:
> >
> > 1) Calling the tlb_fill() only with MMU_DATA_LOAD
> > 2) Considering only entry->addr_read as the tlb_addr
> >
> > Buglink: https://bugs.launchpad.net/qemu/+bug/1825359
> >
> > Signed-off-by: Shahab Vahedi <shahab.vahedi@gmail.com>
> > ---
> > Changelog:
> > - Extra space before closing parenthesis is removed
> >
> >  accel/tcg/cputlb.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> Hi; this patch mostly looks good; thanks for submitting it.
> 
Please let me remind you that there is a newer [PATCH v3].

> > diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> > index 88cc8389e9..4a305ac942 100644
> > --- a/accel/tcg/cputlb.c
> > +++ b/accel/tcg/cputlb.c
> > @@ -878,10 +878,13 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
> >          CPUTLBEntry *entry;
> >          target_ulong tlb_addr;
> >
> > -        tlb_fill(cpu, addr, size, MMU_DATA_LOAD, mmu_idx, retaddr);
> > +        tlb_fill(cpu, addr, size, access_type, mmu_idx, retaddr);
> >
> >          entry = tlb_entry(env, mmu_idx, addr);
> > -        tlb_addr = entry->addr_read;
> > +        tlb_addr =
> > +            (access_type == MMU_DATA_LOAD)  ? entry->addr_read  :
> > +            (access_type == MMU_DATA_STORE) ? entry->addr_write :
> > +            entry->addr_code;
> 
> Here you don't need to handle MMU_DATA_STORE, because
> we're in io_readx -- stores will go to io_writex, not here.
> 
This concern was already raised by Alex (Bennée) and in [PATCH v3] I
addressed it the way he suggested: an _assert_ in the beginning to
verify that "access_type" is only for fetching and reading.
I must say, Philippe (Mathieu-Daudé) has his doubts about using an
_assert_ like this.

>
> Style-wise it's probably better just to use an
>   if (...) {
>       tlb_addr = ...;
>   } else {
>       tlb_addr = ...;
>   }
> 
> rather than a multi-line ?: expression.
> 
Sure Peter, I will do that, but please let me remind you that in
[PATCH v3] the conditional part is a two-liner (i.s.o the three-
liner here).

> >          if (!(tlb_addr & ~(TARGET_PAGE_MASK | TLB_RECHECK))) {
> >              /* RAM access */
> >              uintptr_t haddr = addr + entry->addend;
> > --
> > 2.21.0
> >
> 
> thanks
> -- PMM

I have a question though: Richard (Henderson) has already _reviewed_
[PATCH v3]. Is it OK if I change the code further and submit yet a
newer version?

Cheers,
Shahab

Re: [Qemu-devel] [PATCH] cputlb: Fix io_readx() to respect the access_type

[Peter Maydell]

On Sun, 21 Apr 2019 at 09:03, Shahab Vahedi <shahab.vahedi@gmail.com> wrote:
>
> Hi Peter,
>
> On Sat, Apr 20, 2019 at 07:57:31PM +0100, Peter Maydell wrote:
> > On Fri, 19 Apr 2019 at 12:46, Shahab Vahedi <shahab.vahedi@gmail.com> wrote:
> > >
> > > This change adapts io_readx() to its input access_type. Currently
> > > io_readx() treats any memory access as a read, although it has an
> > > input argument "MMUAccessType access_type". This results in:
> > >
> > > 1) Calling the tlb_fill() only with MMU_DATA_LOAD
> > > 2) Considering only entry->addr_read as the tlb_addr
> > >
> > > Buglink: https://bugs.launchpad.net/qemu/+bug/1825359
> > >
> > > Signed-off-by: Shahab Vahedi <shahab.vahedi@gmail.com>
> > > ---
> > > Changelog:
> > > - Extra space before closing parenthesis is removed
> > >
> > >  accel/tcg/cputlb.c | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > Hi; this patch mostly looks good; thanks for submitting it.
> >
> Please let me remind you that there is a newer [PATCH v3].

Oops, yes, I missed that (was going through a lot of
list emails after being away for a bit). Sorry.

> I have a question though: Richard (Henderson) has already
> _reviewed_ [PATCH v3]. Is it OK if I change the code further
> and submit yet a newer version?

If you think it's necessary, you can -- for instance if
you find a bug or other problem in it. If you change
the code much then you should drop the reviewed-by:
line as the code needs re-review. If the change is
entirely trivial (eg fixing a comment typo) then you
can let the reviewed-by tag stand (ie include it in
the version you send out).

But if you're thinking of doing it just to fiddle
with the ?: style issue I suggested above, don't
bother -- it doesn't matter that much (and not
trying to deal with all 3 cases means you don't
have the nested ?: anyway).

thanks
-- PMM

Re: [Qemu-devel] [PATCH] cputlb: Fix io_readx() to respect the access_type

[Peter Maydell]

On Sun, 21 Apr 2019 at 09:03, Shahab Vahedi <shahab.vahedi@gmail.com> wrote:
>
> Hi Peter,
>
> On Sat, Apr 20, 2019 at 07:57:31PM +0100, Peter Maydell wrote:
> > On Fri, 19 Apr 2019 at 12:46, Shahab Vahedi <shahab.vahedi@gmail.com> wrote:
> > >
> > > This change adapts io_readx() to its input access_type. Currently
> > > io_readx() treats any memory access as a read, although it has an
> > > input argument "MMUAccessType access_type". This results in:
> > >
> > > 1) Calling the tlb_fill() only with MMU_DATA_LOAD
> > > 2) Considering only entry->addr_read as the tlb_addr
> > >
> > > Buglink: https://bugs.launchpad.net/qemu/+bug/1825359
> > >
> > > Signed-off-by: Shahab Vahedi <shahab.vahedi@gmail.com>
> > > ---
> > > Changelog:
> > > - Extra space before closing parenthesis is removed
> > >
> > >  accel/tcg/cputlb.c | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > Hi; this patch mostly looks good; thanks for submitting it.
> >
> Please let me remind you that there is a newer [PATCH v3].

Oops, yes, I missed that (was going through a lot of
list emails after being away for a bit). Sorry.

> I have a question though: Richard (Henderson) has already
> _reviewed_ [PATCH v3]. Is it OK if I change the code further
> and submit yet a newer version?

If you think it's necessary, you can -- for instance if
you find a bug or other problem in it. If you change
the code much then you should drop the reviewed-by:
line as the code needs re-review. If the change is
entirely trivial (eg fixing a comment typo) then you
can let the reviewed-by tag stand (ie include it in
the version you send out).

But if you're thinking of doing it just to fiddle
with the ?: style issue I suggested above, don't
bother -- it doesn't matter that much (and not
trying to deal with all 3 cases means you don't
have the nested ?: anyway).

thanks
-- PMM

[Qemu-devel] [PATCH] cputlb: Fix io_readx() to respect the access_type

[Shahab Vahedi]

This change adapts io_readx() to its input access_type. Currently
io_readx() treats any memory access as a read, although it has an
input argument "MMUAccessType access_type". This results in:

1) Calling the tlb_fill() only with MMU_DATA_LOAD
2) Considering only entry->addr_read as the tlb_addr

Buglink: https://bugs.launchpad.net/qemu/+bug/1825359

Signed-off-by: Shahab Vahedi <shahab.vahedi@gmail.com>
---
 accel/tcg/cputlb.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 88cc8389e9..0daac0e806 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -878,10 +878,13 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
         CPUTLBEntry *entry;
         target_ulong tlb_addr;
 
-        tlb_fill(cpu, addr, size, MMU_DATA_LOAD, mmu_idx, retaddr);
+        tlb_fill(cpu, addr, size, access_type, mmu_idx, retaddr);
 
         entry = tlb_entry(env, mmu_idx, addr);
-        tlb_addr = entry->addr_read;
+        tlb_addr =
+            (access_type == MMU_DATA_LOAD ) ? entry->addr_read  :
+            (access_type == MMU_DATA_STORE) ? entry->addr_write :
+            entry->addr_code;
         if (!(tlb_addr & ~(TARGET_PAGE_MASK | TLB_RECHECK))) {
             /* RAM access */
             uintptr_t haddr = addr + entry->addend;
-- 
2.21.0

[Qemu-devel] [PATCH] cputlb: Fix io_readx() to respect the access_type

[Shahab Vahedi]

This change adapts io_readx() to its input access_type. Currently
io_readx() treats any memory access as a read, although it has an
input argument "MMUAccessType access_type". This results in:

1) Calling the tlb_fill() only with MMU_DATA_LOAD
2) Considering only entry->addr_read as the tlb_addr

Buglink: https://bugs.launchpad.net/qemu/+bug/1825359

Signed-off-by: Shahab Vahedi <shahab.vahedi@gmail.com>
---
 accel/tcg/cputlb.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 88cc8389e9..0daac0e806 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -878,10 +878,13 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
         CPUTLBEntry *entry;
         target_ulong tlb_addr;
 
-        tlb_fill(cpu, addr, size, MMU_DATA_LOAD, mmu_idx, retaddr);
+        tlb_fill(cpu, addr, size, access_type, mmu_idx, retaddr);
 
         entry = tlb_entry(env, mmu_idx, addr);
-        tlb_addr = entry->addr_read;
+        tlb_addr =
+            (access_type == MMU_DATA_LOAD ) ? entry->addr_read  :
+            (access_type == MMU_DATA_STORE) ? entry->addr_write :
+            entry->addr_code;
         if (!(tlb_addr & ~(TARGET_PAGE_MASK | TLB_RECHECK))) {
             /* RAM access */
             uintptr_t haddr = addr + entry->addend;
-- 
2.21.0

Re: [Qemu-devel] [PATCH] cputlb: Fix io_readx() to respect the access_type

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190419103722.17062-1-shahab.vahedi@gmail.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20190419103722.17062-1-shahab.vahedi@gmail.com
Subject: [Qemu-devel] [PATCH] cputlb: Fix io_readx() to respect the access_type

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]               patchew/20190419103722.17062-1-shahab.vahedi@gmail.com -> patchew/20190419103722.17062-1-shahab.vahedi@gmail.com
Switched to a new branch 'test'
f4446976ec cputlb: Fix io_readx() to respect the access_type

=== OUTPUT BEGIN ===
ERROR: space prohibited before that close parenthesis ')'
#33: FILE: accel/tcg/cputlb.c:885:
+            (access_type == MMU_DATA_LOAD ) ? entry->addr_read  :

total: 1 errors, 0 warnings, 15 lines checked

Commit f4446976ec23 (cputlb: Fix io_readx() to respect the access_type) has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20190419103722.17062-1-shahab.vahedi@gmail.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH] cputlb: Fix io_readx() to respect the access_type

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190419103722.17062-1-shahab.vahedi@gmail.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20190419103722.17062-1-shahab.vahedi@gmail.com
Subject: [Qemu-devel] [PATCH] cputlb: Fix io_readx() to respect the access_type

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]               patchew/20190419103722.17062-1-shahab.vahedi@gmail.com -> patchew/20190419103722.17062-1-shahab.vahedi@gmail.com
Switched to a new branch 'test'
f4446976ec cputlb: Fix io_readx() to respect the access_type

=== OUTPUT BEGIN ===
ERROR: space prohibited before that close parenthesis ')'
#33: FILE: accel/tcg/cputlb.c:885:
+            (access_type == MMU_DATA_LOAD ) ? entry->addr_read  :

total: 1 errors, 0 warnings, 15 lines checked

Commit f4446976ec23 (cputlb: Fix io_readx() to respect the access_type) has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20190419103722.17062-1-shahab.vahedi@gmail.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH] cputlb: Fix io_readx() to respect the access_type

[Alex Bennée]

Shahab Vahedi <shahab.vahedi@gmail.com> writes:

> This change adapts io_readx() to its input access_type. Currently
> io_readx() treats any memory access as a read, although it has an
> input argument "MMUAccessType access_type". This results in:
>
> 1) Calling the tlb_fill() only with MMU_DATA_LOAD
> 2) Considering only entry->addr_read as the tlb_addr
>
> Buglink: https://bugs.launchpad.net/qemu/+bug/1825359

This bug talks about the distinction between DATA_LOAD and INST_FETCH but...

>
> Signed-off-by: Shahab Vahedi <shahab.vahedi@gmail.com>
> ---
>  accel/tcg/cputlb.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index 88cc8389e9..0daac0e806 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -878,10 +878,13 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
>          CPUTLBEntry *entry;
>          target_ulong tlb_addr;
>
> -        tlb_fill(cpu, addr, size, MMU_DATA_LOAD, mmu_idx, retaddr);
> +        tlb_fill(cpu, addr, size, access_type, mmu_idx, retaddr);
>
>          entry = tlb_entry(env, mmu_idx, addr);
> -        tlb_addr = entry->addr_read;
> +        tlb_addr =
> +            (access_type == MMU_DATA_LOAD ) ? entry->addr_read  :
> +            (access_type == MMU_DATA_STORE) ? entry->addr_write :
> +            entry->addr_code;

...why do we care here about MMU_DATA_STORE?

We could just assert (access_type == MMU_DATA_LOAD || access_type ==
MMU_INST_FETCH) and then have:

  (access_type == MMU_DATA_LOAD ) ? entry->addr_read  : entry->addr_code


>          if (!(tlb_addr & ~(TARGET_PAGE_MASK | TLB_RECHECK))) {
>              /* RAM access */
>              uintptr_t haddr = addr + entry->addend;


--
Alex Bennée

Re: [Qemu-devel] [PATCH] cputlb: Fix io_readx() to respect the access_type

[Philippe Mathieu-Daudé]

Hi Alex,

Le sam. 20 avr. 2019 01:05, Alex Bennée <alex.bennee@linaro.org> a écrit :

>
> Shahab Vahedi <shahab.vahedi@gmail.com> writes:
>
> > This change adapts io_readx() to its input access_type. Currently
> > io_readx() treats any memory access as a read, although it has an
> > input argument "MMUAccessType access_type". This results in:
> >
> > 1) Calling the tlb_fill() only with MMU_DATA_LOAD
> > 2) Considering only entry->addr_read as the tlb_addr
> >
> > Buglink: https://bugs.launchpad.net/qemu/+bug/1825359
>
> This bug talks about the distinction between DATA_LOAD and INST_FETCH
> but...
>
> >
> > Signed-off-by: Shahab Vahedi <shahab.vahedi@gmail.com>
> > ---
> >  accel/tcg/cputlb.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> > index 88cc8389e9..0daac0e806 100644
> > --- a/accel/tcg/cputlb.c
> > +++ b/accel/tcg/cputlb.c
> > @@ -878,10 +878,13 @@ static uint64_t io_readx(CPUArchState *env,
> CPUIOTLBEntry *iotlbentry,
> >          CPUTLBEntry *entry;
> >          target_ulong tlb_addr;
> >
> > -        tlb_fill(cpu, addr, size, MMU_DATA_LOAD, mmu_idx, retaddr);
> > +        tlb_fill(cpu, addr, size, access_type, mmu_idx, retaddr);
> >
> >          entry = tlb_entry(env, mmu_idx, addr);
> > -        tlb_addr = entry->addr_read;
> > +        tlb_addr =
> > +            (access_type == MMU_DATA_LOAD ) ? entry->addr_read  :
> > +            (access_type == MMU_DATA_STORE) ? entry->addr_write :
> > +            entry->addr_code;
>
> ...why do we care here about MMU_DATA_STORE?
>
> We could just assert (access_type == MMU_DATA_LOAD || access_type ==
> MMU_INST_FETCH) and then have:
>

Is asserting the best we can do here?


>   (access_type == MMU_DATA_LOAD ) ? entry->addr_read  : entry->addr_code
>
>
> >          if (!(tlb_addr & ~(TARGET_PAGE_MASK | TLB_RECHECK))) {
> >              /* RAM access */
> >              uintptr_t haddr = addr + entry->addend;
>
>
> --
> Alex Bennée
>
>

Re: [Qemu-devel] [PATCH] cputlb: Fix io_readx() to respect the access_type

[Philippe Mathieu-Daudé]

Hi Alex,

Le sam. 20 avr. 2019 01:05, Alex Bennée <alex.bennee@linaro.org> a écrit :

>
> Shahab Vahedi <shahab.vahedi@gmail.com> writes:
>
> > This change adapts io_readx() to its input access_type. Currently
> > io_readx() treats any memory access as a read, although it has an
> > input argument "MMUAccessType access_type". This results in:
> >
> > 1) Calling the tlb_fill() only with MMU_DATA_LOAD
> > 2) Considering only entry->addr_read as the tlb_addr
> >
> > Buglink: https://bugs.launchpad.net/qemu/+bug/1825359
>
> This bug talks about the distinction between DATA_LOAD and INST_FETCH
> but...
>
> >
> > Signed-off-by: Shahab Vahedi <shahab.vahedi@gmail.com>
> > ---
> >  accel/tcg/cputlb.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> > index 88cc8389e9..0daac0e806 100644
> > --- a/accel/tcg/cputlb.c
> > +++ b/accel/tcg/cputlb.c
> > @@ -878,10 +878,13 @@ static uint64_t io_readx(CPUArchState *env,
> CPUIOTLBEntry *iotlbentry,
> >          CPUTLBEntry *entry;
> >          target_ulong tlb_addr;
> >
> > -        tlb_fill(cpu, addr, size, MMU_DATA_LOAD, mmu_idx, retaddr);
> > +        tlb_fill(cpu, addr, size, access_type, mmu_idx, retaddr);
> >
> >          entry = tlb_entry(env, mmu_idx, addr);
> > -        tlb_addr = entry->addr_read;
> > +        tlb_addr =
> > +            (access_type == MMU_DATA_LOAD ) ? entry->addr_read  :
> > +            (access_type == MMU_DATA_STORE) ? entry->addr_write :
> > +            entry->addr_code;
>
> ...why do we care here about MMU_DATA_STORE?
>
> We could just assert (access_type == MMU_DATA_LOAD || access_type ==
> MMU_INST_FETCH) and then have:
>

Is asserting the best we can do here?


>   (access_type == MMU_DATA_LOAD ) ? entry->addr_read  : entry->addr_code
>
>
> >          if (!(tlb_addr & ~(TARGET_PAGE_MASK | TLB_RECHECK))) {
> >              /* RAM access */
> >              uintptr_t haddr = addr + entry->addend;
>
>
> --
> Alex Bennée
>
>