On 08/08/18 21:19, Mark Cave-Ayland wrote: > 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 <mark.cave-ayl...@ilande.co.uk> >> >> Reviewed-by: Eduardo Habkost <ehabk...@redhat.com> >> >> 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 <ehabk...@redhat.com> >> --- >> 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...
And in fact, I've now rebased and reworked my patchset on a modified version of this patch to confirm that it works as intended - I'll send it along shortly. FWIW I'd like to get the final version of this queued first before posting the full version of my bootindex support patchset, since I've already spent quite a few hours rebasing it in order to test all these different approaches... :) ATB, Mark.
