[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 2/3] physmem: Factor out body of flatview_read/write_continue
|
From: |
Jonathan Cameron |
|
Subject: |
Re: [PATCH 2/3] physmem: Factor out body of flatview_read/write_continue() loop |
|
Date: |
Thu, 7 Mar 2024 14:09:11 +0000 |
On Fri, 1 Mar 2024 13:35:26 +0800
Peter Xu <peterx@redhat.com> wrote:
> 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 tempted to keep the addr1 where it is in the parameter list just so that
it matches up with the caller location but a rename makes a lot of sense.
>
> 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"?
I'll add a precursor patch renaming these for the functions this series touches.
We can tidy up other cases later. I'll put a note in that patch below the cut
to observe that the rename makes sense more widely.
I've not picked up the RB given because of the parameter ordering question.
Thanks,
Jonathan
>
> > > + 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
>