[PATCH qemu v2 0/2] ppc/spapr: Implement H_WATCHDOG

[PATCH qemu v2 0/2] ppc/spapr: Implement H_WATCHDOG

[Alexey Kardashevskiy]

This implements H_WATCHDOG. More detailed comments are in the patches.

This is based on sha1
96c343cc774b Joel Stanley "linux-user: Add PowerPC ISA 3.1 and MMA to hwcap".

Please comment. Thanks.



Alexey Kardashevskiy (2):
  ppc: Define SETFIELD for the ppc target
  ppc/spapr: Implement H_WATCHDOG

 include/hw/pci-host/pnv_phb3_regs.h |  16 --
 include/hw/ppc/spapr.h              |  29 +++-
 target/ppc/cpu.h                    |   5 +
 hw/intc/pnv_xive.c                  |  20 ---
 hw/intc/pnv_xive2.c                 |  20 ---
 hw/pci-host/pnv_phb4.c              |  16 --
 hw/ppc/spapr.c                      |   4 +
 hw/watchdog/spapr_watchdog.c        | 248 ++++++++++++++++++++++++++++
 hw/watchdog/meson.build             |   1 +
 hw/watchdog/trace-events            |   7 +
 10 files changed, 293 insertions(+), 73 deletions(-)
 create mode 100644 hw/watchdog/spapr_watchdog.c

-- 
2.30.2

[PATCH qemu v2 1/2] ppc: Define SETFIELD for the ppc target

[Alexey Kardashevskiy]

It keeps repeating, move it to the header. This uses __builtin_ctzl() to
allow using the macros in #define.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 include/hw/pci-host/pnv_phb3_regs.h | 16 ----------------
 target/ppc/cpu.h                    |  5 +++++
 hw/intc/pnv_xive.c                  | 20 --------------------
 hw/intc/pnv_xive2.c                 | 20 --------------------
 hw/pci-host/pnv_phb4.c              | 16 ----------------
 5 files changed, 5 insertions(+), 72 deletions(-)

diff --git a/include/hw/pci-host/pnv_phb3_regs.h b/include/hw/pci-host/pnv_phb3_regs.h
index a174ef1f7045..38f8ce9d7406 100644
--- a/include/hw/pci-host/pnv_phb3_regs.h
+++ b/include/hw/pci-host/pnv_phb3_regs.h
@@ -12,22 +12,6 @@
 
 #include "qemu/host-utils.h"
 
-/*
- * QEMU version of the GETFIELD/SETFIELD macros
- *
- * These are common with the PnvXive model.
- */
-static inline uint64_t GETFIELD(uint64_t mask, uint64_t word)
-{
-    return (word & mask) >> ctz64(mask);
-}
-
-static inline uint64_t SETFIELD(uint64_t mask, uint64_t word,
-                                uint64_t value)
-{
-    return (word & ~mask) | ((value << ctz64(mask)) & mask);
-}
-
 /*
  * PBCQ XSCOM registers
  */
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 6d78078f379d..9a1f1e9999a3 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -47,6 +47,11 @@
                                  PPC_BIT32(bs))
 #define PPC_BITMASK8(bs, be)    ((PPC_BIT8(bs) - PPC_BIT8(be)) | PPC_BIT8(bs))
 
