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

[Anthony Liguori]

David Woodhouse <address@hidden> 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