Re: [Qemu-devel] [PATCH] fw_cfg: add bootdevice-ignore-suffixes property

[Mark Cave-Ayland]

On 08/08/18 20:53, Eduardo Habkost wrote:

> On Wed, Aug 08, 2018 at 08:19:51PM +0100, Mark Cave-Ayland wrote:
> > For the older machines (such as Mac and SPARC) the DT nodes representing
> > bootdevices for disk nodes are irregular for mainly historical reasons.
> >
> > Since the majority of bootdevice nodes for these machines either do not have a
> > separate disk node or require different (custom) names then it is much easier
> > to disable all suffixes for a particular machine by setting the ignore_suffixes
> > parameter to get_boot_devices_list() to true, and customise the disk nodes as
> > required.
> >
> > Here we add a new bootdevice-ignore-suffixes property to the FW_CFG device to
> > allow the generation of disk suffixes to be controlled on a per-machine basis.
> >
> > Signed-off-by: Mark Cave-Ayland <address@hidden>
>
> Reviewed-by: Eduardo Habkost <address@hidden>
>
> But I would prefer to see this merged only after we see machines
> actually using the property.  Can you send that as a single
> series later?

I don't have any more time until tomorrow evening now, but FWIW I've 
pushed my working branch to 
https://github.com/mcayland/qemu/commits/openbios-bootindex3 if you want 
to take a quick look. Example command line:

$ ./qemu-system-sparc64 -drive 
file=disk.img,if=none,index=0,id=cd,media=cdrom -device 
virtio-blk-pci,bus=pciB,drive=cd,bootindex=0 -m 256 -nographic

Would you still like me to post this to the list properly tomorrow evening?

> Also, maybe we can do it in a simpler way:
>
> I now see that fw_cfg is not the only user of
> get_boot_devices_list().  I didn't want to have a fw_cfg-specific
> field in MachineClass, but but we can make it not fw_cfg-specific
> if we make it affect all get_boot_devices_list() calls.
>
> What do you think of the patch below?
>
> (Patch is untested)
>
> Signed-off-by: Eduardo Habkost <address@hidden>
> ---
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index d139a431a6..f82f28468b 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -206,6 +206,7 @@ struct MachineClass {
>       bool auto_enable_numa_with_memhp;
>       void (*numa_auto_assign_ram)(MachineClass *mc, NodeInfo *nodes,
>                                    int nb_nodes, ram_addr_t size);
> +    bool ignore_boot_device_suffixes;
>   
>       HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
>                                              DeviceState *dev);
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 76ef6196a7..8d6095d98b 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -182,7 +182,7 @@ void hmp_info_usb(Monitor *mon, const QDict *qdict);
>   
>   void add_boot_device_path(int32_t bootindex, DeviceState *dev,
>                             const char *suffix);
> -char *get_boot_devices_list(size_t *size, bool ignore_suffixes);
> +char *get_boot_devices_list(size_t *size);
>   
>   DeviceState *get_boot_device(uint32_t position);
>   void check_boot_index(int32_t bootindex, Error **errp);
> diff --git a/bootdevice.c b/bootdevice.c
> index 1141009114..1d225202f9 100644
> --- a/bootdevice.c
> +++ b/bootdevice.c
> @@ -29,6 +29,7 @@
>   #include "qemu/error-report.h"
>   #include "sysemu/reset.h"
>   #include "hw/qdev-core.h"
> +#include "hw/boards.h"
>   
>   typedef struct FWBootEntry FWBootEntry;
>   
> @@ -208,11 +209,13 @@ DeviceState *get_boot_device(uint32_t position)
>    * memory pointed by "size" is assigned total length of the array in bytes
>    *
>    */
> -char *get_boot_devices_list(size_t *size, bool ignore_suffixes)
> +char *get_boot_devices_list(size_t *size)
>   {
>       FWBootEntry *i;
>       size_t total = 0;
>       char *list = NULL;
> +    MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
> +    bool ignore_suffixes = mc->ignore_boot_device_suffixes;
>   
>       QTAILQ_FOREACH(i, &fw_boot_order, link) {
>           char *devpath = NULL,  *suffix = NULL;
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index b23e7f64a8..d79a568f54 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -861,7 +861,7 @@ static void fw_cfg_machine_reset(void *opaque)
>       void *ptr;
>       size_t len;
>       FWCfgState *s = opaque;
> -    char *bootindex = get_boot_devices_list(&len, false);
> +    char *bootindex = get_boot_devices_list(&len);
>   
>       ptr = fw_cfg_modify_file(s, "bootorder", (uint8_t *)bootindex, len);
>       g_free(ptr);
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 421b2dd09b..47bc63b085 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1160,7 +1160,7 @@ static void spapr_dt_chosen(sPAPRMachineState *spapr, 
> void *fdt)
>       const char *boot_device = machine->boot_order;
>       char *stdout_path = spapr_vio_stdout_path(spapr->vio_bus);
>       size_t cb = 0;
> -    char *bootlist = get_boot_devices_list(&cb, true);
> +    char *bootlist = get_boot_devices_list(&cb);
>   
>       _FDT(chosen = fdt_add_subnode(fdt, 0, "chosen"));
>   
> @@ -3950,6 +3950,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>   
>       mc->desc = "pSeries Logical Partition (PAPR compliant)";
>   
> +    mc->ignore_boot_device_suffixes = true;
>       /*
>        * We set up the default / latest behaviour here.  The class_init
>        * functions for the specific versioned machine types can override

A quick glance makes me think it should work...


ATB,

Mark.