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

[Peter Xu <peterx@redhat.com>]

On Fri, Mar 01, 2024 at 01:29:04PM +0800, Peter Xu wrote:
> 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,

One more thing: this addr var is not used, afaict.  We could drop addr1
below and use this to represent the MR offset.

I'm wondering whether we should start to use some better namings already
for memory API functions to show obviously what AS it is describing.  From
that POV, perhaps rename it to "mr_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

-- 
Peter Xu