Re: [Xen-devel] [Qemu-devel] [PATCH v2] event: Add source information to SHUTDOWN

2017-04-20 Thread Alistair Francis 
On Thu, Apr 20, 2017 at 12:05 PM, Eric Blake  wrote:
> On 04/20/2017 11:18 AM, Markus Armbruster wrote:
>> Eric Blake  writes:
>>
>>> On 04/20/2017 06:59 AM, Markus Armbruster wrote:
>>>

 No objection to Alistair's idea to turn this into an enumeration.
>>>
>>> Question - should the enum be more than just 'guest' and 'host'?  For
>>> example, my patch proves that we have a lot of places that handle
>>> complimentary machine commands to reset and shutdown, and that whether
>>> 'reset' triggers a reset (and the guest keeps running as if rebooted) or
>>> a shutdown is then based on the command-line arguments given to qemu.
>>> So having the enum differentiate between 'guest-reset' and
>>> 'guest-shutdown' would be a possibility, if we want the enum to have
>>> additional states.
>>
>> I don't know.  What I do know is that we better get the enum right:
>> while adding members is backwards-compatible, changing the member sent
>> for a specific trigger is not.  If we want to reserve the option to do
>> that anyway, we need suitable documentation.
>
> Or even this idea:
>
> { 'enum': 'ShutdownCause', 'data': [ 'shutdown', 'reset', 'panic' ] }
> { 'event': 'SHUTDOWN',
>   'data': { 'guest': 'bool', '*cause': 'ShutdownCause' } }
>
> where the enum can grow as we come up with ever more reasons worth
> exposing (maybe even 'qmp', 'gui' and 'interrupt' are reasonable causes
> for a host shutdown).  Our promise would be that 'guest' never changes
> for an existing shutdown reason, but that 'cause' may become more
> refined over time if someone expresses a need for having the distinction.
>
> Thoughts?

I'm not a fan of the 'guest' bool. I do see that it helps with
maintaining backwards compatibility but I think we would be better off
just getting the reasons right in the first place.

What about something that can grow in the future? We start with a
general guest shutdown that is always there and then as we add new
reasons things can be moved to use the new method or continue to use
the general one.

SHUTDOWN_HOST
SHUTDOWN_HOST_GUI
/* This is always a backwards compatible fall-back
 * Maybe this could be SHUTDOWN_GUEST_UNKNOWN instead?
 */
SHUTDOWN_GUEST_GENERAL
SHUTDOWN_GUEST_HALT
SHUTDOWN_GUEST_RESET
...

That way we can guarantee the base coverage but still expand more
specific reasons in the future.

I guess the only problem is that then the reasons aren't always
reliable then as we could introduce a new reason and something gets
stuck using the general fall back.

Thanks,

Alistair

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

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [Qemu-devel] [PATCH v2] event: Add source information to SHUTDOWN

2017-04-20 Thread Eric Blake 
On 04/19/2017 05:22 PM, Eric Blake wrote:
> Libvirt would like to be able to distinguish between a SHUTDOWN
> event triggered solely by guest request and one triggered by a
> SIGTERM or other action on the host.  qemu_kill_report() is
> already able to tell whether a shutdown was triggered by a host
> signal (but NOT by a host UI event, such as clicking the X on
> the window), but that information was then lost after being
> printed to stderr.
> 
> Enhance the shutdown request path to take a parameter of which
> way it is being triggered, and update ALL callers.  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.
> 
> Here is output from 'virsh qemu-monitor-event --loop' with the
> patch installed:
> 
> event SHUTDOWN at 1492639680.731251 for domain fedora_13: {"guest":true}
> event STOP at 1492639680.732116 for domain fedora_13: 
> event SHUTDOWN at 1492639680.732830 for domain fedora_13: {"guest":false}

Another reason I'll need a v3: at least qemu-iotests 87 has to be
updated for new expected output.


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



signature.asc
Description: OpenPGP digital signature
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [Qemu-devel] [PATCH v2] event: Add source information to SHUTDOWN

