Re: [Qemu-devel] [PATCH v2 08/18] nvdimm: init backend memory mapping and config data area

[Xiao Guangrong]

On 09/07/2015 10:11 PM, Igor Mammedov wrote:
> On Fri, 14 Aug 2015 22:52:01 +0800
> Xiao Guangrong <address@hidden> wrote:
>
> > The parameter @file is used as backed memory for NVDIMM which is
> > divided into two parts if @dataconfig is true:
> > - first parts is (0, size - 128K], which is used as PMEM (Persistent
> >    Memory)
> > - 128K at the end of the file, which is used as Config Data Area, it's
> >    used to store Label namespace data
> >
> > The @file supports both regular file and block device, of course we
> > can assign any these two kinds of files for test and emulation, however,
> > in the real word for performance reason, we usually used these files as
> > NVDIMM backed file:
> > - the regular file in the filesystem with DAX enabled created on NVDIMM
> >    device on host
> > - the raw PMEM device on host, e,g /dev/pmem0
>
> A lot of code in this series could reuse what QEMU already
> uses for implementing pc-dimm devices.
>
> here is common concepts that could be reused.
>    - on physical system both DIMM and NVDIMM devices use
>      the same slots. We could share QEMU's '-m slots' option between
>      both devices. An alternative to not sharing would be to introduce
>      '-machine nvdimm_slots' option.
>      And yes, we need to know number of NVDIMMs to describe
>      them all in ACPI table (taking in amount future hotplug
>      include in this possible NVDIMM devices)
>      I'd go the same way as on real hardware on make them share the same slots.

I'd prefer sharing slots for pc-dimm and nvdimm, it's easier to reuse the
logic of slot-assignment and plug/unplug.

>    - they share the same physical address space and limits
>      on how much memory system can handle. So I'd suggest sharing existing
>      '-m maxmem' option and reuse hotplug_memory address space.

Sounds good to me.

> Essentially what I'm suggesting is to inherit NVDIMM's implementation
> from pc-dimm reusing all of its code/backends and
> just override parts that do memory mapping into guest's address space to
> accommodate NVDIMM's requirements.

Good idea!

We have to differentiate pc-dimm and nvdimm in the common code and nvdimm
has different points with pc-dimm (for example, its has reserved-region, and
need support live migration of label data). How about rename 'pc-nvdimm' to
'memory-device' and make it as a common device type, then build pc-dimm and
nvdimm on top of it?

Something like:
static TypeInfo memory_device_info = {
    .name          = TYPE_MEM_DEV,
    .parent        = TYPE_DEVICE,
};

static TypeInfo memory_device_info = {
.name = TYPE_PC_DIMM,
.parent = TYPE_MEM_DEV,
};

static TypeInfo memory_device_info = {
.name = TYPE_NVDIMM,
.parent = TYPE_MEM_DEV,
};

It also make CONIFG_NVDIMM and CONFIG_HOT_PLUG be independent.

> > Signed-off-by: Xiao Guangrong <address@hidden>
> > ---
> >   hw/mem/nvdimm/pc-nvdimm.c  | 109 ++++++++++++++++++++++++++++++++++++++++++++-
> >   include/hw/mem/pc-nvdimm.h |   7 +++
> >   2 files changed, 115 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/mem/nvdimm/pc-nvdimm.c b/hw/mem/nvdimm/pc-nvdimm.c
> > index 7a270a8..97710d1 100644
> > --- a/hw/mem/nvdimm/pc-nvdimm.c
> > +++ b/hw/mem/nvdimm/pc-nvdimm.c
> > @@ -22,12 +22,20 @@
> >    * License along with this library; if not, see <http://www.gnu.org/licenses/>
> >    */
> >
> > +#include <sys/mman.h>
> > +#include <sys/ioctl.h>
> > +#include <linux/fs.h>
> > +
> > +#include "exec/address-spaces.h"
> >   #include "hw/mem/pc-nvdimm.h"
> >
> > -#define PAGE_SIZE      (1UL << 12)
> > +#define PAGE_SIZE               (1UL << 12)
> > +
> > +#define MIN_CONFIG_DATA_SIZE    (128 << 10)
> >
> >   static struct nvdimms_info {
> >       ram_addr_t current_addr;
> > +    int device_index;
> >   } nvdimms_info;
> >
> >   /* the address range [offset, ~0ULL) is reserved for NVDIMM. */
> > @@ -37,6 +45,26 @@ void pc_nvdimm_reserve_range(ram_addr_t offset)
> >       nvdimms_info.current_addr = offset;
> >   }
> >
> > +static ram_addr_t reserved_range_push(uint64_t size)
> > +{
> > +    uint64_t current;
> > +
> > +    current = ROUND_UP(nvdimms_info.current_addr, PAGE_SIZE);
> > +
> > +    /* do not have enough space? */
> > +    if (current + size < current) {
> > +        return 0;
> > +    }
> > +
> > +    nvdimms_info.current_addr = current + size;
> > +    return current;
> > +}
> You can't use all memory above hotplug_memory area since
> we have to tell guest where 64-bit PCI window starts,
> and currently it should start at reserved-memory-end
> (but it isn't due to a bug: I've just posted fix to qemu-devel
>   "[PATCH 0/2] pc: fix 64-bit PCI window clashing with memory hotplug region"
> )

Ah, got it, thanks for you pointing it out.

> > +
> > +static uint32_t new_device_index(void)
> > +{
> > +    return nvdimms_info.device_index++;
> > +}
> > +
> >   static char *get_file(Object *obj, Error **errp)
> >   {
> >       PCNVDIMMDevice *nvdimm = PC_NVDIMM(obj);
> > @@ -48,6 +76,11 @@ static void set_file(Object *obj, const char *str, Error 
> > **errp)
> >   {
> >       PCNVDIMMDevice *nvdimm = PC_NVDIMM(obj);
> >
> > +    if (memory_region_size(&nvdimm->mr)) {
> > +        error_setg(errp, "cannot change property value");
> > +        return;
> > +    }
> > +
> >       if (nvdimm->file) {
> >           g_free(nvdimm->file);
> >       }
> > @@ -76,13 +109,87 @@ static void pc_nvdimm_init(Object *obj)
> >                                set_configdata, NULL);
> >   }
> >
> > +static uint64_t get_file_size(int fd)
> > +{
> > +    struct stat stat_buf;
> > +    uint64_t size;
> > +
> > +    if (fstat(fd, &stat_buf) < 0) {
> > +        return 0;
> > +    }
> > +
> > +    if (S_ISREG(stat_buf.st_mode)) {
> > +        return stat_buf.st_size;
> > +    }
> > +
> > +    if (S_ISBLK(stat_buf.st_mode) && !ioctl(fd, BLKGETSIZE64, &size)) {
> > +        return size;
> > +    }
> > +
> > +    return 0;
> > +}
> All this file stuff I'd leave to already existing backends like
> memory-backend-file or even memory-backend-ram which already do
> above and more allowing to configure persistent and volatile
> NVDIMMs without changing NVDIMM fronted code.

The current memory backends use all memory size and map it to guest's
address space. However, nvdimm needs a reserved region for its label
data which is only accessed in Qemu.

How about introduce two parameters, "reserved_size" and "reserved_addr"
to TYPE_MEMORY_BACKEND, then only the memory region
[0, size - reserved_size) is mapped to guest and the remain part is
pointed by "reserved_addr"?