| Message ID | 20240215142817.1904-3-Jonathan.Cameron@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | physmem: Fix MemoryRegion for second access to cached MMIO Address Space | expand |
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>
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
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 >
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; + } + + 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); + + /* + * Assure Coverity (and ourselves) that we are not going to OVERRUN + * the buffer by following stn_he_p(). + */ +#ifdef QEMU_STATIC_ANALYSIS + assert((*l == 1 && len >= 1) || + (*l == 2 && len >= 2) || + (*l == 4 && len >= 4) || + (*l == 8 && len >= 8)); +#endif + stn_he_p(buf, *l, val); + + if (release_lock) { + bql_unlock(); + } + return result; + } else { + /* RAM case */ + uint8_t *ram_ptr = qemu_ram_ptr_length(mr->ram_block, addr1, l, false); + + memcpy(buf, ram_ptr, *l); + + return MEMTX_OK; + } +} + /* Called within RCU critical section. */ MemTxResult flatview_read_continue(FlatView *fv, hwaddr addr, MemTxAttrs attrs, void *ptr, @@ -2768,38 +2829,8 @@ MemTxResult flatview_read_continue(FlatView *fv, hwaddr addr, fuzz_dma_read_cb(addr, len, mr); for (;;) { - if (!flatview_access_allowed(mr, attrs, addr1, l)) { - result |= MEMTX_ACCESS_ERROR; - /* Keep going. */ - } else if (!memory_access_is_direct(mr, false)) { - /* I/O case */ - uint64_t val; - 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); - - /* - * Assure Coverity (and ourselves) that we are not going to OVERRUN - * the buffer by following stn_he_p(). - */ -#ifdef QEMU_STATIC_ANALYSIS - assert((l == 1 && len >= 1) || - (l == 2 && len >= 2) || - (l == 4 && len >= 4) || - (l == 8 && len >= 8)); -#endif - stn_he_p(buf, l, val); - if (release_lock) { - bql_unlock(); - } - } else { - /* RAM case */ - uint8_t *ram_ptr = qemu_ram_ptr_length(mr->ram_block, addr1, &l, - false); - memcpy(buf, ram_ptr, l); - } + result |= flatview_read_continue_step(addr, attrs, buf, + len, addr1, &l, mr); len -= l; buf += l;
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(-)