[Qemu-devel] [PATCH 0/2] machine: preparation for adding SPARC64/PPC bootindex support

[Qemu-devel] [PATCH 0/2] machine: preparation for adding SPARC64/PPC bootindex support

[Mark Cave-Ayland]

Here is a patchset that contains the preparatory work to enable upcoming
bootindex support in OpenBIOS for SPARC64/PPC.

Patch 1 enables the OFW address generated via the sysbus explicit_ofw_unit_address()
method to always take precendence when generating fw paths, and has already been
discussed on-list with a R-B from Laszlo.

Patch 2 may require further review and discussion and is a consequence of older
device trees having irregular naming conventions for disk nodes. Rather than add
extra string handling complexity in OpenBIOS for handling "/disk" nodes on a
per-interface basis, it is much easier to provide a per-machine option to remove
them completely for these machines and handle the logic in QEMU via a custom
FWPathProvider instead.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


Mark Cave-Ayland (2):
  sysbus: always allow explicit_ofw_unit_address() to override address
    generation
  fw_cfg: set the get_boot_devices_list() ignore_suffixes parameter from
    machine property

 hw/core/machine.c   |  3 +++
 hw/core/sysbus.c    | 15 +++++++--------
 hw/nvram/fw_cfg.c   |  5 ++++-
 include/hw/boards.h |  1 +
 4 files changed, 15 insertions(+), 9 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCH 1/2] sysbus: always allow explicit_ofw_unit_address() to override address generation

[Mark Cave-Ayland]

Some SysBusDevices either use sysbus_init_mmio() without
sysbus_mmio_map() or the first MMIO memory region doesn't represent the
bus address, causing a firmware device path with an invalid address to
be generated.

SysBusDeviceClass does provide a virtual explicit_ofw_unit_address()
method that can be used to override this process, but it was originally intended
only as as a fallback option meaning that any existing MMIO memory regions still
take priority whilst determining the firmware device address.

There is currently only one user of explicit_ofw_unit_address() and that
is the PCI expander bridge (PXB) device which has no MMIO/PIO resources
defined. This enables us to allow explicit_ofw_unit_address() to take
priority without affecting backwards compatibility, allowing the address
to be customised as required.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/core/sysbus.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
index 3c8e53b188..7ac36ad3e7 100644
--- a/hw/core/sysbus.c
+++ b/hw/core/sysbus.c
@@ -293,16 +293,8 @@ static char *sysbus_get_fw_dev_path(DeviceState *dev)
 {
     SysBusDevice *s = SYS_BUS_DEVICE(dev);
     SysBusDeviceClass *sbc = SYS_BUS_DEVICE_GET_CLASS(s);
-    /* for the explicit unit address fallback case: */
     char *addr, *fw_dev_path;
 
-    if (s->num_mmio) {
-        return g_strdup_printf("%s@" TARGET_FMT_plx, qdev_fw_name(dev),
-                               s->mmio[0].addr);
-    }
-    if (s->num_pio) {
-        return g_strdup_printf("%s@i%04x", qdev_fw_name(dev), s->pio[0]);
-    }
     if (sbc->explicit_ofw_unit_address) {
         addr = sbc->explicit_ofw_unit_address(s);
         if (addr) {
@@ -311,6 +303,13 @@ static char *sysbus_get_fw_dev_path(DeviceState *dev)
             return fw_dev_path;
         }
     }
+    if (s->num_mmio) {
+        return g_strdup_printf("%s@" TARGET_FMT_plx, qdev_fw_name(dev),
+                               s->mmio[0].addr);
+    }
+    if (s->num_pio) {
+        return g_strdup_printf("%s@i%04x", qdev_fw_name(dev), s->pio[0]);
+    }
     return g_strdup(qdev_fw_name(dev));
 }
 
-- 
2.11.0

[Qemu-devel] [PATCH 2/2] fw_cfg: set the get_boot_devices_list() ignore_suffixes parameter from machine 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, and
should be handled on an individual basis via a custom FWPathProvider.

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 allow the ignore_suffixes parameter to be set on a per-machine basis via
a machine property.

