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

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

[Thomas Weißschuh]

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@weissschuh.net>
---
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@t-8ch.de/
---
 drivers/misc/pvpanic/pvpanic.c | 19 +++++++++++++++++--
 include/uapi/misc/pvpanic.h    |  1 +
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/misc/pvpanic/pvpanic.c b/drivers/misc/pvpanic/pvpanic.c
index 305b367e0ce3..d7d807f5e47a 100644
--- a/drivers/misc/pvpanic/pvpanic.c
+++ b/drivers/misc/pvpanic/pvpanic.c
@@ -15,6 +15,7 @@
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/panic_notifier.h>
+#include <linux/reboot.h>
 #include <linux/types.h>
 #include <linux/cdev.h>
 #include <linux/list.h>
@@ -74,6 +75,13 @@ static struct notifier_block pvpanic_panic_nb = {
 	.priority = INT_MAX,
 };
 
+static int pvpanic_sys_off(struct sys_off_data *data)
+{
+	pvpanic_send_event(PVPANIC_SHUTDOWN);
+
+	return NOTIFY_DONE;
+}
+
 static void pvpanic_remove(void *param)
 {
 	struct pvpanic_instance *pi_cur, *pi_next;
@@ -152,7 +160,7 @@ int devm_pvpanic_probe(struct device *dev, void __iomem *base)
 		return -ENOMEM;
 
 	pi->base = base;
-	pi->capability = PVPANIC_PANICKED | PVPANIC_CRASH_LOADED;
+	pi->capability = PVPANIC_PANICKED | PVPANIC_CRASH_LOADED | PVPANIC_SHUTDOWN;
 
 	/* initlize capability by RDPT */
 	pi->capability &= ioread8(base);
@@ -168,12 +176,18 @@ int devm_pvpanic_probe(struct device *dev, void __iomem *base)
 }
 EXPORT_SYMBOL_GPL(devm_pvpanic_probe);
 
+static struct sys_off_handler *sys_off_handler;
+
 static int pvpanic_init(void)
 {
 	INIT_LIST_HEAD(&pvpanic_list);
 	spin_lock_init(&pvpanic_lock);
 
 	atomic_notifier_chain_register(&panic_notifier_list, &pvpanic_panic_nb);
+	sys_off_handler = register_sys_off_handler(SYS_OFF_MODE_POWER_OFF, SYS_OFF_PRIO_DEFAULT,
+						   pvpanic_sys_off, NULL);
+	if (IS_ERR(sys_off_handler))
+		sys_off_handler = NULL;
 
 	return 0;
 }
@@ -182,6 +196,7 @@ module_init(pvpanic_init);
 static void pvpanic_exit(void)
 {
 	atomic_notifier_chain_unregister(&panic_notifier_list, &pvpanic_panic_nb);
-
+	if (sys_off_handler)
+		unregister_sys_off_handler(sys_off_handler);
 }
 module_exit(pvpanic_exit);
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)
 
 #endif /* __PVPANIC_H__ */

---
base-commit: 90b0c2b2edd1adff742c621e246562fbefa11b70
change-id: 20231104-pvpanic-shutdown-5fa38a879ad1

Best regards,
-- 
Thomas Weißschuh <linux@weissschuh.net>

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

[Greg Kroah-Hartman]

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@weissschuh.net>
> ---
> 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@t-8ch.de/
> ---
>  drivers/misc/pvpanic/pvpanic.c | 19 +++++++++++++++++--
>  include/uapi/misc/pvpanic.h    |  1 +
>  2 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/misc/pvpanic/pvpanic.c b/drivers/misc/pvpanic/pvpanic.c
> index 305b367e0ce3..d7d807f5e47a 100644
> --- a/drivers/misc/pvpanic/pvpanic.c
> +++ b/drivers/misc/pvpanic/pvpanic.c
> @@ -15,6 +15,7 @@
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  #include <linux/panic_notifier.h>
> +#include <linux/reboot.h>
>  #include <linux/types.h>
>  #include <linux/cdev.h>
>  #include <linux/list.h>
> @@ -74,6 +75,13 @@ static struct notifier_block pvpanic_panic_nb = {
>  	.priority = INT_MAX,
>  };
>  
> +static int pvpanic_sys_off(struct sys_off_data *data)
> +{
> +	pvpanic_send_event(PVPANIC_SHUTDOWN);
> +
> +	return NOTIFY_DONE;
> +}
> +
>  static void pvpanic_remove(void *param)
>  {
>  	struct pvpanic_instance *pi_cur, *pi_next;
> @@ -152,7 +160,7 @@ int devm_pvpanic_probe(struct device *dev, void __iomem *base)
>  		return -ENOMEM;
>  
>  	pi->base = base;
> -	pi->capability = PVPANIC_PANICKED | PVPANIC_CRASH_LOADED;
> +	pi->capability = PVPANIC_PANICKED | PVPANIC_CRASH_LOADED | PVPANIC_SHUTDOWN;
>  
>  	/* initlize capability by RDPT */
>  	pi->capability &= ioread8(base);
> @@ -168,12 +176,18 @@ int devm_pvpanic_probe(struct device *dev, void __iomem *base)
>  }
>  EXPORT_SYMBOL_GPL(devm_pvpanic_probe);
>  
> +static struct sys_off_handler *sys_off_handler;
> +
>  static int pvpanic_init(void)
>  {
>  	INIT_LIST_HEAD(&pvpanic_list);
>  	spin_lock_init(&pvpanic_lock);
>  
>  	atomic_notifier_chain_register(&panic_notifier_list, &pvpanic_panic_nb);
> +	sys_off_handler = register_sys_off_handler(SYS_OFF_MODE_POWER_OFF, SYS_OFF_PRIO_DEFAULT,
> +						   pvpanic_sys_off, NULL);
> +	if (IS_ERR(sys_off_handler))
> +		sys_off_handler = NULL;

Why are you allowing this to succeed?  Shouldn't you be returning the
error instead?

>  	return 0;
>  }
> @@ -182,6 +196,7 @@ module_init(pvpanic_init);
>  static void pvpanic_exit(void)
>  {
>  	atomic_notifier_chain_unregister(&panic_notifier_list, &pvpanic_panic_nb);
> -
> +	if (sys_off_handler)
> +		unregister_sys_off_handler(sys_off_handler);
>  }
>  module_exit(pvpanic_exit);
> 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?

And if they need to be here, why not use the proper BIT() macro for it?

thanks,

greg k-h

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

[Thomas Weißschuh]

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@weissschuh.net>
> > ---
> > 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@t-8ch.de/
> > ---
> >  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.
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.
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.


Thomas

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

[Greg Kroah-Hartman]

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@weissschuh.net>
> > > ---
> > > 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@t-8ch.de/
> > > ---
> > >  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.

> 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?

> 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 :)

thanks,

greg k-h

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@weissschuh.net>
> > > > ---
> > > > 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@t-8ch.de/
> > > > ---
> > > >  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

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

[Willy Tarreau]

On Sat, Nov 04, 2023 at 02:53:37PM +0100, Thomas Weißschuh wrote:
> > > 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.

Be careful here, we don't want to expose this kernel macro to userland,
it would break programs that define their own (possibly different) BIT
macro. BIT() is used in kernel headers but we should not presume that
it is available from userland.

Willy

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

[Greg Kroah-Hartman]

On Sat, Nov 04, 2023 at 02:56:34PM +0100, Willy Tarreau wrote:
> On Sat, Nov 04, 2023 at 02:53:37PM +0100, Thomas Weißschuh wrote:
> > > > 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.
> 
> Be careful here, we don't want to expose this kernel macro to userland,
> it would break programs that define their own (possibly different) BIT
> macro. BIT() is used in kernel headers but we should not presume that
> it is available from userland.

It's already there :(

I thought we had a uapi-safe version somewhere, but I can't seem to find
it anymore, so I don't remember what it is called.

thanks,

greg k-h

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

[Thomas Weißschuh]

On 2023-11-04 18:07:21+0100, Greg Kroah-Hartman wrote:
> On Sat, Nov 04, 2023 at 02:56:34PM +0100, Willy Tarreau wrote:
> > On Sat, Nov 04, 2023 at 02:53:37PM +0100, Thomas Weißschuh wrote:
> > > > > 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.
> > 
> > Be careful here, we don't want to expose this kernel macro to userland,
> > it would break programs that define their own (possibly different) BIT
> > macro. BIT() is used in kernel headers but we should not presume that
> > it is available from userland.
> 
> It's already there :(
> 
> I thought we had a uapi-safe version somewhere, but I can't seem to find
> it anymore, so I don't remember what it is called.

It seems to be _BITUL() and _BITULL() from include/uapi/linux/const.h.

But first we'd need to figure out if we he can drop the pvpanic.h uapi
header. I hoped you could give a definitive answer for that.
Personally I'd hate to break stuff for qemu.


Thomas

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

[Willy Tarreau]

On Sat, Nov 04, 2023 at 06:32:57PM +0100, Thomas Weißschuh wrote:
> On 2023-11-04 18:07:21+0100, Greg Kroah-Hartman wrote:
> > On Sat, Nov 04, 2023 at 02:56:34PM +0100, Willy Tarreau wrote:
> > > On Sat, Nov 04, 2023 at 02:53:37PM +0100, Thomas Weißschuh wrote:
> > > > > > 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.
> > > 
> > > Be careful here, we don't want to expose this kernel macro to userland,
> > > it would break programs that define their own (possibly different) BIT
> > > macro. BIT() is used in kernel headers but we should not presume that
> > > it is available from userland.
> > 
> > It's already there :(
> > 
> > I thought we had a uapi-safe version somewhere, but I can't seem to find
> > it anymore, so I don't remember what it is called.
> 
> It seems to be _BITUL() and _BITULL() from include/uapi/linux/const.h.
> 
> But first we'd need to figure out if we he can drop the pvpanic.h uapi
> header. I hoped you could give a definitive answer for that.
> Personally I'd hate to break stuff for qemu.

Agreed, and I tend as well to be careful not to change uapi stuff in
ways that can break, as it can take time to flow to applications and
cause subtle breakage much later :-/

Willy

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

[Michael S. Tsirkin]

On Sat, Nov 04, 2023 at 02:05:02PM +0100, Greg Kroah-Hartman wrote:
> > 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?
> 
> And if they need to be here, why not use the proper BIT() macro for it?
> 
> thanks,
> 
> greg k-h

This is interface with hypervisor not userspace, but for PV historically
we do this since the compatibility implications are about the same,
hypervisors (e.g. QEMU) are mostly userspace and so it is convenient for
them to reuse the same machinery to export the headers.

Let's stick to that, cleaner than duplicating everything I think.

-- 
MST

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

[Thomas Weißschuh]

Hi Greg,

On 2024-02-13 05:41:48-0500, Michael S. Tsirkin wrote:
> On Sat, Nov 04, 2023 at 02:05:02PM +0100, Greg Kroah-Hartman wrote:
> > > 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?
> > 
> > And if they need to be here, why not use the proper BIT() macro for it?
> > 
> > thanks,
> > 
> > greg k-h
> 
> This is interface with hypervisor not userspace, but for PV historically
> we do this since the compatibility implications are about the same,
> hypervisors (e.g. QEMU) are mostly userspace and so it is convenient for
> them to reuse the same machinery to export the headers.
> 
> Let's stick to that, cleaner than duplicating everything I think.

could you confirm that it's fine to keep this UAPI header file?

Thomas

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

[Thomas Weißschuh]

Hi Michael,

On 2024-02-13 05:41:48-0500, Michael S. Tsirkin wrote:
> On Sat, Nov 04, 2023 at 02:05:02PM +0100, Greg Kroah-Hartman wrote:
> > > 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?
> > 
> > And if they need to be here, why not use the proper BIT() macro for it?
> > 
> > thanks,
> > 
> > greg k-h
> 
> This is interface with hypervisor not userspace, but for PV historically
> we do this since the compatibility implications are about the same,
> hypervisors (e.g. QEMU) are mostly userspace and so it is convenient for
> them to reuse the same machinery to export the headers.
> 
> Let's stick to that, cleaner than duplicating everything I think.

as Greg seems to be busy with other stuff I'd like to go ahead with
submitting this again using the existing header file.
It seems unfortunate to block this work on the non-function detail of
the location if a header file which already exists and which we could
always change after the fact, too.

Do you have any recommendations in which parts and order to submit these
changes to the kernel and Qemu?

I need to touch the specification document in qemu, the header in the
kernel, the import of the header in qemu and drivers in both qemu and
the kernel.


Thomas

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

[Arnd Bergmann]

On Wed, Feb 28, 2024, at 07:48, Thomas Weißschuh wrote:
> Hi Michael,
>
> On 2024-02-13 05:41:48-0500, Michael S. Tsirkin wrote:
>> On Sat, Nov 04, 2023 at 02:05:02PM +0100, Greg Kroah-Hartman wrote:
>> > > 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?
>> > 
>> > And if they need to be here, why not use the proper BIT() macro for it?
>> > 
>> 
>> This is interface with hypervisor not userspace, but for PV historically
>> we do this since the compatibility implications are about the same,
>> hypervisors (e.g. QEMU) are mostly userspace and so it is convenient for
>> them to reuse the same machinery to export the headers.
>> 
>> Let's stick to that, cleaner than duplicating everything I think.
>
> as Greg seems to be busy with other stuff I'd like to go ahead with
> submitting this again using the existing header file.

FWIW, I agree using the uapi header for APIs shared between
kernel and qemu is fine, and we don't really have any other
place for those, so please add

Acked-by: Arnd Bergmann <arnd@arndb.de>