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.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
