[PATCH 0/5] Implement port 92 in south bridges

[PATCH 0/5] Implement port 92 in south bridges

[Bernhard Beschow]

This series attempts to make QEMU's south bridge families PIIX, ICH9, and VIA
82xx more self-contained by integrating IO port 92 like the originals do.

In QEMU, the IO port is currently instantiated as a dedicated device in common
PC code. While this works and even results in less code, it seems cleaner to
model the behavior of the real devices. For example, software running on the
Malta machine, which uses PIIX4, needs to take port 92 into account, even if it
doesn't use it (does it?). Moreover, the FDC37M81x used in the original Malta
machine provides a port 92 too, which can be activated. If QEMU implemented the
FDC37M81x more closely, one could check if Yamon (or any alternative boot
loader) deals correctly with these ports.

Moving port 92 into the south bridges might also help with configuration-driven
machine creation. In such a scenario it is probably desirable if machine code
had less of its own idea of which devices it creates. Moving port 92 from
machine code into a potentially user-creaeable device (where it is part of per
datasheet) seems like a good direction. Of course, machine code still wires up
port 92 and I don't have a good idea on how to make this user-configurable.
Such insights might provide some input for discussions around
configuration-driven machine creation.

This series is structured as follows: Patch 1 moves TYPE_PORT92 into the isa
directory to make it reusable by other architectures. It also adds a
configuration switch. Patch 2 integrates TYPE_PORT92 into the PC south bridges
and adapts PC code accordingly. While at it, patch 3 cleans up wiring of the
A20 line with the keyboard controller. Patch 4 simply adds TYPE_PORT92 to the
VIA south bridges which is also needed when using the VIA south bridges in the
pc machine.

Testing done:
* `qemu-system-x86_64 -M {q35,pc},i8042={true,false} ...`
  -> `info mtree` confirms port92 to be present iff i8042=true
* `make check`
* `make check-avocado`
* Start amigaone and pegasos2 machines as described in
    https://patchew.org/QEMU/20240216001019.69A524E601F@zero.eik.bme.hu/
  -> no regressions compared to master

Best regards,
Bernhard

Bernhard Beschow (5):
  hw/isa/meson.build: Sort alphabetically
  hw/i386/port92: Allow for TYPE_PORT92 to be embedded in devices
  hw/isa: Embed TYPE_PORT92 in south bridges used in PC machines
  hw/i386/pc: Inline i8042_setup_a20_line() and remove it
  hw/isa/vt82c686: Embed TYPE_PORT92

 include/hw/i386/pc.h          |  7 +------
 include/hw/input/i8042.h      |  1 -
 include/hw/isa/port92.h       | 30 ++++++++++++++++++++++++++++++
 include/hw/southbridge/ich9.h |  4 ++++
 include/hw/southbridge/piix.h |  3 +++
 hw/i386/pc.c                  | 21 ++++++++++++++-------
 hw/i386/pc_piix.c             |  9 +++++++--
 hw/i386/pc_q35.c              |  8 +++++---
 hw/input/pckbd.c              |  5 -----
 hw/isa/lpc_ich9.c             |  9 +++++++++
 hw/isa/piix.c                 |  9 +++++++++
 hw/{i386 => isa}/port92.c     | 14 +-------------
 hw/isa/vt82c686.c             |  7 +++++++
 hw/i386/Kconfig               |  1 +
 hw/i386/meson.build           |  3 +--
 hw/i386/trace-events          |  4 ----
 hw/isa/Kconfig                |  6 ++++++
 hw/isa/meson.build            |  3 ++-
 hw/isa/trace-events           |  4 ++++
 19 files changed, 104 insertions(+), 44 deletions(-)
 create mode 100644 include/hw/isa/port92.h
 rename hw/{i386 => isa}/port92.c (91%)

-- 
2.43.2

[PATCH 1/5] hw/isa/meson.build: Sort alphabetically

[Bernhard Beschow]

Fixes: fbd758008f0f "hw/isa: extract FDC37M81X to a separate file"

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/isa/meson.build | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/isa/meson.build b/hw/isa/meson.build
index f650b39507..3219282217 100644
--- a/hw/isa/meson.build
+++ b/hw/isa/meson.build
@@ -1,10 +1,10 @@
 system_ss.add(when: 'CONFIG_APM', if_true: files('apm.c'))
+system_ss.add(when: 'CONFIG_FDC37M81X', if_true: files('fdc37m81x-superio.c'))
 system_ss.add(when: 'CONFIG_I82378', if_true: files('i82378.c'))
 system_ss.add(when: 'CONFIG_ISA_BUS', if_true: files('isa-bus.c'))
 system_ss.add(when: 'CONFIG_ISA_SUPERIO', if_true: files('isa-superio.c'))
 system_ss.add(when: 'CONFIG_PC87312', if_true: files('pc87312.c'))
 system_ss.add(when: 'CONFIG_PIIX', if_true: files('piix.c'))
-system_ss.add(when: 'CONFIG_FDC37M81X', if_true: files('fdc37m81x-superio.c'))
 system_ss.add(when: 'CONFIG_SMC37C669', if_true: files('smc37c669-superio.c'))
 system_ss.add(when: 'CONFIG_VT82C686', if_true: files('vt82c686.c'))
 
-- 
2.43.2

Re: [PATCH 1/5] hw/isa/meson.build: Sort alphabetically

[Philippe Mathieu-Daudé]

On 18/2/24 14:16, Bernhard Beschow wrote:
> Fixes: fbd758008f0f "hw/isa: extract FDC37M81X to a separate file"
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/isa/meson.build | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

$ git grep -i alphab docs/devel/build-system.rst
$

Should we document this?

Re: [PATCH 1/5] hw/isa/meson.build: Sort alphabetically

[Mark Cave-Ayland]

On 18/02/2024 13:16, Bernhard Beschow wrote:

> Fixes: fbd758008f0f "hw/isa: extract FDC37M81X to a separate file"
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/isa/meson.build | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/isa/meson.build b/hw/isa/meson.build
> index f650b39507..3219282217 100644
> --- a/hw/isa/meson.build
> +++ b/hw/isa/meson.build
> @@ -1,10 +1,10 @@
>   system_ss.add(when: 'CONFIG_APM', if_true: files('apm.c'))
> +system_ss.add(when: 'CONFIG_FDC37M81X', if_true: files('fdc37m81x-superio.c'))
>   system_ss.add(when: 'CONFIG_I82378', if_true: files('i82378.c'))
>   system_ss.add(when: 'CONFIG_ISA_BUS', if_true: files('isa-bus.c'))
>   system_ss.add(when: 'CONFIG_ISA_SUPERIO', if_true: files('isa-superio.c'))
>   system_ss.add(when: 'CONFIG_PC87312', if_true: files('pc87312.c'))
>   system_ss.add(when: 'CONFIG_PIIX', if_true: files('piix.c'))
> -system_ss.add(when: 'CONFIG_FDC37M81X', if_true: files('fdc37m81x-superio.c'))
>   system_ss.add(when: 'CONFIG_SMC37C669', if_true: files('smc37c669-superio.c'))
>   system_ss.add(when: 'CONFIG_VT82C686', if_true: files('vt82c686.c'))

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

[PATCH 2/5] hw/i386/port92: Allow for TYPE_PORT92 to be embedded in devices

[Bernhard Beschow]

Port 92 is an integral part of south bridges. Allow for embedding it there.

South bridges aren't architecture-specific, so move port92.c to hw/isa which is
accessible to other architectures than x86.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/i386/pc.h      |  5 -----
 include/hw/isa/port92.h   | 30 ++++++++++++++++++++++++++++++
 hw/i386/pc.c              |  1 +
 hw/{i386 => isa}/port92.c | 14 +-------------
 hw/i386/Kconfig           |  1 +
 hw/i386/meson.build       |  3 +--
 hw/i386/trace-events      |  4 ----
 hw/isa/Kconfig            |  3 +++
 hw/isa/meson.build        |  1 +
 hw/isa/trace-events       |  4 ++++
 10 files changed, 42 insertions(+), 24 deletions(-)
 create mode 100644 include/hw/isa/port92.h
 rename hw/{i386 => isa}/port92.c (91%)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index ec0e5efcb2..b2987209b1 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -188,11 +188,6 @@ void pc_nic_init(PCMachineClass *pcmc, ISABus *isa_bus, PCIBus *pci_bus);
 
 void pc_i8259_create(ISABus *isa_bus, qemu_irq *i8259_irqs);
 
-/* port92.c */
-#define PORT92_A20_LINE "a20"
-
-#define TYPE_PORT92 "port92"
-
 /* pc_sysfw.c */
 void pc_system_flash_create(PCMachineState *pcms);
 void pc_system_flash_cleanup_unused(PCMachineState *pcms);
diff --git a/include/hw/isa/port92.h b/include/hw/isa/port92.h
new file mode 100644
index 0000000000..214783d071
--- /dev/null
+++ b/include/hw/isa/port92.h
@@ -0,0 +1,30 @@
+/*
+ * QEMU I/O port 0x92 (System Control Port A, to handle Fast Gate A20)
+ *
+ * Copyright (c) 2003-2004 Fabrice Bellard
+ *
+ * SPDX-License-Identifier: MIT
+ */
+
+#ifndef HW_PORT92_H
+#define HW_PORT92_H
+
+#include "exec/memory.h"
+#include "hw/irq.h"
+#include "hw/isa/isa.h"
+#include "qom/object.h"
+
+#define TYPE_PORT92 "port92"
+OBJECT_DECLARE_SIMPLE_TYPE(Port92State, PORT92)
+
+struct Port92State {
+    ISADevice parent_obj;
+
+    MemoryRegion io;
+    uint8_t outport;
+    qemu_irq a20_out;
+};
+
+#define PORT92_A20_LINE "a20"
+
+#endif /* HW_PORT92_H */
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 196827531a..0b11d4576e 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -32,6 +32,7 @@
 #include "hw/i386/vmport.h"
 #include "sysemu/cpus.h"
 #include "hw/ide/internal.h"
+#include "hw/isa/port92.h"
 #include "hw/timer/hpet.h"
 #include "hw/loader.h"
 #include "hw/rtc/mc146818rtc.h"
diff --git a/hw/i386/port92.c b/hw/isa/port92.c
similarity index 91%
rename from hw/i386/port92.c
rename to hw/isa/port92.c
index 1070bfbf36..06df06b088 100644
--- a/hw/i386/port92.c
+++ b/hw/isa/port92.c
@@ -9,20 +9,8 @@
 #include "qemu/osdep.h"
 #include "sysemu/runstate.h"
 #include "migration/vmstate.h"
-#include "hw/irq.h"
-#include "hw/i386/pc.h"
+#include "hw/isa/port92.h"
 #include "trace.h"
-#include "qom/object.h"
-
-OBJECT_DECLARE_SIMPLE_TYPE(Port92State, PORT92)
-
-struct Port92State {
-    ISADevice parent_obj;
-
-    MemoryRegion io;
-    uint8_t outport;
-    qemu_irq a20_out;
-};
 
 static void port92_write(void *opaque, hwaddr addr, uint64_t val,
                          unsigned size)
diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig
index a1846be6f7..ccf6de4a00 100644
--- a/hw/i386/Kconfig
+++ b/hw/i386/Kconfig
@@ -37,6 +37,7 @@ config PC
     select I8254
     select PCKBD
     select PCSPK
+    select PORT92
     select I8257
     select MC146818RTC
     # For ACPI builder:
diff --git a/hw/i386/meson.build b/hw/i386/meson.build
index b9c1ca39cb..94d558edd6 100644
--- a/hw/i386/meson.build
+++ b/hw/i386/meson.build
@@ -24,8 +24,7 @@ i386_ss.add(when: 'CONFIG_ACPI', if_true: files('acpi-common.c'))
 i386_ss.add(when: 'CONFIG_PC', if_true: files(
   'pc.c',
   'pc_sysfw.c',
-  'acpi-build.c',
-  'port92.c'))
+  'acpi-build.c'))
 i386_ss.add(when: 'CONFIG_X86_FW_OVMF', if_true: files('pc_sysfw_ovmf.c'),
                                         if_false: files('pc_sysfw_ovmf-stubs.c'))
 
diff --git a/hw/i386/trace-events b/hw/i386/trace-events
index 53c02d7ac8..b730589394 100644
--- a/hw/i386/trace-events
+++ b/hw/i386/trace-events
@@ -118,10 +118,6 @@ vmport_command(unsigned char command) "command: 0x%02x"
 x86_gsi_interrupt(int irqn, int level) "GSI interrupt #%d level:%d"
 x86_pic_interrupt(int irqn, int level) "PIC interrupt #%d level:%d"
 
-# port92.c
-port92_read(uint8_t val) "port92: read 0x%02x"
-port92_write(uint8_t val) "port92: write 0x%02x"
-
 # vmmouse.c
 vmmouse_get_status(void) ""
 vmmouse_mouse_event(int x, int y, int dz, int buttons_state) "event: x=%d y=%d dz=%d state=%d"
diff --git a/hw/isa/Kconfig b/hw/isa/Kconfig
index 73c6470805..efdf43e92c 100644
--- a/hw/isa/Kconfig
+++ b/hw/isa/Kconfig
@@ -49,6 +49,9 @@ config PIIX
     select MC146818RTC
     select USB_UHCI
 
+config PORT92
+    bool
+
 config VT82C686
     bool
     select ISA_BUS
diff --git a/hw/isa/meson.build b/hw/isa/meson.build
index 3219282217..fb7cd44984 100644
--- a/hw/isa/meson.build
+++ b/hw/isa/meson.build
@@ -5,6 +5,7 @@ system_ss.add(when: 'CONFIG_ISA_BUS', if_true: files('isa-bus.c'))
 system_ss.add(when: 'CONFIG_ISA_SUPERIO', if_true: files('isa-superio.c'))
 system_ss.add(when: 'CONFIG_PC87312', if_true: files('pc87312.c'))
 system_ss.add(when: 'CONFIG_PIIX', if_true: files('piix.c'))
+system_ss.add(when: 'CONFIG_PORT92', if_true: files('port92.c'))
 system_ss.add(when: 'CONFIG_SMC37C669', if_true: files('smc37c669-superio.c'))
 system_ss.add(when: 'CONFIG_VT82C686', if_true: files('vt82c686.c'))
 
diff --git a/hw/isa/trace-events b/hw/isa/trace-events
index 1816e8307a..bb5d9f1078 100644
--- a/hw/isa/trace-events
+++ b/hw/isa/trace-events
@@ -10,6 +10,10 @@ superio_create_ide(int id, uint16_t base, unsigned int irq) "id=%d, base 0x%03x,
 pc87312_io_read(uint32_t addr, uint32_t val) "read addr=0x%x val=0x%x"
 pc87312_io_write(uint32_t addr, uint32_t val) "write addr=0x%x val=0x%x"
 
+# port92.c
+port92_read(uint8_t val) "port92: read 0x%02x"
+port92_write(uint8_t val) "port92: write 0x%02x"
+
 # apm.c
 apm_io_read(uint8_t addr, uint8_t val) "read addr=0x%x val=0x%02x"
 apm_io_write(uint8_t addr, uint8_t val) "write addr=0x%x val=0x%02x"
-- 
2.43.2

Re: [PATCH 2/5] hw/i386/port92: Allow for TYPE_PORT92 to be embedded in devices

[Mark Cave-Ayland]

On 18/02/2024 13:16, Bernhard Beschow wrote:

