Re: [Qemu-devel] [PATCH v7 3/5] shutdown: Add source information to SHUTDOWN and RESET

From: Eric Blake <eblake@redhat.com>
To: Markus Armbruster <armbru@redhat.com>
Cc: qemu-devel@nongnu.org, Peter Maydell <peter.maydell@linaro.org>,
	"open list:Overall" <kvm@vger.kernel.org>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>,
	Alexander Graf <agraf@suse.de>,
	Yongbok Kim <yongbok.kim@imgtec.com>,
	Gerd Hoffmann <kraxel@redhat.com>,
	"Edgar E. Iglesias" <edgar.iglesias@gmail.com>,
	Rob Herring <robh@kernel.org>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Magnus Damm <magnus.damm@gmail.com>,
	Christian Borntraeger <borntraeger@de.ibm.com>,
	Anthony Perard <anthony.perard@citrix.com>,
	"open list:X86" <xen-devel@lists.xenproject.org>,
	Richard Henderson <rth@twiddle.net>,
	Artyom Tarasenko <atar4qemu@gmail.com>,
	Eduardo Habkost <ehabkost@redhat.com>,
	Stefan Weil <sw@weilnetz.de>, alistair.
Subject: Re: [Qemu-devel] [PATCH v7 3/5] shutdown: Add source information to SHUTDOWN and RESET
Date: Tue, 9 May 2017 09:07:26 -0500	[thread overview]
Message-ID: <81782139-d66d-72d4-c228-9dd9073a33b9@redhat.com> (raw)
In-Reply-To: <87inlap7k1.fsf@dusky.pond.sub.org>

[-- Attachment #1.1: Type: text/plain, Size: 3348 bytes --]

On 05/09/2017 06:56 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> Time to wire up all the call sites that request a shutdown or
>> reset to use the enum added in the previous patch.
>>
>> It would have been less churn to keep the common case with no
>> arguments as meaning guest-triggered, and only modified the
>> host-triggered code paths, via a wrapper function, but then we'd
>> still have to audit that I didn't miss any host-triggered spots;
>> changing the signature forces us to double-check that I correctly
>> categorized all callers.
>>
>> Since command line options can change whether a guest reset request
>> causes an actual reset vs. a shutdown, it's easy to also add the
>> information to reset requests.
>>
>> Replay adds a FIXME to preserve the cause across the replay stream,
>> that will be tackled in the next patch.
>>

>> @@ -569,7 +569,7 @@ static void acpi_pm1_cnt_write(ACPIREGS *ar, uint16_t val)
>>          default:
>>              if (sus_typ == ar->pm1.cnt.s4_val) { /* S4 request */
>>                  qapi_event_send_suspend_disk(&error_abort);
>> -                qemu_system_shutdown_request();
>> +                qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> 
> I'm fine with using SHUTDOWN_CAUSE_GUEST_SHUTDOWN for suspend, but have
> you considered SHUTDOWN_CAUSE_GUEST_SUSPEND?

It was easy to do
s/qemu_system_shutdown_request()/qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN)/
for all hw/ files.  Harder would be picking a difference between
_SHUTDOWN and a new _SUSPEND. I can do it if hardware owners want the
distinction; but remember that this series will intentionally NOT expose
that distinction to QMP, so I don't know how much it will buy us.

>>  void qmp_stop(Error **errp)
>> @@ -105,7 +105,7 @@ void qmp_stop(Error **errp)
>>
>>  void qmp_system_reset(Error **errp)
>>  {
>> -    qemu_system_reset_request();
>> +    qemu_system_reset_request(SHUTDOWN_CAUSE_HOST_QMP);
> 
> This is the only place where we pass something other than
> SHUTDOWN_CAUSE_GUEST_RESET.  We could avoid churn the obvious way, but I
> guess having the churn eases patch review.  Okay.

Yes, and that was the comment I made in the commit message about
changing the signature everywhere instead of adding wrappers that make
the common case become the default.

>> +++ b/replay/replay.c
>> @@ -51,7 +51,8 @@ bool replay_next_event_is(int event)
>>          switch (replay_state.data_kind) {
>>          case EVENT_SHUTDOWN:
>>              replay_finish_event();
>> -            qemu_system_shutdown_request();
>> +            /* FIXME - store actual reason */
>> +            qemu_system_shutdown_request(SHUTDOWN_CAUSE_HOST_ERROR);
> 
> The temporary replay breakage is no big deal.  Still, can we avoid it by
> extending replay first, using a dummy value like
> SHUTDOWN_CAUSE_HOST_ERROR until the real cause becomes available?  Not
> sure it's worth a respin, though.
> 
>>              break;
>>          default:
>>              /* clock, time_t, checkpoint and other events */
> [...]
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]