[PATCH v4 00/11] hw/isa/vt82c686: Implement relocation and toggling of SuperI/O functions

[PATCH v4 00/11] hw/isa/vt82c686: Implement relocation and toggling of SuperI/O functions

[Bernhard Beschow]

This series implements relocation of the SuperI/O functions of the VIA south
bridges which resolves some FIXME's. It is part of my via-apollo-pro-133t
branch [1] which is an extension of bringing the VIA south bridges to the PC
machine [2]. This branch is able to run some real-world X86 BIOSes in the hope
that it allows us to form a better understanding of the real vt82c686b devices.
Implementing relocation and toggling of the SuperI/O functions is one step to
make these BIOSes run without error messages, so here we go.

The series is structured as follows: Patches 1-3 prepare the TYPE_ISA_FDC,
TYPE_ISA_PARALLEL and TYPE_ISA_SERIAL to relocate and toggle (enable/disable)
themselves without breaking encapsulation of their respective device states.
This is achieved by moving the MemoryRegions and PortioLists from the device
states into the encapsulating ISA devices since they will be relocated and
toggled.

Inspired by the memory API patches 4-6 add two convenience functions to the
portio_list API to toggle and relocate portio lists. Patch 5 is a preparation
for that which removes some redundancies which otherwise had to be dealt with
during relocation.

Patches 7-9 implement toggling and relocation for types TYPE_ISA_FDC,
TYPE_ISA_PARALLEL and TYPE_ISA_SERIAL. Patch 10 prepares the pegasos2 machine
which would end up with all SuperI/O functions disabled if no -bios argument is
given. Patch 11 finally implements the main feature which now relies on
firmware to configure the SuperI/O functions accordingly (except for pegasos2).

v4:
* Drop incomplete SuperI/O vmstate handling (Zoltan)

v3:
* Rework various commit messages (Zoltan)
* Drop patch "hw/char/serial: Free struct SerialState from MemoryRegion"
  (Zoltan)
* Generalize wording in migration.rst to include portio_list API (Zoltan)

v2:
* Improve commit messages (Zoltan)
* Split pegasos2 from vt82c686 patch (Zoltan)
* Avoid poking into device internals (Zoltan)

Testing done:
* `make check`
* `make check-avocado`
* Run MorphOS on pegasos2 with and without pegasos2.rom
* Run Linux on amigaone
* Run real-world BIOSes on via-apollo-pro-133t branch
* Start rescue-yl on fuloong2e

[1] https://github.com/shentok/qemu/tree/via-apollo-pro-133t
[2] https://github.com/shentok/qemu/tree/pc-via

Bernhard Beschow (11):
  hw/block/fdc-isa: Move portio_list from FDCtrl to FDCtrlISABus
  hw/block/fdc-sysbus: Move iomem from FDCtrl to FDCtrlSysBus
  hw/char/parallel: Move portio_list from ParallelState to
    ISAParallelState
  exec/ioport: Resolve redundant .base attribute in struct
    MemoryRegionPortio
  exec/ioport: Add portio_list_set_address()
  exec/ioport: Add portio_list_set_enabled()
  hw/block/fdc-isa: Implement relocation and enabling/disabling for
    TYPE_ISA_FDC
  hw/char/serial-isa: Implement relocation and enabling/disabling for
    TYPE_ISA_SERIAL
  hw/char/parallel-isa: Implement relocation and enabling/disabling for
    TYPE_ISA_PARALLEL
  hw/ppc/pegasos2: Let pegasos2 machine configure SuperI/O functions
  hw/isa/vt82c686: Implement relocation and toggling of SuperI/O
    functions

 docs/devel/migration.rst       |  6 ++--
 hw/block/fdc-internal.h        |  4 ---
 include/exec/ioport.h          |  4 ++-
 include/hw/block/fdc.h         |  3 ++
 include/hw/char/parallel-isa.h |  5 +++
 include/hw/char/parallel.h     |  2 --
 include/hw/char/serial.h       |  2 ++
 hw/block/fdc-isa.c             | 18 +++++++++-
 hw/block/fdc-sysbus.c          |  6 ++--
 hw/char/parallel-isa.c         | 14 ++++++++
 hw/char/parallel.c             |  2 +-
 hw/char/serial-isa.c           | 14 ++++++++
 hw/isa/vt82c686.c              | 66 ++++++++++++++++++++++++++++------
 hw/ppc/pegasos2.c              | 15 ++++++++
 system/ioport.c                | 41 +++++++++++++++++----
 15 files changed, 172 insertions(+), 30 deletions(-)

-- 
2.43.0

[PATCH v4 01/11] hw/block/fdc-isa: Move portio_list from FDCtrl to FDCtrlISABus

[Bernhard Beschow]

FDCtrl::portio_list isn't used inside FDCtrl context but only inside
FDCtrlISABus context, so move it there.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/block/fdc-internal.h | 2 --
 hw/block/fdc-isa.c      | 4 +++-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/block/fdc-internal.h b/hw/block/fdc-internal.h
index 036392e9fc..fef2bfbbf5 100644
--- a/hw/block/fdc-internal.h
+++ b/hw/block/fdc-internal.h
@@ -26,7 +26,6 @@
 #define HW_BLOCK_FDC_INTERNAL_H
 
 #include "exec/memory.h"
-#include "exec/ioport.h"
 #include "hw/block/block.h"
 #include "hw/block/fdc.h"
 #include "qapi/qapi-types-block.h"
@@ -140,7 +139,6 @@ struct FDCtrl {
     /* Timers state */
     uint8_t timer0;
     uint8_t timer1;
-    PortioList portio_list;
 };
 
 extern const FDFormat fd_formats[];
diff --git a/hw/block/fdc-isa.c b/hw/block/fdc-isa.c
index ad0921c7d3..2d8a98ce7d 100644
--- a/hw/block/fdc-isa.c
+++ b/hw/block/fdc-isa.c
@@ -42,6 +42,7 @@
 #include "sysemu/block-backend.h"
 #include "sysemu/blockdev.h"
 #include "sysemu/sysemu.h"
+#include "exec/ioport.h"
 #include "qemu/log.h"
 #include "qemu/main-loop.h"
 #include "qemu/module.h"
@@ -60,6 +61,7 @@ struct FDCtrlISABus {
     uint32_t irq;
     uint32_t dma;
     struct FDCtrl state;
+    PortioList portio_list;
     int32_t bootindexA;
     int32_t bootindexB;
 };
@@ -91,7 +93,7 @@ static void isabus_fdc_realize(DeviceState *dev, Error **errp)
     FDCtrl *fdctrl = &isa->state;
     Error *err = NULL;
 
