stellaris: Don't hw_error() on bad register accesses

Current recommended style is to log a guest error on bad register
accesses, not kill the whole system with hw_error().  Change the
hw_error() calls to log as LOG_GUEST_ERROR or LOG_UNIMP or use
g_assert_not_reached() as appropriate.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

---
 hw/arm/stellaris.c | 60 +++++++++++++++++++++++++++++++++---------------------
 1 file changed, 37 insertions(+), 23 deletions(-)

-- 
2.7.4

Hi Peter,

On 04/06/2017 10:45 AM, Peter Maydell wrote:
> Current recommended style is to log a guest error on bad register

> accesses, not kill the whole system with hw_error().  Change the

> hw_error() calls to log as LOG_GUEST_ERROR or LOG_UNIMP or use

> g_assert_not_reached() as appropriate.

>

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

> ---

>  hw/arm/stellaris.c | 60 +++++++++++++++++++++++++++++++++---------------------

>  1 file changed, 37 insertions(+), 23 deletions(-)

>

> diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c

> index 9edcd49..ea7a809 100644

> --- a/hw/arm/stellaris.c

> +++ b/hw/arm/stellaris.c

> @@ -108,7 +108,10 @@ static void gptm_reload(gptm_state *s, int n, int reset)

>      } else if (s->mode[n] == 0xa) {

>          /* PWM mode.  Not implemented.  */

>      } else {

> -        hw_error("TODO: 16-bit timer mode 0x%x\n", s->mode[n]);

> +        qemu_log_mask(LOG_UNIMP,

> +                      "GPTM: 16-bit timer mode unimplemented: 0x%x\n",

> +                      s->mode[n]);

> +        return;

>      }

>      s->tick[n] = tick;

>      timer_mod(s->timer[n], tick);

> @@ -149,7 +152,9 @@ static void gptm_tick(void *opaque)

>      } else if (s->mode[n] == 0xa) {

>          /* PWM mode.  Not implemented.  */

>      } else {

> -        hw_error("TODO: 16-bit timer mode 0x%x\n", s->mode[n]);

> +        qemu_log_mask(LOG_UNIMP,

> +                      "GPTM: 16-bit timer mode unimplemented: 0x%x\n",

> +                      s->mode[n]);

>      }

>      gptm_update_irq(s);

>  }

> @@ -286,7 +291,8 @@ static void gptm_write(void *opaque, hwaddr offset,

>          s->match_prescale[0] = value;

>          break;

>      default:

> -        hw_error("gptm_write: Bad offset 0x%x\n", (int)offset);

> +        qemu_log_mask(LOG_GUEST_ERROR,

> +                      "GPTM: read at bad offset 0x%x\n", (int)offset);


use HWADDR_PRIx to remove this unnecessary casts here in following changes?

ie: "GPTM: read at bad offset 0x%" HWADDR_PRIx "\n", offset);

either way:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>


>      }

>      gptm_update_irq(s);

>  }

> @@ -425,7 +431,10 @@ static int ssys_board_class(const ssys_state *s)

>          }

>          /* for unknown classes, fall through */

>      default:

> -        hw_error("ssys_board_class: Unknown class 0x%08x\n", did0);

> +        /* This can only happen if the hardwired constant did0 value

> +         * in this board's stellaris_board_info struct is wrong.

> +         */

> +        g_assert_not_reached();

>      }

>  }

>

> @@ -479,8 +488,7 @@ static uint64_t ssys_read(void *opaque, hwaddr offset,

>              case DID0_CLASS_SANDSTORM:

>                  return pllcfg_sandstorm[xtal];

>              default:

> -                hw_error("ssys_read: Unhandled class for PLLCFG read.\n");

> -                return 0;

> +                g_assert_not_reached();

>              }

>          }

>      case 0x070: /* RCC2 */

> @@ -512,7 +520,8 @@ static uint64_t ssys_read(void *opaque, hwaddr offset,

>      case 0x1e4: /* USER1 */

>          return s->user1;

>      default:

> -        hw_error("ssys_read: Bad offset 0x%x\n", (int)offset);

> +        qemu_log_mask(LOG_GUEST_ERROR,

> +                      "SSYS: read at bad offset 0x%x\n", (int)offset);

>          return 0;

>      }

>  }

