Re: [PATCH v2 1/2] hw/m68k/mcf5206: Reduce m5206_mbar_read/write() offset arg to 16-bit

[Thomas Huth]

Am Mon, 18 May 2020 19:38:58 +0200
schrieb Philippe Mathieu-Daudé <address@hidden>:

> All calls to m5206_mbar_read/m5206_mbar_write are used with
> 'offset = hwaddr & 0x3ff', so we are sure the offset fits
> in 16-bit.
> 
> Suggested-by: Thomas Huth <address@hidden>
> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> ---
>  hw/m68k/mcf5206.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/m68k/mcf5206.c b/hw/m68k/mcf5206.c
> index b155dd8170..45f44c43f0 100644
> --- a/hw/m68k/mcf5206.c
> +++ b/hw/m68k/mcf5206.c
> @@ -273,7 +273,7 @@ static void m5206_mbar_reset(m5206_mbar_state *s)
>  }
>  
>  static uint64_t m5206_mbar_read(m5206_mbar_state *s,
> -                                uint64_t offset, unsigned size)
> +                                uint16_t offset, unsigned size)
>  {
>      if (offset >= 0x100 && offset < 0x120) {
>          return m5206_timer_read(s->timer[0], offset - 0x100);
> @@ -306,11 +306,11 @@ static uint64_t
> m5206_mbar_read(m5206_mbar_state *s, case 0x170: return s->uivr[0];
>      case 0x1b0: return s->uivr[1];
>      }
> -    hw_error("Bad MBAR read offset 0x%x", (int)offset);
> +    hw_error("Bad MBAR read offset 0x%x", offset);
>      return 0;
>  }
>  
> -static void m5206_mbar_write(m5206_mbar_state *s, uint32_t offset,
> +static void m5206_mbar_write(m5206_mbar_state *s, uint16_t offset,
>                               uint64_t value, unsigned size)
>  {
>      if (offset >= 0x100 && offset < 0x120) {
> @@ -360,7 +360,7 @@ static void m5206_mbar_write(m5206_mbar_state *s,
> uint32_t offset, s->uivr[1] = value;
>          break;
>      default:
> -        hw_error("Bad MBAR write offset 0x%x", (int)offset);
> +        hw_error("Bad MBAR write offset 0x%x", offset);

Isn't the correct format string for short ints (i.e. 16-bit) rather %hx
instead of %x ? ... I think it does not matter on x86, but IIRC there
are other architectures where this could be a problem. I'd say, let's
simply use "int" for offset instead, that's likely the best solution.
(I can do the change when picking up the patch in case you don't want
to respin, just let me know)

 Thomas