From patchwork Thu Jan 29 11:08:46 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Gonglei (Arei)" X-Patchwork-Id: 434506 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 60DA51402A2 for ; Thu, 29 Jan 2015 22:09:49 +1100 (AEDT) Received: from localhost ([::1]:58943 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YGmyp-0002f0-IL for incoming@patchwork.ozlabs.org; Thu, 29 Jan 2015 06:09:47 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35838) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YGmyV-0002Gn-7y for qemu-devel@nongnu.org; Thu, 29 Jan 2015 06:09:28 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YGmyR-0003XD-0w for qemu-devel@nongnu.org; Thu, 29 Jan 2015 06:09:27 -0500 Received: from szxga01-in.huawei.com ([119.145.14.64]:50250) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YGmyQ-0003WY-FN; Thu, 29 Jan 2015 06:09:22 -0500 Received: from 172.24.2.119 (EHLO szxeml425-hub.china.huawei.com) ([172.24.2.119]) by szxrg01-dlp.huawei.com (MOS 4.3.7-GA FastPath queued) with ESMTP id CIS76217; Thu, 29 Jan 2015 19:09:07 +0800 (CST) Received: from [127.0.0.1] (10.177.19.102) by szxeml425-hub.china.huawei.com (10.82.67.180) with Microsoft SMTP Server id 14.3.158.1; Thu, 29 Jan 2015 19:09:03 +0800 Message-ID: <54CA14BE.2080004@huawei.com> Date: Thu, 29 Jan 2015 19:08:46 +0800 From: Gonglei User-Agent: Mozilla/5.0 (Windows NT 6.1; rv:11.0) Gecko/20120327 Thunderbird/11.0.1 MIME-Version: 1.0 To: Alexander Graf 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> <54CA1186.7060907@suse.de> In-Reply-To: <54CA1186.7060907@suse.de> X-Originating-IP: [10.177.19.102] X-CFilter-Loop: Reflected X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.4.x-2.6.x [generic] X-Received-From: 119.145.14.64 Cc: Dinar Valeev , Dinar Valeev , "qemu-ppc@nongnu.org" , "qemu-devel@nongnu.org" , "armbru@redhat.com" Subject: Re: [Qemu-devel] [PATCH 2/2] bootdevice: update boot_order in MachineState X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org On 2015/1/29 18:55, Alexander Graf wrote: > > > On 28.01.15 02:48, 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. > > Why is it not a good idea? The check is only checking whether there are > callbacks. The boot order changes nevertheless. > I mean we can't simply ignore this error. If you don't use boot_set_handler but machine->boot_order then the check should not report error. Something like this: Regards, -Gonglei diff --git a/bootdevice.c b/bootdevice.c index c3a010c..98ed2d2 100644 --- a/bootdevice.c +++ b/bootdevice.c @@ -51,19 +51,15 @@ void qemu_boot_set(const char *boot_order, Error **errp) { Error *local_err = NULL; - if (!boot_set_handler) { - error_setg(errp, "no function defined to set boot device list for" - " this architecture"); - return; - } - validate_bootdevices(boot_order, &local_err); if (local_err) { error_propagate(errp, local_err); return; } - boot_set_handler(boot_set_opaque, boot_order, errp); + if (boot_set_handler) { + boot_set_handler(boot_set_opaque, boot_order, errp); + } } void validate_bootdevices(const char *devices, Error **errp)