+#define GETFIELD(mask, word)   \
+    (((word) & (mask)) >> __builtin_ctzl(mask))
+#define SETFIELD(mask, word, val)   \
+    (((word) & ~(mask)) | (((uint64_t)(val) << __builtin_ctzl(mask)) & (mask)))
+
 /*****************************************************************************/
 /* Exception vectors definitions                                             */
 enum {
diff --git a/hw/intc/pnv_xive.c b/hw/intc/pnv_xive.c
index 1ce1d7b07d63..c7b75ed12ee0 100644
--- a/hw/intc/pnv_xive.c
+++ b/hw/intc/pnv_xive.c
@@ -66,26 +66,6 @@ static const XiveVstInfo vst_infos[] = {
     qemu_log_mask(LOG_GUEST_ERROR, "XIVE[%x] - " fmt "\n",              \
                   (xive)->chip->chip_id, ## __VA_ARGS__);
 
-/*
- * QEMU version of the GETFIELD/SETFIELD macros
- *
- * TODO: It might be better to use the existing extract64() and
- * deposit64() but this means that all the register definitions will
- * change and become incompatible with the ones found in skiboot.
- *
- * Keep it as it is for now until we find a common ground.
- */
-static inline uint64_t GETFIELD(uint64_t mask, uint64_t word)
-{
-    return (word & mask) >> ctz64(mask);
-}
-
-static inline uint64_t SETFIELD(uint64_t mask, uint64_t word,
-                                uint64_t value)
-{
-    return (word & ~mask) | ((value << ctz64(mask)) & mask);
-}
-
 /*
  * When PC_TCTXT_CHIPID_OVERRIDE is configured, the PC_TCTXT_CHIPID
  * field overrides the hardwired chip ID in the Powerbus operations
diff --git a/hw/intc/pnv_xive2.c b/hw/intc/pnv_xive2.c
index a39e070e82d2..3fe349749384 100644
--- a/hw/intc/pnv_xive2.c
+++ b/hw/intc/pnv_xive2.c
@@ -75,26 +75,6 @@ static const XiveVstInfo vst_infos[] = {
     qemu_log_mask(LOG_GUEST_ERROR, "XIVE[%x] - " fmt "\n",              \
                   (xive)->chip->chip_id, ## __VA_ARGS__);
 
-/*
- * QEMU version of the GETFIELD/SETFIELD macros
- *
- * TODO: It might be better to use the existing extract64() and
- * deposit64() but this means that all the register definitions will
- * change and become incompatible with the ones found in skiboot.
- *
- * Keep it as it is for now until we find a common ground.
- */
-static inline uint64_t GETFIELD(uint64_t mask, uint64_t word)
-{
-    return (word & mask) >> ctz64(mask);
-}
-
-static inline uint64_t SETFIELD(uint64_t mask, uint64_t word,
-                                uint64_t value)
-{
-    return (word & ~mask) | ((value << ctz64(mask)) & mask);
-}
-
 /*
  * TODO: Document block id override
  */
diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
index 13ba9e45d8b6..0913e7c8f015 100644
--- a/hw/pci-host/pnv_phb4.c
+++ b/hw/pci-host/pnv_phb4.c
@@ -31,22 +31,6 @@
     qemu_log_mask(LOG_GUEST_ERROR, "phb4_pec[%d:%d]: " fmt "\n",        \
                   (pec)->chip_id, (pec)->index, ## __VA_ARGS__)
 
-/*
- * QEMU version of the GETFIELD/SETFIELD macros
- *
- * These are common with the PnvXive model.
- */
-static inline uint64_t GETFIELD(uint64_t mask, uint64_t word)
-{
-    return (word & mask) >> ctz64(mask);
-}
-
-static inline uint64_t SETFIELD(uint64_t mask, uint64_t word,
-                                uint64_t value)
-{
-    return (word & ~mask) | ((value << ctz64(mask)) & mask);
-}
-
 static PCIDevice *pnv_phb4_find_cfg_dev(PnvPHB4 *phb)
 {
     PCIHostState *pci = PCI_HOST_BRIDGE(phb);
-- 
2.30.2

Re: [PATCH qemu v2 1/2] ppc: Define SETFIELD for the ppc target

[Daniel Henrique Barboza]

On 6/17/22 03:07, Alexey Kardashevskiy wrote:
> It keeps repeating, move it to the header. This uses __builtin_ctzl() to
> allow using the macros in #define.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>


>   include/hw/pci-host/pnv_phb3_regs.h | 16 ----------------
>   target/ppc/cpu.h                    |  5 +++++
>   hw/intc/pnv_xive.c                  | 20 --------------------
>   hw/intc/pnv_xive2.c                 | 20 --------------------
>   hw/pci-host/pnv_phb4.c              | 16 ----------------
>   5 files changed, 5 insertions(+), 72 deletions(-)
> 
> diff --git a/include/hw/pci-host/pnv_phb3_regs.h b/include/hw/pci-host/pnv_phb3_regs.h
> index a174ef1f7045..38f8ce9d7406 100644
> --- a/include/hw/pci-host/pnv_phb3_regs.h
> +++ b/include/hw/pci-host/pnv_phb3_regs.h
> @@ -12,22 +12,6 @@
>   
>   #include "qemu/host-utils.h"
>   
> -/*
> - * QEMU version of the GETFIELD/SETFIELD macros
> - *
> - * These are common with the PnvXive model.
> - */
> -static inline uint64_t GETFIELD(uint64_t mask, uint64_t word)
> -{
> -    return (word & mask) >> ctz64(mask);
> -}
> -
> -static inline uint64_t SETFIELD(uint64_t mask, uint64_t word,
> -                                uint64_t value)
> -{
> -    return (word & ~mask) | ((value << ctz64(mask)) & mask);
> -}
> -
>   /*
>    * PBCQ XSCOM registers
>    */
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 6d78078f379d..9a1f1e9999a3 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -47,6 +47,11 @@
>                                    PPC_BIT32(bs))
>   #define PPC_BITMASK8(bs, be)    ((PPC_BIT8(bs) - PPC_BIT8(be)) | PPC_BIT8(bs))
>   
> +#define GETFIELD(mask, word)   \
> +    (((word) & (mask)) >> __builtin_ctzl(mask))
> +#define SETFIELD(mask, word, val)   \
> +    (((word) & ~(mask)) | (((uint64_t)(val) << __builtin_ctzl(mask)) & (mask)))
> +
>   /*****************************************************************************/
>   /* Exception vectors definitions                                             */
>   enum {
> diff --git a/hw/intc/pnv_xive.c b/hw/intc/pnv_xive.c
> index 1ce1d7b07d63..c7b75ed12ee0 100644
> --- a/hw/intc/pnv_xive.c
> +++ b/hw/intc/pnv_xive.c
> @@ -66,26 +66,6 @@ static const XiveVstInfo vst_infos[] = {
>       qemu_log_mask(LOG_GUEST_ERROR, "XIVE[%x] - " fmt "\n",              \
>                     (xive)->chip->chip_id, ## __VA_ARGS__);
>   
> -/*
> - * QEMU version of the GETFIELD/SETFIELD macros
> - *
> - * TODO: It might be better to use the existing extract64() and
> - * deposit64() but this means that all the register definitions will
> - * change and become incompatible with the ones found in skiboot.
> - *
> - * Keep it as it is for now until we find a common ground.
> - */
> -static inline uint64_t GETFIELD(uint64_t mask, uint64_t word)
> -{
> -    return (word & mask) >> ctz64(mask);
> -}
> -
> -static inline uint64_t SETFIELD(uint64_t mask, uint64_t word,
> -                                uint64_t value)
> -{
> -    return (word & ~mask) | ((value << ctz64(mask)) & mask);
> -}
> -
>   /*
>    * When PC_TCTXT_CHIPID_OVERRIDE is configured, the PC_TCTXT_CHIPID
>    * field overrides the hardwired chip ID in the Powerbus operations
> diff --git a/hw/intc/pnv_xive2.c b/hw/intc/pnv_xive2.c
> index a39e070e82d2..3fe349749384 100644
> --- a/hw/intc/pnv_xive2.c
> +++ b/hw/intc/pnv_xive2.c
> @@ -75,26 +75,6 @@ static const XiveVstInfo vst_infos[] = {
>       qemu_log_mask(LOG_GUEST_ERROR, "XIVE[%x] - " fmt "\n",              \
>                     (xive)->chip->chip_id, ## __VA_ARGS__);
>   
> -/*
> - * QEMU version of the GETFIELD/SETFIELD macros
> - *
> - * TODO: It might be better to use the existing extract64() and
> - * deposit64() but this means that all the register definitions will
> - * change and become incompatible with the ones found in skiboot.
> - *
> - * Keep it as it is for now until we find a common ground.
> - */
> -static inline uint64_t GETFIELD(uint64_t mask, uint64_t word)
> -{
> -    return (word & mask) >> ctz64(mask);
> -}
> -
> -static inline uint64_t SETFIELD(uint64_t mask, uint64_t word,
> -                                uint64_t value)
> -{
> -    return (word & ~mask) | ((value << ctz64(mask)) & mask);
> -}
> -
>   /*
>    * TODO: Document block id override
>    */
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index 13ba9e45d8b6..0913e7c8f015 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -31,22 +31,6 @@
>       qemu_log_mask(LOG_GUEST_ERROR, "phb4_pec[%d:%d]: " fmt "\n",        \
>                     (pec)->chip_id, (pec)->index, ## __VA_ARGS__)
>   
> -/*
> - * QEMU version of the GETFIELD/SETFIELD macros
> - *
> - * These are common with the PnvXive model.
> - */
> -static inline uint64_t GETFIELD(uint64_t mask, uint64_t word)
> -{
> -    return (word & mask) >> ctz64(mask);
> -}
> -
> -static inline uint64_t SETFIELD(uint64_t mask, uint64_t word,
> -                                uint64_t value)
> -{
> -    return (word & ~mask) | ((value << ctz64(mask)) & mask);
> -}
> -
>   static PCIDevice *pnv_phb4_find_cfg_dev(PnvPHB4 *phb)
>   {
>       PCIHostState *pci = PCI_HOST_BRIDGE(phb);

Re: [PATCH qemu v2 1/2] ppc: Define SETFIELD for the ppc target

[Alexey Kardashevskiy]

On 6/18/22 02:50, Daniel Henrique Barboza wrote:
> 
> 
> On 6/17/22 03:07, Alexey Kardashevskiy wrote:
>> It keeps repeating, move it to the header. This uses __builtin_ctzl() to
>> allow using the macros in #define.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
> 
> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>



soooo I do not need to repost it, right?


-- 
Alexey

Re: [PATCH qemu v2 1/2] ppc: Define SETFIELD for the ppc target

[Cédric Le Goater]

On 6/20/22 05:37, Alexey Kardashevskiy wrote:
> 
> 
> On 6/18/22 02:50, Daniel Henrique Barboza wrote:
>>
>>
>> On 6/17/22 03:07, Alexey Kardashevskiy wrote:
>>> It keeps repeating, move it to the header. This uses __builtin_ctzl() to
>>> allow using the macros in #define.
>>>
>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>> ---
>>
>> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> 
> 
> 
> soooo I do not need to repost it, right?
> 
> 

The watchdog patch depends on the availability of these macros.
Doesn't it ?

C.

Re: [PATCH qemu v2 1/2] ppc: Define SETFIELD for the ppc target

[Alexey Kardashevskiy]

On 6/20/22 16:17, Cédric Le Goater wrote:
> On 6/20/22 05:37, Alexey Kardashevskiy wrote:
>>
>>
>> On 6/18/22 02:50, Daniel Henrique Barboza wrote:
>>>
>>>
>>> On 6/17/22 03:07, Alexey Kardashevskiy wrote:
>>>> It keeps repeating, move it to the header. This uses 
>>>> __builtin_ctzl() to
>>>> allow using the macros in #define.
>>>>
>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>> ---
>>>
>>> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>
>>
>>
>> soooo I do not need to repost it, right?
>>
>>
> 
> The watchdog patch depends on the availability of these macros.
> Doesn't it ?

yes but this one could go in now and I will repost 2/2 a little later on 
top.


-- 
Alexey

Re: [PATCH qemu v2 1/2] ppc: Define SETFIELD for the ppc target

[Daniel Henrique Barboza]

On 6/20/22 05:10, Alexey Kardashevskiy wrote:
> 
> 
> On 6/20/22 16:17, Cédric Le Goater wrote:
>> On 6/20/22 05:37, Alexey Kardashevskiy wrote:
>>>
>>>
>>> On 6/18/22 02:50, Daniel Henrique Barboza wrote:
>>>>
>>>>
>>>> On 6/17/22 03:07, Alexey Kardashevskiy wrote:
>>>>> It keeps repeating, move it to the header. This uses __builtin_ctzl() to
>>>>> allow using the macros in #define.
>>>>>
>>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>>> ---
>>>>
>>>> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>>
>>>
>>>
>>> soooo I do not need to repost it, right?
>>>
>>>
>>
>> The watchdog patch depends on the availability of these macros.
>> Doesn't it ?
> 
> yes but this one could go in now and I will repost 2/2 a little later on top.

As soon as the current ppc PR is merged I can take this patch. If the 2/2 repost
happens earlier than that you can just re-submit it with 1/2 again.


Thanks,


Daniel



> 
> 

Re: [PATCH qemu v2 1/2] ppc: Define SETFIELD for the ppc target

[Cédric Le Goater]

On 6/17/22 08:07, Alexey Kardashevskiy wrote:
> It keeps repeating, move it to the header. This uses __builtin_ctzl() to
> allow using the macros in #define.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks Alexey,

C.


> ---
>   include/hw/pci-host/pnv_phb3_regs.h | 16 ----------------
>   target/ppc/cpu.h                    |  5 +++++
>   hw/intc/pnv_xive.c                  | 20 --------------------
>   hw/intc/pnv_xive2.c                 | 20 --------------------
>   hw/pci-host/pnv_phb4.c              | 16 ----------------
>   5 files changed, 5 insertions(+), 72 deletions(-)
> 
> diff --git a/include/hw/pci-host/pnv_phb3_regs.h b/include/hw/pci-host/pnv_phb3_regs.h
> index a174ef1f7045..38f8ce9d7406 100644
> --- a/include/hw/pci-host/pnv_phb3_regs.h
> +++ b/include/hw/pci-host/pnv_phb3_regs.h
> @@ -12,22 +12,6 @@
>   
>   #include "qemu/host-utils.h"
>   
> -/*
> - * QEMU version of the GETFIELD/SETFIELD macros
> - *
> - * These are common with the PnvXive model.
> - */
> -static inline uint64_t GETFIELD(uint64_t mask, uint64_t word)
> -{
> -    return (word & mask) >> ctz64(mask);
> -}
> -
> -static inline uint64_t SETFIELD(uint64_t mask, uint64_t word,
> -                                uint64_t value)
> -{
> -    return (word & ~mask) | ((value << ctz64(mask)) & mask);
> -}
> -
>   /*
>    * PBCQ XSCOM registers
>    */
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 6d78078f379d..9a1f1e9999a3 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -47,6 +47,11 @@
>                                    PPC_BIT32(bs))
>   #define PPC_BITMASK8(bs, be)    ((PPC_BIT8(bs) - PPC_BIT8(be)) | PPC_BIT8(bs))
>   
> +#define GETFIELD(mask, word)   \
> +    (((word) & (mask)) >> __builtin_ctzl(mask))
> +#define SETFIELD(mask, word, val)   \
> +    (((word) & ~(mask)) | (((uint64_t)(val) << __builtin_ctzl(mask)) & (mask)))
> +
>   /*****************************************************************************/
>   /* Exception vectors definitions                                             */
>   enum {
> diff --git a/hw/intc/pnv_xive.c b/hw/intc/pnv_xive.c
> index 1ce1d7b07d63..c7b75ed12ee0 100644
> --- a/hw/intc/pnv_xive.c
> +++ b/hw/intc/pnv_xive.c
> @@ -66,26 +66,6 @@ static const XiveVstInfo vst_infos[] = {
>       qemu_log_mask(LOG_GUEST_ERROR, "XIVE[%x] - " fmt "\n",              \
>                     (xive)->chip->chip_id, ## __VA_ARGS__);
>   
> -/*
> - * QEMU version of the GETFIELD/SETFIELD macros
> - *
> - * TODO: It might be better to use the existing extract64() and
> - * deposit64() but this means that all the register definitions will
> - * change and become incompatible with the ones found in skiboot.
> - *
> - * Keep it as it is for now until we find a common ground.
> - */
> -static inline uint64_t GETFIELD(uint64_t mask, uint64_t word)
> -{
> -    return (word & mask) >> ctz64(mask);
> -}
> -
> -static inline uint64_t SETFIELD(uint64_t mask, uint64_t word,
> -                                uint64_t value)
> -{
> -    return (word & ~mask) | ((value << ctz64(mask)) & mask);
> -}
> -
>   /*
>    * When PC_TCTXT_CHIPID_OVERRIDE is configured, the PC_TCTXT_CHIPID
>    * field overrides the hardwired chip ID in the Powerbus operations
> diff --git a/hw/intc/pnv_xive2.c b/hw/intc/pnv_xive2.c
> index a39e070e82d2..3fe349749384 100644
> --- a/hw/intc/pnv_xive2.c
> +++ b/hw/intc/pnv_xive2.c
> @@ -75,26 +75,6 @@ static const XiveVstInfo vst_infos[] = {
>       qemu_log_mask(LOG_GUEST_ERROR, "XIVE[%x] - " fmt "\n",              \
>                     (xive)->chip->chip_id, ## __VA_ARGS__);
>   
> -/*
> - * QEMU version of the GETFIELD/SETFIELD macros
> - *
> - * TODO: It might be better to use the existing extract64() and
> - * deposit64() but this means that all the register definitions will
> - * change and become incompatible with the ones found in skiboot.
> - *
> - * Keep it as it is for now until we find a common ground.
> - */
> -static inline uint64_t GETFIELD(uint64_t mask, uint64_t word)
> -{
> -    return (word & mask) >> ctz64(mask);
> -}
> -
> -static inline uint64_t SETFIELD(uint64_t mask, uint64_t word,
> -                                uint64_t value)
> -{
> -    return (word & ~mask) | ((value << ctz64(mask)) & mask);
> -}
> -
>   /*
>    * TODO: Document block id override
>    */
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index 13ba9e45d8b6..0913e7c8f015 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -31,22 +31,6 @@
>       qemu_log_mask(LOG_GUEST_ERROR, "phb4_pec[%d:%d]: " fmt "\n",        \
>                     (pec)->chip_id, (pec)->index, ## __VA_ARGS__)
>   
> -/*
> - * QEMU version of the GETFIELD/SETFIELD macros
> - *
> - * These are common with the PnvXive model.
> - */
> -static inline uint64_t GETFIELD(uint64_t mask, uint64_t word)
> -{
> -    return (word & mask) >> ctz64(mask);
> -}
> -
> -static inline uint64_t SETFIELD(uint64_t mask, uint64_t word,
> -                                uint64_t value)
> -{
> -    return (word & ~mask) | ((value << ctz64(mask)) & mask);
> -}
> -
>   static PCIDevice *pnv_phb4_find_cfg_dev(PnvPHB4 *phb)
>   {
>       PCIHostState *pci = PCI_HOST_BRIDGE(phb);

Re: [PATCH qemu v2 1/2] ppc: Define SETFIELD for the ppc target

[Peter Maydell]

On Fri, 17 Jun 2022 at 07:20, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>
> It keeps repeating, move it to the header. This uses __builtin_ctzl() to
> allow using the macros in #define.
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>  include/hw/pci-host/pnv_phb3_regs.h | 16 ----------------
>  target/ppc/cpu.h                    |  5 +++++
>  hw/intc/pnv_xive.c                  | 20 --------------------
>  hw/intc/pnv_xive2.c                 | 20 --------------------
>  hw/pci-host/pnv_phb4.c              | 16 ----------------
>  5 files changed, 5 insertions(+), 72 deletions(-)
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 6d78078f379d..9a1f1e9999a3 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -47,6 +47,11 @@
>                                   PPC_BIT32(bs))
>  #define PPC_BITMASK8(bs, be)    ((PPC_BIT8(bs) - PPC_BIT8(be)) | PPC_BIT8(bs))
>
> +#define GETFIELD(mask, word)   \
> +    (((word) & (mask)) >> __builtin_ctzl(mask))
> +#define SETFIELD(mask, word, val)   \
> +    (((word) & ~(mask)) | (((uint64_t)(val) << __builtin_ctzl(mask)) & (mask)))

Can we retain the explanatory comment that says why we don't
use the standard QEMU mechanism for field extraction
(ie the FIELD_EX*/FIELD_DP* macros and the extract64()/deposit64()
functions) ?

> -/*
> - * QEMU version of the GETFIELD/SETFIELD macros
> - *
> - * TODO: It might be better to use the existing extract64() and
> - * deposit64() but this means that all the register definitions will
> - * change and become incompatible with the ones found in skiboot.
> - *
> - * Keep it as it is for now until we find a common ground.
> - */

thanks
-- PMM

Re: [PATCH qemu v2 1/2] ppc: Define SETFIELD for the ppc target

[Daniel Henrique Barboza]

Alexey,

The newer version of this patch is having trouble with Gitlab runners, as
you can read in my feedback there.

I've tested this one just in case. The same problems happen. E.g. for the
cross-armel-system runner:


In file included from ../hw/intc/pnv_xive.c:14:
../hw/intc/pnv_xive.c: In function ‘pnv_xive_block_id’:
/builds/danielhb/qemu/target/ppc/cpu.h:45:33: error: conversion from ‘long long unsigned int’ to ‘long unsigned int’ changes value from ‘4222124650659840’ to ‘0’ [-Werror=overflow]
    45 | #define PPC_BITMASK(bs, be)     ((PPC_BIT(bs) - PPC_BIT(be)) | PPC_BIT(bs))
       |                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/builds/danielhb/qemu/target/ppc/cpu.h:51:42: note: in definition of macro ‘GETFIELD’
    51 |     (((word) & (mask)) >> __builtin_ctzl(mask))
       |                                          ^~~~
../hw/intc/pnv_xive_regs.h:77:41: note: in expansion of macro ‘PPC_BITMASK’
    77 | #define  PC_TCTXT_CHIPID                PPC_BITMASK(12, 15)
       |                                         ^~~~~~~~~~~
../hw/intc/pnv_xive.c:80:24: note: in expansion of macro ‘PC_TCTXT_CHIPID’
    80 |         blk = GETFIELD(PC_TCTXT_CHIPID, cfg_val);
       |                        ^~~~~~~~~~~~~~~
../hw/intc/pnv_xive.c: In function ‘pnv_xive_vst_addr’:
/builds/danielhb/qemu/target/ppc/cpu.h:45:33: error: conversion from ‘long long unsigned int’ to ‘long unsigned int’ changes value from ‘13835058055282163712’ to ‘0’ [-Werror=overflow]
    45 | #define PPC_BITMASK(bs, be)     ((PPC_BIT(bs) - PPC_BIT(be)) | PPC_BIT(bs))
       |                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/builds/danielhb/qemu/target/ppc/cpu.h:51:42: note: in definition of macro ‘GETFIELD’
    51 |     (((word) & (mask)) >> __builtin_ctzl(mask))
       |                                          ^~~~
../hw/intc/pnv_xive_regs.h:230:33: note: in expansion of macro ‘PPC_BITMASK’
   230 | #define VSD_MODE                PPC_BITMASK(0, 1)
       |                                 ^~~~~~~~~~~
../hw/intc/pnv_xive.c:226:18: note: in expansion of macro ‘VSD_MODE’
   226 |     if (GETFIELD(VSD_MODE, vsd) == VSD_MODE_FORWARD) {
       |                  ^~~~~~~~
../hw/intc/pnv_xive.c: In function ‘pnv_xive_end_update’:


Link:

https://gitlab.com/danielhb/qemu/-/jobs/2637716673


I don´t know how to deal with that.


For the record: if this is too troublesome to fix, I am ok with just consolidating
the GETFIELD and SETFIELD inlines we already have, under cpu.h, keeping them exactly
as they are today (functions, not macros).


Thanks,


Daniel



On 6/17/22 03:07, Alexey Kardashevskiy wrote:
> It keeps repeating, move it to the header. This uses __builtin_ctzl() to
> allow using the macros in #define.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>   include/hw/pci-host/pnv_phb3_regs.h | 16 ----------------
>   target/ppc/cpu.h                    |  5 +++++
>   hw/intc/pnv_xive.c                  | 20 --------------------
>   hw/intc/pnv_xive2.c                 | 20 --------------------
>   hw/pci-host/pnv_phb4.c              | 16 ----------------
>   5 files changed, 5 insertions(+), 72 deletions(-)
> 
> diff --git a/include/hw/pci-host/pnv_phb3_regs.h b/include/hw/pci-host/pnv_phb3_regs.h
> index a174ef1f7045..38f8ce9d7406 100644
> --- a/include/hw/pci-host/pnv_phb3_regs.h
> +++ b/include/hw/pci-host/pnv_phb3_regs.h
> @@ -12,22 +12,6 @@
>   
>   #include "qemu/host-utils.h"
>   
> -/*
> - * QEMU version of the GETFIELD/SETFIELD macros
> - *
> - * These are common with the PnvXive model.
> - */
> -static inline uint64_t GETFIELD(uint64_t mask, uint64_t word)
> -{
> -    return (word & mask) >> ctz64(mask);
> -}
> -
> -static inline uint64_t SETFIELD(uint64_t mask, uint64_t word,
> -                                uint64_t value)
> -{
> -    return (word & ~mask) | ((value << ctz64(mask)) & mask);
> -}
> -
>   /*
>    * PBCQ XSCOM registers
>    */
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 6d78078f379d..9a1f1e9999a3 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -47,6 +47,11 @@
>                                    PPC_BIT32(bs))
>   #define PPC_BITMASK8(bs, be)    ((PPC_BIT8(bs) - PPC_BIT8(be)) | PPC_BIT8(bs))
>   
> +#define GETFIELD(mask, word)   \
> +    (((word) & (mask)) >> __builtin_ctzl(mask))
> +#define SETFIELD(mask, word, val)   \
> +    (((word) & ~(mask)) | (((uint64_t)(val) << __builtin_ctzl(mask)) & (mask)))
> +
>   /*****************************************************************************/
>   /* Exception vectors definitions                                             */
>   enum {
> diff --git a/hw/intc/pnv_xive.c b/hw/intc/pnv_xive.c
> index 1ce1d7b07d63..c7b75ed12ee0 100644
> --- a/hw/intc/pnv_xive.c
> +++ b/hw/intc/pnv_xive.c
> @@ -66,26 +66,6 @@ static const XiveVstInfo vst_infos[] = {
>       qemu_log_mask(LOG_GUEST_ERROR, "XIVE[%x] - " fmt "\n",              \
>                     (xive)->chip->chip_id, ## __VA_ARGS__);
>   
> -/*
> - * QEMU version of the GETFIELD/SETFIELD macros
> - *
> - * TODO: It might be better to use the existing extract64() and
> - * deposit64() but this means that all the register definitions will
> - * change and become incompatible with the ones found in skiboot.
> - *
> - * Keep it as it is for now until we find a common ground.
> - */
> -static inline uint64_t GETFIELD(uint64_t mask, uint64_t word)
> -{
> -    return (word & mask) >> ctz64(mask);
> -}
> -
> -static inline uint64_t SETFIELD(uint64_t mask, uint64_t word,
> -                                uint64_t value)
> -{
> -    return (word & ~mask) | ((value << ctz64(mask)) & mask);
> -}
> -
>   /*
>    * When PC_TCTXT_CHIPID_OVERRIDE is configured, the PC_TCTXT_CHIPID
>    * field overrides the hardwired chip ID in the Powerbus operations
> diff --git a/hw/intc/pnv_xive2.c b/hw/intc/pnv_xive2.c
> index a39e070e82d2..3fe349749384 100644
> --- a/hw/intc/pnv_xive2.c
> +++ b/hw/intc/pnv_xive2.c
> @@ -75,26 +75,6 @@ static const XiveVstInfo vst_infos[] = {
>       qemu_log_mask(LOG_GUEST_ERROR, "XIVE[%x] - " fmt "\n",              \
>                     (xive)->chip->chip_id, ## __VA_ARGS__);
>   
> -/*
> - * QEMU version of the GETFIELD/SETFIELD macros
> - *
> - * TODO: It might be better to use the existing extract64() and
> - * deposit64() but this means that all the register definitions will
> - * change and become incompatible with the ones found in skiboot.
> - *
> - * Keep it as it is for now until we find a common ground.
> - */
> -static inline uint64_t GETFIELD(uint64_t mask, uint64_t word)
> -{
> -    return (word & mask) >> ctz64(mask);
> -}
> -
> -static inline uint64_t SETFIELD(uint64_t mask, uint64_t word,
> -                                uint64_t value)
> -{
> -    return (word & ~mask) | ((value << ctz64(mask)) & mask);
> -}
> -
>   /*
>    * TODO: Document block id override
>    */
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index 13ba9e45d8b6..0913e7c8f015 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -31,22 +31,6 @@
>       qemu_log_mask(LOG_GUEST_ERROR, "phb4_pec[%d:%d]: " fmt "\n",        \
>                     (pec)->chip_id, (pec)->index, ## __VA_ARGS__)
>   
> -/*
> - * QEMU version of the GETFIELD/SETFIELD macros
> - *
> - * These are common with the PnvXive model.
> - */
> -static inline uint64_t GETFIELD(uint64_t mask, uint64_t word)
> -{
> -    return (word & mask) >> ctz64(mask);
> -}
> -
> -static inline uint64_t SETFIELD(uint64_t mask, uint64_t word,
> -                                uint64_t value)
> -{
> -    return (word & ~mask) | ((value << ctz64(mask)) & mask);
> -}
> -
>   static PCIDevice *pnv_phb4_find_cfg_dev(PnvPHB4 *phb)
>   {
>       PCIHostState *pci = PCI_HOST_BRIDGE(phb);

Re: [PATCH qemu v2 1/2] ppc: Define SETFIELD for the ppc target

[Alexey Kardashevskiy]

On 6/25/22 06:12, Daniel Henrique Barboza wrote:
> Alexey,
> 
> The newer version of this patch is having trouble with Gitlab runners, as
> you can read in my feedback there.
> 
> I've tested this one just in case. The same problems happen. E.g. for the
> cross-armel-system runner:
> 
> 
> In file included from ../hw/intc/pnv_xive.c:14:
> ../hw/intc/pnv_xive.c: In function ‘pnv_xive_block_id’:
> /builds/danielhb/qemu/target/ppc/cpu.h:45:33: error: conversion from 
> ‘long long unsigned int’ to ‘long unsigned int’ changes value from 
> ‘4222124650659840’ to ‘0’ [-Werror=overflow]
>     45 | #define PPC_BITMASK(bs, be)     ((PPC_BIT(bs) - PPC_BIT(be)) | 
> PPC_BIT(bs))
>        |                                 
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> /builds/danielhb/qemu/target/ppc/cpu.h:51:42: note: in definition of 
> macro ‘GETFIELD’
>     51 |     (((word) & (mask)) >> __builtin_ctzl(mask))
>        |                                          ^~~~
> ../hw/intc/pnv_xive_regs.h:77:41: note: in expansion of macro ‘PPC_BITMASK’
>     77 | #define  PC_TCTXT_CHIPID                PPC_BITMASK(12, 15)
>        |                                         ^~~~~~~~~~~
> ../hw/intc/pnv_xive.c:80:24: note: in expansion of macro ‘PC_TCTXT_CHIPID’
>     80 |         blk = GETFIELD(PC_TCTXT_CHIPID, cfg_val);
>        |                        ^~~~~~~~~~~~~~~
> ../hw/intc/pnv_xive.c: In function ‘pnv_xive_vst_addr’:
> /builds/danielhb/qemu/target/ppc/cpu.h:45:33: error: conversion from 
> ‘long long unsigned int’ to ‘long unsigned int’ changes value from 
> ‘13835058055282163712’ to ‘0’ [-Werror=overflow]
>     45 | #define PPC_BITMASK(bs, be)     ((PPC_BIT(bs) - PPC_BIT(be)) | 
> PPC_BIT(bs))
>        |                                 
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> /builds/danielhb/qemu/target/ppc/cpu.h:51:42: note: in definition of 
> macro ‘GETFIELD’
>     51 |     (((word) & (mask)) >> __builtin_ctzl(mask))
>        |                                          ^~~~
> ../hw/intc/pnv_xive_regs.h:230:33: note: in expansion of macro 
> ‘PPC_BITMASK’
>    230 | #define VSD_MODE                PPC_BITMASK(0, 1)
>        |                                 ^~~~~~~~~~~
> ../hw/intc/pnv_xive.c:226:18: note: in expansion of macro ‘VSD_MODE’
>    226 |     if (GETFIELD(VSD_MODE, vsd) == VSD_MODE_FORWARD) {
>        |                  ^~~~~~~~
> ../hw/intc/pnv_xive.c: In function ‘pnv_xive_end_update’:
> 
> 
> Link:
> 
> https://gitlab.com/danielhb/qemu/-/jobs/2637716673
> 
> 
> I don´t know how to deal with that.
> 
> 
> For the record: if this is too troublesome to fix, I am ok with just 
> consolidating
> the GETFIELD and SETFIELD inlines we already have, under cpu.h, keeping 
> them exactly
> as they are today (functions, not macros).
> 
> 
> Thanks,
> 
> 
> Daniel
> 
> 
> 
> On 6/17/22 03:07, Alexey Kardashevskiy wrote:
>> It keeps repeating, move it to the header. This uses __builtin_ctzl() to
>> allow using the macros in #define.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>>   include/hw/pci-host/pnv_phb3_regs.h | 16 ----------------
>>   target/ppc/cpu.h                    |  5 +++++
>>   hw/intc/pnv_xive.c                  | 20 --------------------
>>   hw/intc/pnv_xive2.c                 | 20 --------------------
>>   hw/pci-host/pnv_phb4.c              | 16 ----------------
>>   5 files changed, 5 insertions(+), 72 deletions(-)
>>
>> diff --git a/include/hw/pci-host/pnv_phb3_regs.h 
>> b/include/hw/pci-host/pnv_phb3_regs.h
>> index a174ef1f7045..38f8ce9d7406 100644
>> --- a/include/hw/pci-host/pnv_phb3_regs.h
>> +++ b/include/hw/pci-host/pnv_phb3_regs.h
>> @@ -12,22 +12,6 @@
>>   #include "qemu/host-utils.h"
>> -/*
>> - * QEMU version of the GETFIELD/SETFIELD macros
>> - *
>> - * These are common with the PnvXive model.
>> - */
>> -static inline uint64_t GETFIELD(uint64_t mask, uint64_t word)
>> -{
>> -    return (word & mask) >> ctz64(mask);
>> -}
>> -
>> -static inline uint64_t SETFIELD(uint64_t mask, uint64_t word,
>> -                                uint64_t value)
>> -{
>> -    return (word & ~mask) | ((value << ctz64(mask)) & mask);
>> -}
>> -
>>   /*
>>    * PBCQ XSCOM registers
>>    */
>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
>> index 6d78078f379d..9a1f1e9999a3 100644
>> --- a/target/ppc/cpu.h
>> +++ b/target/ppc/cpu.h
>> @@ -47,6 +47,11 @@
>>                                    PPC_BIT32(bs))
>>   #define PPC_BITMASK8(bs, be)    ((PPC_BIT8(bs) - PPC_BIT8(be)) | 
>> PPC_BIT8(bs))
>> +#define GETFIELD(mask, word)   \
>> +    (((word) & (mask)) >> __builtin_ctzl(mask))


Replacing __builtin_ctzl with __builtin_ctzll seems fixing it though, do 
you have a quick way to test this? Gitlab's CI takes time :)
https://gitlab.com/aik1/qemu/-/pipelines/573497191 is the current run.
Thanks,


-- 
Alexey

Re: [PATCH qemu v2 1/2] ppc: Define SETFIELD for the ppc target

[Daniel Henrique Barboza]

On 6/27/22 01:54, Alexey Kardashevskiy wrote:
> 
> 
> On 6/25/22 06:12, Daniel Henrique Barboza wrote:
>> Alexey,
>>
>> The newer version of this patch is having trouble with Gitlab runners, as
>> you can read in my feedback there.
>>
>> I've tested this one just in case. The same problems happen. E.g. for the
>> cross-armel-system runner:
>>
>>
>> In file included from ../hw/intc/pnv_xive.c:14:
>> ../hw/intc/pnv_xive.c: In function ‘pnv_xive_block_id’:
>> /builds/danielhb/qemu/target/ppc/cpu.h:45:33: error: conversion from ‘long long unsigned int’ to ‘long unsigned int’ changes value from ‘4222124650659840’ to ‘0’ [-Werror=overflow]
>>     45 | #define PPC_BITMASK(bs, be)     ((PPC_BIT(bs) - PPC_BIT(be)) | PPC_BIT(bs))
>>        | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> /builds/danielhb/qemu/target/ppc/cpu.h:51:42: note: in definition of macro ‘GETFIELD’
>>     51 |     (((word) & (mask)) >> __builtin_ctzl(mask))
>>        |                                          ^~~~
>> ../hw/intc/pnv_xive_regs.h:77:41: note: in expansion of macro ‘PPC_BITMASK’
>>     77 | #define  PC_TCTXT_CHIPID                PPC_BITMASK(12, 15)
>>        |                                         ^~~~~~~~~~~
>> ../hw/intc/pnv_xive.c:80:24: note: in expansion of macro ‘PC_TCTXT_CHIPID’
>>     80 |         blk = GETFIELD(PC_TCTXT_CHIPID, cfg_val);
>>        |                        ^~~~~~~~~~~~~~~
>> ../hw/intc/pnv_xive.c: In function ‘pnv_xive_vst_addr’:
>> /builds/danielhb/qemu/target/ppc/cpu.h:45:33: error: conversion from ‘long long unsigned int’ to ‘long unsigned int’ changes value from ‘13835058055282163712’ to ‘0’ [-Werror=overflow]
>>     45 | #define PPC_BITMASK(bs, be)     ((PPC_BIT(bs) - PPC_BIT(be)) | PPC_BIT(bs))
>>        | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> /builds/danielhb/qemu/target/ppc/cpu.h:51:42: note: in definition of macro ‘GETFIELD’
>>     51 |     (((word) & (mask)) >> __builtin_ctzl(mask))
>>        |                                          ^~~~
>> ../hw/intc/pnv_xive_regs.h:230:33: note: in expansion of macro ‘PPC_BITMASK’
>>    230 | #define VSD_MODE                PPC_BITMASK(0, 1)
>>        |                                 ^~~~~~~~~~~
>> ../hw/intc/pnv_xive.c:226:18: note: in expansion of macro ‘VSD_MODE’
>>    226 |     if (GETFIELD(VSD_MODE, vsd) == VSD_MODE_FORWARD) {
>>        |                  ^~~~~~~~
>> ../hw/intc/pnv_xive.c: In function ‘pnv_xive_end_update’:
>>
>>
>> Link:
>>
>> https://gitlab.com/danielhb/qemu/-/jobs/2637716673
>>
>>
>> I don´t know how to deal with that.
>>
>>
>> For the record: if this is too troublesome to fix, I am ok with just consolidating
>> the GETFIELD and SETFIELD inlines we already have, under cpu.h, keeping them exactly
>> as they are today (functions, not macros).
>>
>>
>> Thanks,
>>
>>
>> Daniel
>>
>>
>>
>> On 6/17/22 03:07, Alexey Kardashevskiy wrote:
>>> It keeps repeating, move it to the header. This uses __builtin_ctzl() to
>>> allow using the macros in #define.
>>>
>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>> ---
>>>   include/hw/pci-host/pnv_phb3_regs.h | 16 ----------------
>>>   target/ppc/cpu.h                    |  5 +++++
>>>   hw/intc/pnv_xive.c                  | 20 --------------------
>>>   hw/intc/pnv_xive2.c                 | 20 --------------------
>>>   hw/pci-host/pnv_phb4.c              | 16 ----------------
>>>   5 files changed, 5 insertions(+), 72 deletions(-)
>>>
>>> diff --git a/include/hw/pci-host/pnv_phb3_regs.h b/include/hw/pci-host/pnv_phb3_regs.h
>>> index a174ef1f7045..38f8ce9d7406 100644
>>> --- a/include/hw/pci-host/pnv_phb3_regs.h
>>> +++ b/include/hw/pci-host/pnv_phb3_regs.h
>>> @@ -12,22 +12,6 @@
>>>   #include "qemu/host-utils.h"
>>> -/*
>>> - * QEMU version of the GETFIELD/SETFIELD macros
>>> - *
>>> - * These are common with the PnvXive model.
>>> - */
>>> -static inline uint64_t GETFIELD(uint64_t mask, uint64_t word)
>>> -{
>>> -    return (word & mask) >> ctz64(mask);
>>> -}
>>> -
>>> -static inline uint64_t SETFIELD(uint64_t mask, uint64_t word,
>>> -                                uint64_t value)
>>> -{
>>> -    return (word & ~mask) | ((value << ctz64(mask)) & mask);
>>> -}
>>> -
>>>   /*
>>>    * PBCQ XSCOM registers
>>>    */
>>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
>>> index 6d78078f379d..9a1f1e9999a3 100644
>>> --- a/target/ppc/cpu.h
>>> +++ b/target/ppc/cpu.h
>>> @@ -47,6 +47,11 @@
>>>                                    PPC_BIT32(bs))
>>>   #define PPC_BITMASK8(bs, be)    ((PPC_BIT8(bs) - PPC_BIT8(be)) | PPC_BIT8(bs))
>>> +#define GETFIELD(mask, word)   \
>>> +    (((word) & (mask)) >> __builtin_ctzl(mask))
> 
> 
> Replacing __builtin_ctzl with __builtin_ctzll seems fixing it though, do you have a quick way to test this? Gitlab's CI takes time :)
> https://gitlab.com/aik1/qemu/-/pipelines/573497191 is the current run.


I'll take a look, but apparently it fixed the problem I reported up above.

Also, there's an error in the msys2-64bit runner that I keep seeing from time to
time. It goes away eventually if you keep retrying it:


[301/1664] Generating input-keymap-qcode-to-qnum.c.inc with a custom command (wrapped by meson to capture output)
[302/1664] Generating input-keymap-qcode-to-sun.c.inc with a custom command (wrapped by meson to capture output)
[303/1664] Generating input-keymap-qnum-to-qcode.c.inc with a custom command (wrapped by meson to capture output)
[304/1664] Generating input-keymap-usb-to-qcode.c.inc with a custom command (wrapped by meson to capture output)
FAILED: ui/input-keymap-usb-to-qcode.c.inc
"C:/GitLab-Runner/builds/aik1/qemu/msys64/mingw64/bin/python3.exe" "C:/GitLab-Runner/builds/aik1/qemu/meson/meson.py" "--internal" "exe" "--capture" "ui/input-keymap-usb-to-qcode.c.inc" "--" "C:/GitLab-Runner/builds/aik1/qemu/msys64/mingw64/bin/python3.exe" "../ui/keycodemapdb/tools/keymap-gen" "code-map" "--lang" "glib2" "--varname" "qemu_input_map_usb_to_qcode" "../ui/keycodemapdb/data/keymaps.csv" "usb" "qcode"
[305/1664] Generating input-keymap-win32-to-qcode.c.inc with a custom command (wrapped by meson to capture output)
ninja: build stopped: subcommand failed.
make[1]: Leaving directory '/c/GitLab-Runner/builds/aik1/qemu/build'
make[1]: *** [Makefile:162: run-ninja] Error 1
make: *** [GNUmakefile:11: all] Error 2


It's a strange one because it's an error triggered by an ui/keymap file which you're
not changing.

Richard already created a thread about it in the QEMU ML, so I'll assume that
this has nothing to do with powerpc code.


Thanks,

Daniel


> Thanks,
> 
> 

Re: [PATCH qemu v2 1/2] ppc: Define SETFIELD for the ppc target

[Daniel Henrique Barboza]

On 6/27/22 01:54, Alexey Kardashevskiy wrote:
> 
> 
> On 6/25/22 06:12, Daniel Henrique Barboza wrote:
>> Alexey,
>>
>> The newer version of this patch is having trouble with Gitlab runners, as
>> you can read in my feedback there.
>>
>> I've tested this one just in case. The same problems happen. E.g. for the
>> cross-armel-system runner:
>>
>>
>> In file included from ../hw/intc/pnv_xive.c:14:
>> ../hw/intc/pnv_xive.c: In function ‘pnv_xive_block_id’:
>> /builds/danielhb/qemu/target/ppc/cpu.h:45:33: error: conversion from ‘long long unsigned int’ to ‘long unsigned int’ changes value from ‘4222124650659840’ to ‘0’ [-Werror=overflow]
>>     45 | #define PPC_BITMASK(bs, be)     ((PPC_BIT(bs) - PPC_BIT(be)) | PPC_BIT(bs))
>>        | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> /builds/danielhb/qemu/target/ppc/cpu.h:51:42: note: in definition of macro ‘GETFIELD’
>>     51 |     (((word) & (mask)) >> __builtin_ctzl(mask))
>>        |                                          ^~~~
>> ../hw/intc/pnv_xive_regs.h:77:41: note: in expansion of macro ‘PPC_BITMASK’
>>     77 | #define  PC_TCTXT_CHIPID                PPC_BITMASK(12, 15)
>>        |                                         ^~~~~~~~~~~
>> ../hw/intc/pnv_xive.c:80:24: note: in expansion of macro ‘PC_TCTXT_CHIPID’
>>     80 |         blk = GETFIELD(PC_TCTXT_CHIPID, cfg_val);
>>        |                        ^~~~~~~~~~~~~~~
>> ../hw/intc/pnv_xive.c: In function ‘pnv_xive_vst_addr’:
>> /builds/danielhb/qemu/target/ppc/cpu.h:45:33: error: conversion from ‘long long unsigned int’ to ‘long unsigned int’ changes value from ‘13835058055282163712’ to ‘0’ [-Werror=overflow]
>>     45 | #define PPC_BITMASK(bs, be)     ((PPC_BIT(bs) - PPC_BIT(be)) | PPC_BIT(bs))
>>        | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> /builds/danielhb/qemu/target/ppc/cpu.h:51:42: note: in definition of macro ‘GETFIELD’
>>     51 |     (((word) & (mask)) >> __builtin_ctzl(mask))
>>        |                                          ^~~~
>> ../hw/intc/pnv_xive_regs.h:230:33: note: in expansion of macro ‘PPC_BITMASK’
>>    230 | #define VSD_MODE                PPC_BITMASK(0, 1)
>>        |                                 ^~~~~~~~~~~
>> ../hw/intc/pnv_xive.c:226:18: note: in expansion of macro ‘VSD_MODE’
>>    226 |     if (GETFIELD(VSD_MODE, vsd) == VSD_MODE_FORWARD) {
>>        |                  ^~~~~~~~
>> ../hw/intc/pnv_xive.c: In function ‘pnv_xive_end_update’:
>>
>>
>> Link:
>>
>> https://gitlab.com/danielhb/qemu/-/jobs/2637716673
>>
>>
>> I don´t know how to deal with that.
>>
>>
>> For the record: if this is too troublesome to fix, I am ok with just consolidating
>> the GETFIELD and SETFIELD inlines we already have, under cpu.h, keeping them exactly
>> as they are today (functions, not macros).
>>
>>
>> Thanks,
>>
>>
>> Daniel
>>
>>
>>
>> On 6/17/22 03:07, Alexey Kardashevskiy wrote:
>>> It keeps repeating, move it to the header. This uses __builtin_ctzl() to
>>> allow using the macros in #define.
>>>
>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>> ---
>>>   include/hw/pci-host/pnv_phb3_regs.h | 16 ----------------
>>>   target/ppc/cpu.h                    |  5 +++++
>>>   hw/intc/pnv_xive.c                  | 20 --------------------
>>>   hw/intc/pnv_xive2.c                 | 20 --------------------
>>>   hw/pci-host/pnv_phb4.c              | 16 ----------------
>>>   5 files changed, 5 insertions(+), 72 deletions(-)
>>>
>>> diff --git a/include/hw/pci-host/pnv_phb3_regs.h b/include/hw/pci-host/pnv_phb3_regs.h
>>> index a174ef1f7045..38f8ce9d7406 100644
>>> --- a/include/hw/pci-host/pnv_phb3_regs.h
>>> +++ b/include/hw/pci-host/pnv_phb3_regs.h
>>> @@ -12,22 +12,6 @@
>>>   #include "qemu/host-utils.h"
>>> -/*
>>> - * QEMU version of the GETFIELD/SETFIELD macros
>>> - *
>>> - * These are common with the PnvXive model.
>>> - */
>>> -static inline uint64_t GETFIELD(uint64_t mask, uint64_t word)
>>> -{
>>> -    return (word & mask) >> ctz64(mask);
>>> -}
>>> -
>>> -static inline uint64_t SETFIELD(uint64_t mask, uint64_t word,
>>> -                                uint64_t value)
>>> -{
>>> -    return (word & ~mask) | ((value << ctz64(mask)) & mask);
>>> -}
>>> -
>>>   /*
>>>    * PBCQ XSCOM registers
>>>    */
>>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
>>> index 6d78078f379d..9a1f1e9999a3 100644
>>> --- a/target/ppc/cpu.h
>>> +++ b/target/ppc/cpu.h
>>> @@ -47,6 +47,11 @@
>>>                                    PPC_BIT32(bs))
>>>   #define PPC_BITMASK8(bs, be)    ((PPC_BIT8(bs) - PPC_BIT8(be)) | PPC_BIT8(bs))
>>> +#define GETFIELD(mask, word)   \
>>> +    (((word) & (mask)) >> __builtin_ctzl(mask))
> 
> 
> Replacing __builtin_ctzl with __builtin_ctzll seems fixing it though, do you have a quick way to test this? Gitlab's CI takes time :)
> https://gitlab.com/aik1/qemu/-/pipelines/573497191 is the current run.
> Thanks,

This worked for me as well.

Can you re-send this patch with this fix plus the extra comment Peter mentioned
in his review?

----
Can we retain the explanatory comment that says why we don't
use the standard QEMU mechanism for field extraction
(ie the FIELD_EX*/FIELD_DP* macros and the extract64()/deposit64()
functions) ?
------


You can re-send this as v4 (I'm assuming that we're giving up trying to copy
the Skiboot macros) and then I'll take the patch together with the watchdog
v3 implementation.


Thanks,

Daniel

> 
> 

Re: [PATCH qemu v2 1/2] ppc: Define SETFIELD for the ppc target

[Alexey Kardashevskiy]

On 6/28/22 04:04, Daniel Henrique Barboza wrote:
> 
> 
> On 6/27/22 01:54, Alexey Kardashevskiy wrote:
>>
>>
>> On 6/25/22 06:12, Daniel Henrique Barboza wrote:
>>> Alexey,
>>>
>>> The newer version of this patch is having trouble with Gitlab 
>>> runners, as
>>> you can read in my feedback there.
>>>
>>> I've tested this one just in case. The same problems happen. E.g. for 
>>> the
>>> cross-armel-system runner:
>>>
>>>
>>> In file included from ../hw/intc/pnv_xive.c:14:
>>> ../hw/intc/pnv_xive.c: In function ‘pnv_xive_block_id’:
>>> /builds/danielhb/qemu/target/ppc/cpu.h:45:33: error: conversion from 
>>> ‘long long unsigned int’ to ‘long unsigned int’ changes value from 
>>> ‘4222124650659840’ to ‘0’ [-Werror=overflow]
>>>     45 | #define PPC_BITMASK(bs, be)     ((PPC_BIT(bs) - PPC_BIT(be)) 
>>> | PPC_BIT(bs))
>>>        | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> /builds/danielhb/qemu/target/ppc/cpu.h:51:42: note: in definition of 
>>> macro ‘GETFIELD’
>>>     51 |     (((word) & (mask)) >> __builtin_ctzl(mask))
>>>        |                                          ^~~~
>>> ../hw/intc/pnv_xive_regs.h:77:41: note: in expansion of macro 
>>> ‘PPC_BITMASK’
>>>     77 | #define  PC_TCTXT_CHIPID                PPC_BITMASK(12, 15)
>>>        |                                         ^~~~~~~~~~~
>>> ../hw/intc/pnv_xive.c:80:24: note: in expansion of macro 
>>> ‘PC_TCTXT_CHIPID’
>>>     80 |         blk = GETFIELD(PC_TCTXT_CHIPID, cfg_val);
>>>        |                        ^~~~~~~~~~~~~~~
>>> ../hw/intc/pnv_xive.c: In function ‘pnv_xive_vst_addr’:
>>> /builds/danielhb/qemu/target/ppc/cpu.h:45:33: error: conversion from 
>>> ‘long long unsigned int’ to ‘long unsigned int’ changes value from 
>>> ‘13835058055282163712’ to ‘0’ [-Werror=overflow]
>>>     45 | #define PPC_BITMASK(bs, be)     ((PPC_BIT(bs) - PPC_BIT(be)) 
>>> | PPC_BIT(bs))
>>>        | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> /builds/danielhb/qemu/target/ppc/cpu.h:51:42: note: in definition of 
>>> macro ‘GETFIELD’
>>>     51 |     (((word) & (mask)) >> __builtin_ctzl(mask))
>>>        |                                          ^~~~
>>> ../hw/intc/pnv_xive_regs.h:230:33: note: in expansion of macro 
>>> ‘PPC_BITMASK’
>>>    230 | #define VSD_MODE                PPC_BITMASK(0, 1)
>>>        |                                 ^~~~~~~~~~~
>>> ../hw/intc/pnv_xive.c:226:18: note: in expansion of macro ‘VSD_MODE’
>>>    226 |     if (GETFIELD(VSD_MODE, vsd) == VSD_MODE_FORWARD) {
>>>        |                  ^~~~~~~~
>>> ../hw/intc/pnv_xive.c: In function ‘pnv_xive_end_update’:
>>>
>>>
>>> Link:
>>>
>>> https://gitlab.com/danielhb/qemu/-/jobs/2637716673
>>>
>>>
>>> I don´t know how to deal with that.
>>>
>>>
>>> For the record: if this is too troublesome to fix, I am ok with just 
>>> consolidating
>>> the GETFIELD and SETFIELD inlines we already have, under cpu.h, 
>>> keeping them exactly
>>> as they are today (functions, not macros).
>>>
>>>
>>> Thanks,
>>>
>>>
>>> Daniel
>>>
>>>
>>>
>>> On 6/17/22 03:07, Alexey Kardashevskiy wrote:
>>>> It keeps repeating, move it to the header. This uses 
>>>> __builtin_ctzl() to
>>>> allow using the macros in #define.
>>>>
>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>> ---
>>>>   include/hw/pci-host/pnv_phb3_regs.h | 16 ----------------
>>>>   target/ppc/cpu.h                    |  5 +++++
>>>>   hw/intc/pnv_xive.c                  | 20 --------------------
>>>>   hw/intc/pnv_xive2.c                 | 20 --------------------
>>>>   hw/pci-host/pnv_phb4.c              | 16 ----------------
>>>>   5 files changed, 5 insertions(+), 72 deletions(-)
>>>>
>>>> diff --git a/include/hw/pci-host/pnv_phb3_regs.h 
>>>> b/include/hw/pci-host/pnv_phb3_regs.h
>>>> index a174ef1f7045..38f8ce9d7406 100644
>>>> --- a/include/hw/pci-host/pnv_phb3_regs.h
>>>> +++ b/include/hw/pci-host/pnv_phb3_regs.h
>>>> @@ -12,22 +12,6 @@
>>>>   #include "qemu/host-utils.h"
>>>> -/*
>>>> - * QEMU version of the GETFIELD/SETFIELD macros
>>>> - *
>>>> - * These are common with the PnvXive model.
>>>> - */
>>>> -static inline uint64_t GETFIELD(uint64_t mask, uint64_t word)
>>>> -{
>>>> -    return (word & mask) >> ctz64(mask);
>>>> -}
>>>> -
>>>> -static inline uint64_t SETFIELD(uint64_t mask, uint64_t word,
>>>> -                                uint64_t value)
>>>> -{
>>>> -    return (word & ~mask) | ((value << ctz64(mask)) & mask);
>>>> -}
>>>> -
>>>>   /*
>>>>    * PBCQ XSCOM registers
>>>>    */
>>>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
>>>> index 6d78078f379d..9a1f1e9999a3 100644
>>>> --- a/target/ppc/cpu.h
>>>> +++ b/target/ppc/cpu.h
>>>> @@ -47,6 +47,11 @@
>>>>                                    PPC_BIT32(bs))
>>>>   #define PPC_BITMASK8(bs, be)    ((PPC_BIT8(bs) - PPC_BIT8(be)) | 
>>>> PPC_BIT8(bs))
>>>> +#define GETFIELD(mask, word)   \
>>>> +    (((word) & (mask)) >> __builtin_ctzl(mask))
>>
>>
>> Replacing __builtin_ctzl with __builtin_ctzll seems fixing it though, 
>> do you have a quick way to test this? Gitlab's CI takes time :)
>> https://gitlab.com/aik1/qemu/-/pipelines/573497191 is the current run.
>> Thanks,
> 
> This worked for me as well.
> 
> Can you re-send this patch with this fix plus the extra comment Peter 
> mentioned
> in his review?
> 
> ----
> Can we retain the explanatory comment that says why we don't
> use the standard QEMU mechanism for field extraction
> (ie the FIELD_EX*/FIELD_DP* macros and the extract64()/deposit64()
> functions) ?
> ------
> 
> 
> You can re-send this as v4 (I'm assuming that we're giving up trying to 
> copy
> the Skiboot macros) and


Sure, will do.

> then I'll take the patch together with the watchdog
> v3 implementation.

The watchdog patch is not using these macros anymore, I moved to QEMU's 
macros, this is why separate patches. Thanks,


> 
> 
> Thanks,
> 
> Daniel
> 
>>
>>

-- 
Alexey

[PATCH qemu v2 2/2] ppc/spapr: Implement H_WATCHDOG

[Alexey Kardashevskiy]

The new PAPR 2.12 defines a watchdog facility managed via the new
H_WATCHDOG hypercall.

This adds H_WATCHDOG support which a proposed driver for pseries uses:
https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=303120

This was tested by running QEMU with a debug kernel and command line:
-append \
 "pseries-wdt.timeout=60 pseries-wdt.nowayout=1 pseries-wdt.action=2"

and running "echo V > /dev/watchdog0" inside the VM.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
Changes:
v2:
* QOM'ed timers, "action" and "expire" are available via QMP
* removed @timeout from SpaprWatchdog
* moved the driver to hw/watchdog
* fixed error handling in the hcall handler
* used new SETFIELD/GETFIELD
---
 include/hw/ppc/spapr.h       |  29 +++-
 hw/ppc/spapr.c               |   4 +
 hw/watchdog/spapr_watchdog.c | 248 +++++++++++++++++++++++++++++++++++
 hw/watchdog/meson.build      |   1 +
 hw/watchdog/trace-events     |   7 +
 5 files changed, 288 insertions(+), 1 deletion(-)
 create mode 100644 hw/watchdog/spapr_watchdog.c

diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 072dda2c7265..ef1e38abd5c7 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -164,6 +164,25 @@ struct SpaprMachineClass {
     SpaprIrq *irq;
 };
 
+#define WDT_MAX_WATCHDOGS       4      /* Maximum number of watchdog devices */
+
+#define WDT_HARD_POWER_OFF      0
+#define WDT_HARD_RESTART        1
+#define WDT_DUMP_RESTART        2
+
+#define TYPE_SPAPR_WDT "spapr-wdt"
+OBJECT_DECLARE_SIMPLE_TYPE(SpaprWatchdog, SPAPR_WDT)
+
+typedef struct SpaprWatchdog {
+    /*< private >*/
+    DeviceState parent_obj;
+    /*< public >*/
+
+    unsigned num;
+    QEMUTimer timer;
+    uint8_t action;
+} SpaprWatchdog;
+
 /**
  * SpaprMachineState:
  */
@@ -264,6 +283,8 @@ struct SpaprMachineState {
     uint32_t FORM2_assoc_array[NUMA_NODES_MAX_NUM][FORM2_NUMA_ASSOC_SIZE];
 
     Error *fwnmi_migration_blocker;
+
+    SpaprWatchdog wds[WDT_MAX_WATCHDOGS];
 };
 
 #define H_SUCCESS         0
@@ -344,6 +365,7 @@ struct SpaprMachineState {
 #define H_P7              -60
 #define H_P8              -61
 #define H_P9              -62
+#define H_NOOP            -63
 #define H_UNSUPPORTED     -67
 #define H_OVERLAP         -68
 #define H_UNSUPPORTED_FLAG -256
@@ -564,8 +586,9 @@ struct SpaprMachineState {
 #define H_SCM_HEALTH            0x400
 #define H_RPT_INVALIDATE        0x448
 #define H_SCM_FLUSH             0x44C
+#define H_WATCHDOG              0x45C
 
-#define MAX_HCALL_OPCODE        H_SCM_FLUSH
+#define MAX_HCALL_OPCODE        H_WATCHDOG
 
 /* The hcalls above are standardized in PAPR and implemented by pHyp
  * as well.
@@ -1027,6 +1050,7 @@ extern const VMStateDescription vmstate_spapr_cap_large_decr;
 extern const VMStateDescription vmstate_spapr_cap_ccf_assist;
 extern const VMStateDescription vmstate_spapr_cap_fwnmi;
 extern const VMStateDescription vmstate_spapr_cap_rpt_invalidate;
+extern const VMStateDescription vmstate_spapr_wdt;
 
 static inline uint8_t spapr_get_cap(SpaprMachineState *spapr, int cap)
 {
@@ -1063,4 +1087,7 @@ target_ulong spapr_vof_client_architecture_support(MachineState *ms,
                                                    target_ulong ovec_addr);
 void spapr_vof_client_dt_finalize(SpaprMachineState *spapr, void *fdt);
 
+/* H_WATCHDOG */
+void spapr_watchdog_init(SpaprMachineState *spapr);
+
 #endif /* HW_SPAPR_H */
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index fd4942e8813c..9a5382d5270f 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -898,6 +898,8 @@ static void spapr_dt_rtas(SpaprMachineState *spapr, void *fdt)
         add_str(hypertas, "hcall-hpt-resize");
     }
 
+    add_str(hypertas, "hcall-watchdog");
+
     _FDT(fdt_setprop(fdt, rtas, "ibm,hypertas-functions",
                      hypertas->str, hypertas->len));
     g_string_free(hypertas, TRUE);
@@ -3051,6 +3053,8 @@ static void spapr_machine_init(MachineState *machine)
         spapr->vof->fw_size = fw_size; /* for claim() on itself */
         spapr_register_hypercall(KVMPPC_H_VOF_CLIENT, spapr_h_vof_client);
     }
+
+    spapr_watchdog_init(spapr);
 }
 
 #define DEFAULT_KVM_TYPE "auto"
diff --git a/hw/watchdog/spapr_watchdog.c b/hw/watchdog/spapr_watchdog.c
new file mode 100644
index 000000000000..aeaf7c52cbad
--- /dev/null
+++ b/hw/watchdog/spapr_watchdog.c
@@ -0,0 +1,248 @@
+/*
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "target/ppc/cpu.h"
+#include "migration/vmstate.h"
+#include "trace.h"
+
+#include "hw/ppc/spapr.h"
+
+/*
+ * Bits 47: "leaveOtherWatchdogsRunningOnTimeout", specified on
+ * the "Start watchdog" operation,
+ * 0 - stop out-standing watchdogs on timeout,
+ * 1 - leave outstanding watchdogs running on timeout
+ */
+#define PSERIES_WDTF_LEAVE_OTHER    PPC_BIT(47)
+
+/*    Bits 48-55: "operation" */
+#define PSERIES_WDTF_OP(op)             SETFIELD(PPC_BITMASK(48, 55), 0, (op))
+#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" */
+#define PSERIES_WDTF_ACTION(ac)         SETFIELD(PPC_BITMASK(56, 63), 0, (ac))
+#define PSERIES_WDTF_ACTION_HARD_POWER_OFF  PSERIES_WDTF_ACTION(0x1)
+#define PSERIES_WDTF_ACTION_HARD_RESTART    PSERIES_WDTF_ACTION(0x2)
+#define PSERIES_WDTF_ACTION_DUMP_RESTART    PSERIES_WDTF_ACTION(0x3)
+#define PSERIES_WDTF_RESERVED           PPC_BITMASK(0, 46)
+
+/*
+ * For the "Query watchdog capabilities" operation, a uint64 structure
+ * defined as:
+ * Bits 0-15: The minimum supported timeout in milliseconds
+ * Bits 16-31: The number of watchdogs supported
+ * Bits 32-63: Reserved
+ */
+#define PSERIES_WDTQ_MIN_TIMEOUT(ms)    SETFIELD(PPC_BITMASK(0, 15), 0, (ms))
+#define PSERIES_WDTQ_NUM(n)             SETFIELD(PPC_BITMASK(16, 31), 0, (n))
+
+/*
+ * For the "Query watchdog LPM requirement" operation:
+ * 1 = The given "watchdogNumber" must be stopped prior to suspending
+ * 2 = The given "watchdogNumber" does not have to be stopped prior to
+ * suspending
+ */
+#define PSERIES_WDTQL_STOPPED               1
+#define PSERIES_WDTQL_QUERY_NOT_STOPPED     2
+
+#define WDT_MIN_TIMEOUT 1 /* 1ms */
+
+static void watchdog_expired(void *pw)
+{
+    struct SpaprWatchdog *w = pw;
+    CPUState *cs;
+
+    trace_spapr_watchdog_expired(w->num, w->action);
+    switch (w->action) {
+    case WDT_HARD_POWER_OFF:
+        qemu_system_vmstop_request(RUN_STATE_SHUTDOWN);
+        break;
+    case WDT_HARD_RESTART:
+        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
+        break;
+    case WDT_DUMP_RESTART:
+        CPU_FOREACH(cs) {
+            async_run_on_cpu(cs, spapr_do_system_reset_on_cpu, RUN_ON_CPU_NULL);
+        }
+        break;
+    }
+}
+
+static target_ulong watchdog_stop(unsigned watchdogNumber,
+                                  struct SpaprWatchdog *w)
+{
+    target_ulong ret = H_NOOP;
+
+    if (timer_pending(&w->timer)) {
+        timer_del(&w->timer);
+        ret = H_SUCCESS;
+    }
+    trace_spapr_watchdog_stop(watchdogNumber, ret);
+
+    return ret;
+}
+
+static target_ulong h_watchdog(PowerPCCPU *cpu,
+                               SpaprMachineState *spapr,
+                               target_ulong opcode, target_ulong *args)
+{
+    target_ulong flags = args[0];
+    target_ulong watchdogNumber = args[1];
+    target_ulong timeoutInMs = args[2];
+    unsigned operation = flags & PSERIES_WDTF_OP(~0);
+    unsigned timeoutAction = flags & PSERIES_WDTF_ACTION(~0);
+    struct SpaprWatchdog *w;
+
+    if (flags & PSERIES_WDTF_RESERVED) {
+        return H_PARAMETER;
+    }
+
+    switch (operation) {
+    case PSERIES_WDTF_OP_START:
+        if (watchdogNumber > ARRAY_SIZE(spapr->wds)) {
+            return H_P2;
+        }
+        if (timeoutInMs <= WDT_MIN_TIMEOUT) {
+            return H_P3;
+        }
+
+        w = &spapr->wds[watchdogNumber - 1];
+        switch (timeoutAction) {
+        case PSERIES_WDTF_ACTION_HARD_POWER_OFF:
+            w->action = WDT_HARD_POWER_OFF;
+            break;
+        case PSERIES_WDTF_ACTION_HARD_RESTART:
+            w->action = WDT_HARD_RESTART;
+            break;
+        case PSERIES_WDTF_ACTION_DUMP_RESTART:
+            w->action = WDT_DUMP_RESTART;
+            break;
+        default:
+            return H_PARAMETER;
+        }
+        timer_mod(&w->timer,
+                  qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + timeoutInMs);
+        trace_spapr_watchdog_start(flags, watchdogNumber, timeoutInMs);
+        break;
+    case PSERIES_WDTF_OP_STOP:
+        if (watchdogNumber == (uint64_t) ~0) {
+            int i;
+
+            for (i = 1; i <= ARRAY_SIZE(spapr->wds); ++i) {
+                watchdog_stop(i, &spapr->wds[i - 1]);
+            }
+        } else if (watchdogNumber <= ARRAY_SIZE(spapr->wds)) {
+            watchdog_stop(watchdogNumber, &spapr->wds[watchdogNumber - 1]);
+        } else {
+            return H_P2;
+        }
+        break;
+    case PSERIES_WDTF_OP_QUERY:
+        args[0] = PSERIES_WDTQ_MIN_TIMEOUT(WDT_MIN_TIMEOUT) |
+            PSERIES_WDTQ_NUM(ARRAY_SIZE(spapr->wds));
+        trace_spapr_watchdog_query(args[0]);
+        break;
+    case PSERIES_WDTF_OP_QUERY_LPM:
+        if (watchdogNumber > ARRAY_SIZE(spapr->wds)) {
+            return H_P2;
+        }
+        args[0] = PSERIES_WDTQL_QUERY_NOT_STOPPED;
+        trace_spapr_watchdog_query_lpm(args[0]);
+        break;
+    default:
+        return H_PARAMETER;
+    }
+
+    return H_SUCCESS;
+}
+
+void spapr_watchdog_init(SpaprMachineState *spapr)
+{
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(spapr->wds); ++i) {
+        char name[16];
+        SpaprWatchdog *w = &spapr->wds[i];
+
+        w->num = i + 1;
+        snprintf(name, sizeof(name) - 1, "wdt%d", i + 1);
+        object_initialize_child_with_props(OBJECT(spapr), name, w,
+                                           sizeof(SpaprWatchdog),
+                                           TYPE_SPAPR_WDT,
+                                           &error_fatal, NULL);
+        qdev_realize(DEVICE(w), NULL, &error_fatal);
+    }
+}
+
+static bool watchdog_needed(void *opaque)
+{
+    SpaprWatchdog *w = opaque;
+
+    return timer_pending(&w->timer);
+}
+
+static const VMStateDescription vmstate_wdt = {
+    .name = "spapr_watchdog",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = watchdog_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT8(action, SpaprWatchdog),
+        VMSTATE_TIMER(timer, SpaprWatchdog),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static void spapr_wdt_realize(DeviceState *dev, Error **errp)
+{
+    SpaprWatchdog *w = SPAPR_WDT(dev);
+
+    timer_init_ms(&w->timer, QEMU_CLOCK_VIRTUAL, watchdog_expired, w);
+
+    object_property_add_uint64_ptr(OBJECT(dev), "expire",
+                                   (uint64_t *)&w->timer.expire_time,
+                                   OBJ_PROP_FLAG_READ);
+    object_property_add_uint8_ptr(OBJECT(dev), "action", &w->action,
+                                  OBJ_PROP_FLAG_READ);
+}
+
+static void spapr_wdt_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+
+    dc->realize = spapr_wdt_realize;
+    dc->vmsd = &vmstate_wdt;
+    dc->user_creatable = false;
+}
+
+static const TypeInfo spapr_wdt_info = {
+    .name          = TYPE_SPAPR_WDT,
+    .parent        = TYPE_DEVICE,
+    .instance_size = sizeof(SpaprWatchdog),
+    .class_init    = spapr_wdt_class_init,
+};
+
+static void spapr_watchdog_register_types(void)
+{
+    spapr_register_hypercall(H_WATCHDOG, h_watchdog);
+    type_register_static(&spapr_wdt_info);
+}
+
+type_init(spapr_watchdog_register_types)
diff --git a/hw/watchdog/meson.build b/hw/watchdog/meson.build
index 054c403dea7c..8974b5cf4c8a 100644
--- a/hw/watchdog/meson.build
+++ b/hw/watchdog/meson.build
@@ -6,3 +6,4 @@ softmmu_ss.add(when: 'CONFIG_WDT_DIAG288', if_true: files('wdt_diag288.c'))
 softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files('wdt_aspeed.c'))
 softmmu_ss.add(when: 'CONFIG_WDT_IMX2', if_true: files('wdt_imx2.c'))
 softmmu_ss.add(when: 'CONFIG_WDT_SBSA', if_true: files('sbsa_gwdt.c'))
+specific_ss.add(when: 'CONFIG_PSERIES', if_true: files('spapr_watchdog.c'))
diff --git a/hw/watchdog/trace-events b/hw/watchdog/trace-events
index e7523e22aaf2..89ccbcfdfd20 100644
--- a/hw/watchdog/trace-events
+++ b/hw/watchdog/trace-events
@@ -9,3 +9,10 @@ cmsdk_apb_watchdog_lock(uint32_t lock) "CMSDK APB watchdog: lock %" PRIu32
 # wdt-aspeed.c
 aspeed_wdt_read(uint64_t addr, uint32_t size) "@0x%" PRIx64 " size=%d"
 aspeed_wdt_write(uint64_t addr, uint32_t size, uint64_t data) "@0x%" PRIx64 " size=%d value=0x%"PRIx64
+
+# spapr_watchdog.c
+spapr_watchdog_start(uint64_t flags, uint64_t num, uint64_t timeout) "Flags 0x%" PRIx64 " num=%" PRId64 " %" PRIu64 "ms"
+spapr_watchdog_stop(uint64_t num, uint64_t ret) "num=%" PRIu64 " ret=%" PRId64
+spapr_watchdog_query(uint64_t caps) "caps=0x%" PRIx64
+spapr_watchdog_query_lpm(uint64_t caps) "caps=0x%" PRIx64
+spapr_watchdog_expired(uint64_t num, unsigned action) "num=%" PRIu64 " action=%u"
-- 
2.30.2

Re: [PATCH qemu v2 2/2] ppc/spapr: Implement H_WATCHDOG

[Daniel Henrique Barboza]

On 6/17/22 03:07, Alexey Kardashevskiy wrote:
> The new PAPR 2.12 defines a watchdog facility managed via the new
> H_WATCHDOG hypercall.
> 
> This adds H_WATCHDOG support which a proposed driver for pseries uses:
> https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=303120
> 
> This was tested by running QEMU with a debug kernel and command line:
> -append \
>   "pseries-wdt.timeout=60 pseries-wdt.nowayout=1 pseries-wdt.action=2"
> 
> and running "echo V > /dev/watchdog0" inside the VM.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---

LGTM. The watchdogs can be found under /machines/wdtN:

(qemu) info qom-tree /machine/wdt1
/wdt1 (spapr-wdt)
(qemu) info qom-tree /machine/wdt2
/wdt2 (spapr-wdt)
(qemu) info qom-tree /machine/wdt3
/wdt3 (spapr-wdt)
(qemu) info qom-tree /machine/wdt4
/wdt4 (spapr-wdt)


Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

> Changes:
> v2:
> * QOM'ed timers, "action" and "expire" are available via QMP
> * removed @timeout from SpaprWatchdog
> * moved the driver to hw/watchdog
> * fixed error handling in the hcall handler
> * used new SETFIELD/GETFIELD
> ---
>   include/hw/ppc/spapr.h       |  29 +++-
>   hw/ppc/spapr.c               |   4 +
>   hw/watchdog/spapr_watchdog.c | 248 +++++++++++++++++++++++++++++++++++
>   hw/watchdog/meson.build      |   1 +
>   hw/watchdog/trace-events     |   7 +
>   5 files changed, 288 insertions(+), 1 deletion(-)
>   create mode 100644 hw/watchdog/spapr_watchdog.c
> 
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 072dda2c7265..ef1e38abd5c7 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -164,6 +164,25 @@ struct SpaprMachineClass {
>       SpaprIrq *irq;
>   };
>   
> +#define WDT_MAX_WATCHDOGS       4      /* Maximum number of watchdog devices */
> +
> +#define WDT_HARD_POWER_OFF      0
> +#define WDT_HARD_RESTART        1
> +#define WDT_DUMP_RESTART        2
> +
> +#define TYPE_SPAPR_WDT "spapr-wdt"
> +OBJECT_DECLARE_SIMPLE_TYPE(SpaprWatchdog, SPAPR_WDT)
> +
> +typedef struct SpaprWatchdog {
> +    /*< private >*/
> +    DeviceState parent_obj;
> +    /*< public >*/
> +
> +    unsigned num;
> +    QEMUTimer timer;
> +    uint8_t action;
> +} SpaprWatchdog;
> +
>   /**
>    * SpaprMachineState:
>    */
> @@ -264,6 +283,8 @@ struct SpaprMachineState {
>       uint32_t FORM2_assoc_array[NUMA_NODES_MAX_NUM][FORM2_NUMA_ASSOC_SIZE];
>   
>       Error *fwnmi_migration_blocker;
> +
> +    SpaprWatchdog wds[WDT_MAX_WATCHDOGS];
>   };
>   
>   #define H_SUCCESS         0
> @@ -344,6 +365,7 @@ struct SpaprMachineState {
>   #define H_P7              -60
>   #define H_P8              -61
>   #define H_P9              -62
> +#define H_NOOP            -63
>   #define H_UNSUPPORTED     -67
>   #define H_OVERLAP         -68
>   #define H_UNSUPPORTED_FLAG -256
> @@ -564,8 +586,9 @@ struct SpaprMachineState {
>   #define H_SCM_HEALTH            0x400
>   #define H_RPT_INVALIDATE        0x448
>   #define H_SCM_FLUSH             0x44C
> +#define H_WATCHDOG              0x45C
>   
> -#define MAX_HCALL_OPCODE        H_SCM_FLUSH
> +#define MAX_HCALL_OPCODE        H_WATCHDOG
>   
>   /* The hcalls above are standardized in PAPR and implemented by pHyp
>    * as well.
> @@ -1027,6 +1050,7 @@ extern const VMStateDescription vmstate_spapr_cap_large_decr;
>   extern const VMStateDescription vmstate_spapr_cap_ccf_assist;
>   extern const VMStateDescription vmstate_spapr_cap_fwnmi;
>   extern const VMStateDescription vmstate_spapr_cap_rpt_invalidate;
> +extern const VMStateDescription vmstate_spapr_wdt;
>   
>   static inline uint8_t spapr_get_cap(SpaprMachineState *spapr, int cap)
>   {
> @@ -1063,4 +1087,7 @@ target_ulong spapr_vof_client_architecture_support(MachineState *ms,
>                                                      target_ulong ovec_addr);
>   void spapr_vof_client_dt_finalize(SpaprMachineState *spapr, void *fdt);
>   
> +/* H_WATCHDOG */
> +void spapr_watchdog_init(SpaprMachineState *spapr);
> +
>   #endif /* HW_SPAPR_H */
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index fd4942e8813c..9a5382d5270f 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -898,6 +898,8 @@ static void spapr_dt_rtas(SpaprMachineState *spapr, void *fdt)
>           add_str(hypertas, "hcall-hpt-resize");
>       }
>   
> +    add_str(hypertas, "hcall-watchdog");
> +
>       _FDT(fdt_setprop(fdt, rtas, "ibm,hypertas-functions",
>                        hypertas->str, hypertas->len));
>       g_string_free(hypertas, TRUE);
> @@ -3051,6 +3053,8 @@ static void spapr_machine_init(MachineState *machine)
>           spapr->vof->fw_size = fw_size; /* for claim() on itself */
>           spapr_register_hypercall(KVMPPC_H_VOF_CLIENT, spapr_h_vof_client);
>       }
> +
> +    spapr_watchdog_init(spapr);
>   }
>   
>   #define DEFAULT_KVM_TYPE "auto"
> diff --git a/hw/watchdog/spapr_watchdog.c b/hw/watchdog/spapr_watchdog.c
> new file mode 100644
> index 000000000000..aeaf7c52cbad
> --- /dev/null
> +++ b/hw/watchdog/spapr_watchdog.c
> @@ -0,0 +1,248 @@
> +/*
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "target/ppc/cpu.h"
> +#include "migration/vmstate.h"
> +#include "trace.h"
> +
> +#include "hw/ppc/spapr.h"
> +
> +/*
> + * Bits 47: "leaveOtherWatchdogsRunningOnTimeout", specified on
> + * the "Start watchdog" operation,
> + * 0 - stop out-standing watchdogs on timeout,
> + * 1 - leave outstanding watchdogs running on timeout
> + */
> +#define PSERIES_WDTF_LEAVE_OTHER    PPC_BIT(47)
> +
> +/*    Bits 48-55: "operation" */
> +#define PSERIES_WDTF_OP(op)             SETFIELD(PPC_BITMASK(48, 55), 0, (op))
> +#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" */
> +#define PSERIES_WDTF_ACTION(ac)         SETFIELD(PPC_BITMASK(56, 63), 0, (ac))
> +#define PSERIES_WDTF_ACTION_HARD_POWER_OFF  PSERIES_WDTF_ACTION(0x1)
> +#define PSERIES_WDTF_ACTION_HARD_RESTART    PSERIES_WDTF_ACTION(0x2)
> +#define PSERIES_WDTF_ACTION_DUMP_RESTART    PSERIES_WDTF_ACTION(0x3)
> +#define PSERIES_WDTF_RESERVED           PPC_BITMASK(0, 46)
> +
> +/*
> + * For the "Query watchdog capabilities" operation, a uint64 structure
> + * defined as:
> + * Bits 0-15: The minimum supported timeout in milliseconds
> + * Bits 16-31: The number of watchdogs supported
> + * Bits 32-63: Reserved
> + */
> +#define PSERIES_WDTQ_MIN_TIMEOUT(ms)    SETFIELD(PPC_BITMASK(0, 15), 0, (ms))
> +#define PSERIES_WDTQ_NUM(n)             SETFIELD(PPC_BITMASK(16, 31), 0, (n))
> +
> +/*
> + * For the "Query watchdog LPM requirement" operation:
> + * 1 = The given "watchdogNumber" must be stopped prior to suspending
> + * 2 = The given "watchdogNumber" does not have to be stopped prior to
> + * suspending
> + */
> +#define PSERIES_WDTQL_STOPPED               1
> +#define PSERIES_WDTQL_QUERY_NOT_STOPPED     2
> +
> +#define WDT_MIN_TIMEOUT 1 /* 1ms */
> +
> +static void watchdog_expired(void *pw)
> +{
> +    struct SpaprWatchdog *w = pw;
> +    CPUState *cs;
> +
> +    trace_spapr_watchdog_expired(w->num, w->action);
> +    switch (w->action) {
> +    case WDT_HARD_POWER_OFF:
> +        qemu_system_vmstop_request(RUN_STATE_SHUTDOWN);
> +        break;
> +    case WDT_HARD_RESTART:
> +        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> +        break;
> +    case WDT_DUMP_RESTART:
> +        CPU_FOREACH(cs) {
> +            async_run_on_cpu(cs, spapr_do_system_reset_on_cpu, RUN_ON_CPU_NULL);
> +        }
> +        break;
> +    }
> +}
> +
> +static target_ulong watchdog_stop(unsigned watchdogNumber,
> +                                  struct SpaprWatchdog *w)
> +{
> +    target_ulong ret = H_NOOP;
> +
> +    if (timer_pending(&w->timer)) {
> +        timer_del(&w->timer);
> +        ret = H_SUCCESS;
> +    }
> +    trace_spapr_watchdog_stop(watchdogNumber, ret);
> +
> +    return ret;
> +}
> +
> +static target_ulong h_watchdog(PowerPCCPU *cpu,
> +                               SpaprMachineState *spapr,
> +                               target_ulong opcode, target_ulong *args)
> +{
> +    target_ulong flags = args[0];
> +    target_ulong watchdogNumber = args[1];
> +    target_ulong timeoutInMs = args[2];
> +    unsigned operation = flags & PSERIES_WDTF_OP(~0);
> +    unsigned timeoutAction = flags & PSERIES_WDTF_ACTION(~0);
> +    struct SpaprWatchdog *w;
> +
> +    if (flags & PSERIES_WDTF_RESERVED) {
> +        return H_PARAMETER;
> +    }
> +
> +    switch (operation) {
> +    case PSERIES_WDTF_OP_START:
> +        if (watchdogNumber > ARRAY_SIZE(spapr->wds)) {
> +            return H_P2;
> +        }
> +        if (timeoutInMs <= WDT_MIN_TIMEOUT) {
> +            return H_P3;
> +        }
> +
> +        w = &spapr->wds[watchdogNumber - 1];
> +        switch (timeoutAction) {
> +        case PSERIES_WDTF_ACTION_HARD_POWER_OFF:
> +            w->action = WDT_HARD_POWER_OFF;
> +            break;
> +        case PSERIES_WDTF_ACTION_HARD_RESTART:
> +            w->action = WDT_HARD_RESTART;
> +            break;
> +        case PSERIES_WDTF_ACTION_DUMP_RESTART:
> +            w->action = WDT_DUMP_RESTART;
> +            break;
> +        default:
> +            return H_PARAMETER;
> +        }
> +        timer_mod(&w->timer,
> +                  qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + timeoutInMs);
> +        trace_spapr_watchdog_start(flags, watchdogNumber, timeoutInMs);
> +        break;
> +    case PSERIES_WDTF_OP_STOP:
> +        if (watchdogNumber == (uint64_t) ~0) {
> +            int i;
> +
> +            for (i = 1; i <= ARRAY_SIZE(spapr->wds); ++i) {
> +                watchdog_stop(i, &spapr->wds[i - 1]);
> +            }
> +        } else if (watchdogNumber <= ARRAY_SIZE(spapr->wds)) {
> +            watchdog_stop(watchdogNumber, &spapr->wds[watchdogNumber - 1]);
> +        } else {
> +            return H_P2;
> +        }
> +        break;
> +    case PSERIES_WDTF_OP_QUERY:
> +        args[0] = PSERIES_WDTQ_MIN_TIMEOUT(WDT_MIN_TIMEOUT) |
> +            PSERIES_WDTQ_NUM(ARRAY_SIZE(spapr->wds));
> +        trace_spapr_watchdog_query(args[0]);
> +        break;
> +    case PSERIES_WDTF_OP_QUERY_LPM:
> +        if (watchdogNumber > ARRAY_SIZE(spapr->wds)) {
> +            return H_P2;
> +        }
> +        args[0] = PSERIES_WDTQL_QUERY_NOT_STOPPED;
> +        trace_spapr_watchdog_query_lpm(args[0]);
> +        break;
> +    default:
> +        return H_PARAMETER;
> +    }
> +
> +    return H_SUCCESS;
> +}
> +
> +void spapr_watchdog_init(SpaprMachineState *spapr)
> +{
> +    int i;
> +
> +    for (i = 0; i < ARRAY_SIZE(spapr->wds); ++i) {
> +        char name[16];
> +        SpaprWatchdog *w = &spapr->wds[i];
> +
> +        w->num = i + 1;
> +        snprintf(name, sizeof(name) - 1, "wdt%d", i + 1);
> +        object_initialize_child_with_props(OBJECT(spapr), name, w,
> +                                           sizeof(SpaprWatchdog),
> +                                           TYPE_SPAPR_WDT,
> +                                           &error_fatal, NULL);
> +        qdev_realize(DEVICE(w), NULL, &error_fatal);
> +    }
> +}
> +
> +static bool watchdog_needed(void *opaque)
> +{
> +    SpaprWatchdog *w = opaque;
> +
> +    return timer_pending(&w->timer);
> +}
> +
> +static const VMStateDescription vmstate_wdt = {
> +    .name = "spapr_watchdog",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = watchdog_needed,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT8(action, SpaprWatchdog),
> +        VMSTATE_TIMER(timer, SpaprWatchdog),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static void spapr_wdt_realize(DeviceState *dev, Error **errp)
> +{
> +    SpaprWatchdog *w = SPAPR_WDT(dev);
> +
> +    timer_init_ms(&w->timer, QEMU_CLOCK_VIRTUAL, watchdog_expired, w);
> +
> +    object_property_add_uint64_ptr(OBJECT(dev), "expire",
> +                                   (uint64_t *)&w->timer.expire_time,
> +                                   OBJ_PROP_FLAG_READ);
> +    object_property_add_uint8_ptr(OBJECT(dev), "action", &w->action,
> +                                  OBJ_PROP_FLAG_READ);
> +}
> +
> +static void spapr_wdt_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +
> +    dc->realize = spapr_wdt_realize;
> +    dc->vmsd = &vmstate_wdt;
> +    dc->user_creatable = false;
> +}
> +
> +static const TypeInfo spapr_wdt_info = {
> +    .name          = TYPE_SPAPR_WDT,
> +    .parent        = TYPE_DEVICE,
> +    .instance_size = sizeof(SpaprWatchdog),
> +    .class_init    = spapr_wdt_class_init,
> +};
> +
> +static void spapr_watchdog_register_types(void)
> +{
> +    spapr_register_hypercall(H_WATCHDOG, h_watchdog);
> +    type_register_static(&spapr_wdt_info);
> +}
> +
> +type_init(spapr_watchdog_register_types)
> diff --git a/hw/watchdog/meson.build b/hw/watchdog/meson.build
> index 054c403dea7c..8974b5cf4c8a 100644
> --- a/hw/watchdog/meson.build
> +++ b/hw/watchdog/meson.build
> @@ -6,3 +6,4 @@ softmmu_ss.add(when: 'CONFIG_WDT_DIAG288', if_true: files('wdt_diag288.c'))
>   softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files('wdt_aspeed.c'))
>   softmmu_ss.add(when: 'CONFIG_WDT_IMX2', if_true: files('wdt_imx2.c'))
>   softmmu_ss.add(when: 'CONFIG_WDT_SBSA', if_true: files('sbsa_gwdt.c'))
> +specific_ss.add(when: 'CONFIG_PSERIES', if_true: files('spapr_watchdog.c'))
> diff --git a/hw/watchdog/trace-events b/hw/watchdog/trace-events
> index e7523e22aaf2..89ccbcfdfd20 100644
> --- a/hw/watchdog/trace-events
> +++ b/hw/watchdog/trace-events
> @@ -9,3 +9,10 @@ cmsdk_apb_watchdog_lock(uint32_t lock) "CMSDK APB watchdog: lock %" PRIu32
>   # wdt-aspeed.c
>   aspeed_wdt_read(uint64_t addr, uint32_t size) "@0x%" PRIx64 " size=%d"
>   aspeed_wdt_write(uint64_t addr, uint32_t size, uint64_t data) "@0x%" PRIx64 " size=%d value=0x%"PRIx64
> +
> +# spapr_watchdog.c
> +spapr_watchdog_start(uint64_t flags, uint64_t num, uint64_t timeout) "Flags 0x%" PRIx64 " num=%" PRId64 " %" PRIu64 "ms"
> +spapr_watchdog_stop(uint64_t num, uint64_t ret) "num=%" PRIu64 " ret=%" PRId64
> +spapr_watchdog_query(uint64_t caps) "caps=0x%" PRIx64
> +spapr_watchdog_query_lpm(uint64_t caps) "caps=0x%" PRIx64
> +spapr_watchdog_expired(uint64_t num, unsigned action) "num=%" PRIu64 " action=%u"

Re: [PATCH qemu v2 2/2] ppc/spapr: Implement H_WATCHDOG

[Cédric Le Goater]

On 6/17/22 08:07, Alexey Kardashevskiy wrote:
> The new PAPR 2.12 defines a watchdog facility managed via the new
> H_WATCHDOG hypercall.
> 
> This adds H_WATCHDOG support which a proposed driver for pseries uses:
> https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=303120
> 
> This was tested by running QEMU with a debug kernel and command line:
> -append \
>   "pseries-wdt.timeout=60 pseries-wdt.nowayout=1 pseries-wdt.action=2"
> 
> and running "echo V > /dev/watchdog0" inside the VM.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> Changes:
> v2:
> * QOM'ed timers, "action" and "expire" are available via QMP
> * removed @timeout from SpaprWatchdog
> * moved the driver to hw/watchdog
> * fixed error handling in the hcall handler
> * used new SETFIELD/GETFIELD
> ---
>   include/hw/ppc/spapr.h       |  29 +++-
>   hw/ppc/spapr.c               |   4 +
>   hw/watchdog/spapr_watchdog.c | 248 +++++++++++++++++++++++++++++++++++
>   hw/watchdog/meson.build      |   1 +
>   hw/watchdog/trace-events     |   7 +
>   5 files changed, 288 insertions(+), 1 deletion(-)
>   create mode 100644 hw/watchdog/spapr_watchdog.c
> 
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 072dda2c7265..ef1e38abd5c7 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -164,6 +164,25 @@ struct SpaprMachineClass {
>       SpaprIrq *irq;
>   };
>   
> +#define WDT_MAX_WATCHDOGS       4      /* Maximum number of watchdog devices */
> +
> +#define WDT_HARD_POWER_OFF      0
> +#define WDT_HARD_RESTART        1
> +#define WDT_DUMP_RESTART        2
> +
> +#define TYPE_SPAPR_WDT "spapr-wdt"
> +OBJECT_DECLARE_SIMPLE_TYPE(SpaprWatchdog, SPAPR_WDT)
> +
> +typedef struct SpaprWatchdog {
> +    /*< private >*/
> +    DeviceState parent_obj;
> +    /*< public >*/
> +
> +    unsigned num;

uint8_t should be enough no ? I see num is only used for trace events.

> +    QEMUTimer timer;
> +    uint8_t action;
> +} SpaprWatchdog;
> +
>   /**
>    * SpaprMachineState:
>    */
> @@ -264,6 +283,8 @@ struct SpaprMachineState {
>       uint32_t FORM2_assoc_array[NUMA_NODES_MAX_NUM][FORM2_NUMA_ASSOC_SIZE];
>   
>       Error *fwnmi_migration_blocker;
> +
> +    SpaprWatchdog wds[WDT_MAX_WATCHDOGS];
>   };
>   
>   #define H_SUCCESS         0
> @@ -344,6 +365,7 @@ struct SpaprMachineState {
>   #define H_P7              -60
>   #define H_P8              -61
>   #define H_P9              -62
> +#define H_NOOP            -63
>   #define H_UNSUPPORTED     -67
>   #define H_OVERLAP         -68
>   #define H_UNSUPPORTED_FLAG -256
> @@ -564,8 +586,9 @@ struct SpaprMachineState {
>   #define H_SCM_HEALTH            0x400
>   #define H_RPT_INVALIDATE        0x448
>   #define H_SCM_FLUSH             0x44C
> +#define H_WATCHDOG              0x45C
>   
> -#define MAX_HCALL_OPCODE        H_SCM_FLUSH
> +#define MAX_HCALL_OPCODE        H_WATCHDOG
>   
>   /* The hcalls above are standardized in PAPR and implemented by pHyp
>    * as well.
> @@ -1027,6 +1050,7 @@ extern const VMStateDescription vmstate_spapr_cap_large_decr;
>   extern const VMStateDescription vmstate_spapr_cap_ccf_assist;
>   extern const VMStateDescription vmstate_spapr_cap_fwnmi;
>   extern const VMStateDescription vmstate_spapr_cap_rpt_invalidate;
> +extern const VMStateDescription vmstate_spapr_wdt;
>   
>   static inline uint8_t spapr_get_cap(SpaprMachineState *spapr, int cap)
>   {
> @@ -1063,4 +1087,7 @@ target_ulong spapr_vof_client_architecture_support(MachineState *ms,
>                                                      target_ulong ovec_addr);
>   void spapr_vof_client_dt_finalize(SpaprMachineState *spapr, void *fdt);
>   
> +/* H_WATCHDOG */
> +void spapr_watchdog_init(SpaprMachineState *spapr);
> +
>   #endif /* HW_SPAPR_H */
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index fd4942e8813c..9a5382d5270f 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -898,6 +898,8 @@ static void spapr_dt_rtas(SpaprMachineState *spapr, void *fdt)
>           add_str(hypertas, "hcall-hpt-resize");
>       }
>   
> +    add_str(hypertas, "hcall-watchdog");
> +
>       _FDT(fdt_setprop(fdt, rtas, "ibm,hypertas-functions",
>                        hypertas->str, hypertas->len));
>       g_string_free(hypertas, TRUE);
> @@ -3051,6 +3053,8 @@ static void spapr_machine_init(MachineState *machine)
>           spapr->vof->fw_size = fw_size; /* for claim() on itself */
>           spapr_register_hypercall(KVMPPC_H_VOF_CLIENT, spapr_h_vof_client);
>       }
> +
> +    spapr_watchdog_init(spapr);
>   }
>   
>   #define DEFAULT_KVM_TYPE "auto"
> diff --git a/hw/watchdog/spapr_watchdog.c b/hw/watchdog/spapr_watchdog.c
> new file mode 100644
> index 000000000000..aeaf7c52cbad
> --- /dev/null
> +++ b/hw/watchdog/spapr_watchdog.c
> @@ -0,0 +1,248 @@
> +/*
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "target/ppc/cpu.h"
> +#include "migration/vmstate.h"
> +#include "trace.h"
> +
> +#include "hw/ppc/spapr.h"
> +
> +/*
> + * Bits 47: "leaveOtherWatchdogsRunningOnTimeout", specified on
> + * the "Start watchdog" operation,
> + * 0 - stop out-standing watchdogs on timeout,
> + * 1 - leave outstanding watchdogs running on timeout
> + */
> +#define PSERIES_WDTF_LEAVE_OTHER    PPC_BIT(47)
> +
> +/*    Bits 48-55: "operation" */
> +#define PSERIES_WDTF_OP(op)             SETFIELD(PPC_BITMASK(48, 55), 0, (op))
> +#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" */
> +#define PSERIES_WDTF_ACTION(ac)         SETFIELD(PPC_BITMASK(56, 63), 0, (ac))
> +#define PSERIES_WDTF_ACTION_HARD_POWER_OFF  PSERIES_WDTF_ACTION(0x1)
> +#define PSERIES_WDTF_ACTION_HARD_RESTART    PSERIES_WDTF_ACTION(0x2)
> +#define PSERIES_WDTF_ACTION_DUMP_RESTART    PSERIES_WDTF_ACTION(0x3)
> +#define PSERIES_WDTF_RESERVED           PPC_BITMASK(0, 46)
> +
> +/*
> + * For the "Query watchdog capabilities" operation, a uint64 structure
> + * defined as:
> + * Bits 0-15: The minimum supported timeout in milliseconds
> + * Bits 16-31: The number of watchdogs supported
> + * Bits 32-63: Reserved
> + */
> +#define PSERIES_WDTQ_MIN_TIMEOUT(ms)    SETFIELD(PPC_BITMASK(0, 15), 0, (ms))
> +#define PSERIES_WDTQ_NUM(n)             SETFIELD(PPC_BITMASK(16, 31), 0, (n))
> +
> +/*
> + * For the "Query watchdog LPM requirement" operation:
> + * 1 = The given "watchdogNumber" must be stopped prior to suspending
> + * 2 = The given "watchdogNumber" does not have to be stopped prior to
> + * suspending
> + */
> +#define PSERIES_WDTQL_STOPPED               1
> +#define PSERIES_WDTQL_QUERY_NOT_STOPPED     2
> +
> +#define WDT_MIN_TIMEOUT 1 /* 1ms */
> +
> +static void watchdog_expired(void *pw)
> +{
> +    struct SpaprWatchdog *w = pw;

s/struct//

> +    CPUState *cs;
> +
> +    trace_spapr_watchdog_expired(w->num, w->action);
> +    switch (w->action) {
> +    case WDT_HARD_POWER_OFF:
> +        qemu_system_vmstop_request(RUN_STATE_SHUTDOWN);
> +        break;
> +    case WDT_HARD_RESTART:
> +        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> +        break;
> +    case WDT_DUMP_RESTART:
> +        CPU_FOREACH(cs) {
> +            async_run_on_cpu(cs, spapr_do_system_reset_on_cpu, RUN_ON_CPU_NULL);
> +        }
> +        break;
> +    }
> +}
> +
> +static target_ulong watchdog_stop(unsigned watchdogNumber,
> +                                  struct SpaprWatchdog *w)
> +{
> +    target_ulong ret = H_NOOP;
> +
> +    if (timer_pending(&w->timer)) {
> +        timer_del(&w->timer);
> +        ret = H_SUCCESS;
> +    }
> +    trace_spapr_watchdog_stop(watchdogNumber, ret);
> +
> +    return ret;
> +}
> +
> +static target_ulong h_watchdog(PowerPCCPU *cpu,
> +                               SpaprMachineState *spapr,
> +                               target_ulong opcode, target_ulong *args)
> +{
> +    target_ulong flags = args[0];
> +    target_ulong watchdogNumber = args[1];
> +    target_ulong timeoutInMs = args[2];
> +    unsigned operation = flags & PSERIES_WDTF_OP(~0);
> +    unsigned timeoutAction = flags & PSERIES_WDTF_ACTION(~0);
> +    struct SpaprWatchdog *w;
> +
> +    if (flags & PSERIES_WDTF_RESERVED) {
> +        return H_PARAMETER;
> +    }
> +
> +    switch (operation) {
> +    case PSERIES_WDTF_OP_START:
> +        if (watchdogNumber > ARRAY_SIZE(spapr->wds)) {
> +            return H_P2;
> +        }
> +        if (timeoutInMs <= WDT_MIN_TIMEOUT) {
> +            return H_P3;
> +        }
> +
> +        w = &spapr->wds[watchdogNumber - 1];

So first index is 1 in PAPR ...

> +        switch (timeoutAction) {
> +        case PSERIES_WDTF_ACTION_HARD_POWER_OFF:
> +            w->action = WDT_HARD_POWER_OFF;
> +            break;
> +        case PSERIES_WDTF_ACTION_HARD_RESTART:
> +            w->action = WDT_HARD_RESTART;
> +            break;
> +        case PSERIES_WDTF_ACTION_DUMP_RESTART:
> +            w->action = WDT_DUMP_RESTART;
> +            break;
> +        default:
> +            return H_PARAMETER;
> +        }
> +        timer_mod(&w->timer,
> +                  qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + timeoutInMs);
> +        trace_spapr_watchdog_start(flags, watchdogNumber, timeoutInMs);
> +        break;
> +    case PSERIES_WDTF_OP_STOP:
> +        if (watchdogNumber == (uint64_t) ~0) {

May be add a define for this special value. It's better for readability.

> +            int i;
> +
> +            for (i = 1; i <= ARRAY_SIZE(spapr->wds); ++i) {
> +                watchdog_stop(i, &spapr->wds[i - 1]);
> +            }
> +        } else if (watchdogNumber <= ARRAY_SIZE(spapr->wds)) {
> +            watchdog_stop(watchdogNumber, &spapr->wds[watchdogNumber - 1]);
> +        } else {
> +            return H_P2;
> +        }
> +        break;
> +    case PSERIES_WDTF_OP_QUERY:
> +        args[0] = PSERIES_WDTQ_MIN_TIMEOUT(WDT_MIN_TIMEOUT) |
> +            PSERIES_WDTQ_NUM(ARRAY_SIZE(spapr->wds));
> +        trace_spapr_watchdog_query(args[0]);
> +        break;
> +    case PSERIES_WDTF_OP_QUERY_LPM:
> +        if (watchdogNumber > ARRAY_SIZE(spapr->wds)) {
> +            return H_P2;
> +        }
> +        args[0] = PSERIES_WDTQL_QUERY_NOT_STOPPED;
> +        trace_spapr_watchdog_query_lpm(args[0]);
> +        break;
> +    default:
> +        return H_PARAMETER;
> +    }
> +
> +    return H_SUCCESS;
> +}
> +
> +void spapr_watchdog_init(SpaprMachineState *spapr)

This could have a 'Error **errp' parameter.

> +{
> +    int i;
> +
> +    for (i = 0; i < ARRAY_SIZE(spapr->wds); ++i) {
> +        char name[16];g_autofree char *name = g_strdup_printf("wdt%d", i + 1);

> +        SpaprWatchdog *w = &spapr->wds[i];
> +
> +        w->num = i + 1;

it should be a property.

Thanks,

C.

> +        snprintf(name, sizeof(name) - 1, "wdt%d", i + 1);
> +        object_initialize_child_with_props(OBJECT(spapr), name, w,
> +                                           sizeof(SpaprWatchdog),
> +                                           TYPE_SPAPR_WDT,
> +                                           &error_fatal, NULL);
> +        qdev_realize(DEVICE(w), NULL, &error_fatal);
> +    }
> +}
> +
> +static bool watchdog_needed(void *opaque)
> +{
> +    SpaprWatchdog *w = opaque;
> +
> +    return timer_pending(&w->timer);
> +}
> +
> +static const VMStateDescription vmstate_wdt = {
> +    .name = "spapr_watchdog",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = watchdog_needed,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT8(action, SpaprWatchdog),
> +        VMSTATE_TIMER(timer, SpaprWatchdog),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static void spapr_wdt_realize(DeviceState *dev, Error **errp)
> +{
> +    SpaprWatchdog *w = SPAPR_WDT(dev);
> +
> +    timer_init_ms(&w->timer, QEMU_CLOCK_VIRTUAL, watchdog_expired, w);
> +
> +    object_property_add_uint64_ptr(OBJECT(dev), "expire",
> +                                   (uint64_t *)&w->timer.expire_time,
> +                                   OBJ_PROP_FLAG_READ);
> +    object_property_add_uint8_ptr(OBJECT(dev), "action", &w->action,
> +                                  OBJ_PROP_FLAG_READ);
> +}
> +
> +static void spapr_wdt_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +
> +    dc->renualize = spapr_wdt_realize;
> +    dc->vmsd = &vmstate_wdt;
> +    dc->user_creatable = false;
> +}
> +
> +static const TypeInfo spapr_wdt_info = {
> +    .name          = TYPE_SPAPR_WDT,
> +    .parent        = TYPE_DEVICE,
> +    .instance_size = sizeof(SpaprWatchdog),
> +    .class_init    = spapr_wdt_class_init,
> +};
> +
> +static void spapr_watchdog_register_types(void)
> +{
> +    spapr_register_hypercall(H_WATCHDOG, h_watchdog);
> +    type_register_static(&spapr_wdt_info);
> +}
> +
> +type_init(spapr_watchdog_register_types)
> diff --git a/hw/watchdog/meson.build b/hw/watchdog/meson.build
> index 054c403dea7c..8974b5cf4c8a 100644
> --- a/hw/watchdog/meson.build
> +++ b/hw/watchdog/meson.build
> @@ -6,3 +6,4 @@ softmmu_ss.add(when: 'CONFIG_WDT_DIAG288', if_true: files('wdt_diag288.c'))
>   softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files('wdt_aspeed.c'))
>   softmmu_ss.add(when: 'CONFIG_WDT_IMX2', if_true: files('wdt_imx2.c'))
>   softmmu_ss.add(when: 'CONFIG_WDT_SBSA', if_true: files('sbsa_gwdt.c'))
> +specific_ss.add(when: 'CONFIG_PSERIES', if_true: files('spapr_watchdog.c'))
> diff --git a/hw/watchdog/trace-events b/hw/watchdog/trace-events
> index e7523e22aaf2..89ccbcfdfd20 100644
> --- a/hw/watchdog/trace-events
> +++ b/hw/watchdog/trace-events
> @@ -9,3 +9,10 @@ cmsdk_apb_watchdog_lock(uint32_t lock) "CMSDK APB watchdog: lock %" PRIu32
>   # wdt-aspeed.c
>   aspeed_wdt_read(uint64_t addr, uint32_t size) "@0x%" PRIx64 " size=%d"
>   aspeed_wdt_write(uint64_t addr, uint32_t size, uint64_t data) "@0x%" PRIx64 " size=%d value=0x%"PRIx64
> +
> +# spapr_watchdog.c
> +spapr_watchdog_start(uint64_t flags, uint64_t num, uint64_t timeout) "Flags 0x%" PRIx64 " num=%" PRId64 " %" PRIu64 "ms"
> +spapr_watchdog_stop(uint64_t num, uint64_t ret) "num=%" PRIu64 " ret=%" PRId64
> +spapr_watchdog_query(uint64_t caps) "caps=0x%" PRIx64
> +spapr_watchdog_query_lpm(uint64_t caps) "caps=0x%" PRIx64
> +spapr_watchdog_expired(uint64_t num, unsigned action) "num=%" PRIu64 " action=%u"

Re: [PATCH qemu v2 2/2] ppc/spapr: Implement H_WATCHDOG

[Alexey Kardashevskiy]

On 6/18/22 21:01, Cédric Le Goater wrote:
> On 6/17/22 08:07, Alexey Kardashevskiy wrote:
>> The new PAPR 2.12 defines a watchdog facility managed via the new
>> H_WATCHDOG hypercall.
>>
>> This adds H_WATCHDOG support which a proposed driver for pseries uses:
>> https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=303120
>>
>> This was tested by running QEMU with a debug kernel and command line:
>> -append \
>>   "pseries-wdt.timeout=60 pseries-wdt.nowayout=1 pseries-wdt.action=2"
>>
>> and running "echo V > /dev/watchdog0" inside the VM.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>> Changes:
>> v2:
>> * QOM'ed timers, "action" and "expire" are available via QMP
>> * removed @timeout from SpaprWatchdog
>> * moved the driver to hw/watchdog
>> * fixed error handling in the hcall handler
>> * used new SETFIELD/GETFIELD
>> ---
>>   include/hw/ppc/spapr.h       |  29 +++-
>>   hw/ppc/spapr.c               |   4 +
>>   hw/watchdog/spapr_watchdog.c | 248 +++++++++++++++++++++++++++++++++++
>>   hw/watchdog/meson.build      |   1 +
>>   hw/watchdog/trace-events     |   7 +
>>   5 files changed, 288 insertions(+), 1 deletion(-)
>>   create mode 100644 hw/watchdog/spapr_watchdog.c
>>
>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>> index 072dda2c7265..ef1e38abd5c7 100644
>> --- a/include/hw/ppc/spapr.h
>> +++ b/include/hw/ppc/spapr.h
>> @@ -164,6 +164,25 @@ struct SpaprMachineClass {
>>       SpaprIrq *irq;
>>   };
>> +#define WDT_MAX_WATCHDOGS       4      /* Maximum number of watchdog 
>> devices */
>> +
>> +#define WDT_HARD_POWER_OFF      0
>> +#define WDT_HARD_RESTART        1
>> +#define WDT_DUMP_RESTART        2
>> +
>> +#define TYPE_SPAPR_WDT "spapr-wdt"
>> +OBJECT_DECLARE_SIMPLE_TYPE(SpaprWatchdog, SPAPR_WDT)
>> +
>> +typedef struct SpaprWatchdog {
>> +    /*< private >*/
>> +    DeviceState parent_obj;
>> +    /*< public >*/
>> +
>> +    unsigned num;
> 
> uint8_t should be enough no ? I see num is only used for trace events.


It should but why? It is not migrating, and using uint8_t creates 
alignment gap here, and no benefit :) And I am removing it anyway, see 
below.

> 
>> +    QEMUTimer timer;
>> +    uint8_t action;
>> +} SpaprWatchdog;
>> +
>>   /**
>>    * SpaprMachineState:
>>    */
>> @@ -264,6 +283,8 @@ struct SpaprMachineState {
>>       uint32_t 
>> FORM2_assoc_array[NUMA_NODES_MAX_NUM][FORM2_NUMA_ASSOC_SIZE];
>>       Error *fwnmi_migration_blocker;
>> +
>> +    SpaprWatchdog wds[WDT_MAX_WATCHDOGS];
>>   };
>>   #define H_SUCCESS         0
>> @@ -344,6 +365,7 @@ struct SpaprMachineState {
>>   #define H_P7              -60
>>   #define H_P8              -61
>>   #define H_P9              -62
>> +#define H_NOOP            -63
>>   #define H_UNSUPPORTED     -67
>>   #define H_OVERLAP         -68
>>   #define H_UNSUPPORTED_FLAG -256
>> @@ -564,8 +586,9 @@ struct SpaprMachineState {
>>   #define H_SCM_HEALTH            0x400
>>   #define H_RPT_INVALIDATE        0x448
>>   #define H_SCM_FLUSH             0x44C
>> +#define H_WATCHDOG              0x45C
>> -#define MAX_HCALL_OPCODE        H_SCM_FLUSH
>> +#define MAX_HCALL_OPCODE        H_WATCHDOG
>>   /* The hcalls above are standardized in PAPR and implemented by pHyp
>>    * as well.
>> @@ -1027,6 +1050,7 @@ extern const VMStateDescription 
>> vmstate_spapr_cap_large_decr;
>>   extern const VMStateDescription vmstate_spapr_cap_ccf_assist;
>>   extern const VMStateDescription vmstate_spapr_cap_fwnmi;
>>   extern const VMStateDescription vmstate_spapr_cap_rpt_invalidate;
>> +extern const VMStateDescription vmstate_spapr_wdt;
>>   static inline uint8_t spapr_get_cap(SpaprMachineState *spapr, int cap)
>>   {
>> @@ -1063,4 +1087,7 @@ target_ulong 
>> spapr_vof_client_architecture_support(MachineState *ms,
>>                                                      target_ulong 
>> ovec_addr);
>>   void spapr_vof_client_dt_finalize(SpaprMachineState *spapr, void *fdt);
>> +/* H_WATCHDOG */
>> +void spapr_watchdog_init(SpaprMachineState *spapr);
>> +
>>   #endif /* HW_SPAPR_H */
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index fd4942e8813c..9a5382d5270f 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -898,6 +898,8 @@ static void spapr_dt_rtas(SpaprMachineState 
>> *spapr, void *fdt)
>>           add_str(hypertas, "hcall-hpt-resize");
>>       }
>> +    add_str(hypertas, "hcall-watchdog");
>> +
>>       _FDT(fdt_setprop(fdt, rtas, "ibm,hypertas-functions",
>>                        hypertas->str, hypertas->len));
>>       g_string_free(hypertas, TRUE);
>> @@ -3051,6 +3053,8 @@ static void spapr_machine_init(MachineState 
>> *machine)
>>           spapr->vof->fw_size = fw_size; /* for claim() on itself */
>>           spapr_register_hypercall(KVMPPC_H_VOF_CLIENT, 
>> spapr_h_vof_client);
>>       }
>> +
>> +    spapr_watchdog_init(spapr);
>>   }
>>   #define DEFAULT_KVM_TYPE "auto"
>> diff --git a/hw/watchdog/spapr_watchdog.c b/hw/watchdog/spapr_watchdog.c
>> new file mode 100644
>> index 000000000000..aeaf7c52cbad
>> --- /dev/null
>> +++ b/hw/watchdog/spapr_watchdog.c
>> @@ -0,0 +1,248 @@
>> +/*
>> + * This library is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU Lesser General Public
>> + * License as published by the Free Software Foundation; either
>> + * version 2.1 of the License, or (at your option) any later version.
>> + *
>> + * This library is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * Lesser General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU Lesser General Public
>> + * License along with this library; if not, see 
>> <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qapi/error.h"
>> +#include "target/ppc/cpu.h"
>> +#include "migration/vmstate.h"
>> +#include "trace.h"
>> +
>> +#include "hw/ppc/spapr.h"
>> +
>> +/*
>> + * Bits 47: "leaveOtherWatchdogsRunningOnTimeout", specified on
>> + * the "Start watchdog" operation,
>> + * 0 - stop out-standing watchdogs on timeout,
>> + * 1 - leave outstanding watchdogs running on timeout
>> + */
>> +#define PSERIES_WDTF_LEAVE_OTHER    PPC_BIT(47)
>> +
>> +/*    Bits 48-55: "operation" */
>> +#define PSERIES_WDTF_OP(op)             SETFIELD(PPC_BITMASK(48, 55), 
>> 0, (op))
>> +#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" */
>> +#define PSERIES_WDTF_ACTION(ac)         SETFIELD(PPC_BITMASK(56, 63), 
>> 0, (ac))
>> +#define PSERIES_WDTF_ACTION_HARD_POWER_OFF  PSERIES_WDTF_ACTION(0x1)
>> +#define PSERIES_WDTF_ACTION_HARD_RESTART    PSERIES_WDTF_ACTION(0x2)
>> +#define PSERIES_WDTF_ACTION_DUMP_RESTART    PSERIES_WDTF_ACTION(0x3)
>> +#define PSERIES_WDTF_RESERVED           PPC_BITMASK(0, 46)
>> +
>> +/*
>> + * For the "Query watchdog capabilities" operation, a uint64 structure
>> + * defined as:
>> + * Bits 0-15: The minimum supported timeout in milliseconds
>> + * Bits 16-31: The number of watchdogs supported
>> + * Bits 32-63: Reserved
>> + */
>> +#define PSERIES_WDTQ_MIN_TIMEOUT(ms)    SETFIELD(PPC_BITMASK(0, 15), 
>> 0, (ms))
>> +#define PSERIES_WDTQ_NUM(n)             SETFIELD(PPC_BITMASK(16, 31), 
>> 0, (n))
>> +
>> +/*
>> + * For the "Query watchdog LPM requirement" operation:
>> + * 1 = The given "watchdogNumber" must be stopped prior to suspending
>> + * 2 = The given "watchdogNumber" does not have to be stopped prior to
>> + * suspending
>> + */
>> +#define PSERIES_WDTQL_STOPPED               1
>> +#define PSERIES_WDTQL_QUERY_NOT_STOPPED     2
>> +
>> +#define WDT_MIN_TIMEOUT 1 /* 1ms */
>> +
>> +static void watchdog_expired(void *pw)
>> +{
>> +    struct SpaprWatchdog *w = pw;
> 
> s/struct//
> 
>> +    CPUState *cs;
>> +
>> +    trace_spapr_watchdog_expired(w->num, w->action);
>> +    switch (w->action) {
>> +    case WDT_HARD_POWER_OFF:
>> +        qemu_system_vmstop_request(RUN_STATE_SHUTDOWN);
>> +        break;
>> +    case WDT_HARD_RESTART:
>> +        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
>> +        break;
>> +    case WDT_DUMP_RESTART:
>> +        CPU_FOREACH(cs) {
>> +            async_run_on_cpu(cs, spapr_do_system_reset_on_cpu, 
>> RUN_ON_CPU_NULL);
>> +        }
>> +        break;
>> +    }
>> +}
>> +
>> +static target_ulong watchdog_stop(unsigned watchdogNumber,
>> +                                  struct SpaprWatchdog *w)
>> +{
>> +    target_ulong ret = H_NOOP;
>> +
>> +    if (timer_pending(&w->timer)) {
>> +        timer_del(&w->timer);
>> +        ret = H_SUCCESS;
>> +    }
>> +    trace_spapr_watchdog_stop(watchdogNumber, ret);
>> +
>> +    return ret;
>> +}
>> +
>> +static target_ulong h_watchdog(PowerPCCPU *cpu,
>> +                               SpaprMachineState *spapr,
>> +                               target_ulong opcode, target_ulong *args)
>> +{
>> +    target_ulong flags = args[0];
>> +    target_ulong watchdogNumber = args[1];
>> +    target_ulong timeoutInMs = args[2];
>> +    unsigned operation = flags & PSERIES_WDTF_OP(~0);
>> +    unsigned timeoutAction = flags & PSERIES_WDTF_ACTION(~0);
>> +    struct SpaprWatchdog *w;
>> +
>> +    if (flags & PSERIES_WDTF_RESERVED) {
>> +        return H_PARAMETER;
>> +    }
>> +
>> +    switch (operation) {
>> +    case PSERIES_WDTF_OP_START:
>> +        if (watchdogNumber > ARRAY_SIZE(spapr->wds)) {
>> +            return H_P2;
>> +        }
>> +        if (timeoutInMs <= WDT_MIN_TIMEOUT) {
>> +            return H_P3;
>> +        }
>> +
>> +        w = &spapr->wds[watchdogNumber - 1];
> 
> So first index is 1 in PAPR ...


Yes, I thought I commented on this somewhere but lost in rebases.


> 
>> +        switch (timeoutAction) {
>> +        case PSERIES_WDTF_ACTION_HARD_POWER_OFF:
>> +            w->action = WDT_HARD_POWER_OFF;
>> +            break;
>> +        case PSERIES_WDTF_ACTION_HARD_RESTART:
>> +            w->action = WDT_HARD_RESTART;
>> +            break;
>> +        case PSERIES_WDTF_ACTION_DUMP_RESTART:
>> +            w->action = WDT_DUMP_RESTART;
>> +            break;
>> +        default:
>> +            return H_PARAMETER;
>> +        }
>> +        timer_mod(&w->timer,
>> +                  qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + timeoutInMs);
>> +        trace_spapr_watchdog_start(flags, watchdogNumber, timeoutInMs);
>> +        break;
>> +    case PSERIES_WDTF_OP_STOP:
>> +        if (watchdogNumber == (uint64_t) ~0) {
> 
> May be add a define for this special value. It's better for readability.

Will do.


>> +            int i;
>> +
>> +            for (i = 1; i <= ARRAY_SIZE(spapr->wds); ++i) {
>> +                watchdog_stop(i, &spapr->wds[i - 1]);
>> +            }
>> +        } else if (watchdogNumber <= ARRAY_SIZE(spapr->wds)) {
>> +            watchdog_stop(watchdogNumber, &spapr->wds[watchdogNumber 
>> - 1]);
>> +        } else {
>> +            return H_P2;
>> +        }
>> +        break;
>> +    case PSERIES_WDTF_OP_QUERY:
>> +        args[0] = PSERIES_WDTQ_MIN_TIMEOUT(WDT_MIN_TIMEOUT) |
>> +            PSERIES_WDTQ_NUM(ARRAY_SIZE(spapr->wds));
>> +        trace_spapr_watchdog_query(args[0]);
>> +        break;
>> +    case PSERIES_WDTF_OP_QUERY_LPM:
>> +        if (watchdogNumber > ARRAY_SIZE(spapr->wds)) {
>> +            return H_P2;
>> +        }
>> +        args[0] = PSERIES_WDTQL_QUERY_NOT_STOPPED;
>> +        trace_spapr_watchdog_query_lpm(args[0]);
>> +        break;
>> +    default:
>> +        return H_PARAMETER;
>> +    }
>> +
>> +    return H_SUCCESS;
>> +}
>> +
>> +void spapr_watchdog_init(SpaprMachineState *spapr)
> 
> This could have a 'Error **errp' parameter.