The default value for this new fwcfg_bootdevice_ignore_suffixes machine
property is false to preserve compatibility for existing machines.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/core/machine.c   | 3 +++
 hw/nvram/fw_cfg.c   | 5 ++++-
 include/hw/boards.h | 1 +
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index a9aeb22f03..fbadb35865 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -525,6 +525,9 @@ static void machine_class_init(ObjectClass *oc, void *data)
     mc->default_ram_size = 128 * MiB;
     mc->rom_file_has_mr = true;
 
+    /* Default to using fwcfg bootdevice suffixes */
+    mc->fwcfg_bootdevice_ignore_suffixes = false;
+
     /* numa node memory size aligned on 8MB by default.
      * On Linux, each node's border has to be 8MB aligned
      */
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index b23e7f64a8..ec6b8113ab 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -861,7 +861,10 @@ static void fw_cfg_machine_reset(void *opaque)
     void *ptr;
     size_t len;
     FWCfgState *s = opaque;
-    char *bootindex = get_boot_devices_list(&len, false);
+    MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
+
+    char *bootindex = get_boot_devices_list(&len,
+                          mc->fwcfg_bootdevice_ignore_suffixes);
 
     ptr = fw_cfg_modify_file(s, "bootorder", (uint8_t *)bootindex, len);
     g_free(ptr);
diff --git a/include/hw/boards.h b/include/hw/boards.h
index d139a431a6..2cf76d82a6 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -204,6 +204,7 @@ struct MachineClass {
     const char **valid_cpu_types;
     strList *allowed_dynamic_sysbus_devices;
     bool auto_enable_numa_with_memhp;
+    bool fwcfg_bootdevice_ignore_suffixes;
     void (*numa_auto_assign_ram)(MachineClass *mc, NodeInfo *nodes,
                                  int nb_nodes, ram_addr_t size);
 
-- 
2.11.0

Re: [Qemu-devel] [PATCH 1/2] sysbus: always allow explicit_ofw_unit_address() to override address generation

[Thomas Huth]

On 08/05/2018 01:28 PM, Mark Cave-Ayland wrote:
> Some SysBusDevices either use sysbus_init_mmio() without
> sysbus_mmio_map() or the first MMIO memory region doesn't represent the
> bus address, causing a firmware device path with an invalid address to
> be generated.
> 
> SysBusDeviceClass does provide a virtual explicit_ofw_unit_address()
> method that can be used to override this process, but it was originally intended
> only as as a fallback option meaning that any existing MMIO memory regions still
> take priority whilst determining the firmware device address.
> 
> There is currently only one user of explicit_ofw_unit_address() and that
> is the PCI expander bridge (PXB) device which has no MMIO/PIO resources
> defined. This enables us to allow explicit_ofw_unit_address() to take
> priority without affecting backwards compatibility, allowing the address
> to be customised as required.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  hw/core/sysbus.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)

Looks reasonable.

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH 2/2] fw_cfg: set the get_boot_devices_list() ignore_suffixes parameter from machine property

[Thomas Huth]

On 08/05/2018 01:28 PM, 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, and
> should be handled on an individual basis via a custom FWPathProvider.
> 
> 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 allow the ignore_suffixes parameter to be set on a per-machine basis via
> a machine property.
> 
> The default value for this new fwcfg_bootdevice_ignore_suffixes machine
> property is false to preserve compatibility for existing machines.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/core/machine.c   | 3 +++
>  hw/nvram/fw_cfg.c   | 5 ++++-
>  include/hw/boards.h | 1 +
>  3 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index a9aeb22f03..fbadb35865 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -525,6 +525,9 @@ static void machine_class_init(ObjectClass *oc, void *data)
>      mc->default_ram_size = 128 * MiB;
>      mc->rom_file_has_mr = true;
>  
> +    /* Default to using fwcfg bootdevice suffixes */
> +    mc->fwcfg_bootdevice_ignore_suffixes = false;

