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 <address@hidden> 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 <address@hidden>
> ---
> 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