1. Patchew
  2. QEMU
  3. View series

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

Shahab Vahedi posted 1 patch 4 years, 12 months ago
Test checkpatch failed
Test asan passed
Test docker-clang@ubuntu passed
Test docker-mingw@fedora passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20190419103722.17062-1-shahab.vahedi@gmail.com
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, Richard Henderson <rth@twiddle.net>
There is a newer version of this series
accel/tcg/cputlb.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
[Qemu-devel] [PATCH] cputlb: Fix io_readx() to respect the access_type
Posted by Shahab Vahedi 4 years, 12 months ago 
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
Posted by Alex Bennée 4 years, 12 months ago 
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
Posted by Philippe Mathieu-Daudé 4 years, 12 months ago 
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
Posted by no-reply@patchew.org 4 years, 12 months ago 
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

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.