> Port 92 is an integral part of south bridges. Allow for embedding it there.
> 
> South bridges aren't architecture-specific, so move port92.c to hw/isa which is
> accessible to other architectures than x86.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   include/hw/i386/pc.h      |  5 -----
>   include/hw/isa/port92.h   | 30 ++++++++++++++++++++++++++++++
>   hw/i386/pc.c              |  1 +
>   hw/{i386 => isa}/port92.c | 14 +-------------
>   hw/i386/Kconfig           |  1 +
>   hw/i386/meson.build       |  3 +--
>   hw/i386/trace-events      |  4 ----
>   hw/isa/Kconfig            |  3 +++
>   hw/isa/meson.build        |  1 +
>   hw/isa/trace-events       |  4 ++++
>   10 files changed, 42 insertions(+), 24 deletions(-)
>   create mode 100644 include/hw/isa/port92.h
>   rename hw/{i386 => isa}/port92.c (91%)
> 
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index ec0e5efcb2..b2987209b1 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -188,11 +188,6 @@ void pc_nic_init(PCMachineClass *pcmc, ISABus *isa_bus, PCIBus *pci_bus);
>   
>   void pc_i8259_create(ISABus *isa_bus, qemu_irq *i8259_irqs);
>   
> -/* port92.c */
> -#define PORT92_A20_LINE "a20"
> -
> -#define TYPE_PORT92 "port92"
> -
>   /* pc_sysfw.c */
>   void pc_system_flash_create(PCMachineState *pcms);
>   void pc_system_flash_cleanup_unused(PCMachineState *pcms);
> diff --git a/include/hw/isa/port92.h b/include/hw/isa/port92.h
> new file mode 100644
> index 0000000000..214783d071
> --- /dev/null
> +++ b/include/hw/isa/port92.h
> @@ -0,0 +1,30 @@
> +/*
> + * QEMU I/O port 0x92 (System Control Port A, to handle Fast Gate A20)
> + *
> + * Copyright (c) 2003-2004 Fabrice Bellard
> + *
> + * SPDX-License-Identifier: MIT
> + */
> +
> +#ifndef HW_PORT92_H
> +#define HW_PORT92_H
> +
> +#include "exec/memory.h"
> +#include "hw/irq.h"
> +#include "hw/isa/isa.h"
> +#include "qom/object.h"
> +
> +#define TYPE_PORT92 "port92"
> +OBJECT_DECLARE_SIMPLE_TYPE(Port92State, PORT92)
> +
> +struct Port92State {
> +    ISADevice parent_obj;
> +
> +    MemoryRegion io;
> +    uint8_t outport;
> +    qemu_irq a20_out;
> +};
> +
> +#define PORT92_A20_LINE "a20"
> +
> +#endif /* HW_PORT92_H */
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 196827531a..0b11d4576e 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -32,6 +32,7 @@
>   #include "hw/i386/vmport.h"
>   #include "sysemu/cpus.h"
>   #include "hw/ide/internal.h"
> +#include "hw/isa/port92.h"
>   #include "hw/timer/hpet.h"
>   #include "hw/loader.h"
>   #include "hw/rtc/mc146818rtc.h"
> diff --git a/hw/i386/port92.c b/hw/isa/port92.c
> similarity index 91%
> rename from hw/i386/port92.c
> rename to hw/isa/port92.c
> index 1070bfbf36..06df06b088 100644
> --- a/hw/i386/port92.c
> +++ b/hw/isa/port92.c
> @@ -9,20 +9,8 @@
>   #include "qemu/osdep.h"
>   #include "sysemu/runstate.h"
>   #include "migration/vmstate.h"
> -#include "hw/irq.h"
> -#include "hw/i386/pc.h"
> +#include "hw/isa/port92.h"
>   #include "trace.h"
> -#include "qom/object.h"
> -
> -OBJECT_DECLARE_SIMPLE_TYPE(Port92State, PORT92)
> -
> -struct Port92State {
> -    ISADevice parent_obj;
> -
> -    MemoryRegion io;
> -    uint8_t outport;
> -    qemu_irq a20_out;
> -};
>   
>   static void port92_write(void *opaque, hwaddr addr, uint64_t val,
>                            unsigned size)
> diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig
> index a1846be6f7..ccf6de4a00 100644
> --- a/hw/i386/Kconfig
> +++ b/hw/i386/Kconfig
> @@ -37,6 +37,7 @@ config PC
>       select I8254
>       select PCKBD
>       select PCSPK
> +    select PORT92
>       select I8257
>       select MC146818RTC
>       # For ACPI builder:
> diff --git a/hw/i386/meson.build b/hw/i386/meson.build
> index b9c1ca39cb..94d558edd6 100644
> --- a/hw/i386/meson.build
> +++ b/hw/i386/meson.build
> @@ -24,8 +24,7 @@ i386_ss.add(when: 'CONFIG_ACPI', if_true: files('acpi-common.c'))
>   i386_ss.add(when: 'CONFIG_PC', if_true: files(
>     'pc.c',
>     'pc_sysfw.c',
> -  'acpi-build.c',
> -  'port92.c'))
> +  'acpi-build.c'))
>   i386_ss.add(when: 'CONFIG_X86_FW_OVMF', if_true: files('pc_sysfw_ovmf.c'),
>                                           if_false: files('pc_sysfw_ovmf-stubs.c'))
>   
> diff --git a/hw/i386/trace-events b/hw/i386/trace-events
> index 53c02d7ac8..b730589394 100644
> --- a/hw/i386/trace-events
> +++ b/hw/i386/trace-events
> @@ -118,10 +118,6 @@ vmport_command(unsigned char command) "command: 0x%02x"
>   x86_gsi_interrupt(int irqn, int level) "GSI interrupt #%d level:%d"
>   x86_pic_interrupt(int irqn, int level) "PIC interrupt #%d level:%d"
>   
> -# port92.c
> -port92_read(uint8_t val) "port92: read 0x%02x"
> -port92_write(uint8_t val) "port92: write 0x%02x"
> -
>   # vmmouse.c
>   vmmouse_get_status(void) ""
>   vmmouse_mouse_event(int x, int y, int dz, int buttons_state) "event: x=%d y=%d dz=%d state=%d"
> diff --git a/hw/isa/Kconfig b/hw/isa/Kconfig
> index 73c6470805..efdf43e92c 100644
> --- a/hw/isa/Kconfig
> +++ b/hw/isa/Kconfig
> @@ -49,6 +49,9 @@ config PIIX
>       select MC146818RTC
>       select USB_UHCI
>   
> +config PORT92
> +    bool
> +
>   config VT82C686
>       bool
>       select ISA_BUS
> diff --git a/hw/isa/meson.build b/hw/isa/meson.build
> index 3219282217..fb7cd44984 100644
> --- a/hw/isa/meson.build
> +++ b/hw/isa/meson.build
> @@ -5,6 +5,7 @@ system_ss.add(when: 'CONFIG_ISA_BUS', if_true: files('isa-bus.c'))
>   system_ss.add(when: 'CONFIG_ISA_SUPERIO', if_true: files('isa-superio.c'))
>   system_ss.add(when: 'CONFIG_PC87312', if_true: files('pc87312.c'))
>   system_ss.add(when: 'CONFIG_PIIX', if_true: files('piix.c'))
> +system_ss.add(when: 'CONFIG_PORT92', if_true: files('port92.c'))
>   system_ss.add(when: 'CONFIG_SMC37C669', if_true: files('smc37c669-superio.c'))
>   system_ss.add(when: 'CONFIG_VT82C686', if_true: files('vt82c686.c'))
>   
> diff --git a/hw/isa/trace-events b/hw/isa/trace-events
> index 1816e8307a..bb5d9f1078 100644
> --- a/hw/isa/trace-events
> +++ b/hw/isa/trace-events
> @@ -10,6 +10,10 @@ superio_create_ide(int id, uint16_t base, unsigned int irq) "id=%d, base 0x%03x,
>   pc87312_io_read(uint32_t addr, uint32_t val) "read addr=0x%x val=0x%x"
>   pc87312_io_write(uint32_t addr, uint32_t val) "write addr=0x%x val=0x%x"
>   
> +# port92.c
> +port92_read(uint8_t val) "port92: read 0x%02x"
> +port92_write(uint8_t val) "port92: write 0x%02x"
> +
>   # apm.c
>   apm_io_read(uint8_t addr, uint8_t val) "read addr=0x%x val=0x%02x"
>   apm_io_write(uint8_t addr, uint8_t val) "write addr=0x%x val=0x%02x"

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

[PATCH 3/5] hw/isa: Embed TYPE_PORT92 in south bridges used in PC machines

[Bernhard Beschow]

Port 92 is an integral part of the PIIX and ICH south bridges, so instantiate it
there. The isapc machine now needs to instantiate it explicitly, analoguous to
the RTC.

