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 <shen...@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
