On Tue, 24 Sep 2019 at 01:00, Alex Bennée <alex.ben...@linaro.org> wrote:
>
>
> Beata Michalska <beata.michal...@linaro.org> writes:
>
> > Add an option to trigger memory writeback to sync given memory region
> > with the corresponding backing store, case one is available.
> > This extends the support for persistent memory, allowing syncing on-demand.
> >
> > Also, adding verification for msync support on host.
> >
> > Signed-off-by: Beata Michalska <beata.michal...@linaro.org>
> > ---
> > configure | 24 ++++++++++++++++++++++++
> > exec.c | 38 ++++++++++++++++++++++++++++++++++++++
> > include/exec/memory.h | 6 ++++++
> > include/exec/ram_addr.h | 6 ++++++
> > memory.c | 12 ++++++++++++
> > 5 files changed, 86 insertions(+)
> >
> > diff --git a/configure b/configure
> > index 95134c0180..bdb7dc47e9 100755
> > --- a/configure
> > +++ b/configure
> > @@ -5081,6 +5081,26 @@ if compile_prog "" "" ; then
> > fdatasync=yes
> > fi
> >
> > +##########################################
> > +# verify support for msyc
> > +
> > +msync=no
> > +cat > $TMPC << EOF
> > +#include <unistd.h>
> > +#include <sys/mman.h>
> > +int main(void) {
> > +#if defined(_POSIX_MAPPED_FILES) && _POSIX_MAPPED_FILES > 0 \
> > +&& defined(_POSIX_SYNCHRONIZED_IO) && _POSIX_SYNCHRONIZED_IO > 0
> > +return msync(NULL,0, MS_SYNC);
> > +#else
> > +#error Not supported
> > +#endif
> > +}
> > +EOF
> > +if compile_prog "" "" ; then
> > + msync=yes
> > +fi
> > +
> > ##########################################
> > # check if we have madvise
> >
> > @@ -6413,6 +6433,7 @@ echo "fdt support $fdt"
> > echo "membarrier $membarrier"
> > echo "preadv support $preadv"
> > echo "fdatasync $fdatasync"
> > +echo "msync $msync"
> > echo "madvise $madvise"
> > echo "posix_madvise $posix_madvise"
> > echo "posix_memalign $posix_memalign"
> > @@ -6952,6 +6973,9 @@ fi
> > if test "$fdatasync" = "yes" ; then
> > echo "CONFIG_FDATASYNC=y" >> $config_host_mak
> > fi
> > +if test "$msync" = "yes" ; then
> > + echo "CONFIG_MSYNC=y" >> $config_host_mak
> > +fi
>
> I think it's best to split this configure check into a new prequel patch
> and...
I might just drop it in favour of CONFIG_POSIX switch ......
>
> > if test "$madvise" = "yes" ; then
> > echo "CONFIG_MADVISE=y" >> $config_host_mak
> > fi
> > diff --git a/exec.c b/exec.c
> > index 235d6bc883..5ed6908368 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -65,6 +65,8 @@
> > #include "exec/ram_addr.h"
> > #include "exec/log.h"
> >
> > +#include "qemu/pmem.h"
> > +
> > #include "migration/vmstate.h"
> >
> > #include "qemu/range.h"
> > @@ -2182,6 +2184,42 @@ int qemu_ram_resize(RAMBlock *block, ram_addr_t
> > newsize, Error **errp)
> > return 0;
> > }
> >
> > +/*
> > + * Trigger sync on the given ram block for range [start, start + length]
> > + * with the backing store if available.
> > + * @Note: this is supposed to be a SYNC op.
> > + */
> > +void qemu_ram_writeback(RAMBlock *block, ram_addr_t start, ram_addr_t
> > length)
> > +{
> > + void *addr = ramblock_ptr(block, start);
> > +
> > + /*
> > + * The requested range might spread up to the very end of the block
> > + */
> > + if ((start + length) > block->used_length) {
> > + error_report("%s: sync range outside the block boundires: "
> > + "start: " RAM_ADDR_FMT " length: " RAM_ADDR_FMT
> > + " block length: " RAM_ADDR_FMT " Narrowing down ..." ,
> > + __func__, start, length, block->used_length);
>
> Is this an error or just logging? error_report should be used for stuff
> that the user needs to know about so it will appear on the HMP console
> (or via stderr). If so what is the user expected to do? Have they
> misconfigured their system?
>
This should be logging rather than 'error reporting as such. My bad.
Will address that in the next version.
> > + length = block->used_length - start;
> > + }
> > +
> > +#ifdef CONFIG_LIBPMEM
> > + /* The lack of support for pmem should not block the sync */
> > + if (ramblock_is_pmem(block)) {
> > + pmem_persist(addr, length);
> > + } else
> > +#endif
> > + if (block->fd >= 0) {
> > +#ifdef CONFIG_MSYNC
> > + msync((void *)((uintptr_t)addr & qemu_host_page_mask),
> > + HOST_PAGE_ALIGN(length), MS_SYNC);
> > +#else
> > + qemu_fdatasync(block->fd);
> > +#endif
>
> ... hide the implementation details in util/cutils.c, maybe as
> qemu_msync()?
>
> > + }
> > +}
> > +
> > /* Called with ram_list.mutex held */
> > static void dirty_memory_extend(ram_addr_t old_ram_size,
> > ram_addr_t new_ram_size)
> > diff --git a/include/exec/memory.h b/include/exec/memory.h
> > index 2dd810259d..ff0d7937cf 100644
> > --- a/include/exec/memory.h
> > +++ b/include/exec/memory.h
> > @@ -1242,6 +1242,12 @@ void *memory_region_get_ram_ptr(MemoryRegion *mr);
> > */
> > void memory_region_ram_resize(MemoryRegion *mr, ram_addr_t newsize,
> > Error **errp);
> > +/**
> > + * memory_region_do_writeback: Trigger writeback for selected address range
> > + * [addr, addr + size]
> > + *
> > + */
> > +void memory_region_do_writeback(MemoryRegion *mr, hwaddr addr, hwaddr
> > size);
> >
> > /**
> > * memory_region_set_log: Turn dirty logging on or off for a region.
> > diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> > index a327a80cfe..d4bce81a03 100644
> > --- a/include/exec/ram_addr.h
> > +++ b/include/exec/ram_addr.h
> > @@ -180,6 +180,12 @@ void qemu_ram_free(RAMBlock *block);
> >
> > int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, Error **errp);
> >
> > +void qemu_ram_writeback(RAMBlock *block, ram_addr_t start, ram_addr_t
> > length);
> > +
> > +/* Clear whole block of mem */
> > +#define qemu_ram_block_writeback(rb) \
> > + qemu_ram_writeback(rb, 0, rb->used_length)
> > +
> > #define DIRTY_CLIENTS_ALL ((1 << DIRTY_MEMORY_NUM) - 1)
> > #define DIRTY_CLIENTS_NOCODE (DIRTY_CLIENTS_ALL & ~(1 <<
> > DIRTY_MEMORY_CODE))
> >
> > diff --git a/memory.c b/memory.c
> > index 61a254c3f9..436eb64737 100644
> > --- a/memory.c
> > +++ b/memory.c
> > @@ -2228,6 +2228,18 @@ void memory_region_ram_resize(MemoryRegion *mr,
> > ram_addr_t newsize, Error **errp
> > qemu_ram_resize(mr->ram_block, newsize, errp);
> > }
> >
> > +
> > +void memory_region_do_writeback(MemoryRegion *mr, hwaddr addr, hwaddr size)
> > +{
> > + /*
> > + * Might be extended case needed to cover
> > + * different types of memory regions
> > + */
> > + if (mr->ram_block && mr->dirty_log_mask) {
> > + qemu_ram_writeback(mr->ram_block, addr, size);
> > + }
> > +}
> > +
> > /*
> > * Call proper memory listeners about the change on the newly
> > * added/removed CoalescedMemoryRange.
>
>
> --
> Alex Bennée
Thank you.
BR
Beata
