diff mbox

qemu-m68k: add support for interrupt masking/unmasking

Message ID 20150322090902.GA8451@waldemar-brodkorb.de
State New
Headers show

Commit Message

Waldemar Brodkorb March 22, 2015, 9:09 a.m. UTC 
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 <thomas.petazzoni@free-electrons.com>
Tested-by: Waldemar Brodkorb <wbx@openadk.org>
Signed-off-by: Waldemar Brodkorb <wbx@openadk.org>
---
 hw/m68k/mcf_intc.c |   18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

Comments

Stefan Weil March 22, 2015, 9:49 a.m. UTC | #1 
Technically this implementation looks reasonable. I added some remarks 
below.

Am 22.03.2015 um 10:09 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 <thomas.petazzoni@free-electrons.com>
> Tested-by: Waldemar Brodkorb <wbx@openadk.org>
> Signed-off-by: Waldemar Brodkorb <wbx@openadk.org>
> ---
>   hw/m68k/mcf_intc.c |   18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
>
> diff --git a/hw/m68k/mcf_intc.c b/hw/m68k/mcf_intc.c
> index 621423c..1d161b1 100644
> --- a/hw/m68k/mcf_intc.c
> +++ b/hw/m68k/mcf_intc.c
> @@ -65,6 +65,10 @@ static uint64_t mcf_intc_read(void *opaque, hwaddr addr,
>           return (uint32_t)(s->ifr >> 32);
>       case 0x14:
>           return (uint32_t)s->ifr;
> +    /* Reading from SIMR and CIMR return 0 */
Maybe this comment is not needed if the following code is changed (see 
below).
> +    case 0x1c:

Add /* SIMR */ comment behind case statement like it was done for SWIACK.
Then either add a /* fall through */ comment or a return 0 (to satisfy 
static
code analyzers).

> +    case 0x1d:
Dto. for CIMR.
> +	return 0;
>       case 0xe0: /* SWIACK.  */
>           return s->active_vector;
>       case 0xe1: case 0xe2: case 0xe3: case 0xe4:
> @@ -102,6 +106,20 @@ 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;
UINT64_MAX
> +	else
> +		s->imr |= (1 << (val & 0x3f));
The QEMU coding style requires {}.
> +	break;
> +    /* CIMR allows to easily unmask interrupts */
> +    case 0x1d:
> +	if (val & 0x40)
> +		s->imr = 0ull;
> +	else
> +		s->imr &= ~(1 << (val & 0x3f));
Dto.
> +	break;
>       default:
>           hw_error("mcf_intc_write: Bad write offset %d\n", offset);
>           break;
Peter Maydell March 22, 2015, 11:40 a.m. UTC | #2 
On 22 March 2015 at 09:09, Waldemar Brodkorb <wbx@openadk.org> wrote:
> 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 <thomas.petazzoni@free-electrons.com>
> Tested-by: Waldemar Brodkorb <wbx@openadk.org>
> Signed-off-by: Waldemar Brodkorb <wbx@openadk.org>
> ---
>  hw/m68k/mcf_intc.c |   18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/hw/m68k/mcf_intc.c b/hw/m68k/mcf_intc.c
> index 621423c..1d161b1 100644
> --- a/hw/m68k/mcf_intc.c
> +++ b/hw/m68k/mcf_intc.c
> @@ -65,6 +65,10 @@ static uint64_t mcf_intc_read(void *opaque, hwaddr addr,
>          return (uint32_t)(s->ifr >> 32);
>      case 0x14:
>          return (uint32_t)s->ifr;
> +    /* Reading from SIMR and CIMR return 0 */
> +    case 0x1c:
> +    case 0x1d:
> +       return 0;
>      case 0xe0: /* SWIACK.  */
>          return s->active_vector;
>      case 0xe1: case 0xe2: case 0xe3: case 0xe4:
> @@ -102,6 +106,20 @@ 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;
> +       else
> +               s->imr |= (1 << (val & 0x3f));

This is undefined behaviour for large values of
'val' because 1 is only an int type and we might
try to shift it by more than 30. You need a ULL
suffix on the 1.

> +       break;
> +    /* CIMR allows to easily unmask interrupts */
> +    case 0x1d:
> +       if (val & 0x40)
> +               s->imr = 0ull;
> +       else
> +               s->imr &= ~(1 << (val & 0x3f));
> +       break;
>      default:
>          hw_error("mcf_intc_write: Bad write offset %d\n", offset);
>          break;

thanks
-- PMM
Peter Maydell March 22, 2015, 11:43 a.m. UTC | #3 
On 22 March 2015 at 09:49, Stefan Weil <sw@weilnetz.de> wrote:
> Am 22.03.2015 um 10:09 schrieb Waldemar Brodkorb:
>>
>> +    case 0x1c:
>
>
> Add /* SIMR */ comment behind case statement like it was done for SWIACK.
> Then either add a /* fall through */ comment or a return 0 (to satisfy
> static
> code analyzers).

No, I don't believe you need a /* fall through */ comment here:
analysers are smart enough to realize that two 'case foo:'
lines with no code at all between them means a deliberate
fall through. It's only where you have
 case foo:
     some_code();
 case bar:
     more_code();

that you need to add the comment to indicate that the fall through
was intentional.

Just writing
     case 0x1c: /* SIMR */
     case 0x1d: /* CIMR */
         return 0;

should be good enough.

-- PMM
diff mbox

Patch

diff --git a/hw/m68k/mcf_intc.c b/hw/m68k/mcf_intc.c
index 621423c..1d161b1 100644
--- a/hw/m68k/mcf_intc.c
+++ b/hw/m68k/mcf_intc.c
@@ -65,6 +65,10 @@  static uint64_t mcf_intc_read(void *opaque, hwaddr addr,
         return (uint32_t)(s->ifr >> 32);
     case 0x14:
         return (uint32_t)s->ifr;
+    /* Reading from SIMR and CIMR return 0 */
+    case 0x1c:
+    case 0x1d:
+	return 0;
     case 0xe0: /* SWIACK.  */
         return s->active_vector;
     case 0xe1: case 0xe2: case 0xe3: case 0xe4:
@@ -102,6 +106,20 @@  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;
+	else
+		s->imr |= (1 << (val & 0x3f));
+	break;
+    /* CIMR allows to easily unmask interrupts */
+    case 0x1d:
+	if (val & 0x40)
+		s->imr = 0ull;
+	else
+		s->imr &= ~(1 << (val & 0x3f));
+	break;
     default:
         hw_error("mcf_intc_write: Bad write offset %d\n", offset);
         break;