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

[PATCH v5 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).

v5:
* Use cpu_physical_memory_write() in pegasos2 (Zoltan)
* Use an int as for loop variable (Zoltan)

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              | 65 ++++++++++++++++++++++++++++------
 hw/ppc/pegasos2.c              | 12 +++++++
 system/ioport.c                | 41 +++++++++++++++++----
 15 files changed, 168 insertions(+), 30 deletions(-)

-- 
2.43.0

[PATCH v5 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 v5 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 v5 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 v5 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 v5 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 v5 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 v5 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 v5 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 v5 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 v5 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>
Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/ppc/pegasos2.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
index d84f3f977d..04d6decb2b 100644
--- a/hw/ppc/pegasos2.c
+++ b/hw/ppc/pegasos2.c
@@ -285,6 +285,12 @@ static void pegasos2_pci_config_write(Pegasos2MachineState *pm, int bus,
     pegasos2_mv_reg_write(pm, pcicfg + 4, len, val);
 }
 
+static void pegasos2_superio_write(uint8_t addr, uint8_t val)
+{
+    cpu_physical_memory_write(PCI1_IO_BASE + 0x3f0, &addr, 1);
+    cpu_physical_memory_write(PCI1_IO_BASE + 0x3f1, &val, 1);
+}
+
 static void pegasos2_machine_reset(MachineState *machine, ShutdownCause reason)
 {
     Pegasos2MachineState *pm = PEGASOS2_MACHINE(machine);
@@ -310,6 +316,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(0xf4, 0xbe);
+    pegasos2_superio_write(0xf6, 0xef);
+    pegasos2_superio_write(0xf7, 0xfc);
+    pegasos2_superio_write(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 v5 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>
Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/isa/vt82c686.c | 65 +++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 55 insertions(+), 10 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index d3e0f6d01f..485bb685b7 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,17 @@ 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);
+
+    isa_parallel_set_enabled(s->superio.parallel[0], (data & 0x3) != 3);
+    for (int 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 +382,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 +427,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 +502,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 +565,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 +572,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 v5 00/11] hw/isa/vt82c686: Implement relocation and toggling of SuperI/O functions

[Bernhard Beschow]

Am 14. Januar 2024 12:39:00 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>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).
>
>
>
>v5:
>
>* Use cpu_physical_memory_write() in pegasos2 (Zoltan)
>
>* Use an int as for loop variable (Zoltan)
>
>
>
>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
>

Ping for the above six patches. These are critical for this series and haven't had comments from maintainers for five iterations, so pardon for the annoyance.

Best regards,
Bernhard

>  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              | 65 ++++++++++++++++++++++++++++------
>
> hw/ppc/pegasos2.c              | 12 +++++++
>
> system/ioport.c                | 41 +++++++++++++++++----
>
> 15 files changed, 168 insertions(+), 30 deletions(-)
>
>
>
>-- >
>2.43.0
>
>
>

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

[Michael S. Tsirkin]

On Sun, Jan 14, 2024 at 12:52:53PM +0000, Bernhard Beschow wrote:
> 
> 
> Am 14. Januar 2024 12:39:00 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
> >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).
> >
> >
> >
> >v5:
> >
> >* Use cpu_physical_memory_write() in pegasos2 (Zoltan)
> >
> >* Use an int as for loop variable (Zoltan)
> >
> >
> >
> >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
> >
> 
> Ping for the above six patches. These are critical for this series and haven't had comments from maintainers for five iterations, so pardon for the annoyance.

Was hoping for more reviews. Oh well.
Tagged.


> Best regards,
> Bernhard
> 
> >  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              | 65 ++++++++++++++++++++++++++++------
> >
> > hw/ppc/pegasos2.c              | 12 +++++++
> >
> > system/ioport.c                | 41 +++++++++++++++++----
> >
> > 15 files changed, 168 insertions(+), 30 deletions(-)
> >
> >
> >
> >-- >
> >2.43.0
> >
> >
> >

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

