* [Qemu-devel] [PATCH 1/2] hw/ppc/spapr.c Set default boot order
@ 2015-01-23 22:51 dvaleev
2015-01-23 22:51 ` [Qemu-devel] [PATCH 2/2] hw/ppc/spapr Add qemu_register_boot_set for SPAPR dvaleev
` (2 more replies)
0 siblings, 3 replies; 20+ messages in thread
From: dvaleev @ 2015-01-23 22:51 UTC (permalink / raw)
To: qemu-devel; +Cc: Dinar Valeev, Alexander Graf
From: Dinar Valeev <dvaleev@suse.com>
In order to use -boot once=X option we need to have default list
where restore to on reset.
Signed-off-by: Dinar Valeev <dvaleev@suse.com>
---
hw/ppc/spapr.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index b560459..3d2cfa3 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1733,7 +1733,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
mc->block_default_type = IF_SCSI;
mc->max_cpus = MAX_CPUS;
mc->no_parallel = 1;
- mc->default_boot_order = NULL;
+ mc->default_boot_order = "cdn";
mc->kvm_type = spapr_kvm_type;
mc->has_dynamic_sysbus = true;
--
2.1.2
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [Qemu-devel] [PATCH 2/2] hw/ppc/spapr Add qemu_register_boot_set for SPAPR
2015-01-23 22:51 [Qemu-devel] [PATCH 1/2] hw/ppc/spapr.c Set default boot order dvaleev
@ 2015-01-23 22:51 ` dvaleev
2015-01-23 23:04 ` Alexander Graf
2015-01-23 23:00 ` [Qemu-devel] [PATCH 1/2] hw/ppc/spapr.c Set default boot order Alexander Graf
2015-01-26 9:11 ` Markus Armbruster
2 siblings, 1 reply; 20+ messages in thread
From: dvaleev @ 2015-01-23 22:51 UTC (permalink / raw)
To: qemu-devel; +Cc: Dinar Valeev, Alexander Graf
From: Dinar Valeev <dvaleev@suse.com>
In order to have -boot once=d functioning, it is required to have
qemu_register_boot_set
qemu-system-ppc64 -enable-kvm -boot once=d
Ready!
0 > dev /chosen ok
0 > .properties
...
qemu,boot-device d
...
0 > reset-all
Ready!
0 > dev /chosen ok
0 > .properties
...
qemu,boot-device cdn
...
Signed-off-by: Dinar Valeev <dvaleev@suse.com>
---
hw/ppc/spapr.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 3d2cfa3..38b03fc 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -314,6 +314,16 @@ static void add_str(GString *s, const gchar *s1)
g_string_append_len(s, s1, strlen(s1) + 1);
}
+static void spapr_boot_set(void *opaque, const char *boot_device,
+ Error **errp)
+{
+ int offset;
+ offset = fdt_path_offset(opaque, "/chosen");
+ fdt_setprop_string(opaque, offset, "qemu,boot-device", boot_device);
+
+}
+
+
static void *spapr_create_fdt_skel(hwaddr initrd_base,
hwaddr initrd_size,
hwaddr kernel_size,
@@ -414,6 +424,8 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
if (boot_device) {
_FDT((fdt_property_string(fdt, "qemu,boot-device", boot_device)));
}
+ qemu_register_boot_set(spapr_boot_set, fdt);
+
if (boot_menu) {
_FDT((fdt_property_cell(fdt, "qemu,boot-menu", boot_menu)));
}
--
2.1.2
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] hw/ppc/spapr.c Set default boot order
2015-01-23 22:51 [Qemu-devel] [PATCH 1/2] hw/ppc/spapr.c Set default boot order dvaleev
2015-01-23 22:51 ` [Qemu-devel] [PATCH 2/2] hw/ppc/spapr Add qemu_register_boot_set for SPAPR dvaleev
@ 2015-01-23 23:00 ` Alexander Graf
2015-01-27 4:36 ` Nikunj A Dadhania
2015-01-26 9:11 ` Markus Armbruster
2 siblings, 1 reply; 20+ messages in thread
From: Alexander Graf @ 2015-01-23 23:00 UTC (permalink / raw)
To: dvaleev, qemu-devel; +Cc: qemu-ppc, Dinar Valeev, nikunj
On 23.01.15 23:51, dvaleev@suse.de wrote:
> From: Dinar Valeev <dvaleev@suse.com>
>
> In order to use -boot once=X option we need to have default list
> where restore to on reset.
>
> Signed-off-by: Dinar Valeev <dvaleev@suse.com>
Alexey, Nijunj, where is the default boot order stored usually? Is "cdn"
an accurate equivalent?
Alex
> ---
> hw/ppc/spapr.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index b560459..3d2cfa3 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1733,7 +1733,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
> mc->block_default_type = IF_SCSI;
> mc->max_cpus = MAX_CPUS;
> mc->no_parallel = 1;
> - mc->default_boot_order = NULL;
> + mc->default_boot_order = "cdn";
> mc->kvm_type = spapr_kvm_type;
> mc->has_dynamic_sysbus = true;
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] hw/ppc/spapr Add qemu_register_boot_set for SPAPR
2015-01-23 22:51 ` [Qemu-devel] [PATCH 2/2] hw/ppc/spapr Add qemu_register_boot_set for SPAPR dvaleev
@ 2015-01-23 23:04 ` Alexander Graf
2015-01-24 16:11 ` Dinar Valeev
2015-01-26 10:49 ` Dinar Valeev
0 siblings, 2 replies; 20+ messages in thread
From: Alexander Graf @ 2015-01-23 23:04 UTC (permalink / raw)
To: dvaleev, qemu-devel; +Cc: Dinar Valeev
On 23.01.15 23:51, dvaleev@suse.de wrote:
> From: Dinar Valeev <dvaleev@suse.com>
>
> In order to have -boot once=d functioning, it is required to have
> qemu_register_boot_set
>
> qemu-system-ppc64 -enable-kvm -boot once=d
>
> Ready!
> 0 > dev /chosen ok
> 0 > .properties
> ...
> qemu,boot-device d
> ...
> 0 > reset-all
>
> Ready!
> 0 > dev /chosen ok
> 0 > .properties
> ...
> qemu,boot-device cdn
> ...
>
> Signed-off-by: Dinar Valeev <dvaleev@suse.com>
> ---
> hw/ppc/spapr.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 3d2cfa3..38b03fc 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -314,6 +314,16 @@ static void add_str(GString *s, const gchar *s1)
> g_string_append_len(s, s1, strlen(s1) + 1);
> }
>
> +static void spapr_boot_set(void *opaque, const char *boot_device,
> + Error **errp)
> +{
> + int offset;
> + offset = fdt_path_offset(opaque, "/chosen");
> + fdt_setprop_string(opaque, offset, "qemu,boot-device", boot_device);
> +
> +}
> +
> +
> static void *spapr_create_fdt_skel(hwaddr initrd_base,
> hwaddr initrd_size,
> hwaddr kernel_size,
> @@ -414,6 +424,8 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
> if (boot_device) {
> _FDT((fdt_property_string(fdt, "qemu,boot-device", boot_device)));
> }
> + qemu_register_boot_set(spapr_boot_set, fdt);
If you simply move the code above (the _FDT() one) from create_fdt_skel
to spapr_finalize_fdt() you should have the same net effect and much
cleaner code :).
Alex
> +
> if (boot_menu) {
> _FDT((fdt_property_cell(fdt, "qemu,boot-menu", boot_menu)));
> }
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] hw/ppc/spapr Add qemu_register_boot_set for SPAPR
2015-01-23 23:04 ` Alexander Graf
@ 2015-01-24 16:11 ` Dinar Valeev
2015-01-26 10:49 ` Dinar Valeev
1 sibling, 0 replies; 20+ messages in thread
From: Dinar Valeev @ 2015-01-24 16:11 UTC (permalink / raw)
To: Alexander Graf, qemu-devel; +Cc: qemu-ppc, Dinar Valeev
On 01/24/2015 12:04 AM, Alexander Graf wrote:
>
>
> On 23.01.15 23:51, dvaleev@suse.de wrote:
>> From: Dinar Valeev <dvaleev@suse.com>
>>
>> In order to have -boot once=d functioning, it is required to have
>> qemu_register_boot_set
>>
>> qemu-system-ppc64 -enable-kvm -boot once=d
>>
>> Ready!
>> 0 > dev /chosen ok
>> 0 > .properties
>> ...
>> qemu,boot-device d
>> ...
>> 0 > reset-all
>>
>> Ready!
>> 0 > dev /chosen ok
>> 0 > .properties
>> ...
>> qemu,boot-device cdn
>> ...
>>
>> Signed-off-by: Dinar Valeev <dvaleev@suse.com>
>> ---
>> hw/ppc/spapr.c | 12 ++++++++++++
>> 1 file changed, 12 insertions(+)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 3d2cfa3..38b03fc 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -314,6 +314,16 @@ static void add_str(GString *s, const gchar *s1)
>> g_string_append_len(s, s1, strlen(s1) + 1);
>> }
>>
>> +static void spapr_boot_set(void *opaque, const char *boot_device,
>> + Error **errp)
>> +{
>> + int offset;
>> + offset = fdt_path_offset(opaque, "/chosen");
>> + fdt_setprop_string(opaque, offset, "qemu,boot-device", boot_device);
>> +
>> +}
>> +
>> +
>> static void *spapr_create_fdt_skel(hwaddr initrd_base,
>> hwaddr initrd_size,
>> hwaddr kernel_size,
>> @@ -414,6 +424,8 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
>> if (boot_device) {
>> _FDT((fdt_property_string(fdt, "qemu,boot-device", boot_device)));
>> }
>> + qemu_register_boot_set(spapr_boot_set, fdt);
>
> If you simply move the code above (the _FDT() one) from create_fdt_skel
> to spapr_finalize_fdt() you should have the same net effect and much
> cleaner code :).
Haven't tried it, but I suspect -boot once=d on reset will be still
equal to -boot d behaviour.
And does it make sense to split boot_device from the rest?
>
> Alex
>
>> +
>> if (boot_menu) {
>> _FDT((fdt_property_cell(fdt, "qemu,boot-menu", boot_menu)));
>> }
>>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] hw/ppc/spapr.c Set default boot order
2015-01-23 22:51 [Qemu-devel] [PATCH 1/2] hw/ppc/spapr.c Set default boot order dvaleev
2015-01-23 22:51 ` [Qemu-devel] [PATCH 2/2] hw/ppc/spapr Add qemu_register_boot_set for SPAPR dvaleev
2015-01-23 23:00 ` [Qemu-devel] [PATCH 1/2] hw/ppc/spapr.c Set default boot order Alexander Graf
@ 2015-01-26 9:11 ` Markus Armbruster
2015-01-26 10:32 ` Dinar Valeev
2 siblings, 1 reply; 20+ messages in thread
From: Markus Armbruster @ 2015-01-26 9:11 UTC (permalink / raw)
To: dvaleev; +Cc: Dinar Valeev, qemu-devel, Alexander Graf
dvaleev@suse.de writes:
> From: Dinar Valeev <dvaleev@suse.com>
>
> In order to use -boot once=X option we need to have default list
> where restore to on reset.
Really? What happens without this patch?
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] hw/ppc/spapr.c Set default boot order
2015-01-26 12:57 ` Dinar Valeev
@ 2015-01-26 9:33 ` Alexander Graf
0 siblings, 0 replies; 20+ messages in thread
From: Alexander Graf @ 2015-01-26 9:33 UTC (permalink / raw)
To: Dinar Valeev, Markus Armbruster; +Cc: Dinar Valeev, qemu-devel
On 01/26/2015 01:57 PM, Dinar Valeev wrote:
> On 01/26/2015 01:37 PM, Markus Armbruster wrote:
>> Dinar Valeev <dvaleev@suse.de> writes:
>>
>>> On 01/26/2015 10:11 AM, Markus Armbruster wrote:
>>>> dvaleev@suse.de writes:
>>>>
>>>>> From: Dinar Valeev <dvaleev@suse.com>
>>>>>
>>>>> In order to use -boot once=X option we need to have default list
>>>>> where restore to on reset.
>>>>
>>>> Really? What happens without this patch?
>>>>
>>> qemu segfaults on reset.
>>> 0 > reset-all Segmentation fault
>>
>> Next time, include a backtrace, please.
> Ok, sorry for that.
>>
>> Here's what I think happens.
>>
>> Boot order comes from --boot parameter once, order, or else the machine
>> type's .default_boot_order. The latter is null for you.
>>
>> It gets passed via ppc_spapr_init() to spapr_create_fdt_skel(), which
>> sets qemu,boot-device in the FDT to it, but only when it isn't null.
>>
>> If it comes from parameter once, we additionally register a reset
>> handler to switch it to parameter order or else .default_boot_order on
>> reset. If you specify once, but not order, this is null for you.
>>
>> On reset, reset handler restore_boot_order() runs. Unlike
>> spapr_create_fdt_skel(), it doesn't check for null, and crashes in
>> validate_bootdevices().
>>
>> Correct?
> Yes
>
> qemu_register_boot_set is implemented in PATCH 2/2. on reset
> boot_device is restored to NULL
>>
>> For me, a null .default_boot_order means "machine type does not support
>> boot order" (this is how commit c165473 treats it). Arguably, --boot
>> order and once should be rejected then.
> AFICS SLOF handles qemu,boot-device as boot device, if nothing passed
> then it goes disk, cdrom, network. Which is the same as "cdn" list.
That's not entirely true. If SLOF doesn't see qemu,boot-device, it first
goes via its NVRAM configured boot list and only if nothing is there it
falls back to "cdn".
Maybe the actual appropriate thing would be "mcdn" with m meaning "NVRAM".
Alex
>
>> If I understand you correctly, your machine type does support boot
>> order. Giving it a non-null .default_boot_order makes sense then. The
>> appropriate value depends on firmware. It could even be "".
>>
>> The null check in spapr_create_fdt_skel() looks superfluous then.
>> Consider dropping it.
>>
>> Makes sense?
>>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] hw/ppc/spapr Add qemu_register_boot_set for SPAPR
2015-01-26 10:49 ` Dinar Valeev
@ 2015-01-26 9:35 ` Alexander Graf
2015-01-29 0:40 ` Gonglei
0 siblings, 1 reply; 20+ messages in thread
From: Alexander Graf @ 2015-01-26 9:35 UTC (permalink / raw)
To: Dinar Valeev, qemu-devel; +Cc: qemu-ppc, Dinar Valeev
On 01/26/2015 11:49 AM, Dinar Valeev wrote:
> On 01/24/2015 12:04 AM, Alexander Graf wrote:
>>
>>
>> On 23.01.15 23:51, dvaleev@suse.de wrote:
>>> From: Dinar Valeev <dvaleev@suse.com>
>>>
>>> In order to have -boot once=d functioning, it is required to have
>>> qemu_register_boot_set
>>>
>>> qemu-system-ppc64 -enable-kvm -boot once=d
>>>
>>> Ready!
>>> 0 > dev /chosen ok
>>> 0 > .properties
>>> ...
>>> qemu,boot-device d
>>> ...
>>> 0 > reset-all
>>>
>>> Ready!
>>> 0 > dev /chosen ok
>>> 0 > .properties
>>> ...
>>> qemu,boot-device cdn
>>> ...
>>>
>>> Signed-off-by: Dinar Valeev <dvaleev@suse.com>
>>> ---
>>> hw/ppc/spapr.c | 12 ++++++++++++
>>> 1 file changed, 12 insertions(+)
>>>
>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>> index 3d2cfa3..38b03fc 100644
>>> --- a/hw/ppc/spapr.c
>>> +++ b/hw/ppc/spapr.c
>>> @@ -314,6 +314,16 @@ static void add_str(GString *s, const gchar *s1)
>>> g_string_append_len(s, s1, strlen(s1) + 1);
>>> }
>>>
>>> +static void spapr_boot_set(void *opaque, const char *boot_device,
>>> + Error **errp)
>>> +{
>>> + int offset;
>>> + offset = fdt_path_offset(opaque, "/chosen");
>>> + fdt_setprop_string(opaque, offset, "qemu,boot-device",
>>> boot_device);
>>> +
>>> +}
>>> +
>>> +
>>> static void *spapr_create_fdt_skel(hwaddr initrd_base,
>>> hwaddr initrd_size,
>>> hwaddr kernel_size,
>>> @@ -414,6 +424,8 @@ static void *spapr_create_fdt_skel(hwaddr
>>> initrd_base,
>>> if (boot_device) {
>>> _FDT((fdt_property_string(fdt, "qemu,boot-device",
>>> boot_device)));
>>> }
>>> + qemu_register_boot_set(spapr_boot_set, fdt);
>>
>> If you simply move the code above (the _FDT() one) from create_fdt_skel
>> to spapr_finalize_fdt() you should have the same net effect and much
>> cleaner code :).
> I've tried your proposal, on reset boot-device property stays "d"
Ugh, the machine field doesn't change on reset. I think it'd be a lot
more intuitive if it did. Can you try with the patch below applied as well?
diff --git a/bootdevice.c b/bootdevice.c
index 5914417..3b750ff 100644
--- a/bootdevice.c
+++ b/bootdevice.c
@@ -50,6 +50,7 @@ void qemu_register_boot_set(QEMUBootSetHandler *func,
void *opaque)
void qemu_boot_set(const char *boot_order, Error **errp)
{
Error *local_err = NULL;
+ MachineState *machine = MACHINE(qdev_get_machine());
if (!boot_set_handler) {
error_setg(errp, "no function defined to set boot device list for"
@@ -63,6 +64,7 @@ void qemu_boot_set(const char *boot_order, Error **errp)
return;
}
+ machine->boot_order = boot_order;
boot_set_handler(boot_set_opaque, boot_order, errp);
}
Thanks,
Alex
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] hw/ppc/spapr.c Set default boot order
2015-01-26 9:11 ` Markus Armbruster
@ 2015-01-26 10:32 ` Dinar Valeev
2015-01-26 12:37 ` Markus Armbruster
0 siblings, 1 reply; 20+ messages in thread
From: Dinar Valeev @ 2015-01-26 10:32 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Dinar Valeev, qemu-devel, Alexander Graf
On 01/26/2015 10:11 AM, Markus Armbruster wrote:
> dvaleev@suse.de writes:
>
>> From: Dinar Valeev <dvaleev@suse.com>
>>
>> In order to use -boot once=X option we need to have default list
>> where restore to on reset.
>
> Really? What happens without this patch?
>
qemu segfaults on reset.
0 > reset-all Segmentation fault
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] hw/ppc/spapr Add qemu_register_boot_set for SPAPR
2015-01-23 23:04 ` Alexander Graf
2015-01-24 16:11 ` Dinar Valeev
@ 2015-01-26 10:49 ` Dinar Valeev
2015-01-26 9:35 ` Alexander Graf
1 sibling, 1 reply; 20+ messages in thread
From: Dinar Valeev @ 2015-01-26 10:49 UTC (permalink / raw)
To: Alexander Graf, qemu-devel; +Cc: qemu-ppc, Dinar Valeev
On 01/24/2015 12:04 AM, Alexander Graf wrote:
>
>
> On 23.01.15 23:51, dvaleev@suse.de wrote:
>> From: Dinar Valeev <dvaleev@suse.com>
>>
>> In order to have -boot once=d functioning, it is required to have
>> qemu_register_boot_set
>>
>> qemu-system-ppc64 -enable-kvm -boot once=d
>>
>> Ready!
>> 0 > dev /chosen ok
>> 0 > .properties
>> ...
>> qemu,boot-device d
>> ...
>> 0 > reset-all
>>
>> Ready!
>> 0 > dev /chosen ok
>> 0 > .properties
>> ...
>> qemu,boot-device cdn
>> ...
>>
>> Signed-off-by: Dinar Valeev <dvaleev@suse.com>
>> ---
>> hw/ppc/spapr.c | 12 ++++++++++++
>> 1 file changed, 12 insertions(+)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 3d2cfa3..38b03fc 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -314,6 +314,16 @@ static void add_str(GString *s, const gchar *s1)
>> g_string_append_len(s, s1, strlen(s1) + 1);
>> }
>>
>> +static void spapr_boot_set(void *opaque, const char *boot_device,
>> + Error **errp)
>> +{
>> + int offset;
>> + offset = fdt_path_offset(opaque, "/chosen");
>> + fdt_setprop_string(opaque, offset, "qemu,boot-device", boot_device);
>> +
>> +}
>> +
>> +
>> static void *spapr_create_fdt_skel(hwaddr initrd_base,
>> hwaddr initrd_size,
>> hwaddr kernel_size,
>> @@ -414,6 +424,8 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
>> if (boot_device) {
>> _FDT((fdt_property_string(fdt, "qemu,boot-device", boot_device)));
>> }
>> + qemu_register_boot_set(spapr_boot_set, fdt);
>
> If you simply move the code above (the _FDT() one) from create_fdt_skel
> to spapr_finalize_fdt() you should have the same net effect and much
> cleaner code :).
I've tried your proposal, on reset boot-device property stays "d"
qemu,boot-device d
0 > reset-all
qemu,boot-device d
Here is what I tried:
@@ -411,9 +410,6 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
_FDT((fdt_property(fdt, "qemu,boot-kernel-le", NULL, 0)));
}
}
- if (boot_device) {
- _FDT((fdt_property_string(fdt, "qemu,boot-device", boot_device)));
- }
if (boot_menu) {
_FDT((fdt_property_cell(fdt, "qemu,boot-menu", boot_menu)));
}
@@ -730,6 +726,8 @@ static void spapr_finalize_fdt(sPAPREnvironment *spapr,
char *bootlist;
void *fdt;
sPAPRPHBState *phb;
+ MachineState *machine = MACHINE(qdev_get_machine());
+ const char *boot_device = machine->boot_order;
fdt = g_malloc(FDT_MAX_SIZE);
@@ -769,6 +767,14 @@ static void spapr_finalize_fdt(sPAPREnvironment *spapr,
fprintf(stderr, "Couldn't finalize CPU device tree properties\n");
}
+ if (boot_device) {
+ int offset = fdt_path_offset(fdt, "/chosen");
+ ret = fdt_setprop_string(fdt, offset, "qemu,boot-device",
boot_device);
+ if (ret < 0) {
+ fprintf(stderr, "Couldn't set up boot-device dt property\n");
+ }
+ }
+
bootlist = get_boot_devices_list(&cb, true);
if (cb && bootlist) {
int offset = fdt_path_offset(fdt, "/chosen");
Dinar,
>
> Alex
>
>> +
>> if (boot_menu) {
>> _FDT((fdt_property_cell(fdt, "qemu,boot-menu", boot_menu)));
>> }
>>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] hw/ppc/spapr.c Set default boot order
2015-01-26 10:32 ` Dinar Valeev
@ 2015-01-26 12:37 ` Markus Armbruster
2015-01-26 12:57 ` Dinar Valeev
0 siblings, 1 reply; 20+ messages in thread
From: Markus Armbruster @ 2015-01-26 12:37 UTC (permalink / raw)
To: Dinar Valeev; +Cc: Dinar Valeev, qemu-devel, Alexander Graf
Dinar Valeev <dvaleev@suse.de> writes:
> On 01/26/2015 10:11 AM, Markus Armbruster wrote:
>> dvaleev@suse.de writes:
>>
>>> From: Dinar Valeev <dvaleev@suse.com>
>>>
>>> In order to use -boot once=X option we need to have default list
>>> where restore to on reset.
>>
>> Really? What happens without this patch?
>>
> qemu segfaults on reset.
> 0 > reset-all Segmentation fault
Next time, include a backtrace, please.
Here's what I think happens.
Boot order comes from --boot parameter once, order, or else the machine
type's .default_boot_order. The latter is null for you.
It gets passed via ppc_spapr_init() to spapr_create_fdt_skel(), which
sets qemu,boot-device in the FDT to it, but only when it isn't null.
If it comes from parameter once, we additionally register a reset
handler to switch it to parameter order or else .default_boot_order on
reset. If you specify once, but not order, this is null for you.
On reset, reset handler restore_boot_order() runs. Unlike
spapr_create_fdt_skel(), it doesn't check for null, and crashes in
validate_bootdevices().
Correct?
For me, a null .default_boot_order means "machine type does not support
boot order" (this is how commit c165473 treats it). Arguably, --boot
order and once should be rejected then.
If I understand you correctly, your machine type does support boot
order. Giving it a non-null .default_boot_order makes sense then. The
appropriate value depends on firmware. It could even be "".
The null check in spapr_create_fdt_skel() looks superfluous then.
Consider dropping it.
Makes sense?
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] hw/ppc/spapr.c Set default boot order
2015-01-26 12:37 ` Markus Armbruster
@ 2015-01-26 12:57 ` Dinar Valeev
2015-01-26 9:33 ` Alexander Graf
0 siblings, 1 reply; 20+ messages in thread
From: Dinar Valeev @ 2015-01-26 12:57 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Dinar Valeev, qemu-devel, Alexander Graf
On 01/26/2015 01:37 PM, Markus Armbruster wrote:
> Dinar Valeev <dvaleev@suse.de> writes:
>
>> On 01/26/2015 10:11 AM, Markus Armbruster wrote:
>>> dvaleev@suse.de writes:
>>>
>>>> From: Dinar Valeev <dvaleev@suse.com>
>>>>
>>>> In order to use -boot once=X option we need to have default list
>>>> where restore to on reset.
>>>
>>> Really? What happens without this patch?
>>>
>> qemu segfaults on reset.
>> 0 > reset-all Segmentation fault
>
> Next time, include a backtrace, please.
Ok, sorry for that.
>
> Here's what I think happens.
>
> Boot order comes from --boot parameter once, order, or else the machine
> type's .default_boot_order. The latter is null for you.
>
> It gets passed via ppc_spapr_init() to spapr_create_fdt_skel(), which
> sets qemu,boot-device in the FDT to it, but only when it isn't null.
>
> If it comes from parameter once, we additionally register a reset
> handler to switch it to parameter order or else .default_boot_order on
> reset. If you specify once, but not order, this is null for you.
>
> On reset, reset handler restore_boot_order() runs. Unlike
> spapr_create_fdt_skel(), it doesn't check for null, and crashes in
> validate_bootdevices().
>
> Correct?
Yes
qemu_register_boot_set is implemented in PATCH 2/2. on reset boot_device
is restored to NULL
>
> For me, a null .default_boot_order means "machine type does not support
> boot order" (this is how commit c165473 treats it). Arguably, --boot
> order and once should be rejected then.
AFICS SLOF handles qemu,boot-device as boot device, if nothing passed
then it goes disk, cdrom, network. Which is the same as "cdn" list.
> If I understand you correctly, your machine type does support boot
> order. Giving it a non-null .default_boot_order makes sense then. The
> appropriate value depends on firmware. It could even be "".
>
> The null check in spapr_create_fdt_skel() looks superfluous then.
> Consider dropping it.
>
> Makes sense?
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] hw/ppc/spapr.c Set default boot order
2015-01-23 23:00 ` [Qemu-devel] [PATCH 1/2] hw/ppc/spapr.c Set default boot order Alexander Graf
@ 2015-01-27 4:36 ` Nikunj A Dadhania
0 siblings, 0 replies; 20+ messages in thread
From: Nikunj A Dadhania @ 2015-01-27 4:36 UTC (permalink / raw)
To: Alexander Graf, dvaleev, qemu-devel; +Cc: Dinar Valeev, qemu-ppc
Alexander Graf <agraf@suse.de> writes:
> On 23.01.15 23:51, dvaleev@suse.de wrote:
>> From: Dinar Valeev <dvaleev@suse.com>
>>
>> In order to use -boot once=X option we need to have default list
>> where restore to on reset.
>>
>> Signed-off-by: Dinar Valeev <dvaleev@suse.com>
>
> Alexey, Nijunj, where is the default boot order stored usually?
When -boot argument is not present, the default boot order is NULL for
spapr, and SLOF decides to boot from NVRAM set variable or discovered
disk.
> Is "cdn" an accurate equivalent?
No, we have changed that to NULL.
http://comments.gmane.org/gmane.comp.emulators.qemu/187318
Regards,
Nikunj
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] hw/ppc/spapr Add qemu_register_boot_set for SPAPR
2015-01-26 9:35 ` Alexander Graf
@ 2015-01-29 0:40 ` Gonglei
2015-01-29 0:42 ` Alexander Graf
0 siblings, 1 reply; 20+ messages in thread
From: Gonglei @ 2015-01-29 0:40 UTC (permalink / raw)
To: Alexander Graf; +Cc: Dinar Valeev, Dinar Valeev, qemu-ppc, qemu-devel
On 2015/1/26 17:35, Alexander Graf wrote:
> On 01/26/2015 11:49 AM, Dinar Valeev wrote:
>> On 01/24/2015 12:04 AM, Alexander Graf wrote:
>>>
>>>
>>> On 23.01.15 23:51, dvaleev@suse.de wrote:
>>>> From: Dinar Valeev <dvaleev@suse.com>
>>>>
>>>> In order to have -boot once=d functioning, it is required to have
>>>> qemu_register_boot_set
>>>>
>>>> qemu-system-ppc64 -enable-kvm -boot once=d
>>>>
>>>> Ready!
>>>> 0 > dev /chosen ok
>>>> 0 > .properties
>>>> ...
>>>> qemu,boot-device d
>>>> ...
>>>> 0 > reset-all
>>>>
>>>> Ready!
>>>> 0 > dev /chosen ok
>>>> 0 > .properties
>>>> ...
>>>> qemu,boot-device cdn
>>>> ...
>>>>
>>>> Signed-off-by: Dinar Valeev <dvaleev@suse.com>
>>>> ---
>>>> hw/ppc/spapr.c | 12 ++++++++++++
>>>> 1 file changed, 12 insertions(+)
>>>>
>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>>> index 3d2cfa3..38b03fc 100644
>>>> --- a/hw/ppc/spapr.c
>>>> +++ b/hw/ppc/spapr.c
>>>> @@ -314,6 +314,16 @@ static void add_str(GString *s, const gchar *s1)
>>>> g_string_append_len(s, s1, strlen(s1) + 1);
>>>> }
>>>>
>>>> +static void spapr_boot_set(void *opaque, const char *boot_device,
>>>> + Error **errp)
>>>> +{
>>>> + int offset;
>>>> + offset = fdt_path_offset(opaque, "/chosen");
>>>> + fdt_setprop_string(opaque, offset, "qemu,boot-device", boot_device);
>>>> +
>>>> +}
>>>> +
>>>> +
>>>> static void *spapr_create_fdt_skel(hwaddr initrd_base,
>>>> hwaddr initrd_size,
>>>> hwaddr kernel_size,
>>>> @@ -414,6 +424,8 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
>>>> if (boot_device) {
>>>> _FDT((fdt_property_string(fdt, "qemu,boot-device", boot_device)));
>>>> }
>>>> + qemu_register_boot_set(spapr_boot_set, fdt);
>>>
>>> If you simply move the code above (the _FDT() one) from create_fdt_skel
>>> to spapr_finalize_fdt() you should have the same net effect and much
>>> cleaner code :).
>> I've tried your proposal, on reset boot-device property stays "d"
>
> Ugh, the machine field doesn't change on reset. I think it'd be a lot more intuitive if it did. Can you try with the patch below applied as well?
>
This approach is not good because boot_set_handler is NULL and return error directly.
Please using qemu_register_boot_set for this purpose.
Regards,
-Gonglei
>
> diff --git a/bootdevice.c b/bootdevice.c
> index 5914417..3b750ff 100644
> --- a/bootdevice.c
> +++ b/bootdevice.c
> @@ -50,6 +50,7 @@ void qemu_register_boot_set(QEMUBootSetHandler *func, void *opaque)
> void qemu_boot_set(const char *boot_order, Error **errp)
> {
> Error *local_err = NULL;
> + MachineState *machine = MACHINE(qdev_get_machine());
>
> if (!boot_set_handler) {
> error_setg(errp, "no function defined to set boot device list for"
> @@ -63,6 +64,7 @@ void qemu_boot_set(const char *boot_order, Error **errp)
> return;
> }
>
> + machine->boot_order = boot_order;
> boot_set_handler(boot_set_opaque, boot_order, errp);
> }
>
>
>
> Thanks,
>
> Alex
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] hw/ppc/spapr Add qemu_register_boot_set for SPAPR
2015-01-29 0:40 ` Gonglei
@ 2015-01-29 0:42 ` Alexander Graf
2015-01-29 3:46 ` Gonglei
0 siblings, 1 reply; 20+ messages in thread
From: Alexander Graf @ 2015-01-29 0:42 UTC (permalink / raw)
To: Gonglei; +Cc: Dinar Valeev, Dinar Valeev, qemu-ppc, qemu-devel
On 29.01.15 01:40, Gonglei wrote:
> On 2015/1/26 17:35, Alexander Graf wrote:
>
>> On 01/26/2015 11:49 AM, Dinar Valeev wrote:
>>> On 01/24/2015 12:04 AM, Alexander Graf wrote:
>>>>
>>>>
>>>> On 23.01.15 23:51, dvaleev@suse.de wrote:
>>>>> From: Dinar Valeev <dvaleev@suse.com>
>>>>>
>>>>> In order to have -boot once=d functioning, it is required to have
>>>>> qemu_register_boot_set
>>>>>
>>>>> qemu-system-ppc64 -enable-kvm -boot once=d
>>>>>
>>>>> Ready!
>>>>> 0 > dev /chosen ok
>>>>> 0 > .properties
>>>>> ...
>>>>> qemu,boot-device d
>>>>> ...
>>>>> 0 > reset-all
>>>>>
>>>>> Ready!
>>>>> 0 > dev /chosen ok
>>>>> 0 > .properties
>>>>> ...
>>>>> qemu,boot-device cdn
>>>>> ...
>>>>>
>>>>> Signed-off-by: Dinar Valeev <dvaleev@suse.com>
>>>>> ---
>>>>> hw/ppc/spapr.c | 12 ++++++++++++
>>>>> 1 file changed, 12 insertions(+)
>>>>>
>>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>>>> index 3d2cfa3..38b03fc 100644
>>>>> --- a/hw/ppc/spapr.c
>>>>> +++ b/hw/ppc/spapr.c
>>>>> @@ -314,6 +314,16 @@ static void add_str(GString *s, const gchar *s1)
>>>>> g_string_append_len(s, s1, strlen(s1) + 1);
>>>>> }
>>>>>
>>>>> +static void spapr_boot_set(void *opaque, const char *boot_device,
>>>>> + Error **errp)
>>>>> +{
>>>>> + int offset;
>>>>> + offset = fdt_path_offset(opaque, "/chosen");
>>>>> + fdt_setprop_string(opaque, offset, "qemu,boot-device", boot_device);
>>>>> +
>>>>> +}
>>>>> +
>>>>> +
>>>>> static void *spapr_create_fdt_skel(hwaddr initrd_base,
>>>>> hwaddr initrd_size,
>>>>> hwaddr kernel_size,
>>>>> @@ -414,6 +424,8 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
>>>>> if (boot_device) {
>>>>> _FDT((fdt_property_string(fdt, "qemu,boot-device", boot_device)));
>>>>> }
>>>>> + qemu_register_boot_set(spapr_boot_set, fdt);
>>>>
>>>> If you simply move the code above (the _FDT() one) from create_fdt_skel
>>>> to spapr_finalize_fdt() you should have the same net effect and much
>>>> cleaner code :).
>>> I've tried your proposal, on reset boot-device property stays "d"
>>
>> Ugh, the machine field doesn't change on reset. I think it'd be a lot more intuitive if it did. Can you try with the patch below applied as well?
>>
>
> This approach is not good because boot_set_handler is NULL and return error directly.
> Please using qemu_register_boot_set for this purpose.
I'd personally prefer if we get rid of qemu_register_boot_set
completely. It duplicates the reset logic as well as information holder
locality (machine struct vs parameter).
Alex
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] hw/ppc/spapr Add qemu_register_boot_set for SPAPR
2015-01-29 0:42 ` Alexander Graf
@ 2015-01-29 3:46 ` Gonglei
2015-01-29 12:55 ` Alexander Graf
0 siblings, 1 reply; 20+ messages in thread
From: Gonglei @ 2015-01-29 3:46 UTC (permalink / raw)
To: Alexander Graf; +Cc: Dinar Valeev, Dinar Valeev, qemu-ppc, qemu-devel
On 2015/1/29 8:42, Alexander Graf wrote:
>
>
> On 29.01.15 01:40, Gonglei wrote:
>> On 2015/1/26 17:35, Alexander Graf wrote:
>>
>>> On 01/26/2015 11:49 AM, Dinar Valeev wrote:
>>>> On 01/24/2015 12:04 AM, Alexander Graf wrote:
>>>>>
>>>>>
>>>>> On 23.01.15 23:51, dvaleev@suse.de wrote:
>>>>>> From: Dinar Valeev <dvaleev@suse.com>
>>>>>>
>>>>>> In order to have -boot once=d functioning, it is required to have
>>>>>> qemu_register_boot_set
>>>>>>
>>>>>> qemu-system-ppc64 -enable-kvm -boot once=d
>>>>>>
>>>>>> Ready!
>>>>>> 0 > dev /chosen ok
>>>>>> 0 > .properties
>>>>>> ...
>>>>>> qemu,boot-device d
>>>>>> ...
>>>>>> 0 > reset-all
>>>>>>
>>>>>> Ready!
>>>>>> 0 > dev /chosen ok
>>>>>> 0 > .properties
>>>>>> ...
>>>>>> qemu,boot-device cdn
>>>>>> ...
>>>>>>
>>>>>> Signed-off-by: Dinar Valeev <dvaleev@suse.com>
>>>>>> ---
>>>>>> hw/ppc/spapr.c | 12 ++++++++++++
>>>>>> 1 file changed, 12 insertions(+)
>>>>>>
>>>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>>>>> index 3d2cfa3..38b03fc 100644
>>>>>> --- a/hw/ppc/spapr.c
>>>>>> +++ b/hw/ppc/spapr.c
>>>>>> @@ -314,6 +314,16 @@ static void add_str(GString *s, const gchar *s1)
>>>>>> g_string_append_len(s, s1, strlen(s1) + 1);
>>>>>> }
>>>>>>
>>>>>> +static void spapr_boot_set(void *opaque, const char *boot_device,
>>>>>> + Error **errp)
>>>>>> +{
>>>>>> + int offset;
>>>>>> + offset = fdt_path_offset(opaque, "/chosen");
>>>>>> + fdt_setprop_string(opaque, offset, "qemu,boot-device", boot_device);
>>>>>> +
>>>>>> +}
>>>>>> +
>>>>>> +
>>>>>> static void *spapr_create_fdt_skel(hwaddr initrd_base,
>>>>>> hwaddr initrd_size,
>>>>>> hwaddr kernel_size,
>>>>>> @@ -414,6 +424,8 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
>>>>>> if (boot_device) {
>>>>>> _FDT((fdt_property_string(fdt, "qemu,boot-device", boot_device)));
>>>>>> }
>>>>>> + qemu_register_boot_set(spapr_boot_set, fdt);
>>>>>
>>>>> If you simply move the code above (the _FDT() one) from create_fdt_skel
>>>>> to spapr_finalize_fdt() you should have the same net effect and much
>>>>> cleaner code :).
>>>> I've tried your proposal, on reset boot-device property stays "d"
>>>
>>> Ugh, the machine field doesn't change on reset. I think it'd be a lot more intuitive if it did. Can you try with the patch below applied as well?
>>>
>>
>> This approach is not good because boot_set_handler is NULL and return error directly.
>> Please using qemu_register_boot_set for this purpose.
>
> I'd personally prefer if we get rid of qemu_register_boot_set
> completely. It duplicates the reset logic as well as information holder
> locality (machine struct vs parameter).
>
Maybe yes. But lots of other machines do not register
reset callback. So those machines using qemu_register_boot_set()
register a handler callback achieve this purpose.
Regards,
-Gonglei
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] hw/ppc/spapr Add qemu_register_boot_set for SPAPR
2015-01-29 3:46 ` Gonglei
@ 2015-01-29 12:55 ` Alexander Graf
2015-01-29 13:00 ` Gonglei
0 siblings, 1 reply; 20+ messages in thread
From: Alexander Graf @ 2015-01-29 12:55 UTC (permalink / raw)
To: Gonglei; +Cc: Dinar Valeev, Dinar Valeev, qemu-ppc, qemu-devel
On 29.01.15 04:46, Gonglei wrote:
> On 2015/1/29 8:42, Alexander Graf wrote:
>
>>
>>
>> On 29.01.15 01:40, Gonglei wrote:
>>> On 2015/1/26 17:35, Alexander Graf wrote:
>>>
>>>> On 01/26/2015 11:49 AM, Dinar Valeev wrote:
>>>>> On 01/24/2015 12:04 AM, Alexander Graf wrote:
>>>>>>
>>>>>>
>>>>>> On 23.01.15 23:51, dvaleev@suse.de wrote:
>>>>>>> From: Dinar Valeev <dvaleev@suse.com>
>>>>>>>
>>>>>>> In order to have -boot once=d functioning, it is required to have
>>>>>>> qemu_register_boot_set
>>>>>>>
>>>>>>> qemu-system-ppc64 -enable-kvm -boot once=d
>>>>>>>
>>>>>>> Ready!
>>>>>>> 0 > dev /chosen ok
>>>>>>> 0 > .properties
>>>>>>> ...
>>>>>>> qemu,boot-device d
>>>>>>> ...
>>>>>>> 0 > reset-all
>>>>>>>
>>>>>>> Ready!
>>>>>>> 0 > dev /chosen ok
>>>>>>> 0 > .properties
>>>>>>> ...
>>>>>>> qemu,boot-device cdn
>>>>>>> ...
>>>>>>>
>>>>>>> Signed-off-by: Dinar Valeev <dvaleev@suse.com>
>>>>>>> ---
>>>>>>> hw/ppc/spapr.c | 12 ++++++++++++
>>>>>>> 1 file changed, 12 insertions(+)
>>>>>>>
>>>>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>>>>>> index 3d2cfa3..38b03fc 100644
>>>>>>> --- a/hw/ppc/spapr.c
>>>>>>> +++ b/hw/ppc/spapr.c
>>>>>>> @@ -314,6 +314,16 @@ static void add_str(GString *s, const gchar *s1)
>>>>>>> g_string_append_len(s, s1, strlen(s1) + 1);
>>>>>>> }
>>>>>>>
>>>>>>> +static void spapr_boot_set(void *opaque, const char *boot_device,
>>>>>>> + Error **errp)
>>>>>>> +{
>>>>>>> + int offset;
>>>>>>> + offset = fdt_path_offset(opaque, "/chosen");
>>>>>>> + fdt_setprop_string(opaque, offset, "qemu,boot-device", boot_device);
>>>>>>> +
>>>>>>> +}
>>>>>>> +
>>>>>>> +
>>>>>>> static void *spapr_create_fdt_skel(hwaddr initrd_base,
>>>>>>> hwaddr initrd_size,
>>>>>>> hwaddr kernel_size,
>>>>>>> @@ -414,6 +424,8 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
>>>>>>> if (boot_device) {
>>>>>>> _FDT((fdt_property_string(fdt, "qemu,boot-device", boot_device)));
>>>>>>> }
>>>>>>> + qemu_register_boot_set(spapr_boot_set, fdt);
>>>>>>
>>>>>> If you simply move the code above (the _FDT() one) from create_fdt_skel
>>>>>> to spapr_finalize_fdt() you should have the same net effect and much
>>>>>> cleaner code :).
>>>>> I've tried your proposal, on reset boot-device property stays "d"
>>>>
>>>> Ugh, the machine field doesn't change on reset. I think it'd be a lot more intuitive if it did. Can you try with the patch below applied as well?
>>>>
>>>
>>> This approach is not good because boot_set_handler is NULL and return error directly.
>>> Please using qemu_register_boot_set for this purpose.
>>
>> I'd personally prefer if we get rid of qemu_register_boot_set
>> completely. It duplicates the reset logic as well as information holder
>> locality (machine struct vs parameter).
>>
>
> Maybe yes. But lots of other machines do not register
> reset callback. So those machines using qemu_register_boot_set()
> register a handler callback achieve this purpose.
I think we're better off just registering reset handlers then.
Alex
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] hw/ppc/spapr Add qemu_register_boot_set for SPAPR
2015-01-29 12:55 ` Alexander Graf
@ 2015-01-29 13:00 ` Gonglei
2015-04-02 7:41 ` Alexey Kardashevskiy
0 siblings, 1 reply; 20+ messages in thread
From: Gonglei @ 2015-01-29 13:00 UTC (permalink / raw)
To: Alexander Graf; +Cc: Dinar Valeev, Dinar Valeev, qemu-ppc, qemu-devel
On 2015/1/29 20:55, Alexander Graf wrote:
>
>
> On 29.01.15 04:46, Gonglei wrote:
>> On 2015/1/29 8:42, Alexander Graf wrote:
>>
>>>
>>>
>>> On 29.01.15 01:40, Gonglei wrote:
>>>> On 2015/1/26 17:35, Alexander Graf wrote:
>>>>
>>>>> On 01/26/2015 11:49 AM, Dinar Valeev wrote:
>>>>>> On 01/24/2015 12:04 AM, Alexander Graf wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 23.01.15 23:51, dvaleev@suse.de wrote:
>>>>>>>> From: Dinar Valeev <dvaleev@suse.com>
>>>>>>>>
>>>>>>>> In order to have -boot once=d functioning, it is required to have
>>>>>>>> qemu_register_boot_set
>>>>>>>>
>>>>>>>> qemu-system-ppc64 -enable-kvm -boot once=d
>>>>>>>>
>>>>>>>> Ready!
>>>>>>>> 0 > dev /chosen ok
>>>>>>>> 0 > .properties
>>>>>>>> ...
>>>>>>>> qemu,boot-device d
>>>>>>>> ...
>>>>>>>> 0 > reset-all
>>>>>>>>
>>>>>>>> Ready!
>>>>>>>> 0 > dev /chosen ok
>>>>>>>> 0 > .properties
>>>>>>>> ...
>>>>>>>> qemu,boot-device cdn
>>>>>>>> ...
>>>>>>>>
>>>>>>>> Signed-off-by: Dinar Valeev <dvaleev@suse.com>
>>>>>>>> ---
>>>>>>>> hw/ppc/spapr.c | 12 ++++++++++++
>>>>>>>> 1 file changed, 12 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>>>>>>> index 3d2cfa3..38b03fc 100644
>>>>>>>> --- a/hw/ppc/spapr.c
>>>>>>>> +++ b/hw/ppc/spapr.c
>>>>>>>> @@ -314,6 +314,16 @@ static void add_str(GString *s, const gchar *s1)
>>>>>>>> g_string_append_len(s, s1, strlen(s1) + 1);
>>>>>>>> }
>>>>>>>>
>>>>>>>> +static void spapr_boot_set(void *opaque, const char *boot_device,
>>>>>>>> + Error **errp)
>>>>>>>> +{
>>>>>>>> + int offset;
>>>>>>>> + offset = fdt_path_offset(opaque, "/chosen");
>>>>>>>> + fdt_setprop_string(opaque, offset, "qemu,boot-device", boot_device);
>>>>>>>> +
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +
>>>>>>>> static void *spapr_create_fdt_skel(hwaddr initrd_base,
>>>>>>>> hwaddr initrd_size,
>>>>>>>> hwaddr kernel_size,
>>>>>>>> @@ -414,6 +424,8 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
>>>>>>>> if (boot_device) {
>>>>>>>> _FDT((fdt_property_string(fdt, "qemu,boot-device", boot_device)));
>>>>>>>> }
>>>>>>>> + qemu_register_boot_set(spapr_boot_set, fdt);
>>>>>>>
>>>>>>> If you simply move the code above (the _FDT() one) from create_fdt_skel
>>>>>>> to spapr_finalize_fdt() you should have the same net effect and much
>>>>>>> cleaner code :).
>>>>>> I've tried your proposal, on reset boot-device property stays "d"
>>>>>
>>>>> Ugh, the machine field doesn't change on reset. I think it'd be a lot more intuitive if it did. Can you try with the patch below applied as well?
>>>>>
>>>>
>>>> This approach is not good because boot_set_handler is NULL and return error directly.
>>>> Please using qemu_register_boot_set for this purpose.
>>>
>>> I'd personally prefer if we get rid of qemu_register_boot_set
>>> completely. It duplicates the reset logic as well as information holder
>>> locality (machine struct vs parameter).
>>>
>>
>> Maybe yes. But lots of other machines do not register
>> reset callback. So those machines using qemu_register_boot_set()
>> register a handler callback achieve this purpose.
>
> I think we're better off just registering reset handlers then.
>
Just like what we said in other thread, remove the check about handler,
I will post a patch soon.
Regards,
-Gonglei
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] hw/ppc/spapr Add qemu_register_boot_set for SPAPR
2015-01-29 13:00 ` Gonglei
@ 2015-04-02 7:41 ` Alexey Kardashevskiy
2015-04-02 11:23 ` Gonglei
0 siblings, 1 reply; 20+ messages in thread
From: Alexey Kardashevskiy @ 2015-04-02 7:41 UTC (permalink / raw)
To: Gonglei; +Cc: qemu-ppc, Dinar Valeev, Dinar Valeev, Alexander Graf, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 3820 bytes --]
On Fri, Jan 30, 2015 at 12:00 AM, Gonglei <arei.gonglei@huawei.com> wrote:
> On 2015/1/29 20:55, Alexander Graf wrote:
>
> >
> >
> > On 29.01.15 04:46, Gonglei wrote:
> >> On 2015/1/29 8:42, Alexander Graf wrote:
> >>
> >>>
> >>>
> >>> On 29.01.15 01:40, Gonglei wrote:
> >>>> On 2015/1/26 17:35, Alexander Graf wrote:
> >>>>
> >>>>> On 01/26/2015 11:49 AM, Dinar Valeev wrote:
> >>>>>> On 01/24/2015 12:04 AM, Alexander Graf wrote:
> >>>>>>>
> >>>>>>>
> >>>>>>> On 23.01.15 23:51, dvaleev@suse.de wrote:
> >>>>>>>> From: Dinar Valeev <dvaleev@suse.com>
> >>>>>>>>
> >>>>>>>> In order to have -boot once=d functioning, it is required to have
> >>>>>>>> qemu_register_boot_set
> >>>>>>>>
> >>>>>>>> qemu-system-ppc64 -enable-kvm -boot once=d
> >>>>>>>>
> >>>>>>>> Ready!
> >>>>>>>> 0 > dev /chosen ok
> >>>>>>>> 0 > .properties
> >>>>>>>> ...
> >>>>>>>> qemu,boot-device d
> >>>>>>>> ...
> >>>>>>>> 0 > reset-all
> >>>>>>>>
> >>>>>>>> Ready!
> >>>>>>>> 0 > dev /chosen ok
> >>>>>>>> 0 > .properties
> >>>>>>>> ...
> >>>>>>>> qemu,boot-device cdn
> >>>>>>>> ...
> >>>>>>>>
> >>>>>>>> Signed-off-by: Dinar Valeev <dvaleev@suse.com>
> >>>>>>>> ---
> >>>>>>>> hw/ppc/spapr.c | 12 ++++++++++++
> >>>>>>>> 1 file changed, 12 insertions(+)
> >>>>>>>>
> >>>>>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >>>>>>>> index 3d2cfa3..38b03fc 100644
> >>>>>>>> --- a/hw/ppc/spapr.c
> >>>>>>>> +++ b/hw/ppc/spapr.c
> >>>>>>>> @@ -314,6 +314,16 @@ static void add_str(GString *s, const gchar
> *s1)
> >>>>>>>> g_string_append_len(s, s1, strlen(s1) + 1);
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>> +static void spapr_boot_set(void *opaque, const char *boot_device,
> >>>>>>>> + Error **errp)
> >>>>>>>> +{
> >>>>>>>> + int offset;
> >>>>>>>> + offset = fdt_path_offset(opaque, "/chosen");
> >>>>>>>> + fdt_setprop_string(opaque, offset, "qemu,boot-device",
> boot_device);
> >>>>>>>> +
> >>>>>>>> +}
> >>>>>>>> +
> >>>>>>>> +
> >>>>>>>> static void *spapr_create_fdt_skel(hwaddr initrd_base,
> >>>>>>>> hwaddr initrd_size,
> >>>>>>>> hwaddr kernel_size,
> >>>>>>>> @@ -414,6 +424,8 @@ static void *spapr_create_fdt_skel(hwaddr
> initrd_base,
> >>>>>>>> if (boot_device) {
> >>>>>>>> _FDT((fdt_property_string(fdt, "qemu,boot-device",
> boot_device)));
> >>>>>>>> }
> >>>>>>>> + qemu_register_boot_set(spapr_boot_set, fdt);
> >>>>>>>
> >>>>>>> If you simply move the code above (the _FDT() one) from
> create_fdt_skel
> >>>>>>> to spapr_finalize_fdt() you should have the same net effect and
> much
> >>>>>>> cleaner code :).
> >>>>>> I've tried your proposal, on reset boot-device property stays "d"
> >>>>>
> >>>>> Ugh, the machine field doesn't change on reset. I think it'd be a
> lot more intuitive if it did. Can you try with the patch below applied as
> well?
> >>>>>
> >>>>
> >>>> This approach is not good because boot_set_handler is NULL and return
> error directly.
> >>>> Please using qemu_register_boot_set for this purpose.
> >>>
> >>> I'd personally prefer if we get rid of qemu_register_boot_set
> >>> completely. It duplicates the reset logic as well as information holder
> >>> locality (machine struct vs parameter).
> >>>
> >>
> >> Maybe yes. But lots of other machines do not register
> >> reset callback. So those machines using qemu_register_boot_set()
> >> register a handler callback achieve this purpose.
> >
> > I think we're better off just registering reset handlers then.
> >
>
> Just like what we said in other thread, remove the check about handler,
> I will post a patch soon.
>
Have you posted the patch? Cannot find it in the lists so I assume you have
not :-) Thanks!
>
> Regards,
> -Gonglei
>
>
>
--
--
Alexey
[-- Attachment #2: Type: text/html, Size: 6692 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] hw/ppc/spapr Add qemu_register_boot_set for SPAPR
2015-04-02 7:41 ` Alexey Kardashevskiy
@ 2015-04-02 11:23 ` Gonglei
0 siblings, 0 replies; 20+ messages in thread
From: Gonglei @ 2015-04-02 11:23 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: qemu-ppc, Dinar Valeev, Dinar Valeev, Alexander Graf, qemu-devel
On 2015/4/2 15:41, Alexey Kardashevskiy wrote:
>>
>> Just like what we said in other thread, remove the check about handler,
>> I will post a patch soon.
>>
>
>
> Have you posted the patch? Cannot find it in the lists so I assume you have
> not :-) Thanks!
>
I had posted the patch which caused a regression about HMP command
hmp_boot_set(). By discussion again and again, we decide to remain the
original style. If you guys want to use this feature, please register the handler.
BTW I saw somebody had submitted patches with handler in the maillist. :)
Regards,
-Gonglei
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2015-04-02 11:23 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-23 22:51 [Qemu-devel] [PATCH 1/2] hw/ppc/spapr.c Set default boot order dvaleev
2015-01-23 22:51 ` [Qemu-devel] [PATCH 2/2] hw/ppc/spapr Add qemu_register_boot_set for SPAPR dvaleev
2015-01-23 23:04 ` Alexander Graf
2015-01-24 16:11 ` Dinar Valeev
2015-01-26 10:49 ` Dinar Valeev
2015-01-26 9:35 ` Alexander Graf
2015-01-29 0:40 ` Gonglei
2015-01-29 0:42 ` Alexander Graf
2015-01-29 3:46 ` Gonglei
2015-01-29 12:55 ` Alexander Graf
2015-01-29 13:00 ` Gonglei
2015-04-02 7:41 ` Alexey Kardashevskiy
2015-04-02 11:23 ` Gonglei
2015-01-23 23:00 ` [Qemu-devel] [PATCH 1/2] hw/ppc/spapr.c Set default boot order Alexander Graf
2015-01-27 4:36 ` Nikunj A Dadhania
2015-01-26 9:11 ` Markus Armbruster
2015-01-26 10:32 ` Dinar Valeev
2015-01-26 12:37 ` Markus Armbruster
2015-01-26 12:57 ` Dinar Valeev
2015-01-26 9:33 ` Alexander Graf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.