From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60824) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YGmjV-0000PV-Gi for qemu-devel@nongnu.org; Thu, 29 Jan 2015 05:53:58 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YGmjQ-00058T-Gv for qemu-devel@nongnu.org; Thu, 29 Jan 2015 05:53:57 -0500 Message-ID: <54CA113E.2020701@suse.de> Date: Thu, 29 Jan 2015 11:53:50 +0100 From: Alexander Graf MIME-Version: 1.0 References: <1422316341-28983-1-git-send-email-dvaleev@suse.de> <1422316341-28983-3-git-send-email-dvaleev@suse.de> <54C6FD48.8000204@huawei.com> <54C752EE.5040302@suse.de> <54C757D3.4020001@huawei.com> <54C76D38.3080107@suse.de> <54C83FD2.8050208@huawei.com> <54C96109.5060807@suse.de> <87386urpr6.fsf@blackfin.pond.sub.org> In-Reply-To: <87386urpr6.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/2] bootdevice: update boot_order in MachineState List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster , Dinar Valeev Cc: "qemu-ppc@nongnu.org" , Gonglei , Dinar Valeev , "qemu-devel@nongnu.org" On 29.01.15 08:48, Markus Armbruster wrote: > Dinar Valeev 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, dvaleev@suse.de wrote: >>>>>>> >>>>>>>> From: Dinar Valeev >>>>>>>> >>>>>>>> on sPAPR we need to update boot_order in MachineState in case it >>>>>>>> got changed on reset. >>>>>>>> >>>>>>>> Signed-off-by: Dinar Valeev >>>>>>>> --- >>>>>>>> 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