[Bernhard Beschow]

Am 14. Januar 2024 13:03:07 UTC schrieb "Michael S. Tsirkin" <mst@redhat.com>:
>On Sun, Jan 14, 2024 at 12:52:53PM +0000, Bernhard Beschow wrote:
>> 
>> 
>> Am 14. Januar 2024 12:39:00 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>> >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).
>> >
>> >
>> >
>> >v5:
>> >
>> >* Use cpu_physical_memory_write() in pegasos2 (Zoltan)
>> >
>> >* Use an int as for loop variable (Zoltan)
>> >
>> >
>> >
>> >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
>> >
>> 
>> Ping for the above six patches. These are critical for this series and haven't had comments from maintainers for five iterations, so pardon for the annoyance.
>
>Was hoping for more reviews. Oh well.
>Tagged.

You made my day. Thanks!

>
>
>> Best regards,
>> Bernhard
>> 
>> >  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              | 65 ++++++++++++++++++++++++++++------
>> >
>> > hw/ppc/pegasos2.c              | 12 +++++++
>> >
>> > system/ioport.c                | 41 +++++++++++++++++----
>> >
>> > 15 files changed, 168 insertions(+), 30 deletions(-)
>> >
>> >
>> >
>> >-- >
>> >2.43.0
>> >
>> >
>> >
>

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

[Philippe Mathieu-Daudé]

On 14/1/24 13:39, Bernhard Beschow wrote:
> 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(-)


> +void portio_list_set_address(PortioList *piolist, uint32_t addr)
> +{
> +    MemoryRegionPortioList *mrpio;
> +    unsigned i, j;
> +

        memory_region_transaction_begin();

> +    for (i = 0; i < piolist->nr; ++i) {
> +        mrpio = container_of(piolist->regions[i], MemoryRegionPortioList, mr);

Should we check mrpio->mr is disabled before changing its base address?

> +        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;
> +        }

           memory_region_transaction_commit();

> +    }
> +
> +    piolist->addr = addr;
> +}
> +
>   static void memory_region_portio_list_finalize(Object *obj)
>   {
>       MemoryRegionPortioList *mrpio = MEMORY_REGION_PORTIO_LIST(obj);

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

[Philippe Mathieu-Daudé]

On 14/1/24 13:39, 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>
> Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   hw/isa/vt82c686.c | 65 +++++++++++++++++++++++++++++++++++++++--------
>   1 file changed, 55 insertions(+), 10 deletions(-)


> +static void via_superio_devices_enable(ViaSuperIOState *s, uint8_t data)
> +{
> +    ISASuperIOClass *ic = ISA_SUPERIO_GET_CLASS(s);
> +

        memory_region_transaction_begin();

> +    isa_parallel_set_enabled(s->superio.parallel[0], (data & 0x3) != 3);
> +    for (int 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));

        memory_region_transaction_commit();

> +}
> +

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

[Bernhard Beschow]

