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

[PATCH v2 00/12] 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-4 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 5-7 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 8-10 implement toggling and relocation for types TYPE_ISA_FDC,
TYPE_ISA_PARALLEL and TYPE_ISA_SERIAL. Patch 11 prepares the pegasos2 machine
which would end up with all SuperI/O functions disabled if no -bios argument is
given. Patch 12 finally implements the main feature which now relies on
firmware to configure the SuperI/O functions accordingly (except for pegasos2).

v2:
* Improve commit message (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 (12):
  hw/block/fdc-isa: Free struct FDCtrl from PortioList
  hw/block/fdc-sysbus: Free struct FDCtrl from MemoryRegion
  hw/char/serial: Free struct SerialState from MemoryRegion
  hw/char/parallel: Free struct ParallelState from PortioList
  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 toggling for TYPE_ISA_FDC
  hw/char/serial-isa: Implement relocation and toggling for
    TYPE_ISA_SERIAL
  hw/char/parallel-isa: Implement relocation and toggling 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       |   2 +
 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       |   4 +-
 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           |  21 +++++-
 hw/char/serial-pci-multi.c     |   7 +-
 hw/char/serial-pci.c           |   7 +-
 hw/char/serial.c               |   4 +-
 hw/isa/vt82c686.c              | 121 ++++++++++++++++++++++++---------
 hw/ppc/pegasos2.c              |  15 ++++
 system/ioport.c                |  41 +++++++++--
 18 files changed, 221 insertions(+), 59 deletions(-)

-- 
2.43.0

[PATCH v2 01/12] hw/block/fdc-isa: Free struct FDCtrl from PortioList

[Bernhard Beschow]

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

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 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 7ec075e470..b4c92b40b3 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 v2 02/12] hw/block/fdc-sysbus: Free struct FDCtrl from MemoryRegion

[Bernhard Beschow]

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

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 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 86ea51d003..e197b97262 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 v2 03/12] hw/char/serial: Free struct SerialState from MemoryRegion

[Bernhard Beschow]

SerialState::io isn't used within TYPE_SERIAL directly. Push it to its users to
make them the owner of the MemoryRegion.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/char/serial.h   | 2 +-
 hw/char/serial-isa.c       | 7 +++++--
 hw/char/serial-pci-multi.c | 7 ++++---
 hw/char/serial-pci.c       | 7 +++++--
 hw/char/serial.c           | 4 ++--
 5 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
index 8ba7eca3d6..eb4254edde 100644
--- a/include/hw/char/serial.h
+++ b/include/hw/char/serial.h
@@ -77,7 +77,6 @@ struct SerialState {
     int poll_msl;
 
     QEMUTimer *modem_status_poll;
-    MemoryRegion io;
 };
 typedef struct SerialState SerialState;
 
@@ -85,6 +84,7 @@ struct SerialMM {
     SysBusDevice parent;
 
     SerialState serial;
+    MemoryRegion io;
 
     uint8_t regshift;
     uint8_t endianness;
diff --git a/hw/char/serial-isa.c b/hw/char/serial-isa.c
index 141a6cb168..2be8be980b 100644
--- a/hw/char/serial-isa.c
+++ b/hw/char/serial-isa.c
@@ -26,6 +26,7 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "qemu/module.h"
+#include "exec/memory.h"
 #include "sysemu/sysemu.h"
 #include "hw/acpi/acpi_aml_interface.h"
 #include "hw/char/serial.h"
@@ -43,6 +44,7 @@ struct ISASerialState {
     uint32_t iobase;
     uint32_t isairq;
     SerialState state;
+    MemoryRegion io;
 };
 
 static const int isa_serial_io[MAX_ISA_SERIAL_PORTS] = {
@@ -79,8 +81,9 @@ static void serial_isa_realizefn(DeviceState *dev, Error **errp)
     qdev_realize(DEVICE(s), NULL, errp);
     qdev_set_legacy_instance_id(dev, isa->iobase, 3);
 
-    memory_region_init_io(&s->io, OBJECT(isa), &serial_io_ops, s, "serial", 8);
-    isa_register_ioport(isadev, &s->io, isa->iobase);
+    memory_region_init_io(&isa->io, OBJECT(isa), &serial_io_ops, s, "serial",
+                          8);
+    isa_register_ioport(isadev, &isa->io, isa->iobase);
 }
 
 static void serial_isa_build_aml(AcpiDevAmlIf *adev, Aml *scope)
diff --git a/hw/char/serial-pci-multi.c b/hw/char/serial-pci-multi.c
index 5d65c534cb..16cb2faad7 100644
--- a/hw/char/serial-pci-multi.c
+++ b/hw/char/serial-pci-multi.c
@@ -44,6 +44,7 @@ typedef struct PCIMultiSerialState {
     uint32_t     ports;
     char         *name[PCI_SERIAL_MAX_PORTS];
     SerialState  state[PCI_SERIAL_MAX_PORTS];
+    MemoryRegion io[PCI_SERIAL_MAX_PORTS];
     uint32_t     level[PCI_SERIAL_MAX_PORTS];
     qemu_irq     *irqs;
     uint8_t      prog_if;
@@ -58,7 +59,7 @@ static void multi_serial_pci_exit(PCIDevice *dev)
     for (i = 0; i < pci->ports; i++) {
         s = pci->state + i;
         qdev_unrealize(DEVICE(s));
-        memory_region_del_subregion(&pci->iobar, &s->io);
+        memory_region_del_subregion(&pci->iobar, &pci->io[i]);
         g_free(pci->name[i]);
     }
     qemu_free_irqs(pci->irqs, pci->ports);
@@ -112,9 +113,9 @@ static void multi_serial_pci_realize(PCIDevice *dev, Error **errp)
         }
         s->irq = pci->irqs[i];
         pci->name[i] = g_strdup_printf("uart #%zu", i + 1);
-        memory_region_init_io(&s->io, OBJECT(pci), &serial_io_ops, s,
+        memory_region_init_io(&pci->io[i], OBJECT(pci), &serial_io_ops, s,
                               pci->name[i], 8);
-        memory_region_add_subregion(&pci->iobar, 8 * i, &s->io);
+        memory_region_add_subregion(&pci->iobar, 8 * i, &pci->io[i]);
         pci->ports++;
     }
 }
diff --git a/hw/char/serial-pci.c b/hw/char/serial-pci.c
index 087da3059a..ab3d0e56b5 100644
--- a/hw/char/serial-pci.c
+++ b/hw/char/serial-pci.c
@@ -28,6 +28,7 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "qemu/module.h"
+#include "exec/memory.h"
 #include "hw/char/serial.h"
 #include "hw/irq.h"
 #include "hw/pci/pci_device.h"
@@ -38,6 +39,7 @@
 struct PCISerialState {
     PCIDevice dev;
     SerialState state;
+    MemoryRegion io;
     uint8_t prog_if;
 };
 
@@ -57,8 +59,9 @@ static void serial_pci_realize(PCIDevice *dev, Error **errp)
     pci->dev.config[PCI_INTERRUPT_PIN] = 0x01;
     s->irq = pci_allocate_irq(&pci->dev);
 
-    memory_region_init_io(&s->io, OBJECT(pci), &serial_io_ops, s, "serial", 8);
-    pci_register_bar(&pci->dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &s->io);
+    memory_region_init_io(&pci->io, OBJECT(pci), &serial_io_ops, s, "serial",
+                          8);
+    pci_register_bar(&pci->dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &pci->io);
 }
 
 static void serial_pci_exit(PCIDevice *dev)
diff --git a/hw/char/serial.c b/hw/char/serial.c
index a32eb25f58..83b642aec3 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -1045,10 +1045,10 @@ static void serial_mm_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    memory_region_init_io(&s->io, OBJECT(dev),
+    memory_region_init_io(&smm->io, OBJECT(dev),
                           &serial_mm_ops[smm->endianness], smm, "serial",
                           8 << smm->regshift);
-    sysbus_init_mmio(SYS_BUS_DEVICE(smm), &s->io);
+    sysbus_init_mmio(SYS_BUS_DEVICE(smm), &smm->io);
     sysbus_init_irq(SYS_BUS_DEVICE(smm), &smm->serial.irq);
 }
 
-- 
2.43.0

[PATCH v2 04/12] hw/char/parallel: Free struct ParallelState from PortioList

[Bernhard Beschow]

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

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 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 147c900f0d..c1747cbb75 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 v2 05/12] 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 v2 06/12] 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 |  1 +
 include/exec/ioport.h    |  2 ++
 system/ioport.c          | 19 +++++++++++++++++++
 3 files changed, 22 insertions(+)

diff --git a/docs/devel/migration.rst b/docs/devel/migration.rst
index ec55089b25..389fa24bde 100644
--- a/docs/devel/migration.rst
+++ b/docs/devel/migration.rst
@@ -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 v2 07/12] 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 389fa24bde..466be609a2 100644
--- a/docs/devel/migration.rst
+++ b/docs/devel/migration.rst
@@ -465,6 +465,7 @@ Examples of such memory 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 v2 08/12] hw/block/fdc-isa: Implement relocation and toggling for TYPE_ISA_FDC

[Bernhard Beschow]

Implement isa_fdc_set_{enabled,iobase} in order to implement relocation and
toggling of SuperI/O functions in the VIA south bridges without breaking
encapsulation.

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 b4c92b40b3..c989325de3 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 v2 09/12] hw/char/serial-isa: Implement relocation and toggling for TYPE_ISA_SERIAL

[Bernhard Beschow]

