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