* [PATCH 1/3] physmem: Reduce local variable scope in flatview_read/write_continue()
2024-02-15 14:28 [PATCH 0/3] physmem: Fix MemoryRegion for second access to cached MMIO Address Space Jonathan Cameron via
@ 2024-02-15 14:28 ` Jonathan Cameron via
2024-03-01 5:26 ` Peter Xu
2024-02-15 14:28 ` [PATCH 2/3] physmem: Factor out body of flatview_read/write_continue() loop Jonathan Cameron via
` (2 subsequent siblings)
3 siblings, 1 reply; 11+ messages in thread
From: Jonathan Cameron via @ 2024-02-15 14:28 UTC (permalink / raw)
To: Paolo Bonzini, Peter Xu, David Hildenbrand,
Philippe Mathieu-Daudé, qemu-devel
Cc: linuxarm
Precursor to factoring out the inner loops for reuse.
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
system/physmem.c | 38 ++++++++++++++++++--------------------
1 file changed, 18 insertions(+), 20 deletions(-)
diff --git a/system/physmem.c b/system/physmem.c
index 6a3c9de512..39b5ac751e 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -2684,10 +2684,7 @@ static MemTxResult flatview_write_continue(FlatView *fv, hwaddr addr,
hwaddr len, hwaddr addr1,
hwaddr l, MemoryRegion *mr)
{
- uint8_t *ram_ptr;
- uint64_t val;
MemTxResult result = MEMTX_OK;
- bool release_lock = false;
const uint8_t *buf = ptr;
for (;;) {
@@ -2695,7 +2692,9 @@ static MemTxResult flatview_write_continue(FlatView *fv, hwaddr addr,
result |= MEMTX_ACCESS_ERROR;
/* Keep going. */
} else if (!memory_access_is_direct(mr, true)) {
- release_lock |= prepare_mmio_access(mr);
+ 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 */
@@ -2713,18 +2712,19 @@ static MemTxResult flatview_write_continue(FlatView *fv, hwaddr addr,
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 */
- ram_ptr = qemu_ram_ptr_length(mr->ram_block, addr1, &l, false);
+ 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);
}
- if (release_lock) {
- bql_unlock();
- release_lock = false;
- }
-
len -= l;
buf += l;
addr += l;
@@ -2763,10 +2763,7 @@ MemTxResult flatview_read_continue(FlatView *fv, hwaddr addr,
hwaddr len, hwaddr addr1, hwaddr l,
MemoryRegion *mr)
{
- uint8_t *ram_ptr;
- uint64_t val;
MemTxResult result = MEMTX_OK;
- bool release_lock = false;
uint8_t *buf = ptr;
fuzz_dma_read_cb(addr, len, mr);
@@ -2776,7 +2773,9 @@ MemTxResult flatview_read_continue(FlatView *fv, hwaddr addr,
/* Keep going. */
} else if (!memory_access_is_direct(mr, false)) {
/* I/O case */
- release_lock |= prepare_mmio_access(mr);
+ 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);
@@ -2792,17 +2791,16 @@ MemTxResult flatview_read_continue(FlatView *fv, hwaddr addr,
(l == 8 && len >= 8));
#endif
stn_he_p(buf, l, val);
+ if (release_lock) {
+ bql_unlock();
+ }
} else {
/* RAM case */
- ram_ptr = qemu_ram_ptr_length(mr->ram_block, addr1, &l, false);
+ uint8_t *ram_ptr = qemu_ram_ptr_length(mr->ram_block, addr1, &l,
+ false);
memcpy(buf, ram_ptr, l);
}
- if (release_lock) {
- bql_unlock();
- release_lock = false;
- }
-
len -= l;
buf += l;
addr += l;
--
2.39.2
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 2/3] physmem: Factor out body of flatview_read/write_continue() loop
2024-02-15 14:28 [PATCH 0/3] physmem: Fix MemoryRegion for second access to cached MMIO Address Space Jonathan Cameron via
2024-02-15 14:28 ` [PATCH 1/3] physmem: Reduce local variable scope in flatview_read/write_continue() Jonathan Cameron via
@ 2024-02-15 14:28 ` Jonathan Cameron via
2024-03-01 5:29 ` Peter Xu
2024-02-15 14:28 ` [PATCH 3/3] physmem: Fix wrong MR in large address_space_read/write_cached_slow() Jonathan Cameron via
2024-02-29 10:49 ` [PATCH 0/3] physmem: Fix MemoryRegion for second access to cached MMIO Address Space Jonathan Cameron via
3 siblings, 1 reply; 11+ messages in thread
From: Jonathan Cameron via @ 2024-02-15 14:28 UTC (permalink / raw)
To: Paolo Bonzini, Peter Xu, David Hildenbrand,
Philippe Mathieu-Daudé, qemu-devel
Cc: linuxarm
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;
+ }
+
+ 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;
--
2.39.2
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 2/3] physmem: Factor out body of flatview_read/write_continue() loop
2024-02-15 14:28 ` [PATCH 2/3] physmem: Factor out body of flatview_read/write_continue() loop Jonathan Cameron via
@ 2024-03-01 5:29 ` Peter Xu
2024-03-01 5:35 ` Peter Xu
0 siblings, 1 reply; 11+ messages in thread
From: Peter Xu @ 2024-03-01 5:29 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Paolo Bonzini, David Hildenbrand, Philippe Mathieu-Daudé,
qemu-devel, linuxarm
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 2/3] physmem: Factor out body of flatview_read/write_continue() loop
2024-03-01 5:29 ` Peter Xu
@ 2024-03-01 5:35 ` Peter Xu
2024-03-07 14:09 ` Jonathan Cameron via
0 siblings, 1 reply; 11+ messages in thread
From: Peter Xu @ 2024-03-01 5:35 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Paolo Bonzini, David Hildenbrand, Philippe Mathieu-Daudé,
qemu-devel, linuxarm
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 2/3] physmem: Factor out body of flatview_read/write_continue() loop
2024-03-01 5:35 ` Peter Xu
@ 2024-03-07 14:09 ` Jonathan Cameron via
0 siblings, 0 replies; 11+ messages in thread
From: Jonathan Cameron via @ 2024-03-07 14:09 UTC (permalink / raw)
To: Peter Xu
Cc: Paolo Bonzini, David Hildenbrand, Philippe Mathieu-Daudé,
qemu-devel, linuxarm
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
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/3] physmem: Fix wrong MR in large address_space_read/write_cached_slow()
2024-02-15 14:28 [PATCH 0/3] physmem: Fix MemoryRegion for second access to cached MMIO Address Space Jonathan Cameron via
2024-02-15 14:28 ` [PATCH 1/3] physmem: Reduce local variable scope in flatview_read/write_continue() Jonathan Cameron via
2024-02-15 14:28 ` [PATCH 2/3] physmem: Factor out body of flatview_read/write_continue() loop Jonathan Cameron via
@ 2024-02-15 14:28 ` Jonathan Cameron via
2024-03-01 5:44 ` Peter Xu
2024-02-29 10:49 ` [PATCH 0/3] physmem: Fix MemoryRegion for second access to cached MMIO Address Space Jonathan Cameron via
3 siblings, 1 reply; 11+ messages in thread
From: Jonathan Cameron via @ 2024-02-15 14:28 UTC (permalink / raw)
To: Paolo Bonzini, Peter Xu, David Hildenbrand,
Philippe Mathieu-Daudé, qemu-devel
Cc: linuxarm
If the access is bigger than the MemoryRegion supports,
flatview_read/write_continue() will attempt to update the Memory Region.
but the address passed to flatview_translate() is relative to the cache, not
to the FlatView.
On arm/virt with interleaved CXL memory emulation and virtio-blk-pci this
lead to the first part of descriptor being read from the CXL memory and the
second part from PA 0x8 which happens to be a blank region
of a flash chip and all ffs on this particular configuration.
Note this test requires the out of tree ARM support for CXL, but
the problem is more general.
Avoid this by adding new address_space_read_continue_cached()
and address_space_write_continue_cached() which share all the logic
with the flatview versions except for the MemoryRegion lookup.
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
system/physmem.c | 78 ++++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 72 insertions(+), 6 deletions(-)
diff --git a/system/physmem.c b/system/physmem.c
index 74f92bb3b8..43b37942cf 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -3377,6 +3377,72 @@ static inline MemoryRegion *address_space_translate_cached(
return section.mr;
}
+/* Called within RCU critical section. */
+static MemTxResult address_space_write_continue_cached(MemoryRegionCache *cache,
+ hwaddr addr,
+ MemTxAttrs attrs,
+ const void *ptr,
+ hwaddr len, hwaddr addr1,
+ hwaddr l,
+ MemoryRegion *mr)
+{
+ MemTxResult result = MEMTX_OK;
+ const uint8_t *buf = ptr;
+
+ for (;;) {
+
+ result |= flatview_write_continue_step(addr, attrs, buf, len, addr1, &l,
+ mr);
+
+ len -= l;
+ buf += l;
+ addr += l;
+
+ if (!len) {
+ break;
+ }
+
+ l = len;
+
+ mr = address_space_translate_cached(cache, addr, &addr1, &l, true,
+ attrs);
+ }
+
+ return result;
+}
+
+/* Called within RCU critical section. */
+static MemTxResult address_space_read_continue_cached(MemoryRegionCache *cache,
+ hwaddr addr,
+ MemTxAttrs attrs,
+ void *ptr, hwaddr len,
+ hwaddr addr1, hwaddr l,
+ MemoryRegion *mr)
+{
+ MemTxResult result = MEMTX_OK;
+ uint8_t *buf = ptr;
+
+ fuzz_dma_read_cb(addr, len, mr);
+ for (;;) {
+
+ result |= flatview_read_continue_step(addr, attrs, buf, len, addr1,
+ &l, mr);
+ len -= l;
+ buf += l;
+ addr += l;
+
+ if (!len) {
+ break;
+ }
+ l = len;
+
+ mr = address_space_translate_cached(cache, addr, &addr1, &l, false,
+ attrs);
+ }
+
+ return result;
+}
+
/* Called from RCU critical section. address_space_read_cached uses this
* out of line function when the target is an MMIO or IOMMU region.
*/
@@ -3390,9 +3456,9 @@ address_space_read_cached_slow(MemoryRegionCache *cache, hwaddr addr,
l = len;
mr = address_space_translate_cached(cache, addr, &addr1, &l, false,
MEMTXATTRS_UNSPECIFIED);
- return flatview_read_continue(cache->fv,
- addr, MEMTXATTRS_UNSPECIFIED, buf, len,
- addr1, l, mr);
+ return address_space_read_continue_cached(cache, addr,
+ MEMTXATTRS_UNSPECIFIED, buf, len,
+ addr1, l, mr);
}
/* Called from RCU critical section. address_space_write_cached uses this
@@ -3408,9 +3474,9 @@ address_space_write_cached_slow(MemoryRegionCache *cache, hwaddr addr,
l = len;
mr = address_space_translate_cached(cache, addr, &addr1, &l, true,
MEMTXATTRS_UNSPECIFIED);
- return flatview_write_continue(cache->fv,
- addr, MEMTXATTRS_UNSPECIFIED, buf, len,
- addr1, l, mr);
+ return address_space_write_continue_cached(cache, addr,
+ MEMTXATTRS_UNSPECIFIED,
+ buf, len, addr1, l, mr);
}
#define ARG1_DECL MemoryRegionCache *cache
--
2.39.2
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 3/3] physmem: Fix wrong MR in large address_space_read/write_cached_slow()
2024-02-15 14:28 ` [PATCH 3/3] physmem: Fix wrong MR in large address_space_read/write_cached_slow() Jonathan Cameron via
@ 2024-03-01 5:44 ` Peter Xu
2024-03-07 14:51 ` Jonathan Cameron via
0 siblings, 1 reply; 11+ messages in thread
From: Peter Xu @ 2024-03-01 5:44 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Paolo Bonzini, David Hildenbrand, Philippe Mathieu-Daudé,
qemu-devel, linuxarm
On Thu, Feb 15, 2024 at 02:28:17PM +0000, Jonathan Cameron wrote:
Can we rename the subject?
physmem: Fix wrong MR in large address_space_read/write_cached_slow()
IMHO "wrong MR" is misleading, as the MR was wrong only because the address
passed over is wrong at the first place. Perhaps s/MR/addr/?
> If the access is bigger than the MemoryRegion supports,
> flatview_read/write_continue() will attempt to update the Memory Region.
> but the address passed to flatview_translate() is relative to the cache, not
> to the FlatView.
>
> On arm/virt with interleaved CXL memory emulation and virtio-blk-pci this
> lead to the first part of descriptor being read from the CXL memory and the
> second part from PA 0x8 which happens to be a blank region
> of a flash chip and all ffs on this particular configuration.
> Note this test requires the out of tree ARM support for CXL, but
> the problem is more general.
>
> Avoid this by adding new address_space_read_continue_cached()
> and address_space_write_continue_cached() which share all the logic
> with the flatview versions except for the MemoryRegion lookup.
>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> system/physmem.c | 78 ++++++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 72 insertions(+), 6 deletions(-)
>
[...]
> +/* Called within RCU critical section. */
> +static MemTxResult address_space_read_continue_cached(MemoryRegionCache *cache,
> + hwaddr addr,
> + MemTxAttrs attrs,
> + void *ptr, hwaddr len,
> + hwaddr addr1, hwaddr l,
> + MemoryRegion *mr)
It looks like "addr" (of flatview AS) is not needed for a cached RW (see
below), because we should have a stable (cached) MR to operate anyway?
How about we also use "mr_addr" as the single addr of the MR, then drop
addr1?
> +{
> + MemTxResult result = MEMTX_OK;
> + uint8_t *buf = ptr;
> +
> + fuzz_dma_read_cb(addr, len, mr);
> + for (;;) {
> +
Remove empty line?
> + result |= flatview_read_continue_step(addr, attrs, buf, len, addr1,
> + &l, mr);
> + len -= l;
> + buf += l;
> + addr += l;
> +
> + if (!len) {
> + break;
> + }
> + l = len;
> +
> + mr = address_space_translate_cached(cache, addr, &addr1, &l, false,
> + attrs);
Here IIUC the mr will always be the same as before? If so, maybe "mr_addr
+= l" should be enough?
(similar comment applies to the writer side too)
> + }
> +
> + return result;
> +}
> +
> /* Called from RCU critical section. address_space_read_cached uses this
> * out of line function when the target is an MMIO or IOMMU region.
> */
> @@ -3390,9 +3456,9 @@ address_space_read_cached_slow(MemoryRegionCache *cache, hwaddr addr,
> l = len;
> mr = address_space_translate_cached(cache, addr, &addr1, &l, false,
> MEMTXATTRS_UNSPECIFIED);
> - return flatview_read_continue(cache->fv,
> - addr, MEMTXATTRS_UNSPECIFIED, buf, len,
> - addr1, l, mr);
> + return address_space_read_continue_cached(cache, addr,
> + MEMTXATTRS_UNSPECIFIED, buf, len,
> + addr1, l, mr);
> }
>
> /* Called from RCU critical section. address_space_write_cached uses this
> @@ -3408,9 +3474,9 @@ address_space_write_cached_slow(MemoryRegionCache *cache, hwaddr addr,
> l = len;
> mr = address_space_translate_cached(cache, addr, &addr1, &l, true,
> MEMTXATTRS_UNSPECIFIED);
> - return flatview_write_continue(cache->fv,
> - addr, MEMTXATTRS_UNSPECIFIED, buf, len,
> - addr1, l, mr);
> + return address_space_write_continue_cached(cache, addr,
> + MEMTXATTRS_UNSPECIFIED,
> + buf, len, addr1, l, mr);
> }
>
> #define ARG1_DECL MemoryRegionCache *cache
> --
> 2.39.2
>
--
Peter Xu
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 3/3] physmem: Fix wrong MR in large address_space_read/write_cached_slow()
2024-03-01 5:44 ` Peter Xu
@ 2024-03-07 14:51 ` Jonathan Cameron via
0 siblings, 0 replies; 11+ messages in thread
From: Jonathan Cameron via @ 2024-03-07 14:51 UTC (permalink / raw)
To: Peter Xu
Cc: Paolo Bonzini, David Hildenbrand, Philippe Mathieu-Daudé,
qemu-devel, linuxarm
On Fri, 1 Mar 2024 13:44:01 +0800
Peter Xu <peterx@redhat.com> wrote:
> On Thu, Feb 15, 2024 at 02:28:17PM +0000, Jonathan Cameron wrote:
>
> Can we rename the subject?
>
> physmem: Fix wrong MR in large address_space_read/write_cached_slow()
>
> IMHO "wrong MR" is misleading, as the MR was wrong only because the address
> passed over is wrong at the first place. Perhaps s/MR/addr/?
>
> > If the access is bigger than the MemoryRegion supports,
> > flatview_read/write_continue() will attempt to update the Memory Region.
> > but the address passed to flatview_translate() is relative to the cache, not
> > to the FlatView.
> >
> > On arm/virt with interleaved CXL memory emulation and virtio-blk-pci this
> > lead to the first part of descriptor being read from the CXL memory and the
> > second part from PA 0x8 which happens to be a blank region
> > of a flash chip and all ffs on this particular configuration.
> > Note this test requires the out of tree ARM support for CXL, but
> > the problem is more general.
> >
> > Avoid this by adding new address_space_read_continue_cached()
> > and address_space_write_continue_cached() which share all the logic
> > with the flatview versions except for the MemoryRegion lookup.
> >
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---
> > system/physmem.c | 78 ++++++++++++++++++++++++++++++++++++++++++++----
> > 1 file changed, 72 insertions(+), 6 deletions(-)
> >
>
> [...]
>
> > +/* Called within RCU critical section. */
> > +static MemTxResult address_space_read_continue_cached(MemoryRegionCache *cache,
> > + hwaddr addr,
> > + MemTxAttrs attrs,
> > + void *ptr, hwaddr len,
> > + hwaddr addr1, hwaddr l,
> > + MemoryRegion *mr)
>
> It looks like "addr" (of flatview AS) is not needed for a cached RW (see
> below), because we should have a stable (cached) MR to operate anyway?
>
> How about we also use "mr_addr" as the single addr of the MR, then drop
> addr1?
Agreed, but also need to drop the fuzz_dma_read_cb().
However given the first thing that is checked by the only in tree fuzzing
code is whether we are dealing with RAM, I think that's fine.
>
> > +{
> > + MemTxResult result = MEMTX_OK;
> > + uint8_t *buf = ptr;
> > +
> > + fuzz_dma_read_cb(addr, len, mr);
> > + for (;;) {
> > +
>
> Remove empty line?
>
> > + result |= flatview_read_continue_step(addr, attrs, buf, len, addr1,
> > + &l, mr);
> > + len -= l;
> > + buf += l;
> > + addr += l;
> > +
> > + if (!len) {
> > + break;
> > + }
> > + l = len;
> > +
> > + mr = address_space_translate_cached(cache, addr, &addr1, &l, false,
> > + attrs);
>
> Here IIUC the mr will always be the same as before? If so, maybe "mr_addr
> += l" should be enough?
>
I had the same thought originally but couldn't convince myself that there
was no route to end up with a different MR here. I don't yet
have a good enough grip on how this all fits together so I particularly
appreciate your help.
With hindsight I should have called this out as a question in this patch set.
Can drop passing in cache as well given it is no longer used within
this function.
Thanks,
Jonathan
> (similar comment applies to the writer side too)
>
> > + }
> > +
> > + return result;
> > +}
> > +
> > /* Called from RCU critical section. address_space_read_cached uses this
> > * out of line function when the target is an MMIO or IOMMU region.
> > */
> > @@ -3390,9 +3456,9 @@ address_space_read_cached_slow(MemoryRegionCache *cache, hwaddr addr,
> > l = len;
> > mr = address_space_translate_cached(cache, addr, &addr1, &l, false,
> > MEMTXATTRS_UNSPECIFIED);
> > - return flatview_read_continue(cache->fv,
> > - addr, MEMTXATTRS_UNSPECIFIED, buf, len,
> > - addr1, l, mr);
> > + return address_space_read_continue_cached(cache, addr,
> > + MEMTXATTRS_UNSPECIFIED, buf, len,
> > + addr1, l, mr);
> > }
> >
> > /* Called from RCU critical section. address_space_write_cached uses this
> > @@ -3408,9 +3474,9 @@ address_space_write_cached_slow(MemoryRegionCache *cache, hwaddr addr,
> > l = len;
> > mr = address_space_translate_cached(cache, addr, &addr1, &l, true,
> > MEMTXATTRS_UNSPECIFIED);
> > - return flatview_write_continue(cache->fv,
> > - addr, MEMTXATTRS_UNSPECIFIED, buf, len,
> > - addr1, l, mr);
> > + return address_space_write_continue_cached(cache, addr,
> > + MEMTXATTRS_UNSPECIFIED,
> > + buf, len, addr1, l, mr);
> > }
> >
> > #define ARG1_DECL MemoryRegionCache *cache
> > --
> > 2.39.2
> >
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/3] physmem: Fix MemoryRegion for second access to cached MMIO Address Space
2024-02-15 14:28 [PATCH 0/3] physmem: Fix MemoryRegion for second access to cached MMIO Address Space Jonathan Cameron via
` (2 preceding siblings ...)
2024-02-15 14:28 ` [PATCH 3/3] physmem: Fix wrong MR in large address_space_read/write_cached_slow() Jonathan Cameron via
@ 2024-02-29 10:49 ` Jonathan Cameron via
3 siblings, 0 replies; 11+ messages in thread
From: Jonathan Cameron via @ 2024-02-29 10:49 UTC (permalink / raw)
To: Jonathan Cameron via, Paolo Bonzini, Peter Xu, David Hildenbrand,
Philippe Mathieu-Daudé
Cc: Jonathan Cameron, linuxarm
On Thu, 15 Feb 2024 14:28:14 +0000
Jonathan Cameron via <qemu-devel@nongnu.org> wrote:
Any comments? Almost all the other fixes I need for CXL memory to
work as normal ram are queued up so I'd love it if we can solve this one as
well.
This looks like a big series, but it's really just a refactor + trivial
addition - so shouldn't be too scary!
Jonathan
> Issue seen testing virtio-blk-pci with CXL emulated interleave memory.
> Tests were done on arm64, but the issue isn't architecture specific.
> Note that some additional fixes are needed to TCG to be able to run far
> enough to hit this on arm64 or x86. They are issues so I'll post separate
> series shortly.
>
> The address_space_read_cached_slow() and address_space_write_cached_slow()
> functions query the MemoryRegion for the cached address space correctly
> using address_space_translate_cached() but then call into
> flatview_read_continue() / flatview_write_continue()
> If the access is to a MMIO MemoryRegion and is bigger than the MemoryRegion
> supports, the loop will query the MemoryRegion for the next access to use.
> That query uses flatview_translate() but the address passed is suitable
> for the cache, not the flatview. On my test setup that mean the second
> 8 bytes and onwards of the virtio descriptor was read from flash memory
> at the beginning of the system address map, not the CXL emulated memory
> where the descriptor was found. Result happened to be all fs so easy to
> spot.
>
> Changes these calls to use address_space_translate_cached() to get the
> correct MemoryRegion for the cache. To avoid duplicating most of the
> code, the first 2 patches factor out the common parts of
> flatview_read_continue() and flatview_write_continue() so they can
> be reused.
>
> Write path has not been tested but it so similar to the read path I've
> included it here.
>
> Jonathan Cameron (3):
> physmem: Reduce local variable scope in flatview_read/write_continue()
> physmem: Factor out body of flatview_read/write_continue() loop
> physmem: Fix wrong MR in large address_space_read/write_cached_slow()
>
> system/physmem.c | 245 ++++++++++++++++++++++++++++++++---------------
> 1 file changed, 170 insertions(+), 75 deletions(-)
>
^ permalink raw reply [flat|nested] 11+ messages in thread