Re: [Qemu-devel] [RFC] Writeable files in fw_cfg

[Blue Swirl]

On Sat, Jan 26, 2013 at 8:43 PM, David Woodhouse <address@hidden> 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
>