Dinar Valeev <dval...@suse.de> writes:
> On 01/28/2015 02:48 AM, Gonglei wrote:
>> On 2015/1/27 18:49, Dinar Valeev wrote:
>>
>>> On 01/27/2015 10:18 AM, Gonglei wrote:
>>>> On 2015/1/27 16:57, Dinar Valeev wrote:
>>>>
>>>>> On 01/27/2015 03:51 AM, Gonglei wrote:
>>>>>> On 2015/1/27 7:52, dval...@suse.de wrote:
>>>>>>
>>>>>>> From: Dinar Valeev <dval...@suse.com>
>>>>>>>
>>>>>>> on sPAPR we need to update boot_order in MachineState in case it
>>>>>>> got changed on reset.
>>>>>>>
>>>>>>> Signed-off-by: Dinar Valeev <dval...@suse.com>
>>>>>>> ---
>>>>>>> bootdevice.c | 3 +++
>>>>>>> 1 file changed, 3 insertions(+)
>>>>>>>
>>>>>>> diff --git a/bootdevice.c b/bootdevice.c
>>>>>>> index 5914417..4f11a06 100644
>>>>>>> --- a/bootdevice.c
>>>>>>> +++ b/bootdevice.c
>>>>>>> @@ -26,6 +26,7 @@
>>>>>>> #include "qapi/visitor.h"
>>>>>>> #include "qemu/error-report.h"
>>>>>>> #include "hw/hw.h"
>>>>>>> +#include "hw/boards.h"
>>>>>>>
>>>>>>> typedef struct FWBootEntry FWBootEntry;
>>>>>>>
>>>>>>> @@ -50,6 +51,8 @@ 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());
>>>>>>> + machine->boot_order = boot_order;
>>>>>>>
>>>>>>> if (!boot_set_handler) {
>>>>>>> error_setg(errp, "no function defined to set boot device list
>>>>>>> for"
>>>>>>
>>>>>> Have you registered boot set handler on ppc/sPAPR platform by calling
>>>>>> qemu_register_boot_set()? Otherwise qemu_boot_set function
>>>>>> will return error.
>>>>> No, I set boot_order on each machine reset. My tests are showing
>>>>> it works without an error.
>>>>
>>>> That's interesting. Does this function be called?
>>> Yes, then simply returns.
>>>> Would you debug it by setting a breakpoint ?
>>> I added a trace event.
>>> if (!boot_set_handler) {
>>> + trace_qemu_boot_set(boot_order);
>>> error_setg(errp, "no function defined to set boot device list for"
>>> " this architecture");
>>> return;
>>>
>>> And I see this now in qemu's monitor. Still I don't see error message.
>>
>> That's because NULL is passed to this function in restore_boot_order()
>> the error is ignored (commit f183993). I have seen the previous conversation
>> about your patch serials. And I think this is the reason which
>> you moved machine->boot_order = boot_order before
>> checking boot_set_handler variable based on Alexander's
>> suggestion, right? But I think this is not a good idea.
> Yes
>
> Any proposal how this can be done differently?
>
> It seems I'm almost alone who wants -boot once=X option to get fixed
> for sPAPR. We use it in test automation, and we need to be sure that
> we boot from hard disk once installation is done.
The crash is a bug, and bugs need fixing.
You first patch looked okay to me. I suggested dropping the null check
in spapr_create_fdt_skel(), because I feel it's papering over the bug
you fix.
This isn't a review of your second attempt, it's encouragement. I
haven't looked at this version, nor have I thought through Alex's idea
to patch machine->boot_order in qemu_boot_set().