I was repeating somewhat similar spapr_rtc_create(), and the called - 
spapr_machine_init() - does not have *errp. Seems pointless as it fails 
- something is horrendously broken.


> 
>> +{
>> +    int i;
>> +
>> +    for (i = 0; i < ARRAY_SIZE(spapr->wds); ++i) {
>> +        char name[16];g_autofree char *name = 
>> g_strdup_printf("wdt%d", i + 1);
> 
>> +        SpaprWatchdog *w = &spapr->wds[i];
>> +
>> +        w->num = i + 1;
> 
> it should be a property.

This cannot change and used only for tracing, and the QOM name has the 
number as well. I am replacing it with

SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
unsigned num = w - spapr->wds;

and removing the @num from the struct.

Thanks,

> 
> Thanks,
> 
> C.
> 
>> +        snprintf(name, sizeof(name) - 1, "wdt%d", i + 1);
>> +        object_initialize_child_with_props(OBJECT(spapr), name, w,
>> +                                           sizeof(SpaprWatchdog),
>> +                                           TYPE_SPAPR_WDT,
>> +                                           &error_fatal, NULL);
>> +        qdev_realize(DEVICE(w), NULL, &error_fatal);
>> +    }
>> +}
>> +
>> +static bool watchdog_needed(void *opaque)
>> +{
>> +    SpaprWatchdog *w = opaque;
>> +
>> +    return timer_pending(&w->timer);
>> +}
>> +
>> +static const VMStateDescription vmstate_wdt = {
>> +    .name = "spapr_watchdog",
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .needed = watchdog_needed,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_UINT8(action, SpaprWatchdog),
>> +        VMSTATE_TIMER(timer, SpaprWatchdog),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>> +static void spapr_wdt_realize(DeviceState *dev, Error **errp)
>> +{
>> +    SpaprWatchdog *w = SPAPR_WDT(dev);
>> +
>> +    timer_init_ms(&w->timer, QEMU_CLOCK_VIRTUAL, watchdog_expired, w);
>> +
>> +    object_property_add_uint64_ptr(OBJECT(dev), "expire",
>> +                                   (uint64_t *)&w->timer.expire_time,
>> +                                   OBJ_PROP_FLAG_READ);
>> +    object_property_add_uint8_ptr(OBJECT(dev), "action", &w->action,
>> +                                  OBJ_PROP_FLAG_READ);
>> +}
>> +
>> +static void spapr_wdt_class_init(ObjectClass *oc, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(oc);
>> +
>> +    dc->renualize = spapr_wdt_realize;
>> +    dc->vmsd = &vmstate_wdt;
>> +    dc->user_creatable = false;
>> +}
>> +
>> +static const TypeInfo spapr_wdt_info = {
>> +    .name          = TYPE_SPAPR_WDT,
>> +    .parent        = TYPE_DEVICE,
>> +    .instance_size = sizeof(SpaprWatchdog),
>> +    .class_init    = spapr_wdt_class_init,
>> +};
>> +
>> +static void spapr_watchdog_register_types(void)
>> +{
>> +    spapr_register_hypercall(H_WATCHDOG, h_watchdog);
>> +    type_register_static(&spapr_wdt_info);
>> +}
>> +
>> +type_init(spapr_watchdog_register_types)
>> diff --git a/hw/watchdog/meson.build b/hw/watchdog/meson.build
>> index 054c403dea7c..8974b5cf4c8a 100644
>> --- a/hw/watchdog/meson.build
>> +++ b/hw/watchdog/meson.build
>> @@ -6,3 +6,4 @@ softmmu_ss.add(when: 'CONFIG_WDT_DIAG288', if_true: 
>> files('wdt_diag288.c'))
>>   softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: 
>> files('wdt_aspeed.c'))
>>   softmmu_ss.add(when: 'CONFIG_WDT_IMX2', if_true: files('wdt_imx2.c'))
>>   softmmu_ss.add(when: 'CONFIG_WDT_SBSA', if_true: files('sbsa_gwdt.c'))
>> +specific_ss.add(when: 'CONFIG_PSERIES', if_true: 
>> files('spapr_watchdog.c'))
>> diff --git a/hw/watchdog/trace-events b/hw/watchdog/trace-events
>> index e7523e22aaf2..89ccbcfdfd20 100644
>> --- a/hw/watchdog/trace-events
>> +++ b/hw/watchdog/trace-events
>> @@ -9,3 +9,10 @@ cmsdk_apb_watchdog_lock(uint32_t lock) "CMSDK APB 
>> watchdog: lock %" PRIu32
>>   # wdt-aspeed.c
>>   aspeed_wdt_read(uint64_t addr, uint32_t size) "@0x%" PRIx64 " size=%d"
>>   aspeed_wdt_write(uint64_t addr, uint32_t size, uint64_t data) "@0x%" 
>> PRIx64 " size=%d value=0x%"PRIx64
>> +
>> +# spapr_watchdog.c
>> +spapr_watchdog_start(uint64_t flags, uint64_t num, uint64_t timeout) 
>> "Flags 0x%" PRIx64 " num=%" PRId64 " %" PRIu64 "ms"
>> +spapr_watchdog_stop(uint64_t num, uint64_t ret) "num=%" PRIu64 " 
>> ret=%" PRId64
>> +spapr_watchdog_query(uint64_t caps) "caps=0x%" PRIx64
>> +spapr_watchdog_query_lpm(uint64_t caps) "caps=0x%" PRIx64
>> +spapr_watchdog_expired(uint64_t num, unsigned action) "num=%" PRIu64 
>> " action=%u"

