[Qemu-devel] [RFC PATCH 0/2] smc37c669: Fix parallel overlapping VGA ioport

[Qemu-devel] [RFC PATCH 0/2] smc37c669: Fix parallel overlapping VGA ioport

[Philippe Mathieu-Daudé]

Hi,

This RFC series attempt to fix a regression introduced in a4cb773928e,
and reported by Emilio:
http://lists.nongnu.org/archive/html/qemu-devel/2018-06/msg00317.html

Mark noticed the VGA and parallel I/O were overlapping:

(qemu) info mtree
address-space: memory
  0000000000000000-ffffffffffffffff (prio 0, i/o): system
    00000801fc000000-00000801fdffffff (prio 0, i/o): pci0-io
      ...
      00000801fc0003b4-00000801fc0003b5 (prio 0, i/o): vga
      00000801fc0003ba-00000801fc0003ba (prio 0, i/o): vga
      00000801fc0003bc-00000801fc0003c3 (prio 0, i/o): parallel
                                    ^^^                ^^^^^^^^
      00000801fc0003c0-00000801fc0003cf (prio 0, i/o): vga
                   ^^^
      00000801fc0003d4-00000801fc0003d5 (prio 0, i/o): vga
      00000801fc0003da-00000801fc0003da (prio 0, i/o): vga
      ...

In reset the parallel port is enabled. Since the emulated PALcode
directly accesses the VGA port without checking if the SIO parallel
port is enabled, I suppose the real PALcode does this check.

To avoid changing the emulated PALcode, a "parallel" property is used
to reset the SIO with the parallel device disabled.

I tried to implement the quickest simpler fix...

references used:
- http://www.osdever.net/FreeVGA/vga/portidx.htm
- https://retired.beyondlogic.org/spp/parallel.htm
- http://ww1.microchip.com/downloads/en/DeviceDoc/37c669.pdf
- linux/arch/alpha/kernel/smc37c669.c
- https://github.com/rth7680/qemu-palcode/blob/master/init.c#L290
- https://github.com/rth7680/qemu-palcode/blob/master/ioport.h#L48

Regards,

Phil.

Philippe Mathieu-Daudé (2):
  hw/isa/smc37c669-superio: Basic 'Config Registers' implementation
  hw/alpha/dp264: Disable the Super I/O parallel port

 hw/alpha/dp264.c           |  6 +++-
 hw/isa/smc37c669-superio.c | 73 +++++++++++++++++++++++++++++++++-----
 2 files changed, 70 insertions(+), 9 deletions(-)

-- 
2.17.1

[Qemu-devel] [RFC PATCH 1/2] hw/isa/smc37c669-superio: Basic 'Config Registers' implementation

[Philippe Mathieu-Daudé]

The Config Register 0..3 are used by the BIOS to enable functionalities.

    The FDC37C669 incorporates Software Configurable Logic (SCL)
    for ease of use. Use of the SCL feature allows programmable
    system configuration of key functions such as the FDC,
    parallel port, and UARTs.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/isa/smc37c669-superio.c | 73 +++++++++++++++++++++++++++++++++-----
 1 file changed, 65 insertions(+), 8 deletions(-)

diff --git a/hw/isa/smc37c669-superio.c b/hw/isa/smc37c669-superio.c
index aa233c6967..49d0984ad9 100644
--- a/hw/isa/smc37c669-superio.c
+++ b/hw/isa/smc37c669-superio.c
@@ -6,16 +6,37 @@
  * This code is licensed under the GNU GPLv2 and later.
  * See the COPYING file in the top-level directory.
  * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * Data Sheet (Rev. 06/29/2007):
+ * http://ww1.microchip.com/downloads/en/DeviceDoc/37c669.pdf
  */
 
 #include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qemu/log.h"
 #include "hw/isa/superio.h"
+#include "trace.h"
+
+#define SMC37C669(obj) \
+            OBJECT_CHECK(SMC37C669State, (obj), TYPE_SMC37C669_SUPERIO)
+
+typedef struct SMC37C669State {
+    /*< private >*/
+    ISASuperIODevice parent_dev;
+    /*< public >*/
+
+    uint32_t config; /* initial configuration */
+
+    uint8_t cr[4];
+} SMC37C669State;
 
