Re: [Qemu-devel] [PATCH v2] qemu-m68k: add support for interrupt masking/unmasking

[Stefan Weil]

Am 28.03.2015 um 17:07 schrieb Waldemar Brodkorb:
> Fixes following problem, when trying to boot linux:
> qemu: hardware error: mcf_intc_write: Bad write offset 28
>
> CPU #0:
> D0 = 000000ff   A0 = 402ea5dc   F0 = 0000000000000000 (           0)
> D1 = 00000004   A1 = 402ea5e0   F1 = 0000000000000000 (           0)
> D2 = 00000040   A2 = 40040752   F2 = 0000000000000000 (           0)
> D3 = 00000000   A3 = 40040a98   F3 = 0000000000000000 (           0)
> D4 = 00000000   A4 = 400407b4   F4 = 0000000000000000 (           0)
> D5 = 00000000   A5 = 00000000   F5 = 0000000000000000 (           0)
> D6 = 00000000   A6 = 40195ff8   F6 = 0000000000000000 (           0)
> D7 = 00000000   A7 = 40195fd0   F7 = 0000000000000000 (           0)
> PC = 401b2058   SR = 2704 --Z-- FPRESULT =            0
> Aborted
>
> System started via:
> qemu-system-m68k -nographic -nographic -M mcf5208evb -cpu m5208 -kernel kernel
>
> Patch originally posted here:
> http://lists.busybox.net/pipermail/buildroot/2012-April/052585.html
>
> Signed-off-by: Thomas Petazzoni <address@hidden>
> Tested-by: Waldemar Brodkorb <address@hidden>
> Signed-off-by: Waldemar Brodkorb <address@hidden>
> ---
> v1 -> v2:
>          - add {} to conform to Qemu Coding Style suggested by Stefan Weil
>          - add short comments to case statements with return 0 suggested by 
> Peter Maydell
>          - ull as suffix to integer 1 suggested by Peter Maydell does not work 
> for me
>           as I get a kernel panic shortly after boot

Maybe that's an indicator that it only works with 1ULL. :-)

Did you add it at both locations (for set and clear of interrupt mask)?

If not: does it work if you fix this?

If yes: does it work if you only use 1ULL for SIMR?

You can debug the kernel panic by attaching a cross debugger to the 
running kernel.
If you have a kernel image with debug symbols, this is very comfortable.

> ---
>   hw/m68k/mcf_intc.c |   19 +++++++++++++++++++
>   1 file changed, 19 insertions(+)
>
> diff --git a/hw/m68k/mcf_intc.c b/hw/m68k/mcf_intc.c
> index 621423c..dcd14b9 100644
> --- a/hw/m68k/mcf_intc.c
> +++ b/hw/m68k/mcf_intc.c
> @@ -65,6 +65,9 @@ static uint64_t mcf_intc_read(void *opaque, hwaddr addr,
>           return (uint32_t)(s->ifr >> 32);
>       case 0x14:
>           return (uint32_t)s->ifr;
> +    case 0x1c: /* SIMR */
> +    case 0x1d: /* CIMR */
> +       return 0;
>       case 0xe0: /* SWIACK.  */
>           return s->active_vector;
>       case 0xe1: case 0xe2: case 0xe3: case 0xe4:
> @@ -102,6 +105,22 @@ static void mcf_intc_write(void *opaque, hwaddr addr,
>       case 0x0c:
>           s->imr = (s->imr & 0xffffffff00000000ull) | (uint32_t)val;
>           break;
> +    /* SIMR allows to easily mask interrupts */
> +    case 0x1c:
> +       if (val & 0x40) {
> +               s->imr = ~0ull;

Maybe UINT64_MAX is better than ~0ull.

> +       } else {
> +               s->imr |= (1 << (val & 0x3f));

As Peter already said, 1ULL is needed if you want to allow shifts 
resulting in a 64 bit value.
It's also possible to use a type cast like this: (uint64_t)1.

> +       }
> +       break;
> +    /* CIMR allows to easily unmask interrupts */
> +    case 0x1d:
> +       if (val & 0x40) {
> +               s->imr = 0ull;

Here the ULL is redundant.

> +       } else {
> +               s->imr &= ~(1 << (val & 0x3f));

Here also 1ULL or a type cast is needed.

> +       }
> +       break;
>       default:
>           hw_error("mcf_intc_write: Bad write offset %d\n", offset);
>           break;