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

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

[Laszlo Ersek]

On 06/27/18 21:59, Mark Cave-Ayland wrote:
> On 25/06/18 08:32, Laszlo Ersek wrote:
> 
> Hi Laszlo,
> 
>>> As any class defining explicit_ofw_unit_address() has explicitly
>>> requested a
>>> specialised behaviour then it should be used in preference to the
>>> default
>>> implementation rather than being used as a fallback.
>>
>> I disagree about the last paragraph, when put like this. I don't
>> disagree with the *goal* of the patch, however the original
>> justification for explicit_ofw_unit_address() was different.
>>
>> It was meant as a fallback for distinguishing sysbus devices when those
>> sysbus devices had neither MMIO nor PIO resources. The issue wasn't that
>> MMIO/PIO-based identification was not "right", the issue was that unique
>> identification was impossible in the absence of such resources. Please
>> see commit 0b336b3b98d8 ("hw/core: explicit OFW unit address callback
>> for SysBusDeviceClass", 2015-06-23).
>>
>> I don't have anything against repurposing explicit_ofw_unit_address()
>> like this -- as long as you check that it doesn't change behavior for
>> existing devices -- it's just that we shouldn't justify the new purpose
>> with the original intent. The original intent was different.
>>
>> I suggest stating, "we can have explicit_ofw_unit_address() take
>> priority in a backwards-compatible manner, because no sysbus device
>> currently has both explicit_ofw_unit_address() and MMIO/PIO resources".
> 
> Thanks for the feedback, I'm more than happy to update the commit
> message to better describe the original intent of the patch. How does
> the following sound to you?
> 
> 
> 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 is only
> considered as a fallback option meaning that any existing MMIO memory
> regions still take priority whilst determining the firmware device address.

s/is only considered as/was originally intended only as/

and then it looks great to me.

Thank you!
Laszlo

> 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.
> 
>> (Obviously checking the validity of this statement is up to you; I'm
>> just suggesting what I'd see as one more precise explanation.)
>  Yes, it seems correct to me - grep tells me the PXB device is the only
> user of explicit_ofw_unit_address() in the whole code base, and there
> are no sysbus_init_*() functions anywhere within pci_expander_bridge.c.
> 
> 
> ATB,
> 
> Mark.

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

[Mark Cave-Ayland]

On 25/06/18 08:32, Laszlo Ersek wrote:

Hi Laszlo,

>> As any class defining explicit_ofw_unit_address() has explicitly requested a
>> specialised behaviour then it should be used in preference to the default
>> implementation rather than being used as a fallback.
> 
> I disagree about the last paragraph, when put like this. I don't
> disagree with the *goal* of the patch, however the original
> justification for explicit_ofw_unit_address() was different.
> 
> It was meant as a fallback for distinguishing sysbus devices when those
> sysbus devices had neither MMIO nor PIO resources. The issue wasn't that
> MMIO/PIO-based identification was not "right", the issue was that unique
> identification was impossible in the absence of such resources. Please
> see commit 0b336b3b98d8 ("hw/core: explicit OFW unit address callback
> for SysBusDeviceClass", 2015-06-23).
> 
> I don't have anything against repurposing explicit_ofw_unit_address()
> like this -- as long as you check that it doesn't change behavior for
> existing devices -- it's just that we shouldn't justify the new purpose
> with the original intent. The original intent was different.
> 
> I suggest stating, "we can have explicit_ofw_unit_address() take
> priority in a backwards-compatible manner, because no sysbus device
> currently has both explicit_ofw_unit_address() and MMIO/PIO resources".

Thanks for the feedback, I'm more than happy to update the commit 
message to better describe the original intent of the patch. How does 
the following sound to you?


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 is only 
considered 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.

> (Obviously checking the validity of this statement is up to you; I'm
> just suggesting what I'd see as one more precise explanation.)
  Yes, it seems correct to me - grep tells me the PXB device is the only 
user of explicit_ofw_unit_address() in the whole code base, and there 
are no sysbus_init_*() functions anywhere within pci_expander_bridge.c.


ATB,

Mark.

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

[Laszlo Ersek]

Hi Mark,

On 06/23/18 10:50, 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 an invalid
> firmware device path to be generated.
> 
> SysBusDeviceClass does provide a virtual explicit_ofw_unit_address() method that
> can be used to override this process, but it is only considered as a fallback
> option meaning that any existing MMIO memory regions still take priority whilst
> determining the firmware device address.
> 
> As any class defining explicit_ofw_unit_address() has explicitly requested a
> specialised behaviour then it should be used in preference to the default
> implementation rather than being used as a fallback.

I disagree about the last paragraph, when put like this. I don't
disagree with the *goal* of the patch, however the original
justification for explicit_ofw_unit_address() was different.

It was meant as a fallback for distinguishing sysbus devices when those
sysbus devices had neither MMIO nor PIO resources. The issue wasn't that
MMIO/PIO-based identification was not "right", the issue was that unique
identification was impossible in the absence of such resources. Please
see commit 0b336b3b98d8 ("hw/core: explicit OFW unit address callback
for SysBusDeviceClass", 2015-06-23).

I don't have anything against repurposing explicit_ofw_unit_address()
like this -- as long as you check that it doesn't change behavior for
existing devices -- it's just that we shouldn't justify the new purpose
with the original intent. The original intent was different.

I suggest stating, "we can have explicit_ofw_unit_address() take
priority in a backwards-compatible manner, because no sysbus device
currently has both explicit_ofw_unit_address() and MMIO/PIO resources".

(Obviously checking the validity of this statement is up to you; I'm
just suggesting what I'd see as one more precise explanation.)

Thanks,
Laszlo

> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  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 ecfb0cfc0e..1ee0c162f4 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));
>  }
>  
> 

[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 an invalid
firmware device path to be generated.

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

As any class defining explicit_ofw_unit_address() has explicitly requested a
specialised behaviour then it should be used in preference to the default
implementation rather than being used as a fallback.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 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 ecfb0cfc0e..1ee0c162f4 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