-- 
Alexey

Re: [PATCH qemu v2 2/2] ppc/spapr: Implement H_WATCHDOG

[Cédric Le Goater]

On 6/20/22 05:13, Alexey Kardashevskiy wrote:
> 
> 
> On 6/18/22 21:01, Cédric Le Goater wrote:
>> On 6/17/22 08:07, Alexey Kardashevskiy wrote:
>>> The new PAPR 2.12 defines a watchdog facility managed via the new
>>> H_WATCHDOG hypercall.
>>>
>>> This adds H_WATCHDOG support which a proposed driver for pseries uses:
>>> https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=303120
>>>
>>> This was tested by running QEMU with a debug kernel and command line:
>>> -append \
>>>   "pseries-wdt.timeout=60 pseries-wdt.nowayout=1 pseries-wdt.action=2"
>>>
>>> and running "echo V > /dev/watchdog0" inside the VM.
>>>
>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>> ---
>>> Changes:
>>> v2:
>>> * QOM'ed timers, "action" and "expire" are available via QMP
>>> * removed @timeout from SpaprWatchdog
>>> * moved the driver to hw/watchdog
>>> * fixed error handling in the hcall handler
>>> * used new SETFIELD/GETFIELD
>>> ---
>>>   include/hw/ppc/spapr.h       |  29 +++-
>>>   hw/ppc/spapr.c               |   4 +
>>>   hw/watchdog/spapr_watchdog.c | 248 +++++++++++++++++++++++++++++++++++
>>>   hw/watchdog/meson.build      |   1 +
>>>   hw/watchdog/trace-events     |   7 +
>>>   5 files changed, 288 insertions(+), 1 deletion(-)
>>>   create mode 100644 hw/watchdog/spapr_watchdog.c
>>>
>>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>>> index 072dda2c7265..ef1e38abd5c7 100644
>>> --- a/include/hw/ppc/spapr.h
>>> +++ b/include/hw/ppc/spapr.h
>>> @@ -164,6 +164,25 @@ struct SpaprMachineClass {
>>>       SpaprIrq *irq;
>>>   };
>>> +#define WDT_MAX_WATCHDOGS       4      /* Maximum number of watchdog devices */
>>> +
>>> +#define WDT_HARD_POWER_OFF      0
>>> +#define WDT_HARD_RESTART        1
>>> +#define WDT_DUMP_RESTART        2
>>> +
>>> +#define TYPE_SPAPR_WDT "spapr-wdt"
>>> +OBJECT_DECLARE_SIMPLE_TYPE(SpaprWatchdog, SPAPR_WDT)
>>> +
>>> +typedef struct SpaprWatchdog {
>>> +    /*< private >*/
>>> +    DeviceState parent_obj;
>>> +    /*< public >*/
>>> +
>>> +    unsigned num;
>>
>> uint8_t should be enough no ? I see num is only used for trace events.
> 
> 
> It should but why? It is not migrating, and using uint8_t creates alignment gap here, and no benefit :) And I am removing it anyway, see below.
> 
>>
>>> +    QEMUTimer timer;
>>> +    uint8_t action;
>>> +} SpaprWatchdog;
>>> +
>>>   /**
>>>    * SpaprMachineState:
>>>    */
>>> @@ -264,6 +283,8 @@ struct SpaprMachineState {
>>>       uint32_t FORM2_assoc_array[NUMA_NODES_MAX_NUM][FORM2_NUMA_ASSOC_SIZE];
>>>       Error *fwnmi_migration_blocker;
>>> +
>>> +    SpaprWatchdog wds[WDT_MAX_WATCHDOGS];
>>>   };
>>>   #define H_SUCCESS         0
>>> @@ -344,6 +365,7 @@ struct SpaprMachineState {
>>>   #define H_P7              -60
>>>   #define H_P8              -61
>>>   #define H_P9              -62
>>> +#define H_NOOP            -63
>>>   #define H_UNSUPPORTED     -67
>>>   #define H_OVERLAP         -68
>>>   #define H_UNSUPPORTED_FLAG -256
>>> @@ -564,8 +586,9 @@ struct SpaprMachineState {
>>>   #define H_SCM_HEALTH            0x400
>>>   #define H_RPT_INVALIDATE        0x448
>>>   #define H_SCM_FLUSH             0x44C
>>> +#define H_WATCHDOG              0x45C
>>> -#define MAX_HCALL_OPCODE        H_SCM_FLUSH
>>> +#define MAX_HCALL_OPCODE        H_WATCHDOG
>>>   /* The hcalls above are standardized in PAPR and implemented by pHyp
>>>    * as well.
>>> @@ -1027,6 +1050,7 @@ extern const VMStateDescription vmstate_spapr_cap_large_decr;
>>>   extern const VMStateDescription vmstate_spapr_cap_ccf_assist;
>>>   extern const VMStateDescription vmstate_spapr_cap_fwnmi;
>>>   extern const VMStateDescription vmstate_spapr_cap_rpt_invalidate;
>>> +extern const VMStateDescription vmstate_spapr_wdt;
>>>   static inline uint8_t spapr_get_cap(SpaprMachineState *spapr, int cap)
>>>   {
>>> @@ -1063,4 +1087,7 @@ target_ulong spapr_vof_client_architecture_support(MachineState *ms,
>>>                                                      target_ulong ovec_addr);
>>>   void spapr_vof_client_dt_finalize(SpaprMachineState *spapr, void *fdt);
>>> +/* H_WATCHDOG */
>>> +void spapr_watchdog_init(SpaprMachineState *spapr);
>>> +
>>>   #endif /* HW_SPAPR_H */
>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>> index fd4942e8813c..9a5382d5270f 100644
>>> --- a/hw/ppc/spapr.c
>>> +++ b/hw/ppc/spapr.c
>>> @@ -898,6 +898,8 @@ static void spapr_dt_rtas(SpaprMachineState *spapr, void *fdt)
>>>           add_str(hypertas, "hcall-hpt-resize");
>>>       }
>>> +    add_str(hypertas, "hcall-watchdog");
>>> +
>>>       _FDT(fdt_setprop(fdt, rtas, "ibm,hypertas-functions",
>>>                        hypertas->str, hypertas->len));
>>>       g_string_free(hypertas, TRUE);
>>> @@ -3051,6 +3053,8 @@ static void spapr_machine_init(MachineState *machine)
>>>           spapr->vof->fw_size = fw_size; /* for claim() on itself */
>>>           spapr_register_hypercall(KVMPPC_H_VOF_CLIENT, spapr_h_vof_client);
>>>       }
>>> +
>>> +    spapr_watchdog_init(spapr);
>>>   }
>>>   #define DEFAULT_KVM_TYPE "auto"
>>> diff --git a/hw/watchdog/spapr_watchdog.c b/hw/watchdog/spapr_watchdog.c
>>> new file mode 100644
>>> index 000000000000..aeaf7c52cbad
>>> --- /dev/null
>>> +++ b/hw/watchdog/spapr_watchdog.c
>>> @@ -0,0 +1,248 @@
>>> +/*
>>> + * This library is free software; you can redistribute it and/or
>>> + * modify it under the terms of the GNU Lesser General Public
>>> + * License as published by the Free Software Foundation; either
>>> + * version 2.1 of the License, or (at your option) any later version.
>>> + *
>>> + * This library is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>>> + * Lesser General Public License for more details.
>>> + *
>>> + * You should have received a copy of the GNU Lesser General Public
>>> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
>>> + */
>>> +
>>> +#include "qemu/osdep.h"
>>> +#include "qapi/error.h"
>>> +#include "target/ppc/cpu.h"
>>> +#include "migration/vmstate.h"
>>> +#include "trace.h"
>>> +
>>> +#include "hw/ppc/spapr.h"
>>> +
>>> +/*
>>> + * Bits 47: "leaveOtherWatchdogsRunningOnTimeout", specified on
>>> + * the "Start watchdog" operation,
>>> + * 0 - stop out-standing watchdogs on timeout,
>>> + * 1 - leave outstanding watchdogs running on timeout
>>> + */
>>> +#define PSERIES_WDTF_LEAVE_OTHER    PPC_BIT(47)
>>> +
>>> +/*    Bits 48-55: "operation" */
>>> +#define PSERIES_WDTF_OP(op)             SETFIELD(PPC_BITMASK(48, 55), 0, (op))
>>> +#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" */
>>> +#define PSERIES_WDTF_ACTION(ac)         SETFIELD(PPC_BITMASK(56, 63), 0, (ac))
>>> +#define PSERIES_WDTF_ACTION_HARD_POWER_OFF  PSERIES_WDTF_ACTION(0x1)
>>> +#define PSERIES_WDTF_ACTION_HARD_RESTART    PSERIES_WDTF_ACTION(0x2)
>>> +#define PSERIES_WDTF_ACTION_DUMP_RESTART    PSERIES_WDTF_ACTION(0x3)
>>> +#define PSERIES_WDTF_RESERVED           PPC_BITMASK(0, 46)
>>> +
>>> +/*
>>> + * For the "Query watchdog capabilities" operation, a uint64 structure
>>> + * defined as:
>>> + * Bits 0-15: The minimum supported timeout in milliseconds
>>> + * Bits 16-31: The number of watchdogs supported
>>> + * Bits 32-63: Reserved
>>> + */
>>> +#define PSERIES_WDTQ_MIN_TIMEOUT(ms)    SETFIELD(PPC_BITMASK(0, 15), 0, (ms))
>>> +#define PSERIES_WDTQ_NUM(n)             SETFIELD(PPC_BITMASK(16, 31), 0, (n))
>>> +
>>> +/*
>>> + * For the "Query watchdog LPM requirement" operation:
>>> + * 1 = The given "watchdogNumber" must be stopped prior to suspending
>>> + * 2 = The given "watchdogNumber" does not have to be stopped prior to
>>> + * suspending
>>> + */
>>> +#define PSERIES_WDTQL_STOPPED               1
>>> +#define PSERIES_WDTQL_QUERY_NOT_STOPPED     2
>>> +
>>> +#define WDT_MIN_TIMEOUT 1 /* 1ms */
>>> +
>>> +static void watchdog_expired(void *pw)
>>> +{
>>> +    struct SpaprWatchdog *w = pw;
>>
>> s/struct//
>>
>>> +    CPUState *cs;
>>> +
>>> +    trace_spapr_watchdog_expired(w->num, w->action);
>>> +    switch (w->action) {
>>> +    case WDT_HARD_POWER_OFF:
>>> +        qemu_system_vmstop_request(RUN_STATE_SHUTDOWN);
>>> +        break;
>>> +    case WDT_HARD_RESTART:
>>> +        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
>>> +        break;
>>> +    case WDT_DUMP_RESTART:
>>> +        CPU_FOREACH(cs) {
>>> +            async_run_on_cpu(cs, spapr_do_system_reset_on_cpu, RUN_ON_CPU_NULL);
>>> +        }
>>> +        break;
>>> +    }
>>> +}
>>> +
>>> +static target_ulong watchdog_stop(unsigned watchdogNumber,
>>> +                                  struct SpaprWatchdog *w)
>>> +{
>>> +    target_ulong ret = H_NOOP;
>>> +
>>> +    if (timer_pending(&w->timer)) {
>>> +        timer_del(&w->timer);
>>> +        ret = H_SUCCESS;
>>> +    }
>>> +    trace_spapr_watchdog_stop(watchdogNumber, ret);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static target_ulong h_watchdog(PowerPCCPU *cpu,
>>> +                               SpaprMachineState *spapr,
>>> +                               target_ulong opcode, target_ulong *args)
>>> +{
>>> +    target_ulong flags = args[0];
>>> +    target_ulong watchdogNumber = args[1];
>>> +    target_ulong timeoutInMs = args[2];
>>> +    unsigned operation = flags & PSERIES_WDTF_OP(~0);
>>> +    unsigned timeoutAction = flags & PSERIES_WDTF_ACTION(~0);
>>> +    struct SpaprWatchdog *w;
>>> +
>>> +    if (flags & PSERIES_WDTF_RESERVED) {
>>> +        return H_PARAMETER;
>>> +    }
>>> +
>>> +    switch (operation) {
>>> +    case PSERIES_WDTF_OP_START:
>>> +        if (watchdogNumber > ARRAY_SIZE(spapr->wds)) {
>>> +            return H_P2;
>>> +        }
>>> +        if (timeoutInMs <= WDT_MIN_TIMEOUT) {
>>> +            return H_P3;
>>> +        }
>>> +
>>> +        w = &spapr->wds[watchdogNumber - 1];
>>
>> So first index is 1 in PAPR ...
> 
> 
> Yes, I thought I commented on this somewhere but lost in rebases.
> 
> 
>>
>>> +        switch (timeoutAction) {
>>> +        case PSERIES_WDTF_ACTION_HARD_POWER_OFF:
>>> +            w->action = WDT_HARD_POWER_OFF;
>>> +            break;
>>> +        case PSERIES_WDTF_ACTION_HARD_RESTART:
>>> +            w->action = WDT_HARD_RESTART;
>>> +            break;
>>> +        case PSERIES_WDTF_ACTION_DUMP_RESTART:
>>> +            w->action = WDT_DUMP_RESTART;
>>> +            break;
>>> +        default:
>>> +            return H_PARAMETER;
>>> +        }
>>> +        timer_mod(&w->timer,
>>> +                  qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + timeoutInMs);
>>> +        trace_spapr_watchdog_start(flags, watchdogNumber, timeoutInMs);
>>> +        break;
>>> +    case PSERIES_WDTF_OP_STOP:
>>> +        if (watchdogNumber == (uint64_t) ~0) {
>>
>> May be add a define for this special value. It's better for readability.
> 
> Will do.
> 
> 
>>> +            int i;
>>> +
>>> +            for (i = 1; i <= ARRAY_SIZE(spapr->wds); ++i) {
>>> +                watchdog_stop(i, &spapr->wds[i - 1]);
>>> +            }
>>> +        } else if (watchdogNumber <= ARRAY_SIZE(spapr->wds)) {
>>> +            watchdog_stop(watchdogNumber, &spapr->wds[watchdogNumber - 1]);
>>> +        } else {
>>> +            return H_P2;
>>> +        }
>>> +        break;
>>> +    case PSERIES_WDTF_OP_QUERY:
>>> +        args[0] = PSERIES_WDTQ_MIN_TIMEOUT(WDT_MIN_TIMEOUT) |
>>> +            PSERIES_WDTQ_NUM(ARRAY_SIZE(spapr->wds));
>>> +        trace_spapr_watchdog_query(args[0]);
>>> +        break;
>>> +    case PSERIES_WDTF_OP_QUERY_LPM:
>>> +        if (watchdogNumber > ARRAY_SIZE(spapr->wds)) {
>>> +            return H_P2;
>>> +        }
>>> +        args[0] = PSERIES_WDTQL_QUERY_NOT_STOPPED;
>>> +        trace_spapr_watchdog_query_lpm(args[0]);
>>> +        break;
>>> +    default:
>>> +        return H_PARAMETER;
>>> +    }
>>> +
>>> +    return H_SUCCESS;
>>> +}
>>> +
>>> +void spapr_watchdog_init(SpaprMachineState *spapr)
>>
>> This could have a 'Error **errp' parameter.
> 
> 
> I was repeating somewhat similar spapr_rtc_create(), and the called - spapr_machine_init() - does not have *errp. Seems pointless as it fails - something is horrendously broken.


Well, it's up to the caller to decide what to do in case of
errors. If in this case, it is "log an error and exit",
I would simply :

   spapr_watchdog_init(spapr, &error_fatal);

But it is not necessarily fatal to fail to initialize some
device.

> 
> 
>>
>>> +{
>>> +    int i;
>>> +
>>> +    for (i = 0; i < ARRAY_SIZE(spapr->wds); ++i) {
>>> +        char name[16];g_autofree char *name = g_strdup_printf("wdt%d", i + 1);
>>
>>> +        SpaprWatchdog *w = &spapr->wds[i];
>>> +
>>> +        w->num = i + 1;
>>
>> it should be a property.
> 
> This cannot change and used only for tracing, and the QOM name has the number as well. I am replacing it with
> 
> SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> unsigned num = w - spapr->wds;
> 
> and removing the @num from the struct.

Nice !

Thanks

C.


> 
> Thanks,
> 
>>
>> Thanks,
>>
>> C.
>>
>>> +        snprintf(name, sizeof(name) - 1, "wdt%d", i + 1);
>>> +        object_initialize_child_with_props(OBJECT(spapr), name, w,
>>> +                                           sizeof(SpaprWatchdog),
>>> +                                           TYPE_SPAPR_WDT,
>>> +                                           &error_fatal, NULL);
>>> +        qdev_realize(DEVICE(w), NULL, &error_fatal);
>>> +    }
>>> +}
>>> +
>>> +static bool watchdog_needed(void *opaque)
>>> +{
>>> +    SpaprWatchdog *w = opaque;
>>> +
>>> +    return timer_pending(&w->timer);
>>> +}
>>> +
>>> +static const VMStateDescription vmstate_wdt = {
>>> +    .name = "spapr_watchdog",
>>> +    .version_id = 1,
>>> +    .minimum_version_id = 1,
>>> +    .needed = watchdog_needed,
>>> +    .fields = (VMStateField[]) {
>>> +        VMSTATE_UINT8(action, SpaprWatchdog),
>>> +        VMSTATE_TIMER(timer, SpaprWatchdog),
>>> +        VMSTATE_END_OF_LIST()
>>> +    }
>>> +};
>>> +
>>> +static void spapr_wdt_realize(DeviceState *dev, Error **errp)
>>> +{
>>> +    SpaprWatchdog *w = SPAPR_WDT(dev);
>>> +
>>> +    timer_init_ms(&w->timer, QEMU_CLOCK_VIRTUAL, watchdog_expired, w);
>>> +
>>> +    object_property_add_uint64_ptr(OBJECT(dev), "expire",
>>> +                                   (uint64_t *)&w->timer.expire_time,
>>> +                                   OBJ_PROP_FLAG_READ);
>>> +    object_property_add_uint8_ptr(OBJECT(dev), "action", &w->action,
>>> +                                  OBJ_PROP_FLAG_READ);
>>> +}
>>> +
>>> +static void spapr_wdt_class_init(ObjectClass *oc, void *data)
>>> +{
>>> +    DeviceClass *dc = DEVICE_CLASS(oc);
>>> +
>>> +    dc->renualize = spapr_wdt_realize;
>>> +    dc->vmsd = &vmstate_wdt;
>>> +    dc->user_creatable = false;
>>> +}
>>> +
>>> +static const TypeInfo spapr_wdt_info = {
>>> +    .name          = TYPE_SPAPR_WDT,
>>> +    .parent        = TYPE_DEVICE,
>>> +    .instance_size = sizeof(SpaprWatchdog),
>>> +    .class_init    = spapr_wdt_class_init,
>>> +};
>>> +
>>> +static void spapr_watchdog_register_types(void)
>>> +{
>>> +    spapr_register_hypercall(H_WATCHDOG, h_watchdog);
>>> +    type_register_static(&spapr_wdt_info);
>>> +}
>>> +
>>> +type_init(spapr_watchdog_register_types)
>>> diff --git a/hw/watchdog/meson.build b/hw/watchdog/meson.build
>>> index 054c403dea7c..8974b5cf4c8a 100644
>>> --- a/hw/watchdog/meson.build
>>> +++ b/hw/watchdog/meson.build
>>> @@ -6,3 +6,4 @@ softmmu_ss.add(when: 'CONFIG_WDT_DIAG288', if_true: files('wdt_diag288.c'))
>>>   softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files('wdt_aspeed.c'))
>>>   softmmu_ss.add(when: 'CONFIG_WDT_IMX2', if_true: files('wdt_imx2.c'))
>>>   softmmu_ss.add(when: 'CONFIG_WDT_SBSA', if_true: files('sbsa_gwdt.c'))
>>> +specific_ss.add(when: 'CONFIG_PSERIES', if_true: files('spapr_watchdog.c'))
>>> diff --git a/hw/watchdog/trace-events b/hw/watchdog/trace-events
>>> index e7523e22aaf2..89ccbcfdfd20 100644
>>> --- a/hw/watchdog/trace-events
>>> +++ b/hw/watchdog/trace-events
>>> @@ -9,3 +9,10 @@ cmsdk_apb_watchdog_lock(uint32_t lock) "CMSDK APB watchdog: lock %" PRIu32
>>>   # wdt-aspeed.c
>>>   aspeed_wdt_read(uint64_t addr, uint32_t size) "@0x%" PRIx64 " size=%d"
>>>   aspeed_wdt_write(uint64_t addr, uint32_t size, uint64_t data) "@0x%" PRIx64 " size=%d value=0x%"PRIx64
>>> +
>>> +# spapr_watchdog.c
>>> +spapr_watchdog_start(uint64_t flags, uint64_t num, uint64_t timeout) "Flags 0x%" PRIx64 " num=%" PRId64 " %" PRIu64 "ms"
>>> +spapr_watchdog_stop(uint64_t num, uint64_t ret) "num=%" PRIu64 " ret=%" PRId64
>>> +spapr_watchdog_query(uint64_t caps) "caps=0x%" PRIx64
>>> +spapr_watchdog_query_lpm(uint64_t caps) "caps=0x%" PRIx64
>>> +spapr_watchdog_expired(uint64_t num, unsigned action) "num=%" PRIu64 " action=%u"
> 

