1. Patchew
  2. QEMU
  3. [PATCH v2 0/3] hw/misc/pvpanic: add support for normal shutdowns
  4. View patch

[PATCH v2 3/3] hw/misc/pvpanic: add support for normal shutdowns

Thomas Weißschuh posted 3 patches 12 months ago
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, Cornelia Huck <cohuck@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Thomas Huth <thuth@redhat.com>, Laurent Vivier <lvivier@redhat.com>
There is a newer version of this series
[PATCH v2 3/3] hw/misc/pvpanic: add support for normal shutdowns
Posted by Thomas Weißschuh 12 months ago 
Shutdown requests are normally hardware dependent.
By extending pvpanic to also handle shutdown requests, guests can
submit such requests with an easily implementable and cross-platform
mechanism.

Signed-off-by: Thomas Weißschuh <thomas@t-8ch.de>
---
 docs/specs/pvpanic.rst                   | 2 ++
 hw/misc/pvpanic.c                        | 5 +++++
 include/hw/misc/pvpanic.h                | 2 +-
 include/standard-headers/linux/pvpanic.h | 1 +
 4 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/docs/specs/pvpanic.rst b/docs/specs/pvpanic.rst
index f894bc19555f..796cc0348a38 100644
--- a/docs/specs/pvpanic.rst
+++ b/docs/specs/pvpanic.rst
@@ -29,6 +29,8 @@ bit 1
   a guest panic has happened and will be handled by the guest;
   the host should record it or report it, but should not affect
   the execution of the guest.
+bit 2
+  a guest shutdown has happened and should be processed by the host
 
 PCI Interface
 -------------
diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
index a4982cc5928e..246f9ae4e992 100644
--- a/hw/misc/pvpanic.c
+++ b/hw/misc/pvpanic.c
@@ -40,6 +40,11 @@ static void handle_event(int event)
         qemu_system_guest_crashloaded(NULL);
         return;
     }
+
+    if (event & PVPANIC_SHUTDOWN) {
+        qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
+        return;
+    }
 }
 
 /* return supported events on read */
diff --git a/include/hw/misc/pvpanic.h b/include/hw/misc/pvpanic.h
index 198047dc86ff..fa76ad93d998 100644
--- a/include/hw/misc/pvpanic.h
+++ b/include/hw/misc/pvpanic.h
@@ -23,7 +23,7 @@
 #define TYPE_PVPANIC_PCI_DEVICE "pvpanic-pci"
 
 #define PVPANIC_IOPORT_PROP "ioport"
-#define PVPANIC_EVENTS (PVPANIC_PANICKED | PVPANIC_CRASH_LOADED)
+#define PVPANIC_EVENTS (PVPANIC_PANICKED | PVPANIC_CRASH_LOADED | PVPANIC_SHUTDOWN)
 
 /*
  * PVPanicState for any device type
diff --git a/include/standard-headers/linux/pvpanic.h b/include/standard-headers/linux/pvpanic.h
index 54b7485390d3..38e53ad45929 100644
--- a/include/standard-headers/linux/pvpanic.h
+++ b/include/standard-headers/linux/pvpanic.h
@@ -5,5 +5,6 @@
 
 #define PVPANIC_PANICKED	(1 << 0)
 #define PVPANIC_CRASH_LOADED	(1 << 1)
+#define PVPANIC_SHUTDOWN       	(1 << 2)
 
 #endif /* __PVPANIC_H__ */

-- 
2.43.0
Re: [PATCH v2 3/3] hw/misc/pvpanic: add support for normal shutdowns
Posted by Cornelia Huck 12 months ago 
On Tue, Nov 28 2023, Thomas Weißschuh <thomas@t-8ch.de> wrote:

> Shutdown requests are normally hardware dependent.
> By extending pvpanic to also handle shutdown requests, guests can
> submit such requests with an easily implementable and cross-platform
> mechanism.
>
> Signed-off-by: Thomas Weißschuh <thomas@t-8ch.de>
> ---
>  docs/specs/pvpanic.rst                   | 2 ++
>  hw/misc/pvpanic.c                        | 5 +++++
>  include/hw/misc/pvpanic.h                | 2 +-
>  include/standard-headers/linux/pvpanic.h | 1 +
>  4 files changed, 9 insertions(+), 1 deletion(-)

(...)

> diff --git a/include/standard-headers/linux/pvpanic.h b/include/standard-headers/linux/pvpanic.h
> index 54b7485390d3..38e53ad45929 100644
> --- a/include/standard-headers/linux/pvpanic.h
> +++ b/include/standard-headers/linux/pvpanic.h
> @@ -5,5 +5,6 @@
>  
>  #define PVPANIC_PANICKED	(1 << 0)
>  #define PVPANIC_CRASH_LOADED	(1 << 1)
> +#define PVPANIC_SHUTDOWN       	(1 << 2)
>  
>  #endif /* __PVPANIC_H__ */
>