2017-04-20 Thread Eric Blake 
On 04/20/2017 11:18 AM, Markus Armbruster wrote:
> Eric Blake  writes:
> 
>> On 04/20/2017 06:59 AM, Markus Armbruster wrote:
>>
>>>
>>> No objection to Alistair's idea to turn this into an enumeration.
>>
>> Question - should the enum be more than just 'guest' and 'host'?  For
>> example, my patch proves that we have a lot of places that handle
>> complimentary machine commands to reset and shutdown, and that whether
>> 'reset' triggers a reset (and the guest keeps running as if rebooted) or
>> a shutdown is then based on the command-line arguments given to qemu.
>> So having the enum differentiate between 'guest-reset' and
>> 'guest-shutdown' would be a possibility, if we want the enum to have
>> additional states.
> 
> I don't know.  What I do know is that we better get the enum right:
> while adding members is backwards-compatible, changing the member sent
> for a specific trigger is not.  If we want to reserve the option to do
> that anyway, we need suitable documentation.

Or even this idea:

{ 'enum': 'ShutdownCause', 'data': [ 'shutdown', 'reset', 'panic' ] }
{ 'event': 'SHUTDOWN',
  'data': { 'guest': 'bool', '*cause': 'ShutdownCause' } }

where the enum can grow as we come up with ever more reasons worth
exposing (maybe even 'qmp', 'gui' and 'interrupt' are reasonable causes
for a host shutdown).  Our promise would be that 'guest' never changes
for an existing shutdown reason, but that 'cause' may become more
refined over time if someone expresses a need for having the distinction.

Thoughts?

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



signature.asc
Description: OpenPGP digital signature
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [Qemu-devel] [PATCH v2] event: Add source information to SHUTDOWN

2017-04-20 Thread Eric Blake 
On 04/20/2017 11:12 AM, Markus Armbruster wrote:

