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

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

[Mark Cave-Ayland]

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-ayland@ilande.co.uk>
---
 hw/nvram/fw_cfg.c         | 9 ++++++++-
 include/hw/nvram/fw_cfg.h | 1 +
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index b23e7f64a8..52488b999f 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -861,7 +861,8 @@ 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,
+                                            s->bootdevice_ignore_suffixes);
 
     ptr = fw_cfg_modify_file(s, "bootorder", (uint8_t *)bootindex, len);
     g_free(ptr);
@@ -990,12 +991,18 @@ FWCfgState *fw_cfg_find(void)
     return FW_CFG(object_resolve_path_type("", TYPE_FW_CFG, NULL));
 }
 
+static Property fw_cfg_properties[] = {
+    DEFINE_PROP_BOOL("bootdevice-ignore-suffixes", FWCfgState,
+                     bootdevice_ignore_suffixes, false),
+    DEFINE_PROP_END_OF_LIST(),
+};
 
 static void fw_cfg_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->reset = fw_cfg_reset;
+    dc->props = fw_cfg_properties;
     dc->vmsd = &vmstate_fw_cfg;
 }
 
diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index b2259cc4a3..848c83aef4 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -58,6 +58,7 @@ struct FWCfgState {
     uint16_t cur_entry;
     uint32_t cur_offset;
     Notifier machine_ready;
+    bool bootdevice_ignore_suffixes;
 
     int fw_cfg_order_override;
 
-- 
2.11.0

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

[Laszlo Ersek]

On 08/08/18 21:19, 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-ayland@ilande.co.uk>
> ---
>  hw/nvram/fw_cfg.c         | 9 ++++++++-
>  include/hw/nvram/fw_cfg.h | 1 +
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index b23e7f64a8..52488b999f 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -861,7 +861,8 @@ 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,
> +                                            s->bootdevice_ignore_suffixes);
>  
>      ptr = fw_cfg_modify_file(s, "bootorder", (uint8_t *)bootindex, len);
>      g_free(ptr);
> @@ -990,12 +991,18 @@ FWCfgState *fw_cfg_find(void)
>      return FW_CFG(object_resolve_path_type("", TYPE_FW_CFG, NULL));
>  }
>  
> +static Property fw_cfg_properties[] = {
> +    DEFINE_PROP_BOOL("bootdevice-ignore-suffixes", FWCfgState,
> +                     bootdevice_ignore_suffixes, false),
> +    DEFINE_PROP_END_OF_LIST(),
> +};

I've got two questions which are not "loaded" -- I honestly have no clue:

- Do we intend to expose this to users and higher-level tools? If not,
should it be called "x-..." (experimental)? I can't remember the rules
about "x-" properties.

- I vaguely recall that earlier we tried to add properties to the fw_cfg
base class, but ultimately added them to the derived classes (see
"fw_cfg_mem_properties" and "fw_cfg_io_properties"). Despite the fact
that the referenced fields themselves (dma_enabled, file_slots) belong
to the base class; IOW, the properties refer to "parent_obj.xxx". I
don't really remember why we did this. I seem to recall issues
otherwise, with setting the property from the command line due to object
construction / realization order, or whatever.

Mark, can you verify whether you can control
"bootdevice-ignore-suffixes" from the command line, e.g. via "-global"?

The object model keeps scaring me. :(

Laszlo

>  
>  static void fw_cfg_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      dc->reset = fw_cfg_reset;
> +    dc->props = fw_cfg_properties;
>      dc->vmsd = &vmstate_fw_cfg;
>  }
>  
> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
> index b2259cc4a3..848c83aef4 100644
> --- a/include/hw/nvram/fw_cfg.h
> +++ b/include/hw/nvram/fw_cfg.h
> @@ -58,6 +58,7 @@ struct FWCfgState {
>      uint16_t cur_entry;
>      uint32_t cur_offset;
>      Notifier machine_ready;
> +    bool bootdevice_ignore_suffixes;
>  
>      int fw_cfg_order_override;
>  
> 

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

[Eduardo Habkost]

On Wed, Aug 08, 2018 at 09:39:49PM +0200, Laszlo Ersek wrote:
> On 08/08/18 21:19, 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-ayland@ilande.co.uk>
> > ---
> >  hw/nvram/fw_cfg.c         | 9 ++++++++-
> >  include/hw/nvram/fw_cfg.h | 1 +
> >  2 files changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> > index b23e7f64a8..52488b999f 100644
> > --- a/hw/nvram/fw_cfg.c
> > +++ b/hw/nvram/fw_cfg.c
> > @@ -861,7 +861,8 @@ 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,
> > +                                            s->bootdevice_ignore_suffixes);
> >  
> >      ptr = fw_cfg_modify_file(s, "bootorder", (uint8_t *)bootindex, len);
> >      g_free(ptr);
> > @@ -990,12 +991,18 @@ FWCfgState *fw_cfg_find(void)
> >      return FW_CFG(object_resolve_path_type("", TYPE_FW_CFG, NULL));
> >  }
> >  
> > +static Property fw_cfg_properties[] = {
> > +    DEFINE_PROP_BOOL("bootdevice-ignore-suffixes", FWCfgState,
> > +                     bootdevice_ignore_suffixes, false),
> > +    DEFINE_PROP_END_OF_LIST(),
> > +};
> 
> I've got two questions which are not "loaded" -- I honestly have no clue:
> 
> - Do we intend to expose this to users and higher-level tools? If not,
> should it be called "x-..." (experimental)? I can't remember the rules
> about "x-" properties.

That would be a good idea.  But maybe we can skip using QOM to
address this, and go back to a MachineClass field (but this time
not fw_cfg-specific).  See my other reply.


> 
> - I vaguely recall that earlier we tried to add properties to the fw_cfg
> base class, but ultimately added them to the derived classes (see
> "fw_cfg_mem_properties" and "fw_cfg_io_properties"). Despite the fact
> that the referenced fields themselves (dma_enabled, file_slots) belong
> to the base class; IOW, the properties refer to "parent_obj.xxx". I
> don't really remember why we did this. I seem to recall issues
> otherwise, with setting the property from the command line due to object
> construction / realization order, or whatever.

Maybe it was a workaround to an old bug in compat_props handling.
I will try to find out.

> 
> Mark, can you verify whether you can control
> "bootdevice-ignore-suffixes" from the command line, e.g. via "-global"?
> 
> The object model keeps scaring me. :(

You're not alone.  :(

-- 
Eduardo

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

[Eduardo Habkost]

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-ayland@ilande.co.uk>

Reviewed-by: Eduardo Habkost <ehabkost@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?

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 <ehabkost@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

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 <mark.cave-ayland@ilande.co.uk>
> 
> Reviewed-by: Eduardo Habkost <ehabkost@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 <ehabkost@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...


ATB,

Mark.

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

[Eduardo Habkost]

On Wed, Aug 08, 2018 at 09:19:31PM +0100, 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-ayland@ilande.co.uk>
> > 
> > Reviewed-by: Eduardo Habkost <ehabkost@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?

It's up to you (I don't know when you think your series will be
ready).  Maybe you'll want to adopt the patch below so you don't
need to inline your fw_cfg_init*() calls anymore?  (also, up to
you)


> > 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)
[...]

-- 
Eduardo

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

[Mark Cave-Ayland]

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-ayland@ilande.co.uk>
>>
>> Reviewed-by: Eduardo Habkost <ehabkost@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 <ehabkost@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.