| Message ID | 54C60A51.7030705@suse.de |
|---|---|
| State | New |
| Headers | show |
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 > >
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
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
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
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
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 > > >
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
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) {