-    isa_register_portio_list(isadev, &fdctrl->portio_list,
+    isa_register_portio_list(isadev, &isa->portio_list,
                              isa->iobase, fdc_portio_list, fdctrl,
                              "fdc");
 
-- 
2.43.0

[PATCH v4 02/11] hw/block/fdc-sysbus: Move iomem from FDCtrl to FDCtrlSysBus

[Bernhard Beschow]

FDCtrl::iomem isn't used inside FDCtrl context but only inside FDCtrlSysBus
context, so move it there.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/block/fdc-internal.h | 2 --
 hw/block/fdc-sysbus.c   | 6 ++++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/block/fdc-internal.h b/hw/block/fdc-internal.h
index fef2bfbbf5..e219623dc7 100644
--- a/hw/block/fdc-internal.h
+++ b/hw/block/fdc-internal.h
@@ -25,7 +25,6 @@
 #ifndef HW_BLOCK_FDC_INTERNAL_H
 #define HW_BLOCK_FDC_INTERNAL_H
 
-#include "exec/memory.h"
 #include "hw/block/block.h"
 #include "hw/block/fdc.h"
 #include "qapi/qapi-types-block.h"
@@ -91,7 +90,6 @@ typedef struct FDrive {
 } FDrive;
 
 struct FDCtrl {
-    MemoryRegion iomem;
     qemu_irq irq;
     /* Controller state */
     QEMUTimer *result_timer;
diff --git a/hw/block/fdc-sysbus.c b/hw/block/fdc-sysbus.c
index 266bc4d145..035bc08975 100644
--- a/hw/block/fdc-sysbus.c
+++ b/hw/block/fdc-sysbus.c
@@ -26,6 +26,7 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "qom/object.h"
+#include "exec/memory.h"
 #include "hw/sysbus.h"
 #include "hw/block/fdc.h"
 #include "migration/vmstate.h"
@@ -52,6 +53,7 @@ struct FDCtrlSysBus {
     /*< public >*/
 
     struct FDCtrl state;
+    MemoryRegion iomem;
 };
 
 static uint64_t fdctrl_read_mem(void *opaque, hwaddr reg, unsigned ize)
@@ -146,11 +148,11 @@ static void sysbus_fdc_common_instance_init(Object *obj)
 
     qdev_set_legacy_instance_id(dev, 0 /* io */, 2); /* FIXME */
 
-    memory_region_init_io(&fdctrl->iomem, obj,
+    memory_region_init_io(&sys->iomem, obj,
                           sbdc->use_strict_io ? &fdctrl_mem_strict_ops
                                               : &fdctrl_mem_ops,
                           fdctrl, "fdc", 0x08);
-    sysbus_init_mmio(sbd, &fdctrl->iomem);
+    sysbus_init_mmio(sbd, &sys->iomem);
 
     sysbus_init_irq(sbd, &fdctrl->irq);
     qdev_init_gpio_in(dev, fdctrl_handle_tc, 1);
-- 
2.43.0

[PATCH v4 03/11] hw/char/parallel: Move portio_list from ParallelState to ISAParallelState

[Bernhard Beschow]

ParallelState::portio_list isn't used inside ParallelState context but only
inside ISAParallelState context, so move it there.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 include/hw/char/parallel-isa.h | 2 ++
 include/hw/char/parallel.h     | 2 --
 hw/char/parallel.c             | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/hw/char/parallel-isa.h b/include/hw/char/parallel-isa.h
index d24ccecf05..3b783bd08d 100644
--- a/include/hw/char/parallel-isa.h
+++ b/include/hw/char/parallel-isa.h
@@ -12,6 +12,7 @@
 
 #include "parallel.h"
 
+#include "exec/ioport.h"
 #include "hw/isa/isa.h"
 #include "qom/object.h"
 
@@ -25,6 +26,7 @@ struct ISAParallelState {
     uint32_t iobase;
     uint32_t isairq;
     ParallelState state;
+    PortioList portio_list;
 };
 
 #endif /* HW_PARALLEL_ISA_H */
diff --git a/include/hw/char/parallel.h b/include/hw/char/parallel.h
index 7b5a309a03..cfb97cc7cc 100644
--- a/include/hw/char/parallel.h
+++ b/include/hw/char/parallel.h
@@ -1,7 +1,6 @@
 #ifndef HW_PARALLEL_H
 #define HW_PARALLEL_H
 
-#include "exec/ioport.h"
 #include "exec/memory.h"
 #include "hw/isa/isa.h"
 #include "hw/irq.h"
@@ -22,7 +21,6 @@ typedef struct ParallelState {
     uint32_t last_read_offset; /* For debugging */
     /* Memory-mapped interface */
     int it_shift;
-    PortioList portio_list;
 } ParallelState;
 
 void parallel_hds_isa_init(ISABus *bus, int n);
diff --git a/hw/char/parallel.c b/hw/char/parallel.c
index bd488cd7f9..c394635ada 100644
--- a/hw/char/parallel.c
+++ b/hw/char/parallel.c
@@ -532,7 +532,7 @@ static void parallel_isa_realizefn(DeviceState *dev, Error **errp)
         s->status = dummy;
     }
 
-    isa_register_portio_list(isadev, &s->portio_list, base,
+    isa_register_portio_list(isadev, &isa->portio_list, base,
                              (s->hw_driver
                               ? &isa_parallel_portio_hw_list[0]
                               : &isa_parallel_portio_sw_list[0]),
-- 
2.43.0

[PATCH v4 04/11] exec/ioport: Resolve redundant .base attribute in struct MemoryRegionPortio

[Bernhard Beschow]

portio_list_add_1() creates a MemoryRegionPortioList instance which holds a
MemoryRegion `mr` and an array of MemoryRegionPortio elements named `ports`.
Each element in the array gets assigned the same value for its .base attribute.
The same value also ends up as the .addr attribute of `mr` due to the
memory_region_add_subregion() call. This means that all .base attributes are
the same as `mr.addr`.

The only usages of MemoryRegionPortio::base were in portio_read() and
portio_write(). Both functions get above MemoryRegionPortioList as their
opaque parameter. In both cases find_portio() can only return one of the
MemoryRegionPortio elements of the `ports` array. Due to above observation any
element will have the same .base value equal to `mr.addr` which is also
accessible.

Hence, `mrpio->mr.addr` is equivalent to `mrp->base` and
MemoryRegionPortio::base is redundant and can be removed.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/exec/ioport.h |  1 -
 system/ioport.c       | 13 ++++++-------
 2 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/include/exec/ioport.h b/include/exec/ioport.h
index e34f668998..95f1dc30d0 100644
--- a/include/exec/ioport.h
+++ b/include/exec/ioport.h
@@ -35,7 +35,6 @@ typedef struct MemoryRegionPortio {
     unsigned size;
     uint32_t (*read)(void *opaque, uint32_t address);
     void (*write)(void *opaque, uint32_t address, uint32_t data);
-    uint32_t base; /* private field */
 } MemoryRegionPortio;
 
 #define PORTIO_END_OF_LIST() { }
diff --git a/system/ioport.c b/system/ioport.c
index 1824aa808c..a59e58b716 100644
--- a/system/ioport.c
+++ b/system/ioport.c
@@ -181,13 +181,13 @@ static uint64_t portio_read(void *opaque, hwaddr addr, unsigned size)
 
     data = ((uint64_t)1 << (size * 8)) - 1;
     if (mrp) {
-        data = mrp->read(mrpio->portio_opaque, mrp->base + addr);
+        data = mrp->read(mrpio->portio_opaque, mrpio->mr.addr + addr);
     } else if (size == 2) {
         mrp = find_portio(mrpio, addr, 1, false);
         if (mrp) {
-            data = mrp->read(mrpio->portio_opaque, mrp->base + addr);
+            data = mrp->read(mrpio->portio_opaque, mrpio->mr.addr + addr);
             if (addr + 1 < mrp->offset + mrp->len) {
-                data |= mrp->read(mrpio->portio_opaque, mrp->base + addr + 1) << 8;
+                data |= mrp->read(mrpio->portio_opaque, mrpio->mr.addr + addr + 1) << 8;
             } else {
                 data |= 0xff00;
             }
@@ -203,13 +203,13 @@ static void portio_write(void *opaque, hwaddr addr, uint64_t data,
     const MemoryRegionPortio *mrp = find_portio(mrpio, addr, size, true);
 
     if (mrp) {
-        mrp->write(mrpio->portio_opaque, mrp->base + addr, data);
+        mrp->write(mrpio->portio_opaque, mrpio->mr.addr + addr, data);
     } else if (size == 2) {
         mrp = find_portio(mrpio, addr, 1, true);
         if (mrp) {
-            mrp->write(mrpio->portio_opaque, mrp->base + addr, data & 0xff);
+            mrp->write(mrpio->portio_opaque, mrpio->mr.addr + addr, data & 0xff);
             if (addr + 1 < mrp->offset + mrp->len) {
-                mrp->write(mrpio->portio_opaque, mrp->base + addr + 1, data >> 8);
+                mrp->write(mrpio->portio_opaque, mrpio->mr.addr + addr + 1, data >> 8);
             }
         }
     }
@@ -244,7 +244,6 @@ static void portio_list_add_1(PortioList *piolist,
     /* Adjust the offsets to all be zero-based for the region.  */
     for (i = 0; i < count; ++i) {
         mrpio->ports[i].offset -= off_low;
-        mrpio->ports[i].base = start + off_low;
     }
 
     /*
-- 
2.43.0

[PATCH v4 05/11] exec/ioport: Add portio_list_set_address()

[Bernhard Beschow]

Some SuperI/O devices such as the VIA south bridges or the PC87312 controller
are able to relocate their SuperI/O functions. Add a convenience function for
implementing this in the VIA south bridges.

This convenience function relies on previous simplifications in exec/ioport
which avoids some duplicate synchronization of I/O port base addresses. The
naming of the function is inspired by its memory_region_set_address() pendant.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 docs/devel/migration.rst |  5 +++--
 include/exec/ioport.h    |  2 ++
 system/ioport.c          | 19 +++++++++++++++++++
 3 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/docs/devel/migration.rst b/docs/devel/migration.rst
index 95351ba51f..30b05f0f74 100644
--- a/docs/devel/migration.rst
+++ b/docs/devel/migration.rst
@@ -452,10 +452,10 @@ data doesn't match the stored device data well; it allows an
 intermediate temporary structure to be populated with migration
 data and then transferred to the main structure.
 
-If you use memory API functions that update memory layout outside
+If you use memory or portio_list API functions that update memory layout outside
 initialization (i.e., in response to a guest action), this is a strong
 indication that you need to call these functions in a ``post_load`` callback.
-Examples of such memory API functions are:
+Examples of such API functions are:
 
   - memory_region_add_subregion()
   - memory_region_del_subregion()
@@ -464,6 +464,7 @@ Examples of such memory API functions are:
   - memory_region_set_enabled()
   - memory_region_set_address()
   - memory_region_set_alias_offset()
+  - portio_list_set_address()
 
 Iterative device migration
 --------------------------
diff --git a/include/exec/ioport.h b/include/exec/ioport.h
index 95f1dc30d0..96858e5ac3 100644
--- a/include/exec/ioport.h
+++ b/include/exec/ioport.h
@@ -54,6 +54,7 @@ typedef struct PortioList {
     const struct MemoryRegionPortio *ports;
     Object *owner;
     struct MemoryRegion *address_space;
+    uint32_t addr;
     unsigned nr;
     struct MemoryRegion **regions;
     void *opaque;
@@ -70,5 +71,6 @@ void portio_list_add(PortioList *piolist,
                      struct MemoryRegion *address_space,
                      uint32_t addr);
 void portio_list_del(PortioList *piolist);
+void portio_list_set_address(PortioList *piolist, uint32_t addr);
 
 #endif /* IOPORT_H */
diff --git a/system/ioport.c b/system/ioport.c
index a59e58b716..000e0ee1af 100644
--- a/system/ioport.c
+++ b/system/ioport.c
@@ -133,6 +133,7 @@ void portio_list_init(PortioList *piolist,
     piolist->nr = 0;
     piolist->regions = g_new0(MemoryRegion *, n);
     piolist->address_space = NULL;
+    piolist->addr = 0;
     piolist->opaque = opaque;
     piolist->owner = owner;
     piolist->name = name;
@@ -282,6 +283,7 @@ void portio_list_add(PortioList *piolist,
     unsigned int off_low, off_high, off_last, count;
 
     piolist->address_space = address_space;
+    piolist->addr = start;
 
     /* Handle the first entry specially.  */
     off_last = off_low = pio_start->offset;
@@ -322,6 +324,23 @@ void portio_list_del(PortioList *piolist)
     }
 }
 
+void portio_list_set_address(PortioList *piolist, uint32_t addr)
+{
+    MemoryRegionPortioList *mrpio;
+    unsigned i, j;
+
+    for (i = 0; i < piolist->nr; ++i) {
+        mrpio = container_of(piolist->regions[i], MemoryRegionPortioList, mr);
+        memory_region_set_address(&mrpio->mr,
+                                  mrpio->mr.addr - piolist->addr + addr);
+        for (j = 0; mrpio->ports[j].size; ++j) {
+            mrpio->ports[j].offset += addr - piolist->addr;
+        }
+    }
+
+    piolist->addr = addr;
+}
+
 static void memory_region_portio_list_finalize(Object *obj)
 {
     MemoryRegionPortioList *mrpio = MEMORY_REGION_PORTIO_LIST(obj);
-- 
2.43.0

[PATCH v4 06/11] exec/ioport: Add portio_list_set_enabled()

[Bernhard Beschow]

Some SuperI/O devices such as the VIA south bridges or the PC87312 controller
allow to enable or disable their SuperI/O functions. Add a convenience function
for implementing this in the VIA south bridges.

The naming of the functions is inspired by its memory_region_set_enabled()
pendant.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 docs/devel/migration.rst | 1 +
 include/exec/ioport.h    | 1 +
 system/ioport.c          | 9 +++++++++
 3 files changed, 11 insertions(+)

diff --git a/docs/devel/migration.rst b/docs/devel/migration.rst
index 30b05f0f74..1683fc6026 100644
--- a/docs/devel/migration.rst
+++ b/docs/devel/migration.rst
@@ -465,6 +465,7 @@ Examples of such API functions are:
   - memory_region_set_address()
   - memory_region_set_alias_offset()
   - portio_list_set_address()
+  - portio_list_set_enabled()
 
 Iterative device migration
 --------------------------
diff --git a/include/exec/ioport.h b/include/exec/ioport.h
index 96858e5ac3..4397f12f93 100644
--- a/include/exec/ioport.h
+++ b/include/exec/ioport.h
@@ -71,6 +71,7 @@ void portio_list_add(PortioList *piolist,
                      struct MemoryRegion *address_space,
                      uint32_t addr);
 void portio_list_del(PortioList *piolist);
+void portio_list_set_enabled(PortioList *piolist, bool enabled);
 void portio_list_set_address(PortioList *piolist, uint32_t addr);
 
 #endif /* IOPORT_H */
diff --git a/system/ioport.c b/system/ioport.c
index 000e0ee1af..fd551d0375 100644
--- a/system/ioport.c
+++ b/system/ioport.c
@@ -324,6 +324,15 @@ void portio_list_del(PortioList *piolist)
     }
 }
 
+void portio_list_set_enabled(PortioList *piolist, bool enabled)
+{
+    unsigned i;
+
+    for (i = 0; i < piolist->nr; ++i) {
+        memory_region_set_enabled(piolist->regions[i], enabled);
+    }
+}
+
 void portio_list_set_address(PortioList *piolist, uint32_t addr)
 {
     MemoryRegionPortioList *mrpio;
-- 
2.43.0

[PATCH v4 07/11] hw/block/fdc-isa: Implement relocation and enabling/disabling for TYPE_ISA_FDC

[Bernhard Beschow]

The real SuperI/O chips emulated by QEMU allow for relocating and enabling or
disabling their SuperI/O functions via software. So far this is not implemented.
Prepare for that by adding isa_fdc_set_{enabled,iobase}.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/block/fdc.h |  3 +++
 hw/block/fdc-isa.c     | 14 ++++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/include/hw/block/fdc.h b/include/hw/block/fdc.h
index 35248c0837..c367c5efea 100644
--- a/include/hw/block/fdc.h
+++ b/include/hw/block/fdc.h
@@ -14,6 +14,9 @@ void fdctrl_init_sysbus(qemu_irq irq, hwaddr mmio_base, DriveInfo **fds);
 void sun4m_fdctrl_init(qemu_irq irq, hwaddr io_base,
                        DriveInfo **fds, qemu_irq *fdc_tc);
 
+void isa_fdc_set_iobase(ISADevice *fdc, hwaddr iobase);
+void isa_fdc_set_enabled(ISADevice *fdc, bool enabled);
+
 FloppyDriveType isa_fdc_get_drive_type(ISADevice *fdc, int i);
 int cmos_get_fd_drive_type(FloppyDriveType fd0);
 
diff --git a/hw/block/fdc-isa.c b/hw/block/fdc-isa.c
index 2d8a98ce7d..e43dc532af 100644
--- a/hw/block/fdc-isa.c
+++ b/hw/block/fdc-isa.c
@@ -192,6 +192,20 @@ static Aml *build_fdinfo_aml(int idx, FloppyDriveType type)
     return dev;
 }
 
+void isa_fdc_set_iobase(ISADevice *fdc, hwaddr iobase)
+{
+    FDCtrlISABus *isa = ISA_FDC(fdc);
+
+    fdc->ioport_id = iobase;
+    isa->iobase = iobase;
+    portio_list_set_address(&isa->portio_list, isa->iobase);
+}
+
+void isa_fdc_set_enabled(ISADevice *fdc, bool enabled)
+{
+    portio_list_set_enabled(&ISA_FDC(fdc)->portio_list, enabled);
+}
+
 int cmos_get_fd_drive_type(FloppyDriveType fd0)
 {
     int val;
-- 
2.43.0

[PATCH v4 08/11] hw/char/serial-isa: Implement relocation and enabling/disabling for TYPE_ISA_SERIAL

[Bernhard Beschow]

The real SuperI/O chips emulated by QEMU allow for relocating and enabling or
disabling their SuperI/O functions via software. So far this is not implemented.
Prepare for that by adding isa_serial_set_{enabled,iobase}.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/char/serial.h |  2 ++
 hw/char/serial-isa.c     | 14 ++++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
index 8ba7eca3d6..6e14099ee7 100644
--- a/include/hw/char/serial.h
+++ b/include/hw/char/serial.h
@@ -112,5 +112,7 @@ SerialMM *serial_mm_init(MemoryRegion *address_space,
 
 #define TYPE_ISA_SERIAL "isa-serial"
 void serial_hds_isa_init(ISABus *bus, int from, int to);
+void isa_serial_set_iobase(ISADevice *serial, hwaddr iobase);
+void isa_serial_set_enabled(ISADevice *serial, bool enabled);
 
 #endif
diff --git a/hw/char/serial-isa.c b/hw/char/serial-isa.c
index 1c793b20f7..329b352b9a 100644
--- a/hw/char/serial-isa.c
+++ b/hw/char/serial-isa.c
@@ -184,3 +184,17 @@ void serial_hds_isa_init(ISABus *bus, int from, int to)
         }
     }
 }
+
+void isa_serial_set_iobase(ISADevice *serial, hwaddr iobase)
+{
+    ISASerialState *s = ISA_SERIAL(serial);
+
+    serial->ioport_id = iobase;
+    s->iobase = iobase;
+    memory_region_set_address(&s->state.io, s->iobase);
+}
+
+void isa_serial_set_enabled(ISADevice *serial, bool enabled)
+{
+    memory_region_set_enabled(&ISA_SERIAL(serial)->state.io, enabled);
+}
-- 
2.43.0

[PATCH v4 09/11] hw/char/parallel-isa: Implement relocation and enabling/disabling for TYPE_ISA_PARALLEL

[Bernhard Beschow]

The real SuperI/O chips emulated by QEMU allow for relocating and enabling or
disabling their SuperI/O functions via software. So far this is not implemented.
Prepare for that by adding isa_parallel_set_{enabled,iobase}.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/char/parallel-isa.h |  3 +++
 hw/char/parallel-isa.c         | 14 ++++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/include/hw/char/parallel-isa.h b/include/hw/char/parallel-isa.h
index 3b783bd08d..5284b2ffec 100644
--- a/include/hw/char/parallel-isa.h
+++ b/include/hw/char/parallel-isa.h
@@ -29,4 +29,7 @@ struct ISAParallelState {
     PortioList portio_list;
 };
 
+void isa_parallel_set_iobase(ISADevice *parallel, hwaddr iobase);
+void isa_parallel_set_enabled(ISADevice *parallel, bool enabled);
+
 #endif /* HW_PARALLEL_ISA_H */
diff --git a/hw/char/parallel-isa.c b/hw/char/parallel-isa.c
index ab0f879998..a5ce6ee13a 100644
--- a/hw/char/parallel-isa.c
+++ b/hw/char/parallel-isa.c
@@ -41,3 +41,17 @@ void parallel_hds_isa_init(ISABus *bus, int n)
         }
     }
 }
+
+void isa_parallel_set_iobase(ISADevice *parallel, hwaddr iobase)
+{
+    ISAParallelState *s = ISA_PARALLEL(parallel);
+
+    parallel->ioport_id = iobase;
+    s->iobase = iobase;
+    portio_list_set_address(&s->portio_list, s->iobase);
+}
+
+void isa_parallel_set_enabled(ISADevice *parallel, bool enabled)
+{
+    portio_list_set_enabled(&ISA_PARALLEL(parallel)->portio_list, enabled);
+}
-- 
2.43.0

[PATCH v4 10/11] hw/ppc/pegasos2: Let pegasos2 machine configure SuperI/O functions

[Bernhard Beschow]

This is a preparation for implementing relocation and toggling of SuperI/O
functions in the VT8231 device model. Upon reset, all SuperI/O functions will be
deactivated, so in case if no -bios is given, let the machine configure those
functions the same way Pegasos II firmware would do.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/ppc/pegasos2.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
index 3203a4a728..0a40ebd542 100644
--- a/hw/ppc/pegasos2.c
+++ b/hw/ppc/pegasos2.c
@@ -285,6 +285,15 @@ static void pegasos2_pci_config_write(Pegasos2MachineState *pm, int bus,
     pegasos2_mv_reg_write(pm, pcicfg + 4, len, val);
 }
 
+static void pegasos2_superio_write(Pegasos2MachineState *pm, uint32_t addr,
+                                   uint32_t val)
+{
+    AddressSpace *as = CPU(pm->cpu)->as;
+
+    stb_phys(as, PCI1_IO_BASE + 0x3f0, addr);
+    stb_phys(as, PCI1_IO_BASE + 0x3f1, val);
+}
+
 static void pegasos2_machine_reset(MachineState *machine, ShutdownCause reason)
 {
     Pegasos2MachineState *pm = PEGASOS2_MACHINE(machine);
@@ -310,6 +319,12 @@ static void pegasos2_machine_reset(MachineState *machine, ShutdownCause reason)
 
     pegasos2_pci_config_write(pm, 1, (PCI_DEVFN(12, 0) << 8) |
                               PCI_INTERRUPT_LINE, 2, 0x9);
+    pegasos2_pci_config_write(pm, 1, (PCI_DEVFN(12, 0) << 8) |
+                              0x50, 1, 0x6);
+    pegasos2_superio_write(pm, 0xf4, 0xbe);
+    pegasos2_superio_write(pm, 0xf6, 0xef);
+    pegasos2_superio_write(pm, 0xf7, 0xfc);
+    pegasos2_superio_write(pm, 0xf2, 0x14);
     pegasos2_pci_config_write(pm, 1, (PCI_DEVFN(12, 0) << 8) |
                               0x50, 1, 0x2);
     pegasos2_pci_config_write(pm, 1, (PCI_DEVFN(12, 0) << 8) |
-- 
2.43.0

[PATCH v4 11/11] hw/isa/vt82c686: Implement relocation and toggling of SuperI/O functions

[Bernhard Beschow]

The VIA south bridges are able to relocate and toggle (enable or disable) their
SuperI/O functions. So far this is hardcoded such that all functions are always
enabled and are located at fixed addresses.

Some PC BIOSes seem to probe for I/O occupancy before activating such a function
and issue an error in case of a conflict. Since the functions are currently
enabled on reset, conflicts are always detected. Prevent that by implementing
relocation and toggling of the SuperI/O functions.

Note that all SuperI/O functions are now deactivated upon reset (except for
VT82C686B's serial ports where Fuloong 2e's rescue-yl seems to expect them to be
enabled by default). Rely on firmware to configure the functions accordingly.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/isa/vt82c686.c | 66 ++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 56 insertions(+), 10 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index d3e0f6d01f..9f62fb5964 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -15,6 +15,9 @@
 
 #include "qemu/osdep.h"
 #include "hw/isa/vt82c686.h"
+#include "hw/block/fdc.h"
+#include "hw/char/parallel-isa.h"
+#include "hw/char/serial.h"
 #include "hw/pci/pci.h"
 #include "hw/qdev-properties.h"
 #include "hw/ide/pci.h"
@@ -323,6 +326,18 @@ static uint64_t via_superio_cfg_read(void *opaque, hwaddr addr, unsigned size)
     return val;
 }
 
+static void via_superio_devices_enable(ViaSuperIOState *s, uint8_t data)
+{
+    ISASuperIOClass *ic = ISA_SUPERIO_GET_CLASS(s);
+    size_t i;
+
+    isa_parallel_set_enabled(s->superio.parallel[0], (data & 0x3) != 3);
+    for (i = 0; i < ic->serial.count; i++) {
+        isa_serial_set_enabled(s->superio.serial[i], data & BIT(i + 2));
+    }
+    isa_fdc_set_enabled(s->superio.floppy, data & BIT(4));
+}
+
 static void via_superio_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -368,7 +383,25 @@ static void vt82c686b_superio_cfg_write(void *opaque, hwaddr addr,
     case 0xfd ... 0xff:
         /* ignore write to read only registers */
         return;
-    /* case 0xe6 ... 0xe8: Should set base port of parallel and serial */
+    case 0xe2:
+        data &= 0x1f;
+        via_superio_devices_enable(sc, data);
+        break;
+    case 0xe3:
+        data &= 0xfc;
+        isa_fdc_set_iobase(sc->superio.floppy, data << 2);
+        break;
+    case 0xe6:
+        isa_parallel_set_iobase(sc->superio.parallel[0], data << 2);
+        break;
+    case 0xe7:
+        data &= 0xfe;
+        isa_serial_set_iobase(sc->superio.serial[0], data << 2);
+        break;
+    case 0xe8:
+        data &= 0xfe;
+        isa_serial_set_iobase(sc->superio.serial[1], data << 2);
+        break;
     default:
         qemu_log_mask(LOG_UNIMP,
                       "via_superio_cfg: unimplemented register 0x%x\n", idx);
@@ -395,9 +428,14 @@ static void vt82c686b_superio_reset(DeviceState *dev)
     /* Device ID */
     vt82c686b_superio_cfg_write(s, 0, 0xe0, 1);
     vt82c686b_superio_cfg_write(s, 1, 0x3c, 1);
-    /* Function select - all disabled */
+    /*
+     * Function select - only serial enabled
+     * Fuloong 2e's rescue-yl prints to the serial console w/o enabling it. This
+     * suggests that the serial ports are enabled by default, so override the
+     * datasheet.
+     */
     vt82c686b_superio_cfg_write(s, 0, 0xe2, 1);
-    vt82c686b_superio_cfg_write(s, 1, 0x03, 1);
+    vt82c686b_superio_cfg_write(s, 1, 0x0f, 1);
     /* Floppy ctrl base addr 0x3f0-7 */
     vt82c686b_superio_cfg_write(s, 0, 0xe3, 1);
     vt82c686b_superio_cfg_write(s, 1, 0xfc, 1);
@@ -465,6 +503,21 @@ static void vt8231_superio_cfg_write(void *opaque, hwaddr addr,
     case 0xfd:
         /* ignore write to read only registers */
         return;
+    case 0xf2:
+        data &= 0x17;
+        via_superio_devices_enable(sc, data);
+        break;
+    case 0xf4:
+        data &= 0xfe;
+        isa_serial_set_iobase(sc->superio.serial[0], data << 2);
+        break;
+    case 0xf6:
+        isa_parallel_set_iobase(sc->superio.parallel[0], data << 2);
+        break;
+    case 0xf7:
+        data &= 0xfc;
+        isa_fdc_set_iobase(sc->superio.floppy, data << 2);
+        break;
     default:
         qemu_log_mask(LOG_UNIMP,
                       "via_superio_cfg: unimplemented register 0x%x\n", idx);
@@ -513,12 +566,6 @@ static void vt8231_superio_init(Object *obj)
     VIA_SUPERIO(obj)->io_ops = &vt8231_superio_cfg_ops;
 }
 
-static uint16_t vt8231_superio_serial_iobase(ISASuperIODevice *sio,
-                                             uint8_t index)
-{
-        return 0x2f8; /* FIXME: This should be settable via registers f2-f4 */
-}
-
 static void vt8231_superio_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -526,7 +573,6 @@ static void vt8231_superio_class_init(ObjectClass *klass, void *data)
 
     dc->reset = vt8231_superio_reset;
     sc->serial.count = 1;
-    sc->serial.get_iobase = vt8231_superio_serial_iobase;
     sc->parallel.count = 1;
     sc->ide.count = 0; /* emulated by via-ide */
     sc->floppy.count = 1;
-- 
2.43.0

Re: [PATCH v4 10/11] hw/ppc/pegasos2: Let pegasos2 machine configure SuperI/O functions

[BALATON Zoltan]

On Sat, 6 Jan 2024, Bernhard Beschow wrote:
> This is a preparation for implementing relocation and toggling of SuperI/O
> functions in the VT8231 device model. Upon reset, all SuperI/O functions will be
> deactivated, so in case if no -bios is given, let the machine configure those
> functions the same way Pegasos II firmware would do.
>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> hw/ppc/pegasos2.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
> index 3203a4a728..0a40ebd542 100644
> --- a/hw/ppc/pegasos2.c
> +++ b/hw/ppc/pegasos2.c
> @@ -285,6 +285,15 @@ static void pegasos2_pci_config_write(Pegasos2MachineState *pm, int bus,
>     pegasos2_mv_reg_write(pm, pcicfg + 4, len, val);
> }
>
> +static void pegasos2_superio_write(Pegasos2MachineState *pm, uint32_t addr,
> +                                   uint32_t val)
> +{
> +    AddressSpace *as = CPU(pm->cpu)->as;

I think this function should not need Pegasos2MachineState *pm and can 
just use cpu_physical_memory_write() instead. Otherwise

Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>

> +
> +    stb_phys(as, PCI1_IO_BASE + 0x3f0, addr);
> +    stb_phys(as, PCI1_IO_BASE + 0x3f1, val);
> +}
> +
> static void pegasos2_machine_reset(MachineState *machine, ShutdownCause reason)
> {
>     Pegasos2MachineState *pm = PEGASOS2_MACHINE(machine);
> @@ -310,6 +319,12 @@ static void pegasos2_machine_reset(MachineState *machine, ShutdownCause reason)
>
>     pegasos2_pci_config_write(pm, 1, (PCI_DEVFN(12, 0) << 8) |
>                               PCI_INTERRUPT_LINE, 2, 0x9);
> +    pegasos2_pci_config_write(pm, 1, (PCI_DEVFN(12, 0) << 8) |
> +                              0x50, 1, 0x6);
> +    pegasos2_superio_write(pm, 0xf4, 0xbe);
> +    pegasos2_superio_write(pm, 0xf6, 0xef);
> +    pegasos2_superio_write(pm, 0xf7, 0xfc);
> +    pegasos2_superio_write(pm, 0xf2, 0x14);
>     pegasos2_pci_config_write(pm, 1, (PCI_DEVFN(12, 0) << 8) |
>                               0x50, 1, 0x2);
>     pegasos2_pci_config_write(pm, 1, (PCI_DEVFN(12, 0) << 8) |
>

Re: [PATCH v4 11/11] hw/isa/vt82c686: Implement relocation and toggling of SuperI/O functions

[BALATON Zoltan]

On Sat, 6 Jan 2024, Bernhard Beschow wrote:
> The VIA south bridges are able to relocate and toggle (enable or disable) their
> SuperI/O functions. So far this is hardcoded such that all functions are always
> enabled and are located at fixed addresses.
>
> Some PC BIOSes seem to probe for I/O occupancy before activating such a function
> and issue an error in case of a conflict. Since the functions are currently
> enabled on reset, conflicts are always detected. Prevent that by implementing
> relocation and toggling of the SuperI/O functions.
>
> Note that all SuperI/O functions are now deactivated upon reset (except for
> VT82C686B's serial ports where Fuloong 2e's rescue-yl seems to expect them to be
> enabled by default). Rely on firmware to configure the functions accordingly.
>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> hw/isa/vt82c686.c | 66 ++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 56 insertions(+), 10 deletions(-)
>
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index d3e0f6d01f..9f62fb5964 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -15,6 +15,9 @@
>
> #include "qemu/osdep.h"
> #include "hw/isa/vt82c686.h"
> +#include "hw/block/fdc.h"
> +#include "hw/char/parallel-isa.h"
> +#include "hw/char/serial.h"
> #include "hw/pci/pci.h"
> #include "hw/qdev-properties.h"
> #include "hw/ide/pci.h"
> @@ -323,6 +326,18 @@ static uint64_t via_superio_cfg_read(void *opaque, hwaddr addr, unsigned size)
>     return val;
> }
>
> +static void via_superio_devices_enable(ViaSuperIOState *s, uint8_t data)
> +{
> +    ISASuperIOClass *ic = ISA_SUPERIO_GET_CLASS(s);
> +    size_t i;

The expected value for i is 0 or 1 (maybe up to 3 sometimes it there are 
more serial ports in a chip). so why use such big type? This should just 
be int. Newly it's also allowed to declare it within the for so if you 
want that you could do so but I have no preference on that and declaring 
it here is also OK. Otherwise:

Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>

> +
> +    isa_parallel_set_enabled(s->superio.parallel[0], (data & 0x3) != 3);
> +    for (i = 0; i < ic->serial.count; i++) {
> +        isa_serial_set_enabled(s->superio.serial[i], data & BIT(i + 2));
> +    }
> +    isa_fdc_set_enabled(s->superio.floppy, data & BIT(4));
> +}
> +
> static void via_superio_class_init(ObjectClass *klass, void *data)
> {
>     DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -368,7 +383,25 @@ static void vt82c686b_superio_cfg_write(void *opaque, hwaddr addr,
>     case 0xfd ... 0xff:
>         /* ignore write to read only registers */
>         return;
> -    /* case 0xe6 ... 0xe8: Should set base port of parallel and serial */
> +    case 0xe2:
> +        data &= 0x1f;
> +        via_superio_devices_enable(sc, data);
> +        break;
> +    case 0xe3:
> +        data &= 0xfc;
> +        isa_fdc_set_iobase(sc->superio.floppy, data << 2);
> +        break;
> +    case 0xe6:
> +        isa_parallel_set_iobase(sc->superio.parallel[0], data << 2);
> +        break;
> +    case 0xe7:
> +        data &= 0xfe;
> +        isa_serial_set_iobase(sc->superio.serial[0], data << 2);
> +        break;
> +    case 0xe8:
> +        data &= 0xfe;
> +        isa_serial_set_iobase(sc->superio.serial[1], data << 2);
> +        break;
>     default:
>         qemu_log_mask(LOG_UNIMP,
>                       "via_superio_cfg: unimplemented register 0x%x\n", idx);
> @@ -395,9 +428,14 @@ static void vt82c686b_superio_reset(DeviceState *dev)
>     /* Device ID */
>     vt82c686b_superio_cfg_write(s, 0, 0xe0, 1);
>     vt82c686b_superio_cfg_write(s, 1, 0x3c, 1);
> -    /* Function select - all disabled */
> +    /*
> +     * Function select - only serial enabled
> +     * Fuloong 2e's rescue-yl prints to the serial console w/o enabling it. This
> +     * suggests that the serial ports are enabled by default, so override the
> +     * datasheet.
> +     */
>     vt82c686b_superio_cfg_write(s, 0, 0xe2, 1);
> -    vt82c686b_superio_cfg_write(s, 1, 0x03, 1);
> +    vt82c686b_superio_cfg_write(s, 1, 0x0f, 1);
>     /* Floppy ctrl base addr 0x3f0-7 */
>     vt82c686b_superio_cfg_write(s, 0, 0xe3, 1);
>     vt82c686b_superio_cfg_write(s, 1, 0xfc, 1);
> @@ -465,6 +503,21 @@ static void vt8231_superio_cfg_write(void *opaque, hwaddr addr,
>     case 0xfd:
>         /* ignore write to read only registers */
>         return;
> +    case 0xf2:
> +        data &= 0x17;
> +        via_superio_devices_enable(sc, data);
> +        break;
> +    case 0xf4:
> +        data &= 0xfe;
> +        isa_serial_set_iobase(sc->superio.serial[0], data << 2);
> +        break;
> +    case 0xf6:
> +        isa_parallel_set_iobase(sc->superio.parallel[0], data << 2);
> +        break;
> +    case 0xf7:
> +        data &= 0xfc;
> +        isa_fdc_set_iobase(sc->superio.floppy, data << 2);
> +        break;
>     default:
>         qemu_log_mask(LOG_UNIMP,
>                       "via_superio_cfg: unimplemented register 0x%x\n", idx);
> @@ -513,12 +566,6 @@ static void vt8231_superio_init(Object *obj)
>     VIA_SUPERIO(obj)->io_ops = &vt8231_superio_cfg_ops;
> }
>
> -static uint16_t vt8231_superio_serial_iobase(ISASuperIODevice *sio,
> -                                             uint8_t index)
> -{
> -        return 0x2f8; /* FIXME: This should be settable via registers f2-f4 */
> -}
> -
> static void vt8231_superio_class_init(ObjectClass *klass, void *data)
> {
>     DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -526,7 +573,6 @@ static void vt8231_superio_class_init(ObjectClass *klass, void *data)
>
>     dc->reset = vt8231_superio_reset;
>     sc->serial.count = 1;
> -    sc->serial.get_iobase = vt8231_superio_serial_iobase;
>     sc->parallel.count = 1;
>     sc->ide.count = 0; /* emulated by via-ide */
>     sc->floppy.count = 1;
>

Re: [PATCH v4 00/11] hw/isa/vt82c686: Implement relocation and toggling of SuperI/O functions

[Mark Cave-Ayland]

On 06/01/2024 21:05, Bernhard Beschow wrote:

> This series implements relocation of the SuperI/O functions of the VIA south
> bridges which resolves some FIXME's. It is part of my via-apollo-pro-133t
> branch [1] which is an extension of bringing the VIA south bridges to the PC
> machine [2]. This branch is able to run some real-world X86 BIOSes in the hope
> that it allows us to form a better understanding of the real vt82c686b devices.
> Implementing relocation and toggling of the SuperI/O functions is one step to
> make these BIOSes run without error messages, so here we go.
> 
> The series is structured as follows: Patches 1-3 prepare the TYPE_ISA_FDC,
> TYPE_ISA_PARALLEL and TYPE_ISA_SERIAL to relocate and toggle (enable/disable)
> themselves without breaking encapsulation of their respective device states.
> This is achieved by moving the MemoryRegions and PortioLists from the device
> states into the encapsulating ISA devices since they will be relocated and
> toggled.
> 
> Inspired by the memory API patches 4-6 add two convenience functions to the
> portio_list API to toggle and relocate portio lists. Patch 5 is a preparation
> for that which removes some redundancies which otherwise had to be dealt with
> during relocation.
> 
> Patches 7-9 implement toggling and relocation for types TYPE_ISA_FDC,
> TYPE_ISA_PARALLEL and TYPE_ISA_SERIAL. Patch 10 prepares the pegasos2 machine
> which would end up with all SuperI/O functions disabled if no -bios argument is
> given. Patch 11 finally implements the main feature which now relies on
> firmware to configure the SuperI/O functions accordingly (except for pegasos2).
> 
> v4:
> * Drop incomplete SuperI/O vmstate handling (Zoltan)
> 
> v3:
> * Rework various commit messages (Zoltan)
> * Drop patch "hw/char/serial: Free struct SerialState from MemoryRegion"
>    (Zoltan)
> * Generalize wording in migration.rst to include portio_list API (Zoltan)
> 
> v2:
> * Improve commit messages (Zoltan)
> * Split pegasos2 from vt82c686 patch (Zoltan)
> * Avoid poking into device internals (Zoltan)
> 
> Testing done:
> * `make check`
> * `make check-avocado`
> * Run MorphOS on pegasos2 with and without pegasos2.rom
> * Run Linux on amigaone
> * Run real-world BIOSes on via-apollo-pro-133t branch
> * Start rescue-yl on fuloong2e
> 
> [1] https://github.com/shentok/qemu/tree/via-apollo-pro-133t
> [2] https://github.com/shentok/qemu/tree/pc-via
> 
> Bernhard Beschow (11):
>    hw/block/fdc-isa: Move portio_list from FDCtrl to FDCtrlISABus
>    hw/block/fdc-sysbus: Move iomem from FDCtrl to FDCtrlSysBus
>    hw/char/parallel: Move portio_list from ParallelState to
>      ISAParallelState
>    exec/ioport: Resolve redundant .base attribute in struct
>      MemoryRegionPortio
>    exec/ioport: Add portio_list_set_address()
>    exec/ioport: Add portio_list_set_enabled()
>    hw/block/fdc-isa: Implement relocation and enabling/disabling for
>      TYPE_ISA_FDC
>    hw/char/serial-isa: Implement relocation and enabling/disabling for
>      TYPE_ISA_SERIAL
>    hw/char/parallel-isa: Implement relocation and enabling/disabling for
>      TYPE_ISA_PARALLEL
>    hw/ppc/pegasos2: Let pegasos2 machine configure SuperI/O functions
>    hw/isa/vt82c686: Implement relocation and toggling of SuperI/O
>      functions
> 
>   docs/devel/migration.rst       |  6 ++--
>   hw/block/fdc-internal.h        |  4 ---
>   include/exec/ioport.h          |  4 ++-
>   include/hw/block/fdc.h         |  3 ++
>   include/hw/char/parallel-isa.h |  5 +++
>   include/hw/char/parallel.h     |  2 --
>   include/hw/char/serial.h       |  2 ++
>   hw/block/fdc-isa.c             | 18 +++++++++-
>   hw/block/fdc-sysbus.c          |  6 ++--
>   hw/char/parallel-isa.c         | 14 ++++++++
>   hw/char/parallel.c             |  2 +-
>   hw/char/serial-isa.c           | 14 ++++++++
>   hw/isa/vt82c686.c              | 66 ++++++++++++++++++++++++++++------
>   hw/ppc/pegasos2.c              | 15 ++++++++
>   system/ioport.c                | 41 +++++++++++++++++----
>   15 files changed, 172 insertions(+), 30 deletions(-)

I think this series generally looks good: the only thing I think it's worth checking 
is whether portio lists are considered exclusive to ISA devices or not? (Paolo?).

The portio_list_set_enabled() API looks interesting, and could be considered for use 
by my PCI IDE mode-switching changes too.

Apologies I don't have a huge amount of time for review right now, but I wanted to 
feed back that generally these patches look good, and if people are happy with the 
portio list changes then this series should be considered for merge.


ATB,

Mark.

Re: [PATCH v4 11/11] hw/isa/vt82c686: Implement relocation and toggling of SuperI/O functions

[Bernhard Beschow]

Am 7. Januar 2024 13:59:44 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Sat, 6 Jan 2024, Bernhard Beschow wrote:
>> The VIA south bridges are able to relocate and toggle (enable or disable) their
>> SuperI/O functions. So far this is hardcoded such that all functions are always
>> enabled and are located at fixed addresses.
>> 
>> Some PC BIOSes seem to probe for I/O occupancy before activating such a function
>> and issue an error in case of a conflict. Since the functions are currently
>> enabled on reset, conflicts are always detected. Prevent that by implementing
>> relocation and toggling of the SuperI/O functions.
>> 
>> Note that all SuperI/O functions are now deactivated upon reset (except for
>> VT82C686B's serial ports where Fuloong 2e's rescue-yl seems to expect them to be
>> enabled by default). Rely on firmware to configure the functions accordingly.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>> hw/isa/vt82c686.c | 66 ++++++++++++++++++++++++++++++++++++++++-------
>> 1 file changed, 56 insertions(+), 10 deletions(-)
>> 
>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>> index d3e0f6d01f..9f62fb5964 100644
>> --- a/hw/isa/vt82c686.c
>> +++ b/hw/isa/vt82c686.c
>> @@ -15,6 +15,9 @@
>> 
>> #include "qemu/osdep.h"
>> #include "hw/isa/vt82c686.h"
>> +#include "hw/block/fdc.h"
>> +#include "hw/char/parallel-isa.h"
>> +#include "hw/char/serial.h"
>> #include "hw/pci/pci.h"
>> #include "hw/qdev-properties.h"
>> #include "hw/ide/pci.h"
>> @@ -323,6 +326,18 @@ static uint64_t via_superio_cfg_read(void *opaque, hwaddr addr, unsigned size)
>>     return val;
>> }
>> 
>> +static void via_superio_devices_enable(ViaSuperIOState *s, uint8_t data)
>> +{
>> +    ISASuperIOClass *ic = ISA_SUPERIO_GET_CLASS(s);
>> +    size_t i;
>
>The expected value for i is 0 or 1 (maybe up to 3 sometimes it there are more serial ports in a chip). so why use such big type?

serial.count is of type size_t, that's why I chose it. Let me know if you still want an int, otherwise I'd leave it as is.

Best regards,
Bernhard

> This should just be int. Newly it's also allowed to declare it within the for so if you want that you could do so but I have no preference on that and declaring it here is also OK. Otherwise:
>
>Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>
>
>> +
>> +    isa_parallel_set_enabled(s->superio.parallel[0], (data & 0x3) != 3);
>> +    for (i = 0; i < ic->serial.count; i++) {
>> +        isa_serial_set_enabled(s->superio.serial[i], data & BIT(i + 2));
>> +    }
>> +    isa_fdc_set_enabled(s->superio.floppy, data & BIT(4));
>> +}
>> +
>> static void via_superio_class_init(ObjectClass *klass, void *data)
>> {
>>     DeviceClass *dc = DEVICE_CLASS(klass);
>> @@ -368,7 +383,25 @@ static void vt82c686b_superio_cfg_write(void *opaque, hwaddr addr,
>>     case 0xfd ... 0xff:
>>         /* ignore write to read only registers */
>>         return;
>> -    /* case 0xe6 ... 0xe8: Should set base port of parallel and serial */
>> +    case 0xe2:
>> +        data &= 0x1f;
>> +        via_superio_devices_enable(sc, data);
>> +        break;
>> +    case 0xe3:
>> +        data &= 0xfc;
>> +        isa_fdc_set_iobase(sc->superio.floppy, data << 2);
>> +        break;
>> +    case 0xe6:
>> +        isa_parallel_set_iobase(sc->superio.parallel[0], data << 2);
>> +        break;
>> +    case 0xe7:
>> +        data &= 0xfe;
>> +        isa_serial_set_iobase(sc->superio.serial[0], data << 2);
>> +        break;
>> +    case 0xe8:
>> +        data &= 0xfe;
>> +        isa_serial_set_iobase(sc->superio.serial[1], data << 2);
>> +        break;
>>     default:
>>         qemu_log_mask(LOG_UNIMP,
>>                       "via_superio_cfg: unimplemented register 0x%x\n", idx);
>> @@ -395,9 +428,14 @@ static void vt82c686b_superio_reset(DeviceState *dev)
>>     /* Device ID */
>>     vt82c686b_superio_cfg_write(s, 0, 0xe0, 1);
>>     vt82c686b_superio_cfg_write(s, 1, 0x3c, 1);
>> -    /* Function select - all disabled */
>> +    /*
>> +     * Function select - only serial enabled
>> +     * Fuloong 2e's rescue-yl prints to the serial console w/o enabling it. This
>> +     * suggests that the serial ports are enabled by default, so override the
>> +     * datasheet.
>> +     */
>>     vt82c686b_superio_cfg_write(s, 0, 0xe2, 1);
>> -    vt82c686b_superio_cfg_write(s, 1, 0x03, 1);
>> +    vt82c686b_superio_cfg_write(s, 1, 0x0f, 1);
>>     /* Floppy ctrl base addr 0x3f0-7 */
>>     vt82c686b_superio_cfg_write(s, 0, 0xe3, 1);
>>     vt82c686b_superio_cfg_write(s, 1, 0xfc, 1);
>> @@ -465,6 +503,21 @@ static void vt8231_superio_cfg_write(void *opaque, hwaddr addr,
>>     case 0xfd:
>>         /* ignore write to read only registers */
>>         return;
>> +    case 0xf2:
>> +        data &= 0x17;
>> +        via_superio_devices_enable(sc, data);
>> +        break;
>> +    case 0xf4:
>> +        data &= 0xfe;
>> +        isa_serial_set_iobase(sc->superio.serial[0], data << 2);
>> +        break;
>> +    case 0xf6:
>> +        isa_parallel_set_iobase(sc->superio.parallel[0], data << 2);
>> +        break;
>> +    case 0xf7:
>> +        data &= 0xfc;
>> +        isa_fdc_set_iobase(sc->superio.floppy, data << 2);
>> +        break;
>>     default:
>>         qemu_log_mask(LOG_UNIMP,
>>                       "via_superio_cfg: unimplemented register 0x%x\n", idx);
>> @@ -513,12 +566,6 @@ static void vt8231_superio_init(Object *obj)
>>     VIA_SUPERIO(obj)->io_ops = &vt8231_superio_cfg_ops;
>> }
>> 
>> -static uint16_t vt8231_superio_serial_iobase(ISASuperIODevice *sio,
>> -                                             uint8_t index)
>> -{
>> -        return 0x2f8; /* FIXME: This should be settable via registers f2-f4 */
>> -}
>> -
>> static void vt8231_superio_class_init(ObjectClass *klass, void *data)
>> {
>>     DeviceClass *dc = DEVICE_CLASS(klass);
>> @@ -526,7 +573,6 @@ static void vt8231_superio_class_init(ObjectClass *klass, void *data)
>> 
>>     dc->reset = vt8231_superio_reset;
>>     sc->serial.count = 1;
>> -    sc->serial.get_iobase = vt8231_superio_serial_iobase;
>>     sc->parallel.count = 1;
>>     sc->ide.count = 0; /* emulated by via-ide */
>>     sc->floppy.count = 1;
>> 

Re: [PATCH v4 10/11] hw/ppc/pegasos2: Let pegasos2 machine configure SuperI/O functions

[Bernhard Beschow]

Am 7. Januar 2024 13:54:57 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Sat, 6 Jan 2024, Bernhard Beschow wrote:
>> This is a preparation for implementing relocation and toggling of SuperI/O
>> functions in the VT8231 device model. Upon reset, all SuperI/O functions will be
>> deactivated, so in case if no -bios is given, let the machine configure those
>> functions the same way Pegasos II firmware would do.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>> hw/ppc/pegasos2.c | 15 +++++++++++++++
>> 1 file changed, 15 insertions(+)
>> 
>> diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
>> index 3203a4a728..0a40ebd542 100644
>> --- a/hw/ppc/pegasos2.c
>> +++ b/hw/ppc/pegasos2.c
>> @@ -285,6 +285,15 @@ static void pegasos2_pci_config_write(Pegasos2MachineState *pm, int bus,
>>     pegasos2_mv_reg_write(pm, pcicfg + 4, len, val);
>> }
>> 
>> +static void pegasos2_superio_write(Pegasos2MachineState *pm, uint32_t addr,
>> +                                   uint32_t val)
>> +{
>> +    AddressSpace *as = CPU(pm->cpu)->as;
>
>I think this function should not need Pegasos2MachineState *pm and can just use cpu_physical_memory_write() instead.

Okay, I'll change it.

Best regards,
Bernhard

> Otherwise
>
>Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>
>
>> +
>> +    stb_phys(as, PCI1_IO_BASE + 0x3f0, addr);
>> +    stb_phys(as, PCI1_IO_BASE + 0x3f1, val);
>> +}
>> +
>> static void pegasos2_machine_reset(MachineState *machine, ShutdownCause reason)
>> {
>>     Pegasos2MachineState *pm = PEGASOS2_MACHINE(machine);
>> @@ -310,6 +319,12 @@ static void pegasos2_machine_reset(MachineState *machine, ShutdownCause reason)
>> 
>>     pegasos2_pci_config_write(pm, 1, (PCI_DEVFN(12, 0) << 8) |
>>                               PCI_INTERRUPT_LINE, 2, 0x9);
>> +    pegasos2_pci_config_write(pm, 1, (PCI_DEVFN(12, 0) << 8) |
>> +                              0x50, 1, 0x6);
>> +    pegasos2_superio_write(pm, 0xf4, 0xbe);
>> +    pegasos2_superio_write(pm, 0xf6, 0xef);
>> +    pegasos2_superio_write(pm, 0xf7, 0xfc);
>> +    pegasos2_superio_write(pm, 0xf2, 0x14);
>>     pegasos2_pci_config_write(pm, 1, (PCI_DEVFN(12, 0) << 8) |
>>                               0x50, 1, 0x2);
>>     pegasos2_pci_config_write(pm, 1, (PCI_DEVFN(12, 0) << 8) |
>> 

Re: [PATCH v4 00/11] hw/isa/vt82c686: Implement relocation and toggling of SuperI/O functions

[Bernhard Beschow]

Am 7. Januar 2024 14:13:44 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>On 06/01/2024 21:05, Bernhard Beschow wrote:
>
>> This series implements relocation of the SuperI/O functions of the VIA south
>> bridges which resolves some FIXME's. It is part of my via-apollo-pro-133t
>> branch [1] which is an extension of bringing the VIA south bridges to the PC
>> machine [2]. This branch is able to run some real-world X86 BIOSes in the hope
>> that it allows us to form a better understanding of the real vt82c686b devices.
>> Implementing relocation and toggling of the SuperI/O functions is one step to
>> make these BIOSes run without error messages, so here we go.
>> 
>> The series is structured as follows: Patches 1-3 prepare the TYPE_ISA_FDC,
>> TYPE_ISA_PARALLEL and TYPE_ISA_SERIAL to relocate and toggle (enable/disable)
>> themselves without breaking encapsulation of their respective device states.
>> This is achieved by moving the MemoryRegions and PortioLists from the device
>> states into the encapsulating ISA devices since they will be relocated and
>> toggled.
>> 
>> Inspired by the memory API patches 4-6 add two convenience functions to the
>> portio_list API to toggle and relocate portio lists. Patch 5 is a preparation
>> for that which removes some redundancies which otherwise had to be dealt with
>> during relocation.
>> 
>> Patches 7-9 implement toggling and relocation for types TYPE_ISA_FDC,
>> TYPE_ISA_PARALLEL and TYPE_ISA_SERIAL. Patch 10 prepares the pegasos2 machine
>> which would end up with all SuperI/O functions disabled if no -bios argument is
>> given. Patch 11 finally implements the main feature which now relies on
>> firmware to configure the SuperI/O functions accordingly (except for pegasos2).
>> 
>> v4:
>> * Drop incomplete SuperI/O vmstate handling (Zoltan)
>> 
>> v3:
>> * Rework various commit messages (Zoltan)
>> * Drop patch "hw/char/serial: Free struct SerialState from MemoryRegion"
>>    (Zoltan)
>> * Generalize wording in migration.rst to include portio_list API (Zoltan)
>> 
>> v2:
>> * Improve commit messages (Zoltan)
>> * Split pegasos2 from vt82c686 patch (Zoltan)
>> * Avoid poking into device internals (Zoltan)
>> 
>> Testing done:
>> * `make check`
>> * `make check-avocado`
>> * Run MorphOS on pegasos2 with and without pegasos2.rom
>> * Run Linux on amigaone
>> * Run real-world BIOSes on via-apollo-pro-133t branch
>> * Start rescue-yl on fuloong2e
>> 
>> [1] https://github.com/shentok/qemu/tree/via-apollo-pro-133t
>> [2] https://github.com/shentok/qemu/tree/pc-via
>> 
>> Bernhard Beschow (11):
>>    hw/block/fdc-isa: Move portio_list from FDCtrl to FDCtrlISABus
>>    hw/block/fdc-sysbus: Move iomem from FDCtrl to FDCtrlSysBus
>>    hw/char/parallel: Move portio_list from ParallelState to
>>      ISAParallelState
>>    exec/ioport: Resolve redundant .base attribute in struct
>>      MemoryRegionPortio
>>    exec/ioport: Add portio_list_set_address()
>>    exec/ioport: Add portio_list_set_enabled()
>>    hw/block/fdc-isa: Implement relocation and enabling/disabling for
>>      TYPE_ISA_FDC
>>    hw/char/serial-isa: Implement relocation and enabling/disabling for
>>      TYPE_ISA_SERIAL
>>    hw/char/parallel-isa: Implement relocation and enabling/disabling for
>>      TYPE_ISA_PARALLEL
>>    hw/ppc/pegasos2: Let pegasos2 machine configure SuperI/O functions
>>    hw/isa/vt82c686: Implement relocation and toggling of SuperI/O
>>      functions
>> 
>>   docs/devel/migration.rst       |  6 ++--
>>   hw/block/fdc-internal.h        |  4 ---
>>   include/exec/ioport.h          |  4 ++-
>>   include/hw/block/fdc.h         |  3 ++
>>   include/hw/char/parallel-isa.h |  5 +++
>>   include/hw/char/parallel.h     |  2 --
>>   include/hw/char/serial.h       |  2 ++
>>   hw/block/fdc-isa.c             | 18 +++++++++-
>>   hw/block/fdc-sysbus.c          |  6 ++--
>>   hw/char/parallel-isa.c         | 14 ++++++++
>>   hw/char/parallel.c             |  2 +-
>>   hw/char/serial-isa.c           | 14 ++++++++
>>   hw/isa/vt82c686.c              | 66 ++++++++++++++++++++++++++++------
>>   hw/ppc/pegasos2.c              | 15 ++++++++
>>   system/ioport.c                | 41 +++++++++++++++++----
>>   15 files changed, 172 insertions(+), 30 deletions(-)
>
>I think this series generally looks good: the only thing I think it's worth checking is whether portio lists are considered exclusive to ISA devices or not? (Paolo?).

The modifications preserve the current design, so how is this question related to this series?

I'd appreciate feedback from the maintainers indeed since this part hasn't received any comments so far. Thanks :)

>
>The portio_list_set_enabled() API looks interesting, and could be considered for use by my PCI IDE mode-switching changes too.
>
>Apologies I don't have a huge amount of time for review right now, but I wanted to feed back that generally these patches look good, and if people are happy with the portio list changes then this series should be considered for merge.

Never mind, it's still nice getting some confirmation from your side!

Best regards,
Bernhard

>
>
>ATB,
>
>Mark.
>

Re: [PATCH v4 11/11] hw/isa/vt82c686: Implement relocation and toggling of SuperI/O functions

[BALATON Zoltan]

On Mon, 8 Jan 2024, Bernhard Beschow wrote:
> Am 7. Januar 2024 13:59:44 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>> On Sat, 6 Jan 2024, Bernhard Beschow wrote:
>>> The VIA south bridges are able to relocate and toggle (enable or disable) their
>>> SuperI/O functions. So far this is hardcoded such that all functions are always
>>> enabled and are located at fixed addresses.
>>>
>>> Some PC BIOSes seem to probe for I/O occupancy before activating such a function
>>> and issue an error in case of a conflict. Since the functions are currently
>>> enabled on reset, conflicts are always detected. Prevent that by implementing
>>> relocation and toggling of the SuperI/O functions.
>>>
>>> Note that all SuperI/O functions are now deactivated upon reset (except for
>>> VT82C686B's serial ports where Fuloong 2e's rescue-yl seems to expect them to be
>>> enabled by default). Rely on firmware to configure the functions accordingly.
>>>
>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>> ---
>>> hw/isa/vt82c686.c | 66 ++++++++++++++++++++++++++++++++++++++++-------
>>> 1 file changed, 56 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>> index d3e0f6d01f..9f62fb5964 100644
>>> --- a/hw/isa/vt82c686.c
>>> +++ b/hw/isa/vt82c686.c
>>> @@ -15,6 +15,9 @@
>>>
>>> #include "qemu/osdep.h"
>>> #include "hw/isa/vt82c686.h"
>>> +#include "hw/block/fdc.h"
>>> +#include "hw/char/parallel-isa.h"
>>> +#include "hw/char/serial.h"
>>> #include "hw/pci/pci.h"
>>> #include "hw/qdev-properties.h"
>>> #include "hw/ide/pci.h"
>>> @@ -323,6 +326,18 @@ static uint64_t via_superio_cfg_read(void *opaque, hwaddr addr, unsigned size)
>>>     return val;
>>> }
>>>
>>> +static void via_superio_devices_enable(ViaSuperIOState *s, uint8_t data)
>>> +{
>>> +    ISASuperIOClass *ic = ISA_SUPERIO_GET_CLASS(s);
>>> +    size_t i;
>>
>> The expected value for i is 0 or 1 (maybe up to 3 sometimes it there are more serial ports in a chip). so why use such big type?
>
> serial.count is of type size_t, that's why I chose it. Let me know if you still want an int, otherwise I'd leave it as is.

I think int would suffice here but it's not a big deal. The count is 
indeed size_t, not sure why. How many components were expected? But the 
practical value is just a few so larger type seems to be an overkill.

Regards,
BALATON Zoltan

> Best regards,
> Bernhard
>
>> This should just be int. Newly it's also allowed to declare it within the for so if you want that you could do so but I have no preference on that and declaring it here is also OK. Otherwise:
>>
>> Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>
>>
>>> +
>>> +    isa_parallel_set_enabled(s->superio.parallel[0], (data & 0x3) != 3);
>>> +    for (i = 0; i < ic->serial.count; i++) {
>>> +        isa_serial_set_enabled(s->superio.serial[i], data & BIT(i + 2));
>>> +    }
>>> +    isa_fdc_set_enabled(s->superio.floppy, data & BIT(4));
>>> +}
>>> +
>>> static void via_superio_class_init(ObjectClass *klass, void *data)
>>> {
>>>     DeviceClass *dc = DEVICE_CLASS(klass);
>>> @@ -368,7 +383,25 @@ static void vt82c686b_superio_cfg_write(void *opaque, hwaddr addr,
>>>     case 0xfd ... 0xff:
>>>         /* ignore write to read only registers */
>>>         return;
>>> -    /* case 0xe6 ... 0xe8: Should set base port of parallel and serial */
>>> +    case 0xe2:
>>> +        data &= 0x1f;
>>> +        via_superio_devices_enable(sc, data);
>>> +        break;
>>> +    case 0xe3:
>>> +        data &= 0xfc;
>>> +        isa_fdc_set_iobase(sc->superio.floppy, data << 2);
>>> +        break;
>>> +    case 0xe6:
>>> +        isa_parallel_set_iobase(sc->superio.parallel[0], data << 2);
>>> +        break;
>>> +    case 0xe7:
>>> +        data &= 0xfe;
>>> +        isa_serial_set_iobase(sc->superio.serial[0], data << 2);
>>> +        break;
>>> +    case 0xe8:
>>> +        data &= 0xfe;
>>> +        isa_serial_set_iobase(sc->superio.serial[1], data << 2);
>>> +        break;
>>>     default:
>>>         qemu_log_mask(LOG_UNIMP,
>>>                       "via_superio_cfg: unimplemented register 0x%x\n", idx);
>>> @@ -395,9 +428,14 @@ static void vt82c686b_superio_reset(DeviceState *dev)
>>>     /* Device ID */
>>>     vt82c686b_superio_cfg_write(s, 0, 0xe0, 1);
>>>     vt82c686b_superio_cfg_write(s, 1, 0x3c, 1);
>>> -    /* Function select - all disabled */
>>> +    /*
>>> +     * Function select - only serial enabled
>>> +     * Fuloong 2e's rescue-yl prints to the serial console w/o enabling it. This
>>> +     * suggests that the serial ports are enabled by default, so override the
>>> +     * datasheet.
>>> +     */
>>>     vt82c686b_superio_cfg_write(s, 0, 0xe2, 1);
>>> -    vt82c686b_superio_cfg_write(s, 1, 0x03, 1);
>>> +    vt82c686b_superio_cfg_write(s, 1, 0x0f, 1);
>>>     /* Floppy ctrl base addr 0x3f0-7 */
>>>     vt82c686b_superio_cfg_write(s, 0, 0xe3, 1);
>>>     vt82c686b_superio_cfg_write(s, 1, 0xfc, 1);
>>> @@ -465,6 +503,21 @@ static void vt8231_superio_cfg_write(void *opaque, hwaddr addr,
>>>     case 0xfd:
>>>         /* ignore write to read only registers */
>>>         return;
>>> +    case 0xf2:
>>> +        data &= 0x17;
>>> +        via_superio_devices_enable(sc, data);
>>> +        break;
>>> +    case 0xf4:
>>> +        data &= 0xfe;
>>> +        isa_serial_set_iobase(sc->superio.serial[0], data << 2);
>>> +        break;
>>> +    case 0xf6:
>>> +        isa_parallel_set_iobase(sc->superio.parallel[0], data << 2);
>>> +        break;
>>> +    case 0xf7:
>>> +        data &= 0xfc;
>>> +        isa_fdc_set_iobase(sc->superio.floppy, data << 2);
>>> +        break;
>>>     default:
>>>         qemu_log_mask(LOG_UNIMP,
>>>                       "via_superio_cfg: unimplemented register 0x%x\n", idx);
>>> @@ -513,12 +566,6 @@ static void vt8231_superio_init(Object *obj)
>>>     VIA_SUPERIO(obj)->io_ops = &vt8231_superio_cfg_ops;
>>> }
>>>
>>> -static uint16_t vt8231_superio_serial_iobase(ISASuperIODevice *sio,
>>> -                                             uint8_t index)
>>> -{
>>> -        return 0x2f8; /* FIXME: This should be settable via registers f2-f4 */
>>> -}
>>> -
>>> static void vt8231_superio_class_init(ObjectClass *klass, void *data)
>>> {
>>>     DeviceClass *dc = DEVICE_CLASS(klass);
>>> @@ -526,7 +573,6 @@ static void vt8231_superio_class_init(ObjectClass *klass, void *data)
>>>
>>>     dc->reset = vt8231_superio_reset;
>>>     sc->serial.count = 1;
>>> -    sc->serial.get_iobase = vt8231_superio_serial_iobase;
>>>     sc->parallel.count = 1;
>>>     sc->ide.count = 0; /* emulated by via-ide */
>>>     sc->floppy.count = 1;
>>>
>
>

Re: [PATCH v4 00/11] hw/isa/vt82c686: Implement relocation and toggling of SuperI/O functions

[Mark Cave-Ayland]

On 08/01/2024 20:07, Bernhard Beschow wrote:

> Am 7. Januar 2024 14:13:44 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>> On 06/01/2024 21:05, Bernhard Beschow wrote:
>>
>>> This series implements relocation of the SuperI/O functions of the VIA south
>>> bridges which resolves some FIXME's. It is part of my via-apollo-pro-133t
>>> branch [1] which is an extension of bringing the VIA south bridges to the PC
>>> machine [2]. This branch is able to run some real-world X86 BIOSes in the hope
>>> that it allows us to form a better understanding of the real vt82c686b devices.
>>> Implementing relocation and toggling of the SuperI/O functions is one step to
>>> make these BIOSes run without error messages, so here we go.
>>>
>>> The series is structured as follows: Patches 1-3 prepare the TYPE_ISA_FDC,
>>> TYPE_ISA_PARALLEL and TYPE_ISA_SERIAL to relocate and toggle (enable/disable)
>>> themselves without breaking encapsulation of their respective device states.
>>> This is achieved by moving the MemoryRegions and PortioLists from the device
>>> states into the encapsulating ISA devices since they will be relocated and
>>> toggled.
>>>
>>> Inspired by the memory API patches 4-6 add two convenience functions to the
>>> portio_list API to toggle and relocate portio lists. Patch 5 is a preparation
>>> for that which removes some redundancies which otherwise had to be dealt with
>>> during relocation.
>>>
>>> Patches 7-9 implement toggling and relocation for types TYPE_ISA_FDC,
>>> TYPE_ISA_PARALLEL and TYPE_ISA_SERIAL. Patch 10 prepares the pegasos2 machine
>>> which would end up with all SuperI/O functions disabled if no -bios argument is
>>> given. Patch 11 finally implements the main feature which now relies on
>>> firmware to configure the SuperI/O functions accordingly (except for pegasos2).
>>>
>>> v4:
>>> * Drop incomplete SuperI/O vmstate handling (Zoltan)
>>>
>>> v3:
>>> * Rework various commit messages (Zoltan)
>>> * Drop patch "hw/char/serial: Free struct SerialState from MemoryRegion"
>>>     (Zoltan)
>>> * Generalize wording in migration.rst to include portio_list API (Zoltan)
>>>
>>> v2:
>>> * Improve commit messages (Zoltan)
>>> * Split pegasos2 from vt82c686 patch (Zoltan)
>>> * Avoid poking into device internals (Zoltan)
>>>
>>> Testing done:
>>> * `make check`
>>> * `make check-avocado`
>>> * Run MorphOS on pegasos2 with and without pegasos2.rom
>>> * Run Linux on amigaone
>>> * Run real-world BIOSes on via-apollo-pro-133t branch
>>> * Start rescue-yl on fuloong2e
>>>
>>> [1] https://github.com/shentok/qemu/tree/via-apollo-pro-133t
>>> [2] https://github.com/shentok/qemu/tree/pc-via
>>>
>>> Bernhard Beschow (11):
>>>     hw/block/fdc-isa: Move portio_list from FDCtrl to FDCtrlISABus
>>>     hw/block/fdc-sysbus: Move iomem from FDCtrl to FDCtrlSysBus
>>>     hw/char/parallel: Move portio_list from ParallelState to
>>>       ISAParallelState
>>>     exec/ioport: Resolve redundant .base attribute in struct
>>>       MemoryRegionPortio
>>>     exec/ioport: Add portio_list_set_address()
>>>     exec/ioport: Add portio_list_set_enabled()
>>>     hw/block/fdc-isa: Implement relocation and enabling/disabling for
>>>       TYPE_ISA_FDC
>>>     hw/char/serial-isa: Implement relocation and enabling/disabling for
>>>       TYPE_ISA_SERIAL
>>>     hw/char/parallel-isa: Implement relocation and enabling/disabling for
>>>       TYPE_ISA_PARALLEL
>>>     hw/ppc/pegasos2: Let pegasos2 machine configure SuperI/O functions
>>>     hw/isa/vt82c686: Implement relocation and toggling of SuperI/O
>>>       functions
>>>
>>>    docs/devel/migration.rst       |  6 ++--
>>>    hw/block/fdc-internal.h        |  4 ---
>>>    include/exec/ioport.h          |  4 ++-
>>>    include/hw/block/fdc.h         |  3 ++
>>>    include/hw/char/parallel-isa.h |  5 +++
>>>    include/hw/char/parallel.h     |  2 --
>>>    include/hw/char/serial.h       |  2 ++
>>>    hw/block/fdc-isa.c             | 18 +++++++++-
>>>    hw/block/fdc-sysbus.c          |  6 ++--
>>>    hw/char/parallel-isa.c         | 14 ++++++++
>>>    hw/char/parallel.c             |  2 +-
>>>    hw/char/serial-isa.c           | 14 ++++++++
>>>    hw/isa/vt82c686.c              | 66 ++++++++++++++++++++++++++++------
>>>    hw/ppc/pegasos2.c              | 15 ++++++++
>>>    system/ioport.c                | 41 +++++++++++++++++----
>>>    15 files changed, 172 insertions(+), 30 deletions(-)
>>
>> I think this series generally looks good: the only thing I think it's worth checking is whether portio lists are considered exclusive to ISA devices or not? (Paolo?).
> 
> The modifications preserve the current design, so how is this question related to this series?

I was thinking about patches 1 and 3 where the portio_list variable is moved from the 
core object to the ISA-specific child objects.

> I'd appreciate feedback from the maintainers indeed since this part hasn't received any comments so far. Thanks :)

Agreed. I *think* the portio_lists are ISA-specific as far as QEMU is concerned, but 
a quick nod from an x86 maintainer would be a great help :)

>> The portio_list_set_enabled() API looks interesting, and could be considered for use by my PCI IDE mode-switching changes too.
>>
>> Apologies I don't have a huge amount of time for review right now, but I wanted to feed back that generally these patches look good, and if people are happy with the portio list changes then this series should be considered for merge.
> 
> Never mind, it's still nice getting some confirmation from your side!

No worries!


ATB,

Mark.

Re: [PATCH v4 00/11] hw/isa/vt82c686: Implement relocation and toggling of SuperI/O functions

[Bernhard Beschow]

Am 8. Januar 2024 22:12:12 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>On 08/01/2024 20:07, Bernhard Beschow wrote:
>
>> Am 7. Januar 2024 14:13:44 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>>> On 06/01/2024 21:05, Bernhard Beschow wrote:
>>> 
>>>> This series implements relocation of the SuperI/O functions of the VIA south
>>>> bridges which resolves some FIXME's. It is part of my via-apollo-pro-133t
>>>> branch [1] which is an extension of bringing the VIA south bridges to the PC
>>>> machine [2]. This branch is able to run some real-world X86 BIOSes in the hope
>>>> that it allows us to form a better understanding of the real vt82c686b devices.
>>>> Implementing relocation and toggling of the SuperI/O functions is one step to
>>>> make these BIOSes run without error messages, so here we go.
>>>> 
>>>> The series is structured as follows: Patches 1-3 prepare the TYPE_ISA_FDC,
>>>> TYPE_ISA_PARALLEL and TYPE_ISA_SERIAL to relocate and toggle (enable/disable)
>>>> themselves without breaking encapsulation of their respective device states.
>>>> This is achieved by moving the MemoryRegions and PortioLists from the device
>>>> states into the encapsulating ISA devices since they will be relocated and
>>>> toggled.
>>>> 
>>>> Inspired by the memory API patches 4-6 add two convenience functions to the
>>>> portio_list API to toggle and relocate portio lists. Patch 5 is a preparation
>>>> for that which removes some redundancies which otherwise had to be dealt with
>>>> during relocation.
>>>> 
>>>> Patches 7-9 implement toggling and relocation for types TYPE_ISA_FDC,
>>>> TYPE_ISA_PARALLEL and TYPE_ISA_SERIAL. Patch 10 prepares the pegasos2 machine
>>>> which would end up with all SuperI/O functions disabled if no -bios argument is
>>>> given. Patch 11 finally implements the main feature which now relies on
>>>> firmware to configure the SuperI/O functions accordingly (except for pegasos2).
>>>> 
>>>> v4:
>>>> * Drop incomplete SuperI/O vmstate handling (Zoltan)
>>>> 
>>>> v3:
>>>> * Rework various commit messages (Zoltan)
>>>> * Drop patch "hw/char/serial: Free struct SerialState from MemoryRegion"
>>>>     (Zoltan)
>>>> * Generalize wording in migration.rst to include portio_list API (Zoltan)
>>>> 
>>>> v2:
>>>> * Improve commit messages (Zoltan)
>>>> * Split pegasos2 from vt82c686 patch (Zoltan)
>>>> * Avoid poking into device internals (Zoltan)
>>>> 
>>>> Testing done:
>>>> * `make check`
>>>> * `make check-avocado`
>>>> * Run MorphOS on pegasos2 with and without pegasos2.rom
>>>> * Run Linux on amigaone
>>>> * Run real-world BIOSes on via-apollo-pro-133t branch
>>>> * Start rescue-yl on fuloong2e
>>>> 
>>>> [1] https://github.com/shentok/qemu/tree/via-apollo-pro-133t
>>>> [2] https://github.com/shentok/qemu/tree/pc-via
>>>> 
>>>> Bernhard Beschow (11):
>>>>     hw/block/fdc-isa: Move portio_list from FDCtrl to FDCtrlISABus
>>>>     hw/block/fdc-sysbus: Move iomem from FDCtrl to FDCtrlSysBus
>>>>     hw/char/parallel: Move portio_list from ParallelState to
>>>>       ISAParallelState
>>>>     exec/ioport: Resolve redundant .base attribute in struct
>>>>       MemoryRegionPortio
>>>>     exec/ioport: Add portio_list_set_address()
>>>>     exec/ioport: Add portio_list_set_enabled()
>>>>     hw/block/fdc-isa: Implement relocation and enabling/disabling for
>>>>       TYPE_ISA_FDC
>>>>     hw/char/serial-isa: Implement relocation and enabling/disabling for
>>>>       TYPE_ISA_SERIAL
>>>>     hw/char/parallel-isa: Implement relocation and enabling/disabling for
>>>>       TYPE_ISA_PARALLEL
>>>>     hw/ppc/pegasos2: Let pegasos2 machine configure SuperI/O functions
>>>>     hw/isa/vt82c686: Implement relocation and toggling of SuperI/O
>>>>       functions
>>>> 
>>>>    docs/devel/migration.rst       |  6 ++--
>>>>    hw/block/fdc-internal.h        |  4 ---
>>>>    include/exec/ioport.h          |  4 ++-
>>>>    include/hw/block/fdc.h         |  3 ++
>>>>    include/hw/char/parallel-isa.h |  5 +++
>>>>    include/hw/char/parallel.h     |  2 --
>>>>    include/hw/char/serial.h       |  2 ++
>>>>    hw/block/fdc-isa.c             | 18 +++++++++-
>>>>    hw/block/fdc-sysbus.c          |  6 ++--
>>>>    hw/char/parallel-isa.c         | 14 ++++++++
>>>>    hw/char/parallel.c             |  2 +-
>>>>    hw/char/serial-isa.c           | 14 ++++++++
>>>>    hw/isa/vt82c686.c              | 66 ++++++++++++++++++++++++++++------
>>>>    hw/ppc/pegasos2.c              | 15 ++++++++
>>>>    system/ioport.c                | 41 +++++++++++++++++----
>>>>    15 files changed, 172 insertions(+), 30 deletions(-)
>>> 
>>> I think this series generally looks good: the only thing I think it's worth checking is whether portio lists are considered exclusive to ISA devices or not? (Paolo?).
>> 
>> The modifications preserve the current design, so how is this question related to this series?
>
>I was thinking about patches 1 and 3 where the portio_list variable is moved from the core object to the ISA-specific child objects.

I think of patches 1 - 3 more as cleanups where an attribute unused in the core is moved one level up to where it is needed. In addition, the floppy core had two attributes where only one was ever used in a specific device. From that perspective I think the question of whether the portio_list API is ISA specific or not is unrelated to this series, i.e. should be discussed separately.

>
>> I'd appreciate feedback from the maintainers indeed since this part hasn't received any comments so far. Thanks :)
>
>Agreed. I *think* the portio_lists are ISA-specific as far as QEMU is concerned, but a quick nod from an x86 maintainer would be a great help :)

A quick nod shall be in scope ;)

Best regards,
Bernhard

>
>>> The portio_list_set_enabled() API looks interesting, and could be considered for use by my PCI IDE mode-switching changes too.
>>> 
>>> Apologies I don't have a huge amount of time for review right now, but I wanted to feed back that generally these patches look good, and if people are happy with the portio list changes then this series should be considered for merge.
>> 
>> Never mind, it's still nice getting some confirmation from your side!
>
>No worries!
>
>
>ATB,
>
>Mark.
>