[PATCH 0/2] wdt_aspeed: Fix behaviour of control register

[PATCH 0/2] wdt_aspeed: Fix behaviour of control register

[Andrew Jeffery]

Hello,

I discovered a couple of bugs in the watchdog while testing a tool to poke
Aspeed BMCs over their various AHB bridges. The immediate observation was that
the model for the 2500 wasn't signalling use of the fixed 1MHz clock, which is
resolved in the first patch. The other observation was that sequential writes to
control weren't sticking if the enable bit wasn't toggled, which is fixed in the
second patch.

Please review.

Andrew

Andrew Jeffery (2):
  watchdog: aspeed: Sanitize control register values
  watchdog: aspeed: Fix sequential control writes

 hw/watchdog/wdt_aspeed.c         | 26 ++++++++++++++++++++++++--
 include/hw/watchdog/wdt_aspeed.h |  1 +
 2 files changed, 25 insertions(+), 2 deletions(-)

-- 
2.30.2

[PATCH 1/2] watchdog: aspeed: Sanitize control register values

[Andrew Jeffery]

While some of the critical fields remain the same, there is variation in
the definition of the control register across the SoC generations.
Reserved regions are adjusted, while in other cases the mutability or
behaviour of fields change.

Introduce a callback to sanitize the value on writes to ensure model
behaviour reflects the hardware.

Fixes: 854123bf8d4b ("wdt: Add Aspeed watchdog device model")
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 hw/watchdog/wdt_aspeed.c         | 24 ++++++++++++++++++++++--
 include/hw/watchdog/wdt_aspeed.h |  1 +
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c
index 6352ba1b0e5b..faa3d35fdf21 100644
--- a/hw/watchdog/wdt_aspeed.c
+++ b/hw/watchdog/wdt_aspeed.c
@@ -118,13 +118,27 @@ static void aspeed_wdt_reload_1mhz(AspeedWDTState *s)
     }
 }
 