-/* UARTs (compatible with NS16450 or PC16550) */
+/* UARTs (NS16C550 compatible) */
 
 static bool is_serial_enabled(ISASuperIODevice *sio, uint8_t index)
 {
-    return index < 2;
+    SMC37C669State *s = SMC37C669(sio);
+
+    return extract32(s->cr[2], 3 + index * 4, 1);
 }
 
 static uint16_t get_serial_iobase(ISASuperIODevice *sio, uint8_t index)
@@ -28,11 +49,13 @@ static unsigned int get_serial_irq(ISASuperIODevice *sio, uint8_t index)
     return index ? 3 : 4;
 }
 
-/* Parallel port */
+/* Parallel port (EPP and ECP support) */
 
 static bool is_parallel_enabled(ISASuperIODevice *sio, uint8_t index)
 {
-    return index < 1;
+    SMC37C669State *s = SMC37C669(sio);
+
+    return extract32(s->cr[1], 2, 1);
 }
 
 static uint16_t get_parallel_iobase(ISASuperIODevice *sio, uint8_t index)
@@ -50,11 +73,13 @@ static unsigned int get_parallel_dma(ISASuperIODevice *sio, uint8_t index)
     return 3;
 }
 
-/* Diskette controller (Software compatible with the Intel PC8477) */
+/* Diskette controller (Intel 82077 compatible) */
 
 static bool is_fdc_enabled(ISASuperIODevice *sio, uint8_t index)
 {
-    return index < 1;
+    SMC37C669State *s = SMC37C669(sio);
+
+    return extract32(s->cr[0], 3, 1);
 }
 
 static uint16_t get_fdc_iobase(ISASuperIODevice *sio, uint8_t index)
@@ -72,10 +97,43 @@ static unsigned int get_fdc_dma(ISASuperIODevice *sio, uint8_t index)
     return 2;
 }
 
