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

[Stefan Hajnoczi <stefanha@gmail.com>]

On Fri, Aug 14, 2015 at 10:52:01PM +0800, Xiao Guangrong wrote:
> The parameter @file is used as backed memory for NVDIMM which is
> divided into two parts if @dataconfig is true:

s/dataconfig/configdata/

> @@ -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;
> +    }

#ifdef __linux__ for ioctl(fd, BLKGETSIZE64, &size)?

There is nothing Linux-specific about emulating NVDIMMs so this code
should compile on all platforms.

> +
> +    return 0;
> +}
> +
>  static void pc_nvdimm_realize(DeviceState *dev, Error **errp)
>  {
>      PCNVDIMMDevice *nvdimm = PC_NVDIMM(dev);
> +    char name[512];
> +    void *buf;
> +    ram_addr_t addr;
> +    uint64_t size, nvdimm_size, config_size = MIN_CONFIG_DATA_SIZE;
> +    int fd;
>  
>      if (!nvdimm->file) {
>          error_setg(errp, "file property is not set");
>      }

Missing return here.

> +
> +    fd = open(nvdimm->file, O_RDWR);

Does it make sense to support read-only NVDIMMs?

It could be handy for sharing a read-only file between unprivileged
guests.  The permissions on the file would only allow read, not write.

> +    if (fd < 0) {
> +        error_setg(errp, "can not open %s", nvdimm->file);

s/can not/cannot/

> +        return;
> +    }
> +
> +    size = get_file_size(fd);
> +    buf = mmap(NULL, size, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);

I guess the user will want to choose between MAP_SHARED and MAP_PRIVATE.
This can be added in the future.

> +    if (buf == MAP_FAILED) {
> +        error_setg(errp, "can not do mmap on %s", nvdimm->file);
> +        goto do_close;
> +    }
> +
> +    nvdimm->config_data_size = config_size;
> +    if (nvdimm->configdata) {
> +        /* reserve MIN_CONFIGDATA_AREA_SIZE for configue data. */
> +        nvdimm_size = size - config_size;
> +        nvdimm->config_data_addr = buf + nvdimm_size;
> +    } else {
> +        nvdimm_size = size;
> +        nvdimm->config_data_addr = NULL;
> +    }
> +
> +    if ((int64_t)nvdimm_size <= 0) {

The error cases can be detected before mmap(2).  That avoids the int64_t
cast and also avoids nvdimm_size underflow and the bogus
nvdimm->config_data_addr calculation above.

size = get_file_size(fd);
if (size == 0) {
    error_setg(errp, "empty file or unable to get file size");
    goto do_close;
} else if (nvdimm->configdata && size < config_size) {{
    error_setg(errp, "file size is too small to store NVDIMM"
                     " configure data");
    goto do_close;
}

> +        error_setg(errp, "file size is too small to store NVDIMM"
> +                         " configure data");
> +        goto do_unmap;
> +    }
> +
> +    addr = reserved_range_push(nvdimm_size);
> +    if (!addr) {
> +        error_setg(errp, "do not have enough space for size %#lx.\n", size);

error_setg() messages must not have a newline at the end.

Please use "%#" PRIx64 instead of "%#lx" so compilation works on 32-bit
hosts where sizeof(long) == 4.

> +        goto do_unmap;
> +    }
> +
> +    nvdimm->device_index = new_device_index();
> +    sprintf(name, "NVDIMM-%d", nvdimm->device_index);
> +    memory_region_init_ram_ptr(&nvdimm->mr, OBJECT(dev), name, nvdimm_size,
> +                               buf);

How is the autogenerated name used?

Why not just use "pc-nvdimm.memory"?

> +    vmstate_register_ram(&nvdimm->mr, DEVICE(dev));
> +    memory_region_add_subregion(get_system_memory(), addr, &nvdimm->mr);
> +
> +    return;

fd is leaked.