This hunk needs to come in via a separate headers update, or has to be
split out into a placeholder patch if it is not included in the Linux
kernel yet.
Re: [PATCH v2 3/3] hw/misc/pvpanic: add support for normal shutdowns
Posted by Thomas Weißschuh 12 months ago 
On 2023-11-29 09:23:46+0100, Cornelia Huck wrote:
> On Tue, Nov 28 2023, Thomas Weißschuh <thomas@t-8ch.de> wrote:
> 
> > Shutdown requests are normally hardware dependent.
> > By extending pvpanic to also handle shutdown requests, guests can
> > submit such requests with an easily implementable and cross-platform
> > mechanism.
> >
> > Signed-off-by: Thomas Weißschuh <thomas@t-8ch.de>
> > ---
> >  docs/specs/pvpanic.rst                   | 2 ++
> >  hw/misc/pvpanic.c                        | 5 +++++
> >  include/hw/misc/pvpanic.h                | 2 +-
> >  include/standard-headers/linux/pvpanic.h | 1 +
> >  4 files changed, 9 insertions(+), 1 deletion(-)
> 
> (...)
> 
> > diff --git a/include/standard-headers/linux/pvpanic.h b/include/standard-headers/linux/pvpanic.h
> > index 54b7485390d3..38e53ad45929 100644
> > --- a/include/standard-headers/linux/pvpanic.h
> > +++ b/include/standard-headers/linux/pvpanic.h
> > @@ -5,5 +5,6 @@
> >  
> >  #define PVPANIC_PANICKED	(1 << 0)
> >  #define PVPANIC_CRASH_LOADED	(1 << 1)
> > +#define PVPANIC_SHUTDOWN       	(1 << 2)
> >  
> >  #endif /* __PVPANIC_H__ */
> >
> 
> This hunk needs to come in via a separate headers update, or has to be
> split out into a placeholder patch if it is not included in the Linux
> kernel yet.

Greg KH actually want this header removed from the Linux UAPI headers,
as it is not in fact a Linux UAPI [0].
It's also a weird workflow to have the specification in qemu but the
header as part of Linux that is re-imported in qemu.

What do you think about maintaining the header as a private part of qemu
and dropping it from Linux UAPI?

Contrary to my response to Greg this wouldn't break old versions of
qemu, as qemu is using a private copy that would still exist there.

[0] https://lore.kernel.org/lkml/2023110431-pacemaker-pruning-0e4c@gregkh/
Re: [PATCH v2 3/3] hw/misc/pvpanic: add support for normal shutdowns
Posted by Cornelia Huck 12 months ago 
On Wed, Nov 29 2023, Thomas Weißschuh <thomas@t-8ch.de> wrote:
> On 2023-11-29 09:23:46+0100, Cornelia Huck wrote:
>> On Tue, Nov 28 2023, Thomas Weißschuh <thomas@t-8ch.de> wrote:
>> > diff --git a/include/standard-headers/linux/pvpanic.h b/include/standard-headers/linux/pvpanic.h
>> > index 54b7485390d3..38e53ad45929 100644
>> > --- a/include/standard-headers/linux/pvpanic.h
>> > +++ b/include/standard-headers/linux/pvpanic.h
>> > @@ -5,5 +5,6 @@
>> >  
>> >  #define PVPANIC_PANICKED	(1 << 0)
>> >  #define PVPANIC_CRASH_LOADED	(1 << 1)
>> > +#define PVPANIC_SHUTDOWN       	(1 << 2)
>> >  
>> >  #endif /* __PVPANIC_H__ */
>> >
>> 
>> This hunk needs to come in via a separate headers update, or has to be
>> split out into a placeholder patch if it is not included in the Linux
>> kernel yet.
>
> Greg KH actually want this header removed from the Linux UAPI headers,
> as it is not in fact a Linux UAPI [0].
> It's also a weird workflow to have the specification in qemu but the
> header as part of Linux that is re-imported in qemu.
>
> What do you think about maintaining the header as a private part of qemu
> and dropping it from Linux UAPI?
>
> Contrary to my response to Greg this wouldn't break old versions of
> qemu, as qemu is using a private copy that would still exist there.
>
> [0] https://lore.kernel.org/lkml/2023110431-pacemaker-pruning-0e4c@gregkh/

