[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 08/18] nvdimm: init backend memory mapping an
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH v2 08/18] nvdimm: init backend memory mapping and config data area |
Date: |
Tue, 25 Aug 2015 17:03:53 +0100 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
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.
- Re: [Qemu-devel] [PATCH v2 06/18] pc: implement NVDIMM device abstract, (continued)
[Qemu-devel] [PATCH v2 09/18] nvdimm: build ACPI NFIT table, Xiao Guangrong, 2015/08/14
[Qemu-devel] [PATCH v2 12/18] nvdimm: save arg3 for NVDIMM device _DSM method, Xiao Guangrong, 2015/08/14
[Qemu-devel] [PATCH v2 10/18] nvdimm: init the address region used by DSM method, Xiao Guangrong, 2015/08/14
[Qemu-devel] [PATCH v2 11/18] nvdimm: build ACPI nvdimm devices, Xiao Guangrong, 2015/08/14
[Qemu-devel] [PATCH v2 14/18] nvdimm: support NFIT_CMD_IMPLEMENTED function, Xiao Guangrong, 2015/08/14