+static void smc37c669_reset(DeviceState *d)
+{
+    SMC37C669State *s = SMC37C669(d);
+
+    stl_he_p(s->cr, s->config);
+}
+
+static void smc37c669_realize(DeviceState *dev, Error **errp)
+{
+    ISASuperIOClass *sc = ISA_SUPERIO_GET_CLASS(dev);
+    Error *local_err = NULL;
+
+    smc37c669_reset(dev);
+
+    sc->parent_realize(dev, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+}
+
+static Property smc37c669_properties[] = {
+    DEFINE_PROP_UINT32("config", SMC37C669State, config, 0x78889c28),
+    DEFINE_PROP_BIT("parallel", SMC37C669State, config, 8 + 2, true),
+    DEFINE_PROP_END_OF_LIST()
+};
+
 static void smc37c669_class_init(ObjectClass *klass, void *data)
 {
+    DeviceClass *dc = DEVICE_CLASS(klass);
     ISASuperIOClass *sc = ISA_SUPERIO_CLASS(klass);
 
+    sc->parent_realize = dc->realize;
+    dc->realize = smc37c669_realize;
+    dc->reset = smc37c669_reset;
+    dc->props = smc37c669_properties;
+
     sc->parallel = (ISASuperIOFuncs){
         .count = 1,
         .is_enabled = is_parallel_enabled,
@@ -96,13 +154,12 @@ static void smc37c669_class_init(ObjectClass *klass, void *data)
         .get_irq    = get_fdc_irq,
         .get_dma    = get_fdc_dma,
     };
-    sc->ide.count = 0;
 }
 
 static const TypeInfo smc37c669_type_info = {
     .name          = TYPE_SMC37C669_SUPERIO,
     .parent        = TYPE_ISA_SUPERIO,
-    .instance_size = sizeof(ISASuperIODevice),
+    .instance_size = sizeof(SMC37C669State),
     .class_size    = sizeof(ISASuperIOClass),
     .class_init    = smc37c669_class_init,
 };
-- 
2.17.1

Re: [Qemu-devel] [RFC PATCH 1/2] hw/isa/smc37c669-superio: Basic 'Config Registers' implementation

[Paolo Bonzini]

On 14/06/2018 20:01, Philippe Mathieu-Daudé wrote:
> +
> +static Property smc37c669_properties[] = {
> +    DEFINE_PROP_UINT32("config", SMC37C669State, config, 0x78889c28),
> +    DEFINE_PROP_BIT("parallel", SMC37C669State, config, 8 + 2, true),
> +    DEFINE_PROP_END_OF_LIST()
> +};

I would initialize the config word in instance_init rather
than using a property.  Having overlapping properties for the same field
sounds like a recipe for undesired behavior.

But why isn't the parallel port at 0x378?  That's the expected place on
PC (the second parallel port is at 0x278 and the third is at 0x3bc), and
I would expect other SuperIO chips to have it there too.  That would be
a one line fix.

Yet another possibility is to disable EPP on the parallel port, so that
it occupies only the addresses from 0x3bc to 0x3be.

Paolo

Re: [Qemu-devel] [RFC PATCH 1/2] hw/isa/smc37c669-superio: Basic 'Config Registers' implementation

[Richard Henderson]

On 06/14/2018 08:19 AM, Paolo Bonzini wrote:
> But why isn't the parallel port at 0x378?  That's the expected place on
> PC (the second parallel port is at 0x278 and the third is at 0x3bc), and
> I would expect other SuperIO chips to have it there too.  That would be
> a one line fix.

Agreed.


r~

Re: [Qemu-devel] [RFC PATCH 1/2] hw/isa/smc37c669-superio: Basic 'Config Registers' implementation

[Paolo Bonzini]

On 14/06/2018 23:14, Richard Henderson wrote:
> On 06/14/2018 08:19 AM, Paolo Bonzini wrote:
>> But why isn't the parallel port at 0x378?  That's the expected place on
>> PC (the second parallel port is at 0x278 and the third is at 0x3bc), and
>> I would expect other SuperIO chips to have it there too.  That would be
>> a one line fix.
> 
> Agreed.

FWIW the datasheet says that the base parallel port address is
configurable using the config registers, and can be placed between 0x100
(inclusive) and 0x400 (exclusive), in 4-bytes increments if EPP is
disabled and in 8-byte increments if it is enabled.  Therefore, 0x378
sounds like something that PALcode would probably use.

Paolo

[Qemu-devel] [RFC PATCH 2/2] hw/alpha/dp264: Disable the Super I/O parallel port

[Philippe Mathieu-Daudé]

The 3BCh I/O address was used by "Parallel Ports which were
incorporated on to Video Cards" and "has now reappeared as
an option for Parallel Ports integrated onto motherboards,
upon which their configuration can be changed using BIOS."

The real PALcode is expected to configure the Super I/O and
disable the parallel port, to allow the 3c0-3cf range to be
assigned to the Cirrus VGA.

This fixes an issue introduced in a4cb773928e where the SIO
bind the parallel port in the address range used by the VGA
device, resulting in overlap:

(qemu) info mtree
address-space: memory
  0000000000000000-ffffffffffffffff (prio 0, i/o): system
    00000801fc000000-00000801fdffffff (prio 0, i/o): pci0-io
      ...
      00000801fc0003b4-00000801fc0003b5 (prio 0, i/o): vga
      00000801fc0003ba-00000801fc0003ba (prio 0, i/o): vga
      00000801fc0003bc-00000801fc0003c3 (prio 0, i/o): parallel
                                    ^^^                ^^^^^^^^
      00000801fc0003c0-00000801fc0003cf (prio 0, i/o): vga
                   ^^^
      00000801fc0003d4-00000801fc0003d5 (prio 0, i/o): vga
      00000801fc0003da-00000801fc0003da (prio 0, i/o): vga
      ...

Reported-by: Emilio G. Cota <cota@braap.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/alpha/dp264.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
index 80b987f7fb..87504add8e 100644
--- a/hw/alpha/dp264.c
+++ b/hw/alpha/dp264.c
@@ -58,6 +58,7 @@ static void clipper_init(MachineState *machine)
     AlphaCPU *cpus[4];
     PCIBus *pci_bus;
     ISABus *isa_bus;
+    DeviceState *superio;
     qemu_irq rtc_irq;
     long size, i;
     char *palcode_filename;
@@ -95,7 +96,10 @@ static void clipper_init(MachineState *machine)
     isa_create_simple(isa_bus, "i82374");
 
     /* Super I/O */
-    isa_create_simple(isa_bus, TYPE_SMC37C669_SUPERIO);
+    superio = DEVICE(isa_create(isa_bus, TYPE_SMC37C669_SUPERIO));
+    /* Real PALcode configures the Super I/O and disable the parallel port */
+    qdev_prop_set_bit(superio, "parallel", false);
+    qdev_init_nofail(superio);
 
     /* IDE disk setup.  */
     {
-- 
2.17.1