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

[PATCH 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-8 export the device structs of
the ISA serial and FDC devices. This is used later in the series to access the
I/O regions for relocation and toggling. As part of the exposal the MAINTAINERS
file gets a fix for the serial headers.

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

Patch 12 finally implements the main feature which required adaption of the
pegasos2 sources, otherwise the machine wouldn't boot when given no bios.

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: Remove unused includes of hw/block/fdc.h
  hw/i386/pc: No need to include hw/block/fdc.h in header
  hw/block/fdc-isa: Rename header to match source file
  hw/block/fdc: Expose internal header
  hw/block/fdc: Move constant #define to where it is imposed
  hw/block/fdc-isa: Expose struct FDCtrlISABus
  MAINTAINERS: Add include/hw/char/serial*.h to the "PC Chipset" section
  hw/char/serial-isa: Export struct ISASerialState
  exec/ioport: Resolve redundant .base attribute in struct
    MemoryRegionPortio
  exec/ioport: Add portio_list_set_address()
  exec/ioport: Add portio_list_set_enabled()
  hw/isa/vt82c686: Implement relocation of SuperI/O functions

 MAINTAINERS                  |   3 +-
 docs/devel/migration.rst     |   2 +
 hw/block/fdc-internal.h      | 158 ----------------------------------
 include/exec/ioport.h        |   4 +-
 include/hw/block/fdc-isa.h   |  32 +++++++
 include/hw/block/fdc.h       | 161 ++++++++++++++++++++++++++++++++---
 include/hw/char/serial-isa.h |  50 +++++++++++
 include/hw/char/serial.h     |   7 --
 include/hw/i386/pc.h         |   1 -
 hw/block/fdc-isa.c           |  19 +----
 hw/block/fdc-sysbus.c        |   2 +-
 hw/block/fdc.c               |   3 +-
 hw/char/serial-isa.c         |  14 +--
 hw/i386/microvm-dt.c         |   2 +-
 hw/i386/microvm.c            |   2 +-
 hw/i386/pc.c                 |   4 +-
 hw/isa/isa-superio.c         |   3 +-
 hw/isa/vt82c686.c            | 140 +++++++++++++++++++++++-------
 hw/m68k/next-cube.c          |   1 -
 hw/mips/jazz.c               |   1 +
 hw/ppc/pegasos2.c            |  15 ++++
 hw/ppc/pnv.c                 |   2 +-
 hw/ppc/prep.c                |   1 -
 hw/sparc/sun4m.c             |   2 +-
 hw/sparc64/sun4u.c           |   2 +
 stubs/cmos.c                 |   2 +-
 system/ioport.c              |  41 +++++++--
 27 files changed, 414 insertions(+), 260 deletions(-)
 delete mode 100644 hw/block/fdc-internal.h
 create mode 100644 include/hw/block/fdc-isa.h
 create mode 100644 include/hw/char/serial-isa.h

-- 
2.43.0

[PATCH 01/12] hw: Remove unused includes of hw/block/fdc.h

[Bernhard Beschow]

Results running grep:

  `grep -i -e "fdc" hw/ppc/prep.c`
  (no output)

  `grep -i -e "fdc" hw/m68k/next-cube.c`
  DPRINTF("FDCSR Write: %x\n", value);

This indicates that hw/block/fdc.h isn't used there.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/m68k/next-cube.c | 1 -
 hw/ppc/prep.c       | 1 -
 2 files changed, 2 deletions(-)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index fabd861941..04989c2648 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -21,7 +21,6 @@
 #include "hw/sysbus.h"
 #include "qom/object.h"
 #include "hw/char/escc.h" /* ZILOG 8530 Serial Emulation */
-#include "hw/block/fdc.h"
 #include "hw/qdev-properties.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
index 137276bcb9..edaed85d95 100644
--- a/hw/ppc/prep.c
+++ b/hw/ppc/prep.c
@@ -26,7 +26,6 @@
 #include "qemu/osdep.h"
 #include "hw/rtc/m48t59.h"
 #include "hw/char/serial.h"
-#include "hw/block/fdc.h"
 #include "net/net.h"
 #include "hw/isa/isa.h"
 #include "hw/pci/pci.h"
-- 
2.43.0

[PATCH 02/12] hw/i386/pc: No need to include hw/block/fdc.h in header

[Bernhard Beschow]

Only the source file uses fdc.h but not the header, so remove it from the public
interface.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/i386/pc.h | 1 -
 hw/i386/pc.c         | 1 +
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index a10ceeabbf..48097c9124 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -5,7 +5,6 @@
 #include "qapi/qapi-types-common.h"
 #include "qemu/uuid.h"
 #include "hw/boards.h"
-#include "hw/block/fdc.h"
 #include "hw/block/flash.h"
 #include "hw/i386/x86.h"
 
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 29b9964733..0d732b7530 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -25,6 +25,7 @@
 #include "qemu/osdep.h"
 #include "qemu/units.h"
 #include "hw/i386/pc.h"
+#include "hw/block/fdc.h"
 #include "hw/char/serial.h"
 #include "hw/char/parallel.h"
 #include "hw/hyperv/hv-balloon.h"
-- 
2.43.0

[PATCH 03/12] hw/block/fdc-isa: Rename header to match source file

[Bernhard Beschow]

Rename the header which allows for exposing fdc-internal.h (dropping the
-internal suffix) which in turn allows for exposing struct FDCtrlISABus.
Exposing a device struct is in line with OOM/qdev guidelines.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 MAINTAINERS                           | 2 +-
 hw/block/fdc-internal.h               | 2 +-
 include/hw/block/{fdc.h => fdc-isa.h} | 4 ++--
 hw/block/fdc-isa.c                    | 2 +-
 hw/block/fdc-sysbus.c                 | 2 +-
 hw/block/fdc.c                        | 2 +-
 hw/i386/pc.c                          | 2 +-
 hw/isa/isa-superio.c                  | 2 +-
 hw/mips/jazz.c                        | 2 +-
 hw/sparc/sun4m.c                      | 2 +-
 hw/sparc64/sun4u.c                    | 2 +-
 stubs/cmos.c                          | 2 +-
 12 files changed, 13 insertions(+), 13 deletions(-)
 rename include/hw/block/{fdc.h => fdc-isa.h} (92%)

diff --git a/MAINTAINERS b/MAINTAINERS
index 695e0bd34f..b4718fcf59 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1948,7 +1948,7 @@ F: hw/block/fdc.c
 F: hw/block/fdc-internal.h
 F: hw/block/fdc-isa.c
 F: hw/block/fdc-sysbus.c
-F: include/hw/block/fdc.h
+F: include/hw/block/fdc-isa.h
 F: tests/qtest/fdc-test.c
 T: git https://gitlab.com/jsnow/qemu.git ide
 
diff --git a/hw/block/fdc-internal.h b/hw/block/fdc-internal.h
index 036392e9fc..1728231a26 100644
--- a/hw/block/fdc-internal.h
+++ b/hw/block/fdc-internal.h
@@ -28,7 +28,7 @@
 #include "exec/memory.h"
 #include "exec/ioport.h"
 #include "hw/block/block.h"
-#include "hw/block/fdc.h"
+#include "hw/block/fdc-isa.h"
 #include "qapi/qapi-types-block.h"
 
 typedef struct FDCtrl FDCtrl;
diff --git a/include/hw/block/fdc.h b/include/hw/block/fdc-isa.h
similarity index 92%
rename from include/hw/block/fdc.h
rename to include/hw/block/fdc-isa.h
index 35248c0837..95807fdc65 100644
--- a/include/hw/block/fdc.h
+++ b/include/hw/block/fdc-isa.h
@@ -1,5 +1,5 @@
-#ifndef HW_FDC_H
-#define HW_FDC_H
+#ifndef HW_FDC_ISA_H
+#define HW_FDC_ISA_H
 
 #include "exec/hwaddr.h"
 #include "qapi/qapi-types-block.h"
diff --git a/hw/block/fdc-isa.c b/hw/block/fdc-isa.c
index 7ec075e470..6387dc94fa 100644
--- a/hw/block/fdc-isa.c
+++ b/hw/block/fdc-isa.c
@@ -28,7 +28,7 @@
  */
 
 #include "qemu/osdep.h"
-#include "hw/block/fdc.h"
+#include "hw/block/fdc-isa.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "qemu/timer.h"
diff --git a/hw/block/fdc-sysbus.c b/hw/block/fdc-sysbus.c
index 86ea51d003..f18f0d19b0 100644
--- a/hw/block/fdc-sysbus.c
+++ b/hw/block/fdc-sysbus.c
@@ -27,7 +27,7 @@
 #include "qapi/error.h"
 #include "qom/object.h"
 #include "hw/sysbus.h"
-#include "hw/block/fdc.h"
+#include "hw/block/fdc-isa.h"
 #include "migration/vmstate.h"
 #include "fdc-internal.h"
 #include "trace.h"
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index d7cc4d3ec1..2bd6d925b5 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -28,7 +28,7 @@
  */
 
 #include "qemu/osdep.h"
-#include "hw/block/fdc.h"
+#include "hw/block/fdc-isa.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "qemu/timer.h"
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 0d732b7530..aeecf56e72 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -25,7 +25,7 @@
 #include "qemu/osdep.h"
 #include "qemu/units.h"
 #include "hw/i386/pc.h"
-#include "hw/block/fdc.h"
+#include "hw/block/fdc-isa.h"
 #include "hw/char/serial.h"
 #include "hw/char/parallel.h"
 #include "hw/hyperv/hv-balloon.h"
diff --git a/hw/isa/isa-superio.c b/hw/isa/isa-superio.c
index 7dbfc374da..ea6cb4213f 100644
--- a/hw/isa/isa-superio.c
+++ b/hw/isa/isa-superio.c
@@ -17,7 +17,7 @@
 #include "sysemu/blockdev.h"
 #include "chardev/char.h"
 #include "hw/char/parallel.h"
-#include "hw/block/fdc.h"
+#include "hw/block/fdc-isa.h"
 #include "hw/isa/superio.h"
 #include "hw/qdev-properties.h"
 #include "hw/input/i8042.h"
diff --git a/hw/mips/jazz.c b/hw/mips/jazz.c
index d33a76ad4d..bc74d1fd96 100644
--- a/hw/mips/jazz.c
+++ b/hw/mips/jazz.c
@@ -31,7 +31,7 @@
 #include "hw/char/serial.h"
 #include "hw/char/parallel.h"
 #include "hw/isa/isa.h"
-#include "hw/block/fdc.h"
+#include "hw/block/fdc-isa.h"
 #include "sysemu/sysemu.h"
 #include "hw/boards.h"
 #include "net/net.h"
diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
index 17bf5f2879..751e52b282 100644
--- a/hw/sparc/sun4m.c
+++ b/hw/sparc/sun4m.c
@@ -34,7 +34,7 @@
 #include "hw/rtc/m48t59.h"
 #include "migration/vmstate.h"
 #include "hw/sparc/sparc32_dma.h"
-#include "hw/block/fdc.h"
+#include "hw/block/fdc-isa.h"
 #include "sysemu/reset.h"
 #include "sysemu/runstate.h"
 #include "sysemu/sysemu.h"
diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index c871170378..9a88772f6f 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -39,7 +39,7 @@
 #include "hw/rtc/m48t59.h"
 #include "migration/vmstate.h"
 #include "hw/input/i8042.h"
-#include "hw/block/fdc.h"
+#include "hw/block/fdc-isa.h"
 #include "net/net.h"
 #include "qemu/timer.h"
 #include "sysemu/runstate.h"
diff --git a/stubs/cmos.c b/stubs/cmos.c
index 3fdbae2c69..32d921e065 100644
--- a/stubs/cmos.c
+++ b/stubs/cmos.c
@@ -1,5 +1,5 @@
 #include "qemu/osdep.h"
-#include "hw/block/fdc.h"
+#include "hw/block/fdc-isa.h"
 
 int cmos_get_fd_drive_type(FloppyDriveType fd0)
 {
-- 
2.43.0

[PATCH 04/12] hw/block/fdc: Expose internal header

[Bernhard Beschow]

Exposing the internal header allows for exposing struct FDCtrlISABus which is
encuraged by qdev guidelines.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 MAINTAINERS                                       | 2 +-
 hw/block/fdc-internal.h => include/hw/block/fdc.h | 4 ++--
 hw/block/fdc-isa.c                                | 2 +-
 hw/block/fdc-sysbus.c                             | 2 +-
 hw/block/fdc.c                                    | 2 +-
 5 files changed, 6 insertions(+), 6 deletions(-)
 rename hw/block/fdc-internal.h => include/hw/block/fdc.h (98%)

diff --git a/MAINTAINERS b/MAINTAINERS
index b4718fcf59..939f518701 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1945,9 +1945,9 @@ M: John Snow <jsnow@redhat.com>
 L: qemu-block@nongnu.org
 S: Odd Fixes
 F: hw/block/fdc.c
-F: hw/block/fdc-internal.h
 F: hw/block/fdc-isa.c
 F: hw/block/fdc-sysbus.c
+F: include/hw/block/fdc.h
 F: include/hw/block/fdc-isa.h
 F: tests/qtest/fdc-test.c
 T: git https://gitlab.com/jsnow/qemu.git ide
diff --git a/hw/block/fdc-internal.h b/include/hw/block/fdc.h
similarity index 98%
rename from hw/block/fdc-internal.h
rename to include/hw/block/fdc.h
index 1728231a26..acca7e0d0e 100644
--- a/hw/block/fdc-internal.h
+++ b/include/hw/block/fdc.h
@@ -22,8 +22,8 @@
  * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
  * THE SOFTWARE.
  */
-#ifndef HW_BLOCK_FDC_INTERNAL_H
-#define HW_BLOCK_FDC_INTERNAL_H
+#ifndef HW_BLOCK_FDC_H
+#define HW_BLOCK_FDC_H
 
 #include "exec/memory.h"
 #include "exec/ioport.h"
diff --git a/hw/block/fdc-isa.c b/hw/block/fdc-isa.c
index 6387dc94fa..7058d4118f 100644
--- a/hw/block/fdc-isa.c
+++ b/hw/block/fdc-isa.c
@@ -39,6 +39,7 @@
 #include "hw/qdev-properties-system.h"
 #include "migration/vmstate.h"
 #include "hw/block/block.h"
+#include "hw/block/fdc.h"
 #include "sysemu/block-backend.h"
 #include "sysemu/blockdev.h"
 #include "sysemu/sysemu.h"
@@ -47,7 +48,6 @@
 #include "qemu/module.h"
 #include "trace.h"
 #include "qom/object.h"
-#include "fdc-internal.h"
 
 OBJECT_DECLARE_SIMPLE_TYPE(FDCtrlISABus, ISA_FDC)
 
diff --git a/hw/block/fdc-sysbus.c b/hw/block/fdc-sysbus.c
index f18f0d19b0..cff21c02b3 100644
--- a/hw/block/fdc-sysbus.c
+++ b/hw/block/fdc-sysbus.c
@@ -28,8 +28,8 @@
 #include "qom/object.h"
 #include "hw/sysbus.h"
 #include "hw/block/fdc-isa.h"
+#include "hw/block/fdc.h"
 #include "migration/vmstate.h"
-#include "fdc-internal.h"
 #include "trace.h"
 
 #define TYPE_SYSBUS_FDC "base-sysbus-fdc"
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 2bd6d925b5..0e2fa527f9 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -39,6 +39,7 @@
 #include "hw/qdev-properties-system.h"
 #include "migration/vmstate.h"
 #include "hw/block/block.h"
+#include "hw/block/fdc.h"
 #include "sysemu/block-backend.h"
 #include "sysemu/blockdev.h"
 #include "sysemu/sysemu.h"
@@ -47,7 +48,6 @@
 #include "qemu/module.h"
 #include "trace.h"
 #include "qom/object.h"
-#include "fdc-internal.h"
 
 /********************************************************/
 /* debug Floppy devices */
-- 
2.43.0

Re: [PATCH 04/12] hw/block/fdc: Expose internal header

[BALATON Zoltan]

On Sun, 17 Dec 2023, Bernhard Beschow wrote:
> Exposing the internal header allows for exposing struct FDCtrlISABus which is
> encuraged by qdev guidelines.

Hopefully the guidelines don't encourage this as object orientation indeed 
encourages object encapsulation so only the object itseld should poke its 
internals and other objects should use methods the change object state. In 
QOM some object states were exposed in public headers to allow embedding 
those objects in other objects becuase C needs the struct size to allow 
that. This was to simplify memory management so the embedded objects don't 
need to be tracked and freed but would be created and freed with the other 
object embedding it but this does not mean the other object should poke 
into these object or that this is a general guideline to expose internal 
object state. I'd say the exposed objects are an exception instead of 
recommended guideline and only allowed for objects that need to be embeded 
in others but generally object encapsulation would be better to preserve 
where possible. This patch exposes objects so others can poke into them 
which would make those other objects dependent on the implementation of 
these objects making these harder to chnage in the future so a better way 
may be to add methods to fdc and serial to allow changing their base 
address and map/unmap their ports and keep their internals unexposed.

Regards,
BALATON Zoltan

> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> MAINTAINERS                                       | 2 +-
> hw/block/fdc-internal.h => include/hw/block/fdc.h | 4 ++--
> hw/block/fdc-isa.c                                | 2 +-
> hw/block/fdc-sysbus.c                             | 2 +-
> hw/block/fdc.c                                    | 2 +-
> 5 files changed, 6 insertions(+), 6 deletions(-)
> rename hw/block/fdc-internal.h => include/hw/block/fdc.h (98%)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b4718fcf59..939f518701 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1945,9 +1945,9 @@ M: John Snow <jsnow@redhat.com>
> L: qemu-block@nongnu.org
> S: Odd Fixes
> F: hw/block/fdc.c
> -F: hw/block/fdc-internal.h
> F: hw/block/fdc-isa.c
> F: hw/block/fdc-sysbus.c
> +F: include/hw/block/fdc.h
> F: include/hw/block/fdc-isa.h
> F: tests/qtest/fdc-test.c
> T: git https://gitlab.com/jsnow/qemu.git ide
> diff --git a/hw/block/fdc-internal.h b/include/hw/block/fdc.h
> similarity index 98%
> rename from hw/block/fdc-internal.h
> rename to include/hw/block/fdc.h
> index 1728231a26..acca7e0d0e 100644
> --- a/hw/block/fdc-internal.h
> +++ b/include/hw/block/fdc.h
> @@ -22,8 +22,8 @@
>  * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>  * THE SOFTWARE.
>  */
> -#ifndef HW_BLOCK_FDC_INTERNAL_H
> -#define HW_BLOCK_FDC_INTERNAL_H
> +#ifndef HW_BLOCK_FDC_H
> +#define HW_BLOCK_FDC_H
>
> #include "exec/memory.h"
> #include "exec/ioport.h"
> diff --git a/hw/block/fdc-isa.c b/hw/block/fdc-isa.c
> index 6387dc94fa..7058d4118f 100644
> --- a/hw/block/fdc-isa.c
> +++ b/hw/block/fdc-isa.c
> @@ -39,6 +39,7 @@
> #include "hw/qdev-properties-system.h"
> #include "migration/vmstate.h"
> #include "hw/block/block.h"
> +#include "hw/block/fdc.h"
> #include "sysemu/block-backend.h"
> #include "sysemu/blockdev.h"
> #include "sysemu/sysemu.h"
> @@ -47,7 +48,6 @@
> #include "qemu/module.h"
> #include "trace.h"
> #include "qom/object.h"
> -#include "fdc-internal.h"
>
> OBJECT_DECLARE_SIMPLE_TYPE(FDCtrlISABus, ISA_FDC)
>
> diff --git a/hw/block/fdc-sysbus.c b/hw/block/fdc-sysbus.c
> index f18f0d19b0..cff21c02b3 100644
> --- a/hw/block/fdc-sysbus.c
> +++ b/hw/block/fdc-sysbus.c
> @@ -28,8 +28,8 @@
> #include "qom/object.h"
> #include "hw/sysbus.h"
> #include "hw/block/fdc-isa.h"
> +#include "hw/block/fdc.h"
> #include "migration/vmstate.h"
> -#include "fdc-internal.h"
> #include "trace.h"
>
> #define TYPE_SYSBUS_FDC "base-sysbus-fdc"
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index 2bd6d925b5..0e2fa527f9 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -39,6 +39,7 @@
> #include "hw/qdev-properties-system.h"
> #include "migration/vmstate.h"
> #include "hw/block/block.h"
> +#include "hw/block/fdc.h"
> #include "sysemu/block-backend.h"
> #include "sysemu/blockdev.h"
> #include "sysemu/sysemu.h"
> @@ -47,7 +48,6 @@
> #include "qemu/module.h"
> #include "trace.h"
> #include "qom/object.h"
> -#include "fdc-internal.h"
>
> /********************************************************/
> /* debug Floppy devices */
>

Re: [PATCH 04/12] hw/block/fdc: Expose internal header

[Bernhard Beschow]

Am 17. Dezember 2023 15:47:33 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Sun, 17 Dec 2023, Bernhard Beschow wrote:
>> Exposing the internal header allows for exposing struct FDCtrlISABus which is
>> encuraged by qdev guidelines.
>
>Hopefully the guidelines don't encourage this as object orientation indeed encourages object encapsulation so only the object itseld should poke its internals and other objects should use methods the change object state. In QOM some object states were exposed in public headers to allow embedding those objects in other objects becuase C needs the struct size to allow that. This was to simplify memory management so the embedded objects don't need to be tracked and freed but would be created and freed with the other object embedding it but this does not mean the other object should poke into these object or that this is a general guideline to expose internal object state. I'd say the exposed objects are an exception instead of recommended guideline and only allowed for objects that need to be embeded in others but generally object encapsulation would be better to preserve where possible. This patch exposes objects so others can poke into them which would make those other objects dependent on the implementation of these objects making these harder to chnage in the future so a better way may be to add methods to fdc and serial to allow changing their base address and map/unmap their ports and keep their internals unexposed.

Each ISADevice sub class would need concenience methods as well as each state  class. This series touches three of each: fdc, parallel, serial. And each of those need two convenience methods: set_enabled() and set_address(). This would add another 12 functions on top of the current ones.

Then ISASuperIODevice would require at least 6 more such methods (not counting the unneeded ones for IDE which might be desirable for consistency). So in the end we'd have at least 18 more methods. Is this really worth it?

I didn't feel very comfortable going this route, so ended up with the current solution poking the states directly. I'm open to different approaches including the one above but I'd really like to know the opinion of the maintainers, too.

Best regards,
Bernhard

>
>Regards,
>BALATON Zoltan
>
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>> MAINTAINERS                                       | 2 +-
>> hw/block/fdc-internal.h => include/hw/block/fdc.h | 4 ++--
>> hw/block/fdc-isa.c                                | 2 +-
>> hw/block/fdc-sysbus.c                             | 2 +-
>> hw/block/fdc.c                                    | 2 +-
>> 5 files changed, 6 insertions(+), 6 deletions(-)
>> rename hw/block/fdc-internal.h => include/hw/block/fdc.h (98%)
>> 
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index b4718fcf59..939f518701 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -1945,9 +1945,9 @@ M: John Snow <jsnow@redhat.com>
>> L: qemu-block@nongnu.org
>> S: Odd Fixes
>> F: hw/block/fdc.c
>> -F: hw/block/fdc-internal.h
>> F: hw/block/fdc-isa.c
>> F: hw/block/fdc-sysbus.c
>> +F: include/hw/block/fdc.h
>> F: include/hw/block/fdc-isa.h
>> F: tests/qtest/fdc-test.c
>> T: git https://gitlab.com/jsnow/qemu.git ide
>> diff --git a/hw/block/fdc-internal.h b/include/hw/block/fdc.h
>> similarity index 98%
>> rename from hw/block/fdc-internal.h
>> rename to include/hw/block/fdc.h
>> index 1728231a26..acca7e0d0e 100644
>> --- a/hw/block/fdc-internal.h
>> +++ b/include/hw/block/fdc.h
>> @@ -22,8 +22,8 @@
>>  * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>>  * THE SOFTWARE.
>>  */
>> -#ifndef HW_BLOCK_FDC_INTERNAL_H
>> -#define HW_BLOCK_FDC_INTERNAL_H
>> +#ifndef HW_BLOCK_FDC_H
>> +#define HW_BLOCK_FDC_H
>> 
>> #include "exec/memory.h"
>> #include "exec/ioport.h"
>> diff --git a/hw/block/fdc-isa.c b/hw/block/fdc-isa.c
>> index 6387dc94fa..7058d4118f 100644
>> --- a/hw/block/fdc-isa.c
>> +++ b/hw/block/fdc-isa.c
>> @@ -39,6 +39,7 @@
>> #include "hw/qdev-properties-system.h"
>> #include "migration/vmstate.h"
>> #include "hw/block/block.h"
>> +#include "hw/block/fdc.h"
>> #include "sysemu/block-backend.h"
>> #include "sysemu/blockdev.h"
>> #include "sysemu/sysemu.h"
>> @@ -47,7 +48,6 @@
>> #include "qemu/module.h"
>> #include "trace.h"
>> #include "qom/object.h"
>> -#include "fdc-internal.h"
>> 
>> OBJECT_DECLARE_SIMPLE_TYPE(FDCtrlISABus, ISA_FDC)
>> 
>> diff --git a/hw/block/fdc-sysbus.c b/hw/block/fdc-sysbus.c
>> index f18f0d19b0..cff21c02b3 100644
>> --- a/hw/block/fdc-sysbus.c
>> +++ b/hw/block/fdc-sysbus.c
>> @@ -28,8 +28,8 @@
>> #include "qom/object.h"
>> #include "hw/sysbus.h"
>> #include "hw/block/fdc-isa.h"
>> +#include "hw/block/fdc.h"
>> #include "migration/vmstate.h"
>> -#include "fdc-internal.h"
>> #include "trace.h"
>> 
>> #define TYPE_SYSBUS_FDC "base-sysbus-fdc"
>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>> index 2bd6d925b5..0e2fa527f9 100644
>> --- a/hw/block/fdc.c
>> +++ b/hw/block/fdc.c
>> @@ -39,6 +39,7 @@
>> #include "hw/qdev-properties-system.h"
>> #include "migration/vmstate.h"
>> #include "hw/block/block.h"
>> +#include "hw/block/fdc.h"
>> #include "sysemu/block-backend.h"
>> #include "sysemu/blockdev.h"
>> #include "sysemu/sysemu.h"
>> @@ -47,7 +48,6 @@
>> #include "qemu/module.h"
>> #include "trace.h"
>> #include "qom/object.h"
>> -#include "fdc-internal.h"
>> 
>> /********************************************************/
>> /* debug Floppy devices */
>> 

Re: [PATCH 04/12] hw/block/fdc: Expose internal header

[BALATON Zoltan]

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

On Sun, 17 Dec 2023, Bernhard Beschow wrote:
> Am 17. Dezember 2023 15:47:33 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>> On Sun, 17 Dec 2023, Bernhard Beschow wrote:
>>> Exposing the internal header allows for exposing struct FDCtrlISABus which is
>>> encuraged by qdev guidelines.
>>
>> Hopefully the guidelines don't encourage this as object orientation indeed encourages object encapsulation so only the object itseld should poke its internals and other objects should use methods the change object state. In QOM some object states were exposed in public headers to allow embedding those objects in other objects becuase C needs the struct size to allow that. This was to simplify memory management so the embedded objects don't need to be tracked and freed but would be created and freed with the other object embedding it but this does not mean the other object should poke into these object or that this is a general guideline to expose internal object state. I'd say the exposed objects are an exception instead of recommended guideline and only allowed for objects that need to be embeded in others but generally object encapsulation would be better to preserve where possible. This patch exposes objects so others can poke into them which would make those other objects depe
 ndent on the implementation of these objects making these harder to chnage in the future so a better way may be to add methods to fdc and serial to allow changing their base address and map/unmap their ports and keep their internals unexposed.
>
> Each ISADevice sub class would need concenience methods as well as each 
> state class. This series touches three of each: fdc, parallel, serial. 
> And each of those need two convenience methods: set_enabled() and 
> set_address(). This would add another 12 functions on top of the current 
> ones.

If all ISA devices need this then these should really be methods of 
ISADevice but since that's just an empty wrapper over devices each of 
which handles its own ports, the ISADevice does not know about those and 
since each device may have different ports and not all of them uses portio 
lists for this, moving port handling to ISADevice might be too big 
refactoring to do for this. Keeping these functions with the superio 
component devices so their implementation is kept private still worth it 
in my opinion so even if that adds 2 functions to superio component 
devices (which is not all ISA devices just a limited set) seems to be a 
better approach to me than breaking encapsulation of objects. These are 
simple access methods for internal object state which are common in object 
otiented programming.

> Then ISASuperIODevice would require at least 6 more such methods (not 
> counting the unneeded ones for IDE which might be desirable for 
> consistency). So in the end we'd have at least 18 more methods. Is this 
> really worth it?

We may do without these if we say superio is just a container of 
components so don't add forwarding methods but we can call the accessor 
methods of component objects from vt82c686.c. That's still better than 
reaching into object internals from foreign objects.

Regards,
BALATON Zoltan

> I didn't feel very comfortable going this route, so ended up with the 
> current solution poking the states directly. I'm open to different 
> approaches including the one above but I'd really like to know the 
> opinion of the maintainers, too.
>
> Best regards,
> Bernhard
>
>>
>> Regards,
>> BALATON Zoltan
>>
>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>> ---
>>> MAINTAINERS                                       | 2 +-
>>> hw/block/fdc-internal.h => include/hw/block/fdc.h | 4 ++--
>>> hw/block/fdc-isa.c                                | 2 +-
>>> hw/block/fdc-sysbus.c                             | 2 +-
>>> hw/block/fdc.c                                    | 2 +-
>>> 5 files changed, 6 insertions(+), 6 deletions(-)
>>> rename hw/block/fdc-internal.h => include/hw/block/fdc.h (98%)
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index b4718fcf59..939f518701 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -1945,9 +1945,9 @@ M: John Snow <jsnow@redhat.com>
>>> L: qemu-block@nongnu.org
>>> S: Odd Fixes
>>> F: hw/block/fdc.c
>>> -F: hw/block/fdc-internal.h
>>> F: hw/block/fdc-isa.c
>>> F: hw/block/fdc-sysbus.c
>>> +F: include/hw/block/fdc.h
>>> F: include/hw/block/fdc-isa.h
>>> F: tests/qtest/fdc-test.c
>>> T: git https://gitlab.com/jsnow/qemu.git ide
>>> diff --git a/hw/block/fdc-internal.h b/include/hw/block/fdc.h
>>> similarity index 98%
>>> rename from hw/block/fdc-internal.h
>>> rename to include/hw/block/fdc.h
>>> index 1728231a26..acca7e0d0e 100644
>>> --- a/hw/block/fdc-internal.h
>>> +++ b/include/hw/block/fdc.h
>>> @@ -22,8 +22,8 @@
>>>  * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>>>  * THE SOFTWARE.
>>>  */
>>> -#ifndef HW_BLOCK_FDC_INTERNAL_H
>>> -#define HW_BLOCK_FDC_INTERNAL_H
>>> +#ifndef HW_BLOCK_FDC_H
>>> +#define HW_BLOCK_FDC_H
>>>
>>> #include "exec/memory.h"
>>> #include "exec/ioport.h"
>>> diff --git a/hw/block/fdc-isa.c b/hw/block/fdc-isa.c
>>> index 6387dc94fa..7058d4118f 100644
>>> --- a/hw/block/fdc-isa.c
>>> +++ b/hw/block/fdc-isa.c
>>> @@ -39,6 +39,7 @@
>>> #include "hw/qdev-properties-system.h"
>>> #include "migration/vmstate.h"
>>> #include "hw/block/block.h"
>>> +#include "hw/block/fdc.h"
>>> #include "sysemu/block-backend.h"
>>> #include "sysemu/blockdev.h"
>>> #include "sysemu/sysemu.h"
>>> @@ -47,7 +48,6 @@
>>> #include "qemu/module.h"
>>> #include "trace.h"
>>> #include "qom/object.h"
>>> -#include "fdc-internal.h"
>>>
>>> OBJECT_DECLARE_SIMPLE_TYPE(FDCtrlISABus, ISA_FDC)
>>>
>>> diff --git a/hw/block/fdc-sysbus.c b/hw/block/fdc-sysbus.c
>>> index f18f0d19b0..cff21c02b3 100644
>>> --- a/hw/block/fdc-sysbus.c
>>> +++ b/hw/block/fdc-sysbus.c
>>> @@ -28,8 +28,8 @@
>>> #include "qom/object.h"
>>> #include "hw/sysbus.h"
>>> #include "hw/block/fdc-isa.h"
>>> +#include "hw/block/fdc.h"
>>> #include "migration/vmstate.h"
>>> -#include "fdc-internal.h"
>>> #include "trace.h"
>>>
>>> #define TYPE_SYSBUS_FDC "base-sysbus-fdc"
>>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>>> index 2bd6d925b5..0e2fa527f9 100644
>>> --- a/hw/block/fdc.c
>>> +++ b/hw/block/fdc.c
>>> @@ -39,6 +39,7 @@
>>> #include "hw/qdev-properties-system.h"
>>> #include "migration/vmstate.h"
>>> #include "hw/block/block.h"
>>> +#include "hw/block/fdc.h"
>>> #include "sysemu/block-backend.h"
>>> #include "sysemu/blockdev.h"
>>> #include "sysemu/sysemu.h"
>>> @@ -47,7 +48,6 @@
>>> #include "qemu/module.h"
>>> #include "trace.h"
>>> #include "qom/object.h"
>>> -#include "fdc-internal.h"
>>>
>>> /********************************************************/
>>> /* debug Floppy devices */
>>>
>
>

Re: [PATCH 04/12] hw/block/fdc: Expose internal header

[Bernhard Beschow]

Am 18. Dezember 2023 10:54:56 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Sun, 17 Dec 2023, Bernhard Beschow wrote:
>> Am 17. Dezember 2023 15:47:33 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>>> On Sun, 17 Dec 2023, Bernhard Beschow wrote:
>>>> Exposing the internal header allows for exposing struct FDCtrlISABus which is
>>>> encuraged by qdev guidelines.
>>> 
>>> Hopefully the guidelines don't encourage this as object orientation indeed encourages object encapsulation so only the object itseld should poke its internals and other objects should use methods the change object state. In QOM some object states were exposed in public headers to allow embedding those objects in other objects becuase C needs the struct size to allow that. This was to simplify memory management so the embedded objects don't need to be tracked and freed but would be created and freed with the other object embedding it but this does not mean the other object should poke into these object or that this is a general guideline to expose internal object state. I'd say the exposed objects are an exception instead of recommended guideline and only allowed for objects that need to be embeded in others but generally object encapsulation would be better to preserve where possible. This patch exposes objects so others can poke into them which would make those other objects depe
>ndent on the implementation of these objects making these harder to chnage in the future so a better way may be to add methods to fdc and serial to allow changing their base address and map/unmap their ports and keep their internals unexposed.
>> 
>> Each ISADevice sub class would need concenience methods as well as each state class. This series touches three of each: fdc, parallel, serial. And each of those need two convenience methods: set_enabled() and set_address(). This would add another 12 functions on top of the current ones.
>
>If all ISA devices need this then these should really be methods of ISADevice but since that's just an empty wrapper over devices each of which handles its own ports, the ISADevice does not know about those and since each device may have different ports and not all of them uses portio lists for this, moving port handling to ISADevice might be too big refactoring to do for this. Keeping these functions with the superio component devices so their implementation is kept private still worth it in my opinion so even if that adds 2 functions to superio component devices (which is not all ISA devices just a limited set) seems to be a better approach to me than breaking encapsulation of objects. These are simple access methods for internal object state which are common in object otiented programming.
>
>> Then ISASuperIODevice would require at least 6 more such methods (not counting the unneeded ones for IDE which might be desirable for consistency). So in the end we'd have at least 18 more methods. Is this really worth it?
>
>We may do without these if we say superio is just a container of components so don't add forwarding methods but we can call the accessor methods of component objects from vt82c686.c. That's still better than reaching into object internals from foreign objects.

Version 2 is out which should address all of your comments.

Best regards,
Bernhard

>
>Regards,
>BALATON Zoltan
>
>> I didn't feel very comfortable going this route, so ended up with the current solution poking the states directly. I'm open to different approaches including the one above but I'd really like to know the opinion of the maintainers, too.
>> 
>> Best regards,
>> Bernhard
>> 
>>> 
>>> Regards,
>>> BALATON Zoltan
>>> 
>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>> ---
>>>> MAINTAINERS                                       | 2 +-
>>>> hw/block/fdc-internal.h => include/hw/block/fdc.h | 4 ++--
>>>> hw/block/fdc-isa.c                                | 2 +-
>>>> hw/block/fdc-sysbus.c                             | 2 +-
>>>> hw/block/fdc.c                                    | 2 +-
>>>> 5 files changed, 6 insertions(+), 6 deletions(-)
>>>> rename hw/block/fdc-internal.h => include/hw/block/fdc.h (98%)
>>>> 
>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>> index b4718fcf59..939f518701 100644
>>>> --- a/MAINTAINERS
>>>> +++ b/MAINTAINERS
>>>> @@ -1945,9 +1945,9 @@ M: John Snow <jsnow@redhat.com>
>>>> L: qemu-block@nongnu.org
>>>> S: Odd Fixes
>>>> F: hw/block/fdc.c
>>>> -F: hw/block/fdc-internal.h
>>>> F: hw/block/fdc-isa.c
>>>> F: hw/block/fdc-sysbus.c
>>>> +F: include/hw/block/fdc.h
>>>> F: include/hw/block/fdc-isa.h
>>>> F: tests/qtest/fdc-test.c
>>>> T: git https://gitlab.com/jsnow/qemu.git ide
>>>> diff --git a/hw/block/fdc-internal.h b/include/hw/block/fdc.h
>>>> similarity index 98%
>>>> rename from hw/block/fdc-internal.h
>>>> rename to include/hw/block/fdc.h
>>>> index 1728231a26..acca7e0d0e 100644
>>>> --- a/hw/block/fdc-internal.h
>>>> +++ b/include/hw/block/fdc.h
>>>> @@ -22,8 +22,8 @@
>>>>  * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>>>>  * THE SOFTWARE.
>>>>  */
>>>> -#ifndef HW_BLOCK_FDC_INTERNAL_H
>>>> -#define HW_BLOCK_FDC_INTERNAL_H
>>>> +#ifndef HW_BLOCK_FDC_H
>>>> +#define HW_BLOCK_FDC_H
>>>> 
>>>> #include "exec/memory.h"
>>>> #include "exec/ioport.h"
>>>> diff --git a/hw/block/fdc-isa.c b/hw/block/fdc-isa.c
>>>> index 6387dc94fa..7058d4118f 100644
>>>> --- a/hw/block/fdc-isa.c
>>>> +++ b/hw/block/fdc-isa.c
>>>> @@ -39,6 +39,7 @@
>>>> #include "hw/qdev-properties-system.h"
>>>> #include "migration/vmstate.h"
>>>> #include "hw/block/block.h"
>>>> +#include "hw/block/fdc.h"
>>>> #include "sysemu/block-backend.h"
>>>> #include "sysemu/blockdev.h"
>>>> #include "sysemu/sysemu.h"
>>>> @@ -47,7 +48,6 @@
>>>> #include "qemu/module.h"
>>>> #include "trace.h"
>>>> #include "qom/object.h"
>>>> -#include "fdc-internal.h"
>>>> 
>>>> OBJECT_DECLARE_SIMPLE_TYPE(FDCtrlISABus, ISA_FDC)
>>>> 
>>>> diff --git a/hw/block/fdc-sysbus.c b/hw/block/fdc-sysbus.c
>>>> index f18f0d19b0..cff21c02b3 100644
>>>> --- a/hw/block/fdc-sysbus.c
>>>> +++ b/hw/block/fdc-sysbus.c
>>>> @@ -28,8 +28,8 @@
>>>> #include "qom/object.h"
>>>> #include "hw/sysbus.h"
>>>> #include "hw/block/fdc-isa.h"
>>>> +#include "hw/block/fdc.h"
>>>> #include "migration/vmstate.h"
>>>> -#include "fdc-internal.h"
>>>> #include "trace.h"
>>>> 
>>>> #define TYPE_SYSBUS_FDC "base-sysbus-fdc"
>>>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>>>> index 2bd6d925b5..0e2fa527f9 100644
>>>> --- a/hw/block/fdc.c
>>>> +++ b/hw/block/fdc.c
>>>> @@ -39,6 +39,7 @@
>>>> #include "hw/qdev-properties-system.h"
>>>> #include "migration/vmstate.h"
>>>> #include "hw/block/block.h"
>>>> +#include "hw/block/fdc.h"
>>>> #include "sysemu/block-backend.h"
>>>> #include "sysemu/blockdev.h"
>>>> #include "sysemu/sysemu.h"
>>>> @@ -47,7 +48,6 @@
>>>> #include "qemu/module.h"
>>>> #include "trace.h"
>>>> #include "qom/object.h"
>>>> -#include "fdc-internal.h"
>>>> 
>>>> /********************************************************/
>>>> /* debug Floppy devices */
>>>> 
>> 
>>

Re: [PATCH 04/12] hw/block/fdc: Expose internal header

[BALATON Zoltan]

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

On Mon, 18 Dec 2023, Bernhard Beschow wrote:
> Am 18. Dezember 2023 10:54:56 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>> On Sun, 17 Dec 2023, Bernhard Beschow wrote:
>>> Am 17. Dezember 2023 15:47:33 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>>>> On Sun, 17 Dec 2023, Bernhard Beschow wrote:
>>>>> Exposing the internal header allows for exposing struct FDCtrlISABus which is
>>>>> encuraged by qdev guidelines.
>>>>
>>>> Hopefully the guidelines don't encourage this as object orientation indeed encourages object encapsulation so only the object itseld should poke its internals and other objects should use methods the change object state. In QOM some object states were exposed in public headers to allow embedding those objects in other objects becuase C needs the struct size to allow that. This was to simplify memory management so the embedded objects don't need to be tracked and freed but would be created and freed with the other object embedding it but this does not mean the other object should poke into these object or that this is a general guideline to expose internal object state. I'd say the exposed objects are an exception instead of recommended guideline and only allowed for objects that need to be embeded in others but generally object encapsulation would be better to preserve where possible. This patch exposes objects so others can poke into them which would make those other objects de
 pe
>> ndent on the implementation of these objects making these harder to chnage in the future so a better way may be to add methods to fdc and serial to allow changing their base address and map/unmap their ports and keep their internals unexposed.
>>>
>>> Each ISADevice sub class would need concenience methods as well as each state class. This series touches three of each: fdc, parallel, serial. And each of those need two convenience methods: set_enabled() and set_address(). This would add another 12 functions on top of the current ones.
>>
>> If all ISA devices need this then these should really be methods of ISADevice but since that's just an empty wrapper over devices each of which handles its own ports, the ISADevice does not know about those and since each device may have different ports and not all of them uses portio lists for this, moving port handling to ISADevice might be too big refactoring to do for this. Keeping these functions with the superio component devices so their implementation is kept private still worth it in my opinion so even if that adds 2 functions to superio component devices (which is not all ISA devices just a limited set) seems to be a better approach to me than breaking encapsulation of objects. These are simple access methods for internal object state which are common in object otiented programming.
>>
>>> Then ISASuperIODevice would require at least 6 more such methods (not counting the unneeded ones for IDE which might be desirable for consistency). So in the end we'd have at least 18 more methods. Is this really worth it?
>>
>> We may do without these if we say superio is just a container of components so don't add forwarding methods but we can call the accessor methods of component objects from vt82c686.c. That's still better than reaching into object internals from foreign objects.
>
> Version 2 is out which should address all of your comments.

I think this version looks better. I only have time for somw preliminary 
comments after a quick look now, I'll plan to give it more testing during 
Xmas holiday.

Regards,
BALATON Zoltan

[PATCH 05/12] hw/block/fdc: Move constant #define to where it is imposed

[Bernhard Beschow]

The MAX_FD is a limitation of struct FDCtrl which is defined in fdc.h. Now that
this header is exposed the definition can be moved there.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/block/fdc-isa.h | 3 ---
 include/hw/block/fdc.h     | 3 ++-
 hw/block/fdc.c             | 1 -
 hw/i386/pc.c               | 1 +
 hw/isa/isa-superio.c       | 1 +
 hw/mips/jazz.c             | 1 +
 hw/sparc64/sun4u.c         | 1 +
 7 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/include/hw/block/fdc-isa.h b/include/hw/block/fdc-isa.h
index 95807fdc65..42abd001dd 100644
--- a/include/hw/block/fdc-isa.h
+++ b/include/hw/block/fdc-isa.h
@@ -4,9 +4,6 @@
 #include "exec/hwaddr.h"
 #include "qapi/qapi-types-block.h"
 
-/* fdc.c */
-#define MAX_FD 2
-
 #define TYPE_ISA_FDC "isa-fdc"
 
 void isa_fdc_init_drives(ISADevice *fdc, DriveInfo **fds);
diff --git a/include/hw/block/fdc.h b/include/hw/block/fdc.h
index acca7e0d0e..0484280939 100644
--- a/include/hw/block/fdc.h
+++ b/include/hw/block/fdc.h
@@ -28,9 +28,10 @@
 #include "exec/memory.h"
 #include "exec/ioport.h"
 #include "hw/block/block.h"
-#include "hw/block/fdc-isa.h"
 #include "qapi/qapi-types-block.h"
 
+#define MAX_FD 2
+
 typedef struct FDCtrl FDCtrl;
 
 /* Floppy bus emulation */
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 0e2fa527f9..7f58cf1c1f 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -28,7 +28,6 @@
  */
 
 #include "qemu/osdep.h"
-#include "hw/block/fdc-isa.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "qemu/timer.h"
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index aeecf56e72..a8051feacd 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -25,6 +25,7 @@
 #include "qemu/osdep.h"
 #include "qemu/units.h"
 #include "hw/i386/pc.h"
+#include "hw/block/fdc.h"
 #include "hw/block/fdc-isa.h"
 #include "hw/char/serial.h"
 #include "hw/char/parallel.h"
diff --git a/hw/isa/isa-superio.c b/hw/isa/isa-superio.c
index ea6cb4213f..99d2aa491b 100644
--- a/hw/isa/isa-superio.c
+++ b/hw/isa/isa-superio.c
@@ -17,6 +17,7 @@
 #include "sysemu/blockdev.h"
 #include "chardev/char.h"
 #include "hw/char/parallel.h"
+#include "hw/block/fdc.h"
 #include "hw/block/fdc-isa.h"
 #include "hw/isa/superio.h"
 #include "hw/qdev-properties.h"
diff --git a/hw/mips/jazz.c b/hw/mips/jazz.c
index bc74d1fd96..646b5eb3f1 100644
--- a/hw/mips/jazz.c
+++ b/hw/mips/jazz.c
@@ -31,6 +31,7 @@
 #include "hw/char/serial.h"
 #include "hw/char/parallel.h"
 #include "hw/isa/isa.h"
+#include "hw/block/fdc.h"
 #include "hw/block/fdc-isa.h"
 #include "sysemu/sysemu.h"
 #include "hw/boards.h"
diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index 9a88772f6f..0d7b539ace 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -39,6 +39,7 @@
 #include "hw/rtc/m48t59.h"
 #include "migration/vmstate.h"
 #include "hw/input/i8042.h"
+#include "hw/block/fdc.h"
 #include "hw/block/fdc-isa.h"
 #include "net/net.h"
 #include "qemu/timer.h"
-- 
2.43.0

[PATCH 06/12] hw/block/fdc-isa: Expose struct FDCtrlISABus

[Bernhard Beschow]

Exposing device structs in headers is encuraged by qdev guidelines.

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

diff --git a/include/hw/block/fdc-isa.h b/include/hw/block/fdc-isa.h
index 42abd001dd..965c749c96 100644
--- a/include/hw/block/fdc-isa.h
+++ b/include/hw/block/fdc-isa.h
@@ -3,9 +3,24 @@
 
 #include "exec/hwaddr.h"
 #include "qapi/qapi-types-block.h"
+#include "hw/block/fdc.h"
+#include "hw/isa/isa.h"
 
 #define TYPE_ISA_FDC "isa-fdc"
 
+OBJECT_DECLARE_SIMPLE_TYPE(FDCtrlISABus, ISA_FDC)
+
+struct FDCtrlISABus {
+    ISADevice parent_obj;
+
+    uint32_t iobase;
+    uint32_t irq;
+    uint32_t dma;
+    FDCtrl state;
+    int32_t bootindexA;
+    int32_t bootindexB;
+};
+
 void isa_fdc_init_drives(ISADevice *fdc, DriveInfo **fds);
 void fdctrl_init_sysbus(qemu_irq irq, hwaddr mmio_base, DriveInfo **fds);
 void sun4m_fdctrl_init(qemu_irq irq, hwaddr io_base,
diff --git a/hw/block/fdc-isa.c b/hw/block/fdc-isa.c
index 7058d4118f..090dc03381 100644
--- a/hw/block/fdc-isa.c
+++ b/hw/block/fdc-isa.c
@@ -34,12 +34,10 @@
 #include "qemu/timer.h"
 #include "hw/acpi/acpi_aml_interface.h"
 #include "hw/irq.h"
-#include "hw/isa/isa.h"
 #include "hw/qdev-properties.h"
 #include "hw/qdev-properties-system.h"
 #include "migration/vmstate.h"
 #include "hw/block/block.h"
-#include "hw/block/fdc.h"
 #include "sysemu/block-backend.h"
 #include "sysemu/blockdev.h"
 #include "sysemu/sysemu.h"
@@ -49,21 +47,6 @@
 #include "trace.h"
 #include "qom/object.h"
 
-OBJECT_DECLARE_SIMPLE_TYPE(FDCtrlISABus, ISA_FDC)
-
-struct FDCtrlISABus {
-    /*< private >*/
-    ISADevice parent_obj;
-    /*< public >*/
-
-    uint32_t iobase;
-    uint32_t irq;
-    uint32_t dma;
-    struct FDCtrl state;
-    int32_t bootindexA;
-    int32_t bootindexB;
-};
-
 static void fdctrl_external_reset_isa(DeviceState *d)
 {
     FDCtrlISABus *isa = ISA_FDC(d);
-- 
2.43.0

[PATCH 07/12] MAINTAINERS: Add include/hw/char/serial*.h to the "PC Chipset" section

[Bernhard Beschow]

The source files are already in this section. Add the headers, too.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 939f518701..69135a45b4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1827,6 +1827,7 @@ F: hw/watchdog/wdt_ib700.c
 F: hw/watchdog/wdt_i6300esb.c
 F: include/hw/display/vga.h
 F: include/hw/char/parallel*.h
+F: include/hw/char/serial*.h
 F: include/hw/dma/i8257.h
 F: include/hw/i2c/pm_smbus.h
 F: include/hw/input/i8042.h
-- 
2.43.0

[PATCH 08/12] hw/char/serial-isa: Export struct ISASerialState

[Bernhard Beschow]

Exposing device structs in headers is encuraged by qdev guidelines.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/char/serial-isa.h | 50 ++++++++++++++++++++++++++++++++++++
 include/hw/char/serial.h     |  7 -----
 hw/char/serial-isa.c         | 14 +---------
 hw/i386/microvm-dt.c         |  2 +-
 hw/i386/microvm.c            |  2 +-
 hw/i386/pc.c                 |  2 +-
 hw/isa/isa-superio.c         |  2 +-
 hw/ppc/pnv.c                 |  2 +-
 hw/sparc64/sun4u.c           |  1 +
 9 files changed, 57 insertions(+), 25 deletions(-)
 create mode 100644 include/hw/char/serial-isa.h

diff --git a/include/hw/char/serial-isa.h b/include/hw/char/serial-isa.h
new file mode 100644
index 0000000000..4bd01ef45b
--- /dev/null
+++ b/include/hw/char/serial-isa.h
@@ -0,0 +1,50 @@
+/*
+ * QEMU 16550A UART emulation
+ *
+ * Copyright (c) 2003-2004 Fabrice Bellard
+ * Copyright (c) 2008 Citrix Systems, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#ifndef HW_SERIAL_ISA_H
+#define HW_SERIAL_ISA_H
+
+#include "serial.h"
+
+#include "hw/isa/isa.h"
+#include "qom/object.h"
+
+#define TYPE_ISA_SERIAL "isa-serial"
+OBJECT_DECLARE_SIMPLE_TYPE(ISASerialState, ISA_SERIAL)
+
+struct ISASerialState {
+    ISADevice parent_obj;
+
+    uint32_t index;
+    uint32_t iobase;
+    uint32_t isairq;
+    SerialState state;
+};
+
+#define MAX_ISA_SERIAL_PORTS 4
+
+void serial_hds_isa_init(ISABus *bus, int from, int to);
+
+#endif /* HW_SERIAL_ISA_H */
diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
index 8ba7eca3d6..fca32a532b 100644
--- a/include/hw/char/serial.h
+++ b/include/hw/char/serial.h
@@ -106,11 +106,4 @@ SerialMM *serial_mm_init(MemoryRegion *address_space,
                          qemu_irq irq, int baudbase,
                          Chardev *chr, enum device_endian end);
 
-/* serial-isa.c */
-
-#define MAX_ISA_SERIAL_PORTS 4
-
-#define TYPE_ISA_SERIAL "isa-serial"
-void serial_hds_isa_init(ISABus *bus, int from, int to);
-
 #endif
diff --git a/hw/char/serial-isa.c b/hw/char/serial-isa.c
index 141a6cb168..315982efb5 100644
--- a/hw/char/serial-isa.c
+++ b/hw/char/serial-isa.c
@@ -28,23 +28,11 @@
 #include "qemu/module.h"
 #include "sysemu/sysemu.h"
 #include "hw/acpi/acpi_aml_interface.h"
-#include "hw/char/serial.h"
-#include "hw/isa/isa.h"
+#include "hw/char/serial-isa.h"
 #include "hw/qdev-properties.h"
 #include "migration/vmstate.h"
 #include "qom/object.h"
 
-OBJECT_DECLARE_SIMPLE_TYPE(ISASerialState, ISA_SERIAL)
-
-struct ISASerialState {
-    ISADevice parent_obj;
-
-    uint32_t index;
-    uint32_t iobase;
-    uint32_t isairq;
-    SerialState state;
-};
-
 static const int isa_serial_io[MAX_ISA_SERIAL_PORTS] = {
     0x3f8, 0x2f8, 0x3e8, 0x2e8
 };
diff --git a/hw/i386/microvm-dt.c b/hw/i386/microvm-dt.c
index b3049e4f9f..fc5db6ed7f 100644
--- a/hw/i386/microvm-dt.c
+++ b/hw/i386/microvm-dt.c
@@ -34,7 +34,7 @@
 #include "qemu/cutils.h"
 #include "qapi/error.h"
 #include "sysemu/device_tree.h"
-#include "hw/char/serial.h"
+#include "hw/char/serial-isa.h"
 #include "hw/i386/fw_cfg.h"
 #include "hw/rtc/mc146818rtc.h"
 #include "hw/sysbus.h"
diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c
index ca55aecc3b..a39d382367 100644
--- a/hw/i386/microvm.c
+++ b/hw/i386/microvm.c
@@ -39,7 +39,7 @@
 #include "hw/intc/i8259.h"
 #include "hw/timer/i8254.h"
 #include "hw/rtc/mc146818rtc.h"
-#include "hw/char/serial.h"
+#include "hw/char/serial-isa.h"
 #include "hw/display/ramfb.h"
 #include "hw/i386/topology.h"
 #include "hw/i386/e820_memory_layout.h"
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index a8051feacd..1ddfcefbe4 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -27,7 +27,7 @@
 #include "hw/i386/pc.h"
 #include "hw/block/fdc.h"
 #include "hw/block/fdc-isa.h"
-#include "hw/char/serial.h"
+#include "hw/char/serial-isa.h"
 #include "hw/char/parallel.h"
 #include "hw/hyperv/hv-balloon.h"
 #include "hw/i386/fw_cfg.h"
diff --git a/hw/isa/isa-superio.c b/hw/isa/isa-superio.c
index 99d2aa491b..74162f26be 100644
--- a/hw/isa/isa-superio.c
+++ b/hw/isa/isa-superio.c
@@ -23,7 +23,7 @@
 #include "hw/qdev-properties.h"
 #include "hw/input/i8042.h"
 #include "hw/char/parallel-isa.h"
-#include "hw/char/serial.h"
+#include "hw/char/serial-isa.h"
 #include "trace.h"
 
 static void isa_superio_realize(DeviceState *dev, Error **errp)
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 0297871bdd..78a33832d3 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -54,7 +54,7 @@
 #include "hw/ppc/pnv_pnor.h"
 
 #include "hw/isa/isa.h"
-#include "hw/char/serial.h"
+#include "hw/char/serial-isa.h"
 #include "hw/rtc/mc146818rtc.h"
 
 #include <libfdt.h>
diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index 0d7b539ace..2f0aebfb29 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -35,6 +35,7 @@
 #include "hw/qdev-properties.h"
 #include "hw/pci-host/sabre.h"
 #include "hw/char/serial.h"
+#include "hw/char/serial-isa.h"
 #include "hw/char/parallel-isa.h"
 #include "hw/rtc/m48t59.h"
 #include "migration/vmstate.h"
-- 
2.43.0

[PATCH 09/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 10/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 11/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 12/12] hw/isa/vt82c686: Implement relocation of SuperI/O functions

[Bernhard Beschow]

The VIA south bridges are able to relocate and 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 of
the SuperI/O functions.

Note that the reset I/O region of VT8231's serial port changes from
0x2f8/enabled to 0x3f8/disabled. The ROM of the Pegasos II machine can handle it
since it enables and relocates the I/O region accordingly.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/isa/vt82c686.c | 140 ++++++++++++++++++++++++++++++++++++----------
 hw/ppc/pegasos2.c |  15 +++++
 2 files changed, 124 insertions(+), 31 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 9c2333a277..8fbc016755 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-isa.h"
+#include "hw/char/parallel-isa.h"
+#include "hw/char/serial-isa.h"
 #include "hw/pci/pci.h"
 #include "hw/qdev-properties.h"
 #include "hw/ide/pci.h"
@@ -343,6 +346,46 @@ static const TypeInfo via_superio_info = {
 
 #define TYPE_VT82C686B_SUPERIO "vt82c686b-superio"
 
+static void vt82c686b_superio_update(ViaSuperIOState *s)
+{
+    FDCtrlISABus *fd = ISA_FDC(s->superio.floppy);
+    ISASerialState *ss1 = ISA_SERIAL(s->superio.serial[0]);
+    ISASerialState *ss2 = ISA_SERIAL(s->superio.serial[1]);
+    ISAParallelState *ps = ISA_PARALLEL(s->superio.parallel[0]);
+
+    portio_list_set_enabled(&ps->state.portio_list, (s->regs[0xe2] & 0x3) != 3);
+    memory_region_set_enabled(&ss1->state.io, s->regs[0xe2] & BIT(2));
+    memory_region_set_enabled(&ss2->state.io, s->regs[0xe2] & BIT(3));
+    portio_list_set_enabled(&fd->state.portio_list, s->regs[0xe2] & BIT(4));
+
+    fd->iobase = (s->regs[0xe3] & 0xfc) << 2;
+    portio_list_set_address(&fd->state.portio_list, fd->iobase);
+
+    ps->iobase = s->regs[0xe6] << 2;
+    portio_list_set_address(&ps->state.portio_list, ps->iobase);
+
+    ss1->iobase = (s->regs[0xe7] & 0xfe) << 2;
+    memory_region_set_address(&ss1->state.io, ss1->iobase);
+
+    ss2->iobase = (s->regs[0xe8] & 0xfe) << 2;
+    memory_region_set_address(&ss2->state.io, ss2->iobase);
+}
+
+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 +411,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 +440,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 +475,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 +490,41 @@ static const TypeInfo vt82c686b_superio_info = {
 
 #define TYPE_VT8231_SUPERIO "vt8231-superio"
 
+static void vt8231_superio_update(ViaSuperIOState *s)
+{
+    FDCtrlISABus *fd = ISA_FDC(s->superio.floppy);
+    ISASerialState *ss = ISA_SERIAL(s->superio.serial[0]);
+    ISAParallelState *ps = ISA_PARALLEL(s->superio.parallel[0]);
+
+    portio_list_set_enabled(&ps->state.portio_list, (s->regs[0xf2] & 0x3) != 3);
+    memory_region_set_enabled(&ss->state.io, s->regs[0xf2] & BIT(2));
+    portio_list_set_enabled(&fd->state.portio_list, s->regs[0xf2] & BIT(4));
+
+    ss->iobase = (s->regs[0xf4] & 0xfe) << 2;
+    memory_region_set_address(&ss->state.io, ss->iobase);
+
+    ps->iobase = s->regs[0xf6] << 2;
+    portio_list_set_address(&ps->state.portio_list, ps->iobase);
+
+    fd->iobase = (s->regs[0xf7] & 0xfc) << 2;
+    portio_list_set_address(&fd->state.portio_list, fd->iobase);
+}
+
+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 +547,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 +581,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 +597,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 +604,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 = {
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

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

[BALATON Zoltan]

On Sun, 17 Dec 2023, Bernhard Beschow wrote:
> The VIA south bridges are able to relocate and 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 of
> the SuperI/O functions.
>
> Note that the reset I/O region of VT8231's serial port changes from
> 0x2f8/enabled to 0x3f8/disabled. The ROM of the Pegasos II machine can handle it
> since it enables and relocates the I/O region accordingly.

"... but we need to do that when running without firmware which this patch 
does." or something like that is missing here to complete the sentence. I 
think this part changing pegasos2.c could be split off in its own patch 
coming before this one. Poking those registers before they are implemented 
is harmless (the ROM does that already) but would make two simpler patches 
instead of one doing two things.

This is a welcome change but since vt82c686 uses isa-superio I wonder if 
it there could be a way to add functions to isa-superio.c to set these 
base addresses and enable/disable deivces in runtime instead of poking the 
internals od superio in vt82c686.c? That looks to me a more object 
oriented approach. Or going further with that maybe the fdc and serial 
device objects should have methods to set their base that then either 
superio or vt82c686 could then use without peeking them or exposing the 
device sturctures.

Regards,
BALATON Zoltan

> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> hw/isa/vt82c686.c | 140 ++++++++++++++++++++++++++++++++++++----------
> hw/ppc/pegasos2.c |  15 +++++
> 2 files changed, 124 insertions(+), 31 deletions(-)
>
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index 9c2333a277..8fbc016755 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-isa.h"
> +#include "hw/char/parallel-isa.h"
> +#include "hw/char/serial-isa.h"
> #include "hw/pci/pci.h"
> #include "hw/qdev-properties.h"
> #include "hw/ide/pci.h"
> @@ -343,6 +346,46 @@ static const TypeInfo via_superio_info = {
>
> #define TYPE_VT82C686B_SUPERIO "vt82c686b-superio"
>
> +static void vt82c686b_superio_update(ViaSuperIOState *s)
> +{
> +    FDCtrlISABus *fd = ISA_FDC(s->superio.floppy);
> +    ISASerialState *ss1 = ISA_SERIAL(s->superio.serial[0]);
> +    ISASerialState *ss2 = ISA_SERIAL(s->superio.serial[1]);
> +    ISAParallelState *ps = ISA_PARALLEL(s->superio.parallel[0]);
> +
> +    portio_list_set_enabled(&ps->state.portio_list, (s->regs[0xe2] & 0x3) != 3);
> +    memory_region_set_enabled(&ss1->state.io, s->regs[0xe2] & BIT(2));
> +    memory_region_set_enabled(&ss2->state.io, s->regs[0xe2] & BIT(3));
> +    portio_list_set_enabled(&fd->state.portio_list, s->regs[0xe2] & BIT(4));
> +
> +    fd->iobase = (s->regs[0xe3] & 0xfc) << 2;
> +    portio_list_set_address(&fd->state.portio_list, fd->iobase);
> +
> +    ps->iobase = s->regs[0xe6] << 2;
> +    portio_list_set_address(&ps->state.portio_list, ps->iobase);
> +
> +    ss1->iobase = (s->regs[0xe7] & 0xfe) << 2;
> +    memory_region_set_address(&ss1->state.io, ss1->iobase);
> +
> +    ss2->iobase = (s->regs[0xe8] & 0xfe) << 2;
> +    memory_region_set_address(&ss2->state.io, ss2->iobase);
> +}
> +
> +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 +411,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 +440,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 +475,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 +490,41 @@ static const TypeInfo vt82c686b_superio_info = {
>
> #define TYPE_VT8231_SUPERIO "vt8231-superio"
>
> +static void vt8231_superio_update(ViaSuperIOState *s)
> +{
> +    FDCtrlISABus *fd = ISA_FDC(s->superio.floppy);
> +    ISASerialState *ss = ISA_SERIAL(s->superio.serial[0]);
> +    ISAParallelState *ps = ISA_PARALLEL(s->superio.parallel[0]);
> +
> +    portio_list_set_enabled(&ps->state.portio_list, (s->regs[0xf2] & 0x3) != 3);
> +    memory_region_set_enabled(&ss->state.io, s->regs[0xf2] & BIT(2));
> +    portio_list_set_enabled(&fd->state.portio_list, s->regs[0xf2] & BIT(4));
> +
> +    ss->iobase = (s->regs[0xf4] & 0xfe) << 2;
> +    memory_region_set_address(&ss->state.io, ss->iobase);
> +
> +    ps->iobase = s->regs[0xf6] << 2;
> +    portio_list_set_address(&ps->state.portio_list, ps->iobase);
> +
> +    fd->iobase = (s->regs[0xf7] & 0xfc) << 2;
> +    portio_list_set_address(&fd->state.portio_list, fd->iobase);
> +}
> +
> +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 +547,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 +581,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 +597,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 +604,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 = {
> 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 12/12] hw/isa/vt82c686: Implement relocation of SuperI/O functions

[Bernhard Beschow]

Am 17. Dezember 2023 15:40:58 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Sun, 17 Dec 2023, Bernhard Beschow wrote:
>> The VIA south bridges are able to relocate and 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 of
>> the SuperI/O functions.
>> 
>> Note that the reset I/O region of VT8231's serial port changes from
>> 0x2f8/enabled to 0x3f8/disabled. The ROM of the Pegasos II machine can handle it
>> since it enables and relocates the I/O region accordingly.
>
>"... but we need to do that when running without firmware which this patch does." or something like that is missing here to complete the sentence.

I'll mention this in the next iteration.

> I think this part changing pegasos2.c could be split off in its own patch coming before this one. Poking those registers before they are implemented is harmless (the ROM does that already) but would make two simpler patches instead of one doing two things.

Will do. I considered this but went with the combined approach for compactness.

>
>This is a welcome change but since vt82c686 uses isa-superio I wonder if it there could be a way to add functions to isa-superio.c to set these base addresses and enable/disable deivces in runtime instead of poking the internals od superio in vt82c686.c? That looks to me a more object oriented approach. Or going further with that maybe the fdc and serial device objects should have methods to set their base that then either superio or vt82c686 could then use without peeking them or exposing the device sturctures.

Let's focus on this topic in the other thread.

Best regards,
Bernhard

>
>Regards,
>BALATON Zoltan
>
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>> hw/isa/vt82c686.c | 140 ++++++++++++++++++++++++++++++++++++----------
>> hw/ppc/pegasos2.c |  15 +++++
>> 2 files changed, 124 insertions(+), 31 deletions(-)
>> 
>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>> index 9c2333a277..8fbc016755 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-isa.h"
>> +#include "hw/char/parallel-isa.h"
>> +#include "hw/char/serial-isa.h"
>> #include "hw/pci/pci.h"
>> #include "hw/qdev-properties.h"
>> #include "hw/ide/pci.h"
>> @@ -343,6 +346,46 @@ static const TypeInfo via_superio_info = {
>> 
>> #define TYPE_VT82C686B_SUPERIO "vt82c686b-superio"
>> 
>> +static void vt82c686b_superio_update(ViaSuperIOState *s)
>> +{
>> +    FDCtrlISABus *fd = ISA_FDC(s->superio.floppy);
>> +    ISASerialState *ss1 = ISA_SERIAL(s->superio.serial[0]);
>> +    ISASerialState *ss2 = ISA_SERIAL(s->superio.serial[1]);
>> +    ISAParallelState *ps = ISA_PARALLEL(s->superio.parallel[0]);
>> +
>> +    portio_list_set_enabled(&ps->state.portio_list, (s->regs[0xe2] & 0x3) != 3);
>> +    memory_region_set_enabled(&ss1->state.io, s->regs[0xe2] & BIT(2));
>> +    memory_region_set_enabled(&ss2->state.io, s->regs[0xe2] & BIT(3));
>> +    portio_list_set_enabled(&fd->state.portio_list, s->regs[0xe2] & BIT(4));
>> +
>> +    fd->iobase = (s->regs[0xe3] & 0xfc) << 2;
>> +    portio_list_set_address(&fd->state.portio_list, fd->iobase);
>> +
>> +    ps->iobase = s->regs[0xe6] << 2;
>> +    portio_list_set_address(&ps->state.portio_list, ps->iobase);
>> +
>> +    ss1->iobase = (s->regs[0xe7] & 0xfe) << 2;
>> +    memory_region_set_address(&ss1->state.io, ss1->iobase);
>> +
>> +    ss2->iobase = (s->regs[0xe8] & 0xfe) << 2;
>> +    memory_region_set_address(&ss2->state.io, ss2->iobase);
>> +}
>> +
>> +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 +411,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 +440,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 +475,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 +490,41 @@ static const TypeInfo vt82c686b_superio_info = {
>> 
>> #define TYPE_VT8231_SUPERIO "vt8231-superio"
>> 
>> +static void vt8231_superio_update(ViaSuperIOState *s)
>> +{
>> +    FDCtrlISABus *fd = ISA_FDC(s->superio.floppy);
>> +    ISASerialState *ss = ISA_SERIAL(s->superio.serial[0]);
>> +    ISAParallelState *ps = ISA_PARALLEL(s->superio.parallel[0]);
>> +
>> +    portio_list_set_enabled(&ps->state.portio_list, (s->regs[0xf2] & 0x3) != 3);
>> +    memory_region_set_enabled(&ss->state.io, s->regs[0xf2] & BIT(2));
>> +    portio_list_set_enabled(&fd->state.portio_list, s->regs[0xf2] & BIT(4));
>> +
>> +    ss->iobase = (s->regs[0xf4] & 0xfe) << 2;
>> +    memory_region_set_address(&ss->state.io, ss->iobase);
>> +
>> +    ps->iobase = s->regs[0xf6] << 2;
>> +    portio_list_set_address(&ps->state.portio_list, ps->iobase);
>> +
>> +    fd->iobase = (s->regs[0xf7] & 0xfc) << 2;
>> +    portio_list_set_address(&fd->state.portio_list, fd->iobase);
>> +}
>> +
>> +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 +547,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 +581,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 +597,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 +604,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 = {
>> 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) |
>>