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

[Thomas Weißschuh]

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.