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 */
>