Re: [PATCH qemu v2 2/2] ppc/spapr: Implement H_WATCHDOG

[Alexey Kardashevskiy]

On 6/20/22 16:23, Cédric Le Goater wrote:
> On 6/20/22 05:13, Alexey Kardashevskiy wrote:
>>
>>
>> On 6/18/22 21:01, Cédric Le Goater wrote:
>>> On 6/17/22 08:07, Alexey Kardashevskiy wrote:
>>>> The new PAPR 2.12 defines a watchdog facility managed via the new
>>>> H_WATCHDOG hypercall.
>>>>
>>>> This adds H_WATCHDOG support which a proposed driver for pseries uses:
>>>> https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=303120
>>>>
>>>> This was tested by running QEMU with a debug kernel and command line:
>>>> -append \
>>>>   "pseries-wdt.timeout=60 pseries-wdt.nowayout=1 pseries-wdt.action=2"
>>>>
>>>> and running "echo V > /dev/watchdog0" inside the VM.
>>>>
>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>> ---
>>>> Changes:
>>>> v2:
>>>> * QOM'ed timers, "action" and "expire" are available via QMP
>>>> * removed @timeout from SpaprWatchdog
>>>> * moved the driver to hw/watchdog
>>>> * fixed error handling in the hcall handler
>>>> * used new SETFIELD/GETFIELD
>>>> ---
>>>>   include/hw/ppc/spapr.h       |  29 +++-
>>>>   hw/ppc/spapr.c               |   4 +
>>>>   hw/watchdog/spapr_watchdog.c | 248 
>>>> +++++++++++++++++++++++++++++++++++
>>>>   hw/watchdog/meson.build      |   1 +
>>>>   hw/watchdog/trace-events     |   7 +
>>>>   5 files changed, 288 insertions(+), 1 deletion(-)
>>>>   create mode 100644 hw/watchdog/spapr_watchdog.c
>>>>
>>>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>>>> index 072dda2c7265..ef1e38abd5c7 100644
>>>> --- a/include/hw/ppc/spapr.h
>>>> +++ b/include/hw/ppc/spapr.h
>>>> @@ -164,6 +164,25 @@ struct SpaprMachineClass {
>>>>       SpaprIrq *irq;
>>>>   };
>>>> +#define WDT_MAX_WATCHDOGS       4      /* Maximum number of 
>>>> watchdog devices */
>>>> +
>>>> +#define WDT_HARD_POWER_OFF      0
>>>> +#define WDT_HARD_RESTART        1
>>>> +#define WDT_DUMP_RESTART        2
>>>> +
>>>> +#define TYPE_SPAPR_WDT "spapr-wdt"
>>>> +OBJECT_DECLARE_SIMPLE_TYPE(SpaprWatchdog, SPAPR_WDT)
>>>> +
>>>> +typedef struct SpaprWatchdog {
>>>> +    /*< private >*/
>>>> +    DeviceState parent_obj;
>>>> +    /*< public >*/
>>>> +
>>>> +    unsigned num;
>>>
>>> uint8_t should be enough no ? I see num is only used for trace events.
>>
>>
>> It should but why? It is not migrating, and using uint8_t creates 
>> alignment gap here, and no benefit :) And I am removing it anyway, see 
>> below.
>>
>>>
>>>> +    QEMUTimer timer;
>>>> +    uint8_t action;
>>>> +} SpaprWatchdog;
>>>> +
>>>>   /**
>>>>    * SpaprMachineState:
>>>>    */
>>>> @@ -264,6 +283,8 @@ struct SpaprMachineState {
>>>>       uint32_t 
>>>> FORM2_assoc_array[NUMA_NODES_MAX_NUM][FORM2_NUMA_ASSOC_SIZE];
>>>>       Error *fwnmi_migration_blocker;
>>>> +
>>>> +    SpaprWatchdog wds[WDT_MAX_WATCHDOGS];
>>>>   };
>>>>   #define H_SUCCESS         0
>>>> @@ -344,6 +365,7 @@ struct SpaprMachineState {
>>>>   #define H_P7              -60
>>>>   #define H_P8              -61
>>>>   #define H_P9              -62
>>>> +#define H_NOOP            -63
>>>>   #define H_UNSUPPORTED     -67
>>>>   #define H_OVERLAP         -68
>>>>   #define H_UNSUPPORTED_FLAG -256
>>>> @@ -564,8 +586,9 @@ struct SpaprMachineState {
>>>>   #define H_SCM_HEALTH            0x400
>>>>   #define H_RPT_INVALIDATE        0x448
>>>>   #define H_SCM_FLUSH             0x44C
>>>> +#define H_WATCHDOG              0x45C
>>>> -#define MAX_HCALL_OPCODE        H_SCM_FLUSH
>>>> +#define MAX_HCALL_OPCODE        H_WATCHDOG
>>>>   /* The hcalls above are standardized in PAPR and implemented by pHyp
>>>>    * as well.
>>>> @@ -1027,6 +1050,7 @@ extern const VMStateDescription 
>>>> vmstate_spapr_cap_large_decr;
>>>>   extern const VMStateDescription vmstate_spapr_cap_ccf_assist;
>>>>   extern const VMStateDescription vmstate_spapr_cap_fwnmi;
>>>>   extern const VMStateDescription vmstate_spapr_cap_rpt_invalidate;
>>>> +extern const VMStateDescription vmstate_spapr_wdt;
>>>>   static inline uint8_t spapr_get_cap(SpaprMachineState *spapr, int 
>>>> cap)
>>>>   {
>>>> @@ -1063,4 +1087,7 @@ target_ulong 
>>>> spapr_vof_client_architecture_support(MachineState *ms,
>>>>                                                      target_ulong 
>>>> ovec_addr);
>>>>   void spapr_vof_client_dt_finalize(SpaprMachineState *spapr, void 
>>>> *fdt);
>>>> +/* H_WATCHDOG */
>>>> +void spapr_watchdog_init(SpaprMachineState *spapr);
>>>> +
>>>>   #endif /* HW_SPAPR_H */
>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>>> index fd4942e8813c..9a5382d5270f 100644
>>>> --- a/hw/ppc/spapr.c
>>>> +++ b/hw/ppc/spapr.c
>>>> @@ -898,6 +898,8 @@ static void spapr_dt_rtas(SpaprMachineState 
>>>> *spapr, void *fdt)
>>>>           add_str(hypertas, "hcall-hpt-resize");
>>>>       }
>>>> +    add_str(hypertas, "hcall-watchdog");
>>>> +
>>>>       _FDT(fdt_setprop(fdt, rtas, "ibm,hypertas-functions",
>>>>                        hypertas->str, hypertas->len));
>>>>       g_string_free(hypertas, TRUE);
>>>> @@ -3051,6 +3053,8 @@ static void spapr_machine_init(MachineState 
>>>> *machine)
>>>>           spapr->vof->fw_size = fw_size; /* for claim() on itself */
>>>>           spapr_register_hypercall(KVMPPC_H_VOF_CLIENT, 
>>>> spapr_h_vof_client);
>>>>       }
>>>> +
>>>> +    spapr_watchdog_init(spapr);
>>>>   }
>>>>   #define DEFAULT_KVM_TYPE "auto"
>>>> diff --git a/hw/watchdog/spapr_watchdog.c 
>>>> b/hw/watchdog/spapr_watchdog.c
>>>> new file mode 100644
>>>> index 000000000000..aeaf7c52cbad
>>>> --- /dev/null
>>>> +++ b/hw/watchdog/spapr_watchdog.c
>>>> @@ -0,0 +1,248 @@
>>>> +/*
>>>> + * This library is free software; you can redistribute it and/or
>>>> + * modify it under the terms of the GNU Lesser General Public
>>>> + * License as published by the Free Software Foundation; either
>>>> + * version 2.1 of the License, or (at your option) any later version.
>>>> + *
>>>> + * This library is distributed in the hope that it will be useful,
>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>>>> + * Lesser General Public License for more details.
>>>> + *
>>>> + * You should have received a copy of the GNU Lesser General Public
>>>> + * License along with this library; if not, see 
>>>> <http://www.gnu.org/licenses/>.
>>>> + */
>>>> +
>>>> +#include "qemu/osdep.h"
>>>> +#include "qapi/error.h"
>>>> +#include "target/ppc/cpu.h"
>>>> +#include "migration/vmstate.h"
>>>> +#include "trace.h"
>>>> +
>>>> +#include "hw/ppc/spapr.h"
>>>> +
>>>> +/*
>>>> + * Bits 47: "leaveOtherWatchdogsRunningOnTimeout", specified on
>>>> + * the "Start watchdog" operation,
>>>> + * 0 - stop out-standing watchdogs on timeout,
>>>> + * 1 - leave outstanding watchdogs running on timeout
>>>> + */
>>>> +#define PSERIES_WDTF_LEAVE_OTHER    PPC_BIT(47)
>>>> +
>>>> +/*    Bits 48-55: "operation" */
>>>> +#define PSERIES_WDTF_OP(op)             SETFIELD(PPC_BITMASK(48, 
>>>> 55), 0, (op))
>>>> +#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" */
>>>> +#define PSERIES_WDTF_ACTION(ac)         SETFIELD(PPC_BITMASK(56, 
>>>> 63), 0, (ac))
>>>> +#define PSERIES_WDTF_ACTION_HARD_POWER_OFF  PSERIES_WDTF_ACTION(0x1)
>>>> +#define PSERIES_WDTF_ACTION_HARD_RESTART    PSERIES_WDTF_ACTION(0x2)
>>>> +#define PSERIES_WDTF_ACTION_DUMP_RESTART    PSERIES_WDTF_ACTION(0x3)
>>>> +#define PSERIES_WDTF_RESERVED           PPC_BITMASK(0, 46)
>>>> +
>>>> +/*
>>>> + * For the "Query watchdog capabilities" operation, a uint64 structure
>>>> + * defined as:
>>>> + * Bits 0-15: The minimum supported timeout in milliseconds
>>>> + * Bits 16-31: The number of watchdogs supported
>>>> + * Bits 32-63: Reserved
>>>> + */
>>>> +#define PSERIES_WDTQ_MIN_TIMEOUT(ms)    SETFIELD(PPC_BITMASK(0, 
>>>> 15), 0, (ms))
>>>> +#define PSERIES_WDTQ_NUM(n)             SETFIELD(PPC_BITMASK(16, 
>>>> 31), 0, (n))
>>>> +
>>>> +/*
>>>> + * For the "Query watchdog LPM requirement" operation:
>>>> + * 1 = The given "watchdogNumber" must be stopped prior to suspending
>>>> + * 2 = The given "watchdogNumber" does not have to be stopped prior to
>>>> + * suspending
>>>> + */
>>>> +#define PSERIES_WDTQL_STOPPED               1
>>>> +#define PSERIES_WDTQL_QUERY_NOT_STOPPED     2
>>>> +
>>>> +#define WDT_MIN_TIMEOUT 1 /* 1ms */
>>>> +
>>>> +static void watchdog_expired(void *pw)
>>>> +{
>>>> +    struct SpaprWatchdog *w = pw;
>>>
>>> s/struct//
>>>
>>>> +    CPUState *cs;
>>>> +
>>>> +    trace_spapr_watchdog_expired(w->num, w->action);
>>>> +    switch (w->action) {
>>>> +    case WDT_HARD_POWER_OFF:
>>>> +        qemu_system_vmstop_request(RUN_STATE_SHUTDOWN);
>>>> +        break;
>>>> +    case WDT_HARD_RESTART:
>>>> +        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
>>>> +        break;
>>>> +    case WDT_DUMP_RESTART:
>>>> +        CPU_FOREACH(cs) {
>>>> +            async_run_on_cpu(cs, spapr_do_system_reset_on_cpu, 
>>>> RUN_ON_CPU_NULL);
>>>> +        }
>>>> +        break;
>>>> +    }
>>>> +}
>>>> +
>>>> +static target_ulong watchdog_stop(unsigned watchdogNumber,
>>>> +                                  struct SpaprWatchdog *w)
>>>> +{
>>>> +    target_ulong ret = H_NOOP;
>>>> +
>>>> +    if (timer_pending(&w->timer)) {
>>>> +        timer_del(&w->timer);
>>>> +        ret = H_SUCCESS;
>>>> +    }
>>>> +    trace_spapr_watchdog_stop(watchdogNumber, ret);
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +static target_ulong h_watchdog(PowerPCCPU *cpu,
>>>> +                               SpaprMachineState *spapr,
>>>> +                               target_ulong opcode, target_ulong 
>>>> *args)
>>>> +{
>>>> +    target_ulong flags = args[0];
>>>> +    target_ulong watchdogNumber = args[1];
>>>> +    target_ulong timeoutInMs = args[2];
>>>> +    unsigned operation = flags & PSERIES_WDTF_OP(~0);
>>>> +    unsigned timeoutAction = flags & PSERIES_WDTF_ACTION(~0);
>>>> +    struct SpaprWatchdog *w;
>>>> +
>>>> +    if (flags & PSERIES_WDTF_RESERVED) {
>>>> +        return H_PARAMETER;
>>>> +    }
>>>> +
>>>> +    switch (operation) {
>>>> +    case PSERIES_WDTF_OP_START:
>>>> +        if (watchdogNumber > ARRAY_SIZE(spapr->wds)) {
>>>> +            return H_P2;
>>>> +        }
>>>> +        if (timeoutInMs <= WDT_MIN_TIMEOUT) {
>>>> +            return H_P3;
>>>> +        }
>>>> +
>>>> +        w = &spapr->wds[watchdogNumber - 1];
>>>
>>> So first index is 1 in PAPR ...
>>
>>
>> Yes, I thought I commented on this somewhere but lost in rebases.
>>
>>
>>>
>>>> +        switch (timeoutAction) {
>>>> +        case PSERIES_WDTF_ACTION_HARD_POWER_OFF:
>>>> +            w->action = WDT_HARD_POWER_OFF;
>>>> +            break;
>>>> +        case PSERIES_WDTF_ACTION_HARD_RESTART:
>>>> +            w->action = WDT_HARD_RESTART;
>>>> +            break;
>>>> +        case PSERIES_WDTF_ACTION_DUMP_RESTART:
>>>> +            w->action = WDT_DUMP_RESTART;
>>>> +            break;
>>>> +        default:
>>>> +            return H_PARAMETER;
>>>> +        }
>>>> +        timer_mod(&w->timer,
>>>> +                  qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 
>>>> timeoutInMs);
>>>> +        trace_spapr_watchdog_start(flags, watchdogNumber, 
>>>> timeoutInMs);
>>>> +        break;
>>>> +    case PSERIES_WDTF_OP_STOP:
>>>> +        if (watchdogNumber == (uint64_t) ~0) {
>>>
>>> May be add a define for this special value. It's better for readability.
>>
>> Will do.
>>
>>
>>>> +            int i;
>>>> +
>>>> +            for (i = 1; i <= ARRAY_SIZE(spapr->wds); ++i) {
>>>> +                watchdog_stop(i, &spapr->wds[i - 1]);
>>>> +            }
>>>> +        } else if (watchdogNumber <= ARRAY_SIZE(spapr->wds)) {
>>>> +            watchdog_stop(watchdogNumber, 
>>>> &spapr->wds[watchdogNumber - 1]);
>>>> +        } else {
>>>> +            return H_P2;
>>>> +        }
>>>> +        break;
>>>> +    case PSERIES_WDTF_OP_QUERY:
>>>> +        args[0] = PSERIES_WDTQ_MIN_TIMEOUT(WDT_MIN_TIMEOUT) |
>>>> +            PSERIES_WDTQ_NUM(ARRAY_SIZE(spapr->wds));
>>>> +        trace_spapr_watchdog_query(args[0]);
>>>> +        break;
>>>> +    case PSERIES_WDTF_OP_QUERY_LPM:
>>>> +        if (watchdogNumber > ARRAY_SIZE(spapr->wds)) {
>>>> +            return H_P2;
>>>> +        }
>>>> +        args[0] = PSERIES_WDTQL_QUERY_NOT_STOPPED;
>>>> +        trace_spapr_watchdog_query_lpm(args[0]);
>>>> +        break;
>>>> +    default:
>>>> +        return H_PARAMETER;
>>>> +    }
>>>> +
>>>> +    return H_SUCCESS;
>>>> +}
>>>> +
>>>> +void spapr_watchdog_init(SpaprMachineState *spapr)
>>>
>>> This could have a 'Error **errp' parameter.
>>
>>
>> I was repeating somewhat similar spapr_rtc_create(), and the called - 
>> spapr_machine_init() - does not have *errp. Seems pointless as it 
>> fails - something is horrendously broken.
> 
> 
> Well, it's up to the caller to decide what to do in case of
> errors. If in this case, it is "log an error and exit",
> I would simply :
> 
>    spapr_watchdog_init(spapr, &error_fatal);
> 
> But it is not necessarily fatal to fail to initialize some
> device.

The device is so simple and not configurable via the command line that 
it is fatal. And it does not touch *errp anyway.

Passing errp also means spapr_watchdog_init() should do clean up of 
whatever it did in a loop before hitting the error, for example in this 
case - unrealize() previously realized watchdogs. This adds useless code 
as it is going to be fatal anyway. Thanks,


> 
>>
>>
>>>
>>>> +{
>>>> +    int i;
>>>> +
>>>> +    for (i = 0; i < ARRAY_SIZE(spapr->wds); ++i) {
>>>> +        char name[16];g_autofree char *name = 
>>>> g_strdup_printf("wdt%d", i + 1);
>>>
>>>> +        SpaprWatchdog *w = &spapr->wds[i];
>>>> +
>>>> +        w->num = i + 1;
>>>
>>> it should be a property.
>>
>> This cannot change and used only for tracing, and the QOM name has the 
>> number as well. I am replacing it with
>>
>> SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>> unsigned num = w - spapr->wds;
>>
>> and removing the @num from the struct.
> 
> Nice !
> 
> Thanks
> 
> C.
> 
> 
>>
>> Thanks,
>>
>>>
>>> Thanks,
>>>
>>> C.
>>>
>>>> +        snprintf(name, sizeof(name) - 1, "wdt%d", i + 1);
>>>> +        object_initialize_child_with_props(OBJECT(spapr), name, w,
>>>> +                                           sizeof(SpaprWatchdog),
>>>> +                                           TYPE_SPAPR_WDT,
>>>> +                                           &error_fatal, NULL);
>>>> +        qdev_realize(DEVICE(w), NULL, &error_fatal);
>>>> +    }
>>>> +}
>>>> +
>>>> +static bool watchdog_needed(void *opaque)
>>>> +{
>>>> +    SpaprWatchdog *w = opaque;
>>>> +
>>>> +    return timer_pending(&w->timer);
>>>> +}
>>>> +
>>>> +static const VMStateDescription vmstate_wdt = {
>>>> +    .name = "spapr_watchdog",
>>>> +    .version_id = 1,
>>>> +    .minimum_version_id = 1,
>>>> +    .needed = watchdog_needed,
>>>> +    .fields = (VMStateField[]) {
>>>> +        VMSTATE_UINT8(action, SpaprWatchdog),
>>>> +        VMSTATE_TIMER(timer, SpaprWatchdog),
>>>> +        VMSTATE_END_OF_LIST()
>>>> +    }
>>>> +};
>>>> +
>>>> +static void spapr_wdt_realize(DeviceState *dev, Error **errp)
>>>> +{
>>>> +    SpaprWatchdog *w = SPAPR_WDT(dev);
>>>> +
>>>> +    timer_init_ms(&w->timer, QEMU_CLOCK_VIRTUAL, watchdog_expired, w);
>>>> +
>>>> +    object_property_add_uint64_ptr(OBJECT(dev), "expire",
>>>> +                                   (uint64_t *)&w->timer.expire_time,
>>>> +                                   OBJ_PROP_FLAG_READ);
>>>> +    object_property_add_uint8_ptr(OBJECT(dev), "action", &w->action,
>>>> +                                  OBJ_PROP_FLAG_READ);
>>>> +}
>>>> +
>>>> +static void spapr_wdt_class_init(ObjectClass *oc, void *data)
>>>> +{
>>>> +    DeviceClass *dc = DEVICE_CLASS(oc);
>>>> +
>>>> +    dc->renualize = spapr_wdt_realize;
>>>> +    dc->vmsd = &vmstate_wdt;
>>>> +    dc->user_creatable = false;
>>>> +}
>>>> +
>>>> +static const TypeInfo spapr_wdt_info = {
>>>> +    .name          = TYPE_SPAPR_WDT,
>>>> +    .parent        = TYPE_DEVICE,
>>>> +    .instance_size = sizeof(SpaprWatchdog),
>>>> +    .class_init    = spapr_wdt_class_init,
>>>> +};
>>>> +
>>>> +static void spapr_watchdog_register_types(void)
>>>> +{
>>>> +    spapr_register_hypercall(H_WATCHDOG, h_watchdog);
>>>> +    type_register_static(&spapr_wdt_info);
>>>> +}
>>>> +
>>>> +type_init(spapr_watchdog_register_types)
>>>> diff --git a/hw/watchdog/meson.build b/hw/watchdog/meson.build
>>>> index 054c403dea7c..8974b5cf4c8a 100644
>>>> --- a/hw/watchdog/meson.build
>>>> +++ b/hw/watchdog/meson.build
>>>> @@ -6,3 +6,4 @@ softmmu_ss.add(when: 'CONFIG_WDT_DIAG288', if_true: 
>>>> files('wdt_diag288.c'))
>>>>   softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: 
>>>> files('wdt_aspeed.c'))
>>>>   softmmu_ss.add(when: 'CONFIG_WDT_IMX2', if_true: files('wdt_imx2.c'))
>>>>   softmmu_ss.add(when: 'CONFIG_WDT_SBSA', if_true: 
>>>> files('sbsa_gwdt.c'))
>>>> +specific_ss.add(when: 'CONFIG_PSERIES', if_true: 
>>>> files('spapr_watchdog.c'))
>>>> diff --git a/hw/watchdog/trace-events b/hw/watchdog/trace-events
>>>> index e7523e22aaf2..89ccbcfdfd20 100644
>>>> --- a/hw/watchdog/trace-events
>>>> +++ b/hw/watchdog/trace-events
>>>> @@ -9,3 +9,10 @@ cmsdk_apb_watchdog_lock(uint32_t lock) "CMSDK APB 
>>>> watchdog: lock %" PRIu32
>>>>   # wdt-aspeed.c
>>>>   aspeed_wdt_read(uint64_t addr, uint32_t size) "@0x%" PRIx64 " 
>>>> size=%d"
>>>>   aspeed_wdt_write(uint64_t addr, uint32_t size, uint64_t data) 
>>>> "@0x%" PRIx64 " size=%d value=0x%"PRIx64
>>>> +
>>>> +# spapr_watchdog.c
>>>> +spapr_watchdog_start(uint64_t flags, uint64_t num, uint64_t 
>>>> timeout) "Flags 0x%" PRIx64 " num=%" PRId64 " %" PRIu64 "ms"
>>>> +spapr_watchdog_stop(uint64_t num, uint64_t ret) "num=%" PRIu64 " 
>>>> ret=%" PRId64
>>>> +spapr_watchdog_query(uint64_t caps) "caps=0x%" PRIx64
>>>> +spapr_watchdog_query_lpm(uint64_t caps) "caps=0x%" PRIx64
>>>> +spapr_watchdog_expired(uint64_t num, unsigned action) "num=%" 
>>>> PRIu64 " action=%u"
>>
> 