> @@ -614,7 +623,8 @@ static void ssys_write(void *opaque, hwaddr offset,

>          s->ldoarst = value;

>          break;

>      default:

> -        hw_error("ssys_write: Bad offset 0x%x\n", (int)offset);

> +        qemu_log_mask(LOG_GUEST_ERROR,

> +                      "SSYS: write at bad offset 0x%x\n", (int)offset);

>      }

>      ssys_update(s);

>  }

> @@ -748,7 +758,8 @@ static uint64_t stellaris_i2c_read(void *opaque, hwaddr offset,

>      case 0x20: /* MCR */

>          return s->mcr;

>      default:

> -        hw_error("strllaris_i2c_read: Bad offset 0x%x\n", (int)offset);

> +        qemu_log_mask(LOG_GUEST_ERROR,

> +                      "stellaris_i2c: read at bad offset 0x%x\n", (int)offset);

>          return 0;

>      }

>  }

> @@ -823,17 +834,18 @@ static void stellaris_i2c_write(void *opaque, hwaddr offset,

>          s->mris &= ~value;

>          break;

>      case 0x20: /* MCR */

> -        if (value & 1)

> -            hw_error(

> -                      "stellaris_i2c_write: Loopback not implemented\n");

> -        if (value & 0x20)

> -            hw_error(

> -                      "stellaris_i2c_write: Slave mode not implemented\n");

> +        if (value & 1) {

> +            qemu_log_mask(LOG_UNIMP, "stellaris_i2c: Loopback not implemented");

> +        }

> +        if (value & 0x20) {

> +            qemu_log_mask(LOG_UNIMP,

> +                          "stellaris_i2c: Slave mode not implemented");

> +        }

>          s->mcr = value & 0x31;

>          break;

>      default:

> -        hw_error("stellaris_i2c_write: Bad offset 0x%x\n",

> -                  (int)offset);

> +        qemu_log_mask(LOG_GUEST_ERROR,

> +                      "stellaris_i2c: write at bad offset 0x%x\n", (int)offset);

>      }

>      stellaris_i2c_update(s);

>  }

> @@ -1057,8 +1069,8 @@ static uint64_t stellaris_adc_read(void *opaque, hwaddr offset,

>      case 0x30: /* SAC */

>          return s->sac;

>      default:

> -        hw_error("strllaris_adc_read: Bad offset 0x%x\n",

> -                  (int)offset);

> +        qemu_log_mask(LOG_GUEST_ERROR,

> +                      "stellaris_adc: read at bad offset 0x%x\n", (int)offset);

>          return 0;

>      }

>  }

> @@ -1078,8 +1090,9 @@ static void stellaris_adc_write(void *opaque, hwaddr offset,

>              return;

>          case 0x04: /* SSCTL */

>              if (value != 6) {

> -                hw_error("ADC: Unimplemented sequence %" PRIx64 "\n",

> -                          value);

> +                qemu_log_mask(LOG_UNIMP,

> +                              "ADC: Unimplemented sequence %" PRIx64 "\n",

> +                              value);

>              }

>              s->ssctl[n] = value;

>              return;

> @@ -1110,13 +1123,14 @@ static void stellaris_adc_write(void *opaque, hwaddr offset,

>          s->sspri = value;

>          break;

>      case 0x28: /* PSSI */

> -        hw_error("Not implemented:  ADC sample initiate\n");

> +        qemu_log_mask(LOG_UNIMP, "ADC: sample initiate unimplemented");

>          break;

>      case 0x30: /* SAC */

>          s->sac = value;

>          break;

>      default:

> -        hw_error("stellaris_adc_write: Bad offset 0x%x\n", (int)offset);

> +        qemu_log_mask(LOG_GUEST_ERROR,

> +                      "stellaris_adc: write at bad offset 0x%x\n", (int)offset);

>      }

>      stellaris_adc_update(s);

>  }

>

On 6 April 2017 at 17:24, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Hi Peter,

>

>

> On 04/06/2017 10:45 AM, Peter Maydell wrote:

>>      default:

>> -        hw_error("gptm_write: Bad offset 0x%x\n", (int)offset);

>> +        qemu_log_mask(LOG_GUEST_ERROR,

>> +                      "GPTM: read at bad offset 0x%x\n", (int)offset);

>

>

> use HWADDR_PRIx to remove this unnecessary casts here in following changes?

>

> ie: "GPTM: read at bad offset 0x%" HWADDR_PRIx "\n", offset);


I don't think either of the two is clearly better for this
sort of case where the offset is known to be small, so I opted
to leave the code the way it was already written.

> either way:

> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>


Thanks.

-- PMM