Re: Re: [PATCH v4 4/4] hw/misc/pvpanic: add support for normal shutdowns

[Thomas Weißschuh]

Hi Alejandro,

On 2024-01-26 13:47:33-0500, Alejandro Jimenez wrote:
> On 1/7/24 09:05, Thomas Weißschuh 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 | 3 ++-
> >   3 files changed, 9 insertions(+), 1 deletion(-)
> > 
> [snip]
> 
> >   -------------
> > 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);
> 
> I would suggest that instead of directly requesting a system shutdown,
> we should follow the same convention/handling of the other pvpanic
> events and emit a QMP message signaling the specific event that took
> place, to help a management layer application that might be listening
> to determine the cause of the shutdown. It can also be a helpful
> signal to let us know if a guest is (ab)using the new functionality.

This sounds reasonable, thanks for the suggestion and patch.

> If you agree with my reasoning and you'd allow me to piggyback on your
> series, please add my complementary [PATCH 5/4] change that implements
> the suggestion:

I picked up the patch and will test and resend the series in a few days.

[snip]

If one of the maintainers reads this:

Maybe patch 1, 2 and 3 could already be picked up as they seem not to be
controversial.
Then I can also continue to remove the UAPI header on the kernel side.

Thomas