Re: [PATCH RFC] misc/pvpanic: add support for normal shutdowns

[Thomas Weißschuh]

On 2023-11-04 14:28:37+0100, Greg Kroah-Hartman wrote:
> On Sat, Nov 04, 2023 at 02:16:53PM +0100, Thomas Weißschuh wrote:
> > On 2023-11-04 14:05:02+0100, Greg Kroah-Hartman wrote:
> > > On Sat, Nov 04, 2023 at 12:29:30PM +0100, 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 <linux@xxxxxxxxxxxxxx>
> > > > ---
> > > > The corresponding patch to qemu has also been submitted[0].
> > > > General discussions about the feature should happen on the other thread.
> > > > 
> > > > [0] https://lore.kernel.org/qemu-devel/20231104-pvpanic-shutdown-v1-0-02353157891b@xxxxxxxx/
> > > > ---
> > > >  drivers/misc/pvpanic/pvpanic.c | 19 +++++++++++++++++--
> > > >  include/uapi/misc/pvpanic.h    |  1 +
> > > >  2 files changed, 18 insertions(+), 2 deletions(-)
> > 
> > [..]
> > 
> > > > diff --git a/include/uapi/misc/pvpanic.h b/include/uapi/misc/pvpanic.h
> > > > index 54b7485390d3..82fc618bfbcf 100644
> > > > --- a/include/uapi/misc/pvpanic.h
> > > > +++ b/include/uapi/misc/pvpanic.h
> > > > @@ -5,5 +5,6 @@
> > > >  
> > > >  #define PVPANIC_PANICKED	(1 << 0)
> > > >  #define PVPANIC_CRASH_LOADED	(1 << 1)
> > > > +#define PVPANIC_SHUTDOWN	(1 << 2)
> > > 
> > > Why are these in a uapi file?
> > 
> > They are ABI between qemu and its guest.
> 
> But there's no interaction between Linux and userspace for these values,
> so I would just drop them from here.

There is one point where they are used:

The pvpanic sysfs files 'events' and 'capability' contain numeric values
which are using these constants.

> 
> > The specification for these values is part of qemu but for some reason
> > the header is part of Linux which is then imported back into qemu.
> > 
> > I guess this has historical reasons, maybe because qemu doesn't really
> > ship ABI headers and for Linux it's natural.
> 
> That feels odd, are there other in-kernel examples of the Linux uapi
> files being abused like this?

Looking at qemu scripts/update-linux-headers.sh at least 
linux/qemu_fw_cfg.h and linux/pci_regs.h seem similar in that they are
not directly related to Linux' own uapi.

(Assuming you want *one* and not *all* examples)

> > The real reason probably doesn't matter today as the header propably
> > can't be dropped from Linux anyways for compatibility reasons.
> > 
> > > And if they need to be here, why not use the proper BIT() macro for it?
> > 
> > This was for uniformity with the existing code.
> > I can send a (standalone?) patch to fix it up.
> 
> If we keep it, sure, that would be nice.  But let's try to drop it if
> possible :)

It will break the mentioned scripts/update-linux-headers.sh from qemu.


Note:

BIT() is part of include/vdso/bits.h which is not part of the
uapi. How is it supposed to work?
Some other uapi header also use BIT() but that seems to work by accident
as the users have the macro defined themselves.


Thomas