Re: [PATCH v1 4/4] watchdog/pseries-wdt: initial support for PAPR H_WATCHDOG timers
On 6/2/22 00:48, Scott Cheloha wrote:
On Wed, May 25, 2022 at 04:35:11PM +1000, Alexey Kardashevskiy wrote:
On 5/21/22 04:35, Scott Cheloha wrote:
PAPR v2.12 defines a new hypercall, H_WATCHDOG. The hypercall permits
guest control of one or more virtual watchdog timers. The timers have
millisecond granularity. The guest is terminated when a timer
expires.
This patch adds a watchdog driver for these timers, "pseries-wdt".
pseries_wdt_probe() currently assumes the existence of only one
platform device and always assigns it watchdogNumber 1. If we ever
expose more than one timer to userspace we will need to devise a way
to assign a distinct watchdogNumber to each platform device at device
registration time.
This one should go before 4/4 in the series for bisectability.
What is platform_device_register_simple("pseries-wdt",...) going to do
without the driver?
This is a chicken-and-egg problem without an obvious solution. A
device without a driver is a body without a soul. A driver without a
device is a ghost without a machine.
... or something like that, don't quote me :)
Absent some very compelling reasoning, I would like to keep the
current order. It feels logical to me to keep the powerpc/pseries
patches adjacent and prior to the watchdog driver patch.
Signed-off-by: Scott Cheloha
---
.../watchdog/watchdog-parameters.rst | 12 +
drivers/watchdog/Kconfig | 8 +
drivers/watchdog/Makefile | 1 +
drivers/watchdog/pseries-wdt.c| 337 ++
4 files changed, 358 insertions(+)
create mode 100644 drivers/watchdog/pseries-wdt.c
diff --git a/Documentation/watchdog/watchdog-parameters.rst
b/Documentation/watchdog/watchdog-parameters.rst
index 223c99361a30..4ffe725e796c 100644
--- a/Documentation/watchdog/watchdog-parameters.rst
+++ b/Documentation/watchdog/watchdog-parameters.rst
@@ -425,6 +425,18 @@ pnx833x_wdt:
-
+pseries-wdt:
+action:
+ Action taken when watchdog expires: 1 (power off), 2 (restart),
+ 3 (dump and restart). (default=2)
+timeout:
+ Initial watchdog timeout in seconds. (default=60)
+nowayout:
+ Watchdog cannot be stopped once started.
+ (default=kernel config parameter)
+
+-
+
rc32434_wdt:
timeout:
Watchdog timeout value, in seconds (default=20)
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index c4e82a8d863f..06b412603f3e 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1932,6 +1932,14 @@ config MEN_A21_WDT
# PPC64 Architecture
+config PSERIES_WDT
+ tristate "POWER Architecture Platform Watchdog Timer"
+ depends on PPC_PSERIES
+ select WATCHDOG_CORE
+ help
+ Driver for virtual watchdog timers provided by PAPR
+ hypervisors (e.g. PowerVM, KVM).
+
config WATCHDOG_RTAS
tristate "RTAS watchdog"
depends on PPC_RTAS
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index f7da867e8782..f35660409f17 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -184,6 +184,7 @@ obj-$(CONFIG_BOOKE_WDT) += booke_wdt.o
obj-$(CONFIG_MEN_A21_WDT) += mena21_wdt.o
# PPC64 Architecture
+obj-$(CONFIG_PSERIES_WDT) += pseries-wdt.o
obj-$(CONFIG_WATCHDOG_RTAS) += wdrtas.o
# S390 Architecture
diff --git a/drivers/watchdog/pseries-wdt.c b/drivers/watchdog/pseries-wdt.c
new file mode 100644
index ..f41bc4d3b7a2
--- /dev/null
+++ b/drivers/watchdog/pseries-wdt.c
@@ -0,0 +1,337 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2022 International Business Machines, Inc.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define DRV_NAME "pseries-wdt"
+
+/*
+ * The PAPR's MSB->LSB bit ordering is 0->63. These macros simplify
+ * defining bitfields as described in the PAPR without needing to
+ * transpose values to the more C-like 63->0 ordering.
+ */
+#define SETFIELD(_v, _b, _e) \
+ (((unsigned long)(_v) << PPC_BITLSHIFT(_e)) & PPC_BITMASK((_b), (_e)))
+#define GETFIELD(_v, _b, _e) \
+ (((unsigned long)(_v) & PPC_BITMASK((_b), (_e))) >> PPC_BITLSHIFT(_e))
+
+/*
+ * H_WATCHDOG Hypercall Input
+ *
+ * R4: "flags":
+ *
+ * A 64-bit value structured as follows:
+ *
+ * Bits 0-46: Reserved (must be zero).
+ */
+#define PSERIES_WDTF_RESERVED PPC_BITMASK(0, 46)
+
+/*
+ * Bit 47: "leaveOtherWatchdogsRunningOnTimeout"
+ *
+ * 0 Stop outstanding watchdogs on timeout.
+ * 1 Leave outstanding watchdogs running on timeout.
+ */
+#define PSERIES_WDTF_LEAVE_OTHER PPC_BIT(47)
+
+/*
+ * Bits 48-55: "operation"
+ *
+ * 0x01 Start Watchdog
+ * 0x02 Stop Watchdog
+ * 0x03 Query Watchdog Capabilities
+ * 0x04 Query
Re: [PATCH v1 4/4] watchdog/pseries-wdt: initial support for PAPR H_WATCHDOG timers
On Wed, Jun 01, 2022 at 08:45:03AM -0700, Guenter Roeck wrote:
> On 6/1/22 08:07, Scott Cheloha wrote:
> [ ... ]
> > > > > +static unsigned long action = PSERIES_WDTF_ACTION_HARD_RESTART;
> > > > > +
> > > > > +static int action_get(char *buf, const struct kernel_param *kp)
> > > > > +{
> > > > > + int val;
> > > > > +
> > > > > + switch (action) {
> > > > > + case PSERIES_WDTF_ACTION_HARD_POWEROFF:
> > > > > + val = 1;
> > > > > + break;
> > > > > + case PSERIES_WDTF_ACTION_HARD_RESTART:
> > > > > + val = 2;
> > > > > + break;
> > > > > + case PSERIES_WDTF_ACTION_DUMP_RESTART:
> > > > > + val = 3;
> > > > > + break;
> > > > > + default:
> > > > > + return -EINVAL;
> > > > > + }
> > > > > + return sprintf(buf, "%d\n", val);
> > > > > +}
> > > > > +
> > > > > +static int action_set(const char *val, const struct kernel_param *kp)
> > > > > +{
> > > > > + int choice;
> > > > > +
> > > > > + if (kstrtoint(val, 10, ))
> > > > > + return -EINVAL;
> > > > > + switch (choice) {
> > > > > + case 1:
> > > > > + action = PSERIES_WDTF_ACTION_HARD_POWEROFF;
> > > > > + return 0;
> > > > > + case 2:
> > > > > + action = PSERIES_WDTF_ACTION_HARD_RESTART;
> > > > > + return 0;
> > > > > + case 3:
> > > > > + action = PSERIES_WDTF_ACTION_DUMP_RESTART;
> > > > > + return 0;
> > > > > + }
> > > > > + return -EINVAL;
> > > > > +}
> > > > > +
> > > > > +static const struct kernel_param_ops action_ops = {
> > > > > + .get = action_get,
> > > > > + .set = action_set,
> > > > > +};
> > > > > +module_param_cb(action, _ops, NULL, 0444);
> > > >
> > > >
> > > > 0644 here and below?
> > > >
> > >
> > > That would make the module parameters have to be runtime
> > > configurable, which does not make sense at least for
> > > the two parameters below.
> >
> > Agreed.
> >
> > > I don't know though if it is really valuable to have all the
> > > above code instead of just
> > > storing the action numbers and doing the conversion to action
> > > once in the probe function. The above code really only
> > > makes sense if the action is changeable during runtime and more
> > > is done that just converting it to another value.
> >
> > Having a setter that runs exactly once during module attach is
> > obvious. We need a corresponding .get() method to convert on the way
> > out anyway.
> >
>
> Why would a get method be needed if the module parameter is just kept as-is ?
In my original plan they weren't kept as-is. They were converted on
the way in and on the way out.
> > I don't see any upside to moving the action_set() code into
> > pseries_wdt_probe() aside from maybe shaving a few SLOC. The module
> > is already very compact.
>
> I disagree. The get method is unnecessary. The module parameter values (1..3)
> add unnecessary complexity. It could as well be 0..2, making it easier to
> convert.
Yes, we could do that.
> The actual action could be stored in struct pseries_wdt, or converted using
> something
> like
>
> u8 pseries_actions[] = {
> PSERIES_WDTF_ACTION_HARD_POWEROFF,
> PSERIES_WDTF_ACTION_HARD_RESTART,
> PSERIES_WDTF_ACTION_DUMP_RESTART
> };
>
> flags = pseries_actions[action] | PSERIES_WDTF_OP_START;
>
> or, alternatively, in probe
>
> if (action > 2)
> return -EINVAL;
> pw->action = pseries_actions[action];
> and, in the start function,
> flags = pw->action | PSERIES_WDTF_OP_START;
I like this, we'll go with this.
> That not only reduces code size but also improves readability.
> get/set methods are useful, but should be limited to cases where they
> are really needed, ie do something besides converting values. You could argue
> that you want to be able to change the reboot method on the fly, by making
> the module parameter writeable, but then the .set method should actually
> change the method accordingly and not just convert values. And even then
> I'd argue that it would be better not to convert the 'action' value itself
> but to keep it at 0, 1, 2 (or 1, 2, 3 if you prefer) and use param_get_uint
> (or param_get_ulong) for the get method.
Okay, that makes sense.
> Regarding max_timeout, we have calculations such as
>
> unsigned int t = wdd->timeout * 1000;
>
> in the assumption that timeouts larger than UINT_MAX/1000 seconds (or ~50
> days)
> don't really make much sense. watchdog_timeout_invalid() will also return
> -EINVAL
> if the provided timeout value is larger than UINT_MAX / 1000.
Oh, I see. Changed in v2.
Re: [PATCH v1 4/4] watchdog/pseries-wdt: initial support for PAPR H_WATCHDOG timers
On 6/1/22 08:07, Scott Cheloha wrote:
[ ... ]
+static unsigned long action = PSERIES_WDTF_ACTION_HARD_RESTART;
+
+static int action_get(char *buf, const struct kernel_param *kp)
+{
+ int val;
+
+ switch (action) {
+ case PSERIES_WDTF_ACTION_HARD_POWEROFF:
+ val = 1;
+ break;
+ case PSERIES_WDTF_ACTION_HARD_RESTART:
+ val = 2;
+ break;
+ case PSERIES_WDTF_ACTION_DUMP_RESTART:
+ val = 3;
+ break;
+ default:
+ return -EINVAL;
+ }
+ return sprintf(buf, "%d\n", val);
+}
+
+static int action_set(const char *val, const struct kernel_param *kp)
+{
+ int choice;
+
+ if (kstrtoint(val, 10, ))
+ return -EINVAL;
+ switch (choice) {
+ case 1:
+ action = PSERIES_WDTF_ACTION_HARD_POWEROFF;
+ return 0;
+ case 2:
+ action = PSERIES_WDTF_ACTION_HARD_RESTART;
+ return 0;
+ case 3:
+ action = PSERIES_WDTF_ACTION_DUMP_RESTART;
+ return 0;
+ }
+ return -EINVAL;
+}
+
+static const struct kernel_param_ops action_ops = {
+ .get = action_get,
+ .set = action_set,
+};
+module_param_cb(action, _ops, NULL, 0444);
0644 here and below?
That would make the module parameters have to be runtime
configurable, which does not make sense at least for
the two parameters below.
Agreed.
I don't know though if it is really valuable to have all the
above code instead of just
storing the action numbers and doing the conversion to action
once in the probe function. The above code really only
makes sense if the action is changeable during runtime and more
is done that just converting it to another value.
Having a setter that runs exactly once during module attach is
obvious. We need a corresponding .get() method to convert on the way
out anyway.
Why would a get method be needed if the module parameter is just kept as-is ?
I don't see any upside to moving the action_set() code into
pseries_wdt_probe() aside from maybe shaving a few SLOC. The module
is already very compact.
I disagree. The get method is unnecessary. The module parameter values (1..3)
add unnecessary complexity. It could as well be 0..2, making it easier to
convert.
The actual action could be stored in struct pseries_wdt, or converted using
something
like
u8 pseries_actions[] = {
PSERIES_WDTF_ACTION_HARD_POWEROFF,
PSERIES_WDTF_ACTION_HARD_RESTART,
PSERIES_WDTF_ACTION_DUMP_RESTART
};
flags = pseries_actions[action] | PSERIES_WDTF_OP_START;
or, alternatively, in probe
if (action > 2)
return -EINVAL;
pw->action = pseries_actions[action];
and, in the start function,
flags = pw->action | PSERIES_WDTF_OP_START;
That not only reduces code size but also improves readability.
get/set methods are useful, but should be limited to cases where they
are really needed, ie do something besides converting values. You could argue
that you want to be able to change the reboot method on the fly, by making
the module parameter writeable, but then the .set method should actually
change the method accordingly and not just convert values. And even then
I'd argue that it would be better not to convert the 'action' value itself
but to keep it at 0, 1, 2 (or 1, 2, 3 if you prefer) and use param_get_uint
(or param_get_ulong) for the get method.
Regarding max_timeout, we have calculations such as
unsigned int t = wdd->timeout * 1000;
in the assumption that timeouts larger than UINT_MAX/1000 seconds (or ~50 days)
don't really make much sense. watchdog_timeout_invalid() will also return
-EINVAL
if the provided timeout value is larger than UINT_MAX / 1000.
Guenter
Re: [PATCH v1 4/4] watchdog/pseries-wdt: initial support for PAPR H_WATCHDOG timers
On Wed, May 25, 2022 at 12:52:09AM -0700, Guenter Roeck wrote:
> On 5/24/22 23:35, Alexey Kardashevskiy wrote:
> >
> > On 5/21/22 04:35, Scott Cheloha wrote:
> > > PAPR v2.12 defines a new hypercall, H_WATCHDOG. The hypercall permits
> > > guest control of one or more virtual watchdog timers. The timers have
> > > millisecond granularity. The guest is terminated when a timer
> > > expires.
> > >
> > > This patch adds a watchdog driver for these timers, "pseries-wdt".
> > >
> > > pseries_wdt_probe() currently assumes the existence of only one
> > > platform device and always assigns it watchdogNumber 1. If we ever
> > > expose more than one timer to userspace we will need to devise a way
> > > to assign a distinct watchdogNumber to each platform device at device
> > > registration time.
> >
> > This one should go before 4/4 in the series for bisectability.
> >
> > What is platform_device_register_simple("pseries-wdt",...) going to do
> > without the driver?
> >
> > >
> > > Signed-off-by: Scott Cheloha
> > > ---
> > > .../watchdog/watchdog-parameters.rst | 12 +
> > > drivers/watchdog/Kconfig | 8 +
> > > drivers/watchdog/Makefile | 1 +
> > > drivers/watchdog/pseries-wdt.c | 337 ++
> > > 4 files changed, 358 insertions(+)
> > > create mode 100644 drivers/watchdog/pseries-wdt.c
> > >
> > > diff --git a/Documentation/watchdog/watchdog-parameters.rst
> > > b/Documentation/watchdog/watchdog-parameters.rst
> > > index 223c99361a30..4ffe725e796c 100644
> > > --- a/Documentation/watchdog/watchdog-parameters.rst
> > > +++ b/Documentation/watchdog/watchdog-parameters.rst
> > > @@ -425,6 +425,18 @@ pnx833x_wdt:
> > > -
> > > +pseries-wdt:
> > > + action:
> > > + Action taken when watchdog expires: 1 (power off), 2 (restart),
> > > + 3 (dump and restart). (default=2)
> > > + timeout:
> > > + Initial watchdog timeout in seconds. (default=60)
> > > + nowayout:
> > > + Watchdog cannot be stopped once started.
> > > + (default=kernel config parameter)
> > > +
> > > +-
> > > +
> > > rc32434_wdt:
> > > timeout:
> > > Watchdog timeout value, in seconds (default=20)
> > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > > index c4e82a8d863f..06b412603f3e 100644
> > > --- a/drivers/watchdog/Kconfig
> > > +++ b/drivers/watchdog/Kconfig
> > > @@ -1932,6 +1932,14 @@ config MEN_A21_WDT
> > > # PPC64 Architecture
> > > +config PSERIES_WDT
> > > + tristate "POWER Architecture Platform Watchdog Timer"
> > > + depends on PPC_PSERIES
> > > + select WATCHDOG_CORE
> > > + help
> > > + Driver for virtual watchdog timers provided by PAPR
> > > + hypervisors (e.g. PowerVM, KVM).
> > > +
> > > config WATCHDOG_RTAS
> > > tristate "RTAS watchdog"
> > > depends on PPC_RTAS
> > > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> > > index f7da867e8782..f35660409f17 100644
> > > --- a/drivers/watchdog/Makefile
> > > +++ b/drivers/watchdog/Makefile
> > > @@ -184,6 +184,7 @@ obj-$(CONFIG_BOOKE_WDT) += booke_wdt.o
> > > obj-$(CONFIG_MEN_A21_WDT) += mena21_wdt.o
> > > # PPC64 Architecture
> > > +obj-$(CONFIG_PSERIES_WDT) += pseries-wdt.o
> > > obj-$(CONFIG_WATCHDOG_RTAS) += wdrtas.o
> > > # S390 Architecture
> > > diff --git a/drivers/watchdog/pseries-wdt.c
> > > b/drivers/watchdog/pseries-wdt.c
> > > new file mode 100644
> > > index ..f41bc4d3b7a2
> > > --- /dev/null
> > > +++ b/drivers/watchdog/pseries-wdt.c
> > > @@ -0,0 +1,337 @@
> > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > +/*
> > > + * Copyright (c) 2022 International Business Machines, Inc.
> > > + */
> > > +
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +
> > > +#define DRV_NAME "pseries-wdt"
> > > +
> > > +/*
> > > + * The PAPR's MSB->LSB bit ordering is 0->63. These macros simplify
> > > + * defining bitfields as described in the PAPR without needing to
> > > + * transpose values to the more C-like 63->0 ordering.
> > > + */
> > > +#define SETFIELD(_v, _b, _e) \
> > > + (((unsigned long)(_v) << PPC_BITLSHIFT(_e)) & PPC_BITMASK((_b),
> > > (_e)))
> > > +#define GETFIELD(_v, _b, _e) \
> > > + (((unsigned long)(_v) & PPC_BITMASK((_b), (_e))) >>
> > > PPC_BITLSHIFT(_e))
> > > +
> > > +/*
> > > + * H_WATCHDOG Hypercall Input
> > > + *
> > > + * R4: "flags":
> > > + *
> > > + * A 64-bit value structured as follows:
> > > + *
> > > + * Bits 0-46: Reserved (must be zero).
> > > + */
> > > +#define PSERIES_WDTF_RESERVED PPC_BITMASK(0, 46)
> > > +
> > > +/*
> > > + * Bit 47: "leaveOtherWatchdogsRunningOnTimeout"
> > > + *
> > > + * 0 Stop outstanding watchdogs on timeout.
> > > + * 1 Leave
Re: [PATCH v1 4/4] watchdog/pseries-wdt: initial support for PAPR H_WATCHDOG timers
On Wed, May 25, 2022 at 04:35:11PM +1000, Alexey Kardashevskiy wrote:
>
> On 5/21/22 04:35, Scott Cheloha wrote:
> > PAPR v2.12 defines a new hypercall, H_WATCHDOG. The hypercall permits
> > guest control of one or more virtual watchdog timers. The timers have
> > millisecond granularity. The guest is terminated when a timer
> > expires.
> >
> > This patch adds a watchdog driver for these timers, "pseries-wdt".
> >
> > pseries_wdt_probe() currently assumes the existence of only one
> > platform device and always assigns it watchdogNumber 1. If we ever
> > expose more than one timer to userspace we will need to devise a way
> > to assign a distinct watchdogNumber to each platform device at device
> > registration time.
>
> This one should go before 4/4 in the series for bisectability.
>
> What is platform_device_register_simple("pseries-wdt",...) going to do
> without the driver?
This is a chicken-and-egg problem without an obvious solution. A
device without a driver is a body without a soul. A driver without a
device is a ghost without a machine.
... or something like that, don't quote me :)
Absent some very compelling reasoning, I would like to keep the
current order. It feels logical to me to keep the powerpc/pseries
patches adjacent and prior to the watchdog driver patch.
> > Signed-off-by: Scott Cheloha
> > ---
> > .../watchdog/watchdog-parameters.rst | 12 +
> > drivers/watchdog/Kconfig | 8 +
> > drivers/watchdog/Makefile | 1 +
> > drivers/watchdog/pseries-wdt.c| 337 ++
> > 4 files changed, 358 insertions(+)
> > create mode 100644 drivers/watchdog/pseries-wdt.c
> >
> > diff --git a/Documentation/watchdog/watchdog-parameters.rst
> > b/Documentation/watchdog/watchdog-parameters.rst
> > index 223c99361a30..4ffe725e796c 100644
> > --- a/Documentation/watchdog/watchdog-parameters.rst
> > +++ b/Documentation/watchdog/watchdog-parameters.rst
> > @@ -425,6 +425,18 @@ pnx833x_wdt:
> > -
> > +pseries-wdt:
> > +action:
> > + Action taken when watchdog expires: 1 (power off), 2 (restart),
> > + 3 (dump and restart). (default=2)
> > +timeout:
> > + Initial watchdog timeout in seconds. (default=60)
> > +nowayout:
> > + Watchdog cannot be stopped once started.
> > + (default=kernel config parameter)
> > +
> > +-
> > +
> > rc32434_wdt:
> > timeout:
> > Watchdog timeout value, in seconds (default=20)
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > index c4e82a8d863f..06b412603f3e 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -1932,6 +1932,14 @@ config MEN_A21_WDT
> > # PPC64 Architecture
> > +config PSERIES_WDT
> > + tristate "POWER Architecture Platform Watchdog Timer"
> > + depends on PPC_PSERIES
> > + select WATCHDOG_CORE
> > + help
> > + Driver for virtual watchdog timers provided by PAPR
> > + hypervisors (e.g. PowerVM, KVM).
> > +
> > config WATCHDOG_RTAS
> > tristate "RTAS watchdog"
> > depends on PPC_RTAS
> > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> > index f7da867e8782..f35660409f17 100644
> > --- a/drivers/watchdog/Makefile
> > +++ b/drivers/watchdog/Makefile
> > @@ -184,6 +184,7 @@ obj-$(CONFIG_BOOKE_WDT) += booke_wdt.o
> > obj-$(CONFIG_MEN_A21_WDT) += mena21_wdt.o
> > # PPC64 Architecture
> > +obj-$(CONFIG_PSERIES_WDT) += pseries-wdt.o
> > obj-$(CONFIG_WATCHDOG_RTAS) += wdrtas.o
> > # S390 Architecture
> > diff --git a/drivers/watchdog/pseries-wdt.c b/drivers/watchdog/pseries-wdt.c
> > new file mode 100644
> > index ..f41bc4d3b7a2
> > --- /dev/null
> > +++ b/drivers/watchdog/pseries-wdt.c
> > @@ -0,0 +1,337 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (c) 2022 International Business Machines, Inc.
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#define DRV_NAME "pseries-wdt"
> > +
> > +/*
> > + * The PAPR's MSB->LSB bit ordering is 0->63. These macros simplify
> > + * defining bitfields as described in the PAPR without needing to
> > + * transpose values to the more C-like 63->0 ordering.
> > + */
> > +#define SETFIELD(_v, _b, _e) \
> > + (((unsigned long)(_v) << PPC_BITLSHIFT(_e)) & PPC_BITMASK((_b), (_e)))
> > +#define GETFIELD(_v, _b, _e) \
> > + (((unsigned long)(_v) & PPC_BITMASK((_b), (_e))) >> PPC_BITLSHIFT(_e))
> > +
> > +/*
> > + * H_WATCHDOG Hypercall Input
> > + *
> > + * R4: "flags":
> > + *
> > + * A 64-bit value structured as follows:
> > + *
> > + * Bits 0-46: Reserved (must be zero).
> > + */
> > +#define PSERIES_WDTF_RESERVED PPC_BITMASK(0, 46)
> > +
> > +/*
> > + * Bit 47: "leaveOtherWatchdogsRunningOnTimeout"
> > + *
> > + *
Re: [PATCH v1 4/4] watchdog/pseries-wdt: initial support for PAPR H_WATCHDOG timers
On 5/24/22 23:35, Alexey Kardashevskiy wrote:
On 5/21/22 04:35, Scott Cheloha wrote:
PAPR v2.12 defines a new hypercall, H_WATCHDOG. The hypercall permits
guest control of one or more virtual watchdog timers. The timers have
millisecond granularity. The guest is terminated when a timer
expires.
This patch adds a watchdog driver for these timers, "pseries-wdt".
pseries_wdt_probe() currently assumes the existence of only one
platform device and always assigns it watchdogNumber 1. If we ever
expose more than one timer to userspace we will need to devise a way
to assign a distinct watchdogNumber to each platform device at device
registration time.
This one should go before 4/4 in the series for bisectability.
What is platform_device_register_simple("pseries-wdt",...) going to do without
the driver?
Signed-off-by: Scott Cheloha
---
.../watchdog/watchdog-parameters.rst | 12 +
drivers/watchdog/Kconfig | 8 +
drivers/watchdog/Makefile | 1 +
drivers/watchdog/pseries-wdt.c | 337 ++
4 files changed, 358 insertions(+)
create mode 100644 drivers/watchdog/pseries-wdt.c
diff --git a/Documentation/watchdog/watchdog-parameters.rst
b/Documentation/watchdog/watchdog-parameters.rst
index 223c99361a30..4ffe725e796c 100644
--- a/Documentation/watchdog/watchdog-parameters.rst
+++ b/Documentation/watchdog/watchdog-parameters.rst
@@ -425,6 +425,18 @@ pnx833x_wdt:
-
+pseries-wdt:
+ action:
+ Action taken when watchdog expires: 1 (power off), 2 (restart),
+ 3 (dump and restart). (default=2)
+ timeout:
+ Initial watchdog timeout in seconds. (default=60)
+ nowayout:
+ Watchdog cannot be stopped once started.
+ (default=kernel config parameter)
+
+-
+
rc32434_wdt:
timeout:
Watchdog timeout value, in seconds (default=20)
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index c4e82a8d863f..06b412603f3e 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1932,6 +1932,14 @@ config MEN_A21_WDT
# PPC64 Architecture
+config PSERIES_WDT
+ tristate "POWER Architecture Platform Watchdog Timer"
+ depends on PPC_PSERIES
+ select WATCHDOG_CORE
+ help
+ Driver for virtual watchdog timers provided by PAPR
+ hypervisors (e.g. PowerVM, KVM).
+
config WATCHDOG_RTAS
tristate "RTAS watchdog"
depends on PPC_RTAS
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index f7da867e8782..f35660409f17 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -184,6 +184,7 @@ obj-$(CONFIG_BOOKE_WDT) += booke_wdt.o
obj-$(CONFIG_MEN_A21_WDT) += mena21_wdt.o
# PPC64 Architecture
+obj-$(CONFIG_PSERIES_WDT) += pseries-wdt.o
obj-$(CONFIG_WATCHDOG_RTAS) += wdrtas.o
# S390 Architecture
diff --git a/drivers/watchdog/pseries-wdt.c b/drivers/watchdog/pseries-wdt.c
new file mode 100644
index ..f41bc4d3b7a2
--- /dev/null
+++ b/drivers/watchdog/pseries-wdt.c
@@ -0,0 +1,337 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2022 International Business Machines, Inc.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define DRV_NAME "pseries-wdt"
+
+/*
+ * The PAPR's MSB->LSB bit ordering is 0->63. These macros simplify
+ * defining bitfields as described in the PAPR without needing to
+ * transpose values to the more C-like 63->0 ordering.
+ */
+#define SETFIELD(_v, _b, _e) \
+ (((unsigned long)(_v) << PPC_BITLSHIFT(_e)) & PPC_BITMASK((_b), (_e)))
+#define GETFIELD(_v, _b, _e) \
+ (((unsigned long)(_v) & PPC_BITMASK((_b), (_e))) >> PPC_BITLSHIFT(_e))
+
+/*
+ * H_WATCHDOG Hypercall Input
+ *
+ * R4: "flags":
+ *
+ * A 64-bit value structured as follows:
+ *
+ * Bits 0-46: Reserved (must be zero).
+ */
+#define PSERIES_WDTF_RESERVED PPC_BITMASK(0, 46)
+
+/*
+ * Bit 47: "leaveOtherWatchdogsRunningOnTimeout"
+ *
+ * 0 Stop outstanding watchdogs on timeout.
+ * 1 Leave outstanding watchdogs running on timeout.
+ */
+#define PSERIES_WDTF_LEAVE_OTHER PPC_BIT(47)
+
+/*
+ * Bits 48-55: "operation"
+ *
+ * 0x01 Start Watchdog
+ * 0x02 Stop Watchdog
+ * 0x03 Query Watchdog Capabilities
+ * 0x04 Query Watchdog LPM Requirement
+ */
+#define PSERIES_WDTF_OP(op) SETFIELD((op), 48, 55)
+#define PSERIES_WDTF_OP_START PSERIES_WDTF_OP(0x1)
+#define PSERIES_WDTF_OP_STOP PSERIES_WDTF_OP(0x2)
+#define PSERIES_WDTF_OP_QUERY PSERIES_WDTF_OP(0x3)
+#define PSERIES_WDTF_OP_QUERY_LPM PSERIES_WDTF_OP(0x4)
+
+/*
+ * Bits 56-63: "timeoutAction"
+ *
+ * 0x01 Hard poweroff
+ * 0x02 Hard restart
+ * 0x03 Dump restart
+ */
+#define PSERIES_WDTF_ACTION(ac)
Re: [PATCH v1 4/4] watchdog/pseries-wdt: initial support for PAPR H_WATCHDOG timers
On 5/21/22 04:35, Scott Cheloha wrote:
PAPR v2.12 defines a new hypercall, H_WATCHDOG. The hypercall permits
guest control of one or more virtual watchdog timers. The timers have
millisecond granularity. The guest is terminated when a timer
expires.
This patch adds a watchdog driver for these timers, "pseries-wdt".
pseries_wdt_probe() currently assumes the existence of only one
platform device and always assigns it watchdogNumber 1. If we ever
expose more than one timer to userspace we will need to devise a way
to assign a distinct watchdogNumber to each platform device at device
registration time.
This one should go before 4/4 in the series for bisectability.
What is platform_device_register_simple("pseries-wdt",...) going to do
without the driver?
Signed-off-by: Scott Cheloha
---
.../watchdog/watchdog-parameters.rst | 12 +
drivers/watchdog/Kconfig | 8 +
drivers/watchdog/Makefile | 1 +
drivers/watchdog/pseries-wdt.c| 337 ++
4 files changed, 358 insertions(+)
create mode 100644 drivers/watchdog/pseries-wdt.c
diff --git a/Documentation/watchdog/watchdog-parameters.rst
b/Documentation/watchdog/watchdog-parameters.rst
index 223c99361a30..4ffe725e796c 100644
--- a/Documentation/watchdog/watchdog-parameters.rst
+++ b/Documentation/watchdog/watchdog-parameters.rst
@@ -425,6 +425,18 @@ pnx833x_wdt:
-
+pseries-wdt:
+action:
+ Action taken when watchdog expires: 1 (power off), 2 (restart),
+ 3 (dump and restart). (default=2)
+timeout:
+ Initial watchdog timeout in seconds. (default=60)
+nowayout:
+ Watchdog cannot be stopped once started.
+ (default=kernel config parameter)
+
+-
+
rc32434_wdt:
timeout:
Watchdog timeout value, in seconds (default=20)
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index c4e82a8d863f..06b412603f3e 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1932,6 +1932,14 @@ config MEN_A21_WDT
# PPC64 Architecture
+config PSERIES_WDT
+ tristate "POWER Architecture Platform Watchdog Timer"
+ depends on PPC_PSERIES
+ select WATCHDOG_CORE
+ help
+ Driver for virtual watchdog timers provided by PAPR
+ hypervisors (e.g. PowerVM, KVM).
+
config WATCHDOG_RTAS
tristate "RTAS watchdog"
depends on PPC_RTAS
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index f7da867e8782..f35660409f17 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -184,6 +184,7 @@ obj-$(CONFIG_BOOKE_WDT) += booke_wdt.o
obj-$(CONFIG_MEN_A21_WDT) += mena21_wdt.o
# PPC64 Architecture
+obj-$(CONFIG_PSERIES_WDT) += pseries-wdt.o
obj-$(CONFIG_WATCHDOG_RTAS) += wdrtas.o
# S390 Architecture
diff --git a/drivers/watchdog/pseries-wdt.c b/drivers/watchdog/pseries-wdt.c
new file mode 100644
index ..f41bc4d3b7a2
--- /dev/null
+++ b/drivers/watchdog/pseries-wdt.c
@@ -0,0 +1,337 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2022 International Business Machines, Inc.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define DRV_NAME "pseries-wdt"
+
+/*
+ * The PAPR's MSB->LSB bit ordering is 0->63. These macros simplify
+ * defining bitfields as described in the PAPR without needing to
+ * transpose values to the more C-like 63->0 ordering.
+ */
+#define SETFIELD(_v, _b, _e) \
+ (((unsigned long)(_v) << PPC_BITLSHIFT(_e)) & PPC_BITMASK((_b), (_e)))
+#define GETFIELD(_v, _b, _e) \
+ (((unsigned long)(_v) & PPC_BITMASK((_b), (_e))) >> PPC_BITLSHIFT(_e))
+
+/*
+ * H_WATCHDOG Hypercall Input
+ *
+ * R4: "flags":
+ *
+ * A 64-bit value structured as follows:
+ *
+ * Bits 0-46: Reserved (must be zero).
+ */
+#define PSERIES_WDTF_RESERVED PPC_BITMASK(0, 46)
+
+/*
+ * Bit 47: "leaveOtherWatchdogsRunningOnTimeout"
+ *
+ * 0 Stop outstanding watchdogs on timeout.
+ * 1 Leave outstanding watchdogs running on timeout.
+ */
+#define PSERIES_WDTF_LEAVE_OTHER PPC_BIT(47)
+
+/*
+ * Bits 48-55: "operation"
+ *
+ * 0x01 Start Watchdog
+ * 0x02 Stop Watchdog
+ * 0x03 Query Watchdog Capabilities
+ * 0x04 Query Watchdog LPM Requirement
+ */
+#define PSERIES_WDTF_OP(op)SETFIELD((op), 48, 55)
+#define PSERIES_WDTF_OP_START PSERIES_WDTF_OP(0x1)
+#define PSERIES_WDTF_OP_STOP PSERIES_WDTF_OP(0x2)
+#define PSERIES_WDTF_OP_QUERY PSERIES_WDTF_OP(0x3)
+#define PSERIES_WDTF_OP_QUERY_LPM PSERIES_WDTF_OP(0x4)
+
+/*
+ * Bits 56-63: "timeoutAction"
+ *
+ * 0x01 Hard poweroff
+ * 0x02 Hard restart
+ * 0x03 Dump restart
+ */
+#define
[PATCH v1 4/4] watchdog/pseries-wdt: initial support for PAPR H_WATCHDOG timers
PAPR v2.12 defines a new hypercall, H_WATCHDOG. The hypercall permits guest control of one or more virtual watchdog timers. The timers have millisecond granularity. The guest is terminated when a timer expires. This patch adds a watchdog driver for these timers, "pseries-wdt". pseries_wdt_probe() currently assumes the existence of only one platform device and always assigns it watchdogNumber 1. If we ever expose more than one timer to userspace we will need to devise a way to assign a distinct watchdogNumber to each platform device at device registration time. Signed-off-by: Scott Cheloha --- .../watchdog/watchdog-parameters.rst | 12 + drivers/watchdog/Kconfig | 8 + drivers/watchdog/Makefile | 1 + drivers/watchdog/pseries-wdt.c| 337 ++ 4 files changed, 358 insertions(+) create mode 100644 drivers/watchdog/pseries-wdt.c diff --git a/Documentation/watchdog/watchdog-parameters.rst b/Documentation/watchdog/watchdog-parameters.rst index 223c99361a30..4ffe725e796c 100644 --- a/Documentation/watchdog/watchdog-parameters.rst +++ b/Documentation/watchdog/watchdog-parameters.rst @@ -425,6 +425,18 @@ pnx833x_wdt: - +pseries-wdt: +action: + Action taken when watchdog expires: 1 (power off), 2 (restart), + 3 (dump and restart). (default=2) +timeout: + Initial watchdog timeout in seconds. (default=60) +nowayout: + Watchdog cannot be stopped once started. + (default=kernel config parameter) + +- + rc32434_wdt: timeout: Watchdog timeout value, in seconds (default=20) diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index c4e82a8d863f..06b412603f3e 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -1932,6 +1932,14 @@ config MEN_A21_WDT # PPC64 Architecture +config PSERIES_WDT + tristate "POWER Architecture Platform Watchdog Timer" + depends on PPC_PSERIES + select WATCHDOG_CORE + help + Driver for virtual watchdog timers provided by PAPR + hypervisors (e.g. PowerVM, KVM). + config WATCHDOG_RTAS tristate "RTAS watchdog" depends on PPC_RTAS diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index f7da867e8782..f35660409f17 100644 --- a/drivers/watchdog/Makefile +++ b/drivers/watchdog/Makefile @@ -184,6 +184,7 @@ obj-$(CONFIG_BOOKE_WDT) += booke_wdt.o obj-$(CONFIG_MEN_A21_WDT) += mena21_wdt.o # PPC64 Architecture +obj-$(CONFIG_PSERIES_WDT) += pseries-wdt.o obj-$(CONFIG_WATCHDOG_RTAS) += wdrtas.o # S390 Architecture diff --git a/drivers/watchdog/pseries-wdt.c b/drivers/watchdog/pseries-wdt.c new file mode 100644 index ..f41bc4d3b7a2 --- /dev/null +++ b/drivers/watchdog/pseries-wdt.c @@ -0,0 +1,337 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (c) 2022 International Business Machines, Inc. + */ + +#include +#include +#include +#include +#include +#include +#include +#include + +#define DRV_NAME "pseries-wdt" + +/* + * The PAPR's MSB->LSB bit ordering is 0->63. These macros simplify + * defining bitfields as described in the PAPR without needing to + * transpose values to the more C-like 63->0 ordering. + */ +#define SETFIELD(_v, _b, _e) \ + (((unsigned long)(_v) << PPC_BITLSHIFT(_e)) & PPC_BITMASK((_b), (_e))) +#define GETFIELD(_v, _b, _e) \ + (((unsigned long)(_v) & PPC_BITMASK((_b), (_e))) >> PPC_BITLSHIFT(_e)) + +/* + * H_WATCHDOG Hypercall Input + * + * R4: "flags": + * + * A 64-bit value structured as follows: + * + * Bits 0-46: Reserved (must be zero). + */ +#define PSERIES_WDTF_RESERVED PPC_BITMASK(0, 46) + +/* + * Bit 47: "leaveOtherWatchdogsRunningOnTimeout" + * + * 0 Stop outstanding watchdogs on timeout. + * 1 Leave outstanding watchdogs running on timeout. + */ +#define PSERIES_WDTF_LEAVE_OTHER PPC_BIT(47) + +/* + * Bits 48-55: "operation" + * + * 0x01 Start Watchdog + * 0x02 Stop Watchdog + * 0x03 Query Watchdog Capabilities + * 0x04 Query Watchdog LPM Requirement + */ +#define PSERIES_WDTF_OP(op)SETFIELD((op), 48, 55) +#define PSERIES_WDTF_OP_START PSERIES_WDTF_OP(0x1) +#define PSERIES_WDTF_OP_STOP PSERIES_WDTF_OP(0x2) +#define PSERIES_WDTF_OP_QUERY PSERIES_WDTF_OP(0x3) +#define PSERIES_WDTF_OP_QUERY_LPM PSERIES_WDTF_OP(0x4) + +/* + * Bits 56-63: "timeoutAction" + * + * 0x01 Hard poweroff + * 0x02 Hard restart + * 0x03 Dump restart + */ +#define PSERIES_WDTF_ACTION(ac)SETFIELD(ac, 56, 63) +#define PSERIES_WDTF_ACTION_HARD_POWEROFF PSERIES_WDTF_ACTION(0x1) +#define PSERIES_WDTF_ACTION_HARD_RESTART PSERIES_WDTF_ACTION(0x2) +#define
[PATCH v1 4/4] watchdog/pseries-wdt: initial support for PAPR H_WATCHDOG timers
PAPR v2.12 defines a new hypercall, H_WATCHDOG. The hypercall permits guest control of one or more virtual watchdog timers. The timers have millisecond granularity. The guest is terminated when a timer expires. This patch adds a watchdog driver for these timers, "pseries-wdt". pseries_wdt_probe() currently assumes the existence of only one platform device and always assigns it watchdogNumber 1. If we ever expose more than one timer to userspace we will need to devise a way to assign a distinct watchdogNumber to each platform device at device registration time. Signed-off-by: Scott Cheloha --- .../watchdog/watchdog-parameters.rst | 12 + drivers/watchdog/Kconfig | 8 + drivers/watchdog/Makefile | 1 + drivers/watchdog/pseries-wdt.c| 337 ++ 4 files changed, 358 insertions(+) create mode 100644 drivers/watchdog/pseries-wdt.c diff --git a/Documentation/watchdog/watchdog-parameters.rst b/Documentation/watchdog/watchdog-parameters.rst index 223c99361a30..4ffe725e796c 100644 --- a/Documentation/watchdog/watchdog-parameters.rst +++ b/Documentation/watchdog/watchdog-parameters.rst @@ -425,6 +425,18 @@ pnx833x_wdt: - +pseries-wdt: +action: + Action taken when watchdog expires: 1 (power off), 2 (restart), + 3 (dump and restart). (default=2) +timeout: + Initial watchdog timeout in seconds. (default=60) +nowayout: + Watchdog cannot be stopped once started. + (default=kernel config parameter) + +- + rc32434_wdt: timeout: Watchdog timeout value, in seconds (default=20) diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index c4e82a8d863f..06b412603f3e 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -1932,6 +1932,14 @@ config MEN_A21_WDT # PPC64 Architecture +config PSERIES_WDT + tristate "POWER Architecture Platform Watchdog Timer" + depends on PPC_PSERIES + select WATCHDOG_CORE + help + Driver for virtual watchdog timers provided by PAPR + hypervisors (e.g. PowerVM, KVM). + config WATCHDOG_RTAS tristate "RTAS watchdog" depends on PPC_RTAS diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index f7da867e8782..f35660409f17 100644 --- a/drivers/watchdog/Makefile +++ b/drivers/watchdog/Makefile @@ -184,6 +184,7 @@ obj-$(CONFIG_BOOKE_WDT) += booke_wdt.o obj-$(CONFIG_MEN_A21_WDT) += mena21_wdt.o # PPC64 Architecture +obj-$(CONFIG_PSERIES_WDT) += pseries-wdt.o obj-$(CONFIG_WATCHDOG_RTAS) += wdrtas.o # S390 Architecture diff --git a/drivers/watchdog/pseries-wdt.c b/drivers/watchdog/pseries-wdt.c new file mode 100644 index ..f41bc4d3b7a2 --- /dev/null +++ b/drivers/watchdog/pseries-wdt.c @@ -0,0 +1,337 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (c) 2022 International Business Machines, Inc. + */ + +#include +#include +#include +#include +#include +#include +#include +#include + +#define DRV_NAME "pseries-wdt" + +/* + * The PAPR's MSB->LSB bit ordering is 0->63. These macros simplify + * defining bitfields as described in the PAPR without needing to + * transpose values to the more C-like 63->0 ordering. + */ +#define SETFIELD(_v, _b, _e) \ + (((unsigned long)(_v) << PPC_BITLSHIFT(_e)) & PPC_BITMASK((_b), (_e))) +#define GETFIELD(_v, _b, _e) \ + (((unsigned long)(_v) & PPC_BITMASK((_b), (_e))) >> PPC_BITLSHIFT(_e)) + +/* + * H_WATCHDOG Hypercall Input + * + * R4: "flags": + * + * A 64-bit value structured as follows: + * + * Bits 0-46: Reserved (must be zero). + */ +#define PSERIES_WDTF_RESERVED PPC_BITMASK(0, 46) + +/* + * Bit 47: "leaveOtherWatchdogsRunningOnTimeout" + * + * 0 Stop outstanding watchdogs on timeout. + * 1 Leave outstanding watchdogs running on timeout. + */ +#define PSERIES_WDTF_LEAVE_OTHER PPC_BIT(47) + +/* + * Bits 48-55: "operation" + * + * 0x01 Start Watchdog + * 0x02 Stop Watchdog + * 0x03 Query Watchdog Capabilities + * 0x04 Query Watchdog LPM Requirement + */ +#define PSERIES_WDTF_OP(op)SETFIELD((op), 48, 55) +#define PSERIES_WDTF_OP_START PSERIES_WDTF_OP(0x1) +#define PSERIES_WDTF_OP_STOP PSERIES_WDTF_OP(0x2) +#define PSERIES_WDTF_OP_QUERY PSERIES_WDTF_OP(0x3) +#define PSERIES_WDTF_OP_QUERY_LPM PSERIES_WDTF_OP(0x4) + +/* + * Bits 56-63: "timeoutAction" + * + * 0x01 Hard poweroff + * 0x02 Hard restart + * 0x03 Dump restart + */ +#define PSERIES_WDTF_ACTION(ac)SETFIELD(ac, 56, 63) +#define PSERIES_WDTF_ACTION_HARD_POWEROFF PSERIES_WDTF_ACTION(0x1) +#define PSERIES_WDTF_ACTION_HARD_RESTART PSERIES_WDTF_ACTION(0x2) +#define
