| Message ID | 20210415100409.3977971-2-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/elf_ops: clear uninitialized segment space | expand |
Le 15/04/2021 à 12:04, Philippe Mathieu-Daudé a écrit : > dma_memory_set() does a DMA barrier, set the address space with > a constant value. The constant value filling code is not specific > to DMA and can be used for AddressSpace. Extract it as a new > helper: address_space_set(). > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > include/exec/memory.h | 16 ++++++++++++++++ > softmmu/dma-helpers.c | 16 +--------------- > softmmu/physmem.c | 19 +++++++++++++++++++ > 3 files changed, 36 insertions(+), 15 deletions(-) > > diff --git a/include/exec/memory.h b/include/exec/memory.h > index 5728a681b27..192139af58e 100644 > --- a/include/exec/memory.h > +++ b/include/exec/memory.h > @@ -2568,6 +2568,22 @@ address_space_write_cached(MemoryRegionCache *cache, hwaddr addr, > } > } > > +/** > + * address_space_set: Fill address space with a constant byte. > + * > + * Return a MemTxResult indicating whether the operation succeeded > + * or failed (eg unassigned memory, device rejected the transaction, > + * IOMMU fault). > + * > + * @as: #AddressSpace to be accessed > + * @addr: address within that address space > + * @c: constant byte to fill the memory > + * @len: the number of bytes to fill with the constant byte > + * @attrs: memory transaction attributes > + */ > +MemTxResult address_space_set(AddressSpace *as, hwaddr addr, > + uint8_t c, hwaddr len, MemTxAttrs attrs); > + > #ifdef NEED_CPU_H > /* enum device_endian to MemOp. */ > static inline MemOp devend_memop(enum device_endian end) > diff --git a/softmmu/dma-helpers.c b/softmmu/dma-helpers.c > index 7d766a5e89a..8e1e7ad5320 100644 > --- a/softmmu/dma-helpers.c > +++ b/softmmu/dma-helpers.c > @@ -23,21 +23,7 @@ MemTxResult dma_memory_set(AddressSpace *as, dma_addr_t addr, > { > dma_barrier(as, DMA_DIRECTION_FROM_DEVICE); > > -#define FILLBUF_SIZE 512 > - uint8_t fillbuf[FILLBUF_SIZE]; > - int l; > - MemTxResult error = MEMTX_OK; > - > - memset(fillbuf, c, FILLBUF_SIZE); > - while (len > 0) { > - l = len < FILLBUF_SIZE ? len : FILLBUF_SIZE; > - error |= address_space_write(as, addr, MEMTXATTRS_UNSPECIFIED, > - fillbuf, l); > - len -= l; > - addr += l; > - } > - > - return error; > + return address_space_set(as, addr, c, len, MEMTXATTRS_UNSPECIFIED); > } > > void qemu_sglist_init(QEMUSGList *qsg, DeviceState *dev, int alloc_hint, > diff --git a/softmmu/physmem.c b/softmmu/physmem.c > index 85034d9c11e..c9117527ae7 100644 > --- a/softmmu/physmem.c > +++ b/softmmu/physmem.c > @@ -2891,6 +2891,25 @@ MemTxResult address_space_rw(AddressSpace *as, hwaddr addr, MemTxAttrs attrs, > } > } > > +MemTxResult address_space_set(AddressSpace *as, hwaddr addr, > + uint8_t c, hwaddr len, MemTxAttrs attrs) > +{ > +#define FILLBUF_SIZE 512 > + uint8_t fillbuf[FILLBUF_SIZE]; > + int l; > + MemTxResult error = MEMTX_OK; > + > + memset(fillbuf, c, FILLBUF_SIZE); > + while (len > 0) { > + l = len < FILLBUF_SIZE ? len : FILLBUF_SIZE; > + error |= address_space_write(as, addr, attrs, fillbuf, l); > + len -= l; > + addr += l; > + } > + > + return error; > +} > + > void cpu_physical_memory_rw(hwaddr addr, void *buf, > hwaddr len, bool is_write) > { > Reviewed-by: Laurent Vivier <laurent@vivier.eu>
On Thu, Apr 15, 2021 at 12:04:08PM +0200, Philippe Mathieu-Daudé wrote: >dma_memory_set() does a DMA barrier, set the address space with >a constant value. The constant value filling code is not specific >to DMA and can be used for AddressSpace. Extract it as a new >helper: address_space_set(). > >Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >--- > include/exec/memory.h | 16 ++++++++++++++++ > softmmu/dma-helpers.c | 16 +--------------- > softmmu/physmem.c | 19 +++++++++++++++++++ > 3 files changed, 36 insertions(+), 15 deletions(-) > >diff --git a/include/exec/memory.h b/include/exec/memory.h >index 5728a681b27..192139af58e 100644 >--- a/include/exec/memory.h >+++ b/include/exec/memory.h >@@ -2568,6 +2568,22 @@ address_space_write_cached(MemoryRegionCache *cache, hwaddr addr, > } > } > >+/** >+ * address_space_set: Fill address space with a constant byte. >+ * >+ * Return a MemTxResult indicating whether the operation succeeded >+ * or failed (eg unassigned memory, device rejected the transaction, >+ * IOMMU fault). >+ * >+ * @as: #AddressSpace to be accessed >+ * @addr: address within that address space >+ * @c: constant byte to fill the memory >+ * @len: the number of bytes to fill with the constant byte >+ * @attrs: memory transaction attributes >+ */ >+MemTxResult address_space_set(AddressSpace *as, hwaddr addr, >+ uint8_t c, hwaddr len, MemTxAttrs attrs); >+ > #ifdef NEED_CPU_H > /* enum device_endian to MemOp. */ > static inline MemOp devend_memop(enum device_endian end) >diff --git a/softmmu/dma-helpers.c b/softmmu/dma-helpers.c >index 7d766a5e89a..8e1e7ad5320 100644 >--- a/softmmu/dma-helpers.c >+++ b/softmmu/dma-helpers.c >@@ -23,21 +23,7 @@ MemTxResult dma_memory_set(AddressSpace *as, dma_addr_t addr, > { > dma_barrier(as, DMA_DIRECTION_FROM_DEVICE); > >-#define FILLBUF_SIZE 512 >- uint8_t fillbuf[FILLBUF_SIZE]; >- int l; >- MemTxResult error = MEMTX_OK; >- >- memset(fillbuf, c, FILLBUF_SIZE); >- while (len > 0) { >- l = len < FILLBUF_SIZE ? len : FILLBUF_SIZE; >- error |= address_space_write(as, addr, MEMTXATTRS_UNSPECIFIED, >- fillbuf, l); >- len -= l; >- addr += l; >- } >- >- return error; >+ return address_space_set(as, addr, c, len, MEMTXATTRS_UNSPECIFIED); > } > > void qemu_sglist_init(QEMUSGList *qsg, DeviceState *dev, int alloc_hint, >diff --git a/softmmu/physmem.c b/softmmu/physmem.c >index 85034d9c11e..c9117527ae7 100644 >--- a/softmmu/physmem.c >+++ b/softmmu/physmem.c >@@ -2891,6 +2891,25 @@ MemTxResult address_space_rw(AddressSpace *as, hwaddr addr, MemTxAttrs attrs, > } > } > >+MemTxResult address_space_set(AddressSpace *as, hwaddr addr, >+ uint8_t c, hwaddr len, MemTxAttrs attrs) >+{ >+#define FILLBUF_SIZE 512 >+ uint8_t fillbuf[FILLBUF_SIZE]; >+ int l; >+ MemTxResult error = MEMTX_OK; >+ >+ memset(fillbuf, c, FILLBUF_SIZE); >+ while (len > 0) { What about return immediately if there is an error? I mean: while (len > 0 && result == MEMTX_OK) { I don't have a strong opinion on that, so in both cases: Reviewed-by: Stefano Garzarella <sgarzare@redhat.com> >+ l = len < FILLBUF_SIZE ? len : FILLBUF_SIZE; >+ error |= address_space_write(as, addr, attrs, fillbuf, l); >+ len -= l; >+ addr += l; >+ } >+ >+ return error; >+} >+ > void cpu_physical_memory_rw(hwaddr addr, void *buf, > hwaddr len, bool is_write) > { >-- >2.26.3 >
On Thu, 15 Apr 2021 at 12:00, Stefano Garzarella <sgarzare@redhat.com> wrote: > > On Thu, Apr 15, 2021 at 12:04:08PM +0200, Philippe Mathieu-Daudé wrote: > >dma_memory_set() does a DMA barrier, set the address space with > >a constant value. The constant value filling code is not specific > >to DMA and can be used for AddressSpace. Extract it as a new > >helper: address_space_set(). > > > >+MemTxResult address_space_set(AddressSpace *as, hwaddr addr, > >+ uint8_t c, hwaddr len, MemTxAttrs attrs) > >+{ > >+#define FILLBUF_SIZE 512 > >+ uint8_t fillbuf[FILLBUF_SIZE]; > >+ int l; > >+ MemTxResult error = MEMTX_OK; > >+ > >+ memset(fillbuf, c, FILLBUF_SIZE); > >+ while (len > 0) { > > What about return immediately if there is an error? > I mean: > while (len > 0 && result == MEMTX_OK) { I think that (a) we're just moving code here so we don't want to also change semantics; (b) there's a comment in memattrs.h that says * A zero (MEMTX_OK) response means success; anything else is a failure * of some kind. The memory subsystem will bitwise-OR together results * if it is synthesizing an operation from multiple smaller accesses. so in this function "keep going but merge errors" is in keeping with that principle. thanks -- PMM
On Thu, Apr 15, 2021 at 12:05:07PM +0100, Peter Maydell wrote: >On Thu, 15 Apr 2021 at 12:00, Stefano Garzarella <sgarzare@redhat.com> wrote: >> >> On Thu, Apr 15, 2021 at 12:04:08PM +0200, Philippe Mathieu-Daudé wrote: >> >dma_memory_set() does a DMA barrier, set the address space with >> >a constant value. The constant value filling code is not specific >> >to DMA and can be used for AddressSpace. Extract it as a new >> >helper: address_space_set(). > >> > >> >+MemTxResult address_space_set(AddressSpace *as, hwaddr addr, >> >+ uint8_t c, hwaddr len, MemTxAttrs attrs) >> >+{ >> >+#define FILLBUF_SIZE 512 >> >+ uint8_t fillbuf[FILLBUF_SIZE]; >> >+ int l; >> >+ MemTxResult error = MEMTX_OK; >> >+ >> >+ memset(fillbuf, c, FILLBUF_SIZE); >> >+ while (len > 0) { >> >> What about return immediately if there is an error? >> I mean: >> while (len > 0 && result == MEMTX_OK) { > >I think that (a) we're just moving code here so we don't want to also >change semantics; (b) there's a comment in memattrs.h that says > * A zero (MEMTX_OK) response means success; anything else is a failure > * of some kind. The memory subsystem will bitwise-OR together results > * if it is synthesizing an operation from multiple smaller accesses. > >so in this function "keep going but merge errors" is in keeping with that >principle. Got it, thanks for the explanation! :-) Stefano
On 4/15/21 3:04 AM, Philippe Mathieu-Daudé wrote: > dma_memory_set() does a DMA barrier, set the address space with > a constant value. The constant value filling code is not specific > to DMA and can be used for AddressSpace. Extract it as a new > helper: address_space_set(). > > Signed-off-by: Philippe Mathieu-Daudé<philmd@redhat.com> > --- > include/exec/memory.h | 16 ++++++++++++++++ > softmmu/dma-helpers.c | 16 +--------------- > softmmu/physmem.c | 19 +++++++++++++++++++ > 3 files changed, 36 insertions(+), 15 deletions(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
diff --git a/include/exec/memory.h b/include/exec/memory.h index 5728a681b27..192139af58e 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -2568,6 +2568,22 @@ address_space_write_cached(MemoryRegionCache *cache, hwaddr addr, } } +/** + * address_space_set: Fill address space with a constant byte. + * + * Return a MemTxResult indicating whether the operation succeeded + * or failed (eg unassigned memory, device rejected the transaction, + * IOMMU fault). + * + * @as: #AddressSpace to be accessed + * @addr: address within that address space + * @c: constant byte to fill the memory + * @len: the number of bytes to fill with the constant byte + * @attrs: memory transaction attributes + */ +MemTxResult address_space_set(AddressSpace *as, hwaddr addr, + uint8_t c, hwaddr len, MemTxAttrs attrs); + #ifdef NEED_CPU_H /* enum device_endian to MemOp. */ static inline MemOp devend_memop(enum device_endian end) diff --git a/softmmu/dma-helpers.c b/softmmu/dma-helpers.c index 7d766a5e89a..8e1e7ad5320 100644 --- a/softmmu/dma-helpers.c +++ b/softmmu/dma-helpers.c @@ -23,21 +23,7 @@ MemTxResult dma_memory_set(AddressSpace *as, dma_addr_t addr, { dma_barrier(as, DMA_DIRECTION_FROM_DEVICE); -#define FILLBUF_SIZE 512 - uint8_t fillbuf[FILLBUF_SIZE]; - int l; - MemTxResult error = MEMTX_OK; - - memset(fillbuf, c, FILLBUF_SIZE); - while (len > 0) { - l = len < FILLBUF_SIZE ? len : FILLBUF_SIZE; - error |= address_space_write(as, addr, MEMTXATTRS_UNSPECIFIED, - fillbuf, l); - len -= l; - addr += l; - } - - return error; + return address_space_set(as, addr, c, len, MEMTXATTRS_UNSPECIFIED); } void qemu_sglist_init(QEMUSGList *qsg, DeviceState *dev, int alloc_hint, diff --git a/softmmu/physmem.c b/softmmu/physmem.c index 85034d9c11e..c9117527ae7 100644 --- a/softmmu/physmem.c +++ b/softmmu/physmem.c @@ -2891,6 +2891,25 @@ MemTxResult address_space_rw(AddressSpace *as, hwaddr addr, MemTxAttrs attrs, } } +MemTxResult address_space_set(AddressSpace *as, hwaddr addr, + uint8_t c, hwaddr len, MemTxAttrs attrs) +{ +#define FILLBUF_SIZE 512 + uint8_t fillbuf[FILLBUF_SIZE]; + int l; + MemTxResult error = MEMTX_OK; + + memset(fillbuf, c, FILLBUF_SIZE); + while (len > 0) { + l = len < FILLBUF_SIZE ? len : FILLBUF_SIZE; + error |= address_space_write(as, addr, attrs, fillbuf, l); + len -= l; + addr += l; + } + + return error; +} + void cpu_physical_memory_rw(hwaddr addr, void *buf, hwaddr len, bool is_write) {
dma_memory_set() does a DMA barrier, set the address space with a constant value. The constant value filling code is not specific to DMA and can be used for AddressSpace. Extract it as a new helper: address_space_set(). Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- include/exec/memory.h | 16 ++++++++++++++++ softmmu/dma-helpers.c | 16 +--------------- softmmu/physmem.c | 19 +++++++++++++++++++ 3 files changed, 36 insertions(+), 15 deletions(-)