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.vah...@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.vah...@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
