[Qemu-devel] Re: [PATCHv4 15/15] Pass boot device list to firmware.

["Michael S. Tsirkin" <mst@redhat.com>]

On Sun, Nov 14, 2010 at 05:39:41PM +0200, Gleb Natapov wrote:
> 
> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> ---
>  hw/fw_cfg.c |   14 ++++++++++++++
>  hw/fw_cfg.h |    4 +++-
>  sysemu.h    |    1 +
>  vl.c        |   51 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 69 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c
> index 7b9434f..f6a67db 100644
> --- a/hw/fw_cfg.c
> +++ b/hw/fw_cfg.c
> @@ -53,6 +53,7 @@ struct FWCfgState {
>      FWCfgFiles *files;
>      uint16_t cur_entry;
>      uint32_t cur_offset;
> +    Notifier machine_ready;
>  };
>  
>  static void fw_cfg_write(FWCfgState *s, uint8_t value)
> @@ -315,6 +316,15 @@ int fw_cfg_add_file(FWCfgState *s,  const char *filename, uint8_t *data,
>      return 1;
>  }
>  
> +static void fw_cfg_machine_ready(struct Notifier* n)
> +{
> +    uint32_t len;
> +    char *bootindex = get_boot_devices_list(&len);
> +
> +    fw_cfg_add_bytes(container_of(n, FWCfgState, machine_ready),
> +                     FW_CFG_BOOTINDEX, (uint8_t*)bootindex, len);
> +}
> +
>  FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
>                          target_phys_addr_t ctl_addr, target_phys_addr_t data_addr)
>  {
> @@ -343,6 +353,10 @@ FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
>      fw_cfg_add_i16(s, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
>      fw_cfg_add_i16(s, FW_CFG_BOOT_MENU, (uint16_t)boot_menu);
>  
> +
> +    s->machine_ready.notify = fw_cfg_machine_ready;
> +    qemu_add_machine_init_done_notifier(&s->machine_ready);
> +
>      return s;
>  }
>  
> diff --git a/hw/fw_cfg.h b/hw/fw_cfg.h
> index 856bf91..4d61410 100644
> --- a/hw/fw_cfg.h
> +++ b/hw/fw_cfg.h
> @@ -30,7 +30,9 @@
>  
>  #define FW_CFG_FILE_FIRST       0x20
>  #define FW_CFG_FILE_SLOTS       0x10
> -#define FW_CFG_MAX_ENTRY        (FW_CFG_FILE_FIRST+FW_CFG_FILE_SLOTS)
> +#define FW_CFG_FILE_LAST_SLOT   (FW_CFG_FILE_FIRST+FW_CFG_FILE_SLOTS)
> +#define FW_CFG_BOOTINDEX        (FW_CFG_FILE_LAST_SLOT + 1)
> +#define FW_CFG_MAX_ENTRY        FW_CFG_BOOTINDEX 
>  
>  #define FW_CFG_WRITE_CHANNEL    0x4000
>  #define FW_CFG_ARCH_LOCAL       0x8000
> diff --git a/sysemu.h b/sysemu.h
> index c42f33a..38a20a3 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -196,4 +196,5 @@ void register_devices(void);
>  
>  void add_boot_device_path(int32_t bootindex, DeviceState *dev,
>                            const char *suffix);
> +char *get_boot_devices_list(uint32_t *size);
>  #endif
> diff --git a/vl.c b/vl.c
> index 918d988..cca1e76 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -735,6 +735,57 @@ void add_boot_device_path(int32_t bootindex, DeviceState *dev,
>      QTAILQ_INSERT_TAIL(&fw_boot_order, node, link);
>  }
>  
> +/*
> + * This function returns device list as an array in a below format:
> + * +-----+-----+---------------+-----+---------------+--
> + * |  n  |  l1 |   devpath1    |  l2 |  devpath2     | ...
> + * +-----+-----+---------------+-----+---------------+--

No one will ever want > 256 devices? Let's make it 4 byte or something.

> + * where:
> + *   n - a number of devise pathes (one byte)
> + *   l - length of following device path string (one byte)

Might not fit: with pci we can have 256 nested buses.
How about simply null-terminating each path?

> + *   devpath - non-null terminated string of length l representing
> + *             one device path
> + */

Document return value + parameters as well?

> +char *get_boot_devices_list(uint32_t *size)
> +{
> +    FWBootEntry *i;
> +    uint32_t total = 1, c = 0;
> +    char *list = qemu_malloc(1);
> +
> +    QTAILQ_FOREACH(i, &fw_boot_order, link) {
> +        char *devpath = NULL, *bootpath;
> +        int len;
> +
> +        if (i->dev) {
> +            devpath = qdev_get_fw_dev_path(i->dev);
> +            assert(devpath);
> +        }
> +
> +        if (i->suffix && devpath) {
> +            bootpath = qemu_malloc(strlen(devpath) + strlen(i->suffix) + 2);
> +            sprintf(bootpath, "%s/%s", devpath, i->suffix);
> +            qemu_free(devpath);

devpath is allocated with strdup, not qemu_malloc,
so I guess it should be freed with free?
Alternatively, let's add qemu_strdup
Might be a good idea: fix error handling here and elsewhere.

> +        } else if (devpath) {
> +            bootpath = devpath;
> +        } else {
> +            bootpath = strdup(i->suffix);
> +        }

assert(bootpath).

> +
> +        len = strlen(bootpath);
> +        list = qemu_realloc(list, total + len + 1);
> +        list[total++] = len;
> +        memcpy(&list[total], bootpath, len);
> +        total += len;
> +        c++;
> +        qemu_free(bootpath);

Man, is this tricky.

> +    }
> +
> +    list[0] = c;
> +    *size = total;
> +
> +    return list;
> +}
> +
>  static void numa_add(const char *optarg)
>  {
>      char option[128];
> -- 
> 1.7.1