Hm... we have a bunch of examples where we use things exported via the
Linux uapi header files that are not a kernel<->userspace interface, but
rather a host<->guest interface (sometimes defining the interface,
sometimes more as a convenience mechanism). I agree that this is not
quite what the Linux uapi is supposed to be (and yes, it's weird), but
we've being doing that for many years now and changing it would be a
non-zero effort (and we'd have to figure out another way to make sure
the kernel and QEMU do not diverge if there's no authorative third party
around.)

In the case of the pvpanic device, this seems manageable, though; if we
decide to go that way, we should

1. copy the header on the QEMU side somewhere else under include/ and
   remove it from the header update script
2. wait until this hits QEMU mainline (so nobody will try to run the old
   update script)
3. move the uapi file on the Linux side

(We've had changes in the kernel break the update script before, but if
we can do it more smoothly, I'd prefer that way -- the kernel merge
window won't open before the new year anyway, and by that time, we'll
have the QEMU tree open again.)

Main downside is that you'd have extra hassle for something that looks
like a straightforward feature, which is not ideal. (Also, are we sure
that nobody else consumes that header file?)

I'm not sure if dealing with the other host<->guest interfaces that get
copied over is worth the effort, though...

Opinions?
Re: [PATCH v2 3/3] hw/misc/pvpanic: add support for normal shutdowns
Posted by Thomas Weißschuh 12 months ago 
On 2023-11-29 14:15:14+0100, Cornelia Huck wrote:
> On Wed, Nov 29 2023, Thomas Weißschuh <thomas@t-8ch.de> wrote:
> > On 2023-11-29 09:23:46+0100, Cornelia Huck wrote:
> >> On Tue, Nov 28 2023, Thomas Weißschuh <thomas@t-8ch.de> wrote:
> >> > diff --git a/include/standard-headers/linux/pvpanic.h b/include/standard-headers/linux/pvpanic.h
> >> > index 54b7485390d3..38e53ad45929 100644
> >> > --- a/include/standard-headers/linux/pvpanic.h
> >> > +++ b/include/standard-headers/linux/pvpanic.h
> >> > @@ -5,5 +5,6 @@
> >> >  
> >> >  #define PVPANIC_PANICKED	(1 << 0)
> >> >  #define PVPANIC_CRASH_LOADED	(1 << 1)
> >> > +#define PVPANIC_SHUTDOWN       	(1 << 2)
> >> >  
> >> >  #endif /* __PVPANIC_H__ */
> >> >
> >> 
> >> This hunk needs to come in via a separate headers update, or has to be
> >> split out into a placeholder patch if it is not included in the Linux
> >> kernel yet.
> >
> > Greg KH actually want this header removed from the Linux UAPI headers,
> > as it is not in fact a Linux UAPI [0].
> > It's also a weird workflow to have the specification in qemu but the
> > header as part of Linux that is re-imported in qemu.
> >
> > What do you think about maintaining the header as a private part of qemu
> > and dropping it from Linux UAPI?
> >
> > Contrary to my response to Greg this wouldn't break old versions of
> > qemu, as qemu is using a private copy that would still exist there.
> >
> > [0] https://lore.kernel.org/lkml/2023110431-pacemaker-pruning-0e4c@gregkh/
> 
> Hm... we have a bunch of examples where we use things exported via the
> Linux uapi header files that are not a kernel<->userspace interface, but
> rather a host<->guest interface (sometimes defining the interface,
> sometimes more as a convenience mechanism). I agree that this is not
> quite what the Linux uapi is supposed to be (and yes, it's weird), but
> we've being doing that for many years now and changing it would be a
> non-zero effort (and we'd have to figure out another way to make sure
> the kernel and QEMU do not diverge if there's no authorative third party
> around.)
> 
> In the case of the pvpanic device, this seems manageable, though; if we
> decide to go that way, we should
> 
> 1. copy the header on the QEMU side somewhere else under include/ and
>    remove it from the header update script

There is already include/hw/misc/pvpanic.h which seems to be the best
place.

> 2. wait until this hits QEMU mainline (so nobody will try to run the old
>    update script)
> 3. move the uapi file on the Linux side
> 
> (We've had changes in the kernel break the update script before, but if
> we can do it more smoothly, I'd prefer that way -- the kernel merge
> window won't open before the new year anyway, and by that time, we'll
> have the QEMU tree open again.)

The kernel side isn't urgent anyways.

> Main downside is that you'd have extra hassle for something that looks
> like a straightforward feature, which is not ideal. (Also, are we sure
> that nobody else consumes that header file?)

Otherwise I have the hassle on the kernel side :-)

Debian codesearch did not find other users.

> I'm not sure if dealing with the other host<->guest interfaces that get
> copied over is worth the effort, though...
> 
> Opinions?

I'll resend a new revision that drops the import this evening, if
nothing new comes up.

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.