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
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
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
>>
>>
>
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.
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
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.
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>
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> >
© 2016 - 2025 Red Hat, Inc.