+static uint64_t aspeed_2400_sanitize_ctrl(uint64_t data)
+{
+    return data & 0xffff;
+}
+
+static uint64_t aspeed_2500_sanitize_ctrl(uint64_t data)
+{
+    return (data & ~(0xfUL << 8)) | WDT_CTRL_1MHZ_CLK;
+}
+
+static uint64_t aspeed_2600_sanitize_ctrl(uint64_t data)
+{
+    return data & ~(0x7UL << 7);
+}
 
 static void aspeed_wdt_write(void *opaque, hwaddr offset, uint64_t data,
                              unsigned size)
 {
     AspeedWDTState *s = ASPEED_WDT(opaque);
     AspeedWDTClass *awc = ASPEED_WDT_GET_CLASS(s);
-    bool enable = data & WDT_CTRL_ENABLE;
+    bool enable;
 
     offset >>= 2;
 
@@ -144,6 +158,8 @@ static void aspeed_wdt_write(void *opaque, hwaddr offset, uint64_t data,
         }
         break;
     case WDT_CTRL:
+        data = awc->sanitize_ctrl(data);
+        enable = data & WDT_CTRL_ENABLE;
         if (enable && !aspeed_wdt_is_enabled(s)) {
             s->regs[WDT_CTRL] = data;
             awc->wdt_reload(s);
@@ -207,11 +223,12 @@ static const MemoryRegionOps aspeed_wdt_ops = {
 static void aspeed_wdt_reset(DeviceState *dev)
 {
     AspeedWDTState *s = ASPEED_WDT(dev);
+    AspeedWDTClass *awc = ASPEED_WDT_GET_CLASS(s);
 
     s->regs[WDT_STATUS] = 0x3EF1480;
     s->regs[WDT_RELOAD_VALUE] = 0x03EF1480;
     s->regs[WDT_RESTART] = 0;
-    s->regs[WDT_CTRL] = 0;
+    s->regs[WDT_CTRL] = awc->sanitize_ctrl(0);
     s->regs[WDT_RESET_WIDTH] = 0xFF;
 
     timer_del(s->timer);
@@ -293,6 +310,7 @@ static void aspeed_2400_wdt_class_init(ObjectClass *klass, void *data)
     awc->ext_pulse_width_mask = 0xff;
     awc->reset_ctrl_reg = SCU_RESET_CONTROL1;
     awc->wdt_reload = aspeed_wdt_reload;
+    awc->sanitize_ctrl = aspeed_2400_sanitize_ctrl;
 }
 
 static const TypeInfo aspeed_2400_wdt_info = {
@@ -328,6 +346,7 @@ static void aspeed_2500_wdt_class_init(ObjectClass *klass, void *data)
     awc->reset_ctrl_reg = SCU_RESET_CONTROL1;
     awc->reset_pulse = aspeed_2500_wdt_reset_pulse;
     awc->wdt_reload = aspeed_wdt_reload_1mhz;
+    awc->sanitize_ctrl = aspeed_2500_sanitize_ctrl;
 }
 
 static const TypeInfo aspeed_2500_wdt_info = {
@@ -348,6 +367,7 @@ static void aspeed_2600_wdt_class_init(ObjectClass *klass, void *data)
     awc->reset_ctrl_reg = AST2600_SCU_RESET_CONTROL1;
     awc->reset_pulse = aspeed_2500_wdt_reset_pulse;
     awc->wdt_reload = aspeed_wdt_reload_1mhz;
+    awc->sanitize_ctrl = aspeed_2600_sanitize_ctrl;
 }
 
 static const TypeInfo aspeed_2600_wdt_info = {
diff --git a/include/hw/watchdog/wdt_aspeed.h b/include/hw/watchdog/wdt_aspeed.h
index 80b03661e303..f945cd6c5833 100644
--- a/include/hw/watchdog/wdt_aspeed.h
+++ b/include/hw/watchdog/wdt_aspeed.h
@@ -44,6 +44,7 @@ struct AspeedWDTClass {
     uint32_t reset_ctrl_reg;
     void (*reset_pulse)(AspeedWDTState *s, uint32_t property);
     void (*wdt_reload)(AspeedWDTState *s);
+    uint64_t (*sanitize_ctrl)(uint64_t data);
 };
 
 #endif /* WDT_ASPEED_H */
-- 
2.30.2

Re: [PATCH 1/2] watchdog: aspeed: Sanitize control register values

[Cédric Le Goater]

On 7/9/21 7:31 AM, Andrew Jeffery wrote:
> While some of the critical fields remain the same, there is variation in
> the definition of the control register across the SoC generations.
> Reserved regions are adjusted, while in other cases the mutability or
> behaviour of fields change.
> 
> Introduce a callback to sanitize the value on writes to ensure model
> behaviour reflects the hardware.
> 
> Fixes: 854123bf8d4b ("wdt: Add Aspeed watchdog device model")
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>

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


> ---
>  hw/watchdog/wdt_aspeed.c         | 24 ++++++++++++++++++++++--
>  include/hw/watchdog/wdt_aspeed.h |  1 +
>  2 files changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c
> index 6352ba1b0e5b..faa3d35fdf21 100644
> --- a/hw/watchdog/wdt_aspeed.c
> +++ b/hw/watchdog/wdt_aspeed.c
> @@ -118,13 +118,27 @@ static void aspeed_wdt_reload_1mhz(AspeedWDTState *s)
>      }
>  }
>  
> +static uint64_t aspeed_2400_sanitize_ctrl(uint64_t data)
> +{
> +    return data & 0xffff;
> +}
> +
> +static uint64_t aspeed_2500_sanitize_ctrl(uint64_t data)
> +{
> +    return (data & ~(0xfUL << 8)) | WDT_CTRL_1MHZ_CLK;
> +}
> +
> +static uint64_t aspeed_2600_sanitize_ctrl(uint64_t data)
> +{
> +    return data & ~(0x7UL << 7);
> +}
>  
>  static void aspeed_wdt_write(void *opaque, hwaddr offset, uint64_t data,
>                               unsigned size)
>  {
>      AspeedWDTState *s = ASPEED_WDT(opaque);
>      AspeedWDTClass *awc = ASPEED_WDT_GET_CLASS(s);
> -    bool enable = data & WDT_CTRL_ENABLE;
> +    bool enable;
>  
>      offset >>= 2;
>  
> @@ -144,6 +158,8 @@ static void aspeed_wdt_write(void *opaque, hwaddr offset, uint64_t data,
>          }
>          break;
>      case WDT_CTRL:
> +        data = awc->sanitize_ctrl(data);
> +        enable = data & WDT_CTRL_ENABLE;
>          if (enable && !aspeed_wdt_is_enabled(s)) {
>              s->regs[WDT_CTRL] = data;
>              awc->wdt_reload(s);
> @@ -207,11 +223,12 @@ static const MemoryRegionOps aspeed_wdt_ops = {
>  static void aspeed_wdt_reset(DeviceState *dev)
>  {
>      AspeedWDTState *s = ASPEED_WDT(dev);
> +    AspeedWDTClass *awc = ASPEED_WDT_GET_CLASS(s);
>  
>      s->regs[WDT_STATUS] = 0x3EF1480;
>      s->regs[WDT_RELOAD_VALUE] = 0x03EF1480;
>      s->regs[WDT_RESTART] = 0;
> -    s->regs[WDT_CTRL] = 0;
> +    s->regs[WDT_CTRL] = awc->sanitize_ctrl(0);
>      s->regs[WDT_RESET_WIDTH] = 0xFF;
>  
>      timer_del(s->timer);
> @@ -293,6 +310,7 @@ static void aspeed_2400_wdt_class_init(ObjectClass *klass, void *data)
>      awc->ext_pulse_width_mask = 0xff;
>      awc->reset_ctrl_reg = SCU_RESET_CONTROL1;
>      awc->wdt_reload = aspeed_wdt_reload;
> +    awc->sanitize_ctrl = aspeed_2400_sanitize_ctrl;
>  }
>  
>  static const TypeInfo aspeed_2400_wdt_info = {
> @@ -328,6 +346,7 @@ static void aspeed_2500_wdt_class_init(ObjectClass *klass, void *data)
>      awc->reset_ctrl_reg = SCU_RESET_CONTROL1;
>      awc->reset_pulse = aspeed_2500_wdt_reset_pulse;
>      awc->wdt_reload = aspeed_wdt_reload_1mhz;
> +    awc->sanitize_ctrl = aspeed_2500_sanitize_ctrl;
>  }
>  
>  static const TypeInfo aspeed_2500_wdt_info = {
> @@ -348,6 +367,7 @@ static void aspeed_2600_wdt_class_init(ObjectClass *klass, void *data)
>      awc->reset_ctrl_reg = AST2600_SCU_RESET_CONTROL1;
>      awc->reset_pulse = aspeed_2500_wdt_reset_pulse;
>      awc->wdt_reload = aspeed_wdt_reload_1mhz;
> +    awc->sanitize_ctrl = aspeed_2600_sanitize_ctrl;
>  }
>  
>  static const TypeInfo aspeed_2600_wdt_info = {
> diff --git a/include/hw/watchdog/wdt_aspeed.h b/include/hw/watchdog/wdt_aspeed.h
> index 80b03661e303..f945cd6c5833 100644
> --- a/include/hw/watchdog/wdt_aspeed.h
> +++ b/include/hw/watchdog/wdt_aspeed.h
> @@ -44,6 +44,7 @@ struct AspeedWDTClass {
>      uint32_t reset_ctrl_reg;
>      void (*reset_pulse)(AspeedWDTState *s, uint32_t property);
>      void (*wdt_reload)(AspeedWDTState *s);
> +    uint64_t (*sanitize_ctrl)(uint64_t data);
>  };
>  
>  #endif /* WDT_ASPEED_H */
> 

[PATCH 2/2] watchdog: aspeed: Fix sequential control writes

[Andrew Jeffery]

The logic in the handling for the control register required toggling the
enable state for writes to stick. Rework the condition chain to allow
sequential writes that do not update the enable state.

Fixes: 854123bf8d4b ("wdt: Add Aspeed watchdog device model")
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 hw/watchdog/wdt_aspeed.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c
index faa3d35fdf21..69c37af9a6e9 100644
--- a/hw/watchdog/wdt_aspeed.c
+++ b/hw/watchdog/wdt_aspeed.c
@@ -166,6 +166,8 @@ static void aspeed_wdt_write(void *opaque, hwaddr offset, uint64_t data,
         } else if (!enable && aspeed_wdt_is_enabled(s)) {
             s->regs[WDT_CTRL] = data;
             timer_del(s->timer);
+        } else {
+            s->regs[WDT_CTRL] = data;
         }
         break;
     case WDT_RESET_WIDTH:
-- 
2.30.2

Re: [PATCH 2/2] watchdog: aspeed: Fix sequential control writes

[Philippe Mathieu-Daudé]

On 7/9/21 7:31 AM, Andrew Jeffery wrote:
> The logic in the handling for the control register required toggling the
> enable state for writes to stick. Rework the condition chain to allow
> sequential writes that do not update the enable state.
> 
> Fixes: 854123bf8d4b ("wdt: Add Aspeed watchdog device model")
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>  hw/watchdog/wdt_aspeed.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c
> index faa3d35fdf21..69c37af9a6e9 100644
> --- a/hw/watchdog/wdt_aspeed.c
> +++ b/hw/watchdog/wdt_aspeed.c
> @@ -166,6 +166,8 @@ static void aspeed_wdt_write(void *opaque, hwaddr offset, uint64_t data,
>          } else if (!enable && aspeed_wdt_is_enabled(s)) {
>              s->regs[WDT_CTRL] = data;
>              timer_del(s->timer);
> +        } else {
> +            s->regs[WDT_CTRL] = data;

What about simplifying by moving here:

               if (!enable && aspeed_wdt_is_enabled(s)) {
                   timer_del(s->timer);
               }

>          }
>          break;
>      case WDT_RESET_WIDTH:
> 

Re: [PATCH 2/2] watchdog: aspeed: Fix sequential control writes

[Andrew Jeffery]

On Fri, 9 Jul 2021, at 16:59, Philippe Mathieu-Daudé wrote:
> On 7/9/21 7:31 AM, Andrew Jeffery wrote:
> > The logic in the handling for the control register required toggling the
> > enable state for writes to stick. Rework the condition chain to allow
> > sequential writes that do not update the enable state.
> > 
> > Fixes: 854123bf8d4b ("wdt: Add Aspeed watchdog device model")
> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > ---
> >  hw/watchdog/wdt_aspeed.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c
> > index faa3d35fdf21..69c37af9a6e9 100644
> > --- a/hw/watchdog/wdt_aspeed.c
> > +++ b/hw/watchdog/wdt_aspeed.c
> > @@ -166,6 +166,8 @@ static void aspeed_wdt_write(void *opaque, hwaddr offset, uint64_t data,
> >          } else if (!enable && aspeed_wdt_is_enabled(s)) {
> >              s->regs[WDT_CTRL] = data;
> >              timer_del(s->timer);
> > +        } else {
> > +            s->regs[WDT_CTRL] = data;
> 
> What about simplifying by moving here:
> 
>                if (!enable && aspeed_wdt_is_enabled(s)) {
>                    timer_del(s->timer);
>                }
> 

I don't think that works, as aspeed_wdt_is_enabled() tests the value of 
s->regs[WDT_CTRL]. If you set it before you test then you end up in the 
wrong state.

Andrew

Re: [PATCH 2/2] watchdog: aspeed: Fix sequential control writes

[Cédric Le Goater]

On 7/9/21 7:31 AM, Andrew Jeffery wrote:
> The logic in the handling for the control register required toggling the
> enable state for writes to stick. Rework the condition chain to allow
> sequential writes that do not update the enable state.
> 
> Fixes: 854123bf8d4b ("wdt: Add Aspeed watchdog device model")
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>

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

> ---
>  hw/watchdog/wdt_aspeed.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c
> index faa3d35fdf21..69c37af9a6e9 100644
> --- a/hw/watchdog/wdt_aspeed.c
> +++ b/hw/watchdog/wdt_aspeed.c
> @@ -166,6 +166,8 @@ static void aspeed_wdt_write(void *opaque, hwaddr offset, uint64_t data,
>          } else if (!enable && aspeed_wdt_is_enabled(s)) {
>              s->regs[WDT_CTRL] = data;
>              timer_del(s->timer);
> +        } else {
> +            s->regs[WDT_CTRL] = data;
>          }
>          break;
>      case WDT_RESET_WIDTH:
>