On 2019-01-14 at 14:04:25 -0500, Michael S. Tsirkin wrote:
> On Mon, Jan 14, 2019 at 04:50:36PM -0200, Eduardo Habkost wrote:
> > On Wed, Jan 02, 2019 at 01:26:06PM +0800, Zhang Yi wrote:
> > > As more flag parameters besides the existing 'shared' are going to be
> > > added to qemu_ram_mmap(), let's switch 'shared' to a 'flags' parameter
> > > in advance, so as to ease the further additions.
> > >
> > > Signed-off-by: Haozhong Zhang <haozhong.zh...@intel.com>
> > > Signed-off-by: Zhang Yi <yi.z.zh...@linux.intel.com>
> > > ---
> > > exec.c | 7 ++++---
> > > include/exec/memory.h | 22 ++++++++++++++++------
> > > include/qemu/mmap-alloc.h | 19 ++++++++++++++++++-
> > > util/mmap-alloc.c | 8 +++++---
> > > util/oslib-posix.c | 9 ++++++++-
> > > 5 files changed, 51 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/exec.c b/exec.c
> > > index bb6170d..e92a7da 100644
> > > --- a/exec.c
> > > +++ b/exec.c
> > > @@ -1810,6 +1810,7 @@ static void *file_ram_alloc(RAMBlock *block,
> > > ram_addr_t memory,
> > > int fd,
> > > bool truncate,
> > > + uint32_t flags,
> > > Error **errp)
> >
> > I suggest documenting on the commit message why you are changing
> > file_ram_alloc() too. The commit message mentions only
> > qemu_ram_mmap().
> >
> > > {
> > > void *area;
> > > @@ -1859,8 +1860,7 @@ static void *file_ram_alloc(RAMBlock *block,
> > > perror("ftruncate");
> > > }
> > >
> > > - area = qemu_ram_mmap(fd, memory, block->mr->align,
> > > - block->flags & RAM_SHARED);
> > > + area = qemu_ram_mmap(fd, memory, block->mr->align, flags);
> > > if (area == MAP_FAILED) {
> > > error_setg_errno(errp, errno,
> > > "unable to map backing store for guest RAM");
> > > @@ -2279,7 +2279,8 @@ RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size,
> > > MemoryRegion *mr,
> > > new_block->used_length = size;
> > > new_block->max_length = size;
> > > new_block->flags = ram_flags;
> > > - new_block->host = file_ram_alloc(new_block, size, fd, !file_size,
> > > errp);
> > > + new_block->host = file_ram_alloc(new_block, size, fd, !file_size,
> > > + ram_flags, errp);
> > > if (!new_block->host) {
> > > g_free(new_block);
> > > return NULL;
> > > diff --git a/include/exec/memory.h b/include/exec/memory.h
> > > index 667466b..6e30c23 100644
> > > --- a/include/exec/memory.h
> > > +++ b/include/exec/memory.h
> > > @@ -103,28 +103,38 @@ struct IOMMUNotifier {
> > > };
> > > typedef struct IOMMUNotifier IOMMUNotifier;
> > >
> > > +#ifdef __CHECKER__
> > > +#define QEMU_BITWISE __attribute__((bitwise))
> > > +#define QEMU_FORCE __attribute__((force))
> > > +#else
> > > +#define QEMU_BITWISE
> > > +#define QEMU_FORCE
> > > +#endif
> > > +
> >
> > I assume this is a sparse feature?
> >
> > Why is it part of this patch? I suggest doing this in a separate
> > patch series and in a common header file, so other developers
> > have a better chance to review it and decide how to use this
> > sparse feature in QEMU.
>
> It's needed for this series but yes, this ifdefery belongs in
> a more central header. Maybe qemu/osdep.h
> And it needs documentation and be a separate patch.
Agree, Thank Michael's explanation, better to doing this in a separate
patch series.
>
>
> >
> > > +typedef unsigned QEMU_BITWISE QemuMmapFlags;
> > > +
> > > /* RAM is pre-allocated and passed into qemu_ram_alloc_from_ptr */
> > > -#define RAM_PREALLOC (1 << 0)
> > > +#define RAM_PREALLOC ((QEMU_FORCE QemuMmapFlags) (1 << 0))
> > >
> > > /* RAM is mmap-ed with MAP_SHARED */
> > > -#define RAM_SHARED (1 << 1)
> > > +#define RAM_SHARED ((QEMU_FORCE QemuMmapFlags) (1 << 1))
> > >
> > > /* Only a portion of RAM (used_length) is actually used, and migrated.
> > > * This used_length size can change across reboots.
> > > */
> > > -#define RAM_RESIZEABLE (1 << 2)
> > > +#define RAM_RESIZEABLE ((QEMU_FORCE QemuMmapFlags) (1 << 2))
> > >
> > > /* UFFDIO_ZEROPAGE is available on this RAMBlock to atomically
> > > * zero the page and wake waiting processes.
> > > * (Set during postcopy)
> > > */
> > > -#define RAM_UF_ZEROPAGE (1 << 3)
> > > +#define RAM_UF_ZEROPAGE ((QEMU_FORCE QemuMmapFlags) (1 << 3))
> > >
> > > /* RAM can be migrated */
> > > -#define RAM_MIGRATABLE (1 << 4)
> > > +#define RAM_MIGRATABLE ((QEMU_FORCE QemuMmapFlags) (1 << 4))
> > >
> > > /* RAM is a persistent kind memory */
> > > -#define RAM_PMEM (1 << 5)
> > > +#define RAM_PMEM ((QEMU_FORCE QemuMmapFlags) (1 << 5))
> > >
> > > static inline void iommu_notifier_init(IOMMUNotifier *n, IOMMUNotify fn,
> > > IOMMUNotifierFlag flags,
> > > diff --git a/include/qemu/mmap-alloc.h b/include/qemu/mmap-alloc.h
> > > index 50385e3..6fe6ed4 100644
> > > --- a/include/qemu/mmap-alloc.h
> > > +++ b/include/qemu/mmap-alloc.h
> > > @@ -7,7 +7,24 @@ size_t qemu_fd_getpagesize(int fd);
> > >
> > > size_t qemu_mempath_getpagesize(const char *mem_path);
> > >
> > > -void *qemu_ram_mmap(int fd, size_t size, size_t align, bool shared);
> > > +/**
> > > + * qemu_ram_mmap: mmap the specified file or device.
> > > + *
> > > + * Parameters:
> > > + * @fd: the file or the device to mmap
> > > + * @size: the number of bytes to be mmaped
> > > + * @align: if not zero, specify the alignment of the starting mapping
> > > address;
> > > + * otherwise, the alignment in use will be determined by QEMU.
> > > + * @flags: specifies additional properties of the mapping, which can be
> > > one or
> > > + * bit-or of following values
> > > + * - RAM_SHARED: mmap with MAP_SHARED flag
> > > + * Other bits are ignored.
> > > + *
> > > + * Return:
> > > + * On success, return a pointer to the mapped area.
> > > + * On failure, return MAP_FAILED.
> > > + */
> > > +void *qemu_ram_mmap(int fd, size_t size, size_t align, uint32_t flags);
> > >
> > > void qemu_ram_munmap(void *ptr, size_t size);
> > >
> > > diff --git a/util/mmap-alloc.c b/util/mmap-alloc.c
> > > index fd329ec..8f0a740 100644
> > > --- a/util/mmap-alloc.c
> > > +++ b/util/mmap-alloc.c
> > > @@ -13,6 +13,7 @@
> > > #include "qemu/osdep.h"
> > > #include "qemu/mmap-alloc.h"
> > > #include "qemu/host-utils.h"
> > > +#include "exec/memory.h"
> > >
> > > #define HUGETLBFS_MAGIC 0x958458f6
> > >
> > > @@ -75,7 +76,7 @@ size_t qemu_mempath_getpagesize(const char *mem_path)
> > > return getpagesize();
> > > }
> > >
> > > -void *qemu_ram_mmap(int fd, size_t size, size_t align, bool shared)
> > > +void *qemu_ram_mmap(int fd, size_t size, size_t align, uint32_t flags)
> > > {
> > > /*
> > > * Note: this always allocates at least one extra page of virtual
> > > address
> > > @@ -92,11 +93,12 @@ void *qemu_ram_mmap(int fd, size_t size, size_t
> > > align, bool shared)
> > > * anonymous memory is OK.
> > > */
> > > int anonfd = fd == -1 || qemu_fd_getpagesize(fd) == getpagesize() ?
> > > -1 : fd;
> > > - int flags = anonfd == -1 ? MAP_ANONYMOUS : MAP_NORESERVE;
> > > - void *ptr = mmap(0, total, PROT_NONE, flags | MAP_PRIVATE, anonfd,
> > > 0);
> > > + int mmap_flags = anonfd == -1 ? MAP_ANONYMOUS : MAP_NORESERVE;
> > > + void *ptr = mmap(0, total, PROT_NONE, mmap_flags | MAP_PRIVATE,
> > > anonfd, 0);
> > > #else
> > > void *ptr = mmap(0, total, PROT_NONE, MAP_ANONYMOUS | MAP_PRIVATE,
> > > -1, 0);
> > > #endif
> > > + bool shared = flags & RAM_SHARED;
> > > size_t offset;
> > > void *ptr1;
> > >
> > > diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> > > index fbd0dc8..75a0171 100644
> > > --- a/util/oslib-posix.c
> > > +++ b/util/oslib-posix.c
> > > @@ -54,6 +54,7 @@
> > > #endif
> > >
> > > #include "qemu/mmap-alloc.h"
> > > +#include "exec/memory.h"
> > >
> > > #ifdef CONFIG_DEBUG_STACK_USAGE
> > > #include "qemu/error-report.h"
> > > @@ -203,7 +204,13 @@ void *qemu_memalign(size_t alignment, size_t size)
> > > void *qemu_anon_ram_alloc(size_t size, uint64_t *alignment, bool shared)
> > > {
> > > size_t align = QEMU_VMALLOC_ALIGN;
> > > - void *ptr = qemu_ram_mmap(-1, size, align, shared);
> > > + uint32_t flags = 0;
> > > + void *ptr;
> > > +
> > > + if (shared) {
> > > + flags = RAM_SHARED;
> > > + }
> > > + ptr = qemu_ram_mmap(-1, size, align, flags);
> > >
> > > if (ptr == MAP_FAILED) {
> > > return NULL;
> > > --
> > > 2.7.4
> > >
> > >
> >
> > --
> > Eduardo
