[PATCH] hw/intc/i8259: Implement legacy LTIM Edge/Level Bank Select

David Woodhouse posted 1 patch 1 year ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20230302090626.1085437-1-dwmw2@infradead.org
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>
hw/intc/i8259.c                 | 10 ++++------
hw/intc/i8259_common.c          | 24 +++++++++++++++++++++++-
include/hw/isa/i8259_internal.h |  1 +
3 files changed, 28 insertions(+), 7 deletions(-)
[PATCH] hw/intc/i8259: Implement legacy LTIM Edge/Level Bank Select
Posted by David Woodhouse 1 year ago
Back in the mists of time, before IBM PS/2 came along with MCA and added
per-pin level control in the ELCR register, the i8259 had a chip-wide
level-mode control in bit 3 of ICW1.

Even in the PIIX3 datasheet from 1996 this is documented as 'This bit is
disabled', but apparently MorphOS is using it in the version of the
i8259 which is in the Pegasos2 board as part of the vt82c686 chipset.

It's easy enough to implement, and I think it's harmless enough to do so
unconditionally.

Signed-off-by: David Woodhouse <dwmw2@infradead.org>
---
 hw/intc/i8259.c                 | 10 ++++------
 hw/intc/i8259_common.c          | 24 +++++++++++++++++++++++-
 include/hw/isa/i8259_internal.h |  1 +
 3 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/hw/intc/i8259.c b/hw/intc/i8259.c
index 17910f3bcb..bbae2d87f4 100644
--- a/hw/intc/i8259.c
+++ b/hw/intc/i8259.c
@@ -133,7 +133,7 @@ static void pic_set_irq(void *opaque, int irq, int level)
     }
 #endif
 