I guess you could omit this line since the memory for the machine class
is pre-initialized to zero. Anyway:

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH 2/2] fw_cfg: set the get_boot_devices_list() ignore_suffixes parameter from machine property

[Laszlo Ersek]

On 08/06/18 07:50, Thomas Huth wrote:
> On 08/05/2018 01:28 PM, 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, and
>> should be handled on an individual basis via a custom FWPathProvider.
>>
>> 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 allow the ignore_suffixes parameter to be set on a per-machine basis via
>> a machine property.
>>
>> The default value for this new fwcfg_bootdevice_ignore_suffixes machine
>> property is false to preserve compatibility for existing machines.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>  hw/core/machine.c   | 3 +++
>>  hw/nvram/fw_cfg.c   | 5 ++++-
>>  include/hw/boards.h | 1 +
>>  3 files changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index a9aeb22f03..fbadb35865 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -525,6 +525,9 @@ static void machine_class_init(ObjectClass *oc, void *data)
>>      mc->default_ram_size = 128 * MiB;
>>      mc->rom_file_has_mr = true;
>>  
>> +    /* Default to using fwcfg bootdevice suffixes */
>> +    mc->fwcfg_bootdevice_ignore_suffixes = false;
> 
> I guess you could omit this line since the memory for the machine class
> is pre-initialized to zero.

I was about to make the same recommendation.

I believe the patch should be respun for this; while the assignment is
correct / harmless, I believe we should stay consistent with the rest of
the code, and assign machine class fields when really necessary.


Another remark: I think the subject line is a bit too long (87
characters). How about:

  fw_cfg: ignore suffixes in the bootdev list dependent on machine class

(70 chars -- hopefully still precise enough)

Apologies about the bike-shedding, of course.

Thanks!
Laszlo

> Anyway:
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 

Re: [Qemu-devel] [PATCH 1/2] sysbus: always allow explicit_ofw_unit_address() to override address generation

[Eduardo Habkost]

On Sun, Aug 05, 2018 at 12:28:49PM +0100, Mark Cave-Ayland wrote:
> Some SysBusDevices either use sysbus_init_mmio() without
> sysbus_mmio_map() or the first MMIO memory region doesn't represent the
> bus address, causing a firmware device path with an invalid address to
> be generated.
> 
> SysBusDeviceClass does provide a virtual explicit_ofw_unit_address()
> method that can be used to override this process, but it was originally intended
> only as as a fallback option meaning that any existing MMIO memory regions still
> take priority whilst determining the firmware device address.
> 
> There is currently only one user of explicit_ofw_unit_address() and that
> is the PCI expander bridge (PXB) device which has no MMIO/PIO resources
> defined. This enables us to allow explicit_ofw_unit_address() to take
> priority without affecting backwards compatibility, allowing the address
> to be customised as required.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

Queueing for 3.1.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 2/2] fw_cfg: set the get_boot_devices_list() ignore_suffixes parameter from machine property

[Eduardo Habkost]

