On 29.01.15 08:48, Markus Armbruster wrote:
> 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'd rather not modify the "base fdt" ;). Today we have a pretty simple
layout that does
base fdt -> init
runtime changes -> reset
I'd prefer to keep it that way, so over the lifetime of a VM, the base
fdt shouldn't change.
> 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().
If people are against it (though I can't see why), we can also move the
logic to spapr.c and change the machine->boot_order in a local callback
to set_boot_handler().
Alex