Implement isa_serial_set_{enabled,iobase} in order to implement relocation and
toggling of SuperI/O functions in the VIA south bridges without breaking
encapsulation.

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 eb4254edde..ba9f8f21d7 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 2be8be980b..d51c9ec87c 100644
--- a/hw/char/serial-isa.c
+++ b/hw/char/serial-isa.c
@@ -187,3 +187,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->io, s->iobase);
+}
+
+void isa_serial_set_enabled(ISADevice *serial, bool enabled)
+{
+    memory_region_set_enabled(&ISA_SERIAL(serial)->io, enabled);
+}
-- 
2.43.0

[PATCH v2 10/12] hw/char/parallel-isa: Implement relocation and toggling for TYPE_ISA_PARALLEL

[Bernhard Beschow]

Implement isa_parallel_set_{enabled,iobase} in order to implement relocation and
toggling of SuperI/O functions in the VIA south bridges without breaking
encapsulation.

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 v2 11/12] 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 pegasos2.rom would do. For now the meantime this will be
a no-op.

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 v2 12/12] 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 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 -- or in case of pegasos2 on board code if
no -bios is given -- to configure the functions accordingly.

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

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 9c2333a277..be202d23cf 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"
@@ -343,6 +346,35 @@ static const TypeInfo via_superio_info = {
 
 #define TYPE_VT82C686B_SUPERIO "vt82c686b-superio"
 
+static void vt82c686b_superio_update(ViaSuperIOState *s)
+{
+    isa_parallel_set_enabled(s->superio.parallel[0],
+                             (s->regs[0xe2] & 0x3) != 3);
+    isa_serial_set_enabled(s->superio.serial[0], s->regs[0xe2] & BIT(2));
+    isa_serial_set_enabled(s->superio.serial[1], s->regs[0xe2] & BIT(3));
+    isa_fdc_set_enabled(s->superio.floppy, s->regs[0xe2] & BIT(4));
+
+    isa_fdc_set_iobase(s->superio.floppy, (s->regs[0xe3] & 0xfc) << 2);
+    isa_parallel_set_iobase(s->superio.parallel[0], s->regs[0xe6] << 2);
+    isa_serial_set_iobase(s->superio.serial[0], (s->regs[0xe7] & 0xfe) << 2);
+    isa_serial_set_iobase(s->superio.serial[1], (s->regs[0xe8] & 0xfe) << 2);
+}
+
+static int vmstate_vt82c686b_superio_post_load(void *opaque, int version_id)
+{
+    ViaSuperIOState *s = opaque;
+
+    vt82c686b_superio_update(s);
+
+    return 0;
+}
+
+static const VMStateDescription vmstate_vt82c686b_superio = {
+    .name = "vt82c686b_superio",
+    .version_id = 1,
+    .post_load = vmstate_vt82c686b_superio_post_load,
+};
+
 static void vt82c686b_superio_cfg_write(void *opaque, hwaddr addr,
                                         uint64_t data, unsigned size)
 {
@@ -368,7 +400,11 @@ 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 ... 0xe3:
+    case 0xe6 ... 0xe8:
+        sc->regs[idx] = data;
+        vt82c686b_superio_update(sc);
+        return;
     default:
         qemu_log_mask(LOG_UNIMP,
                       "via_superio_cfg: unimplemented register 0x%x\n", idx);
@@ -393,25 +429,24 @@ static void vt82c686b_superio_reset(DeviceState *dev)
 
     memset(s->regs, 0, sizeof(s->regs));
     /* Device ID */
-    vt82c686b_superio_cfg_write(s, 0, 0xe0, 1);
-    vt82c686b_superio_cfg_write(s, 1, 0x3c, 1);
-    /* Function select - all disabled */
-    vt82c686b_superio_cfg_write(s, 0, 0xe2, 1);
-    vt82c686b_superio_cfg_write(s, 1, 0x03, 1);
+    s->regs[0xe0] = 0x3c;
+    /*
+     * 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.
+     */
+    s->regs[0xe2] = 0x0f;
     /* Floppy ctrl base addr 0x3f0-7 */
-    vt82c686b_superio_cfg_write(s, 0, 0xe3, 1);
-    vt82c686b_superio_cfg_write(s, 1, 0xfc, 1);
+    s->regs[0xe3] = 0xfc;
     /* Parallel port base addr 0x378-f */
-    vt82c686b_superio_cfg_write(s, 0, 0xe6, 1);
-    vt82c686b_superio_cfg_write(s, 1, 0xde, 1);
+    s->regs[0xe6] = 0xde;
     /* Serial port 1 base addr 0x3f8-f */
-    vt82c686b_superio_cfg_write(s, 0, 0xe7, 1);
-    vt82c686b_superio_cfg_write(s, 1, 0xfe, 1);
+    s->regs[0xe7] = 0xfe;
     /* Serial port 2 base addr 0x2f8-f */
-    vt82c686b_superio_cfg_write(s, 0, 0xe8, 1);
-    vt82c686b_superio_cfg_write(s, 1, 0xbe, 1);
+    s->regs[0xe8] = 0xbe;
 
-    vt82c686b_superio_cfg_write(s, 0, 0, 1);
+    vt82c686b_superio_update(s);
 }
 
 static void vt82c686b_superio_init(Object *obj)
@@ -429,6 +464,7 @@ static void vt82c686b_superio_class_init(ObjectClass *klass, void *data)
     sc->parallel.count = 1;
     sc->ide.count = 0; /* emulated by via-ide */
     sc->floppy.count = 1;
+    dc->vmsd = &vmstate_vt82c686b_superio;
 }
 
 static const TypeInfo vt82c686b_superio_info = {
@@ -443,6 +479,33 @@ static const TypeInfo vt82c686b_superio_info = {
 
 #define TYPE_VT8231_SUPERIO "vt8231-superio"
 
+static void vt8231_superio_update(ViaSuperIOState *s)
+{
+    isa_parallel_set_enabled(s->superio.parallel[0],
+                             (s->regs[0xf2] & 0x3) != 3);
+    isa_serial_set_enabled(s->superio.serial[0], s->regs[0xf2] & BIT(2));
+    isa_fdc_set_enabled(s->superio.floppy, s->regs[0xf2] & BIT(4));
+
+    isa_serial_set_iobase(s->superio.serial[0], (s->regs[0xf4] & 0xfe) << 2);
+    isa_parallel_set_iobase(s->superio.parallel[0], s->regs[0xf6] << 2);
+    isa_fdc_set_iobase(s->superio.floppy, (s->regs[0xf7] & 0xfc) << 2);
+}
+
+static int vmstate_vt8231_superio_post_load(void *opaque, int version_id)
+{
+    ViaSuperIOState *s = opaque;
+
+    vt8231_superio_update(s);
+
+    return 0;
+}
+
+static const VMStateDescription vmstate_vt8231_superio = {
+    .name = "vt8231_superio",
+    .version_id = 1,
+    .post_load = vmstate_vt8231_superio_post_load,
+};
+
 static void vt8231_superio_cfg_write(void *opaque, hwaddr addr,
                                      uint64_t data, unsigned size)
 {
@@ -465,6 +528,12 @@ static void vt8231_superio_cfg_write(void *opaque, hwaddr addr,
     case 0xfd:
         /* ignore write to read only registers */
         return;
+    case 0xf2:
+    case 0xf4:
+    case 0xf6 ... 0xf7:
+        sc->regs[idx] = data;
+        vt8231_superio_update(sc);
+        return;
     default:
         qemu_log_mask(LOG_UNIMP,
                       "via_superio_cfg: unimplemented register 0x%x\n", idx);
@@ -493,19 +562,15 @@ static void vt8231_superio_reset(DeviceState *dev)
     /* Device revision */
     s->regs[0xf1] = 0x01;
     /* Function select - all disabled */
-    vt8231_superio_cfg_write(s, 0, 0xf2, 1);
-    vt8231_superio_cfg_write(s, 1, 0x03, 1);
+    s->regs[0xf2] = 0x03;
     /* Serial port base addr */
-    vt8231_superio_cfg_write(s, 0, 0xf4, 1);
-    vt8231_superio_cfg_write(s, 1, 0xfe, 1);
+    s->regs[0xf4] = 0xfe;
     /* Parallel port base addr */
-    vt8231_superio_cfg_write(s, 0, 0xf6, 1);
-    vt8231_superio_cfg_write(s, 1, 0xde, 1);
+    s->regs[0xf6] = 0xde;
     /* Floppy ctrl base addr */
-    vt8231_superio_cfg_write(s, 0, 0xf7, 1);
-    vt8231_superio_cfg_write(s, 1, 0xfc, 1);
+    s->regs[0xf7] = 0xfc;
 
-    vt8231_superio_cfg_write(s, 0, 0, 1);
+    vt8231_superio_update(s);
 }
 
 static void vt8231_superio_init(Object *obj)
@@ -513,12 +578,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,10 +585,10 @@ 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;
+    dc->vmsd = &vmstate_vt8231_superio;
 }
 
 static const TypeInfo vt8231_superio_info = {
-- 
2.43.0

Re: [PATCH v2 01/12] hw/block/fdc-isa: Free struct FDCtrl from PortioList

[BALATON Zoltan]

On Mon, 18 Dec 2023, Bernhard Beschow wrote:
> FDCtrl::portio_list isn't used inside FDCtrl context but only inside
> FDCtrlISABus context, so more it there.

"more" -> "move", you have the same typo in several other commit messages. 
Not sure I like the C++ism FDCtrl::portio_list and would write out "The 
portio_list field of FDCtrl" instead but not a big deal. Also the subject 
could say "Move portio_list from FDCtrl to FDCtrlISABus" which is less 
ambiguous than using free that's ususally associated with freeing memory.
Otherwise

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

> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> 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 7ec075e470..b4c92b40b3 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");
>
>

Re: [PATCH v2 02/12] hw/block/fdc-sysbus: Free struct FDCtrl from MemoryRegion

[BALATON Zoltan]

On Mon, 18 Dec 2023, Bernhard Beschow wrote:
> FDCtrl::iomem isn't used inside FDCtrl context but only inside FDCtrlSysBus
> context, so more it there.

Same comments as for patch 1 otherwise

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

> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> 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 86ea51d003..e197b97262 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);
>

Re: [PATCH v2 03/12] hw/char/serial: Free struct SerialState from MemoryRegion

[BALATON Zoltan]

On Mon, 18 Dec 2023, Bernhard Beschow wrote:
> SerialState::io isn't used within TYPE_SERIAL directly. Push it to its users to
> make them the owner of the MemoryRegion.

I'm not sure this patch is needed. The users already own the SerialState 
so can use its memory region so they don't need their own. Since all of 
these need this io region putting it in SerialState saves some 
duplication. Unless I've missed some reason this might be needed.

Regards,
BALATON Zoltan

> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> include/hw/char/serial.h   | 2 +-
> hw/char/serial-isa.c       | 7 +++++--
> hw/char/serial-pci-multi.c | 7 ++++---
> hw/char/serial-pci.c       | 7 +++++--
> hw/char/serial.c           | 4 ++--
> 5 files changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
> index 8ba7eca3d6..eb4254edde 100644
> --- a/include/hw/char/serial.h
> +++ b/include/hw/char/serial.h
> @@ -77,7 +77,6 @@ struct SerialState {
>     int poll_msl;
>
>     QEMUTimer *modem_status_poll;
> -    MemoryRegion io;
> };
> typedef struct SerialState SerialState;
>
> @@ -85,6 +84,7 @@ struct SerialMM {
>     SysBusDevice parent;
>
>     SerialState serial;
> +    MemoryRegion io;
>
>     uint8_t regshift;
>     uint8_t endianness;
> diff --git a/hw/char/serial-isa.c b/hw/char/serial-isa.c
> index 141a6cb168..2be8be980b 100644
> --- a/hw/char/serial-isa.c
> +++ b/hw/char/serial-isa.c
> @@ -26,6 +26,7 @@
> #include "qemu/osdep.h"
> #include "qapi/error.h"
> #include "qemu/module.h"
> +#include "exec/memory.h"
> #include "sysemu/sysemu.h"
> #include "hw/acpi/acpi_aml_interface.h"
> #include "hw/char/serial.h"
> @@ -43,6 +44,7 @@ struct ISASerialState {
>     uint32_t iobase;
>     uint32_t isairq;
>     SerialState state;
> +    MemoryRegion io;
> };
>
> static const int isa_serial_io[MAX_ISA_SERIAL_PORTS] = {
> @@ -79,8 +81,9 @@ static void serial_isa_realizefn(DeviceState *dev, Error **errp)
>     qdev_realize(DEVICE(s), NULL, errp);
>     qdev_set_legacy_instance_id(dev, isa->iobase, 3);
>
> -    memory_region_init_io(&s->io, OBJECT(isa), &serial_io_ops, s, "serial", 8);
> -    isa_register_ioport(isadev, &s->io, isa->iobase);
> +    memory_region_init_io(&isa->io, OBJECT(isa), &serial_io_ops, s, "serial",
> +                          8);
> +    isa_register_ioport(isadev, &isa->io, isa->iobase);
> }
>
> static void serial_isa_build_aml(AcpiDevAmlIf *adev, Aml *scope)
> diff --git a/hw/char/serial-pci-multi.c b/hw/char/serial-pci-multi.c
> index 5d65c534cb..16cb2faad7 100644
> --- a/hw/char/serial-pci-multi.c
> +++ b/hw/char/serial-pci-multi.c
> @@ -44,6 +44,7 @@ typedef struct PCIMultiSerialState {
>     uint32_t     ports;
>     char         *name[PCI_SERIAL_MAX_PORTS];
>     SerialState  state[PCI_SERIAL_MAX_PORTS];
> +    MemoryRegion io[PCI_SERIAL_MAX_PORTS];
>     uint32_t     level[PCI_SERIAL_MAX_PORTS];
>     qemu_irq     *irqs;
>     uint8_t      prog_if;
> @@ -58,7 +59,7 @@ static void multi_serial_pci_exit(PCIDevice *dev)
>     for (i = 0; i < pci->ports; i++) {
>         s = pci->state + i;
>         qdev_unrealize(DEVICE(s));
> -        memory_region_del_subregion(&pci->iobar, &s->io);
> +        memory_region_del_subregion(&pci->iobar, &pci->io[i]);
>         g_free(pci->name[i]);
>     }
>     qemu_free_irqs(pci->irqs, pci->ports);
> @@ -112,9 +113,9 @@ static void multi_serial_pci_realize(PCIDevice *dev, Error **errp)
>         }
>         s->irq = pci->irqs[i];
>         pci->name[i] = g_strdup_printf("uart #%zu", i + 1);
> -        memory_region_init_io(&s->io, OBJECT(pci), &serial_io_ops, s,
> +        memory_region_init_io(&pci->io[i], OBJECT(pci), &serial_io_ops, s,
>                               pci->name[i], 8);
> -        memory_region_add_subregion(&pci->iobar, 8 * i, &s->io);
> +        memory_region_add_subregion(&pci->iobar, 8 * i, &pci->io[i]);
>         pci->ports++;
>     }
> }
> diff --git a/hw/char/serial-pci.c b/hw/char/serial-pci.c
> index 087da3059a..ab3d0e56b5 100644
> --- a/hw/char/serial-pci.c
> +++ b/hw/char/serial-pci.c
> @@ -28,6 +28,7 @@
> #include "qemu/osdep.h"
> #include "qapi/error.h"
> #include "qemu/module.h"
> +#include "exec/memory.h"
> #include "hw/char/serial.h"
> #include "hw/irq.h"
> #include "hw/pci/pci_device.h"
> @@ -38,6 +39,7 @@
> struct PCISerialState {
>     PCIDevice dev;
>     SerialState state;
> +    MemoryRegion io;
>     uint8_t prog_if;
> };
>
> @@ -57,8 +59,9 @@ static void serial_pci_realize(PCIDevice *dev, Error **errp)
>     pci->dev.config[PCI_INTERRUPT_PIN] = 0x01;
>     s->irq = pci_allocate_irq(&pci->dev);
>
> -    memory_region_init_io(&s->io, OBJECT(pci), &serial_io_ops, s, "serial", 8);
> -    pci_register_bar(&pci->dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &s->io);
> +    memory_region_init_io(&pci->io, OBJECT(pci), &serial_io_ops, s, "serial",
> +                          8);
> +    pci_register_bar(&pci->dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &pci->io);
> }
>
> static void serial_pci_exit(PCIDevice *dev)
> diff --git a/hw/char/serial.c b/hw/char/serial.c
> index a32eb25f58..83b642aec3 100644
> --- a/hw/char/serial.c
> +++ b/hw/char/serial.c
> @@ -1045,10 +1045,10 @@ static void serial_mm_realize(DeviceState *dev, Error **errp)
>         return;
>     }
>
> -    memory_region_init_io(&s->io, OBJECT(dev),
> +    memory_region_init_io(&smm->io, OBJECT(dev),
>                           &serial_mm_ops[smm->endianness], smm, "serial",
>                           8 << smm->regshift);
> -    sysbus_init_mmio(SYS_BUS_DEVICE(smm), &s->io);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(smm), &smm->io);
>     sysbus_init_irq(SYS_BUS_DEVICE(smm), &smm->serial.irq);
> }
>
>