Am 29. Juli 2024 09:26:19 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>On 14/1/24 13:39, Bernhard Beschow wrote:
>> 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(-)
>
>
>> +void portio_list_set_address(PortioList *piolist, uint32_t addr)
>> +{
>> +    MemoryRegionPortioList *mrpio;
>> +    unsigned i, j;
>> +
>
>       memory_region_transaction_begin();
>
>> +    for (i = 0; i < piolist->nr; ++i) {
>> +        mrpio = container_of(piolist->regions[i], MemoryRegionPortioList, mr);
>
>Should we check mrpio->mr is disabled before changing its base address?

Isn't that the responsibility of the guest? What should we do if the check fails?

>
>> +        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;
>> +        }
>
>          memory_region_transaction_commit();
>
>> +    }
>> +
>> +    piolist->addr = addr;
>> +}
>> +
>>   static void memory_region_portio_list_finalize(Object *obj)
>>   {
>>       MemoryRegionPortioList *mrpio = MEMORY_REGION_PORTIO_LIST(obj);
>

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

[Philippe Mathieu-Daudé]

On 29/7/24 23:07, Bernhard Beschow wrote:
> 
> 
> Am 29. Juli 2024 09:26:19 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>> On 14/1/24 13:39, Bernhard Beschow wrote:
>>> 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(-)
>>
>>
>>> +void portio_list_set_address(PortioList *piolist, uint32_t addr)
>>> +{
>>> +    MemoryRegionPortioList *mrpio;
>>> +    unsigned i, j;
>>> +
>>
>>        memory_region_transaction_begin();
>>
>>> +    for (i = 0; i < piolist->nr; ++i) {
>>> +        mrpio = container_of(piolist->regions[i], MemoryRegionPortioList, mr);
>>
>> Should we check mrpio->mr is disabled before changing its base address?
> 
> Isn't that the responsibility of the guest? What should we do if the check fails?

What says the datasheet? At least we should log a GUEST_ERROR here.

> 
>>
>>> +        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;
>>> +        }
>>
>>           memory_region_transaction_commit();
>>
>>> +    }
>>> +
>>> +    piolist->addr = addr;
>>> +}
>>> +
>>>    static void memory_region_portio_list_finalize(Object *obj)
>>>    {
>>>        MemoryRegionPortioList *mrpio = MEMORY_REGION_PORTIO_LIST(obj);
>>

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

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 2073 bytes --]

On Tue, 30 Jul 2024, Philippe Mathieu-Daudé wrote:
> On 29/7/24 23:07, Bernhard Beschow wrote:
>> Am 29. Juli 2024 09:26:19 UTC schrieb "Philippe Mathieu-Daudé" 
>> <philmd@linaro.org>:
>>> On 14/1/24 13:39, Bernhard Beschow wrote:
>>>> 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(-)
>>> 
>>> 
>>>> +void portio_list_set_address(PortioList *piolist, uint32_t addr)
>>>> +{
>>>> +    MemoryRegionPortioList *mrpio;
>>>> +    unsigned i, j;
>>>> +
>>>
>>>        memory_region_transaction_begin();
>>> 
>>>> +    for (i = 0; i < piolist->nr; ++i) {
>>>> +        mrpio = container_of(piolist->regions[i], 
>>>> MemoryRegionPortioList, mr);
>>> 
>>> Should we check mrpio->mr is disabled before changing its base address?
>> 
>> Isn't that the responsibility of the guest? What should we do if the check 
>> fails?
>
> What says the datasheet? At least we should log a GUEST_ERROR here.

It does not say what if it's not enabled but only says that parallel port 
tegs should be located at LPTBase (0xf6 of superio config). So I think 
this should move the memory region independent of if it's enabled but then 
mayube the enable bit should check the address and set it when enabling 
the region? But shouldn't portio take care of that remembering the base? I 
don't know how all this works in QEMU and don't have time to check now.

Reagrds,
BALATON Zoltan

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

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 1903 bytes --]

On Mon, 29 Jul 2024, Philippe Mathieu-Daudé wrote:
> On 14/1/24 13:39, 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>
>> Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>   hw/isa/vt82c686.c | 65 +++++++++++++++++++++++++++++++++++++++--------
>>   1 file changed, 55 insertions(+), 10 deletions(-)
>
>
>> +static void via_superio_devices_enable(ViaSuperIOState *s, uint8_t data)
>> +{
>> +    ISASuperIOClass *ic = ISA_SUPERIO_GET_CLASS(s);
>> +
>
>       memory_region_transaction_begin();
>
>> +    isa_parallel_set_enabled(s->superio.parallel[0], (data & 0x3) != 3);
>> +    for (int 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));
>
>       memory_region_transaction_commit();

Is a transaction needed here? We're just enable/disable independent memory 
regions here and I don't think this function can be interrupted but having 
a transaction does not hurt but I don't understand why we would need it.

Regards,
BALATON Zoltan