On Sat, Jan 26, 2013 at 8:43 PM, David Woodhouse <dw...@infradead.org> wrote:
> 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?
It looks like this duplicates rom_add_file() and fw_cfg_add_file(), so
I don't see the point.
Removing unused fw_cfg_add_callback() should be a separate patch and
that would be OK.
In general, QEMU uses different coding style from kernel, so please
read our CODING_STYLE and use checkpatch.pl to catch obvious problems,
like missing braces and C99 comments.
>
> 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
>
