Re: [PATCH 2/3] physmem: Factor out body of flatview_read/write_continue() loop

[Peter Xu <peterx@redhat.com>]

On Thu, Feb 15, 2024 at 02:28:16PM +0000, Jonathan Cameron wrote:
> This code will be reused for the address_space_cached accessors
> shortly.
> 
> Also reduce scope of result variable now we aren't directly
> calling this in the loop.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  system/physmem.c | 165 ++++++++++++++++++++++++++++-------------------
>  1 file changed, 98 insertions(+), 67 deletions(-)
> 
> diff --git a/system/physmem.c b/system/physmem.c
> index 39b5ac751e..74f92bb3b8 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -2677,6 +2677,54 @@ static bool flatview_access_allowed(MemoryRegion *mr, MemTxAttrs attrs,
>      return false;
>  }
>  
> +static MemTxResult flatview_write_continue_step(hwaddr addr,
> +                                                MemTxAttrs attrs,
> +                                                const uint8_t *buf,
> +                                                hwaddr len, hwaddr addr1,
> +                                                hwaddr *l, MemoryRegion *mr)
> +{
> +    if (!flatview_access_allowed(mr, attrs, addr1, *l)) {
> +        return MEMTX_ACCESS_ERROR;
> +    }
> +
> +    if (!memory_access_is_direct(mr, true)) {
> +        uint64_t val;
> +        MemTxResult result;
> +        bool release_lock = prepare_mmio_access(mr);
> +
> +        *l = memory_access_size(mr, *l, addr1);
> +        /* XXX: could force current_cpu to NULL to avoid
> +           potential bugs */
> +
> +        /*
> +         * Assure Coverity (and ourselves) that we are not going to OVERRUN
> +         * the buffer by following ldn_he_p().
> +         */
> +#ifdef QEMU_STATIC_ANALYSIS
> +        assert((*l == 1 && len >= 1) ||
> +               (*l == 2 && len >= 2) ||
> +               (*l == 4 && len >= 4) ||
> +               (*l == 8 && len >= 8));
> +#endif
> +        val = ldn_he_p(buf, *l);
> +        result = memory_region_dispatch_write(mr, addr1, val,
> +                                              size_memop(*l), attrs);
> +        if (release_lock) {
> +            bql_unlock();
> +        }
> +
> +        return result;
> +    } else {
> +        /* RAM case */
> +        uint8_t *ram_ptr = qemu_ram_ptr_length(mr->ram_block, addr1, l, false);
> +
> +        memmove(ram_ptr, buf, *l);
> +        invalidate_and_set_dirty(mr, addr1, *l);
> +
> +        return MEMTX_OK;
> +    }
> +}
> +
>  /* Called within RCU critical section.  */
>  static MemTxResult flatview_write_continue(FlatView *fv, hwaddr addr,
>                                             MemTxAttrs attrs,
> @@ -2688,42 +2736,9 @@ static MemTxResult flatview_write_continue(FlatView *fv, hwaddr addr,
>      const uint8_t *buf = ptr;
>  
>      for (;;) {
> -        if (!flatview_access_allowed(mr, attrs, addr1, l)) {
> -            result |= MEMTX_ACCESS_ERROR;
> -            /* Keep going. */
> -        } else if (!memory_access_is_direct(mr, true)) {
> -            uint64_t val;
> -            bool release_lock = prepare_mmio_access(mr);
> -
> -            l = memory_access_size(mr, l, addr1);
> -            /* XXX: could force current_cpu to NULL to avoid
> -               potential bugs */
> -
> -            /*
> -             * Assure Coverity (and ourselves) that we are not going to OVERRUN
> -             * the buffer by following ldn_he_p().
> -             */
> -#ifdef QEMU_STATIC_ANALYSIS
> -            assert((l == 1 && len >= 1) ||
> -                   (l == 2 && len >= 2) ||
> -                   (l == 4 && len >= 4) ||
> -                   (l == 8 && len >= 8));
> -#endif
> -            val = ldn_he_p(buf, l);
> -            result |= memory_region_dispatch_write(mr, addr1, val,
> -                                                   size_memop(l), attrs);
> -            if (release_lock) {
> -                bql_unlock();
> -            }
> -
>  
> -        } else {
> -            /* RAM case */
> -            uint8_t *ram_ptr = qemu_ram_ptr_length(mr->ram_block, addr1, &l,
> -                                                   false);
> -            memmove(ram_ptr, buf, l);
> -            invalidate_and_set_dirty(mr, addr1, l);
> -        }
> +        result |= flatview_write_continue_step(addr, attrs, buf, len, addr1, &l,
> +                                               mr);
>  
>          len -= l;
>          buf += l;
> @@ -2757,6 +2772,52 @@ static MemTxResult flatview_write(FlatView *fv, hwaddr addr, MemTxAttrs attrs,
>                                     addr1, l, mr);
>  }
>  
> +static MemTxResult flatview_read_continue_step(hwaddr addr,
> +                                               MemTxAttrs attrs, uint8_t *buf,
> +                                               hwaddr len, hwaddr addr1,
> +                                               hwaddr *l,
> +                                               MemoryRegion *mr)
> +{
> +    if (!flatview_access_allowed(mr, attrs, addr1, *l)) {
> +        return  MEMTX_ACCESS_ERROR;
                  |
                  ^ space

> +    }
> +
> +    if (!memory_access_is_direct(mr, false)) {
> +        /* I/O case */
> +        uint64_t val;
> +        MemTxResult result;
> +        bool release_lock = prepare_mmio_access(mr);
> +
> +        *l = memory_access_size(mr, *l, addr1);
> +        result = memory_region_dispatch_read(mr, addr1, &val,
> +                                                  size_memop(*l), attrs);

Please do proper indents.

Other than that:

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu