Message ID | 20231104-pvpanic-shutdown-v1-1-5ee7c9b3e301@weissschuh.net |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:8f47:0:b0:403:3b70:6f57 with SMTP id j7csp1593030vqu; Sat, 4 Nov 2023 04:29:54 -0700 (PDT) X-Google-Smtp-Source: AGHT+IEU3/XaU2tMc5G1wx/E/viNYOS1rOguDpO7rLTIPVb3eiHdHE11PXp9hCXnTVtEcIv7n36D X-Received: by 2002:a05:6358:7e42:b0:169:9788:8741 with SMTP id p2-20020a0563587e4200b0016997888741mr13523877rwm.27.1699097394713; Sat, 04 Nov 2023 04:29:54 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1699097394; cv=none; d=google.com; s=arc-20160816; b=N4lePGnNvNbNbS4RlCHMC2FY1Dshi2itUeCJRxDFG/EBQiaUDrbwXZHfvy52urKchs J3H04TdJJ7/8Z4PdtmzKeKw5eP0UjWzGwfZwG5uoF5uHUxQc33xNEmnFLAknSwdtaPHG 4BvTBp8tq2Tuz5ZhrwmtAABzZ8L99YjBIE413Gv1gMn6SKsspP9dy1SCLtiqA3ujreqO ZeD24zMI+3Lv+P4Xmkxvb+W/DBVJQ8Uj/thqOUwXsHJMGehZjWyPKEh7uKDq9hYmmC0p JkvyQmgy5Q/EVwwHykcvor7fdKKVOrx2VrMA1otcCPubwABsS0pJiWAT6G8rixRqGhRo urhA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:message-id:content-transfer-encoding :mime-version:subject:date:from:dkim-signature; bh=t+6zJdGHyECEncy3qb8nDT15x1yVtXmPrh3yujdkLwM=; fh=x+RsuWN3ti9Ce4MFt005HUD9NuYhcahL6VPYaciRZdE=; b=tFAAiDRY0tOUfMyXa9rQ7qGXeY+9AxmGGDvN4a7pYUmUEBnep2A1TCWRVmM3Vm6b3n xN++Sz4Jz4b3Inpd9S8QfgBh1WXenvmjJTOBA2Sezh7rI54enGp5XBWXIRQ9fFMj1Z6f PuuVwGyImX3eR6IcQzYMA5MqAe4HKw8RMfmtwZne3KgrVp83G9bQ0uiS8dwCDxIng0J7 eg1CVQ7lCeisw6O7vcCmJeuLsXu3pTnMoI35cbERmKvhJ0Wqdi1u2/vTq94V2VC1IdYs xf5JgfTipdounEFIhmbShxWnZtf2L8CswZi/rGvz4flgCCUDEHkcrgNE84d8tSg96xJI Bkyw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@weissschuh.net header.s=mail header.b=J22UjRvy; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from fry.vger.email (fry.vger.email. [2620:137:e000::3:8]) by mx.google.com with ESMTPS id m17-20020a656a11000000b00585a02550e1si3823854pgu.50.2023.11.04.04.29.54 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 04 Nov 2023 04:29:54 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8; Authentication-Results: mx.google.com; dkim=pass header.i=@weissschuh.net header.s=mail header.b=J22UjRvy; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by fry.vger.email (Postfix) with ESMTP id 290F8808EDEE; Sat, 4 Nov 2023 04:29:52 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at fry.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232117AbjKDL3o (ORCPT <rfc822;lhua1029@gmail.com> + 35 others); Sat, 4 Nov 2023 07:29:44 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:48084 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231741AbjKDL3n (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Sat, 4 Nov 2023 07:29:43 -0400 Received: from todd.t-8ch.de (todd.t-8ch.de [159.69.126.157]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 831891AA for <linux-kernel@vger.kernel.org>; Sat, 4 Nov 2023 04:29:39 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=weissschuh.net; s=mail; t=1699097377; bh=5CLEWlb/FZJQNq94xbAoxDHDCfPn4Oh7tFwFnTmNzCA=; h=From:Date:Subject:To:Cc:From; b=J22UjRvyVrsMWv1cyfJRtYIix0l/Q07ZPxnVR4jpGsIvaVjRAZ+Eqj9sKpjQrGkw2 rsjJElGmnnc9UozwXJm3bVY8vjiG/UGdUuqQNLCX6tdl0++AXIJM/5bhFWx3lh1S7C yErA/DsDL1HWWRqjvhVbopjJy90ZLIJ0nHZP/BXU= From: =?utf-8?q?Thomas_Wei=C3=9Fschuh?= <linux@weissschuh.net> Date: Sat, 04 Nov 2023 12:29:30 +0100 Subject: [PATCH RFC] misc/pvpanic: add support for normal shutdowns MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit Message-Id: <20231104-pvpanic-shutdown-v1-1-5ee7c9b3e301@weissschuh.net> X-B4-Tracking: v=1; b=H4sIABkrRmUC/6tWKk4tykwtVrJSqFYqSi3LLM7MzwNyDHUUlJIzE vPSU3UzU4B8JSMDI2NDQwMT3YKygsS8zGTd4ozSkpT88jxd07REY4tEC3PLxBRDJaC2gqLUtMw KsJHRSkFuzkqxtbUA2om/xmcAAAA= To: Arnd Bergmann <arnd@arndb.de>, Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: linux-kernel@vger.kernel.org, Zhangjin Wu <falcon@tinylab.org>, Willy Tarreau <w@1wt.eu>, Yuan Tan <tanyuan@tinylab.org>, =?utf-8?q?Thomas_?= =?utf-8?q?Wei=C3=9Fschuh?= <linux@weissschuh.net> X-Mailer: b4 0.12.4 X-Developer-Signature: v=1; a=ed25519-sha256; t=1699097376; l=3120; i=linux@weissschuh.net; s=20221212; h=from:subject:message-id; bh=5CLEWlb/FZJQNq94xbAoxDHDCfPn4Oh7tFwFnTmNzCA=; b=Z9wMVem+r2d/PaEOBigYC8TqCp8MBn4xEDV9ZrtDPTaxeozFZP3MzKlUIPjqvbgeAUMUoNLXM V8XzLOrG74OCLqHvnXYOvBoSggaQYWan0soq1piXeGPD/IF8IiIoNME X-Developer-Key: i=linux@weissschuh.net; a=ed25519; pk=KcycQgFPX2wGR5azS7RhpBqedglOZVgRPfdFSPB1LNw= X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on fry.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (fry.vger.email [0.0.0.0]); Sat, 04 Nov 2023 04:29:52 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1781632749457862318 X-GMAIL-MSGID: 1781632749457862318 |
Series |
[RFC] misc/pvpanic: add support for normal shutdowns
|
|
Commit Message
Thomas Weißschuh
Nov. 4, 2023, 11:29 a.m. UTC
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(-)
---
base-commit: 90b0c2b2edd1adff742c621e246562fbefa11b70
change-id: 20231104-pvpanic-shutdown-5fa38a879ad1
Best regards,
Comments
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
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
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
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
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
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
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
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
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.
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
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
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>
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__ */