On Sun, Aug 05, 2018 at 12:28:50PM +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, and
> should be handled on an individual basis via a custom FWPathProvider.
> 
> 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 allow the ignore_suffixes parameter to be set on a per-machine basis via
> a machine property.
> 
> The default value for this new fwcfg_bootdevice_ignore_suffixes machine
> property is false to preserve compatibility for existing machines.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/core/machine.c   | 3 +++
>  hw/nvram/fw_cfg.c   | 5 ++++-
>  include/hw/boards.h | 1 +
>  3 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index a9aeb22f03..fbadb35865 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -525,6 +525,9 @@ static void machine_class_init(ObjectClass *oc, void *data)
>      mc->default_ram_size = 128 * MiB;
>      mc->rom_file_has_mr = true;
>  
> +    /* Default to using fwcfg bootdevice suffixes */
> +    mc->fwcfg_bootdevice_ignore_suffixes = false;
> +
>      /* numa node memory size aligned on 8MB by default.
>       * On Linux, each node's border has to be 8MB aligned
>       */
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index b23e7f64a8..ec6b8113ab 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -861,7 +861,10 @@ static void fw_cfg_machine_reset(void *opaque)
>      void *ptr;
>      size_t len;
>      FWCfgState *s = opaque;
> -    char *bootindex = get_boot_devices_list(&len, false);
> +    MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
> +
> +    char *bootindex = get_boot_devices_list(&len,
> +                          mc->fwcfg_bootdevice_ignore_suffixes);
>  
>      ptr = fw_cfg_modify_file(s, "bootorder", (uint8_t *)bootindex, len);
>      g_free(ptr);
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index d139a431a6..2cf76d82a6 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -204,6 +204,7 @@ struct MachineClass {
>      const char **valid_cpu_types;
>      strList *allowed_dynamic_sysbus_devices;
>      bool auto_enable_numa_with_memhp;
> +    bool fwcfg_bootdevice_ignore_suffixes;

We add MachineClass field when there's no obvious place for a
device property (that we could set using compat_props).

In this case you are controlling behavior of TYPE_FW_CFG, so I
suggest adding a compat property to TYPE_FW_CFG, and setting it
on MachineClass::compat_props.  This way we avoid adding a
fw_cfg-specific field to MachineClass.

>      void (*numa_auto_assign_ram)(MachineClass *mc, NodeInfo *nodes,
>                                   int nb_nodes, ram_addr_t size);
>  
> -- 
> 2.11.0
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 2/2] fw_cfg: set the get_boot_devices_list() ignore_suffixes parameter from machine property

[Laszlo Ersek]