Re: [PATCH v2 04/12] hw/char/parallel: Free struct ParallelState from PortioList

[BALATON Zoltan]

On Mon, 18 Dec 2023, Bernhard Beschow wrote:
> ParallelState::portio_list isn't used inside ParallelState context but only
> inside ISAParallelState context, so more it there.

Same comments as for patch 1 otherwise

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

> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> 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 147c900f0d..c1747cbb75 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]),
>

Re: [PATCH v2 06/12] exec/ioport: Add portio_list_set_address()

[BALATON Zoltan]

On Mon, 18 Dec 2023, 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 |  1 +
> include/exec/ioport.h    |  2 ++
> system/ioport.c          | 19 +++++++++++++++++++
> 3 files changed, 22 insertions(+)
>
> diff --git a/docs/devel/migration.rst b/docs/devel/migration.rst
> index ec55089b25..389fa24bde 100644
> --- a/docs/devel/migration.rst
> +++ b/docs/devel/migration.rst
> @@ -464,6 +464,7 @@ Examples of such memory API functions are:
>   - memory_region_set_enabled()
>   - memory_region_set_address()
>   - memory_region_set_alias_offset()

These added here aren't memory API functions so maybe make them a separate 
list with some rewording so that this is not specific to memory API but 
whatever changes memory regions such as memory API or these portio_list 
functions.

Re: [PATCH v2 08/12] hw/block/fdc-isa: Implement relocation and toggling for TYPE_ISA_FDC

[BALATON Zoltan]

On Mon, 18 Dec 2023, Bernhard Beschow wrote:
> Implement isa_fdc_set_{enabled,iobase} in order to implement relocation and
> toggling of SuperI/O functions in the VIA south bridges without breaking
> encapsulation.