>>> Well technically /usr/sbin/halt just terminates all processes / kernel and
>>> halts CPUs, but the virtual machine is still active (and a 'reset' in the
>>> monitor can start it again. /usr/sbin/poweroff is what actually does the
>>> ACPI poweroff to trigger QEMU to exit[1]
>>
>> I'm thinking of this wording:
>>
>> triggered by a guest request (such as the guest running
>> /usr/sbin/poweroff to trigger an ACPI shutdown or machine halt instruction)
> 
> A quick glance at the patch suggests the instructions in question are
> typically writes to some device register.  I wouldn't call them "halt
> instructions", in particular since there's the x86 "hlt" instruction
> that does something else.

Then maybe: a guest request (such as the guest running
/usr/sbin/poweroff to trigger an ACPI or other hardware-specific
shutdown sequence)

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



signature.asc
Description: OpenPGP digital signature
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [Qemu-devel] [PATCH v2] event: Add source information to SHUTDOWN

2017-04-20 Thread Markus Armbruster 
Eric Blake  writes:

> On 04/20/2017 06:59 AM, Markus Armbruster wrote:
>
>> 
>> No objection to Alistair's idea to turn this into an enumeration.
>
> Question - should the enum be more than just 'guest' and 'host'?  For
> example, my patch proves that we have a lot of places that handle
> complimentary machine commands to reset and shutdown, and that whether
> 'reset' triggers a reset (and the guest keeps running as if rebooted) or
> a shutdown is then based on the command-line arguments given to qemu.
> So having the enum differentiate between 'guest-reset' and
> 'guest-shutdown' would be a possibility, if we want the enum to have
> additional states.

I don't know.  What I do know is that we better get the enum right:
while adding members is backwards-compatible, changing the member sent
for a specific trigger is not.  If we want to reserve the option to do
that anyway, we need suitable documentation.

>>> +++ b/vl.c
>>> @@ -1717,7 +1717,7 @@ void qemu_system_guest_panicked(GuestPanicInformation 
>>> *info)
>>>  if (!no_shutdown) {
>>>  qapi_event_send_guest_panicked(GUEST_PANIC_ACTION_POWEROFF,
>>> !!info, info, _abort);
>>> -qemu_system_shutdown_request();
>>> +qemu_system_shutdown_request(false);
>> 
>> Panicking is a guest action.  Whether the shutdown on panic should be
>> attributed to guest or host is perhaps debatable.
>
> And it relates to the idea that a guest request for a 'reset' turns into
> a qemu response of 'shutdown'.  After all, whether a guest panic results
> in a shutdown action is determined by command-line arguments to qemu.
> So if we DO want to differentiate between a guest panic and a normal
> guest shutdown, when both events are wired at the command line to cause
> the SHUTDOWN action, then that's another enum to add to my list.
>
>
>>> +++ b/replay/replay.c
>>> @@ -51,7 +51,10 @@ bool replay_next_event_is(int event)
>>>  switch (replay_state.data_kind) {
>>>  case EVENT_SHUTDOWN:
>>>  replay_finish_event();
>>> -qemu_system_shutdown_request();
>>> +/* TODO: track source of shutdown request, to replay a
>>> + * guest-initiated request rather than always claiming to
>>> + * be from the host? */
>>> +qemu_system_shutdown_request(false);
>> 
>> This is what your "need a followup patch" refers to.  I'd like to have
>> an opinion from someone familiar with replay on what exactly we need
>> here.
>
> replay-internal.h has an enum ReplayEvents, which is a list of one-byte
> codes used in the replay data stream to record which event to replay. I
> don't know if it is easier to change the replay stream to add a 2-byte
> code (shutdown-with-cause, followed by an encoding of the cause enum),
> or a range of one-byte codes (one new code per number of enum members).
> I also don't know how easy or hard it is to extend the enum (are we free
> to add events in the middle, or are we worried about back-compat to an
> older replay stream that must still play correctly with a newer qemu,
> such that new events must be higher than all existing events).
>
> So yes, I'm hoping for feedback from someone familiar with replay.
>
>> 
>> Amazing number of ways to shut down or reset a machine.
>
> And as I said, it would be easier to submit a patch with less churn by
> having the uncommon case (host-triggered) call a new
> qemu_system_shutdown_request_reason(enum), while the common case
> (guest-triggered) be handled by having qemu_system_shutdown_request()
> with no arguments call
> qemu_system_shutdown_request_reason(SHUTDOWN_GUEST).  I'm just worried
> that doing it that way makes it easy for yet another new host shutdown
> method to use the wrong wrapper.

I don't mind the churn.  It does simplify review.

>> Looks sane on first glance.
>> 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [Qemu-devel] [PATCH v2] event: Add source information to SHUTDOWN

2017-04-20 Thread Markus Armbruster 
Eric Blake  writes:

> On 04/20/2017 07:09 AM, Daniel P. Berrange wrote:
>
 +++ b/qapi/event.json
 @@ -10,6 +10,10 @@
  # Emitted when the virtual machine has shut down, indicating that qemu is
  # about to exit.
  #
 +# @guest: If true, the shutdown was triggered by a guest request (such as
 +# executing a halt instruction) rather than a host request (such as 
 sending
 +# qemu a SIGINT). (since 2.10)
 +#
>>>
>>> "executing a halt instruction" suggests "halt" is a machine instruction.
>
> Which is indeed what most of the places I patched to pass 'true' are
> emulating - the machine halt instruction.
>
>>> I think you mean /usr/sbin/halt.  Suggest something like "executing a
>>> halt command".
>> 
>> Well technically /usr/sbin/halt just terminates all processes / kernel and
>> halts CPUs, but the virtual machine is still active (and a 'reset' in the
>> monitor can start it again. /usr/sbin/poweroff is what actually does the
>> ACPI poweroff to trigger QEMU to exit[1]
>
> I'm thinking of this wording:
>
> triggered by a guest request (such as the guest running
> /usr/sbin/poweroff to trigger an ACPI shutdown or machine halt instruction)

A quick glance at the patch suggests the instructions in question are
typically writes to some device register.  I wouldn't call them "halt
instructions", in particular since there's the x86 "hlt" instruction
that does something else.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [Qemu-devel] [PATCH v2] event: Add source information to SHUTDOWN

2017-04-20 Thread Eric Blake 
On 04/20/2017 06:59 AM, Markus Armbruster wrote:

> 
> No objection to Alistair's idea to turn this into an enumeration.

Question - should the enum be more than just 'guest' and 'host'?  For
example, my patch proves that we have a lot of places that handle
complimentary machine commands to reset and shutdown, and that whether
'reset' triggers a reset (and the guest keeps running as if rebooted) or
a shutdown is then based on the command-line arguments given to qemu.
So having the enum differentiate between 'guest-reset' and
'guest-shutdown' would be a possibility, if we want the enum to have
additional states.


>> +++ b/vl.c
>> @@ -1717,7 +1717,7 @@ void qemu_system_guest_panicked(GuestPanicInformation 
>> *info)
>>  if (!no_shutdown) {
>>  qapi_event_send_guest_panicked(GUEST_PANIC_ACTION_POWEROFF,
>> !!info, info, _abort);
>> -qemu_system_shutdown_request();
>> +qemu_system_shutdown_request(false);
> 
> Panicking is a guest action.  Whether the shutdown on panic should be
> attributed to guest or host is perhaps debatable.

And it relates to the idea that a guest request for a 'reset' turns into
a qemu response of 'shutdown'.  After all, whether a guest panic results
in a shutdown action is determined by command-line arguments to qemu.
So if we DO want to differentiate between a guest panic and a normal
guest shutdown, when both events are wired at the command line to cause
the SHUTDOWN action, then that's another enum to add to my list.


>> +++ b/replay/replay.c
>> @@ -51,7 +51,10 @@ bool replay_next_event_is(int event)
>>  switch (replay_state.data_kind) {
>>  case EVENT_SHUTDOWN:
>>  replay_finish_event();
>> -qemu_system_shutdown_request();
>> +/* TODO: track source of shutdown request, to replay a
>> + * guest-initiated request rather than always claiming to
>> + * be from the host? */
>> +qemu_system_shutdown_request(false);
> 
> This is what your "need a followup patch" refers to.  I'd like to have
> an opinion from someone familiar with replay on what exactly we need
> here.

replay-internal.h has an enum ReplayEvents, which is a list of one-byte
codes used in the replay data stream to record which event to replay. I
don't know if it is easier to change the replay stream to add a 2-byte
code (shutdown-with-cause, followed by an encoding of the cause enum),
or a range of one-byte codes (one new code per number of enum members).
I also don't know how easy or hard it is to extend the enum (are we free
to add events in the middle, or are we worried about back-compat to an
older replay stream that must still play correctly with a newer qemu,
such that new events must be higher than all existing events).

So yes, I'm hoping for feedback from someone familiar with replay.

> 
> Amazing number of ways to shut down or reset a machine.

And as I said, it would be easier to submit a patch with less churn by
having the uncommon case (host-triggered) call a new
qemu_system_shutdown_request_reason(enum), while the common case
(guest-triggered) be handled by having qemu_system_shutdown_request()
with no arguments call
qemu_system_shutdown_request_reason(SHUTDOWN_GUEST).  I'm just worried
that doing it that way makes it easy for yet another new host shutdown
method to use the wrong wrapper.

> 
> Looks sane on first glance.
> 

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



signature.asc
Description: OpenPGP digital signature
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [Qemu-devel] [PATCH v2] event: Add source information to SHUTDOWN

2017-04-20 Thread Eric Blake 
On 04/20/2017 07:09 AM, Daniel P. Berrange wrote:

>>> +++ b/qapi/event.json
>>> @@ -10,6 +10,10 @@
>>>  # Emitted when the virtual machine has shut down, indicating that qemu is
>>>  # about to exit.
>>>  #
>>> +# @guest: If true, the shutdown was triggered by a guest request (such as
>>> +# executing a halt instruction) rather than a host request (such as sending
>>> +# qemu a SIGINT). (since 2.10)
>>> +#
>>
>> "executing a halt instruction" suggests "halt" is a machine instruction.

Which is indeed what most of the places I patched to pass 'true' are
emulating - the machine halt instruction.

>> I think you mean /usr/sbin/halt.  Suggest something like "executing a
>> halt command".
> 
> Well technically /usr/sbin/halt just terminates all processes / kernel and
> halts CPUs, but the virtual machine is still active (and a 'reset' in the
> monitor can start it again. /usr/sbin/poweroff is what actually does the
> ACPI poweroff to trigger QEMU to exit[1]

I'm thinking of this wording:

triggered by a guest request (such as the guest running
/usr/sbin/poweroff to trigger an ACPI shutdown or machine halt instruction)


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



signature.asc
Description: OpenPGP digital signature
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [Qemu-devel] [PATCH v2] event: Add source information to SHUTDOWN

2017-04-20 Thread Daniel P. Berrange 
On Thu, Apr 20, 2017 at 01:59:29PM +0200, Markus Armbruster wrote:
> Eric Blake  writes:
> 
> > Libvirt would like to be able to distinguish between a SHUTDOWN
> > event triggered solely by guest request and one triggered by a
> > SIGTERM or other action on the host.  qemu_kill_report() is
> > already able to tell whether a shutdown was triggered by a host
> > signal (but NOT by a host UI event, such as clicking the X on
> > the window), but that information was then lost after being
> > printed to stderr.
> >
> > Enhance the shutdown request path to take a parameter of which
> > way it is being triggered, and update ALL callers.  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.
> >
> > Here is output from 'virsh qemu-monitor-event --loop' with the
> > patch installed:
> >
> > event SHUTDOWN at 1492639680.731251 for domain fedora_13: {"guest":true}
> > event STOP at 1492639680.732116 for domain fedora_13: 
> > event SHUTDOWN at 1492639680.732830 for domain fedora_13: {"guest":false}
> >
> > Note that libvirt runs qemu with -no-quit: the first SHUTDOWN event
> > was triggered by an action I took directly in the guest (shutdown -h),
> > at which point qemu stops the vcpus and waits for libvirt to do any
> > final cleanups; the second SHUTDOWN event is the result of libvirt
> > sending SIGTERM now that it has completed cleanup.
> >
> > See also https://bugzilla.redhat.com/1384007
> >
> > Signed-off-by: Eric Blake 
> >
> > ---
> >
> > I did not wire up the RESET event to report guest-triggered, although
> > I had to plumb that through all the guests since qemu has options that
> > allow remapping reset to shutdown.  It's easy to add that if we want
> > it in v3.
> >
> > The replay driver needs a followup patch if we want to be able to
> > faithfully replay the difference between a host- and guest-initiated
> > shutdown (for now, the replayed event is always attributed to host).
> >
> >
> > v2: retitle (was "event: Add signal information to SHUTDOWN"),
> > completely rework to post bool based on whether it is guest-initiated
> > v1: initial submission, exposing just Unix signals from host
> > ---
> >  qapi/event.json |  8 ++--
> >  include/sysemu/sysemu.h |  4 ++--
> >  vl.c| 19 +++
> >  hw/acpi/core.c  |  4 ++--
> >  hw/arm/highbank.c   |  4 ++--
> >  hw/arm/integratorcp.c   |  2 +-
> >  hw/arm/musicpal.c   |  2 +-
> >  hw/arm/omap1.c  |  6 +++---
> >  hw/arm/omap2.c  |  2 +-
> >  hw/arm/spitz.c  |  2 +-
> >  hw/arm/stellaris.c  |  2 +-
> >  hw/arm/tosa.c   |  2 +-
> >  hw/i386/pc.c|  2 +-
> >  hw/input/pckbd.c|  4 ++--
> >  hw/ipmi/ipmi.c  |  4 ++--
> >  hw/isa/lpc_ich9.c   |  2 +-
> >  hw/mips/boston.c|  2 +-
> >  hw/mips/mips_malta.c|  2 +-
> >  hw/mips/mips_r4k.c  |  4 ++--
> >  hw/misc/arm_sysctl.c|  8 
> >  hw/misc/cbus.c  |  2 +-
> >  hw/misc/macio/cuda.c|  4 ++--
> >  hw/misc/slavio_misc.c   |  4 ++--
> >  hw/misc/zynq_slcr.c |  2 +-
> >  hw/pci-host/apb.c   |  4 ++--
> >  hw/pci-host/bonito.c|  2 +-
> >  hw/pci-host/piix.c  |  2 +-
> >  hw/ppc/e500.c   |  2 +-
> >  hw/ppc/mpc8544_guts.c   |  2 +-
> >  hw/ppc/ppc.c|  2 +-
> >  hw/ppc/ppc405_uc.c  |  2 +-
> >  hw/ppc/spapr_hcall.c|  2 +-
> >  hw/ppc/spapr_rtas.c |  4 ++--
> >  hw/s390x/ipl.c  |  2 +-
> >  hw/sh4/r2d.c|  2 +-
> >  hw/timer/etraxfs_timer.c|  2 +-
> >  hw/timer/m48t59.c   |  4 ++--
> >  hw/timer/milkymist-sysctl.c |  4 ++--
> >  hw/timer/pxa2xx_timer.c |  2 +-
> >  hw/watchdog/watchdog.c  |  2 +-
> >  hw/xenpv/xen_domainbuild.c  |  2 +-
> >  hw/xtensa/xtfpga.c  |  2 +-
> >  kvm-all.c   |  6 +++---
> >  os-win32.c  |  2 +-
> >  qmp.c   |  4 ++--
> >  replay/replay.c |  5 -
> >  target/alpha/sys_helper.c   |  4 ++--
> >  target/arm/psci.c   |  4 ++--
> >  target/i386/excp_helper.c   |  2 +-
> >  target/i386/hax-all.c   |  6 +++---
> >  target/i386/helper.c|  2 +-
> >  target/i386/kvm.c   |  2 +-
> >  target/s390x/helper.c   |  2 +-
> >  target/s390x/kvm.c  |  4 ++--
> >  target/s390x/misc_helper.c  |  4 ++--
> >  target/sparc/int32_helper.c |  2 +-
> >  ui/sdl.c|  2 +-
> >  ui/sdl2.c   |  4 ++--
> >  xen-hvm.c   |  2 +-
> >

Re: [Xen-devel] [Qemu-devel] [PATCH v2] event: Add source information to SHUTDOWN

2017-04-20 Thread Markus Armbruster 
Eric Blake  writes:

> Libvirt would like to be able to distinguish between a SHUTDOWN
> event triggered solely by guest request and one triggered by a
> SIGTERM or other action on the host.  qemu_kill_report() is
> already able to tell whether a shutdown was triggered by a host
> signal (but NOT by a host UI event, such as clicking the X on
> the window), but that information was then lost after being
> printed to stderr.
>
> Enhance the shutdown request path to take a parameter of which
> way it is being triggered, and update ALL callers.  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.
>
> Here is output from 'virsh qemu-monitor-event --loop' with the
> patch installed:
>
> event SHUTDOWN at 1492639680.731251 for domain fedora_13: {"guest":true}
> event STOP at 1492639680.732116 for domain fedora_13: 
> event SHUTDOWN at 1492639680.732830 for domain fedora_13: {"guest":false}
>
> Note that libvirt runs qemu with -no-quit: the first SHUTDOWN event
> was triggered by an action I took directly in the guest (shutdown -h),
> at which point qemu stops the vcpus and waits for libvirt to do any
> final cleanups; the second SHUTDOWN event is the result of libvirt
> sending SIGTERM now that it has completed cleanup.
>
> See also https://bugzilla.redhat.com/1384007
>
> Signed-off-by: Eric Blake 
>
> ---
>
> I did not wire up the RESET event to report guest-triggered, although
> I had to plumb that through all the guests since qemu has options that
> allow remapping reset to shutdown.  It's easy to add that if we want
> it in v3.
>
> The replay driver needs a followup patch if we want to be able to
> faithfully replay the difference between a host- and guest-initiated
> shutdown (for now, the replayed event is always attributed to host).
>
>
> v2: retitle (was "event: Add signal information to SHUTDOWN"),
> completely rework to post bool based on whether it is guest-initiated
> v1: initial submission, exposing just Unix signals from host
> ---
>  qapi/event.json |  8 ++--
>  include/sysemu/sysemu.h |  4 ++--
>  vl.c| 19 +++
>  hw/acpi/core.c  |  4 ++--
>  hw/arm/highbank.c   |  4 ++--
>  hw/arm/integratorcp.c   |  2 +-
>  hw/arm/musicpal.c   |  2 +-
>  hw/arm/omap1.c  |  6 +++---
>  hw/arm/omap2.c  |  2 +-
>  hw/arm/spitz.c  |  2 +-
>  hw/arm/stellaris.c  |  2 +-
>  hw/arm/tosa.c   |  2 +-
>  hw/i386/pc.c|  2 +-
>  hw/input/pckbd.c|  4 ++--
>  hw/ipmi/ipmi.c  |  4 ++--
>  hw/isa/lpc_ich9.c   |  2 +-
>  hw/mips/boston.c|  2 +-
>  hw/mips/mips_malta.c|  2 +-
>  hw/mips/mips_r4k.c  |  4 ++--
>  hw/misc/arm_sysctl.c|  8 
>  hw/misc/cbus.c  |  2 +-
>  hw/misc/macio/cuda.c|  4 ++--
>  hw/misc/slavio_misc.c   |  4 ++--
>  hw/misc/zynq_slcr.c |  2 +-
>  hw/pci-host/apb.c   |  4 ++--
>  hw/pci-host/bonito.c|  2 +-
>  hw/pci-host/piix.c  |  2 +-
>  hw/ppc/e500.c   |  2 +-
>  hw/ppc/mpc8544_guts.c   |  2 +-
>  hw/ppc/ppc.c|  2 +-
>  hw/ppc/ppc405_uc.c  |  2 +-
>  hw/ppc/spapr_hcall.c|  2 +-
>  hw/ppc/spapr_rtas.c |  4 ++--
>  hw/s390x/ipl.c  |  2 +-
>  hw/sh4/r2d.c|  2 +-
>  hw/timer/etraxfs_timer.c|  2 +-
>  hw/timer/m48t59.c   |  4 ++--
>  hw/timer/milkymist-sysctl.c |  4 ++--
>  hw/timer/pxa2xx_timer.c |  2 +-
>  hw/watchdog/watchdog.c  |  2 +-
>  hw/xenpv/xen_domainbuild.c  |  2 +-
>  hw/xtensa/xtfpga.c  |  2 +-
>  kvm-all.c   |  6 +++---
>  os-win32.c  |  2 +-
>  qmp.c   |  4 ++--
>  replay/replay.c |  5 -
>  target/alpha/sys_helper.c   |  4 ++--
>  target/arm/psci.c   |  4 ++--
>  target/i386/excp_helper.c   |  2 +-
>  target/i386/hax-all.c   |  6 +++---
>  target/i386/helper.c|  2 +-
>  target/i386/kvm.c   |  2 +-
>  target/s390x/helper.c   |  2 +-
>  target/s390x/kvm.c  |  4 ++--
>  target/s390x/misc_helper.c  |  4 ++--
>  target/sparc/int32_helper.c |  2 +-
>  ui/sdl.c|  2 +-
>  ui/sdl2.c   |  4 ++--
>  xen-hvm.c   |  2 +-
>  trace-events|  2 +-
>  ui/cocoa.m  |  2 +-
>  61 files changed, 106 insertions(+), 96 deletions(-)
>
> diff --git a/qapi/event.json b/qapi/event.json
> index e80f3f4..c230265 100644
> --- a/qapi/event.json
> +++ b/qapi/event.json
> @@ -10,6 +10,10 @@
>  # Emitted when

Re: [Xen-devel] [Qemu-devel] [PATCH v2] event: Add source information to SHUTDOWN

2017-04-19 Thread Eric Blake 
On 04/19/2017 05:36 PM, Alistair Francis wrote:
> On Wed, Apr 19, 2017 at 3:22 PM, Eric Blake  wrote:
>> Libvirt would like to be able to distinguish between a SHUTDOWN
>> event triggered solely by guest request and one triggered by a
>> SIGTERM or other action on the host.  qemu_kill_report() is
>> already able to tell whether a shutdown was triggered by a host
>> signal (but NOT by a host UI event, such as clicking the X on
>> the window), but that information was then lost after being
>> printed to stderr.
>>
>> Enhance the shutdown request path to take a parameter of which
>> way it is being triggered, and update ALL callers.  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.

> With all this churn is it not worth chaning the bool from_guest to an
> int that we can expand in future?
> 
> I'm imagining an enum with multiple values for different shutdown
> events. At the moment it will just be one for guest and one for host,
> but at some point in the future we might want more.

Yes, that makes sense.  Probably easiest is creating a QMP enum now ( as
in { 'enum': 'ShutdownCause', 'data': ['guest', 'host'] } - although
such an enum defaults to starting at 0, and the shutdown_requested
variable in vl.c would need a tweak to pick a different value when no
shutdown is requested.

> Not only does this future proof us, but I think it makes it more clear
> what you are passing the function instead of just true/false.

Under your proposal, it would be
qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST), which does look a
bit nicer. I'll wait for other review comments, but you have given me a
good reason to do a v3.

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



signature.asc
Description: OpenPGP digital signature
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [Qemu-devel] [PATCH v2] event: Add source information to SHUTDOWN

2017-04-19 Thread Alistair Francis 
On Wed, Apr 19, 2017 at 3:22 PM, Eric Blake  wrote:
> Libvirt would like to be able to distinguish between a SHUTDOWN
> event triggered solely by guest request and one triggered by a
> SIGTERM or other action on the host.  qemu_kill_report() is
> already able to tell whether a shutdown was triggered by a host
> signal (but NOT by a host UI event, such as clicking the X on
> the window), but that information was then lost after being
> printed to stderr.
>
> Enhance the shutdown request path to take a parameter of which
> way it is being triggered, and update ALL callers.  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.
>
> Here is output from 'virsh qemu-monitor-event --loop' with the
> patch installed:
>
> event SHUTDOWN at 1492639680.731251 for domain fedora_13: {"guest":true}
> event STOP at 1492639680.732116 for domain fedora_13: 
> event SHUTDOWN at 1492639680.732830 for domain fedora_13: {"guest":false}
>
> Note that libvirt runs qemu with -no-quit: the first SHUTDOWN event
> was triggered by an action I took directly in the guest (shutdown -h),
> at which point qemu stops the vcpus and waits for libvirt to do any
> final cleanups; the second SHUTDOWN event is the result of libvirt
> sending SIGTERM now that it has completed cleanup.
>
> See also https://bugzilla.redhat.com/1384007
>
> Signed-off-by: Eric Blake 
>
> ---
>
> I did not wire up the RESET event to report guest-triggered, although
> I had to plumb that through all the guests since qemu has options that
> allow remapping reset to shutdown.  It's easy to add that if we want
> it in v3.
>
> The replay driver needs a followup patch if we want to be able to
> faithfully replay the difference between a host- and guest-initiated
> shutdown (for now, the replayed event is always attributed to host).
>
>
> v2: retitle (was "event: Add signal information to SHUTDOWN"),
> completely rework to post bool based on whether it is guest-initiated
> v1: initial submission, exposing just Unix signals from host

With all this churn is it not worth chaning the bool from_guest to an
int that we can expand in future?

I'm imagining an enum with multiple values for different shutdown
events. At the moment it will just be one for guest and one for host,
but at some point in the future we might want more.

Not only does this future proof us, but I think it makes it more clear
what you are passing the function instead of just true/false.

Thanks,

Alistair

> ---
>  qapi/event.json |  8 ++--
>  include/sysemu/sysemu.h |  4 ++--
>  vl.c| 19 +++
>  hw/acpi/core.c  |  4 ++--
>  hw/arm/highbank.c   |  4 ++--
>  hw/arm/integratorcp.c   |  2 +-
>  hw/arm/musicpal.c   |  2 +-
>  hw/arm/omap1.c  |  6 +++---
>  hw/arm/omap2.c  |  2 +-
>  hw/arm/spitz.c  |  2 +-
>  hw/arm/stellaris.c  |  2 +-
>  hw/arm/tosa.c   |  2 +-
>  hw/i386/pc.c|  2 +-
>  hw/input/pckbd.c|  4 ++--
>  hw/ipmi/ipmi.c  |  4 ++--
>  hw/isa/lpc_ich9.c   |  2 +-
>  hw/mips/boston.c|  2 +-
>  hw/mips/mips_malta.c|  2 +-
>  hw/mips/mips_r4k.c  |  4 ++--
>  hw/misc/arm_sysctl.c|  8 
>  hw/misc/cbus.c  |  2 +-
>  hw/misc/macio/cuda.c|  4 ++--
>  hw/misc/slavio_misc.c   |  4 ++--
>  hw/misc/zynq_slcr.c |  2 +-
>  hw/pci-host/apb.c   |  4 ++--
>  hw/pci-host/bonito.c|  2 +-
>  hw/pci-host/piix.c  |  2 +-
>  hw/ppc/e500.c   |  2 +-
>  hw/ppc/mpc8544_guts.c   |  2 +-
>  hw/ppc/ppc.c|  2 +-
>  hw/ppc/ppc405_uc.c  |  2 +-
>  hw/ppc/spapr_hcall.c|  2 +-
>  hw/ppc/spapr_rtas.c |  4 ++--
>  hw/s390x/ipl.c  |  2 +-
>  hw/sh4/r2d.c|  2 +-
>  hw/timer/etraxfs_timer.c|  2 +-
>  hw/timer/m48t59.c   |  4 ++--
>  hw/timer/milkymist-sysctl.c |  4 ++--
>  hw/timer/pxa2xx_timer.c |  2 +-
>  hw/watchdog/watchdog.c  |  2 +-
>  hw/xenpv/xen_domainbuild.c  |  2 +-
>  hw/xtensa/xtfpga.c  |  2 +-
>  kvm-all.c   |  6 +++---
>  os-win32.c  |  2 +-
>  qmp.c   |  4 ++--
>  replay/replay.c |  5 -
>  target/alpha/sys_helper.c   |  4 ++--
>  target/arm/psci.c   |  4 ++--
>  target/i386/excp_helper.c   |  2 +-
>  target/i386/hax-all.c   |  6 +++---
>  target/i386/helper.c|  2 +-
>  target/i386/kvm.c   |  2 +-
>  target/s390x/helper.c   |  2 +-
>  target/s390x/kvm.c  |  4 ++--
>