Note that due to migration compatibility, port92 is optional in the south
bridges. It is always instantiated the isapc machine for simplicity.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/i386/pc.h          |  2 +-
 include/hw/southbridge/ich9.h |  4 ++++
 include/hw/southbridge/piix.h |  3 +++
 hw/i386/pc.c                  | 18 ++++++++++++------
 hw/i386/pc_piix.c             |  9 +++++++--
 hw/i386/pc_q35.c              |  8 +++++---
 hw/isa/lpc_ich9.c             |  9 +++++++++
 hw/isa/piix.c                 |  9 +++++++++
 hw/isa/Kconfig                |  2 ++
 9 files changed, 52 insertions(+), 12 deletions(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index b2987209b1..a9ff1f5ab3 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -178,7 +178,7 @@ uint64_t pc_pci_hole64_start(void);
 DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus);
 void pc_basic_device_init(struct PCMachineState *pcms,
                           ISABus *isa_bus, qemu_irq *gsi,
-                          ISADevice *rtc_state,
+                          ISADevice *rtc_state, ISADevice *port92,
                           bool create_fdctrl,
                           uint32_t hpet_irqs);
 void pc_cmos_init(PCMachineState *pcms,
diff --git a/include/hw/southbridge/ich9.h b/include/hw/southbridge/ich9.h
index fd01649d04..d70a94f5e7 100644
--- a/include/hw/southbridge/ich9.h
+++ b/include/hw/southbridge/ich9.h
@@ -3,6 +3,7 @@
 
 #include "hw/isa/apm.h"
 #include "hw/acpi/ich9.h"
+#include "hw/isa/port92.h"
 #include "hw/intc/ioapic.h"
 #include "hw/pci/pci.h"
 #include "hw/pci/pci_device.h"
@@ -32,6 +33,7 @@ struct ICH9LPCState {
     uint8_t irr[PCI_SLOT_MAX][PCI_NUM_PINS];
 
     MC146818RtcState rtc;
+    Port92State port92;
     APMState apm;
     ICH9LPCPMRegs pm;
     uint32_t sci_level; /* track sci level */
@@ -54,6 +56,8 @@ struct ICH9LPCState {
     uint8_t rst_cnt;
     MemoryRegion rst_cnt_mem;
 
+    bool has_port92;
+
     /* SMI feature negotiation via fw_cfg */
     uint64_t smi_host_features;       /* guest-invisible, host endian */
     uint8_t smi_host_features_le[8];  /* guest-visible, read-only, little
diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
index 86709ba2e4..35058529d1 100644
--- a/include/hw/southbridge/piix.h
+++ b/include/hw/southbridge/piix.h
@@ -15,6 +15,7 @@
 #include "hw/pci/pci_device.h"
 #include "hw/acpi/piix4.h"
 #include "hw/ide/pci.h"
+#include "hw/isa/port92.h"
 #include "hw/rtc/mc146818rtc.h"
 #include "hw/usb/hcd-uhci.h"
 
@@ -56,6 +57,7 @@ struct PIIXState {
     int32_t pci_irq_levels_vmstate[PIIX_NUM_PIRQS];
 
     MC146818RtcState rtc;
+    Port92State port92;
     PCIIDEState ide;
     UHCIState uhci;
     PIIX4PMState pm;
@@ -71,6 +73,7 @@ struct PIIXState {
     bool has_acpi;
     bool has_pic;
     bool has_pit;
+    bool has_port92;
     bool has_usb;
     bool smm_enabled;
 };
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 0b11d4576e..8b601ea6cf 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1160,7 +1160,7 @@ static void pc_superio_init(ISABus *isa_bus, bool create_fdctrl,
     int i;
     DriveInfo *fd[MAX_FD];
     qemu_irq *a20_line;
-    ISADevice *fdc, *i8042, *port92, *vmmouse;
+    ISADevice *fdc, *i8042, *vmmouse;
 
     serial_hds_isa_init(isa_bus, 0, MAX_ISA_SERIAL_PORTS);
     parallel_hds_isa_init(isa_bus, MAX_PARALLEL_PORTS);
@@ -1193,18 +1193,15 @@ static void pc_superio_init(ISABus *isa_bus, bool create_fdctrl,
                                  &error_abort);
         isa_realize_and_unref(vmmouse, isa_bus, &error_fatal);
     }
-    port92 = isa_create_simple(isa_bus, TYPE_PORT92);
 
-    a20_line = qemu_allocate_irqs(handle_a20_line_change, first_cpu, 2);
+    a20_line = qemu_allocate_irqs(handle_a20_line_change, first_cpu, 1);
     i8042_setup_a20_line(i8042, a20_line[0]);
-    qdev_connect_gpio_out_named(DEVICE(port92),
-                                PORT92_A20_LINE, 0, a20_line[1]);
     g_free(a20_line);
 }
 
 void pc_basic_device_init(struct PCMachineState *pcms,
                           ISABus *isa_bus, qemu_irq *gsi,
-                          ISADevice *rtc_state,
+                          ISADevice *rtc_state, ISADevice *port92,
                           bool create_fdctrl,
                           uint32_t hpet_irqs)
 {
@@ -1296,6 +1293,15 @@ void pc_basic_device_init(struct PCMachineState *pcms,
     /* Super I/O */
     pc_superio_init(isa_bus, create_fdctrl, pcms->i8042_enabled,
                     pcms->vmport != ON_OFF_AUTO_ON);
+
+    if (port92) {
+        qemu_irq *a20_line;
+
+        a20_line = qemu_allocate_irqs(handle_a20_line_change, first_cpu, 1);
+        qdev_connect_gpio_out_named(DEVICE(port92),
+                                    PORT92_A20_LINE, 0, a20_line[0]);
+        g_free(a20_line);
+    }
 }
 
 void pc_nic_init(PCMachineClass *pcmc, ISABus *isa_bus, PCIBus *pci_bus)
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 999b7b806c..dfdfd36551 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -115,7 +115,7 @@ static void pc_init1(MachineState *machine,
     qemu_irq smi_irq;
     GSIState *gsi_state;
     BusState *idebus[MAX_IDE_BUS];
-    ISADevice *rtc_state;
+    ISADevice *rtc_state, *port92;
     MemoryRegion *ram_memory;
     MemoryRegion *pci_memory = NULL;
     MemoryRegion *rom_memory = system_memory;
@@ -269,6 +269,8 @@ static void pc_init1(MachineState *machine,
                                  &error_abort);
         object_property_set_bool(OBJECT(pci_dev), "has-pit", false,
                                  &error_abort);
+        object_property_set_bool(OBJECT(pci_dev), "has-port92",
+                                 pcms->i8042_enabled, &error_abort);
         qdev_prop_set_uint32(DEVICE(pci_dev), "smb_io_base", 0xb100);
         object_property_set_bool(OBJECT(pci_dev), "smm-enabled",
                                  x86_machine_is_smm_enabled(x86ms),
@@ -296,6 +298,8 @@ static void pc_init1(MachineState *machine,
         isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(pci_dev), "isa.0"));
         rtc_state = ISA_DEVICE(object_resolve_path_component(OBJECT(pci_dev),
                                                              "rtc"));
+        port92 = ISA_DEVICE(object_resolve_path_component(OBJECT(pci_dev),
+                                                          "port92"));
         piix4_pm = object_resolve_path_component(OBJECT(pci_dev), "pm");
         dev = DEVICE(object_resolve_path_component(OBJECT(pci_dev), "ide"));
         pci_ide_create_devs(PCI_DEVICE(dev));
@@ -310,6 +314,7 @@ static void pc_init1(MachineState *machine,
         qdev_prop_set_int32(DEVICE(rtc_state), "base_year", 2000);
         isa_realize_and_unref(rtc_state, isa_bus, &error_fatal);
 
+        port92 = isa_create_simple(isa_bus, TYPE_PORT92);
         i8257_dma_init(OBJECT(machine), isa_bus, 0);
         pcms->hpet_enabled = false;
         idebus[0] = NULL;
@@ -336,7 +341,7 @@ static void pc_init1(MachineState *machine,
     }
 
     /* init basic PC hardware */
-    pc_basic_device_init(pcms, isa_bus, x86ms->gsi, rtc_state, true,
+    pc_basic_device_init(pcms, isa_bus, x86ms->gsi, rtc_state, port92, true,
                          0x4);
 
     pc_nic_init(pcmc, isa_bus, pci_bus);
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index d346fa3b1d..26bb1c2cb9 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -127,7 +127,7 @@ static void pc_q35_init(MachineState *machine)
     PCIDevice *lpc;
     DeviceState *lpc_dev;
     BusState *idebus[MAX_SATA_PORTS];
-    ISADevice *rtc_state;
+    ISADevice *rtc_state, *port92;
     MemoryRegion *system_memory = get_system_memory();
     MemoryRegion *system_io = get_system_io();
     MemoryRegion *pci_memory = g_new(MemoryRegion, 1);
@@ -238,6 +238,7 @@ static void pc_q35_init(MachineState *machine)
     lpc = pci_new_multifunction(PCI_DEVFN(ICH9_LPC_DEV, ICH9_LPC_FUNC),
                                 TYPE_ICH9_LPC_DEVICE);
     lpc_dev = DEVICE(lpc);
+    qdev_prop_set_bit(lpc_dev, "has-port92", pcms->i8042_enabled);
     qdev_prop_set_bit(lpc_dev, "smm-enabled",
                       x86_machine_is_smm_enabled(x86ms));
     pci_realize_and_unref(lpc, host_bus, &error_fatal);
@@ -246,6 +247,7 @@ static void pc_q35_init(MachineState *machine)
     }
 
     rtc_state = ISA_DEVICE(object_resolve_path_component(OBJECT(lpc), "rtc"));
+    port92 = ISA_DEVICE(object_resolve_path_component(OBJECT(lpc), "port92"));
 
     object_property_add_link(OBJECT(machine), PC_MACHINE_ACPI_DEVICE_PROP,
                              TYPE_HOTPLUG_HANDLER,
@@ -287,8 +289,8 @@ static void pc_q35_init(MachineState *machine)
     }
 
     /* init basic PC hardware */
-    pc_basic_device_init(pcms, isa_bus, x86ms->gsi, rtc_state, !mc->no_floppy,
-                         0xff0104);
+    pc_basic_device_init(pcms, isa_bus, x86ms->gsi, rtc_state, port92,
+                         !mc->no_floppy, 0xff0104);
 
     if (pcms->sata_enabled) {
         PCIDevice *pdev;
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 70c6e8a093..3be5bc01b1 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -749,6 +749,14 @@ static void ich9_lpc_realize(PCIDevice *d, Error **errp)
     irq = object_property_get_uint(OBJECT(&lpc->rtc), "irq", &error_fatal);
     isa_connect_gpio_out(ISA_DEVICE(&lpc->rtc), 0, irq);
 
+    if (lpc->has_port92) {
+        object_initialize_child(OBJECT(lpc), "port92", &lpc->port92,
+                                TYPE_PORT92);
+        if (!qdev_realize(DEVICE(&lpc->port92), BUS(isa_bus), errp)) {
+            return;
+        }
+    }
+
     pci_bus_irqs(pci_bus, ich9_lpc_set_irq, d, ICH9_LPC_NB_PIRQS);
     pci_bus_map_irqs(pci_bus, ich9_lpc_map_irq);
     pci_bus_set_route_irq_fn(pci_bus, ich9_route_intx_pin_to_irq);
@@ -821,6 +829,7 @@ static Property ich9_lpc_properties[] = {
     DEFINE_PROP_BOOL("noreboot", ICH9LPCState, pin_strap.spkr_hi, false),
     DEFINE_PROP_BOOL("smm-compat", ICH9LPCState, pm.smm_compat, false),
     DEFINE_PROP_BOOL("smm-enabled", ICH9LPCState, pm.smm_enabled, false),
+    DEFINE_PROP_BOOL("has-port92", ICH9LPCState, has_port92, true),
     DEFINE_PROP_BIT64("x-smi-broadcast", ICH9LPCState, smi_host_features,
                       ICH9_LPC_SMI_F_BROADCAST_BIT, true),
     DEFINE_PROP_BIT64("x-smi-cpu-hotplug", ICH9LPCState, smi_host_features,
diff --git a/hw/isa/piix.c b/hw/isa/piix.c
index 2d30711b17..4c12855461 100644
--- a/hw/isa/piix.c
+++ b/hw/isa/piix.c
@@ -346,6 +346,14 @@ static void pci_piix_realize(PCIDevice *dev, const char *uhci_type,
     irq = object_property_get_uint(OBJECT(&d->rtc), "irq", &error_fatal);
     isa_connect_gpio_out(ISA_DEVICE(&d->rtc), 0, irq);
 
+    /* Port 92 */
+    if (d->has_port92) {
+        object_initialize_child(OBJECT(d), "port92", &d->port92, TYPE_PORT92);
+        if (!qdev_realize(DEVICE(&d->port92), BUS(isa_bus), errp)) {
+            return;
+        }
+    }
+
     /* IDE */
     qdev_prop_set_int32(DEVICE(&d->ide), "addr", dev->devfn + 1);
     if (!qdev_realize(DEVICE(&d->ide), BUS(pci_bus), errp)) {
@@ -413,6 +421,7 @@ static Property pci_piix_props[] = {
     DEFINE_PROP_BOOL("has-acpi", PIIXState, has_acpi, true),
     DEFINE_PROP_BOOL("has-pic", PIIXState, has_pic, true),
     DEFINE_PROP_BOOL("has-pit", PIIXState, has_pit, true),
+    DEFINE_PROP_BOOL("has-port92", PIIXState, has_port92, true),
     DEFINE_PROP_BOOL("has-usb", PIIXState, has_usb, true),
     DEFINE_PROP_BOOL("smm-enabled", PIIXState, smm_enabled, false),
     DEFINE_PROP_END_OF_LIST(),
diff --git a/hw/isa/Kconfig b/hw/isa/Kconfig
index efdf43e92c..f42a087c07 100644
--- a/hw/isa/Kconfig
+++ b/hw/isa/Kconfig
@@ -47,6 +47,7 @@ config PIIX
     select IDE_PIIX
     select ISA_BUS
     select MC146818RTC
+    select PORT92
     select USB_UHCI
 
 config PORT92
@@ -78,3 +79,4 @@ config LPC_ICH9
     select ISA_BUS
     select ACPI_ICH9
     select MC146818RTC
+    select PORT92
-- 
2.43.2

Re: [PATCH 3/5] hw/isa: Embed TYPE_PORT92 in south bridges used in PC machines

[Mark Cave-Ayland]

On 18/02/2024 13:16, Bernhard Beschow wrote:

> Port 92 is an integral part of the PIIX and ICH south bridges, so instantiate it
> there. The isapc machine now needs to instantiate it explicitly, analoguous to
> the RTC.
> 
> Note that due to migration compatibility, port92 is optional in the south
> bridges. It is always instantiated the isapc machine for simplicity.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   include/hw/i386/pc.h          |  2 +-
>   include/hw/southbridge/ich9.h |  4 ++++
>   include/hw/southbridge/piix.h |  3 +++
>   hw/i386/pc.c                  | 18 ++++++++++++------
>   hw/i386/pc_piix.c             |  9 +++++++--
>   hw/i386/pc_q35.c              |  8 +++++---
>   hw/isa/lpc_ich9.c             |  9 +++++++++
>   hw/isa/piix.c                 |  9 +++++++++
>   hw/isa/Kconfig                |  2 ++
>   9 files changed, 52 insertions(+), 12 deletions(-)
> 
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index b2987209b1..a9ff1f5ab3 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -178,7 +178,7 @@ uint64_t pc_pci_hole64_start(void);
>   DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus);
>   void pc_basic_device_init(struct PCMachineState *pcms,
>                             ISABus *isa_bus, qemu_irq *gsi,
> -                          ISADevice *rtc_state,
> +                          ISADevice *rtc_state, ISADevice *port92,
>                             bool create_fdctrl,
>                             uint32_t hpet_irqs);
>   void pc_cmos_init(PCMachineState *pcms,
> diff --git a/include/hw/southbridge/ich9.h b/include/hw/southbridge/ich9.h
> index fd01649d04..d70a94f5e7 100644
> --- a/include/hw/southbridge/ich9.h
> +++ b/include/hw/southbridge/ich9.h
> @@ -3,6 +3,7 @@
>   
>   #include "hw/isa/apm.h"
>   #include "hw/acpi/ich9.h"
> +#include "hw/isa/port92.h"
>   #include "hw/intc/ioapic.h"
>   #include "hw/pci/pci.h"
>   #include "hw/pci/pci_device.h"
> @@ -32,6 +33,7 @@ struct ICH9LPCState {
>       uint8_t irr[PCI_SLOT_MAX][PCI_NUM_PINS];
>   
>       MC146818RtcState rtc;
> +    Port92State port92;
>       APMState apm;
>       ICH9LPCPMRegs pm;
>       uint32_t sci_level; /* track sci level */
> @@ -54,6 +56,8 @@ struct ICH9LPCState {
>       uint8_t rst_cnt;
>       MemoryRegion rst_cnt_mem;
>   
> +    bool has_port92;
> +
>       /* SMI feature negotiation via fw_cfg */
>       uint64_t smi_host_features;       /* guest-invisible, host endian */
>       uint8_t smi_host_features_le[8];  /* guest-visible, read-only, little
> diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
> index 86709ba2e4..35058529d1 100644
> --- a/include/hw/southbridge/piix.h
> +++ b/include/hw/southbridge/piix.h
> @@ -15,6 +15,7 @@
>   #include "hw/pci/pci_device.h"
>   #include "hw/acpi/piix4.h"
>   #include "hw/ide/pci.h"
> +#include "hw/isa/port92.h"
>   #include "hw/rtc/mc146818rtc.h"
>   #include "hw/usb/hcd-uhci.h"
>   
> @@ -56,6 +57,7 @@ struct PIIXState {
>       int32_t pci_irq_levels_vmstate[PIIX_NUM_PIRQS];
>   
>       MC146818RtcState rtc;
> +    Port92State port92;
>       PCIIDEState ide;
>       UHCIState uhci;
>       PIIX4PMState pm;
> @@ -71,6 +73,7 @@ struct PIIXState {
>       bool has_acpi;
>       bool has_pic;
>       bool has_pit;
> +    bool has_port92;
>       bool has_usb;
>       bool smm_enabled;
>   };
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 0b11d4576e..8b601ea6cf 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1160,7 +1160,7 @@ static void pc_superio_init(ISABus *isa_bus, bool create_fdctrl,
>       int i;
>       DriveInfo *fd[MAX_FD];
>       qemu_irq *a20_line;
> -    ISADevice *fdc, *i8042, *port92, *vmmouse;
> +    ISADevice *fdc, *i8042, *vmmouse;
>   
>       serial_hds_isa_init(isa_bus, 0, MAX_ISA_SERIAL_PORTS);
>       parallel_hds_isa_init(isa_bus, MAX_PARALLEL_PORTS);
> @@ -1193,18 +1193,15 @@ static void pc_superio_init(ISABus *isa_bus, bool create_fdctrl,
>                                    &error_abort);
>           isa_realize_and_unref(vmmouse, isa_bus, &error_fatal);
>       }
> -    port92 = isa_create_simple(isa_bus, TYPE_PORT92);
>   
> -    a20_line = qemu_allocate_irqs(handle_a20_line_change, first_cpu, 2);
> +    a20_line = qemu_allocate_irqs(handle_a20_line_change, first_cpu, 1);
>       i8042_setup_a20_line(i8042, a20_line[0]);
> -    qdev_connect_gpio_out_named(DEVICE(port92),
> -                                PORT92_A20_LINE, 0, a20_line[1]);
>       g_free(a20_line);
>   }
>   
>   void pc_basic_device_init(struct PCMachineState *pcms,
>                             ISABus *isa_bus, qemu_irq *gsi,
> -                          ISADevice *rtc_state,
> +                          ISADevice *rtc_state, ISADevice *port92,
>                             bool create_fdctrl,
>                             uint32_t hpet_irqs)
>   {
> @@ -1296,6 +1293,15 @@ void pc_basic_device_init(struct PCMachineState *pcms,
>       /* Super I/O */
>       pc_superio_init(isa_bus, create_fdctrl, pcms->i8042_enabled,
>                       pcms->vmport != ON_OFF_AUTO_ON);
> +
> +    if (port92) {
> +        qemu_irq *a20_line;
> +
> +        a20_line = qemu_allocate_irqs(handle_a20_line_change, first_cpu, 1);
> +        qdev_connect_gpio_out_named(DEVICE(port92),
> +                                    PORT92_A20_LINE, 0, a20_line[0]);
> +        g_free(a20_line);
> +    }
>   }
>   
>   void pc_nic_init(PCMachineClass *pcmc, ISABus *isa_bus, PCIBus *pci_bus)
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 999b7b806c..dfdfd36551 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -115,7 +115,7 @@ static void pc_init1(MachineState *machine,
>       qemu_irq smi_irq;
>       GSIState *gsi_state;
>       BusState *idebus[MAX_IDE_BUS];
> -    ISADevice *rtc_state;
> +    ISADevice *rtc_state, *port92;
>       MemoryRegion *ram_memory;
>       MemoryRegion *pci_memory = NULL;
>       MemoryRegion *rom_memory = system_memory;
> @@ -269,6 +269,8 @@ static void pc_init1(MachineState *machine,
>                                    &error_abort);
>           object_property_set_bool(OBJECT(pci_dev), "has-pit", false,
>                                    &error_abort);
> +        object_property_set_bool(OBJECT(pci_dev), "has-port92",
> +                                 pcms->i8042_enabled, &error_abort);
>           qdev_prop_set_uint32(DEVICE(pci_dev), "smb_io_base", 0xb100);
>           object_property_set_bool(OBJECT(pci_dev), "smm-enabled",
>                                    x86_machine_is_smm_enabled(x86ms),
> @@ -296,6 +298,8 @@ static void pc_init1(MachineState *machine,
>           isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(pci_dev), "isa.0"));
>           rtc_state = ISA_DEVICE(object_resolve_path_component(OBJECT(pci_dev),
>                                                                "rtc"));
> +        port92 = ISA_DEVICE(object_resolve_path_component(OBJECT(pci_dev),
> +                                                          "port92"));
>           piix4_pm = object_resolve_path_component(OBJECT(pci_dev), "pm");
>           dev = DEVICE(object_resolve_path_component(OBJECT(pci_dev), "ide"));
>           pci_ide_create_devs(PCI_DEVICE(dev));
> @@ -310,6 +314,7 @@ static void pc_init1(MachineState *machine,
>           qdev_prop_set_int32(DEVICE(rtc_state), "base_year", 2000);
>           isa_realize_and_unref(rtc_state, isa_bus, &error_fatal);
>   
> +        port92 = isa_create_simple(isa_bus, TYPE_PORT92);
>           i8257_dma_init(OBJECT(machine), isa_bus, 0);
>           pcms->hpet_enabled = false;
>           idebus[0] = NULL;
> @@ -336,7 +341,7 @@ static void pc_init1(MachineState *machine,
>       }
>   
>       /* init basic PC hardware */
> -    pc_basic_device_init(pcms, isa_bus, x86ms->gsi, rtc_state, true,
> +    pc_basic_device_init(pcms, isa_bus, x86ms->gsi, rtc_state, port92, true,
>                            0x4);
>   
>       pc_nic_init(pcmc, isa_bus, pci_bus);
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index d346fa3b1d..26bb1c2cb9 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -127,7 +127,7 @@ static void pc_q35_init(MachineState *machine)
>       PCIDevice *lpc;
>       DeviceState *lpc_dev;
>       BusState *idebus[MAX_SATA_PORTS];
> -    ISADevice *rtc_state;
> +    ISADevice *rtc_state, *port92;
>       MemoryRegion *system_memory = get_system_memory();
>       MemoryRegion *system_io = get_system_io();
>       MemoryRegion *pci_memory = g_new(MemoryRegion, 1);
> @@ -238,6 +238,7 @@ static void pc_q35_init(MachineState *machine)
>       lpc = pci_new_multifunction(PCI_DEVFN(ICH9_LPC_DEV, ICH9_LPC_FUNC),
>                                   TYPE_ICH9_LPC_DEVICE);
>       lpc_dev = DEVICE(lpc);
> +    qdev_prop_set_bit(lpc_dev, "has-port92", pcms->i8042_enabled);
>       qdev_prop_set_bit(lpc_dev, "smm-enabled",
>                         x86_machine_is_smm_enabled(x86ms));
>       pci_realize_and_unref(lpc, host_bus, &error_fatal);
> @@ -246,6 +247,7 @@ static void pc_q35_init(MachineState *machine)
>       }
>   
>       rtc_state = ISA_DEVICE(object_resolve_path_component(OBJECT(lpc), "rtc"));
> +    port92 = ISA_DEVICE(object_resolve_path_component(OBJECT(lpc), "port92"));
>   
>       object_property_add_link(OBJECT(machine), PC_MACHINE_ACPI_DEVICE_PROP,
>                                TYPE_HOTPLUG_HANDLER,
> @@ -287,8 +289,8 @@ static void pc_q35_init(MachineState *machine)
>       }
>   
>       /* init basic PC hardware */
> -    pc_basic_device_init(pcms, isa_bus, x86ms->gsi, rtc_state, !mc->no_floppy,
> -                         0xff0104);
> +    pc_basic_device_init(pcms, isa_bus, x86ms->gsi, rtc_state, port92,
> +                         !mc->no_floppy, 0xff0104);
>   
>       if (pcms->sata_enabled) {
>           PCIDevice *pdev;
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index 70c6e8a093..3be5bc01b1 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -749,6 +749,14 @@ static void ich9_lpc_realize(PCIDevice *d, Error **errp)
>       irq = object_property_get_uint(OBJECT(&lpc->rtc), "irq", &error_fatal);
>       isa_connect_gpio_out(ISA_DEVICE(&lpc->rtc), 0, irq);
>   
> +    if (lpc->has_port92) {
> +        object_initialize_child(OBJECT(lpc), "port92", &lpc->port92,
> +                                TYPE_PORT92);
> +        if (!qdev_realize(DEVICE(&lpc->port92), BUS(isa_bus), errp)) {
> +            return;
> +        }
> +    }
> +
>       pci_bus_irqs(pci_bus, ich9_lpc_set_irq, d, ICH9_LPC_NB_PIRQS);
>       pci_bus_map_irqs(pci_bus, ich9_lpc_map_irq);
>       pci_bus_set_route_irq_fn(pci_bus, ich9_route_intx_pin_to_irq);
> @@ -821,6 +829,7 @@ static Property ich9_lpc_properties[] = {
>       DEFINE_PROP_BOOL("noreboot", ICH9LPCState, pin_strap.spkr_hi, false),
>       DEFINE_PROP_BOOL("smm-compat", ICH9LPCState, pm.smm_compat, false),
>       DEFINE_PROP_BOOL("smm-enabled", ICH9LPCState, pm.smm_enabled, false),
> +    DEFINE_PROP_BOOL("has-port92", ICH9LPCState, has_port92, true),
>       DEFINE_PROP_BIT64("x-smi-broadcast", ICH9LPCState, smi_host_features,
>                         ICH9_LPC_SMI_F_BROADCAST_BIT, true),
>       DEFINE_PROP_BIT64("x-smi-cpu-hotplug", ICH9LPCState, smi_host_features,
> diff --git a/hw/isa/piix.c b/hw/isa/piix.c
> index 2d30711b17..4c12855461 100644
> --- a/hw/isa/piix.c
> +++ b/hw/isa/piix.c
> @@ -346,6 +346,14 @@ static void pci_piix_realize(PCIDevice *dev, const char *uhci_type,
>       irq = object_property_get_uint(OBJECT(&d->rtc), "irq", &error_fatal);
>       isa_connect_gpio_out(ISA_DEVICE(&d->rtc), 0, irq);
>   
> +    /* Port 92 */
> +    if (d->has_port92) {
> +        object_initialize_child(OBJECT(d), "port92", &d->port92, TYPE_PORT92);
> +        if (!qdev_realize(DEVICE(&d->port92), BUS(isa_bus), errp)) {
> +            return;
> +        }
> +    }
> +
>       /* IDE */
>       qdev_prop_set_int32(DEVICE(&d->ide), "addr", dev->devfn + 1);
>       if (!qdev_realize(DEVICE(&d->ide), BUS(pci_bus), errp)) {
> @@ -413,6 +421,7 @@ static Property pci_piix_props[] = {
>       DEFINE_PROP_BOOL("has-acpi", PIIXState, has_acpi, true),
>       DEFINE_PROP_BOOL("has-pic", PIIXState, has_pic, true),
>       DEFINE_PROP_BOOL("has-pit", PIIXState, has_pit, true),
> +    DEFINE_PROP_BOOL("has-port92", PIIXState, has_port92, true),
>       DEFINE_PROP_BOOL("has-usb", PIIXState, has_usb, true),
>       DEFINE_PROP_BOOL("smm-enabled", PIIXState, smm_enabled, false),
>       DEFINE_PROP_END_OF_LIST(),
> diff --git a/hw/isa/Kconfig b/hw/isa/Kconfig
> index efdf43e92c..f42a087c07 100644
> --- a/hw/isa/Kconfig
> +++ b/hw/isa/Kconfig
> @@ -47,6 +47,7 @@ config PIIX
>       select IDE_PIIX
>       select ISA_BUS
>       select MC146818RTC
> +    select PORT92
>       select USB_UHCI
>   
>   config PORT92
> @@ -78,3 +79,4 @@ config LPC_ICH9
>       select ISA_BUS
>       select ACPI_ICH9
>       select MC146818RTC
> +    select PORT92

I had a look at this (and did a bit of revision around 8042 and A20), and I am 
starting to wonder if the PORT92 device isn't something that belongs to the 
southbridge, but more specifically to the superio chip?

A couple of thoughts as to why I came to this conclusion: firstly the superio chip is 
generally considered to be a single integrated implementation of legacy IO devices, 
so this feels like a natural home for the PORT92 device. Secondly the value of the 
"has-port92" property is controlled by pcms->i8042_enabled, and this value is already 
passed into functions such as pc_superio_init() for example.

I think this would also help reduce the changes required for the individual machines, 
however the devil is always in the details particularly when migration is involved.


ATB,

Mark.

Re: [PATCH 3/5] hw/isa: Embed TYPE_PORT92 in south bridges used in PC machines

[BALATON Zoltan]

On Wed, 21 Feb 2024, Mark Cave-Ayland wrote:
> On 18/02/2024 13:16, Bernhard Beschow wrote:
>> Port 92 is an integral part of the PIIX and ICH south bridges, so 
>> instantiate it
>> there. The isapc machine now needs to instantiate it explicitly, analoguous 
>> to
>> the RTC.
>> 
>> Note that due to migration compatibility, port92 is optional in the south
>> bridges. It is always instantiated the isapc machine for simplicity.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>   include/hw/i386/pc.h          |  2 +-
>>   include/hw/southbridge/ich9.h |  4 ++++
>>   include/hw/southbridge/piix.h |  3 +++
>>   hw/i386/pc.c                  | 18 ++++++++++++------
>>   hw/i386/pc_piix.c             |  9 +++++++--
>>   hw/i386/pc_q35.c              |  8 +++++---
>>   hw/isa/lpc_ich9.c             |  9 +++++++++
>>   hw/isa/piix.c                 |  9 +++++++++
>>   hw/isa/Kconfig                |  2 ++
>>   9 files changed, 52 insertions(+), 12 deletions(-)
[...] 
>> diff --git a/hw/isa/Kconfig b/hw/isa/Kconfig
>> index efdf43e92c..f42a087c07 100644
>> --- a/hw/isa/Kconfig
>> +++ b/hw/isa/Kconfig
>> @@ -47,6 +47,7 @@ config PIIX
>>       select IDE_PIIX
>>       select ISA_BUS
>>       select MC146818RTC
>> +    select PORT92
>>       select USB_UHCI
>>     config PORT92
>> @@ -78,3 +79,4 @@ config LPC_ICH9
>>       select ISA_BUS
>>       select ACPI_ICH9
>>       select MC146818RTC
>> +    select PORT92
>
> I had a look at this (and did a bit of revision around 8042 and A20), and I 
> am starting to wonder if the PORT92 device isn't something that belongs to 
> the southbridge, but more specifically to the superio chip?
>
> A couple of thoughts as to why I came to this conclusion: firstly the superio 
> chip is generally considered to be a single integrated implementation of 
> legacy IO devices, so this feels like a natural home for the PORT92 device. 
> Secondly the value of the "has-port92" property is controlled by 
> pcms->i8042_enabled, and this value is already passed into functions such as 
> pc_superio_init() for example.

One more argument for that may be that this A20 gate was originally 
controlled by the keyboard controller and that's part of the superio. The 
VIA southbridge docs even mention something about the KBC and this port92 
register controlling the A20 gate together. So to implement that correctly 
it may need to be in the same device. However this may be specific to the 
VIA chip so not sure if this implementation can be shared by all the 
southbridge devices.

> I think this would also help reduce the changes required for the individual 
> machines, however the devil is always in the details particularly when 
> migration is involved.

One thing that bothers me very much about this port92 device is that we 
have about 100 lines of code of which the actual functionality is just a 
qemu_irq and an uint8_t controlling it and storing the register value. 
That shouldn't be more than 10 lines or maybe 20 with migration support, 
it's only 100 lines because it's wrapped in a QDev. Bernhard wanted to 
move it out from the PC machines to clean those up but as a result this 
mess is just pushed down in the southbridge (or into superio as you 
propose). It could easily be implemented in the southbridge or superio by 
just adding the qemu_irq and the register value and maybe export it as a 
property so the machine can set it for migration compatibility and then we 
don't need a separate QDev wrapper around it as the superio or southbridge 
is already a QDev and has the needed stuff to store this.

But one could also argue that while these southbridges implement this 
functionality, it's only used on the PC machines so as we already have it 
modelled in a separate object we could just let the PC machines 
instantiate it and leave it as it is and don't add this to other machines 
where it's not needed.

(I like Mark's proposal a bit better only because that leaves the 
southbridge untouched and changes the superio instead that I care less 
about but the issue is the same there.)

Regards,
BALATON Zoltan

Re: [PATCH 3/5] hw/isa: Embed TYPE_PORT92 in south bridges used in PC machines

[Bernhard Beschow]

Am 21. Februar 2024 11:53:21 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>On 18/02/2024 13:16, Bernhard Beschow wrote:
>
>> Port 92 is an integral part of the PIIX and ICH south bridges, so instantiate it
>> there. The isapc machine now needs to instantiate it explicitly, analoguous to
>> the RTC.
>> 
>> Note that due to migration compatibility, port92 is optional in the south
>> bridges. It is always instantiated the isapc machine for simplicity.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>   include/hw/i386/pc.h          |  2 +-
>>   include/hw/southbridge/ich9.h |  4 ++++
>>   include/hw/southbridge/piix.h |  3 +++
>>   hw/i386/pc.c                  | 18 ++++++++++++------
>>   hw/i386/pc_piix.c             |  9 +++++++--
>>   hw/i386/pc_q35.c              |  8 +++++---
>>   hw/isa/lpc_ich9.c             |  9 +++++++++
>>   hw/isa/piix.c                 |  9 +++++++++
>>   hw/isa/Kconfig                |  2 ++
>>   9 files changed, 52 insertions(+), 12 deletions(-)
>> 
>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>> index b2987209b1..a9ff1f5ab3 100644
>> --- a/include/hw/i386/pc.h
>> +++ b/include/hw/i386/pc.h
>> @@ -178,7 +178,7 @@ uint64_t pc_pci_hole64_start(void);
>>   DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus);
>>   void pc_basic_device_init(struct PCMachineState *pcms,
>>                             ISABus *isa_bus, qemu_irq *gsi,
>> -                          ISADevice *rtc_state,
>> +                          ISADevice *rtc_state, ISADevice *port92,
>>                             bool create_fdctrl,
>>                             uint32_t hpet_irqs);
>>   void pc_cmos_init(PCMachineState *pcms,
>> diff --git a/include/hw/southbridge/ich9.h b/include/hw/southbridge/ich9.h
>> index fd01649d04..d70a94f5e7 100644
>> --- a/include/hw/southbridge/ich9.h
>> +++ b/include/hw/southbridge/ich9.h
>> @@ -3,6 +3,7 @@
>>     #include "hw/isa/apm.h"
>>   #include "hw/acpi/ich9.h"
>> +#include "hw/isa/port92.h"
>>   #include "hw/intc/ioapic.h"
>>   #include "hw/pci/pci.h"
>>   #include "hw/pci/pci_device.h"
>> @@ -32,6 +33,7 @@ struct ICH9LPCState {
>>       uint8_t irr[PCI_SLOT_MAX][PCI_NUM_PINS];
>>         MC146818RtcState rtc;
>> +    Port92State port92;
>>       APMState apm;
>>       ICH9LPCPMRegs pm;
>>       uint32_t sci_level; /* track sci level */
>> @@ -54,6 +56,8 @@ struct ICH9LPCState {
>>       uint8_t rst_cnt;
>>       MemoryRegion rst_cnt_mem;
>>   +    bool has_port92;
>> +
>>       /* SMI feature negotiation via fw_cfg */
>>       uint64_t smi_host_features;       /* guest-invisible, host endian */
>>       uint8_t smi_host_features_le[8];  /* guest-visible, read-only, little
>> diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
>> index 86709ba2e4..35058529d1 100644
>> --- a/include/hw/southbridge/piix.h
>> +++ b/include/hw/southbridge/piix.h
>> @@ -15,6 +15,7 @@
>>   #include "hw/pci/pci_device.h"
>>   #include "hw/acpi/piix4.h"
>>   #include "hw/ide/pci.h"
>> +#include "hw/isa/port92.h"
>>   #include "hw/rtc/mc146818rtc.h"
>>   #include "hw/usb/hcd-uhci.h"
>>   @@ -56,6 +57,7 @@ struct PIIXState {
>>       int32_t pci_irq_levels_vmstate[PIIX_NUM_PIRQS];
>>         MC146818RtcState rtc;
>> +    Port92State port92;
>>       PCIIDEState ide;
>>       UHCIState uhci;
>>       PIIX4PMState pm;
>> @@ -71,6 +73,7 @@ struct PIIXState {
>>       bool has_acpi;
>>       bool has_pic;
>>       bool has_pit;
>> +    bool has_port92;
>>       bool has_usb;
>>       bool smm_enabled;
>>   };
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 0b11d4576e..8b601ea6cf 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -1160,7 +1160,7 @@ static void pc_superio_init(ISABus *isa_bus, bool create_fdctrl,
>>       int i;
>>       DriveInfo *fd[MAX_FD];
>>       qemu_irq *a20_line;
>> -    ISADevice *fdc, *i8042, *port92, *vmmouse;
>> +    ISADevice *fdc, *i8042, *vmmouse;
>>         serial_hds_isa_init(isa_bus, 0, MAX_ISA_SERIAL_PORTS);
>>       parallel_hds_isa_init(isa_bus, MAX_PARALLEL_PORTS);
>> @@ -1193,18 +1193,15 @@ static void pc_superio_init(ISABus *isa_bus, bool create_fdctrl,
>>                                    &error_abort);
>>           isa_realize_and_unref(vmmouse, isa_bus, &error_fatal);
>>       }
>> -    port92 = isa_create_simple(isa_bus, TYPE_PORT92);
>>   -    a20_line = qemu_allocate_irqs(handle_a20_line_change, first_cpu, 2);
>> +    a20_line = qemu_allocate_irqs(handle_a20_line_change, first_cpu, 1);
>>       i8042_setup_a20_line(i8042, a20_line[0]);
>> -    qdev_connect_gpio_out_named(DEVICE(port92),
>> -                                PORT92_A20_LINE, 0, a20_line[1]);
>>       g_free(a20_line);
>>   }
>>     void pc_basic_device_init(struct PCMachineState *pcms,
>>                             ISABus *isa_bus, qemu_irq *gsi,
>> -                          ISADevice *rtc_state,
>> +                          ISADevice *rtc_state, ISADevice *port92,
>>                             bool create_fdctrl,
>>                             uint32_t hpet_irqs)
>>   {
>> @@ -1296,6 +1293,15 @@ void pc_basic_device_init(struct PCMachineState *pcms,
>>       /* Super I/O */
>>       pc_superio_init(isa_bus, create_fdctrl, pcms->i8042_enabled,
>>                       pcms->vmport != ON_OFF_AUTO_ON);
>> +
>> +    if (port92) {
>> +        qemu_irq *a20_line;
>> +
>> +        a20_line = qemu_allocate_irqs(handle_a20_line_change, first_cpu, 1);
>> +        qdev_connect_gpio_out_named(DEVICE(port92),
>> +                                    PORT92_A20_LINE, 0, a20_line[0]);
>> +        g_free(a20_line);
>> +    }
>>   }
>>     void pc_nic_init(PCMachineClass *pcmc, ISABus *isa_bus, PCIBus *pci_bus)
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index 999b7b806c..dfdfd36551 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -115,7 +115,7 @@ static void pc_init1(MachineState *machine,
>>       qemu_irq smi_irq;
>>       GSIState *gsi_state;
>>       BusState *idebus[MAX_IDE_BUS];
>> -    ISADevice *rtc_state;
>> +    ISADevice *rtc_state, *port92;
>>       MemoryRegion *ram_memory;
>>       MemoryRegion *pci_memory = NULL;
>>       MemoryRegion *rom_memory = system_memory;
>> @@ -269,6 +269,8 @@ static void pc_init1(MachineState *machine,
>>                                    &error_abort);
>>           object_property_set_bool(OBJECT(pci_dev), "has-pit", false,
>>                                    &error_abort);
>> +        object_property_set_bool(OBJECT(pci_dev), "has-port92",
>> +                                 pcms->i8042_enabled, &error_abort);
>>           qdev_prop_set_uint32(DEVICE(pci_dev), "smb_io_base", 0xb100);
>>           object_property_set_bool(OBJECT(pci_dev), "smm-enabled",
>>                                    x86_machine_is_smm_enabled(x86ms),
>> @@ -296,6 +298,8 @@ static void pc_init1(MachineState *machine,
>>           isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(pci_dev), "isa.0"));
>>           rtc_state = ISA_DEVICE(object_resolve_path_component(OBJECT(pci_dev),
>>                                                                "rtc"));
>> +        port92 = ISA_DEVICE(object_resolve_path_component(OBJECT(pci_dev),
>> +                                                          "port92"));
>>           piix4_pm = object_resolve_path_component(OBJECT(pci_dev), "pm");
>>           dev = DEVICE(object_resolve_path_component(OBJECT(pci_dev), "ide"));
>>           pci_ide_create_devs(PCI_DEVICE(dev));
>> @@ -310,6 +314,7 @@ static void pc_init1(MachineState *machine,
>>           qdev_prop_set_int32(DEVICE(rtc_state), "base_year", 2000);
>>           isa_realize_and_unref(rtc_state, isa_bus, &error_fatal);
>>   +        port92 = isa_create_simple(isa_bus, TYPE_PORT92);
>>           i8257_dma_init(OBJECT(machine), isa_bus, 0);
>>           pcms->hpet_enabled = false;
>>           idebus[0] = NULL;
>> @@ -336,7 +341,7 @@ static void pc_init1(MachineState *machine,
>>       }
>>         /* init basic PC hardware */
>> -    pc_basic_device_init(pcms, isa_bus, x86ms->gsi, rtc_state, true,
>> +    pc_basic_device_init(pcms, isa_bus, x86ms->gsi, rtc_state, port92, true,
>>                            0x4);
>>         pc_nic_init(pcmc, isa_bus, pci_bus);
>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>> index d346fa3b1d..26bb1c2cb9 100644
>> --- a/hw/i386/pc_q35.c
>> +++ b/hw/i386/pc_q35.c
>> @@ -127,7 +127,7 @@ static void pc_q35_init(MachineState *machine)
>>       PCIDevice *lpc;
>>       DeviceState *lpc_dev;
>>       BusState *idebus[MAX_SATA_PORTS];
>> -    ISADevice *rtc_state;
>> +    ISADevice *rtc_state, *port92;
>>       MemoryRegion *system_memory = get_system_memory();
>>       MemoryRegion *system_io = get_system_io();
>>       MemoryRegion *pci_memory = g_new(MemoryRegion, 1);
>> @@ -238,6 +238,7 @@ static void pc_q35_init(MachineState *machine)
>>       lpc = pci_new_multifunction(PCI_DEVFN(ICH9_LPC_DEV, ICH9_LPC_FUNC),
>>                                   TYPE_ICH9_LPC_DEVICE);
>>       lpc_dev = DEVICE(lpc);
>> +    qdev_prop_set_bit(lpc_dev, "has-port92", pcms->i8042_enabled);
>>       qdev_prop_set_bit(lpc_dev, "smm-enabled",
>>                         x86_machine_is_smm_enabled(x86ms));
>>       pci_realize_and_unref(lpc, host_bus, &error_fatal);
>> @@ -246,6 +247,7 @@ static void pc_q35_init(MachineState *machine)
>>       }
>>         rtc_state = ISA_DEVICE(object_resolve_path_component(OBJECT(lpc), "rtc"));
>> +    port92 = ISA_DEVICE(object_resolve_path_component(OBJECT(lpc), "port92"));
>>         object_property_add_link(OBJECT(machine), PC_MACHINE_ACPI_DEVICE_PROP,
>>                                TYPE_HOTPLUG_HANDLER,
>> @@ -287,8 +289,8 @@ static void pc_q35_init(MachineState *machine)
>>       }
>>         /* init basic PC hardware */
>> -    pc_basic_device_init(pcms, isa_bus, x86ms->gsi, rtc_state, !mc->no_floppy,
>> -                         0xff0104);
>> +    pc_basic_device_init(pcms, isa_bus, x86ms->gsi, rtc_state, port92,
>> +                         !mc->no_floppy, 0xff0104);
>>         if (pcms->sata_enabled) {
>>           PCIDevice *pdev;
>> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
>> index 70c6e8a093..3be5bc01b1 100644
>> --- a/hw/isa/lpc_ich9.c
>> +++ b/hw/isa/lpc_ich9.c
>> @@ -749,6 +749,14 @@ static void ich9_lpc_realize(PCIDevice *d, Error **errp)
>>       irq = object_property_get_uint(OBJECT(&lpc->rtc), "irq", &error_fatal);
>>       isa_connect_gpio_out(ISA_DEVICE(&lpc->rtc), 0, irq);
>>   +    if (lpc->has_port92) {
>> +        object_initialize_child(OBJECT(lpc), "port92", &lpc->port92,
>> +                                TYPE_PORT92);
>> +        if (!qdev_realize(DEVICE(&lpc->port92), BUS(isa_bus), errp)) {
>> +            return;
>> +        }
>> +    }
>> +
>>       pci_bus_irqs(pci_bus, ich9_lpc_set_irq, d, ICH9_LPC_NB_PIRQS);
>>       pci_bus_map_irqs(pci_bus, ich9_lpc_map_irq);
>>       pci_bus_set_route_irq_fn(pci_bus, ich9_route_intx_pin_to_irq);
>> @@ -821,6 +829,7 @@ static Property ich9_lpc_properties[] = {
>>       DEFINE_PROP_BOOL("noreboot", ICH9LPCState, pin_strap.spkr_hi, false),
>>       DEFINE_PROP_BOOL("smm-compat", ICH9LPCState, pm.smm_compat, false),
>>       DEFINE_PROP_BOOL("smm-enabled", ICH9LPCState, pm.smm_enabled, false),
>> +    DEFINE_PROP_BOOL("has-port92", ICH9LPCState, has_port92, true),
>>       DEFINE_PROP_BIT64("x-smi-broadcast", ICH9LPCState, smi_host_features,
>>                         ICH9_LPC_SMI_F_BROADCAST_BIT, true),
>>       DEFINE_PROP_BIT64("x-smi-cpu-hotplug", ICH9LPCState, smi_host_features,
>> diff --git a/hw/isa/piix.c b/hw/isa/piix.c
>> index 2d30711b17..4c12855461 100644
>> --- a/hw/isa/piix.c
>> +++ b/hw/isa/piix.c
>> @@ -346,6 +346,14 @@ static void pci_piix_realize(PCIDevice *dev, const char *uhci_type,
>>       irq = object_property_get_uint(OBJECT(&d->rtc), "irq", &error_fatal);
>>       isa_connect_gpio_out(ISA_DEVICE(&d->rtc), 0, irq);
>>   +    /* Port 92 */
>> +    if (d->has_port92) {
>> +        object_initialize_child(OBJECT(d), "port92", &d->port92, TYPE_PORT92);
>> +        if (!qdev_realize(DEVICE(&d->port92), BUS(isa_bus), errp)) {
>> +            return;
>> +        }
>> +    }
>> +
>>       /* IDE */
>>       qdev_prop_set_int32(DEVICE(&d->ide), "addr", dev->devfn + 1);
>>       if (!qdev_realize(DEVICE(&d->ide), BUS(pci_bus), errp)) {
>> @@ -413,6 +421,7 @@ static Property pci_piix_props[] = {
>>       DEFINE_PROP_BOOL("has-acpi", PIIXState, has_acpi, true),
>>       DEFINE_PROP_BOOL("has-pic", PIIXState, has_pic, true),
>>       DEFINE_PROP_BOOL("has-pit", PIIXState, has_pit, true),
>> +    DEFINE_PROP_BOOL("has-port92", PIIXState, has_port92, true),
>>       DEFINE_PROP_BOOL("has-usb", PIIXState, has_usb, true),
>>       DEFINE_PROP_BOOL("smm-enabled", PIIXState, smm_enabled, false),
>>       DEFINE_PROP_END_OF_LIST(),
>> diff --git a/hw/isa/Kconfig b/hw/isa/Kconfig
>> index efdf43e92c..f42a087c07 100644
>> --- a/hw/isa/Kconfig
>> +++ b/hw/isa/Kconfig
>> @@ -47,6 +47,7 @@ config PIIX
>>       select IDE_PIIX
>>       select ISA_BUS
>>       select MC146818RTC
>> +    select PORT92
>>       select USB_UHCI
>>     config PORT92
>> @@ -78,3 +79,4 @@ config LPC_ICH9
>>       select ISA_BUS
>>       select ACPI_ICH9
>>       select MC146818RTC
>> +    select PORT92
>
>I had a look at this (and did a bit of revision around 8042 and A20), and I am starting to wonder if the PORT92 device isn't something that belongs to the southbridge, but more specifically to the superio chip?

If there is agreement to model real hardware in QEMU, then I think that port 92 belongs into any device model where the hardware has one. All our PC-like southbridges (PIIX, ICH, VIA) have port 92. Many FDC37xxxx including the FDC37M81x as used in the Malta board have one, too -- where it must first be enabled.

>
>A couple of thoughts as to why I came to this conclusion: firstly the superio chip is generally considered to be a single integrated implementation of legacy IO devices, so this feels like a natural home for the PORT92 device.

> Secondly the value of the "has-port92" property is controlled by pcms->i8042_enabled, and this value is already passed into functions such as pc_superio_init() for example.

Rhight. There, it also controls the presence of port 92. If we move port 92 into the southbridges, we have to respect this command line switch there to preserve backward compatibility.

I wonder what `-M i8042` is supposed to do. If it is for modeling a stripped-down x86 system, why not use the microvm instead? How is it possible to omit an essential piece of hardware needed to boot x86 systems? Don't we need at least either one (i8042 or port 92)?

>
>I think this would also help reduce the changes required for the individual machines, however the devil is always in the details particularly when migration is involved.

As stated above, this series is more about modeling real hardware, in the hope that this will lend itself for configuration-driven machine creation. It is also about identifying obstacles towards this goal. Does it make sense to deprecate some machine-specific options such as i8042?

Best regards,
Bernhard

>
>
>ATB,
>
>Mark.
>

Re: [PATCH 3/5] hw/isa: Embed TYPE_PORT92 in south bridges used in PC machines

[BALATON Zoltan]

On Tue, 27 Feb 2024, Bernhard Beschow wrote:
> Am 21. Februar 2024 11:53:21 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>> On 18/02/2024 13:16, Bernhard Beschow wrote:
>>> Port 92 is an integral part of the PIIX and ICH south bridges, so instantiate it
>>> there. The isapc machine now needs to instantiate it explicitly, analoguous to
>>> the RTC.
>>>
>>> Note that due to migration compatibility, port92 is optional in the south
>>> bridges. It is always instantiated the isapc machine for simplicity.
>>>
>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>> ---
>>>   include/hw/i386/pc.h          |  2 +-
>>>   include/hw/southbridge/ich9.h |  4 ++++
>>>   include/hw/southbridge/piix.h |  3 +++
>>>   hw/i386/pc.c                  | 18 ++++++++++++------
>>>   hw/i386/pc_piix.c             |  9 +++++++--
>>>   hw/i386/pc_q35.c              |  8 +++++---
>>>   hw/isa/lpc_ich9.c             |  9 +++++++++
>>>   hw/isa/piix.c                 |  9 +++++++++
>>>   hw/isa/Kconfig                |  2 ++
>>>   9 files changed, 52 insertions(+), 12 deletions(-)
>>
>> I had a look at this (and did a bit of revision around 8042 and A20), 
>> and I am starting to wonder if the PORT92 device isn't something that 
>> belongs to the southbridge, but more specifically to the superio chip?
>
> If there is agreement to model real hardware in QEMU, then I think that

I think there's no such agreement and QEMU is more lax about it both for 
historical reasons and to simplify machine models. Indeed, QEMU sometimes 
models non-existing machines (e.g. the mac99 or virt boards) that don't 
correspond to real hardware but allow guest OSes to boot. Even when 
modelllng real hardware it's ofren modelled just enough for guests to work 
and unused details are omitted for simplicity. It is recommended to follow 
what real hardware does when modelling real hardware but not always 
required. Although it might help both with verifying a device model and to 
compose machines with these models to try to follow the real hardware.

> port 92 belongs into any device model where the hardware has one. All 
> our PC-like southbridges (PIIX, ICH, VIA) have port 92. Many FDC37xxxx 
> including the FDC37M81x as used in the Malta board have one, too -- 
> where it must first be enabled.

So port92 is not a real hardware but a QEMU abstraction or model of some 
functionality found in some machines. Real chips probably implement this 
in different ways so we could either model this in these chips 
independently the same way as real hardware does or use the abstracted 
model anywhere in our machine model. Since this does not exist in real 
hardware as this abstract model it also does not belong anywhere so we are 
free to put it where it's most convenient or simple to do.

>> A couple of thoughts as to why I came to this conclusion: firstly the 
>> superio chip is generally considered to be a single integrated 
>> implementation of legacy IO devices, so this feels like a natural home 
>> for the PORT92 device.
>
>> Secondly the value of the "has-port92" property is controlled by 
>> pcms->i8042_enabled, and this value is already passed into functions 
>> such as pc_superio_init() for example.
>
> Rhight. There, it also controls the presence of port 92. If we move port 
> 92 into the southbridges, we have to respect this command line switch 
> there to preserve backward compatibility.
>
> I wonder what `-M i8042` is supposed to do. If it is for modeling a 
> stripped-down x86 system, why not use the microvm instead? How is it 
> possible to omit an essential piece of hardware needed to boot x86 
> systems? Don't we need at least either one (i8042 or port 92)?

Try git log -p 4ccd5fe22fe (found it via git blame and see what added that 
property).

>> I think this would also help reduce the changes required for the 
>> individual machines, however the devil is always in the details 
>> particularly when migration is involved.
>
> As stated above, this series is more about modeling real hardware, in 
> the hope that this will lend itself for configuration-driven machine 
> creation. It is also about identifying obstacles towards this goal. Does 
> it make sense to deprecate some machine-specific options such as i8042?

Only if you want to break downsteam users of those options but maybe they 
won't be happy about that.

Regards,
BALATON Zoltan

Re: [PATCH 3/5] hw/isa: Embed TYPE_PORT92 in south bridges used in PC machines

[Bernhard Beschow]

Am 27. Februar 2024 21:54:19 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Tue, 27 Feb 2024, Bernhard Beschow wrote:
>> Am 21. Februar 2024 11:53:21 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>>> On 18/02/2024 13:16, Bernhard Beschow wrote:
>>>> Port 92 is an integral part of the PIIX and ICH south bridges, so instantiate it
>>>> there. The isapc machine now needs to instantiate it explicitly, analoguous to
>>>> the RTC.
>>>> 
>>>> Note that due to migration compatibility, port92 is optional in the south
>>>> bridges. It is always instantiated the isapc machine for simplicity.
>>>> 
>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>> ---
>>>>   include/hw/i386/pc.h          |  2 +-
>>>>   include/hw/southbridge/ich9.h |  4 ++++
>>>>   include/hw/southbridge/piix.h |  3 +++
>>>>   hw/i386/pc.c                  | 18 ++++++++++++------
>>>>   hw/i386/pc_piix.c             |  9 +++++++--
>>>>   hw/i386/pc_q35.c              |  8 +++++---
>>>>   hw/isa/lpc_ich9.c             |  9 +++++++++
>>>>   hw/isa/piix.c                 |  9 +++++++++
>>>>   hw/isa/Kconfig                |  2 ++
>>>>   9 files changed, 52 insertions(+), 12 deletions(-)
>>> 
>>> I had a look at this (and did a bit of revision around 8042 and A20), and I am starting to wonder if the PORT92 device isn't something that belongs to the southbridge, but more specifically to the superio chip?
>> 
>> If there is agreement to model real hardware in QEMU, then I think that
>
>I think there's no such agreement and QEMU is more lax about it both for historical reasons and to simplify machine models. Indeed, QEMU sometimes models non-existing machines (e.g. the mac99 or virt boards) that don't correspond to real hardware but allow guest OSes to boot. Even when modelllng real hardware it's ofren modelled just enough for guests to work and unused details are omitted for simplicity. It is recommended to follow what real hardware does when modelling real hardware but not always required. Although it might help both with verifying a device model and to compose machines with these models to try to follow the real hardware.

Composing real machines and verifying device models is exactly what I'm after. I'm aware that QEMU provides virt machines such as the microvm, and from the context I didn't refer to these.

>
>> port 92 belongs into any device model where the hardware has one. All our PC-like southbridges (PIIX, ICH, VIA) have port 92. Many FDC37xxxx including the FDC37M81x as used in the Malta board have one, too -- where it must first be enabled.
>
>So port92 is not a real hardware but a QEMU abstraction or model of some functionality found in some machines. Real chips probably implement this in different ways so we could either model this in these chips independently the same way as real hardware does or use the abstracted model anywhere in our machine model. Since this does not exist in real hardware as this abstract model it also does not belong anywhere so we are free to put it where it's most convenient or simple to do.

As mentioned already, port 92 is an integral part of PIIX, ICH, and VIA southbridges. That's why I want to move it there. My goal is to create different PC machines in a data-driven manner which model real boards. I want to see how low-level guests interact with the hardware, including e.g. how they set up the memory map.

>
>>> A couple of thoughts as to why I came to this conclusion: firstly the superio chip is generally considered to be a single integrated implementation of legacy IO devices, so this feels like a natural home for the PORT92 device.
>> 
>>> Secondly the value of the "has-port92" property is controlled by pcms->i8042_enabled, and this value is already passed into functions such as pc_superio_init() for example.
>> 
>> Rhight. There, it also controls the presence of port 92. If we move port 92 into the southbridges, we have to respect this command line switch there to preserve backward compatibility.
>> 
>> I wonder what `-M i8042` is supposed to do. If it is for modeling a stripped-down x86 system, why not use the microvm instead? How is it possible to omit an essential piece of hardware needed to boot x86 systems? Don't we need at least either one (i8042 or port 92)?
>
>Try git log -p 4ccd5fe22fe (found it via git blame and see what added that property).

Alright, the intention was to omit the PS/2 controller in favor of USB. That doesn't mean that port 92 needs to be affected. I see an opportunity here to reduce the scope of the i8042 option which may help with data-driven machine creation in the future.

>
>>> I think this would also help reduce the changes required for the individual machines, however the devil is always in the details particularly when migration is involved.
>> 
>> As stated above, this series is more about modeling real hardware, in the hope that this will lend itself for configuration-driven machine creation. It is also about identifying obstacles towards this goal. Does it make sense to deprecate some machine-specific options such as i8042?
>
>Only if you want to break downsteam users of those options but maybe they won't be happy about that.

I don't want to break downstream users of course.

I want device models true to the original. I think that this series is a step in this direction, hence I'd like feedback for achieving that. What's necessary to get this series in?

Best regards,
Bernhard

>
>Regards,
>BALATON Zoltan

Re: [PATCH 3/5] hw/isa: Embed TYPE_PORT92 in south bridges used in PC machines

[BALATON Zoltan]

On Tue, 27 Feb 2024, Bernhard Beschow wrote:
> Am 27. Februar 2024 21:54:19 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>> On Tue, 27 Feb 2024, Bernhard Beschow wrote:
>>> Am 21. Februar 2024 11:53:21 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>>>> On 18/02/2024 13:16, Bernhard Beschow wrote:
>>>>> Port 92 is an integral part of the PIIX and ICH south bridges, so instantiate it
>>>>> there. The isapc machine now needs to instantiate it explicitly, analoguous to
>>>>> the RTC.
>>>>>
>>>>> Note that due to migration compatibility, port92 is optional in the south
>>>>> bridges. It is always instantiated the isapc machine for simplicity.
>>>>>
>>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>>> ---
>>>>>   include/hw/i386/pc.h          |  2 +-
>>>>>   include/hw/southbridge/ich9.h |  4 ++++
>>>>>   include/hw/southbridge/piix.h |  3 +++
>>>>>   hw/i386/pc.c                  | 18 ++++++++++++------
>>>>>   hw/i386/pc_piix.c             |  9 +++++++--
>>>>>   hw/i386/pc_q35.c              |  8 +++++---
>>>>>   hw/isa/lpc_ich9.c             |  9 +++++++++
>>>>>   hw/isa/piix.c                 |  9 +++++++++
>>>>>   hw/isa/Kconfig                |  2 ++
>>>>>   9 files changed, 52 insertions(+), 12 deletions(-)
>>>>
>>>> I had a look at this (and did a bit of revision around 8042 and A20), and I am starting to wonder if the PORT92 device isn't something that belongs to the southbridge, but more specifically to the superio chip?
>>>
>>> If there is agreement to model real hardware in QEMU, then I think that
>>
>> I think there's no such agreement and QEMU is more lax about it both for historical reasons and to simplify machine models. Indeed, QEMU sometimes models non-existing machines (e.g. the mac99 or virt boards) that don't correspond to real hardware but allow guest OSes to boot. Even when modelllng real hardware it's ofren modelled just enough for guests to work and unused details are omitted for simplicity. It is recommended to follow what real hardware does when modelling real hardware but not always required. Although it might help both with verifying a device model and to compose machines with these models to try to follow the real hardware.
>
> Composing real machines and verifying device models is exactly what I'm 
> after. I'm aware that QEMU provides virt machines such as the microvm, 
> and from the context I didn't refer to these.

Even without pure virt machines currently a lot of QEMU machines don't 
exactly model real hardware. They may roughly follow real hardware but not 
exactly such as mac99 is a non-existent Mac and the pc machines also use 
some parts that don't exist in real life such as PIIX4-PIIX3 hybrid you've 
been working on resolving. Some of these however are restricted by 
backward compatibilty requirements. But you probably aware of all of that 
but this means the argument that real hardware should be followed is not 
enough. At least it should not break backward compatibility too much and 
that's more important than exactly modelling real machine. Also having a 
simple model may be more important than modeling every detail even when 
not used just to follow real hardware.

>>> port 92 belongs into any device model where the hardware has one. All our PC-like southbridges (PIIX, ICH, VIA) have port 92. Many FDC37xxxx including the FDC37M81x as used in the Malta board have one, too -- where it must first be enabled.
>>
>> So port92 is not a real hardware but a QEMU abstraction or model of some functionality found in some machines. Real chips probably implement this in different ways so we could either model this in these chips independently the same way as real hardware does or use the abstracted model anywhere in our machine model. Since this does not exist in real hardware as this abstract model it also does not belong anywhere so we are free to put it where it's most convenient or simple to do.
>
> As mentioned already, port 92 is an integral part of PIIX, ICH, and VIA 
> southbridges.

Mark argued that more specifically it's part of the superio within those 
couthbridges. That makes sense, considering this port92 is related to 
functionality that was in the keyboard contorller before which is part of 
the superio. I don't know PC hardware too well but reading about this fast 
gate A20 feature looks like original PC and XT had only a 1 MB address 
space but addresses above 1 MB wrapped to 0 and some software depended on 
that. Then AT added more memory but then it needed a way to control if 
addresses above 1 MB would wrap or access high memory. This was done with 
some free part of the keyboard controller but that was too slow so an 
alternative fast way was added with this port92 device. But then the old 
keyboard controller and this port92 stuff are interacting so may need to 
consider both. Apart from that all of this is not relevant to other 
machines that don't use this functionality.

QEMU decided to model it as a separate QOM object that is now instantiated 
by the machines that use it. This is not real hardware but a QEMU 
implementation detail. What's wrong with that? It seems you just want to 
simplify the pc machine creation and push this object that does not 
correspond to some real hardware somewhere else. But this belongs nowhere 
as it does not model a real hardware.

> That's why I want to move it there. My goal is to create 
> different PC machines in a data-driven manner which model real boards. I 
> want to see how low-level guests interact with the hardware, including 
> e.g. how they set up the memory map.

Then I think the port92 as a QOM object should be gone completely and 
implemented separately in south bridges beacause in real machine there's 
no such port 92 thing, only a south briege so if you need to create port92 
in a data driver machine description then that's not modeling real 
hardware but reflects QEMU implementation of it. However the QOM object 
could be retained as an internal object whithn the device model if that 
makes it model simpler or helps code reuse. I'm not convinced about that 
but I also don't care enough to mind if it will still be there somewhere 
outside of the VIA model. What I care about is that I don't want 
unneccesary things in the VIA model and want to keep it simple.

>>>> A couple of thoughts as to why I came to this conclusion: firstly the superio chip is generally considered to be a single integrated implementation of legacy IO devices, so this feels like a natural home for the PORT92 device.
>>>
>>>> Secondly the value of the "has-port92" property is controlled by pcms->i8042_enabled, and this value is already passed into functions such as pc_superio_init() for example.
>>>
>>> Rhight. There, it also controls the presence of port 92. If we move port 92 into the southbridges, we have to respect this command line switch there to preserve backward compatibility.
>>>
>>> I wonder what `-M i8042` is supposed to do. If it is for modeling a stripped-down x86 system, why not use the microvm instead? How is it possible to omit an essential piece of hardware needed to boot x86 systems? Don't we need at least either one (i8042 or port 92)?
>>
>> Try git log -p 4ccd5fe22fe (found it via git blame and see what added that property).
>
> Alright, the intention was to omit the PS/2 controller in favor of USB. 
> That doesn't mean that port 92 needs to be affected. I see an 
> opportunity here to reduce the scope of the i8042 option which may help 
> with data-driven machine creation in the future.
>
>>
>>>> I think this would also help reduce the changes required for the individual machines, however the devil is always in the details particularly when migration is involved.
>>>
>>> As stated above, this series is more about modeling real hardware, in the hope that this will lend itself for configuration-driven machine creation. It is also about identifying obstacles towards this goal. Does it make sense to deprecate some machine-specific options such as i8042?
>>
>> Only if you want to break downsteam users of those options but maybe they won't be happy about that.
>
> I don't want to break downstream users of course.
>
> I want device models true to the original. I think that this series is a 
> step in this direction, hence I'd like feedback for achieving that. 
> What's necessary to get this series in?

I think you got feedback. My proposal was to remove port92 object and 
implemnt it in the south bridges instead independently but you did not 
like that because wanted to keep it at a central place and maybe also 
migration compatibility is simpler if it's kept (although I think that can 
be solved by some compatibility function in the machine the same way as 
optional sections or changes in migration format are handled on the 
receving side where it could set a property on some object exporting the 
state of this register).

Then Mark proposed to move it down into superio which is used by all these 
southbridges and it also contains the keyboard controller which this 
might interact with so that seems like a good place to put it (either 
instantiating the port92 object there or implementing it directly). As 
long as this keeps it out from VIA and does not need too much QOM wizardry 
in the VIA model (more than maybe forwarding a property for this), I'm 
happy with that so I think you could try that unless you see some problem 
with this approach.

Now if you also want to change the keyboard controller too, maybe port92 
handling could also be implemented there instead of the superio as an 
optional part that machines that need it can enable. But all this will 
probably result in that either the pc machine will need to dig down to 
find this part or have a chain of property forwarding through superio and 
south bridge models that would complicate those models which may be uglier 
than what we have now to just instantiate the port92 object in the pc 
machine and leave the other models that don't need it without this 
complication.

Regards,
BALATON Zoltan

Re: [PATCH 3/5] hw/isa: Embed TYPE_PORT92 in south bridges used in PC machines

[BALATON Zoltan]

On Wed, 28 Feb 2024, BALATON Zoltan wrote:
> On Tue, 27 Feb 2024, Bernhard Beschow wrote:
>> Am 27. Februar 2024 21:54:19 UTC schrieb BALATON Zoltan 
>> <balaton@eik.bme.hu>:
>>> On Tue, 27 Feb 2024, Bernhard Beschow wrote:
>>>> Am 21. Februar 2024 11:53:21 UTC schrieb Mark Cave-Ayland 
>>>> <mark.cave-ayland@ilande.co.uk>:
>>>>> On 18/02/2024 13:16, Bernhard Beschow wrote:
>>>>>> Port 92 is an integral part of the PIIX and ICH south bridges, so 
>>>>>> instantiate it
>>>>>> there. The isapc machine now needs to instantiate it explicitly, 
>>>>>> analoguous to
>>>>>> the RTC.
>>>>>> 
>>>>>> Note that due to migration compatibility, port92 is optional in the 
>>>>>> south
>>>>>> bridges. It is always instantiated the isapc machine for simplicity.
>>>>>> 
>>>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>>>> ---
>>>>>>   include/hw/i386/pc.h          |  2 +-
>>>>>>   include/hw/southbridge/ich9.h |  4 ++++
>>>>>>   include/hw/southbridge/piix.h |  3 +++
>>>>>>   hw/i386/pc.c                  | 18 ++++++++++++------
>>>>>>   hw/i386/pc_piix.c             |  9 +++++++--
>>>>>>   hw/i386/pc_q35.c              |  8 +++++---
>>>>>>   hw/isa/lpc_ich9.c             |  9 +++++++++
>>>>>>   hw/isa/piix.c                 |  9 +++++++++
>>>>>>   hw/isa/Kconfig                |  2 ++
>>>>>>   9 files changed, 52 insertions(+), 12 deletions(-)
>>>>> 
>>>>> I had a look at this (and did a bit of revision around 8042 and A20), 
>>>>> and I am starting to wonder if the PORT92 device isn't something that 
>>>>> belongs to the southbridge, but more specifically to the superio chip?
>>>> 
>>>> If there is agreement to model real hardware in QEMU, then I think that
>>> 
>>> I think there's no such agreement and QEMU is more lax about it both for 
>>> historical reasons and to simplify machine models. Indeed, QEMU sometimes 
>>> models non-existing machines (e.g. the mac99 or virt boards) that don't 
>>> correspond to real hardware but allow guest OSes to boot. Even when 
>>> modelllng real hardware it's ofren modelled just enough for guests to work 
>>> and unused details are omitted for simplicity. It is recommended to follow 
>>> what real hardware does when modelling real hardware but not always 
>>> required. Although it might help both with verifying a device model and to 
>>> compose machines with these models to try to follow the real hardware.
>> 
>> Composing real machines and verifying device models is exactly what I'm 
>> after. I'm aware that QEMU provides virt machines such as the microvm, and 
>> from the context I didn't refer to these.
>
> Even without pure virt machines currently a lot of QEMU machines don't 
> exactly model real hardware. They may roughly follow real hardware but not 
> exactly such as mac99 is a non-existent Mac and the pc machines also use some 
> parts that don't exist in real life such as PIIX4-PIIX3 hybrid you've been 
> working on resolving. Some of these however are restricted by backward 
> compatibilty requirements. But you probably aware of all of that but this 
> means the argument that real hardware should be followed is not enough. At 
> least it should not break backward compatibility too much and that's more 
> important than exactly modelling real machine. Also having a simple model may 
> be more important than modeling every detail even when not used just to 
> follow real hardware.
>
>>>> port 92 belongs into any device model where the hardware has one. All our 
>>>> PC-like southbridges (PIIX, ICH, VIA) have port 92. Many FDC37xxxx 
>>>> including the FDC37M81x as used in the Malta board have one, too -- where 
>>>> it must first be enabled.
>>> 
>>> So port92 is not a real hardware but a QEMU abstraction or model of some 
>>> functionality found in some machines. Real chips probably implement this 
>>> in different ways so we could either model this in these chips 
>>> independently the same way as real hardware does or use the abstracted 
>>> model anywhere in our machine model. Since this does not exist in real 
>>> hardware as this abstract model it also does not belong anywhere so we are 
>>> free to put it where it's most convenient or simple to do.
>> 
>> As mentioned already, port 92 is an integral part of PIIX, ICH, and VIA 
>> southbridges.
>
> Mark argued that more specifically it's part of the superio within those 
> couthbridges. That makes sense, considering this port92 is related to 
> functionality that was in the keyboard contorller before which is part of the 
> superio. I don't know PC hardware too well but reading about this fast gate 
> A20 feature looks like original PC and XT had only a 1 MB address space but 
> addresses above 1 MB wrapped to 0 and some software depended on that. Then AT 
> added more memory but then it needed a way to control if addresses above 1 MB 
> would wrap or access high memory. This was done with some free part of the 
> keyboard controller but that was too slow so an alternative fast way was 
> added with this port92 device. But then the old keyboard controller and this 
> port92 stuff are interacting so may need to consider both. Apart from that 
> all of this is not relevant to other machines that don't use this 
> functionality.
>
> QEMU decided to model it as a separate QOM object that is now instantiated by 
> the machines that use it. This is not real hardware but a QEMU implementation 
> detail. What's wrong with that? It seems you just want to simplify the pc 
> machine creation and push this object that does not correspond to some real 
> hardware somewhere else. But this belongs nowhere as it does not model a real 
> hardware.
>
>> That's why I want to move it there. My goal is to create different PC 
>> machines in a data-driven manner which model real boards. I want to see how 
>> low-level guests interact with the hardware, including e.g. how they set up 
>> the memory map.
>
> Then I think the port92 as a QOM object should be gone completely and 
> implemented separately in south bridges beacause in real machine there's no 
> such port 92 thing, only a south briege so if you need to create port92 in a 
> data driver machine description then that's not modeling real hardware but 
> reflects QEMU implementation of it. However the QOM object could be retained

So in real VIA vt8231 chip there's a pin that should be connected to the 
CPU. The docs say for this pin: "A20M# A20 Mask. Connect to A20 mask input 
of the CPU to control address bit-20 generation. Logical combination of 
the A20GATE input (from internal or external keyboard controller) and Port 
92 bit-1 (Fast A20). See Device 0 Function 0 Rx59[1]."

There's another pin: "KBCK / A20GATE
MultiFunction Pin (Internal keyboard controller enabled by F0 Rx51[0])
Rx51[0]=1 Keyboard Clock. From internal keyboard controller
Rx51[0]=0 Gate A20. Input from external keyboard controller."

To model this in QEMU following what hardware does these should be named 
gpios that the machine code connects. The functionality seems to be a 
qemu_irq that is either ORed to the similar output from the KBC or not 
depending on register settings. So it seems the KBC model that's embedded 
in the superio should also expose such pin then what uses it should 
connect it somewhere (either forward to other components or combine it 
with its own A20 output).

If you want to follow real hardware then this should be implemented and 
the port92 QOM object should be removed. Also need to check what other 
southbridges do as those may be different at least in how they control the 
qemu_irq so I'm not sure one implementation can be shared between all 
these south bridges that way. It moay be simpler for now to leave it as it 
is.

Regards,
BALATON Zoltan

Re: [PATCH 3/5] hw/isa: Embed TYPE_PORT92 in south bridges used in PC machines

[Bernhard Beschow]

Am 28. Februar 2024 13:02:55 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Wed, 28 Feb 2024, BALATON Zoltan wrote:
>> On Tue, 27 Feb 2024, Bernhard Beschow wrote:
>>> Am 27. Februar 2024 21:54:19 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>>>> On Tue, 27 Feb 2024, Bernhard Beschow wrote:
>>>>> Am 21. Februar 2024 11:53:21 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>>>>>> On 18/02/2024 13:16, Bernhard Beschow wrote:
>>>>>>> Port 92 is an integral part of the PIIX and ICH south bridges, so instantiate it
>>>>>>> there. The isapc machine now needs to instantiate it explicitly, analoguous to
>>>>>>> the RTC.
>>>>>>> 
>>>>>>> Note that due to migration compatibility, port92 is optional in the south
>>>>>>> bridges. It is always instantiated the isapc machine for simplicity.
>>>>>>> 
>>>>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>>>>> ---
>>>>>>>   include/hw/i386/pc.h          |  2 +-
>>>>>>>   include/hw/southbridge/ich9.h |  4 ++++
>>>>>>>   include/hw/southbridge/piix.h |  3 +++
>>>>>>>   hw/i386/pc.c                  | 18 ++++++++++++------
>>>>>>>   hw/i386/pc_piix.c             |  9 +++++++--
>>>>>>>   hw/i386/pc_q35.c              |  8 +++++---
>>>>>>>   hw/isa/lpc_ich9.c             |  9 +++++++++
>>>>>>>   hw/isa/piix.c                 |  9 +++++++++
>>>>>>>   hw/isa/Kconfig                |  2 ++
>>>>>>>   9 files changed, 52 insertions(+), 12 deletions(-)
>>>>>> 
>>>>>> I had a look at this (and did a bit of revision around 8042 and A20), and I am starting to wonder if the PORT92 device isn't something that belongs to the southbridge, but more specifically to the superio chip?
>>>>> 
>>>>> If there is agreement to model real hardware in QEMU, then I think that
>>>> 
>>>> I think there's no such agreement and QEMU is more lax about it both for historical reasons and to simplify machine models. Indeed, QEMU sometimes models non-existing machines (e.g. the mac99 or virt boards) that don't correspond to real hardware but allow guest OSes to boot. Even when modelllng real hardware it's ofren modelled just enough for guests to work and unused details are omitted for simplicity. It is recommended to follow what real hardware does when modelling real hardware but not always required. Although it might help both with verifying a device model and to compose machines with these models to try to follow the real hardware.
>>> 
>>> Composing real machines and verifying device models is exactly what I'm after. I'm aware that QEMU provides virt machines such as the microvm, and from the context I didn't refer to these.
>> 
>> Even without pure virt machines currently a lot of QEMU machines don't exactly model real hardware. They may roughly follow real hardware but not exactly such as mac99 is a non-existent Mac and the pc machines also use some parts that don't exist in real life such as PIIX4-PIIX3 hybrid you've been working on resolving. Some of these however are restricted by backward compatibilty requirements. But you probably aware of all of that but this means the argument that real hardware should be followed is not enough. At least it should not break backward compatibility too much and that's more important than exactly modelling real machine. Also having a simple model may be more important than modeling every detail even when not used just to follow real hardware.
>> 
>>>>> port 92 belongs into any device model where the hardware has one. All our PC-like southbridges (PIIX, ICH, VIA) have port 92. Many FDC37xxxx including the FDC37M81x as used in the Malta board have one, too -- where it must first be enabled.
>>>> 
>>>> So port92 is not a real hardware but a QEMU abstraction or model of some functionality found in some machines. Real chips probably implement this in different ways so we could either model this in these chips independently the same way as real hardware does or use the abstracted model anywhere in our machine model. Since this does not exist in real hardware as this abstract model it also does not belong anywhere so we are free to put it where it's most convenient or simple to do.
>>> 
>>> As mentioned already, port 92 is an integral part of PIIX, ICH, and VIA southbridges.
>> 
>> Mark argued that more specifically it's part of the superio within those couthbridges. That makes sense, considering this port92 is related to functionality that was in the keyboard contorller before which is part of the superio. I don't know PC hardware too well but reading about this fast gate A20 feature looks like original PC and XT had only a 1 MB address space but addresses above 1 MB wrapped to 0 and some software depended on that. Then AT added more memory but then it needed a way to control if addresses above 1 MB would wrap or access high memory. This was done with some free part of the keyboard controller but that was too slow so an alternative fast way was added with this port92 device. But then the old keyboard controller and this port92 stuff are interacting so may need to consider both. Apart from that all of this is not relevant to other machines that don't use this functionality.
>> 
>> QEMU decided to model it as a separate QOM object that is now instantiated by the machines that use it. This is not real hardware but a QEMU implementation detail. What's wrong with that? It seems you just want to simplify the pc machine creation and push this object that does not correspond to some real hardware somewhere else. But this belongs nowhere as it does not model a real hardware.
>> 
>>> That's why I want to move it there. My goal is to create different PC machines in a data-driven manner which model real boards. I want to see how low-level guests interact with the hardware, including e.g. how they set up the memory map.
>> 
>> Then I think the port92 as a QOM object should be gone completely and implemented separately in south bridges beacause in real machine there's no such port 92 thing, only a south briege so if you need to create port92 in a data driver machine description then that's not modeling real hardware but reflects QEMU implementation of it. However the QOM object could be retained
>
>So in real VIA vt8231 chip there's a pin that should be connected to the CPU. The docs say for this pin: "A20M# A20 Mask. Connect to A20 mask input of the CPU to control address bit-20 generation. Logical combination of the A20GATE input (from internal or external keyboard controller) and Port 92 bit-1 (Fast A20). See Device 0 Function 0 Rx59[1]."
>
>There's another pin: "KBCK / A20GATE
>MultiFunction Pin (Internal keyboard controller enabled by F0 Rx51[0])
>Rx51[0]=1 Keyboard Clock. From internal keyboard controller
>Rx51[0]=0 Gate A20. Input from external keyboard controller."
>
>To model this in QEMU following what hardware does these should be named gpios that the machine code connects. The functionality seems to be a qemu_irq that is either ORed to the similar output from the KBC or not depending on register settings. So it seems the KBC model that's embedded in the superio should also expose such pin then what uses it should connect it somewhere (either forward to other components or combine it with its own A20 output).
>
>If you want to follow real hardware then this should be implemented and the port92 QOM object should be removed. Also need to check what other southbridges do as those may be different at least in how they control the qemu_irq so I'm not sure one implementation can be shared between all these south bridges that way. It moay be simpler for now to leave it as it is.

Thanks for sharing how A20 gets handled in VIA. Both ICH9 and PIIX work in a similar manner, where the A20 bits of port 92 and the KBC are logically ORed, except that they have an "A20GATE" input pin instead of an integrated KBC. This means that for the Intel chips the board needs an extra wire from the KBC to the southbridge to handle A20.

Like you say, the correct way of modeling this in QEMU was to move the logic into the southbridges and expose some GPIO pins. I'd need to look into that to figure out how well that plays with bringing the VIA southbridges to the pc machine. Rather than passing port92 through pc_basic_device_init(), like this series does, it might be possible to just pass the southbridge instead.

One interesting outcome of this series is probably that port 92 gets deactivated through the i8042 option which seems to be more than intended. I'd send a patch to fix this.

Best regards,
Bernhard

>
>Regards,
>BALATON Zoltan

Re: [PATCH 3/5] hw/isa: Embed TYPE_PORT92 in south bridges used in PC machines

[BALATON Zoltan]

On Sat, 2 Mar 2024, Bernhard Beschow wrote:
> Am 28. Februar 2024 13:02:55 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>> On Wed, 28 Feb 2024, BALATON Zoltan wrote:
>>> On Tue, 27 Feb 2024, Bernhard Beschow wrote:
>>>> Am 27. Februar 2024 21:54:19 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>>>>> On Tue, 27 Feb 2024, Bernhard Beschow wrote:
>>>>>> Am 21. Februar 2024 11:53:21 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>>>>>>> On 18/02/2024 13:16, Bernhard Beschow wrote:
>>>>>>>> Port 92 is an integral part of the PIIX and ICH south bridges, so instantiate it
>>>>>>>> there. The isapc machine now needs to instantiate it explicitly, analoguous to
>>>>>>>> the RTC.
>>>>>>>>
>>>>>>>> Note that due to migration compatibility, port92 is optional in the south
>>>>>>>> bridges. It is always instantiated the isapc machine for simplicity.
>>>>>>>>
>>>>>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>>>>>> ---
>>>>>>>>   include/hw/i386/pc.h          |  2 +-
>>>>>>>>   include/hw/southbridge/ich9.h |  4 ++++
>>>>>>>>   include/hw/southbridge/piix.h |  3 +++
>>>>>>>>   hw/i386/pc.c                  | 18 ++++++++++++------
>>>>>>>>   hw/i386/pc_piix.c             |  9 +++++++--
>>>>>>>>   hw/i386/pc_q35.c              |  8 +++++---
>>>>>>>>   hw/isa/lpc_ich9.c             |  9 +++++++++
>>>>>>>>   hw/isa/piix.c                 |  9 +++++++++
>>>>>>>>   hw/isa/Kconfig                |  2 ++
>>>>>>>>   9 files changed, 52 insertions(+), 12 deletions(-)
>>>>>>>
>>>>>>> I had a look at this (and did a bit of revision around 8042 and A20), and I am starting to wonder if the PORT92 device isn't something that belongs to the southbridge, but more specifically to the superio chip?
>>>>>>
>>>>>> If there is agreement to model real hardware in QEMU, then I think that
>>>>>
>>>>> I think there's no such agreement and QEMU is more lax about it both for historical reasons and to simplify machine models. Indeed, QEMU sometimes models non-existing machines (e.g. the mac99 or virt boards) that don't correspond to real hardware but allow guest OSes to boot. Even when modelllng real hardware it's ofren modelled just enough for guests to work and unused details are omitted for simplicity. It is recommended to follow what real hardware does when modelling real hardware but not always required. Although it might help both with verifying a device model and to compose machines with these models to try to follow the real hardware.
>>>>
>>>> Composing real machines and verifying device models is exactly what I'm after. I'm aware that QEMU provides virt machines such as the microvm, and from the context I didn't refer to these.
>>>
>>> Even without pure virt machines currently a lot of QEMU machines don't exactly model real hardware. They may roughly follow real hardware but not exactly such as mac99 is a non-existent Mac and the pc machines also use some parts that don't exist in real life such as PIIX4-PIIX3 hybrid you've been working on resolving. Some of these however are restricted by backward compatibilty requirements. But you probably aware of all of that but this means the argument that real hardware should be followed is not enough. At least it should not break backward compatibility too much and that's more important than exactly modelling real machine. Also having a simple model may be more important than modeling every detail even when not used just to follow real hardware.
>>>
>>>>>> port 92 belongs into any device model where the hardware has one. All our PC-like southbridges (PIIX, ICH, VIA) have port 92. Many FDC37xxxx including the FDC37M81x as used in the Malta board have one, too -- where it must first be enabled.
>>>>>
>>>>> So port92 is not a real hardware but a QEMU abstraction or model of some functionality found in some machines. Real chips probably implement this in different ways so we could either model this in these chips independently the same way as real hardware does or use the abstracted model anywhere in our machine model. Since this does not exist in real hardware as this abstract model it also does not belong anywhere so we are free to put it where it's most convenient or simple to do.
>>>>
>>>> As mentioned already, port 92 is an integral part of PIIX, ICH, and VIA southbridges.
>>>
>>> Mark argued that more specifically it's part of the superio within those couthbridges. That makes sense, considering this port92 is related to functionality that was in the keyboard contorller before which is part of the superio. I don't know PC hardware too well but reading about this fast gate A20 feature looks like original PC and XT had only a 1 MB address space but addresses above 1 MB wrapped to 0 and some software depended on that. Then AT added more memory but then it needed a way to control if addresses above 1 MB would wrap or access high memory. This was done with some free part of the keyboard controller but that was too slow so an alternative fast way was added with this port92 device. But then the old keyboard controller and this port92 stuff are interacting so may need to consider both. Apart from that all of this is not relevant to other machines that don't use this functionality.
>>>
>>> QEMU decided to model it as a separate QOM object that is now instantiated by the machines that use it. This is not real hardware but a QEMU implementation detail. What's wrong with that? It seems you just want to simplify the pc machine creation and push this object that does not correspond to some real hardware somewhere else. But this belongs nowhere as it does not model a real hardware.
>>>
>>>> That's why I want to move it there. My goal is to create different PC machines in a data-driven manner which model real boards. I want to see how low-level guests interact with the hardware, including e.g. how they set up the memory map.
>>>
>>> Then I think the port92 as a QOM object should be gone completely and implemented separately in south bridges beacause in real machine there's no such port 92 thing, only a south briege so if you need to create port92 in a data driver machine description then that's not modeling real hardware but reflects QEMU implementation of it. However the QOM object could be retained
>>
>> So in real VIA vt8231 chip there's a pin that should be connected to the CPU. The docs say for this pin: "A20M# A20 Mask. Connect to A20 mask input of the CPU to control address bit-20 generation. Logical combination of the A20GATE input (from internal or external keyboard controller) and Port 92 bit-1 (Fast A20). See Device 0 Function 0 Rx59[1]."
>>
>> There's another pin: "KBCK / A20GATE
>> MultiFunction Pin (Internal keyboard controller enabled by F0 Rx51[0])
>> Rx51[0]=1 Keyboard Clock. From internal keyboard controller
>> Rx51[0]=0 Gate A20. Input from external keyboard controller."
>>
>> To model this in QEMU following what hardware does these should be named gpios that the machine code connects. The functionality seems to be a qemu_irq that is either ORed to the similar output from the KBC or not depending on register settings. So it seems the KBC model that's embedded in the superio should also expose such pin then what uses it should connect it somewhere (either forward to other components or combine it with its own A20 output).
>>
>> If you want to follow real hardware then this should be implemented and the port92 QOM object should be removed. Also need to check what other southbridges do as those may be different at least in how they control the qemu_irq so I'm not sure one implementation can be shared between all these south bridges that way. It moay be simpler for now to leave it as it is.
>
> Thanks for sharing how A20 gets handled in VIA. Both ICH9 and PIIX work 
> in a similar manner, where the A20 bits of port 92 and the KBC are 
> logically ORed, except that they have an "A20GATE" input pin instead of 
> an integrated KBC. This means that for the Intel chips the board needs 
> an extra wire from the KBC to the southbridge to handle A20.
>
> Like you say, the correct way of modeling this in QEMU was to move the 
> logic into the southbridges and expose some GPIO pins. I'd need to look 
> into that to figure out how well that plays with bringing the VIA 
> southbridges to the pc machine. Rather than passing port92 through 
> pc_basic_device_init(), like this series does, it might be possible to 
> just pass the southbridge instead.

Probably the KBC model should have an A20 output which could either be 
connected to the CPU directly (in case of a very old AT without fast A20 
gate) or to the A20GATE input of the southbridge. In case of the VIA chips 
with embedded KBC this would be connected internally. Then the south 
bridges have the A20M output that's the OR of the port92 reg bit and the 
A20 from the KBC and PC models would then connect this to the CPU. On 
other non-x86 boards that don't use it it's just unneeded but could leave 
this output unconnected, it would just make the binary a bit bigger 
carrying this unused part.

> One interesting outcome of this series is probably that port 92 gets 
> deactivated through the i8042 option which seems to be more than 
> intended. I'd send a patch to fix this.

It probably does not matter for the OSes that this is used for as this 
gate A20 is probably only used by DOS to access himem but I don't know.

Regards,
BALATON Zoltan

[PATCH 4/5] hw/i386/pc: Inline i8042_setup_a20_line() and remove it

[Bernhard Beschow]

This function is used once in the pc machines. Remove it since it contains one
line only.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/input/i8042.h | 1 -
 hw/i386/pc.c             | 2 +-
 hw/input/pckbd.c         | 5 -----
 3 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/include/hw/input/i8042.h b/include/hw/input/i8042.h
index 9fb3f8d787..e90f008b66 100644
--- a/include/hw/input/i8042.h
+++ b/include/hw/input/i8042.h
@@ -89,7 +89,6 @@ struct MMIOKBDState {
 
 
 void i8042_isa_mouse_fake_event(ISAKBDState *isa);
-void i8042_setup_a20_line(ISADevice *dev, qemu_irq a20_out);
 
 static inline bool i8042_present(void)
 {
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 8b601ea6cf..1b2077dc32 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1195,7 +1195,7 @@ static void pc_superio_init(ISABus *isa_bus, bool create_fdctrl,
     }
 
     a20_line = qemu_allocate_irqs(handle_a20_line_change, first_cpu, 1);
-    i8042_setup_a20_line(i8042, a20_line[0]);
+    qdev_connect_gpio_out_named(DEVICE(i8042), I8042_A20_LINE, 0, a20_line[0]);
     g_free(a20_line);
 }
 
diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
index 90a4d9eb40..74f10b640f 100644
--- a/hw/input/pckbd.c
+++ b/hw/input/pckbd.c
@@ -777,11 +777,6 @@ void i8042_isa_mouse_fake_event(ISAKBDState *isa)
     ps2_mouse_fake_event(&s->ps2mouse);
 }
 
-void i8042_setup_a20_line(ISADevice *dev, qemu_irq a20_out)
-{
-    qdev_connect_gpio_out_named(DEVICE(dev), I8042_A20_LINE, 0, a20_out);
-}
-
 static const VMStateDescription vmstate_kbd_isa = {
     .name = "pckbd",
     .version_id = 3,
-- 
2.43.2

Re: [PATCH 4/5] hw/i386/pc: Inline i8042_setup_a20_line() and remove it

[Philippe Mathieu-Daudé]

On 18/2/24 14:17, Bernhard Beschow wrote:
> This function is used once in the pc machines. Remove it since it contains one
> line only.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   include/hw/input/i8042.h | 1 -
>   hw/i386/pc.c             | 2 +-
>   hw/input/pckbd.c         | 5 -----
>   3 files changed, 1 insertion(+), 7 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 4/5] hw/i386/pc: Inline i8042_setup_a20_line() and remove it

[Philippe Mathieu-Daudé]

On 18/2/24 14:17, Bernhard Beschow wrote:
> This function is used once in the pc machines. Remove it since it contains one
> line only.

Now reminds me of 
https://lore.kernel.org/qemu-devel/20211218130437.1516929-6-f4bug@amsat.org/

> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   include/hw/input/i8042.h | 1 -
>   hw/i386/pc.c             | 2 +-
>   hw/input/pckbd.c         | 5 -----
>   3 files changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/include/hw/input/i8042.h b/include/hw/input/i8042.h
> index 9fb3f8d787..e90f008b66 100644
> --- a/include/hw/input/i8042.h
> +++ b/include/hw/input/i8042.h
> @@ -89,7 +89,6 @@ struct MMIOKBDState {
>   
>   
>   void i8042_isa_mouse_fake_event(ISAKBDState *isa);
> -void i8042_setup_a20_line(ISADevice *dev, qemu_irq a20_out);
>   
>   static inline bool i8042_present(void)
>   {
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 8b601ea6cf..1b2077dc32 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1195,7 +1195,7 @@ static void pc_superio_init(ISABus *isa_bus, bool create_fdctrl,
>       }
>   
>       a20_line = qemu_allocate_irqs(handle_a20_line_change, first_cpu, 1);
> -    i8042_setup_a20_line(i8042, a20_line[0]);
> +    qdev_connect_gpio_out_named(DEVICE(i8042), I8042_A20_LINE, 0, a20_line[0]);
>       g_free(a20_line);
>   }
>   
> diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
> index 90a4d9eb40..74f10b640f 100644
> --- a/hw/input/pckbd.c
> +++ b/hw/input/pckbd.c
> @@ -777,11 +777,6 @@ void i8042_isa_mouse_fake_event(ISAKBDState *isa)
>       ps2_mouse_fake_event(&s->ps2mouse);
>   }
>   
> -void i8042_setup_a20_line(ISADevice *dev, qemu_irq a20_out)
> -{
> -    qdev_connect_gpio_out_named(DEVICE(dev), I8042_A20_LINE, 0, a20_out);
> -}
> -
>   static const VMStateDescription vmstate_kbd_isa = {
>       .name = "pckbd",
>       .version_id = 3,

[PATCH 5/5] hw/isa/vt82c686: Embed TYPE_PORT92

[Bernhard Beschow]

Port 92 is an integral part of the south bridge, so instantiate it there.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/isa/vt82c686.c | 7 +++++++
 hw/isa/Kconfig    | 1 +
 2 files changed, 8 insertions(+)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index aa91942745..c7b96b3133 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -22,6 +22,7 @@
 #include "hw/qdev-properties.h"
 #include "hw/ide/pci.h"
 #include "hw/isa/isa.h"
+#include "hw/isa/port92.h"
 #include "hw/isa/superio.h"
 #include "hw/intc/i8259.h"
 #include "hw/irq.h"
@@ -597,6 +598,7 @@ struct ViaISAState {
     uint16_t irq_state[ISA_NUM_IRQS];
     ViaSuperIOState via_sio;
     MC146818RtcState rtc;
+    Port92State port92;
     PCIIDEState ide;
     UHCIState uhci[2];
     ViaPMState pm;
@@ -619,6 +621,7 @@ static void via_isa_init(Object *obj)
     ViaISAState *s = VIA_ISA(obj);
 
     object_initialize_child(obj, "rtc", &s->rtc, TYPE_MC146818_RTC);
+    object_initialize_child(obj, "port92", &s->port92, TYPE_PORT92);
     object_initialize_child(obj, "ide", &s->ide, TYPE_VIA_IDE);
     object_initialize_child(obj, "uhci1", &s->uhci[0], TYPE_VT82C686B_USB_UHCI);
     object_initialize_child(obj, "uhci2", &s->uhci[1], TYPE_VT82C686B_USB_UHCI);
@@ -740,6 +743,10 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
     }
     isa_connect_gpio_out(ISA_DEVICE(&s->rtc), 0, s->rtc.isairq);
 
+    if (!qdev_realize(DEVICE(&s->port92), BUS(isa_bus), errp)) {
+        return;
+    }
+
     for (i = 0; i < PCI_CONFIG_HEADER_SIZE; i++) {
         if (i < PCI_COMMAND || i >= PCI_REVISION_ID) {
             d->wmask[i] = 0;
diff --git a/hw/isa/Kconfig b/hw/isa/Kconfig
index f42a087c07..d94f58a2c1 100644
--- a/hw/isa/Kconfig
+++ b/hw/isa/Kconfig
@@ -66,6 +66,7 @@ config VT82C686
     select I8259
     select IDE_VIA
     select MC146818RTC
+    select PORT92
 
 config SMC37C669
     bool
-- 
2.43.2

Re: [PATCH 0/5] Implement port 92 in south bridges

[BALATON Zoltan]

On Sun, 18 Feb 2024, Bernhard Beschow wrote:
> This series attempts to make QEMU's south bridge families PIIX, ICH9, and VIA
> 82xx more self-contained by integrating IO port 92 like the originals do.
>
> In QEMU, the IO port is currently instantiated as a dedicated device in common
> PC code. While this works and even results in less code, it seems cleaner to
> model the behavior of the real devices. For example, software running on the
> Malta machine, which uses PIIX4, needs to take port 92 into account, even if it
> doesn't use it (does it?). Moreover, the FDC37M81x used in the original Malta
> machine provides a port 92 too, which can be activated. If QEMU implemented the
> FDC37M81x more closely, one could check if Yamon (or any alternative boot
> loader) deals correctly with these ports.

Maybe that's unlikely as this register is for controlling A20 line of 
Intel CPUs so probably there's no use for it in a MIPS or PPC board but 
I'm not sure if it may be used for something else.

> Moving port 92 into the south bridges might also help with configuration-driven
> machine creation. In such a scenario it is probably desirable if machine code
> had less of its own idea of which devices it creates.

The direction is probably good as these chips have a pin for A20 control 
and handle the register themselves but I'm not sure this series is the 
right way. One immediate problem is that TYPE_PORT92 has state which is in 
the migration stream so moving it elsewhere would break migration which 
would need to be handled. Does this series handle that? I'm not sure it's 
worth the effort though if it results in more comlex code. If the 
migration issue is handled, then I think we should get rid of TYPE_PORT92 
completely and just add the one reg and qemu_irq modeling the output pin 
as qemu_gpio to the south bridge implementations directly, not embedding a 
separate object for it as these south bridges may already have some io 
region for ports and state where the reg can be stored so it could be 
added there instead of just moving the TYPE_PORT92 there. But with the 
migration issue it's probably easier to just leave it as it is now. Even 
if this would model the real chip better, it would result in more code and 
complexity in QEMU so not sure it's a good idea because of that.

> Moving port 92 from
> machine code into a potentially user-creaeable device (where it is part of per
> datasheet) seems like a good direction. Of course, machine code still wires up
> port 92 and I don't have a good idea on how to make this user-configurable.
> Such insights might provide some input for discussions around
> configuration-driven machine creation.

That's a generic problem for dynamic or declarative machine creation to 
solve. Likely the machine description will also need to describe the 
connections between devices, not just what devices to instantiate. So 
that's not specific to port92. As we're not there yet it's also not urgent 
to touch this port92 stuff.

Maybe I overestimate the migration issue as I'm not familiar with that so 
if others think it's not an issue then I'm not against this series as it 
would bring the model closer to the actual hardware but then go all the 
way and get rid of TYPE_PORT92 and just implement it in the south bridges. 
But due to how it's currently done and how that's now baked in because of 
backward compatibility requrement for migration, I'm not sure it would 
really simplify the code, so we may need to live with what we have now. 
But let me know if I'm wrong and missed something.

Regards,
BALATON Zoltan

> This series is structured as follows: Patch 1 moves TYPE_PORT92 into the isa
> directory to make it reusable by other architectures. It also adds a
> configuration switch. Patch 2 integrates TYPE_PORT92 into the PC south bridges
> and adapts PC code accordingly. While at it, patch 3 cleans up wiring of the
> A20 line with the keyboard controller. Patch 4 simply adds TYPE_PORT92 to the
> VIA south bridges which is also needed when using the VIA south bridges in the
> pc machine.
>
> Testing done:
> * `qemu-system-x86_64 -M {q35,pc},i8042={true,false} ...`
>  -> `info mtree` confirms port92 to be present iff i8042=true
> * `make check`
> * `make check-avocado`
> * Start amigaone and pegasos2 machines as described in
>    https://patchew.org/QEMU/20240216001019.69A524E601F@zero.eik.bme.hu/
>  -> no regressions compared to master
>
> Best regards,
> Bernhard
>
> Bernhard Beschow (5):
>  hw/isa/meson.build: Sort alphabetically
>  hw/i386/port92: Allow for TYPE_PORT92 to be embedded in devices
>  hw/isa: Embed TYPE_PORT92 in south bridges used in PC machines
>  hw/i386/pc: Inline i8042_setup_a20_line() and remove it
>  hw/isa/vt82c686: Embed TYPE_PORT92
>
> include/hw/i386/pc.h          |  7 +------
> include/hw/input/i8042.h      |  1 -
> include/hw/isa/port92.h       | 30 ++++++++++++++++++++++++++++++
> include/hw/southbridge/ich9.h |  4 ++++
> include/hw/southbridge/piix.h |  3 +++
> hw/i386/pc.c                  | 21 ++++++++++++++-------
> hw/i386/pc_piix.c             |  9 +++++++--
> hw/i386/pc_q35.c              |  8 +++++---
> hw/input/pckbd.c              |  5 -----
> hw/isa/lpc_ich9.c             |  9 +++++++++
> hw/isa/piix.c                 |  9 +++++++++
> hw/{i386 => isa}/port92.c     | 14 +-------------
> hw/isa/vt82c686.c             |  7 +++++++
> hw/i386/Kconfig               |  1 +
> hw/i386/meson.build           |  3 +--
> hw/i386/trace-events          |  4 ----
> hw/isa/Kconfig                |  6 ++++++
> hw/isa/meson.build            |  3 ++-
> hw/isa/trace-events           |  4 ++++
> 19 files changed, 104 insertions(+), 44 deletions(-)
> create mode 100644 include/hw/isa/port92.h
> rename hw/{i386 => isa}/port92.c (91%)
>
> --
> 2.43.2
>
>
>

Re: [PATCH 0/5] Implement port 92 in south bridges

[Bernhard Beschow]

Am 18. Februar 2024 16:12:30 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Sun, 18 Feb 2024, Bernhard Beschow wrote:
>> This series attempts to make QEMU's south bridge families PIIX, ICH9, and VIA
>> 82xx more self-contained by integrating IO port 92 like the originals do.
>> 
>> In QEMU, the IO port is currently instantiated as a dedicated device in common
>> PC code. While this works and even results in less code, it seems cleaner to
>> model the behavior of the real devices. For example, software running on the
>> Malta machine, which uses PIIX4, needs to take port 92 into account, even if it
>> doesn't use it (does it?). Moreover, the FDC37M81x used in the original Malta
>> machine provides a port 92 too, which can be activated. If QEMU implemented the
>> FDC37M81x more closely, one could check if Yamon (or any alternative boot
>> loader) deals correctly with these ports.
>
>Maybe that's unlikely as this register is for controlling A20 line of Intel CPUs so probably there's no use for it in a MIPS or PPC board but I'm not sure if it may be used for something else.
>
>> Moving port 92 into the south bridges might also help with configuration-driven
>> machine creation. In such a scenario it is probably desirable if machine code
>> had less of its own idea of which devices it creates.
>
>The direction is probably good as these chips have a pin for A20 control and handle the register themselves but I'm not sure this series is the right way. One immediate problem is that TYPE_PORT92 has state which is in the migration stream so moving it elsewhere would break migration which would need to be handled. Does this series handle that?

I've created two migration streams, one before and one after the change. Then I've converted both to JSON with `analyze-migration.py -d desc`. The only difference is the position of the port92 record in the devices array (before: 31, after: 14). I'm no migration expert, but telling from the JSON content, "name" and "instance_id" seem to uniquely identify a device record and changing positions in an array seems rather innocent to me. Since there will be one port92 record before and after this series, it seems as if migration is not affected. But someone's judgment with migration expertise may be needed here.

> I'm not sure it's worth the effort though if it results in more comlex code. If the migration issue is handled, then I think we should get rid of TYPE_PORT92 completely and just add the one reg and qemu_irq modeling the output pin as qemu_gpio to the south bridge implementations directly, not embedding a separate object for it as these south bridges may already have some io region for ports and state where the reg can be stored so it could be added there instead of just moving the TYPE_PORT92 there.

I'd rather avoid adding copy'n'paste code. Having port 92 wrapped in a dedicated, reusable device model which handles migration seems like the way to go to me.

> But with the migration issue it's probably easier to just leave it as it is now. Even if this would model the real chip better, it would result in more code and complexity in QEMU so not sure it's a good idea because of that.
>
>> Moving port 92 from
>> machine code into a potentially user-creaeable device (where it is part of per
>> datasheet) seems like a good direction. Of course, machine code still wires up
>> port 92 and I don't have a good idea on how to make this user-configurable.
>> Such insights might provide some input for discussions around
>> configuration-driven machine creation.
>
>That's a generic problem for dynamic or declarative machine creation to solve. Likely the machine description will also need to describe the connections between devices, not just what devices to instantiate. So that's not specific to port92. As we're not there yet it's also not urgent to touch this port92 stuff.
>
>Maybe I overestimate the migration issue as I'm not familiar with that so if others think it's not an issue then I'm not against this series as it would bring the model closer to the actual hardware but then go all the way and get rid of TYPE_PORT92 and just implement it in the south bridges. But due to how it's currently done and how that's now baked in because of backward compatibility requrement for migration, I'm not sure it would really simplify the code, so we may need to live with what we have now. But let me know if I'm wrong and missed something.
>
>Regards,
>BALATON Zoltan

Any other opinions?

Best regards,
Bernhard

>
>> This series is structured as follows: Patch 1 moves TYPE_PORT92 into the isa
>> directory to make it reusable by other architectures. It also adds a
>> configuration switch. Patch 2 integrates TYPE_PORT92 into the PC south bridges
>> and adapts PC code accordingly. While at it, patch 3 cleans up wiring of the
>> A20 line with the keyboard controller. Patch 4 simply adds TYPE_PORT92 to the
>> VIA south bridges which is also needed when using the VIA south bridges in the
>> pc machine.
>> 
>> Testing done:
>> * `qemu-system-x86_64 -M {q35,pc},i8042={true,false} ...`
>>  -> `info mtree` confirms port92 to be present iff i8042=true
>> * `make check`
>> * `make check-avocado`
>> * Start amigaone and pegasos2 machines as described in
>>    https://patchew.org/QEMU/20240216001019.69A524E601F@zero.eik.bme.hu/
>>  -> no regressions compared to master
>> 
>> Best regards,
>> Bernhard
>> 
>> Bernhard Beschow (5):
>>  hw/isa/meson.build: Sort alphabetically
>>  hw/i386/port92: Allow for TYPE_PORT92 to be embedded in devices
>>  hw/isa: Embed TYPE_PORT92 in south bridges used in PC machines
>>  hw/i386/pc: Inline i8042_setup_a20_line() and remove it
>>  hw/isa/vt82c686: Embed TYPE_PORT92
>> 
>> include/hw/i386/pc.h          |  7 +------
>> include/hw/input/i8042.h      |  1 -
>> include/hw/isa/port92.h       | 30 ++++++++++++++++++++++++++++++
>> include/hw/southbridge/ich9.h |  4 ++++
>> include/hw/southbridge/piix.h |  3 +++
>> hw/i386/pc.c                  | 21 ++++++++++++++-------
>> hw/i386/pc_piix.c             |  9 +++++++--
>> hw/i386/pc_q35.c              |  8 +++++---
>> hw/input/pckbd.c              |  5 -----
>> hw/isa/lpc_ich9.c             |  9 +++++++++
>> hw/isa/piix.c                 |  9 +++++++++
>> hw/{i386 => isa}/port92.c     | 14 +-------------
>> hw/isa/vt82c686.c             |  7 +++++++
>> hw/i386/Kconfig               |  1 +
>> hw/i386/meson.build           |  3 +--
>> hw/i386/trace-events          |  4 ----
>> hw/isa/Kconfig                |  6 ++++++
>> hw/isa/meson.build            |  3 ++-
>> hw/isa/trace-events           |  4 ++++
>> 19 files changed, 104 insertions(+), 44 deletions(-)
>> create mode 100644 include/hw/isa/port92.h
>> rename hw/{i386 => isa}/port92.c (91%)
>> 
>> --
>> 2.43.2
>> 
>> 
>> 

Re: [PATCH 0/5] Implement port 92 in south bridges

[Michael S. Tsirkin]

On Sun, Feb 18, 2024 at 02:16:56PM +0100, Bernhard Beschow wrote:
> This series attempts to make QEMU's south bridge families PIIX, ICH9, and VIA
> 82xx more self-contained by integrating IO port 92 like the originals do.

Bernhard my understanding is that you agreed with Mark this
needs more work. Dropping for now.

Re: [PATCH 0/5] Implement port 92 in south bridges

[Bernhard Beschow]

Am 12. März 2024 15:47:21 UTC schrieb "Michael S. Tsirkin" <mst@redhat.com>:
>On Sun, Feb 18, 2024 at 02:16:56PM +0100, Bernhard Beschow wrote:
>> This series attempts to make QEMU's south bridge families PIIX, ICH9, and VIA
>> 82xx more self-contained by integrating IO port 92 like the originals do.
>
>Bernhard my understanding is that you agreed with Mark this
>needs more work. Dropping for now.

Yeah, no 9.0 material.

Best regards,
Bernhard