You may want to revise these commit messages. What toggling means is only 
defined in the last patch but I can't think of a better name for it other 
than spelling out enable/disable. It's probably also not relevant in this 
commit message to mention VIA south bridges as this is a generic function 
not specific to that usage only.

Regards,
BALATON Zoltan

> 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 b4c92b40b3..c989325de3 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;
>

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

[BALATON Zoltan]

On Mon, 18 Dec 2023, 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 pegasos2.rom would do. For now the meantime this will be

"same way pegasos2 firmware would do". You can drop the last sentence 
about no-op as it does not make much sense as it is or reword it if you 
want to keep it.

Regards,
BALATON Zoltan

> a no-op.
>
> 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) |
>

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

[BALATON Zoltan]

On Mon, 18 Dec 2023, 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 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 -- or in case of pegasos2 on board code if
> no -bios is given -- to configure the functions accordingly.

Pegasos2 emulates firmware when no -bios is given, this was explained in 
previos commit so maybe not needed to be explained it here again so you 
could drop the comment between -- -- but I don't mind.

> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> hw/isa/vt82c686.c | 121 ++++++++++++++++++++++++++++++++++------------
> 1 file changed, 90 insertions(+), 31 deletions(-)
>
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index 9c2333a277..be202d23cf 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"
> @@ -343,6 +346,35 @@ static const TypeInfo via_superio_info = {
>
> #define TYPE_VT82C686B_SUPERIO "vt82c686b-superio"
>
> +static void vt82c686b_superio_update(ViaSuperIOState *s)
> +{
> +    isa_parallel_set_enabled(s->superio.parallel[0],
> +                             (s->regs[0xe2] & 0x3) != 3);
> +    isa_serial_set_enabled(s->superio.serial[0], s->regs[0xe2] & BIT(2));
> +    isa_serial_set_enabled(s->superio.serial[1], s->regs[0xe2] & BIT(3));
> +    isa_fdc_set_enabled(s->superio.floppy, s->regs[0xe2] & BIT(4));
> +
> +    isa_fdc_set_iobase(s->superio.floppy, (s->regs[0xe3] & 0xfc) << 2);
> +    isa_parallel_set_iobase(s->superio.parallel[0], s->regs[0xe6] << 2);
> +    isa_serial_set_iobase(s->superio.serial[0], (s->regs[0xe7] & 0xfe) << 2);
> +    isa_serial_set_iobase(s->superio.serial[1], (s->regs[0xe8] & 0xfe) << 2);
> +}

I wonder if some code duplication could be saved by adding a shared 
via_superio_update() for this further up in the abstract via-superio class 
instead of this method and vt8231_superio_update() below. This common 
method in abstract class would need to handle the differences which seem 
to be reg addresses offset by 0x10 and VT8231 having only 1 serial port. 
These could either be handled by adding function parameters or fields to 
ViaSuperIOState for this that the subclasses can set and the method check. 
(Such as reg base=0xe2 for vt82c686 and 0xf2 for vt8231 and num_serial or 
similar for how many ports are there then can have a for loop for those 
that would only run once for vt8231).

> +static int vmstate_vt82c686b_superio_post_load(void *opaque, int version_id)
> +{
> +    ViaSuperIOState *s = opaque;
> +
> +    vt82c686b_superio_update(s);
> +
> +    return 0;

You could lose some blank lines here. You seem to love them, half of your 
cover letter is just blank lines :-) but I'm the opposite and like more 
code to fit in one screen even on todays displays that are wider than 
tall. So this funciton would take less space without blank lines. (Even 
the local variable may not be necessary as you don't access any fields 
within and void * should just cast without a warning but for spelling out 
the desired type as a reminder I'm ok with leaving that but no excessive 
blank lines please if you don't mind that much.)

Regards,
BALATON Zoltan

> +}
> +
> +static const VMStateDescription vmstate_vt82c686b_superio = {
> +    .name = "vt82c686b_superio",
> +    .version_id = 1,
> +    .post_load = vmstate_vt82c686b_superio_post_load,
> +};
> +
> static void vt82c686b_superio_cfg_write(void *opaque, hwaddr addr,
>                                         uint64_t data, unsigned size)
> {
> @@ -368,7 +400,11 @@ 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 ... 0xe3:
> +    case 0xe6 ... 0xe8:
> +        sc->regs[idx] = data;
> +        vt82c686b_superio_update(sc);
> +        return;
>     default:
>         qemu_log_mask(LOG_UNIMP,
>                       "via_superio_cfg: unimplemented register 0x%x\n", idx);
> @@ -393,25 +429,24 @@ static void vt82c686b_superio_reset(DeviceState *dev)
>
>     memset(s->regs, 0, sizeof(s->regs));
>     /* Device ID */
> -    vt82c686b_superio_cfg_write(s, 0, 0xe0, 1);
> -    vt82c686b_superio_cfg_write(s, 1, 0x3c, 1);
> -    /* Function select - all disabled */
> -    vt82c686b_superio_cfg_write(s, 0, 0xe2, 1);
> -    vt82c686b_superio_cfg_write(s, 1, 0x03, 1);
> +    s->regs[0xe0] = 0x3c;
> +    /*
> +     * 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.
> +     */
> +    s->regs[0xe2] = 0x0f;
>     /* Floppy ctrl base addr 0x3f0-7 */
> -    vt82c686b_superio_cfg_write(s, 0, 0xe3, 1);
> -    vt82c686b_superio_cfg_write(s, 1, 0xfc, 1);
> +    s->regs[0xe3] = 0xfc;
>     /* Parallel port base addr 0x378-f */
> -    vt82c686b_superio_cfg_write(s, 0, 0xe6, 1);
> -    vt82c686b_superio_cfg_write(s, 1, 0xde, 1);
> +    s->regs[0xe6] = 0xde;
>     /* Serial port 1 base addr 0x3f8-f */
> -    vt82c686b_superio_cfg_write(s, 0, 0xe7, 1);
> -    vt82c686b_superio_cfg_write(s, 1, 0xfe, 1);
> +    s->regs[0xe7] = 0xfe;
>     /* Serial port 2 base addr 0x2f8-f */
> -    vt82c686b_superio_cfg_write(s, 0, 0xe8, 1);
> -    vt82c686b_superio_cfg_write(s, 1, 0xbe, 1);
> +    s->regs[0xe8] = 0xbe;
>
> -    vt82c686b_superio_cfg_write(s, 0, 0, 1);
> +    vt82c686b_superio_update(s);
> }
>
> static void vt82c686b_superio_init(Object *obj)
> @@ -429,6 +464,7 @@ static void vt82c686b_superio_class_init(ObjectClass *klass, void *data)
>     sc->parallel.count = 1;
>     sc->ide.count = 0; /* emulated by via-ide */
>     sc->floppy.count = 1;
> +    dc->vmsd = &vmstate_vt82c686b_superio;
> }
>
> static const TypeInfo vt82c686b_superio_info = {
> @@ -443,6 +479,33 @@ static const TypeInfo vt82c686b_superio_info = {
>
> #define TYPE_VT8231_SUPERIO "vt8231-superio"
>
> +static void vt8231_superio_update(ViaSuperIOState *s)
> +{
> +    isa_parallel_set_enabled(s->superio.parallel[0],
> +                             (s->regs[0xf2] & 0x3) != 3);
> +    isa_serial_set_enabled(s->superio.serial[0], s->regs[0xf2] & BIT(2));
> +    isa_fdc_set_enabled(s->superio.floppy, s->regs[0xf2] & BIT(4));
> +
> +    isa_serial_set_iobase(s->superio.serial[0], (s->regs[0xf4] & 0xfe) << 2);
> +    isa_parallel_set_iobase(s->superio.parallel[0], s->regs[0xf6] << 2);
> +    isa_fdc_set_iobase(s->superio.floppy, (s->regs[0xf7] & 0xfc) << 2);
> +}
> +
> +static int vmstate_vt8231_superio_post_load(void *opaque, int version_id)
> +{
> +    ViaSuperIOState *s = opaque;
> +
> +    vt8231_superio_update(s);
> +
> +    return 0;
> +}
> +
> +static const VMStateDescription vmstate_vt8231_superio = {
> +    .name = "vt8231_superio",
> +    .version_id = 1,
> +    .post_load = vmstate_vt8231_superio_post_load,
> +};
> +
> static void vt8231_superio_cfg_write(void *opaque, hwaddr addr,
>                                      uint64_t data, unsigned size)
> {
> @@ -465,6 +528,12 @@ static void vt8231_superio_cfg_write(void *opaque, hwaddr addr,
>     case 0xfd:
>         /* ignore write to read only registers */
>         return;
> +    case 0xf2:
> +    case 0xf4:
> +    case 0xf6 ... 0xf7:
> +        sc->regs[idx] = data;
> +        vt8231_superio_update(sc);
> +        return;
>     default:
>         qemu_log_mask(LOG_UNIMP,
>                       "via_superio_cfg: unimplemented register 0x%x\n", idx);
> @@ -493,19 +562,15 @@ static void vt8231_superio_reset(DeviceState *dev)
>     /* Device revision */
>     s->regs[0xf1] = 0x01;
>     /* Function select - all disabled */
> -    vt8231_superio_cfg_write(s, 0, 0xf2, 1);
> -    vt8231_superio_cfg_write(s, 1, 0x03, 1);
> +    s->regs[0xf2] = 0x03;
>     /* Serial port base addr */
> -    vt8231_superio_cfg_write(s, 0, 0xf4, 1);
> -    vt8231_superio_cfg_write(s, 1, 0xfe, 1);
> +    s->regs[0xf4] = 0xfe;
>     /* Parallel port base addr */
> -    vt8231_superio_cfg_write(s, 0, 0xf6, 1);
> -    vt8231_superio_cfg_write(s, 1, 0xde, 1);
> +    s->regs[0xf6] = 0xde;
>     /* Floppy ctrl base addr */
> -    vt8231_superio_cfg_write(s, 0, 0xf7, 1);
> -    vt8231_superio_cfg_write(s, 1, 0xfc, 1);
> +    s->regs[0xf7] = 0xfc;
>
> -    vt8231_superio_cfg_write(s, 0, 0, 1);
> +    vt8231_superio_update(s);
> }
>
> static void vt8231_superio_init(Object *obj)
> @@ -513,12 +578,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,10 +585,10 @@ 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;
> +    dc->vmsd = &vmstate_vt8231_superio;
> }
>
> static const TypeInfo vt8231_superio_info = {
>

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

[Bernhard Beschow]

Am 19. Dezember 2023 00:26:15 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Mon, 18 Dec 2023, 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 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 -- or in case of pegasos2 on board code if
>> no -bios is given -- to configure the functions accordingly.
>
>Pegasos2 emulates firmware when no -bios is given, this was explained in previos commit so maybe not needed to be explained it here again so you could drop the comment between -- -- but I don't mind.
>
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>> hw/isa/vt82c686.c | 121 ++++++++++++++++++++++++++++++++++------------
>> 1 file changed, 90 insertions(+), 31 deletions(-)
>> 
>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>> index 9c2333a277..be202d23cf 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"
>> @@ -343,6 +346,35 @@ static const TypeInfo via_superio_info = {
>> 
>> #define TYPE_VT82C686B_SUPERIO "vt82c686b-superio"
>> 
>> +static void vt82c686b_superio_update(ViaSuperIOState *s)
>> +{
>> +    isa_parallel_set_enabled(s->superio.parallel[0],
>> +                             (s->regs[0xe2] & 0x3) != 3);
>> +    isa_serial_set_enabled(s->superio.serial[0], s->regs[0xe2] & BIT(2));
>> +    isa_serial_set_enabled(s->superio.serial[1], s->regs[0xe2] & BIT(3));
>> +    isa_fdc_set_enabled(s->superio.floppy, s->regs[0xe2] & BIT(4));
>> +
>> +    isa_fdc_set_iobase(s->superio.floppy, (s->regs[0xe3] & 0xfc) << 2);
>> +    isa_parallel_set_iobase(s->superio.parallel[0], s->regs[0xe6] << 2);
>> +    isa_serial_set_iobase(s->superio.serial[0], (s->regs[0xe7] & 0xfe) << 2);
>> +    isa_serial_set_iobase(s->superio.serial[1], (s->regs[0xe8] & 0xfe) << 2);
>> +}
>
>I wonder if some code duplication could be saved by adding a shared via_superio_update() for this further up in the abstract via-superio class instead of this method and vt8231_superio_update() below. This common method in abstract class would need to handle the differences which seem to be reg addresses offset by 0x10 and VT8231 having only 1 serial port. These could either be handled by adding function parameters or fields to ViaSuperIOState for this that the subclasses can set and the method check. (Such as reg base=0xe2 for vt82c686 and 0xf2 for vt8231 and num_serial or similar for how many ports are there then can have a for loop for those that would only run once for vt8231).

Only the enable bits and the parallel port base address line up, the serial port(s) and the floppy would need special treatment. Not worth it IMO.

>
>> +static int vmstate_vt82c686b_superio_post_load(void *opaque, int version_id)
>> +{
>> +    ViaSuperIOState *s = opaque;
>> +
>> +    vt82c686b_superio_update(s);
>> +
>> +    return 0;
>
>You could lose some blank lines here. You seem to love them, half of your cover letter is just blank lines :-)

Yes, I want people to see the light rather than a wall (of text) ;-)

> but I'm the opposite and like more code to fit in one screen even on todays displays that are wider than tall. So this funciton would take less space without blank lines. (Even the local variable may not be necessary as you don't access any fields within and void * should just cast without a warning but for spelling out the desired type as a reminder I'm ok with leaving that but no excessive blank lines please if you don't mind that much.)

In this case I'll remove the s variable and the blank line above the return if that's so important to you.

Best regards,
Bernhard
>
>Regards,
>BALATON Zoltan
>
>> +}
>> +
>> +static const VMStateDescription vmstate_vt82c686b_superio = {
>> +    .name = "vt82c686b_superio",
>> +    .version_id = 1,
>> +    .post_load = vmstate_vt82c686b_superio_post_load,
>> +};
>> +
>> static void vt82c686b_superio_cfg_write(void *opaque, hwaddr addr,
>>                                         uint64_t data, unsigned size)
>> {
>> @@ -368,7 +400,11 @@ 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 ... 0xe3:
>> +    case 0xe6 ... 0xe8:
>> +        sc->regs[idx] = data;
>> +        vt82c686b_superio_update(sc);
>> +        return;
>>     default:
>>         qemu_log_mask(LOG_UNIMP,
>>                       "via_superio_cfg: unimplemented register 0x%x\n", idx);
>> @@ -393,25 +429,24 @@ static void vt82c686b_superio_reset(DeviceState *dev)
>> 
>>     memset(s->regs, 0, sizeof(s->regs));
>>     /* Device ID */
>> -    vt82c686b_superio_cfg_write(s, 0, 0xe0, 1);
>> -    vt82c686b_superio_cfg_write(s, 1, 0x3c, 1);
>> -    /* Function select - all disabled */
>> -    vt82c686b_superio_cfg_write(s, 0, 0xe2, 1);
>> -    vt82c686b_superio_cfg_write(s, 1, 0x03, 1);
>> +    s->regs[0xe0] = 0x3c;
>> +    /*
>> +     * 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.
>> +     */
>> +    s->regs[0xe2] = 0x0f;
>>     /* Floppy ctrl base addr 0x3f0-7 */
>> -    vt82c686b_superio_cfg_write(s, 0, 0xe3, 1);
>> -    vt82c686b_superio_cfg_write(s, 1, 0xfc, 1);
>> +    s->regs[0xe3] = 0xfc;
>>     /* Parallel port base addr 0x378-f */
>> -    vt82c686b_superio_cfg_write(s, 0, 0xe6, 1);
>> -    vt82c686b_superio_cfg_write(s, 1, 0xde, 1);
>> +    s->regs[0xe6] = 0xde;
>>     /* Serial port 1 base addr 0x3f8-f */
>> -    vt82c686b_superio_cfg_write(s, 0, 0xe7, 1);
>> -    vt82c686b_superio_cfg_write(s, 1, 0xfe, 1);
>> +    s->regs[0xe7] = 0xfe;
>>     /* Serial port 2 base addr 0x2f8-f */
>> -    vt82c686b_superio_cfg_write(s, 0, 0xe8, 1);
>> -    vt82c686b_superio_cfg_write(s, 1, 0xbe, 1);
>> +    s->regs[0xe8] = 0xbe;
>> 
>> -    vt82c686b_superio_cfg_write(s, 0, 0, 1);
>> +    vt82c686b_superio_update(s);
>> }
>> 
>> static void vt82c686b_superio_init(Object *obj)
>> @@ -429,6 +464,7 @@ static void vt82c686b_superio_class_init(ObjectClass *klass, void *data)
>>     sc->parallel.count = 1;
>>     sc->ide.count = 0; /* emulated by via-ide */
>>     sc->floppy.count = 1;
>> +    dc->vmsd = &vmstate_vt82c686b_superio;
>> }
>> 
>> static const TypeInfo vt82c686b_superio_info = {
>> @@ -443,6 +479,33 @@ static const TypeInfo vt82c686b_superio_info = {
>> 
>> #define TYPE_VT8231_SUPERIO "vt8231-superio"
>> 
>> +static void vt8231_superio_update(ViaSuperIOState *s)
>> +{
>> +    isa_parallel_set_enabled(s->superio.parallel[0],
>> +                             (s->regs[0xf2] & 0x3) != 3);
>> +    isa_serial_set_enabled(s->superio.serial[0], s->regs[0xf2] & BIT(2));
>> +    isa_fdc_set_enabled(s->superio.floppy, s->regs[0xf2] & BIT(4));
>> +
>> +    isa_serial_set_iobase(s->superio.serial[0], (s->regs[0xf4] & 0xfe) << 2);
>> +    isa_parallel_set_iobase(s->superio.parallel[0], s->regs[0xf6] << 2);
>> +    isa_fdc_set_iobase(s->superio.floppy, (s->regs[0xf7] & 0xfc) << 2);
>> +}
>> +
>> +static int vmstate_vt8231_superio_post_load(void *opaque, int version_id)
>> +{
>> +    ViaSuperIOState *s = opaque;
>> +
>> +    vt8231_superio_update(s);
>> +
>> +    return 0;
>> +}
>> +
>> +static const VMStateDescription vmstate_vt8231_superio = {
>> +    .name = "vt8231_superio",
>> +    .version_id = 1,
>> +    .post_load = vmstate_vt8231_superio_post_load,
>> +};
>> +
>> static void vt8231_superio_cfg_write(void *opaque, hwaddr addr,
>>                                      uint64_t data, unsigned size)
>> {
>> @@ -465,6 +528,12 @@ static void vt8231_superio_cfg_write(void *opaque, hwaddr addr,
>>     case 0xfd:
>>         /* ignore write to read only registers */
>>         return;
>> +    case 0xf2:
>> +    case 0xf4:
>> +    case 0xf6 ... 0xf7:
>> +        sc->regs[idx] = data;
>> +        vt8231_superio_update(sc);
>> +        return;
>>     default:
>>         qemu_log_mask(LOG_UNIMP,
>>                       "via_superio_cfg: unimplemented register 0x%x\n", idx);
>> @@ -493,19 +562,15 @@ static void vt8231_superio_reset(DeviceState *dev)
>>     /* Device revision */
>>     s->regs[0xf1] = 0x01;
>>     /* Function select - all disabled */
>> -    vt8231_superio_cfg_write(s, 0, 0xf2, 1);
>> -    vt8231_superio_cfg_write(s, 1, 0x03, 1);
>> +    s->regs[0xf2] = 0x03;
>>     /* Serial port base addr */
>> -    vt8231_superio_cfg_write(s, 0, 0xf4, 1);
>> -    vt8231_superio_cfg_write(s, 1, 0xfe, 1);
>> +    s->regs[0xf4] = 0xfe;
>>     /* Parallel port base addr */
>> -    vt8231_superio_cfg_write(s, 0, 0xf6, 1);
>> -    vt8231_superio_cfg_write(s, 1, 0xde, 1);
>> +    s->regs[0xf6] = 0xde;
>>     /* Floppy ctrl base addr */
>> -    vt8231_superio_cfg_write(s, 0, 0xf7, 1);
>> -    vt8231_superio_cfg_write(s, 1, 0xfc, 1);
>> +    s->regs[0xf7] = 0xfc;
>> 
>> -    vt8231_superio_cfg_write(s, 0, 0, 1);
>> +    vt8231_superio_update(s);
>> }
>> 
>> static void vt8231_superio_init(Object *obj)
>> @@ -513,12 +578,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,10 +585,10 @@ 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;
>> +    dc->vmsd = &vmstate_vt8231_superio;
>> }
>> 
>> static const TypeInfo vt8231_superio_info = {
>> 

Re: [PATCH v2 08/12] hw/block/fdc-isa: Implement relocation and toggling for TYPE_ISA_FDC

[Bernhard Beschow]

Am 19. Dezember 2023 00:09:23 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Mon, 18 Dec 2023, Bernhard Beschow wrote:
>> Implement isa_fdc_set_{enabled,iobase} in order to implement relocation and
>> toggling of SuperI/O functions in the VIA south bridges without breaking
>> encapsulation.
>
>You may want to revise these commit messages. What toggling means is only defined in the last patch but I can't think of a better name for it other than spelling out enable/disable.

I'll use enable/disable then.

> It's probably also not relevant in this commit message to mention VIA south bridges as this is a generic function not specific to that usage only.

I'll refer to SuperI/O chips rather than VIA specifically since I want to point out the distinction to properties.

Best regards,
Bernhard

>
>Regards,
>BALATON Zoltan
>
>> 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 b4c92b40b3..c989325de3 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;
>> 

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

[Bernhard Beschow]

Am 19. Dezember 2023 00:11:37 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Mon, 18 Dec 2023, 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 pegasos2.rom would do. For now the meantime this will be
>
>"same way pegasos2 firmware would do".

Good idea. Will change.

> You can drop the last sentence about no-op as it does not make much sense as it is or reword it if you want to keep it.

Yeah, I messed up the last sentence somehow. I'll drop it then.

Best regards,
Bernhard

>
>Regards,
>BALATON Zoltan
>
>> a no-op.
>> 
>> 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) |
>> 

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

[BALATON Zoltan]

On Tue, 19 Dec 2023, Bernhard Beschow wrote:
> Am 19. Dezember 2023 00:26:15 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>> On Mon, 18 Dec 2023, 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 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 -- or in case of pegasos2 on board code if
>>> no -bios is given -- to configure the functions accordingly.
>>
>> Pegasos2 emulates firmware when no -bios is given, this was explained in previos commit so maybe not needed to be explained it here again so you could drop the comment between -- -- but I don't mind.
>>
>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>> ---
>>> hw/isa/vt82c686.c | 121 ++++++++++++++++++++++++++++++++++------------
>>> 1 file changed, 90 insertions(+), 31 deletions(-)
>>>
>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>> index 9c2333a277..be202d23cf 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"
>>> @@ -343,6 +346,35 @@ static const TypeInfo via_superio_info = {
>>>
>>> #define TYPE_VT82C686B_SUPERIO "vt82c686b-superio"
>>>
>>> +static void vt82c686b_superio_update(ViaSuperIOState *s)
>>> +{
>>> +    isa_parallel_set_enabled(s->superio.parallel[0],
>>> +                             (s->regs[0xe2] & 0x3) != 3);
>>> +    isa_serial_set_enabled(s->superio.serial[0], s->regs[0xe2] & BIT(2));
>>> +    isa_serial_set_enabled(s->superio.serial[1], s->regs[0xe2] & BIT(3));
>>> +    isa_fdc_set_enabled(s->superio.floppy, s->regs[0xe2] & BIT(4));
>>> +
>>> +    isa_fdc_set_iobase(s->superio.floppy, (s->regs[0xe3] & 0xfc) << 2);
>>> +    isa_parallel_set_iobase(s->superio.parallel[0], s->regs[0xe6] << 2);
>>> +    isa_serial_set_iobase(s->superio.serial[0], (s->regs[0xe7] & 0xfe) << 2);
>>> +    isa_serial_set_iobase(s->superio.serial[1], (s->regs[0xe8] & 0xfe) << 2);
>>> +}
>>
>> I wonder if some code duplication could be saved by adding a shared via_superio_update() for this further up in the abstract via-superio class instead of this method and vt8231_superio_update() below. This common method in abstract class would need to handle the differences which seem to be reg addresses offset by 0x10 and VT8231 having only 1 serial port. These could either be handled by adding function parameters or fields to ViaSuperIOState for this that the subclasses can set and the method check. (Such as reg base=0xe2 for vt82c686 and 0xf2 for vt8231 and num_serial or similar for how many ports are there then can have a for loop for those that would only run once for vt8231).
>
> Only the enable bits and the parallel port base address line up, the serial port(s) and the floppy would need special treatment. Not worth it IMO.
>
>>
>>> +static int vmstate_vt82c686b_superio_post_load(void *opaque, int version_id)
>>> +{
>>> +    ViaSuperIOState *s = opaque;
>>> +
>>> +    vt82c686b_superio_update(s);
>>> +
>>> +    return 0;
>>
>> You could lose some blank lines here. You seem to love them, half of your cover letter is just blank lines :-)
>
> Yes, I want people to see the light rather than a wall (of text) ;-)

Well, information is in the text and anything else just distracts from it. 
I guess you can configure your editor to present text the way you like but 
it's hard for me or others to get rid of hard formatting so it's better to 
only add the needed text.

>> but I'm the opposite and like more code to fit in one screen even on todays displays that are wider than tall. So this funciton would take less space without blank lines. (Even the local variable may not be necessary as you don't access any fields within and void * should just cast without a warning but for spelling out the desired type as a reminder I'm ok with leaving that but no excessive blank lines please if you don't mind that much.)
>
> In this case I'll remove the s variable and the blank line above the return if that's so important to you.

I think it's simper and more readable that way that's why it's important 
to me.

Regards,
BALATON Zoltan

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

[BALATON Zoltan]

On Tue, 19 Dec 2023, Bernhard Beschow wrote:
> Am 19. Dezember 2023 00:26:15 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>> On Mon, 18 Dec 2023, 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 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 -- or in case of pegasos2 on board code if
>>> no -bios is given -- to configure the functions accordingly.
>>
>> Pegasos2 emulates firmware when no -bios is given, this was explained in previos commit so maybe not needed to be explained it here again so you could drop the comment between -- -- but I don't mind.
>>
>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>> ---
>>> hw/isa/vt82c686.c | 121 ++++++++++++++++++++++++++++++++++------------
>>> 1 file changed, 90 insertions(+), 31 deletions(-)
>>>
>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>> index 9c2333a277..be202d23cf 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"
>>> @@ -343,6 +346,35 @@ static const TypeInfo via_superio_info = {
>>>
>>> #define TYPE_VT82C686B_SUPERIO "vt82c686b-superio"
>>>
>>> +static void vt82c686b_superio_update(ViaSuperIOState *s)
>>> +{
>>> +    isa_parallel_set_enabled(s->superio.parallel[0],
>>> +                             (s->regs[0xe2] & 0x3) != 3);
>>> +    isa_serial_set_enabled(s->superio.serial[0], s->regs[0xe2] & BIT(2));
>>> +    isa_serial_set_enabled(s->superio.serial[1], s->regs[0xe2] & BIT(3));
>>> +    isa_fdc_set_enabled(s->superio.floppy, s->regs[0xe2] & BIT(4));
>>> +
>>> +    isa_fdc_set_iobase(s->superio.floppy, (s->regs[0xe3] & 0xfc) << 2);
>>> +    isa_parallel_set_iobase(s->superio.parallel[0], s->regs[0xe6] << 2);
>>> +    isa_serial_set_iobase(s->superio.serial[0], (s->regs[0xe7] & 0xfe) << 2);
>>> +    isa_serial_set_iobase(s->superio.serial[1], (s->regs[0xe8] & 0xfe) << 2);
>>> +}
>>
>> I wonder if some code duplication could be saved by adding a shared via_superio_update() for this further up in the abstract via-superio class instead of this method and vt8231_superio_update() below. This common method in abstract class would need to handle the differences which seem to be reg addresses offset by 0x10 and VT8231 having only 1 serial port. These could either be handled by adding function parameters or fields to ViaSuperIOState for this that the subclasses can set and the method check. (Such as reg base=0xe2 for vt82c686 and 0xf2 for vt8231 and num_serial or similar for how many ports are there then can have a for loop for those that would only run once for vt8231).
>
> Only the enable bits and the parallel port base address line up, the 
> serial port(s) and the floppy would need special treatment. Not worth it 
> IMO.

Missed this part in previous reply. The serial ports would be taken care 
of by a loop for number of ports so only the floppy needs an if which 
could also use the number of serial ports for lack of better way to 
distinguish these cips easily. Number of ports are in the superio class 
which you could get with ISA_SUPERIO_GET_CLASS (see via_superio_realize) 
then serial.count would be 2 for 686b and 1 for 8231.

But now I think another way may be better that is to drop the 
superio_update function as this updates all functions on writing any 
register unnecessarily and put the lines from it in the 
vt*_superio_cfg_write() functions under the separate cases. This was the 
original intent, that's why the reset function goes through that write 
function so it can enable/disable functions. That way you could apply mask 
on write so via_superio_cfg_read() would return 0 bits as 0 (although the 
data sheet is not clear about what real chip does, just says these must be 
0 not that it's enforced but if we enforce that it's probably better to 
return the effective value on read as well). Then when state saving is 
added in separate patch you can have a similar function as 
vt82c686b_superio_reset() (or rename that to update and make it use 
regs[xx] instead of constant values and call that from reset after setting 
regs values like you did here. But that needs more thought as the vmstate 
added by this patch is incomplete and would not work so you could just 
drop it for now and add it later with also adding other necessary state as 
well. The idea was to implement the chip first then add state saving so we 
don't need to bother with breaking it until we have a good enough 
implementation. So far the state saving there is just left over from the 
old model which never worked and only left there for reminder but only 
wanted to fix once the model is complete enough.

So I think for now you could drop vmstate stuff and distribute the 
superio_update lines in the superio_cfg_write functions so each reg only 
controls the function it should control. Then when vmstate saving is added 
later it could reuse superio_reset as an update function adding a new 
reset func setting reg values and calling the old reset/new update 
function. Does that make sense?

Regards,
BALATON Zoltan

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

[Bernhard Beschow]

Am 24. Dezember 2023 00:51:53 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Tue, 19 Dec 2023, Bernhard Beschow wrote:
>> Am 19. Dezember 2023 00:26:15 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>>> On Mon, 18 Dec 2023, 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 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 -- or in case of pegasos2 on board code if
>>>> no -bios is given -- to configure the functions accordingly.
>>> 
>>> Pegasos2 emulates firmware when no -bios is given, this was explained in previos commit so maybe not needed to be explained it here again so you could drop the comment between -- -- but I don't mind.
>>> 
>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>> ---
>>>> hw/isa/vt82c686.c | 121 ++++++++++++++++++++++++++++++++++------------
>>>> 1 file changed, 90 insertions(+), 31 deletions(-)
>>>> 
>>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>>> index 9c2333a277..be202d23cf 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"
>>>> @@ -343,6 +346,35 @@ static const TypeInfo via_superio_info = {
>>>> 
>>>> #define TYPE_VT82C686B_SUPERIO "vt82c686b-superio"
>>>> 
>>>> +static void vt82c686b_superio_update(ViaSuperIOState *s)
>>>> +{
>>>> +    isa_parallel_set_enabled(s->superio.parallel[0],
>>>> +                             (s->regs[0xe2] & 0x3) != 3);
>>>> +    isa_serial_set_enabled(s->superio.serial[0], s->regs[0xe2] & BIT(2));
>>>> +    isa_serial_set_enabled(s->superio.serial[1], s->regs[0xe2] & BIT(3));
>>>> +    isa_fdc_set_enabled(s->superio.floppy, s->regs[0xe2] & BIT(4));
>>>> +
>>>> +    isa_fdc_set_iobase(s->superio.floppy, (s->regs[0xe3] & 0xfc) << 2);
>>>> +    isa_parallel_set_iobase(s->superio.parallel[0], s->regs[0xe6] << 2);
>>>> +    isa_serial_set_iobase(s->superio.serial[0], (s->regs[0xe7] & 0xfe) << 2);
>>>> +    isa_serial_set_iobase(s->superio.serial[1], (s->regs[0xe8] & 0xfe) << 2);
>>>> +}
>>> 
>>> I wonder if some code duplication could be saved by adding a shared via_superio_update() for this further up in the abstract via-superio class instead of this method and vt8231_superio_update() below. This common method in abstract class would need to handle the differences which seem to be reg addresses offset by 0x10 and VT8231 having only 1 serial port. These could either be handled by adding function parameters or fields to ViaSuperIOState for this that the subclasses can set and the method check. (Such as reg base=0xe2 for vt82c686 and 0xf2 for vt8231 and num_serial or similar for how many ports are there then can have a for loop for those that would only run once for vt8231).
>> 
>> Only the enable bits and the parallel port base address line up, the serial port(s) and the floppy would need special treatment. Not worth it IMO.
>
>Missed this part in previous reply. The serial ports would be taken care of by a loop for number of ports so only the floppy needs an if which could also use the number of serial ports for lack of better way to distinguish these cips easily. Number of ports are in the superio class which you could get with ISA_SUPERIO_GET_CLASS (see via_superio_realize) then serial.count would be 2 for 686b and 1 for 8231.

I'm not very convinced about telling the device models apart by their number of sub devices. So let's omit this part for now.

>
>But now I think another way may be better that is to drop the superio_update function as this updates all functions on writing any register unnecessarily and put the lines from it in the vt*_superio_cfg_write() functions under the separate cases. This was the original intent, that's why the reset function goes through that write function so it can enable/disable functions. That way you could apply mask on write so via_superio_cfg_read() would return 0 bits as 0 (although the data sheet is not clear about what real chip does, just says these must be 0 not that it's enforced but if we enforce that it's probably better to return the effective value on read as well). Then when state saving is added in separate patch you can have a similar function as vt82c686b_superio_reset() (or rename that to update and make it use regs[xx] instead of constant values and call that from reset after setting regs values like you did here. But that needs more thought as the vmstate added by this patch is incomplete and would not work so you could just drop it for now and add it later with also adding other necessary state as well. The idea was to implement the chip first then add state saving so we don't need to bother with breaking it until we have a good enough implementation. So far the state saving there is just left over from the old model which never worked and only left there for reminder but only wanted to fix once the model is complete enough.

Indeed, the patch obviously does too much if it misses details in vmstate. Let's omit vmstate handling for now and go with your suggestion.

Any other comments from your side before the next iteration?

>
>So I think for now you could drop vmstate stuff and distribute the superio_update lines in the superio_cfg_write functions so each reg only controls the function it should control. Then when vmstate saving is added later it could reuse superio_reset as an update function adding a new reset func setting reg values and calling the old reset/new update function. Does that make sense?

What I don't like about the vt*_superio_cfg_write() being called during reset is the trace logs they produce. They are hard to tell apart from guests poking these registers, especially during reboot. So I wonder if this could be addressed when implementing vmstate handling as you suggest. Not too big of a deal, though.

Best regards,
Bernhard

>
>Regards,
>BALATON Zoltan

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

[BALATON Zoltan]

On Tue, 2 Jan 2024, Bernhard Beschow wrote:
> Am 24. Dezember 2023 00:51:53 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>> On Tue, 19 Dec 2023, Bernhard Beschow wrote:
>>> Am 19. Dezember 2023 00:26:15 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>>>> On Mon, 18 Dec 2023, 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 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 -- or in case of pegasos2 on board code if
>>>>> no -bios is given -- to configure the functions accordingly.
>>>>
>>>> Pegasos2 emulates firmware when no -bios is given, this was explained in previos commit so maybe not needed to be explained it here again so you could drop the comment between -- -- but I don't mind.
>>>>
>>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>>> ---
>>>>> hw/isa/vt82c686.c | 121 ++++++++++++++++++++++++++++++++++------------
>>>>> 1 file changed, 90 insertions(+), 31 deletions(-)
>>>>>
>>>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>>>> index 9c2333a277..be202d23cf 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"
>>>>> @@ -343,6 +346,35 @@ static const TypeInfo via_superio_info = {
>>>>>
>>>>> #define TYPE_VT82C686B_SUPERIO "vt82c686b-superio"
>>>>>
>>>>> +static void vt82c686b_superio_update(ViaSuperIOState *s)
>>>>> +{
>>>>> +    isa_parallel_set_enabled(s->superio.parallel[0],
>>>>> +                             (s->regs[0xe2] & 0x3) != 3);
>>>>> +    isa_serial_set_enabled(s->superio.serial[0], s->regs[0xe2] & BIT(2));
>>>>> +    isa_serial_set_enabled(s->superio.serial[1], s->regs[0xe2] & BIT(3));
>>>>> +    isa_fdc_set_enabled(s->superio.floppy, s->regs[0xe2] & BIT(4));
>>>>> +
>>>>> +    isa_fdc_set_iobase(s->superio.floppy, (s->regs[0xe3] & 0xfc) << 2);
>>>>> +    isa_parallel_set_iobase(s->superio.parallel[0], s->regs[0xe6] << 2);
>>>>> +    isa_serial_set_iobase(s->superio.serial[0], (s->regs[0xe7] & 0xfe) << 2);
>>>>> +    isa_serial_set_iobase(s->superio.serial[1], (s->regs[0xe8] & 0xfe) << 2);
>>>>> +}
>>>>
>>>> I wonder if some code duplication could be saved by adding a shared via_superio_update() for this further up in the abstract via-superio class instead of this method and vt8231_superio_update() below. This common method in abstract class would need to handle the differences which seem to be reg addresses offset by 0x10 and VT8231 having only 1 serial port. These could either be handled by adding function parameters or fields to ViaSuperIOState for this that the subclasses can set and the method check. (Such as reg base=0xe2 for vt82c686 and 0xf2 for vt8231 and num_serial or similar for how many ports are there then can have a for loop for those that would only run once for vt8231).
>>>
>>> Only the enable bits and the parallel port base address line up, the serial port(s) and the floppy would need special treatment. Not worth it IMO.
>>
>> Missed this part in previous reply. The serial ports would be taken care of by a loop for number of ports so only the floppy needs an if which could also use the number of serial ports for lack of better way to distinguish these cips easily. Number of ports are in the superio class which you could get with ISA_SUPERIO_GET_CLASS (see via_superio_realize) then serial.count would be 2 for 686b and 1 for 8231.
>
> I'm not very convinced about telling the device models apart by their number of sub devices. So let's omit this part for now.
>
>>
>> But now I think another way may be better that is to drop the 
>> superio_update function as this updates all functions on writing any 
>> register unnecessarily and put the lines from it in the 
>> vt*_superio_cfg_write() functions under the separate cases. This was 
>> the original intent, that's why the reset function goes through that 
>> write function so it can enable/disable functions. That way you could 
>> apply mask on write so via_superio_cfg_read() would return 0 bits as 0 
>> (although the data sheet is not clear about what real chip does, just 
>> says these must be 0 not that it's enforced but if we enforce that it's 
>> probably better to return the effective value on read as well). Then 
>> when state saving is added in separate patch you can have a similar 
>> function as vt82c686b_superio_reset() (or rename that to update and 
>> make it use regs[xx] instead of constant values and call that from 
>> reset after setting regs values like you did here. But that needs more 
>> thought as the vmstate added by this patch is incomplete and would not 
>> work so you could just drop it for now and add it later with also 
>> adding other necessary state as well. The idea was to implement the 
>> chip first then add state saving so we don't need to bother with 
>> breaking it until we have a good enough implementation. So far the 
>> state saving there is just left over from the old model which never 
>> worked and only left there for reminder but only wanted to fix once the 
>> model is complete enough.
>
> Indeed, the patch obviously does too much if it misses details in vmstate. Let's omit vmstate handling for now and go with your suggestion.
>
> Any other comments from your side before the next iteration?

Nothing else from me unless Philippe has something to add. You can keep a 
common function in the abstract via superclass for handling the enable 
bits in the function select register to reduce code duplication as those 
match and handle the address setting separately.

>> So I think for now you could drop vmstate stuff and distribute the 
>> superio_update lines in the superio_cfg_write functions so each reg 
>> only controls the function it should control. Then when vmstate saving 
>> is added later it could reuse superio_reset as an update function 
>> adding a new reset func setting reg values and calling the old 
>> reset/new update function. Does that make sense?
>
> What I don't like about the vt*_superio_cfg_write() being called during 
> reset is the trace logs they produce. They are hard to tell apart from 
> guests poking these registers, especially during reboot. So I wonder if 
> this could be addressed when implementing vmstate handling as you 
> suggest. Not too big of a deal, though.

An easy way around that is to start qemu with -S then these setup logs 
come before qemu stops then logs from guest actions will be printed after 
continue|c in monitor.

Regards,
BALATON Zoltan

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

[Bernhard Beschow]

Am 3. Januar 2024 12:26:07 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Tue, 2 Jan 2024, Bernhard Beschow wrote:
>> Am 24. Dezember 2023 00:51:53 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>>> On Tue, 19 Dec 2023, Bernhard Beschow wrote:
>>>> Am 19. Dezember 2023 00:26:15 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>>>>> On Mon, 18 Dec 2023, 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 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 -- or in case of pegasos2 on board code if
>>>>>> no -bios is given -- to configure the functions accordingly.
>>>>> 
>>>>> Pegasos2 emulates firmware when no -bios is given, this was explained in previos commit so maybe not needed to be explained it here again so you could drop the comment between -- -- but I don't mind.
>>>>> 
>>>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>>>> ---
>>>>>> hw/isa/vt82c686.c | 121 ++++++++++++++++++++++++++++++++++------------
>>>>>> 1 file changed, 90 insertions(+), 31 deletions(-)
>>>>>> 
>>>>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>>>>> index 9c2333a277..be202d23cf 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"
>>>>>> @@ -343,6 +346,35 @@ static const TypeInfo via_superio_info = {
>>>>>> 
>>>>>> #define TYPE_VT82C686B_SUPERIO "vt82c686b-superio"
>>>>>> 
>>>>>> +static void vt82c686b_superio_update(ViaSuperIOState *s)
>>>>>> +{
>>>>>> +    isa_parallel_set_enabled(s->superio.parallel[0],
>>>>>> +                             (s->regs[0xe2] & 0x3) != 3);
>>>>>> +    isa_serial_set_enabled(s->superio.serial[0], s->regs[0xe2] & BIT(2));
>>>>>> +    isa_serial_set_enabled(s->superio.serial[1], s->regs[0xe2] & BIT(3));
>>>>>> +    isa_fdc_set_enabled(s->superio.floppy, s->regs[0xe2] & BIT(4));
>>>>>> +
>>>>>> +    isa_fdc_set_iobase(s->superio.floppy, (s->regs[0xe3] & 0xfc) << 2);
>>>>>> +    isa_parallel_set_iobase(s->superio.parallel[0], s->regs[0xe6] << 2);
>>>>>> +    isa_serial_set_iobase(s->superio.serial[0], (s->regs[0xe7] & 0xfe) << 2);
>>>>>> +    isa_serial_set_iobase(s->superio.serial[1], (s->regs[0xe8] & 0xfe) << 2);
>>>>>> +}
>>>>> 
>>>>> I wonder if some code duplication could be saved by adding a shared via_superio_update() for this further up in the abstract via-superio class instead of this method and vt8231_superio_update() below. This common method in abstract class would need to handle the differences which seem to be reg addresses offset by 0x10 and VT8231 having only 1 serial port. These could either be handled by adding function parameters or fields to ViaSuperIOState for this that the subclasses can set and the method check. (Such as reg base=0xe2 for vt82c686 and 0xf2 for vt8231 and num_serial or similar for how many ports are there then can have a for loop for those that would only run once for vt8231).
>>>> 
>>>> Only the enable bits and the parallel port base address line up, the serial port(s) and the floppy would need special treatment. Not worth it IMO.
>>> 
>>> Missed this part in previous reply. The serial ports would be taken care of by a loop for number of ports so only the floppy needs an if which could also use the number of serial ports for lack of better way to distinguish these cips easily. Number of ports are in the superio class which you could get with ISA_SUPERIO_GET_CLASS (see via_superio_realize) then serial.count would be 2 for 686b and 1 for 8231.
>> 
>> I'm not very convinced about telling the device models apart by their number of sub devices. So let's omit this part for now.
>> 
>>> 
>>> But now I think another way may be better that is to drop the superio_update function as this updates all functions on writing any register unnecessarily and put the lines from it in the vt*_superio_cfg_write() functions under the separate cases. This was the original intent, that's why the reset function goes through that write function so it can enable/disable functions. That way you could apply mask on write so via_superio_cfg_read() would return 0 bits as 0 (although the data sheet is not clear about what real chip does, just says these must be 0 not that it's enforced but if we enforce that it's probably better to return the effective value on read as well). Then when state saving is added in separate patch you can have a similar function as vt82c686b_superio_reset() (or rename that to update and make it use regs[xx] instead of constant values and call that from reset after setting regs values like you did here. But that needs more thought as the vmstate added by this patch is incomplete and would not work so you could just drop it for now and add it later with also adding other necessary state as well. The idea was to implement the chip first then add state saving so we don't need to bother with breaking it until we have a good enough implementation. So far the state saving there is just left over from the old model which never worked and only left there for reminder but only wanted to fix once the model is complete enough.
>> 
>> Indeed, the patch obviously does too much if it misses details in vmstate. Let's omit vmstate handling for now and go with your suggestion.
>> 
>> Any other comments from your side before the next iteration?
>
>Nothing else from me unless Philippe has something to add. You can keep a common function in the abstract via superclass for handling the enable bits in the function select register to reduce code duplication as those match and handle the address setting separately.

I've just sent v4.

Best regards,
Bernhard

>
>>> So I think for now you could drop vmstate stuff and distribute the superio_update lines in the superio_cfg_write functions so each reg only controls the function it should control. Then when vmstate saving is added later it could reuse superio_reset as an update function adding a new reset func setting reg values and calling the old reset/new update function. Does that make sense?
>> 
>> What I don't like about the vt*_superio_cfg_write() being called during reset is the trace logs they produce. They are hard to tell apart from guests poking these registers, especially during reboot. So I wonder if this could be addressed when implementing vmstate handling as you suggest. Not too big of a deal, though.
>
>An easy way around that is to start qemu with -S then these setup logs come before qemu stops then logs from guest actions will be printed after continue|c in monitor.
>
>Regards,
>BALATON Zoltan