-- 
Alexey

Re: [PATCH qemu v2 0/2] ppc/spapr: Implement H_WATCHDOG

[Daniel Henrique Barboza]

On 6/17/22 03:07, Alexey Kardashevskiy wrote:
> This implements H_WATCHDOG. More detailed comments are in the patches.
> 
> This is based on sha1
> 96c343cc774b Joel Stanley "linux-user: Add PowerPC ISA 3.1 and MMA to hwcap".
> 
> Please comment. Thanks.

This version worked with the kernel side patches you mentioned in patch 2/2,
thanks. Also tested migrating the guest with the WDT active and the guest
rebooted in the destination.


Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com>


> 
> 
> 
> Alexey Kardashevskiy (2):
>    ppc: Define SETFIELD for the ppc target
>    ppc/spapr: Implement H_WATCHDOG
> 
>   include/hw/pci-host/pnv_phb3_regs.h |  16 --
>   include/hw/ppc/spapr.h              |  29 +++-
>   target/ppc/cpu.h                    |   5 +
>   hw/intc/pnv_xive.c                  |  20 ---
>   hw/intc/pnv_xive2.c                 |  20 ---
>   hw/pci-host/pnv_phb4.c              |  16 --
>   hw/ppc/spapr.c                      |   4 +
>   hw/watchdog/spapr_watchdog.c        | 248 ++++++++++++++++++++++++++++
>   hw/watchdog/meson.build             |   1 +
>   hw/watchdog/trace-events            |   7 +
>   10 files changed, 293 insertions(+), 73 deletions(-)
>   create mode 100644 hw/watchdog/spapr_watchdog.c
>