David Woodhouse <dw...@infradead.org> writes: > For OVMF we really want to have a way to store non-volatile variables, > other than the dirty hack that currently puts them on a file in the EFI > system partition. > > It looks like we've supported writing to fw_cfg items fairly much since > they were introduced, but we've never actually made use of that. > > This WIP patch kills the existing fw_cfg_add_callback() because I can't > see how it would ever be useful, and it doesn't seem to have been used > for years, if ever. And adds something slightly more useful. > > Other then the obvious bits that need finishing, any objections?
The main issue is malicious guests. The administrator has to be aware of and in control of how much disk space the guest can use. The secondary issue is how to tie into the block layer so things like live migration work. This is why "use a flash device" is an attractive answer here because it gives a fixed sized storage pool that's configurable by the administrator. Since it's backed by a block device, it supports all of the features needed for non-volatile storage (snapshotting, live copy, etc.). The only real downside is that it's opaque to the host. If it's desirable to have something that's visible to the host, then I think we would still need to make use of BlockDriverState as the means to make it non-volatile. That essentially involves writing a file system in QEMU on top of BlockDriverState and then having a PV inteface with the guest to get/set file content. Would be useful to have, but so far no one has cared enough about making these stores non-opaque to do it. Regards, Anthony Liguori > > diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c > index 3b31d77..1318a2e 100644 > --- a/hw/fw_cfg.c > +++ b/hw/fw_cfg.c > @@ -33,6 +33,9 @@ > #define FW_CFG_SIZE 2 > #define FW_CFG_DATA_SIZE 1 > > +struct FWCfgEntry; > +typedef void (*FWCfgCallback)(struct FWCfgState *s, struct FWCfgEntry *e, > int value); > + > typedef struct FWCfgEntry { > uint32_t len; > uint8_t *data; > @@ -206,20 +209,19 @@ static void fw_cfg_write(FWCfgState *s, uint8_t value) > > trace_fw_cfg_write(s, value); > > - if (s->cur_entry & FW_CFG_WRITE_CHANNEL && e->callback && > - s->cur_offset < e->len) { > - e->data[s->cur_offset++] = value; > - if (s->cur_offset == e->len) { > - e->callback(e->callback_opaque, e->data); > - s->cur_offset = 0; > - } > - } > + if (s->cur_entry & FW_CFG_WRITE_CHANNEL && e->callback) > + e->callback(s, e, value); > } > > static int fw_cfg_select(FWCfgState *s, uint16_t key) > { > + int arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL); > + FWCfgEntry *e = &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK]; > int ret; > > + if (e->callback) > + e->callback(s, e, -1); > + > s->cur_offset = 0; > if ((key & FW_CFG_ENTRY_MASK) >= FW_CFG_MAX_ENTRY) { > s->cur_entry = FW_CFG_INVALID; > @@ -419,8 +421,8 @@ void fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t > value) > fw_cfg_add_bytes(s, key, copy, sizeof(value)); > } > > -void fw_cfg_add_callback(FWCfgState *s, uint16_t key, FWCfgCallback callback, > - void *callback_opaque, void *data, size_t len) > +static void fw_cfg_add_callback(FWCfgState *s, uint16_t key, FWCfgCallback > callback, > + void *callback_opaque, void *data, size_t > len) > { > int arch = !!(key & FW_CFG_ARCH_LOCAL); > > @@ -436,8 +438,9 @@ void fw_cfg_add_callback(FWCfgState *s, uint16_t key, > FWCfgCallback callback, > s->entries[arch][key].callback = callback; > } > > -void fw_cfg_add_file(FWCfgState *s, const char *filename, > - void *data, size_t len) > +static void fw_cfg_add_file_writeable(FWCfgState *s, const char *filename, > + void *data, size_t len, > + FWCfgCallback callback, void *cb_opaque) > { > int i, index; > size_t dsize; > @@ -451,7 +454,8 @@ void fw_cfg_add_file(FWCfgState *s, const char *filename, > index = be32_to_cpu(s->files->count); > assert(index < FW_CFG_FILE_SLOTS); > > - fw_cfg_add_bytes(s, FW_CFG_FILE_FIRST + index, data, len); > + fw_cfg_add_callback(s, FW_CFG_WRITE_CHANNEL + FW_CFG_FILE_FIRST + index, > + callback, cb_opaque, data, len); > > pstrcpy(s->files->f[index].name, sizeof(s->files->f[index].name), > filename); > @@ -464,11 +468,74 @@ void fw_cfg_add_file(FWCfgState *s, const char > *filename, > > s->files->f[index].size = cpu_to_be32(len); > s->files->f[index].select = cpu_to_be16(FW_CFG_FILE_FIRST + index); > + if (callback) > + s->files->f[index].select |= cpu_to_be16(FW_CFG_WRITE_CHANNEL); > trace_fw_cfg_add_file(s, index, s->files->f[index].name, len); > > s->files->count = cpu_to_be32(index+1); > } > > +void fw_cfg_add_file(FWCfgState *s, const char *filename, > + void *data, size_t len) > +{ > + fw_cfg_add_file_writeable(s, filename, data, len, NULL, NULL); > +} > + > +// Lifted from pc.c > +static long get_file_size(FILE *f) > +{ > + long where, size; > + > + /* XXX: on Unix systems, using fstat() probably makes more sense */ > + > + where = ftell(f); > + fseek(f, 0, SEEK_END); > + size = ftell(f); > + fseek(f, where, SEEK_SET); > + > + return size; > +} > + > +static void nvstorage_callback(struct FWCfgState *s, struct FWCfgEntry *e, > int value) > + > +{ > + if (value == -1) { > + FILE *f = fopen(e->callback_opaque, "wb"); > + if (f) { > + fwrite(e->data, e->len, 1, f); > + fclose(f); > + } > + return; > + } > + if (s->cur_offset == e->len) > + e->data = realloc(e->data, ++e->len); > + e->data[s->cur_offset++] = value; > +} > + > +void fw_cfg_add_nvstorage(FWCfgState *s, const char *filename) > +{ > + FILE *f; > + int file_size = 0; > + int f2; > + uint8_t *data = NULL; > + > + f = fopen(filename, "rb"); > + if (f) { > + file_size = get_file_size(f); > + if (file_size) { > + data = malloc(file_size); > + if ((f2=fread(data, 1, file_size, f)) != file_size) { > + fprintf(stderr, "qemu: Could not load non-volatile storage > file '%s' %d %d: %s\n", > + filename, file_size, f2, strerror(errno)); > + exit(1); > + } > + } > + fclose(f); > + } > + fw_cfg_add_file_writeable(s, "etc/nvstorage", data, file_size, > + nvstorage_callback, strdup(filename)); > +} > + > static void fw_cfg_machine_ready(struct Notifier *n, void *data) > { > size_t len; > @@ -507,6 +574,7 @@ FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t > data_port, > fw_cfg_add_i16(s, FW_CFG_BOOT_MENU, (uint16_t)boot_menu); > fw_cfg_bootsplash(s); > fw_cfg_reboot(s); > + fw_cfg_add_nvstorage(s, "/tmp/nvstorage"); > > s->machine_ready.notify = fw_cfg_machine_ready; > qemu_add_machine_init_done_notifier(&s->machine_ready); > diff --git a/hw/fw_cfg.h b/hw/fw_cfg.h > index 05c8df1..298bc47 100644 > --- a/hw/fw_cfg.h > +++ b/hw/fw_cfg.h > @@ -51,20 +51,17 @@ typedef struct FWCfgFiles { > FWCfgFile f[]; > } FWCfgFiles; > > -typedef void (*FWCfgCallback)(void *opaque, uint8_t *data); > - > typedef struct FWCfgState FWCfgState; > void fw_cfg_add_bytes(FWCfgState *s, uint16_t key, void *data, size_t len); > void fw_cfg_add_string(FWCfgState *s, uint16_t key, const char *value); > void fw_cfg_add_i16(FWCfgState *s, uint16_t key, uint16_t value); > void fw_cfg_add_i32(FWCfgState *s, uint16_t key, uint32_t value); > void fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t value); > -void fw_cfg_add_callback(FWCfgState *s, uint16_t key, FWCfgCallback callback, > - void *callback_opaque, void *data, size_t len); > void fw_cfg_add_file(FWCfgState *s, const char *filename, void *data, > size_t len); > FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port, > hwaddr crl_addr, hwaddr data_addr); > +void fw_cfg_add_nvstorage(FWCfgState *s, const char *filename); > > #endif /* NO_QEMU_PROTOS */ > > > -- > dwmw2