On 08/06/18 22:11, Eduardo Habkost wrote:
> On Sun, Aug 05, 2018 at 12:28:50PM +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, and
>> should be handled on an individual basis via a custom FWPathProvider.
>>
>> 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 allow the ignore_suffixes parameter to be set on a per-machine basis via
>> a machine property.
>>
>> The default value for this new fwcfg_bootdevice_ignore_suffixes machine
>> property is false to preserve compatibility for existing machines.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>  hw/core/machine.c   | 3 +++
>>  hw/nvram/fw_cfg.c   | 5 ++++-
>>  include/hw/boards.h | 1 +
>>  3 files changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index a9aeb22f03..fbadb35865 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -525,6 +525,9 @@ static void machine_class_init(ObjectClass *oc, void *data)
>>      mc->default_ram_size = 128 * MiB;
>>      mc->rom_file_has_mr = true;
>>  
>> +    /* Default to using fwcfg bootdevice suffixes */
>> +    mc->fwcfg_bootdevice_ignore_suffixes = false;
>> +
>>      /* numa node memory size aligned on 8MB by default.
>>       * On Linux, each node's border has to be 8MB aligned
>>       */
>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>> index b23e7f64a8..ec6b8113ab 100644
>> --- a/hw/nvram/fw_cfg.c
>> +++ b/hw/nvram/fw_cfg.c
>> @@ -861,7 +861,10 @@ static void fw_cfg_machine_reset(void *opaque)
>>      void *ptr;
>>      size_t len;
>>      FWCfgState *s = opaque;
>> -    char *bootindex = get_boot_devices_list(&len, false);
>> +    MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
>> +
>> +    char *bootindex = get_boot_devices_list(&len,
>> +                          mc->fwcfg_bootdevice_ignore_suffixes);
>>  
>>      ptr = fw_cfg_modify_file(s, "bootorder", (uint8_t *)bootindex, len);
>>      g_free(ptr);
>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>> index d139a431a6..2cf76d82a6 100644
>> --- a/include/hw/boards.h
>> +++ b/include/hw/boards.h
>> @@ -204,6 +204,7 @@ struct MachineClass {
>>      const char **valid_cpu_types;
>>      strList *allowed_dynamic_sysbus_devices;
>>      bool auto_enable_numa_with_memhp;
>> +    bool fwcfg_bootdevice_ignore_suffixes;
> 
> We add MachineClass field when there's no obvious place for a
> device property (that we could set using compat_props).
> 
> In this case you are controlling behavior of TYPE_FW_CFG, so I
> suggest adding a compat property to TYPE_FW_CFG, and setting it
> on MachineClass::compat_props.  This way we avoid adding a
> fw_cfg-specific field to MachineClass.

Sigh, good point, I felt I was missing something!

Laszlo

> 
>>      void (*numa_auto_assign_ram)(MachineClass *mc, NodeInfo *nodes,
>>                                   int nb_nodes, ram_addr_t size);
>>  
>> -- 
>> 2.11.0
>>
>>
> 

Re: [Qemu-devel] [PATCH 2/2] fw_cfg: set the get_boot_devices_list() ignore_suffixes parameter from machine property

[Mark Cave-Ayland]

On 06/08/18 21:11, Eduardo Habkost wrote:

> On Sun, Aug 05, 2018 at 12:28:50PM +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, and
>> should be handled on an individual basis via a custom FWPathProvider.
>>
>> 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 allow the ignore_suffixes parameter to be set on a per-machine basis via
>> a machine property.
>>
>> The default value for this new fwcfg_bootdevice_ignore_suffixes machine
>> property is false to preserve compatibility for existing machines.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/core/machine.c   | 3 +++
>>   hw/nvram/fw_cfg.c   | 5 ++++-
>>   include/hw/boards.h | 1 +
>>   3 files changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index a9aeb22f03..fbadb35865 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -525,6 +525,9 @@ static void machine_class_init(ObjectClass *oc, void *data)
>>       mc->default_ram_size = 128 * MiB;
>>       mc->rom_file_has_mr = true;
>>   
>> +    /* Default to using fwcfg bootdevice suffixes */
>> +    mc->fwcfg_bootdevice_ignore_suffixes = false;
>> +
>>       /* numa node memory size aligned on 8MB by default.
>>        * On Linux, each node's border has to be 8MB aligned
>>        */
>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>> index b23e7f64a8..ec6b8113ab 100644
>> --- a/hw/nvram/fw_cfg.c
>> +++ b/hw/nvram/fw_cfg.c
>> @@ -861,7 +861,10 @@ static void fw_cfg_machine_reset(void *opaque)
>>       void *ptr;
>>       size_t len;
>>       FWCfgState *s = opaque;
>> -    char *bootindex = get_boot_devices_list(&len, false);
>> +    MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
>> +
>> +    char *bootindex = get_boot_devices_list(&len,
>> +                          mc->fwcfg_bootdevice_ignore_suffixes);
>>   
>>       ptr = fw_cfg_modify_file(s, "bootorder", (uint8_t *)bootindex, len);
>>       g_free(ptr);
>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>> index d139a431a6..2cf76d82a6 100644
>> --- a/include/hw/boards.h
>> +++ b/include/hw/boards.h
>> @@ -204,6 +204,7 @@ struct MachineClass {
>>       const char **valid_cpu_types;
>>       strList *allowed_dynamic_sysbus_devices;
>>       bool auto_enable_numa_with_memhp;
>> +    bool fwcfg_bootdevice_ignore_suffixes;
> 
> We add MachineClass field when there's no obvious place for a
> device property (that we could set using compat_props).
> 
> In this case you are controlling behavior of TYPE_FW_CFG, so I
> suggest adding a compat property to TYPE_FW_CFG, and setting it
> on MachineClass::compat_props.  This way we avoid adding a
> fw_cfg-specific field to MachineClass.

Ah I see, thanks for the pointer! Just out of curiosity, is there any 
documentation anywhere regarding compat_props?

I've managed to get something working using a fw_cfg property (patch for 
follow shortly) and the relevant section of the machine code looks like 
this:


#define HEATHROW_COMPAT \
     {\
         .driver = "fw_cfg",\
         .property = "bootdevice_ignore_suffixes",\
         .value = "on",\
     },

static void heathrow_class_init(ObjectClass *oc, void *data)
{
     MachineClass *mc = MACHINE_CLASS(oc);

     ....
     ....
     SET_MACHINE_COMPAT(mc, HEATHROW_COMPAT);
}


Is this sufficient, or are the compat properties supposed to be 
versioned according to the QEMU machine version?


ATB,

Mark.

Re: [Qemu-devel] [PATCH 2/2] fw_cfg: set the get_boot_devices_list() ignore_suffixes parameter from machine property

[Eduardo Habkost]

On Tue, Aug 07, 2018 at 08:27:30PM +0100, Mark Cave-Ayland wrote:
> On 06/08/18 21:11, Eduardo Habkost wrote:
> > On Sun, Aug 05, 2018 at 12:28:50PM +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, and
> > > should be handled on an individual basis via a custom FWPathProvider.
> > > 
> > > 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 allow the ignore_suffixes parameter to be set on a per-machine basis via
> > > a machine property.
> > > 
> > > The default value for this new fwcfg_bootdevice_ignore_suffixes machine
> > > property is false to preserve compatibility for existing machines.
> > > 
> > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
[...]
> > > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > > index d139a431a6..2cf76d82a6 100644
> > > --- a/include/hw/boards.h
> > > +++ b/include/hw/boards.h
> > > @@ -204,6 +204,7 @@ struct MachineClass {
> > >       const char **valid_cpu_types;
> > >       strList *allowed_dynamic_sysbus_devices;
> > >       bool auto_enable_numa_with_memhp;
> > > +    bool fwcfg_bootdevice_ignore_suffixes;
> > 
> > We add MachineClass field when there's no obvious place for a
> > device property (that we could set using compat_props).
> > 
> > In this case you are controlling behavior of TYPE_FW_CFG, so I
> > suggest adding a compat property to TYPE_FW_CFG, and setting it
> > on MachineClass::compat_props.  This way we avoid adding a
> > fw_cfg-specific field to MachineClass.
> 
> Ah I see, thanks for the pointer! Just out of curiosity, is there any
> documentation anywhere regarding compat_props?
> 
> I've managed to get something working using a fw_cfg property (patch for
> follow shortly) and the relevant section of the machine code looks like
> this:
> 
> 
> #define HEATHROW_COMPAT \
>     {\
>         .driver = "fw_cfg",\
>         .property = "bootdevice_ignore_suffixes",\
>         .value = "on",\
>     },
> 
> static void heathrow_class_init(ObjectClass *oc, void *data)
> {
>     MachineClass *mc = MACHINE_CLASS(oc);
> 
>     ....
>     ....
>     SET_MACHINE_COMPAT(mc, HEATHROW_COMPAT);
> }
> 
> 
> Is this sufficient, or are the compat properties supposed to be versioned
> according to the QEMU machine version?

I never saw compat_properties being used for non-versioned
machines, but it should work for this use case as well.

But, I'm not sure this is the best option.  If the machine type
is not versioned, you can simply manually set the device property
to the desired value when creating the device inside your machine
init function.  See how the "data_width" and "dma_enabled"
properties are set by existing machines, for an example.

I think moving towards more introspectable/data-driven machine
initialization may be nice, but the existing SET_MACHINE_COMPAT
interface isn't pretty.  Maybe we should refactor that interface
before increasing its usage.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 2/2] fw_cfg: set the get_boot_devices_list() ignore_suffixes parameter from machine property

[Mark Cave-Ayland]

On 07/08/18 20:45, Eduardo Habkost wrote:

>> Is this sufficient, or are the compat properties supposed to be versioned
>> according to the QEMU machine version?
> 
> I never saw compat_properties being used for non-versioned
> machines, but it should work for this use case as well.
> 
> But, I'm not sure this is the best option.  If the machine type
> is not versioned, you can simply manually set the device property
> to the desired value when creating the device inside your machine
> init function.  See how the "data_width" and "dma_enabled"
> properties are set by existing machines, for an example.

After some more digging, I can see that you are right and that we can 
actually get away with a standard qdev property for the fw_cfg device 
rather than a machine property after all - it's just that I need to 
convert both machines away from using the old fw_cfg_init() functions 
first. I'll send over what I have shortly.


ATB,

Mark.