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

[Gleb Natapov <gleb@redhat.com>]

On Sun, Nov 14, 2010 at 08:41:37PM +0200, Michael S. Tsirkin wrote:
> 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.
> 
More then 256 _boot_ devices. I think this is more then reasonable.

> > + * 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.
Theoretically. But will it practically happen?

> How about simply null-terminating each path?
> 
Will be harder for Seabios. I can use more then one byte for length, but
I tried to avoid endianess handling.

> > + *   devpath - non-null terminated string of length l representing
> > + *             one device path
> > + */
> 
> Document return value + parameters as well?
> 
Return value is documented above :)

> > +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?
There is code that do it like this all over qemu.

> 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).
> 
OK

> > +
> > +        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.
> 
Nah, not at all.

--
			Gleb.