-    if (s->elcr & mask) {
+    if (s->ltim || (s->elcr & mask)) {
         /* level triggered */
         if (level) {
             s->irr |= mask;
@@ -167,7 +167,7 @@ static void pic_intack(PICCommonState *s, int irq)
         s->isr |= (1 << irq);
     }
     /* We don't clear a level sensitive interrupt here */
-    if (!(s->elcr & (1 << irq))) {
+    if (!s->ltim && !(s->elcr & (1 << irq))) {
         s->irr &= ~(1 << irq);
     }
     pic_update_irq(s);
@@ -224,6 +224,7 @@ static void pic_reset(DeviceState *dev)
     PICCommonState *s = PIC_COMMON(dev);
 
     s->elcr = 0;
+    s->ltim = 0;
     pic_init_reset(s);
 }
 
@@ -243,10 +244,7 @@ static void pic_ioport_write(void *opaque, hwaddr addr64,
             s->init_state = 1;
             s->init4 = val & 1;
             s->single_mode = val & 2;
-            if (val & 0x08) {
-                qemu_log_mask(LOG_UNIMP,
-                              "i8259: level sensitive irq not supported\n");
-            }
+            s->ltim = val & 8;
         } else if (val & 0x08) {
             if (val & 0x04) {
                 s->poll = 1;
diff --git a/hw/intc/i8259_common.c b/hw/intc/i8259_common.c
index af2e4a2241..c931dc2d07 100644
--- a/hw/intc/i8259_common.c
+++ b/hw/intc/i8259_common.c
@@ -51,7 +51,7 @@ void pic_reset_common(PICCommonState *s)
     s->special_fully_nested_mode = 0;
     s->init4 = 0;
     s->single_mode = 0;
-    /* Note: ELCR is not reset */
+    /* Note: ELCR and LTIM are not reset */
 }
 
 static int pic_dispatch_pre_save(void *opaque)
@@ -144,6 +144,24 @@ static void pic_print_info(InterruptStatsProvider *obj, Monitor *mon)
                    s->special_fully_nested_mode);
 }
 
+static bool ltim_state_needed(void *opaque)
+{
+    PICCommonState *s = PIC_COMMON(opaque);
+
+    return !!s->ltim;
+}
+
+static const VMStateDescription vmstate_pic_ltim = {
+    .name = "i8259/ltim",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = ltim_state_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT8(ltim, PICCommonState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_pic_common = {
     .name = "i8259",
     .version_id = 1,
@@ -168,6 +186,10 @@ static const VMStateDescription vmstate_pic_common = {
         VMSTATE_UINT8(single_mode, PICCommonState),
         VMSTATE_UINT8(elcr, PICCommonState),
         VMSTATE_END_OF_LIST()
+    },
+    .subsections = (const VMStateDescription*[]) {
+        &vmstate_pic_ltim,
+        NULL
     }
 };
 
diff --git a/include/hw/isa/i8259_internal.h b/include/hw/isa/i8259_internal.h
index 155b098452..f9dcc4163e 100644
--- a/include/hw/isa/i8259_internal.h
+++ b/include/hw/isa/i8259_internal.h
@@ -61,6 +61,7 @@ struct PICCommonState {
     uint8_t single_mode; /* true if slave pic is not initialized */
     uint8_t elcr; /* PIIX edge/trigger selection*/
     uint8_t elcr_mask;
+    uint8_t ltim; /* Edge/Level Bank Select (pre-PIIX, chip-wide) */
     qemu_irq int_out[1];
     uint32_t master; /* reflects /SP input pin */
     uint32_t iobase;
-- 
2.39.0
Re: [PATCH] hw/intc/i8259: Implement legacy LTIM Edge/Level Bank Select
Posted by BALATON Zoltan 1 year ago
On Thu, 2 Mar 2023, David Woodhouse wrote:
> Back in the mists of time, before IBM PS/2 came along with MCA and added
> per-pin level control in the ELCR register, the i8259 had a chip-wide
> level-mode control in bit 3 of ICW1.

Thanks a lot for doing this, it's easy if you already understand the PIC 
and know where to look for the rest but takes more time if I have to learn 
all that first. I can only comment on comments, the rest looks plausible 
but I lack the knowledge to review that. I'll test this later today wirh 
MorphOS and if no problems found I can make chnages to commit message as 
you've asked and add it to the series to replace my workaround.

I'm not sure how to test migration, I've never used that with QEMU so if 
somebody can do that with PC machine that would help. The pegasos2 does 
not yet support migration because it did not work before and we are still 
implementing parts of VT8231 so it was decided to only fix vmstate saving 
after that's settled so we don't have to worry about that now.

> Even in the PIIX3 datasheet from 1996 this is documented as 'This bit is
> disabled', but apparently MorphOS is using it in the version of the
> i8259 which is in the Pegasos2 board as part of the vt82c686 chipset.

The pegasos2 actually has VT8231 which is closely related to 686B but a 
bit later member of that family.

> It's easy enough to implement, and I think it's harmless enough to do so
> unconditionally.
>
> Signed-off-by: David Woodhouse <dwmw2@infradead.org>
> ---
> hw/intc/i8259.c                 | 10 ++++------
> hw/intc/i8259_common.c          | 24 +++++++++++++++++++++++-
> include/hw/isa/i8259_internal.h |  1 +
> 3 files changed, 28 insertions(+), 7 deletions(-)
>
> diff --git a/hw/intc/i8259.c b/hw/intc/i8259.c
> index 17910f3bcb..bbae2d87f4 100644
> --- a/hw/intc/i8259.c
> +++ b/hw/intc/i8259.c
> @@ -133,7 +133,7 @@ static void pic_set_irq(void *opaque, int irq, int level)
>     }
> #endif
>
> -    if (s->elcr & mask) {
> +    if (s->ltim || (s->elcr & mask)) {
>         /* level triggered */
>         if (level) {
>             s->irr |= mask;
> @@ -167,7 +167,7 @@ static void pic_intack(PICCommonState *s, int irq)
>         s->isr |= (1 << irq);
>     }
>     /* We don't clear a level sensitive interrupt here */
> -    if (!(s->elcr & (1 << irq))) {
> +    if (!s->ltim && !(s->elcr & (1 << irq))) {
>         s->irr &= ~(1 << irq);
>     }
>     pic_update_irq(s);
> @@ -224,6 +224,7 @@ static void pic_reset(DeviceState *dev)
>     PICCommonState *s = PIC_COMMON(dev);
>
>     s->elcr = 0;
> +    s->ltim = 0;
>     pic_init_reset(s);
> }
>
> @@ -243,10 +244,7 @@ static void pic_ioport_write(void *opaque, hwaddr addr64,
>             s->init_state = 1;
>             s->init4 = val & 1;
>             s->single_mode = val & 2;
> -            if (val & 0x08) {
> -                qemu_log_mask(LOG_UNIMP,
> -                              "i8259: level sensitive irq not supported\n");
> -            }
> +            s->ltim = val & 8;
>         } else if (val & 0x08) {
>             if (val & 0x04) {
>                 s->poll = 1;
> diff --git a/hw/intc/i8259_common.c b/hw/intc/i8259_common.c
> index af2e4a2241..c931dc2d07 100644
> --- a/hw/intc/i8259_common.c
> +++ b/hw/intc/i8259_common.c
> @@ -51,7 +51,7 @@ void pic_reset_common(PICCommonState *s)
>     s->special_fully_nested_mode = 0;
>     s->init4 = 0;
>     s->single_mode = 0;
> -    /* Note: ELCR is not reset */
> +    /* Note: ELCR and LTIM are not reset */
> }
>
> static int pic_dispatch_pre_save(void *opaque)
> @@ -144,6 +144,24 @@ static void pic_print_info(InterruptStatsProvider *obj, Monitor *mon)
>                    s->special_fully_nested_mode);
> }
>
> +static bool ltim_state_needed(void *opaque)
> +{
> +    PICCommonState *s = PIC_COMMON(opaque);
> +
> +    return !!s->ltim;
> +}
> +
> +static const VMStateDescription vmstate_pic_ltim = {
> +    .name = "i8259/ltim",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = ltim_state_needed,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT8(ltim, PICCommonState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> static const VMStateDescription vmstate_pic_common = {
>     .name = "i8259",
>     .version_id = 1,
> @@ -168,6 +186,10 @@ static const VMStateDescription vmstate_pic_common = {
>         VMSTATE_UINT8(single_mode, PICCommonState),
>         VMSTATE_UINT8(elcr, PICCommonState),
>         VMSTATE_END_OF_LIST()
> +    },
> +    .subsections = (const VMStateDescription*[]) {
> +        &vmstate_pic_ltim,
> +        NULL
>     }
> };
>
> diff --git a/include/hw/isa/i8259_internal.h b/include/hw/isa/i8259_internal.h
> index 155b098452..f9dcc4163e 100644
> --- a/include/hw/isa/i8259_internal.h
> +++ b/include/hw/isa/i8259_internal.h
> @@ -61,6 +61,7 @@ struct PICCommonState {
>     uint8_t single_mode; /* true if slave pic is not initialized */
>     uint8_t elcr; /* PIIX edge/trigger selection*/
>     uint8_t elcr_mask;
> +    uint8_t ltim; /* Edge/Level Bank Select (pre-PIIX, chip-wide) */

Is this rather called Edge/Level Interrupt Mode or something like that?

Regards,
BALATON Zoltan

>     qemu_irq int_out[1];
>     uint32_t master; /* reflects /SP input pin */
>     uint32_t iobase;
>
Re: [PATCH] hw/intc/i8259: Implement legacy LTIM Edge/Level Bank Select
Posted by David Woodhouse 1 year ago
On Thu, 2023-03-02 at 09:06 +0000, David Woodhouse wrote:
> Back in the mists of time, before IBM PS/2 came along with MCA and added
> per-pin level control in the ELCR register, the i8259 had a chip-wide
> level-mode control in bit 3 of ICW1.

Actually... I think MCA might have been level triggered system-wide,
and still used this LTIM bit. Per-pin control via ELCR probably came in
with EISA?