accel/tcg/cputlb.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
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
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
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
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
© 2016 - 2024 Red Hat, Inc.