* [PATCH 00/14] hw/southbridge: Extract ICH9 QOM container model
@ 2024-02-19 16:38 Philippe Mathieu-Daudé
2024-02-19 16:38 ` [PATCH 01/14] MAINTAINERS: Add 'ICH9 South Bridge' section Philippe Mathieu-Daudé
` (13 more replies)
0 siblings, 14 replies; 28+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-02-19 16:38 UTC (permalink / raw)
To: qemu-devel, Bernhard Beschow
Cc: Michael S. Tsirkin, Ani Sinha, Richard Henderson, Igor Mammedov,
Mark Cave-Ayland, Laurent Vivier, Thomas Huth, Marcel Apfelbaum,
Eduardo Habkost, Paolo Bonzini, BALATON Zoltan,
Philippe Mathieu-Daudé
Hi,
I have a long standing southbridge QOM rework branches. Since
Bernhard is actively working on the PIIX, I'll try to refresh
and post. This is also motivated by the Dynamic Machine work
where we are trying to figure the ideal DSL for QEMU, so having
complex models well designed help.
Here we introduce the ICH9 'southbridge' as a QOM container.
Since the chipset comes as a whole, we shouldn't instantiate
its components separately. However in order to maintain old
code we expose some properties to configure the container and
not introduce any change for the Q35 machine. There is no
migration change, only QOM objects moved around.
More work remain in the LPC function (more code to remove from
Q35). Maybe worth doing in parallel with the PIIX to clean both
PC machines.
Also we'd need to decouple the cpu_interrupt() calls between hw/
and target/.
Note that GSI is currently broken [1]. Once the LPC/ISA part is
done, it might be easier to fix it.
Based-on: <20240213043859.61019-1-philmd@linaro.org> [2]
Based-on: <20240219141412.71418-1-philmd@linaro.org> [3]
[1] https://lore.kernel.org/qemu-devel/cd0e13c6-c03d-411f-83a5-1d4d28ea4345@linaro.org/
[2] https://lore.kernel.org/qemu-devel/20240213043859.61019-1-philmd@linaro.org/ (USB)
[3] https://lore.kernel.org/qemu-devel/20240219141412.71418-1-philmd@linaro.org/ (AHCI)
Philippe Mathieu-Daudé (14):
MAINTAINERS: Add 'ICH9 South Bridge' section
hw/i386/q35: Add local 'lpc_obj' variable
hw/acpi/ich9: Restrict definitions from 'hw/southbridge/ich9.h'
hw/acpi/ich9_tco: Include 'ich9' in names
hw/acpi/ich9_tco: Restrict ich9_generate_smi() declaration
hw/pci-bridge: Extract QOM ICH definitions to 'ich_dmi_pci.h'
hw/southbridge/ich9: Introduce TYPE_ICH9_SOUTHBRIDGE stub
hw/southbridge/ich9: Add the DMI-to-PCI bridge
hw/southbridge/ich9: Add a AHCI function
hw/i2c/smbus: Extract QOM ICH9 definitions to 'smbus_ich9.h'
hw/southbridge/ich9: Add the SMBus function
hw/southbridge/ich9: Add the USB EHCI/UHCI functions
hw/southbridge/ich9: Extract LPC definitions to 'hw/isa/ich9_lpc.h'
hw/southbridge/ich9: Add the LPC / ISA bridge function
MAINTAINERS | 21 ++-
include/hw/acpi/ich9.h | 15 ++
include/hw/acpi/ich9_tco.h | 6 +-
include/hw/i2c/smbus_ich9.h | 25 +++
include/hw/isa/ich9_lpc.h | 166 ++++++++++++++++++++
include/hw/pci-bridge/ich_dmi_pci.h | 20 +++
include/hw/southbridge/ich9.h | 235 +---------------------------
hw/acpi/ich9.c | 9 +-
hw/acpi/ich9_tco.c | 5 +-
hw/i2c/smbus_ich9.c | 36 +++--
hw/i386/acpi-build.c | 1 +
hw/i386/pc_q35.c | 124 +++------------
hw/isa/lpc_ich9.c | 37 ++++-
hw/pci-bridge/i82801b11.c | 11 +-
hw/southbridge/ich9.c | 212 +++++++++++++++++++++++++
tests/qtest/tco-test.c | 2 +-
hw/Kconfig | 1 +
hw/i386/Kconfig | 3 +-
hw/meson.build | 1 +
hw/southbridge/Kconfig | 11 ++
hw/southbridge/meson.build | 3 +
21 files changed, 581 insertions(+), 363 deletions(-)
create mode 100644 include/hw/i2c/smbus_ich9.h
create mode 100644 include/hw/isa/ich9_lpc.h
create mode 100644 include/hw/pci-bridge/ich_dmi_pci.h
create mode 100644 hw/southbridge/ich9.c
create mode 100644 hw/southbridge/Kconfig
create mode 100644 hw/southbridge/meson.build
--
2.41.0
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 01/14] MAINTAINERS: Add 'ICH9 South Bridge' section
2024-02-19 16:38 [PATCH 00/14] hw/southbridge: Extract ICH9 QOM container model Philippe Mathieu-Daudé
@ 2024-02-19 16:38 ` Philippe Mathieu-Daudé
2024-02-19 16:38 ` [PATCH 02/14] hw/i386/q35: Add local 'lpc_obj' variable Philippe Mathieu-Daudé
` (12 subsequent siblings)
13 siblings, 0 replies; 28+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-02-19 16:38 UTC (permalink / raw)
To: qemu-devel, Bernhard Beschow
Cc: Michael S. Tsirkin, Ani Sinha, Richard Henderson, Igor Mammedov,
Mark Cave-Ayland, Laurent Vivier, Thomas Huth, Marcel Apfelbaum,
Eduardo Habkost, Paolo Bonzini, BALATON Zoltan,
Philippe Mathieu-Daudé
Extract 'ICH9 South Bridge' from the 'PC' section.
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
MAINTAINERS | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index 7d61fb9319..1b210c5cc1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1808,12 +1808,7 @@ F: include/hw/pci-host/i440fx.h
F: include/hw/pci-host/q35.h
F: include/hw/pci-host/pam.h
F: hw/isa/piix.c
-F: hw/isa/lpc_ich9.c
-F: hw/i2c/smbus_ich9.c
F: hw/acpi/piix4.c
-F: hw/acpi/ich9*.c
-F: include/hw/acpi/ich9*.h
-F: include/hw/southbridge/ich9.h
F: include/hw/southbridge/piix.h
F: hw/isa/apm.c
F: include/hw/isa/apm.h
@@ -2606,6 +2601,16 @@ F: hw/display/edid*
F: include/hw/display/edid.h
F: qemu-edid.c
+ICH9 South Bridge (82801i)
+M: Michael S. Tsirkin <mst@redhat.com>
+M: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
+S: Supported
+F: hw/acpi/ich9*.c
+F: hw/i2c/smbus_ich9.c
+F: hw/isa/lpc_ich9.c
+F: include/hw/acpi/ich9*.h
+F: include/hw/southbridge/ich9.h
+
PIIX4 South Bridge (i82371AB)
M: Hervé Poussineau <hpoussin@reactos.org>
M: Philippe Mathieu-Daudé <philmd@linaro.org>
--
2.41.0
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 02/14] hw/i386/q35: Add local 'lpc_obj' variable
2024-02-19 16:38 [PATCH 00/14] hw/southbridge: Extract ICH9 QOM container model Philippe Mathieu-Daudé
2024-02-19 16:38 ` [PATCH 01/14] MAINTAINERS: Add 'ICH9 South Bridge' section Philippe Mathieu-Daudé
@ 2024-02-19 16:38 ` Philippe Mathieu-Daudé
2024-02-19 16:38 ` [PATCH 03/14] hw/acpi/ich9: Restrict definitions from 'hw/southbridge/ich9.h' Philippe Mathieu-Daudé
` (11 subsequent siblings)
13 siblings, 0 replies; 28+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-02-19 16:38 UTC (permalink / raw)
To: qemu-devel, Bernhard Beschow
Cc: Michael S. Tsirkin, Ani Sinha, Richard Henderson, Igor Mammedov,
Mark Cave-Ayland, Laurent Vivier, Thomas Huth, Marcel Apfelbaum,
Eduardo Habkost, Paolo Bonzini, BALATON Zoltan,
Philippe Mathieu-Daudé
Instead of casting OBJECT(lpc) multiple times,
do it once in the new 'lpc_obj' variable.
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
hw/i386/pc_q35.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index a91f414922..621661a738 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -126,6 +126,7 @@ static void pc_q35_init(MachineState *machine)
Object *phb;
PCIBus *host_bus;
PCIDevice *lpc;
+ Object *lpc_obj;
DeviceState *lpc_dev;
BusState *idebus[MAX_SATA_PORTS];
ISADevice *rtc_state;
@@ -238,6 +239,7 @@ static void pc_q35_init(MachineState *machine)
/* create ISA bus */
lpc = pci_new_multifunction(PCI_DEVFN(ICH9_LPC_DEV, ICH9_LPC_FUNC),
TYPE_ICH9_LPC_DEVICE);
+ lpc_obj = OBJECT(lpc);
lpc_dev = DEVICE(lpc);
qdev_prop_set_bit(lpc_dev, "smm-enabled",
x86_machine_is_smm_enabled(x86ms));
@@ -246,7 +248,7 @@ static void pc_q35_init(MachineState *machine)
qdev_connect_gpio_out_named(lpc_dev, ICH9_GPIO_GSI, i, x86ms->gsi[i]);
}
- rtc_state = ISA_DEVICE(object_resolve_path_component(OBJECT(lpc), "rtc"));
+ rtc_state = ISA_DEVICE(object_resolve_path_component(lpc_obj, "rtc"));
object_property_add_link(OBJECT(machine), PC_MACHINE_ACPI_DEVICE_PROP,
TYPE_HOTPLUG_HANDLER,
@@ -254,13 +256,13 @@ static void pc_q35_init(MachineState *machine)
object_property_allow_set_link,
OBJ_PROP_LINK_STRONG);
object_property_set_link(OBJECT(machine), PC_MACHINE_ACPI_DEVICE_PROP,
- OBJECT(lpc), &error_abort);
+ lpc_obj, &error_abort);
- acpi_pcihp = object_property_get_bool(OBJECT(lpc),
+ acpi_pcihp = object_property_get_bool(lpc_obj,
ACPI_PM_PROP_ACPI_PCIHP_BRIDGE,
NULL);
- keep_pci_slot_hpc = object_property_get_bool(OBJECT(lpc),
+ keep_pci_slot_hpc = object_property_get_bool(lpc_obj,
"x-keep-pci-slot-hpc",
NULL);
--
2.41.0
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 03/14] hw/acpi/ich9: Restrict definitions from 'hw/southbridge/ich9.h'
2024-02-19 16:38 [PATCH 00/14] hw/southbridge: Extract ICH9 QOM container model Philippe Mathieu-Daudé
2024-02-19 16:38 ` [PATCH 01/14] MAINTAINERS: Add 'ICH9 South Bridge' section Philippe Mathieu-Daudé
2024-02-19 16:38 ` [PATCH 02/14] hw/i386/q35: Add local 'lpc_obj' variable Philippe Mathieu-Daudé
@ 2024-02-19 16:38 ` Philippe Mathieu-Daudé
2024-02-19 16:38 ` [PATCH 04/14] hw/acpi/ich9_tco: Include 'ich9' in names Philippe Mathieu-Daudé
` (10 subsequent siblings)
13 siblings, 0 replies; 28+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-02-19 16:38 UTC (permalink / raw)
To: qemu-devel, Bernhard Beschow
Cc: Michael S. Tsirkin, Ani Sinha, Richard Henderson, Igor Mammedov,
Mark Cave-Ayland, Laurent Vivier, Thomas Huth, Marcel Apfelbaum,
Eduardo Habkost, Paolo Bonzini, BALATON Zoltan,
Philippe Mathieu-Daudé
Restrict ACPI definitions from "hw/southbridge/ich9.h"
to the ACPI files where they are used.
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
include/hw/acpi/ich9.h | 15 +++++++++++++++
include/hw/southbridge/ich9.h | 18 ------------------
hw/acpi/ich9.c | 4 ++++
3 files changed, 19 insertions(+), 18 deletions(-)
diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
index 215de3c91f..3587a35c9f 100644
--- a/include/hw/acpi/ich9.h
+++ b/include/hw/acpi/ich9.h
@@ -91,4 +91,19 @@ void ich9_pm_device_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
bool ich9_pm_is_hotpluggable_bus(HotplugHandler *hotplug_dev, BusState *bus);
void ich9_pm_ospm_status(AcpiDeviceIf *adev, ACPIOSTInfoList ***list);
+
+#define ICH9_PMIO_PM1_STS 0x00
+#define ICH9_PMIO_PM1_EN 0x02
+#define ICH9_PMIO_PM1_CNT 0x04
+#define ICH9_PMIO_PM1_TMR 0x08
+#define ICH9_PMIO_GPE0_STS 0x20
+#define ICH9_PMIO_GPE0_EN 0x28
+#define ICH9_PMIO_GPE0_LEN 16
+#define ICH9_PMIO_SMI_EN 0x30
+#define ICH9_PMIO_SMI_EN_APMC_EN (1 << 5)
+#define ICH9_PMIO_SMI_EN_TCO_EN (1 << 13)
+#define ICH9_PMIO_SMI_STS 0x34
+#define ICH9_PMIO_TCO_RLD 0x60
+#define ICH9_PMIO_TCO_LEN 32
+
#endif /* HW_ACPI_ICH9_H */
diff --git a/include/hw/southbridge/ich9.h b/include/hw/southbridge/ich9.h
index fd01649d04..1ac4238f7e 100644
--- a/include/hw/southbridge/ich9.h
+++ b/include/hw/southbridge/ich9.h
@@ -183,24 +183,6 @@ struct ICH9LPCState {
/* D31:F0 power management I/O registers
offset from the address ICH9_LPC_PMBASE */
-/* ICH9 LPC PM I/O registers are 128 ports and 128-aligned */
-#define ICH9_PMIO_SIZE 128
-#define ICH9_PMIO_MASK (ICH9_PMIO_SIZE - 1)
-
-#define ICH9_PMIO_PM1_STS 0x00
-#define ICH9_PMIO_PM1_EN 0x02
-#define ICH9_PMIO_PM1_CNT 0x04
-#define ICH9_PMIO_PM1_TMR 0x08
-#define ICH9_PMIO_GPE0_STS 0x20
-#define ICH9_PMIO_GPE0_EN 0x28
-#define ICH9_PMIO_GPE0_LEN 16
-#define ICH9_PMIO_SMI_EN 0x30
-#define ICH9_PMIO_SMI_EN_APMC_EN (1 << 5)
-#define ICH9_PMIO_SMI_EN_TCO_EN (1 << 13)
-#define ICH9_PMIO_SMI_STS 0x34
-#define ICH9_PMIO_TCO_RLD 0x60
-#define ICH9_PMIO_TCO_LEN 32
-
/* FADT ACPI_ENABLE/ACPI_DISABLE */
#define ICH9_APM_ACPI_ENABLE 0x2
#define ICH9_APM_ACPI_DISABLE 0x3
diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index be375a8b9d..228ebc9a1e 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -49,6 +49,10 @@ do { printf("%s "fmt, __func__, ## __VA_ARGS__); } while (0)
#define ICH9_DEBUG(fmt, ...) do { } while (0)
#endif
+/* ICH9 LPC PM I/O registers are 128 ports and 128-aligned */
+#define ICH9_PMIO_SIZE 128
+#define ICH9_PMIO_MASK (ICH9_PMIO_SIZE - 1)
+
static void ich9_pm_update_sci_fn(ACPIREGS *regs)
{
ICH9LPCPMRegs *pm = container_of(regs, ICH9LPCPMRegs, acpi_regs);
--
2.41.0
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 04/14] hw/acpi/ich9_tco: Include 'ich9' in names
2024-02-19 16:38 [PATCH 00/14] hw/southbridge: Extract ICH9 QOM container model Philippe Mathieu-Daudé
` (2 preceding siblings ...)
2024-02-19 16:38 ` [PATCH 03/14] hw/acpi/ich9: Restrict definitions from 'hw/southbridge/ich9.h' Philippe Mathieu-Daudé
@ 2024-02-19 16:38 ` Philippe Mathieu-Daudé
2024-02-19 16:38 ` [PATCH 05/14] hw/acpi/ich9_tco: Restrict ich9_generate_smi() declaration Philippe Mathieu-Daudé
` (9 subsequent siblings)
13 siblings, 0 replies; 28+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-02-19 16:38 UTC (permalink / raw)
To: qemu-devel, Bernhard Beschow
Cc: Michael S. Tsirkin, Ani Sinha, Richard Henderson, Igor Mammedov,
Mark Cave-Ayland, Laurent Vivier, Thomas Huth, Marcel Apfelbaum,
Eduardo Habkost, Paolo Bonzini, BALATON Zoltan,
Philippe Mathieu-Daudé
Make it explicit the following are ICH9 specific:
acpi_pm_tco_init() -> ich9_acpi_pm_tco_init()
vmstate_tco_io_sts -> vmstate_ich9_sm_tco.
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
include/hw/acpi/ich9_tco.h | 5 ++---
hw/acpi/ich9.c | 4 ++--
hw/acpi/ich9_tco.c | 4 ++--
3 files changed, 6 insertions(+), 7 deletions(-)
diff --git a/include/hw/acpi/ich9_tco.h b/include/hw/acpi/ich9_tco.h
index 2562a7cf39..1c99781a79 100644
--- a/include/hw/acpi/ich9_tco.h
+++ b/include/hw/acpi/ich9_tco.h
@@ -75,9 +75,8 @@ typedef struct TCOIORegs {
MemoryRegion io;
} TCOIORegs;
-/* tco.c */
-void acpi_pm_tco_init(TCOIORegs *tr, MemoryRegion *parent);
+void ich9_acpi_pm_tco_init(TCOIORegs *tr, MemoryRegion *parent);
-extern const VMStateDescription vmstate_tco_io_sts;
+extern const VMStateDescription vmstate_ich9_sm_tco;
#endif /* HW_ACPI_TCO_H */
diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 228ebc9a1e..660fa6a082 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -186,7 +186,7 @@ static const VMStateDescription vmstate_tco_io_state = {
.minimum_version_id = 1,
.needed = vmstate_test_use_tco,
.fields = (const VMStateField[]) {
- VMSTATE_STRUCT(tco_regs, ICH9LPCPMRegs, 1, vmstate_tco_io_sts,
+ VMSTATE_STRUCT(tco_regs, ICH9LPCPMRegs, 1, vmstate_ich9_sm_tco,
TCOIORegs),
VMSTATE_END_OF_LIST()
}
@@ -317,7 +317,7 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm, qemu_irq sci_irq)
memory_region_add_subregion(&pm->io, ICH9_PMIO_SMI_EN, &pm->io_smi);
if (pm->enable_tco) {
- acpi_pm_tco_init(&pm->tco_regs, &pm->io);
+ ich9_acpi_pm_tco_init(&pm->tco_regs, &pm->io);
}
if (pm->acpi_pci_hotplug.use_acpi_hotplug_bridge) {
diff --git a/hw/acpi/ich9_tco.c b/hw/acpi/ich9_tco.c
index 81606219f7..dd4aff82e0 100644
--- a/hw/acpi/ich9_tco.c
+++ b/hw/acpi/ich9_tco.c
@@ -224,7 +224,7 @@ static const MemoryRegionOps tco_io_ops = {
.endianness = DEVICE_LITTLE_ENDIAN,
};
-void acpi_pm_tco_init(TCOIORegs *tr, MemoryRegion *parent)
+void ich9_acpi_pm_tco_init(TCOIORegs *tr, MemoryRegion *parent)
{
*tr = (TCOIORegs) {
.tco = {
@@ -250,7 +250,7 @@ void acpi_pm_tco_init(TCOIORegs *tr, MemoryRegion *parent)
memory_region_add_subregion(parent, ICH9_PMIO_TCO_RLD, &tr->io);
}
-const VMStateDescription vmstate_tco_io_sts = {
+const VMStateDescription vmstate_ich9_sm_tco = {
.name = "tco io device status",
.version_id = 1,
.minimum_version_id = 1,
--
2.41.0
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 05/14] hw/acpi/ich9_tco: Restrict ich9_generate_smi() declaration
2024-02-19 16:38 [PATCH 00/14] hw/southbridge: Extract ICH9 QOM container model Philippe Mathieu-Daudé
` (3 preceding siblings ...)
2024-02-19 16:38 ` [PATCH 04/14] hw/acpi/ich9_tco: Include 'ich9' in names Philippe Mathieu-Daudé
@ 2024-02-19 16:38 ` Philippe Mathieu-Daudé
2024-02-20 6:32 ` Philippe Mathieu-Daudé
2024-02-19 16:38 ` [PATCH 06/14] hw/pci-bridge: Extract QOM ICH definitions to 'ich_dmi_pci.h' Philippe Mathieu-Daudé
` (8 subsequent siblings)
13 siblings, 1 reply; 28+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-02-19 16:38 UTC (permalink / raw)
To: qemu-devel, Bernhard Beschow
Cc: Michael S. Tsirkin, Ani Sinha, Richard Henderson, Igor Mammedov,
Mark Cave-Ayland, Laurent Vivier, Thomas Huth, Marcel Apfelbaum,
Eduardo Habkost, Paolo Bonzini, BALATON Zoltan,
Philippe Mathieu-Daudé
Only files including "hw/acpi/ich9_tco.h" require
the ich9_generate_smi() declaration.
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
include/hw/acpi/ich9_tco.h | 1 +
include/hw/southbridge/ich9.h | 2 --
2 files changed, 1 insertion(+), 2 deletions(-)
diff --git a/include/hw/acpi/ich9_tco.h b/include/hw/acpi/ich9_tco.h
index 1c99781a79..68ee64942f 100644
--- a/include/hw/acpi/ich9_tco.h
+++ b/include/hw/acpi/ich9_tco.h
@@ -76,6 +76,7 @@ typedef struct TCOIORegs {
} TCOIORegs;
void ich9_acpi_pm_tco_init(TCOIORegs *tr, MemoryRegion *parent);
+void ich9_generate_smi(void);
extern const VMStateDescription vmstate_ich9_sm_tco;
diff --git a/include/hw/southbridge/ich9.h b/include/hw/southbridge/ich9.h
index 1ac4238f7e..bee522a4cf 100644
--- a/include/hw/southbridge/ich9.h
+++ b/include/hw/southbridge/ich9.h
@@ -11,8 +11,6 @@
#include "qemu/notify.h"
#include "qom/object.h"
-void ich9_generate_smi(void);
-
#define ICH9_CC_SIZE (16 * 1024) /* 16KB. Chipset configuration registers */
#define TYPE_ICH9_LPC_DEVICE "ICH9-LPC"
--
2.41.0
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 06/14] hw/pci-bridge: Extract QOM ICH definitions to 'ich_dmi_pci.h'
2024-02-19 16:38 [PATCH 00/14] hw/southbridge: Extract ICH9 QOM container model Philippe Mathieu-Daudé
` (4 preceding siblings ...)
2024-02-19 16:38 ` [PATCH 05/14] hw/acpi/ich9_tco: Restrict ich9_generate_smi() declaration Philippe Mathieu-Daudé
@ 2024-02-19 16:38 ` Philippe Mathieu-Daudé
2024-02-19 18:15 ` BALATON Zoltan
2024-02-20 19:25 ` Bernhard Beschow
2024-02-19 16:38 ` [PATCH 07/14] hw/southbridge/ich9: Introduce TYPE_ICH9_SOUTHBRIDGE stub Philippe Mathieu-Daudé
` (7 subsequent siblings)
13 siblings, 2 replies; 28+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-02-19 16:38 UTC (permalink / raw)
To: qemu-devel, Bernhard Beschow
Cc: Michael S. Tsirkin, Ani Sinha, Richard Henderson, Igor Mammedov,
Mark Cave-Ayland, Laurent Vivier, Thomas Huth, Marcel Apfelbaum,
Eduardo Habkost, Paolo Bonzini, BALATON Zoltan,
Philippe Mathieu-Daudé
Expose TYPE_ICH_DMI_PCI_BRIDGE to the new
"hw/pci-bridge/ich_dmi_pci.h" header.
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
MAINTAINERS | 1 +
include/hw/pci-bridge/ich_dmi_pci.h | 20 ++++++++++++++++++++
include/hw/southbridge/ich9.h | 2 --
hw/pci-bridge/i82801b11.c | 11 ++++-------
4 files changed, 25 insertions(+), 9 deletions(-)
create mode 100644 include/hw/pci-bridge/ich_dmi_pci.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 1b210c5cc1..50507c3dd6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2609,6 +2609,7 @@ F: hw/acpi/ich9*.c
F: hw/i2c/smbus_ich9.c
F: hw/isa/lpc_ich9.c
F: include/hw/acpi/ich9*.h
+F: include/hw/pci-bridge/ich_dmi_pci.h
F: include/hw/southbridge/ich9.h
PIIX4 South Bridge (i82371AB)
diff --git a/include/hw/pci-bridge/ich_dmi_pci.h b/include/hw/pci-bridge/ich_dmi_pci.h
new file mode 100644
index 0000000000..7623b32b8e
--- /dev/null
+++ b/include/hw/pci-bridge/ich_dmi_pci.h
@@ -0,0 +1,20 @@
+/*
+ * QEMU ICH4 i82801b11 dmi-to-pci Bridge Emulation
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef HW_PCI_BRIDGE_ICH_D2P_H
+#define HW_PCI_BRIDGE_ICH_D2P_H
+
+#include "qom/object.h"
+#include "hw/pci/pci_bridge.h"
+
+#define TYPE_ICH_DMI_PCI_BRIDGE "i82801b11-bridge"
+OBJECT_DECLARE_SIMPLE_TYPE(I82801b11Bridge, ICH_DMI_PCI_BRIDGE)
+
+struct I82801b11Bridge {
+ PCIBridge parent_obj;
+};
+
+#endif
diff --git a/include/hw/southbridge/ich9.h b/include/hw/southbridge/ich9.h
index bee522a4cf..b2abf483e0 100644
--- a/include/hw/southbridge/ich9.h
+++ b/include/hw/southbridge/ich9.h
@@ -114,8 +114,6 @@ struct ICH9LPCState {
#define ICH9_D2P_SECONDARY_DEFAULT (256 - 8)
-#define ICH9_D2P_A2_REVISION 0x92
-
/* D31:F0 LPC Processor Interface */
#define ICH9_RST_CNT_IOPORT 0xCF9
diff --git a/hw/pci-bridge/i82801b11.c b/hw/pci-bridge/i82801b11.c
index c140919cbc..dd17e35b0a 100644
--- a/hw/pci-bridge/i82801b11.c
+++ b/hw/pci-bridge/i82801b11.c
@@ -45,7 +45,7 @@
#include "hw/pci/pci_bridge.h"
#include "migration/vmstate.h"
#include "qemu/module.h"
-#include "hw/southbridge/ich9.h"
+#include "hw/pci-bridge/ich_dmi_pci.h"
/*****************************************************************************/
/* ICH9 DMI-to-PCI bridge */
@@ -53,11 +53,8 @@
#define I82801ba_SSVID_SVID 0
#define I82801ba_SSVID_SSID 0
-typedef struct I82801b11Bridge {
- /*< private >*/
- PCIBridge parent_obj;
- /*< public >*/
-} I82801b11Bridge;
+
+#define ICH9_D2P_A2_REVISION 0x92
static void i82801b11_bridge_realize(PCIDevice *d, Error **errp)
{
@@ -103,7 +100,7 @@ static void i82801b11_bridge_class_init(ObjectClass *klass, void *data)
}
static const TypeInfo i82801b11_bridge_info = {
- .name = "i82801b11-bridge",
+ .name = TYPE_ICH_DMI_PCI_BRIDGE,
.parent = TYPE_PCI_BRIDGE,
.instance_size = sizeof(I82801b11Bridge),
.class_init = i82801b11_bridge_class_init,
--
2.41.0
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 07/14] hw/southbridge/ich9: Introduce TYPE_ICH9_SOUTHBRIDGE stub
2024-02-19 16:38 [PATCH 00/14] hw/southbridge: Extract ICH9 QOM container model Philippe Mathieu-Daudé
` (5 preceding siblings ...)
2024-02-19 16:38 ` [PATCH 06/14] hw/pci-bridge: Extract QOM ICH definitions to 'ich_dmi_pci.h' Philippe Mathieu-Daudé
@ 2024-02-19 16:38 ` Philippe Mathieu-Daudé
2024-02-19 16:38 ` [PATCH 08/14] hw/southbridge/ich9: Add the DMI-to-PCI bridge Philippe Mathieu-Daudé
` (6 subsequent siblings)
13 siblings, 0 replies; 28+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-02-19 16:38 UTC (permalink / raw)
To: qemu-devel, Bernhard Beschow
Cc: Michael S. Tsirkin, Ani Sinha, Richard Henderson, Igor Mammedov,
Mark Cave-Ayland, Laurent Vivier, Thomas Huth, Marcel Apfelbaum,
Eduardo Habkost, Paolo Bonzini, BALATON Zoltan,
Philippe Mathieu-Daudé
Start the TYPE_ICH9_SOUTHBRIDGE stub, a kind of QOM
container which will contain all the ICH9 parts.
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
MAINTAINERS | 1 +
include/hw/southbridge/ich9.h | 3 ++
hw/i386/pc_q35.c | 7 ++++
hw/southbridge/ich9.c | 61 +++++++++++++++++++++++++++++++++++
hw/Kconfig | 1 +
hw/i386/Kconfig | 1 +
hw/meson.build | 1 +
hw/southbridge/Kconfig | 5 +++
hw/southbridge/meson.build | 3 ++
9 files changed, 83 insertions(+)
create mode 100644 hw/southbridge/ich9.c
create mode 100644 hw/southbridge/Kconfig
create mode 100644 hw/southbridge/meson.build
diff --git a/MAINTAINERS b/MAINTAINERS
index 50507c3dd6..d1a2eddd4c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2608,6 +2608,7 @@ S: Supported
F: hw/acpi/ich9*.c
F: hw/i2c/smbus_ich9.c
F: hw/isa/lpc_ich9.c
+F: hw/southbridge/ich9.c
F: include/hw/acpi/ich9*.h
F: include/hw/pci-bridge/ich_dmi_pci.h
F: include/hw/southbridge/ich9.h
diff --git a/include/hw/southbridge/ich9.h b/include/hw/southbridge/ich9.h
index b2abf483e0..162ae3baa1 100644
--- a/include/hw/southbridge/ich9.h
+++ b/include/hw/southbridge/ich9.h
@@ -11,6 +11,9 @@
#include "qemu/notify.h"
#include "qom/object.h"
+#define TYPE_ICH9_SOUTHBRIDGE "ICH9-southbridge"
+OBJECT_DECLARE_SIMPLE_TYPE(ICH9State, ICH9_SOUTHBRIDGE)
+
#define ICH9_CC_SIZE (16 * 1024) /* 16KB. Chipset configuration registers */
#define TYPE_ICH9_LPC_DEVICE "ICH9-LPC"
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 621661a738..311ac2be6f 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -125,6 +125,7 @@ static void pc_q35_init(MachineState *machine)
X86MachineState *x86ms = X86_MACHINE(machine);
Object *phb;
PCIBus *host_bus;
+ DeviceState *ich9;
PCIDevice *lpc;
Object *lpc_obj;
DeviceState *lpc_dev;
@@ -233,6 +234,12 @@ static void pc_q35_init(MachineState *machine)
host_bus = PCI_BUS(qdev_get_child_bus(DEVICE(phb), "pcie.0"));
pcms->bus = host_bus;
+ ich9 = qdev_new(TYPE_ICH9_SOUTHBRIDGE);
+ object_property_add_child(OBJECT(machine), "ich9", OBJECT(ich9));
+ object_property_set_link(OBJECT(ich9), "mch-pcie-bus",
+ OBJECT(host_bus), &error_abort);
+ qdev_realize_and_unref(ich9, NULL, &error_fatal);
+
/* irq lines */
gsi_state = pc_gsi_create(&x86ms->gsi, true);
diff --git a/hw/southbridge/ich9.c b/hw/southbridge/ich9.c
new file mode 100644
index 0000000000..f3a9b932ab
--- /dev/null
+++ b/hw/southbridge/ich9.c
@@ -0,0 +1,61 @@
+/*
+ * QEMU Intel ICH9 south bridge emulation
+ *
+ * SPDX-FileCopyrightText: 2024 Linaro Ltd
+ * SPDX-FileContributor: Philippe Mathieu-Daudé
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "hw/qdev-properties.h"
+#include "hw/southbridge/ich9.h"
+#include "hw/pci/pci.h"
+
+struct ICH9State {
+ DeviceState parent_obj;
+
+ PCIBus *pci_bus;
+};
+
+static Property ich9_props[] = {
+ DEFINE_PROP_LINK("mch-pcie-bus", ICH9State, pci_bus,
+ TYPE_PCIE_BUS, PCIBus *),
+ DEFINE_PROP_END_OF_LIST(),
+};
+
+static void ich9_init(Object *obj)
+{
+}
+
+static void ich9_realize(DeviceState *dev, Error **errp)
+{
+ ICH9State *s = ICH9_SOUTHBRIDGE(dev);
+
+ if (!s->pci_bus) {
+ error_setg(errp, "'pcie-bus' property must be set");
+ return;
+ }
+}
+
+static void ich9_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+
+ dc->realize = ich9_realize;
+ device_class_set_props(dc, ich9_props);
+ set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
+}
+
+static const TypeInfo ich9_types[] = {
+ {
+ .name = TYPE_ICH9_SOUTHBRIDGE,
+ .parent = TYPE_DEVICE,
+ .instance_size = sizeof(ICH9State),
+ .instance_init = ich9_init,
+ .class_init = ich9_class_init,
+ }
+};
+
+DEFINE_TYPES(ich9_types)
diff --git a/hw/Kconfig b/hw/Kconfig
index 2c00936c28..6584f2f72a 100644
--- a/hw/Kconfig
+++ b/hw/Kconfig
@@ -36,6 +36,7 @@ source scsi/Kconfig
source sd/Kconfig
source sensor/Kconfig
source smbios/Kconfig
+source southbridge/Kconfig
source ssi/Kconfig
source timer/Kconfig
source tpm/Kconfig
diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig
index a1846be6f7..d21638f4f9 100644
--- a/hw/i386/Kconfig
+++ b/hw/i386/Kconfig
@@ -99,6 +99,7 @@ config Q35
select PC_PCI
select PC_ACPI
select PCI_EXPRESS_Q35
+ select ICH9
select LPC_ICH9
select AHCI_ICH9
select DIMM
diff --git a/hw/meson.build b/hw/meson.build
index 463d702683..7f9ae8659a 100644
--- a/hw/meson.build
+++ b/hw/meson.build
@@ -33,6 +33,7 @@ subdir('rtc')
subdir('scsi')
subdir('sd')
subdir('sensor')
+subdir('southbridge')
subdir('smbios')
subdir('ssi')
subdir('timer')
diff --git a/hw/southbridge/Kconfig b/hw/southbridge/Kconfig
new file mode 100644
index 0000000000..852b7f346f
--- /dev/null
+++ b/hw/southbridge/Kconfig
@@ -0,0 +1,5 @@
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+config ICH9
+ bool
+ depends on PCI_EXPRESS
diff --git a/hw/southbridge/meson.build b/hw/southbridge/meson.build
new file mode 100644
index 0000000000..70c1fa3cb2
--- /dev/null
+++ b/hw/southbridge/meson.build
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+system_ss.add(when: 'CONFIG_ICH9', if_true: files('ich9.c'))
--
2.41.0
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 08/14] hw/southbridge/ich9: Add the DMI-to-PCI bridge
2024-02-19 16:38 [PATCH 00/14] hw/southbridge: Extract ICH9 QOM container model Philippe Mathieu-Daudé
` (6 preceding siblings ...)
2024-02-19 16:38 ` [PATCH 07/14] hw/southbridge/ich9: Introduce TYPE_ICH9_SOUTHBRIDGE stub Philippe Mathieu-Daudé
@ 2024-02-19 16:38 ` Philippe Mathieu-Daudé
2024-02-19 16:38 ` [PATCH 09/14] hw/southbridge/ich9: Add a AHCI function Philippe Mathieu-Daudé
` (5 subsequent siblings)
13 siblings, 0 replies; 28+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-02-19 16:38 UTC (permalink / raw)
To: qemu-devel, Bernhard Beschow
Cc: Michael S. Tsirkin, Ani Sinha, Richard Henderson, Igor Mammedov,
Mark Cave-Ayland, Laurent Vivier, Thomas Huth, Marcel Apfelbaum,
Eduardo Habkost, Paolo Bonzini, BALATON Zoltan,
Philippe Mathieu-Daudé
Instantiate TYPE_ICH_DMI_PCI_BRIDGE in TYPE_ICH9_SOUTHBRIDGE.
Since the Q35 machine doesn't use it, add the 'd2p-enabled'
property to disable it.
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
include/hw/southbridge/ich9.h | 9 ---------
hw/i386/pc_q35.c | 1 +
hw/southbridge/ich9.c | 27 +++++++++++++++++++++++++++
hw/southbridge/Kconfig | 1 +
4 files changed, 29 insertions(+), 9 deletions(-)
diff --git a/include/hw/southbridge/ich9.h b/include/hw/southbridge/ich9.h
index 162ae3baa1..b9122d299d 100644
--- a/include/hw/southbridge/ich9.h
+++ b/include/hw/southbridge/ich9.h
@@ -108,15 +108,6 @@ struct ICH9LPCState {
#define ICH9_USB_UHCI1_DEV 29
#define ICH9_USB_UHCI1_FUNC 0
-/* D30:F0 DMI-to-PCI bridge */
-#define ICH9_D2P_BRIDGE "ICH9 D2P BRIDGE"
-#define ICH9_D2P_BRIDGE_SAVEVM_VERSION 0
-
-#define ICH9_D2P_BRIDGE_DEV 30
-#define ICH9_D2P_BRIDGE_FUNC 0
-
-#define ICH9_D2P_SECONDARY_DEFAULT (256 - 8)
-
/* D31:F0 LPC Processor Interface */
#define ICH9_RST_CNT_IOPORT 0xCF9
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 311ac2be6f..2f15af540f 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -238,6 +238,7 @@ static void pc_q35_init(MachineState *machine)
object_property_add_child(OBJECT(machine), "ich9", OBJECT(ich9));
object_property_set_link(OBJECT(ich9), "mch-pcie-bus",
OBJECT(host_bus), &error_abort);
+ qdev_prop_set_bit(ich9, "d2p-enabled", false);
qdev_realize_and_unref(ich9, NULL, &error_fatal);
/* irq lines */
diff --git a/hw/southbridge/ich9.c b/hw/southbridge/ich9.c
index f3a9b932ab..6df47e81fb 100644
--- a/hw/southbridge/ich9.c
+++ b/hw/southbridge/ich9.c
@@ -12,19 +12,42 @@
#include "hw/qdev-properties.h"
#include "hw/southbridge/ich9.h"
#include "hw/pci/pci.h"
+#include "hw/pci-bridge/ich_dmi_pci.h"
+
+#define ICH9_D2P_DEVFN PCI_DEVFN(30, 0)
struct ICH9State {
DeviceState parent_obj;
+ I82801b11Bridge d2p;
+
PCIBus *pci_bus;
+ bool d2p_enabled;
};
static Property ich9_props[] = {
DEFINE_PROP_LINK("mch-pcie-bus", ICH9State, pci_bus,
TYPE_PCIE_BUS, PCIBus *),
+ DEFINE_PROP_BOOL("d2p-enabled", ICH9State, d2p_enabled, true),
DEFINE_PROP_END_OF_LIST(),
};
+static bool ich9_realize_d2p(ICH9State *s, Error **errp)
+{
+ if (!module_object_class_by_name(TYPE_ICH_DMI_PCI_BRIDGE)) {
+ error_setg(errp, "DMI-to-PCI function not available in this build");
+ return false;
+ }
+ object_initialize_child(OBJECT(s), "d2p", &s->d2p, TYPE_ICH_DMI_PCI_BRIDGE);
+ qdev_prop_set_int32(DEVICE(&s->d2p), "addr", ICH9_D2P_DEVFN);
+ if (!qdev_realize(DEVICE(&s->d2p), BUS(s->pci_bus), errp)) {
+ return false;
+ }
+ object_property_add_alias(OBJECT(s), "pci.0", OBJECT(&s->d2p), "pci.0");
+
+ return true;
+}
+
static void ich9_init(Object *obj)
{
}
@@ -37,6 +60,10 @@ static void ich9_realize(DeviceState *dev, Error **errp)
error_setg(errp, "'pcie-bus' property must be set");
return;
}
+
+ if (s->d2p_enabled && !ich9_realize_d2p(s, errp)) {
+ return;
+ }
}
static void ich9_class_init(ObjectClass *klass, void *data)
diff --git a/hw/southbridge/Kconfig b/hw/southbridge/Kconfig
index 852b7f346f..db7259bf6f 100644
--- a/hw/southbridge/Kconfig
+++ b/hw/southbridge/Kconfig
@@ -3,3 +3,4 @@
config ICH9
bool
depends on PCI_EXPRESS
+ imply I82801B11
--
2.41.0
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 09/14] hw/southbridge/ich9: Add a AHCI function
2024-02-19 16:38 [PATCH 00/14] hw/southbridge: Extract ICH9 QOM container model Philippe Mathieu-Daudé
` (7 preceding siblings ...)
2024-02-19 16:38 ` [PATCH 08/14] hw/southbridge/ich9: Add the DMI-to-PCI bridge Philippe Mathieu-Daudé
@ 2024-02-19 16:38 ` Philippe Mathieu-Daudé
2024-02-19 18:31 ` BALATON Zoltan
2024-02-19 16:38 ` [PATCH 10/14] hw/i2c/smbus: Extract QOM ICH9 definitions to 'smbus_ich9.h' Philippe Mathieu-Daudé
` (4 subsequent siblings)
13 siblings, 1 reply; 28+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-02-19 16:38 UTC (permalink / raw)
To: qemu-devel, Bernhard Beschow
Cc: Michael S. Tsirkin, Ani Sinha, Richard Henderson, Igor Mammedov,
Mark Cave-Ayland, Laurent Vivier, Thomas Huth, Marcel Apfelbaum,
Eduardo Habkost, Paolo Bonzini, BALATON Zoltan,
Philippe Mathieu-Daudé
Instantiate TYPE_ICH9_AHCI in TYPE_ICH9_SOUTHBRIDGE.
Since the PC machines can disable SATA (see the
PC_MACHINE_SATA dynamic property), add the 'sata-enabled'
property to disable it.
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
MAINTAINERS | 2 ++
include/hw/southbridge/ich9.h | 4 ----
hw/i386/pc_q35.c | 25 ++++---------------------
hw/southbridge/ich9.c | 35 +++++++++++++++++++++++++++++++++++
hw/i386/Kconfig | 1 -
hw/southbridge/Kconfig | 1 +
6 files changed, 42 insertions(+), 26 deletions(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index d1a2eddd4c..937ebb5c96 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2607,9 +2607,11 @@ M: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
S: Supported
F: hw/acpi/ich9*.c
F: hw/i2c/smbus_ich9.c
+F: hw/ide/ich.c
F: hw/isa/lpc_ich9.c
F: hw/southbridge/ich9.c
F: include/hw/acpi/ich9*.h
+F: include/hw/ide/ahci-pci.h
F: include/hw/pci-bridge/ich_dmi_pci.h
F: include/hw/southbridge/ich9.h
diff --git a/include/hw/southbridge/ich9.h b/include/hw/southbridge/ich9.h
index b9122d299d..ac7f9f4ff5 100644
--- a/include/hw/southbridge/ich9.h
+++ b/include/hw/southbridge/ich9.h
@@ -166,10 +166,6 @@ struct ICH9LPCState {
#define ICH9_GPIO_GSI "gsi"
-/* D31:F2 SATA Controller #1 */
-#define ICH9_SATA1_DEV 31
-#define ICH9_SATA1_FUNC 2
-
/* D31:F0 power management I/O registers
offset from the address ICH9_LPC_PMBASE */
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 2f15af540f..060358d449 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -61,9 +61,6 @@
#include "hw/acpi/acpi.h"
#include "target/i386/cpu.h"
-/* ICH9 AHCI has 6 ports */
-#define MAX_SATA_PORTS 6
-
struct ehci_companions {
const char *name;
int func;
@@ -129,7 +126,7 @@ static void pc_q35_init(MachineState *machine)
PCIDevice *lpc;
Object *lpc_obj;
DeviceState *lpc_dev;
- BusState *idebus[MAX_SATA_PORTS];
+ BusState *idebus[2] = { };
ISADevice *rtc_state;
MemoryRegion *system_memory = get_system_memory();
MemoryRegion *system_io = get_system_io();
@@ -138,7 +135,6 @@ static void pc_q35_init(MachineState *machine)
ISABus *isa_bus;
int i;
ram_addr_t lowmem;
- DriveInfo *hd[MAX_SATA_PORTS];
MachineClass *mc = MACHINE_GET_CLASS(machine);
bool acpi_pcihp;
bool keep_pci_slot_hpc;
@@ -239,6 +235,7 @@ static void pc_q35_init(MachineState *machine)
object_property_set_link(OBJECT(ich9), "mch-pcie-bus",
OBJECT(host_bus), &error_abort);
qdev_prop_set_bit(ich9, "d2p-enabled", false);
+ qdev_prop_set_bit(ich9, "sata-enabled", pcms->sata_enabled);
qdev_realize_and_unref(ich9, NULL, &error_fatal);
/* irq lines */
@@ -302,22 +299,8 @@ static void pc_q35_init(MachineState *machine)
0xff0104);
if (pcms->sata_enabled) {
- PCIDevice *pdev;
- AHCIPCIState *ich9;
-
- /* ahci and SATA device, for q35 1 ahci controller is built-in */
- pdev = pci_create_simple_multifunction(host_bus,
- PCI_DEVFN(ICH9_SATA1_DEV,
- ICH9_SATA1_FUNC),
- "ich9-ahci");
- ich9 = ICH9_AHCI(pdev);
- idebus[0] = qdev_get_child_bus(DEVICE(pdev), "ide.0");
- idebus[1] = qdev_get_child_bus(DEVICE(pdev), "ide.1");
- g_assert(MAX_SATA_PORTS == ich9->ahci.ports);
- ide_drive_get(hd, ich9->ahci.ports);
- ahci_ide_create_devs(&ich9->ahci, hd);
- } else {
- idebus[0] = idebus[1] = NULL;
+ idebus[0] = qdev_get_child_bus(ich9, "ide.0");
+ idebus[1] = qdev_get_child_bus(ich9, "ide.1");
}
if (machine_usb(machine)) {
diff --git a/hw/southbridge/ich9.c b/hw/southbridge/ich9.c
index 6df47e81fb..233dc1c5d7 100644
--- a/hw/southbridge/ich9.c
+++ b/hw/southbridge/ich9.c
@@ -13,22 +13,30 @@
#include "hw/southbridge/ich9.h"
#include "hw/pci/pci.h"
#include "hw/pci-bridge/ich_dmi_pci.h"
+#include "hw/ide/ahci-pci.h"
+#include "hw/ide.h"
#define ICH9_D2P_DEVFN PCI_DEVFN(30, 0)
+#define ICH9_SATA1_DEVFN PCI_DEVFN(31, 2)
+
+#define SATA_PORTS 6
struct ICH9State {
DeviceState parent_obj;
I82801b11Bridge d2p;
+ AHCIPCIState sata0;
PCIBus *pci_bus;
bool d2p_enabled;
+ bool sata_enabled;
};
static Property ich9_props[] = {
DEFINE_PROP_LINK("mch-pcie-bus", ICH9State, pci_bus,
TYPE_PCIE_BUS, PCIBus *),
DEFINE_PROP_BOOL("d2p-enabled", ICH9State, d2p_enabled, true),
+ DEFINE_PROP_BOOL("sata-enabled", ICH9State, sata_enabled, true),
DEFINE_PROP_END_OF_LIST(),
};
@@ -48,6 +56,29 @@ static bool ich9_realize_d2p(ICH9State *s, Error **errp)
return true;
}
+static bool ich9_realize_sata(ICH9State *s, Error **errp)
+{
+ DriveInfo *hd[SATA_PORTS];
+
+ object_initialize_child(OBJECT(s), "sata[0]", &s->sata0, TYPE_ICH9_AHCI);
+ qdev_prop_set_int32(DEVICE(&s->sata0), "addr", ICH9_SATA1_DEVFN);
+ if (!qdev_realize(DEVICE(&s->sata0), BUS(s->pci_bus), errp)) {
+ return false;
+ }
+ for (unsigned i = 0; i < SATA_PORTS; i++) {
+ g_autofree char *portname = g_strdup_printf("ide.%u", i);
+
+ object_property_add_alias(OBJECT(s), portname,
+ OBJECT(&s->sata0), portname);
+ }
+
+ g_assert(SATA_PORTS == s->sata0.ahci.ports);
+ ide_drive_get(hd, s->sata0.ahci.ports);
+ ahci_ide_create_devs(&s->sata0.ahci, hd);
+
+ return true;
+}
+
static void ich9_init(Object *obj)
{
}
@@ -64,6 +95,10 @@ static void ich9_realize(DeviceState *dev, Error **errp)
if (s->d2p_enabled && !ich9_realize_d2p(s, errp)) {
return;
}
+
+ if (s->sata_enabled && !ich9_realize_sata(s, errp)) {
+ return;
+ }
}
static void ich9_class_init(ObjectClass *klass, void *data)
diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig
index d21638f4f9..226d7f6916 100644
--- a/hw/i386/Kconfig
+++ b/hw/i386/Kconfig
@@ -101,7 +101,6 @@ config Q35
select PCI_EXPRESS_Q35
select ICH9
select LPC_ICH9
- select AHCI_ICH9
select DIMM
select SMBIOS
select FW_CFG_DMA
diff --git a/hw/southbridge/Kconfig b/hw/southbridge/Kconfig
index db7259bf6f..f806033d48 100644
--- a/hw/southbridge/Kconfig
+++ b/hw/southbridge/Kconfig
@@ -4,3 +4,4 @@ config ICH9
bool
depends on PCI_EXPRESS
imply I82801B11
+ select AHCI_ICH9
--
2.41.0
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 10/14] hw/i2c/smbus: Extract QOM ICH9 definitions to 'smbus_ich9.h'
2024-02-19 16:38 [PATCH 00/14] hw/southbridge: Extract ICH9 QOM container model Philippe Mathieu-Daudé
` (8 preceding siblings ...)
2024-02-19 16:38 ` [PATCH 09/14] hw/southbridge/ich9: Add a AHCI function Philippe Mathieu-Daudé
@ 2024-02-19 16:38 ` Philippe Mathieu-Daudé
2024-02-19 16:38 ` [PATCH 11/14] hw/southbridge/ich9: Add the SMBus function Philippe Mathieu-Daudé
` (3 subsequent siblings)
13 siblings, 0 replies; 28+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-02-19 16:38 UTC (permalink / raw)
To: qemu-devel, Bernhard Beschow
Cc: Michael S. Tsirkin, Ani Sinha, Richard Henderson, Igor Mammedov,
Mark Cave-Ayland, Laurent Vivier, Thomas Huth, Marcel Apfelbaum,
Eduardo Habkost, Paolo Bonzini, BALATON Zoltan,
Philippe Mathieu-Daudé
Expose TYPE_ICH9_SMB_DEVICE to the new "hw/i2c/smbus_ich9.h"
header.
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
MAINTAINERS | 1 +
include/hw/i2c/smbus_ich9.h | 25 +++++++++++++++++++++++++
hw/i2c/smbus_ich9.c | 15 ++-------------
3 files changed, 28 insertions(+), 13 deletions(-)
create mode 100644 include/hw/i2c/smbus_ich9.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 937ebb5c96..b896d953af 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2611,6 +2611,7 @@ F: hw/ide/ich.c
F: hw/isa/lpc_ich9.c
F: hw/southbridge/ich9.c
F: include/hw/acpi/ich9*.h
+F: include/hw/i2c/smbus_ich9.h
F: include/hw/ide/ahci-pci.h
F: include/hw/pci-bridge/ich_dmi_pci.h
F: include/hw/southbridge/ich9.h
diff --git a/include/hw/i2c/smbus_ich9.h b/include/hw/i2c/smbus_ich9.h
new file mode 100644
index 0000000000..d716cbca33
--- /dev/null
+++ b/include/hw/i2c/smbus_ich9.h
@@ -0,0 +1,25 @@
+/*
+ * QEMU ICH9 SMBus emulation
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#ifndef HW_I2C_SMBUS_ICH9_H
+#define HW_I2C_SMBUS_ICH9_H
+
+#include "qom/object.h"
+#include "hw/pci/pci_device.h"
+#include "hw/i2c/pm_smbus.h"
+
+#define TYPE_ICH9_SMB_DEVICE "ICH9-SMB"
+
+OBJECT_DECLARE_SIMPLE_TYPE(ICH9SMBState, ICH9_SMB_DEVICE)
+
+struct ICH9SMBState {
+ PCIDevice dev;
+
+ bool irq_enabled;
+
+ PMSMBus smb;
+};
+
+#endif
diff --git a/hw/i2c/smbus_ich9.c b/hw/i2c/smbus_ich9.c
index 208f263ac5..3980bca4c5 100644
--- a/hw/i2c/smbus_ich9.c
+++ b/hw/i2c/smbus_ich9.c
@@ -1,5 +1,5 @@
/*
- * ACPI implementation
+ * QEMU ICH9 SMBus emulation
*
* Copyright (c) 2006 Fabrice Bellard
* Copyright (c) 2009 Isaku Yamahata <yamahata at valinux co jp>
@@ -22,8 +22,7 @@
#include "qemu/osdep.h"
#include "qemu/range.h"
-#include "hw/i2c/pm_smbus.h"
-#include "hw/pci/pci.h"
+#include "hw/i2c/smbus_ich9.h"
#include "migration/vmstate.h"
#include "qemu/module.h"
@@ -31,16 +30,6 @@
#include "qom/object.h"
#include "hw/acpi/acpi_aml_interface.h"
-OBJECT_DECLARE_SIMPLE_TYPE(ICH9SMBState, ICH9_SMB_DEVICE)
-
-struct ICH9SMBState {
- PCIDevice dev;
-
- bool irq_enabled;
-
- PMSMBus smb;
-};
-
static bool ich9_vmstate_need_smbus(void *opaque, int version_id)
{
return pm_smbus_vmstate_needed();
--
2.41.0
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 11/14] hw/southbridge/ich9: Add the SMBus function
2024-02-19 16:38 [PATCH 00/14] hw/southbridge: Extract ICH9 QOM container model Philippe Mathieu-Daudé
` (9 preceding siblings ...)
2024-02-19 16:38 ` [PATCH 10/14] hw/i2c/smbus: Extract QOM ICH9 definitions to 'smbus_ich9.h' Philippe Mathieu-Daudé
@ 2024-02-19 16:38 ` Philippe Mathieu-Daudé
2024-02-19 16:38 ` [PATCH 12/14] hw/southbridge/ich9: Add the USB EHCI/UHCI functions Philippe Mathieu-Daudé
` (2 subsequent siblings)
13 siblings, 0 replies; 28+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-02-19 16:38 UTC (permalink / raw)
To: qemu-devel, Bernhard Beschow
Cc: Michael S. Tsirkin, Ani Sinha, Richard Henderson, Igor Mammedov,
Mark Cave-Ayland, Laurent Vivier, Thomas Huth, Marcel Apfelbaum,
Eduardo Habkost, Paolo Bonzini, BALATON Zoltan,
Philippe Mathieu-Daudé
Instantiate TYPE_ICH9_SMB_DEVICE in TYPE_ICH9_SOUTHBRIDGE.
Since the PC machines can disable SMBus (see the
PC_MACHINE_SMBUS dynamic property), add the 'smbus-enabled'
property to disable it.
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
include/hw/southbridge/ich9.h | 32 --------------------------------
hw/i2c/smbus_ich9.c | 29 ++++++++++++++++++++++++++++-
hw/i386/pc_q35.c | 10 ++--------
hw/southbridge/ich9.c | 21 +++++++++++++++++++++
hw/southbridge/Kconfig | 1 +
5 files changed, 52 insertions(+), 41 deletions(-)
diff --git a/include/hw/southbridge/ich9.h b/include/hw/southbridge/ich9.h
index ac7f9f4ff5..d4b299bf3c 100644
--- a/include/hw/southbridge/ich9.h
+++ b/include/hw/southbridge/ich9.h
@@ -173,38 +173,6 @@ struct ICH9LPCState {
#define ICH9_APM_ACPI_ENABLE 0x2
#define ICH9_APM_ACPI_DISABLE 0x3
-
-/* D31:F3 SMBus controller */
-#define TYPE_ICH9_SMB_DEVICE "ICH9-SMB"
-
-#define ICH9_A2_SMB_REVISION 0x02
-#define ICH9_SMB_PI 0x00
-
-#define ICH9_SMB_SMBMBAR0 0x10
-#define ICH9_SMB_SMBMBAR1 0x14
-#define ICH9_SMB_SMBM_BAR 0
-#define ICH9_SMB_SMBM_SIZE (1 << 8)
-#define ICH9_SMB_SMB_BASE 0x20
-#define ICH9_SMB_SMB_BASE_BAR 4
-#define ICH9_SMB_SMB_BASE_SIZE (1 << 5)
-#define ICH9_SMB_HOSTC 0x40
-#define ICH9_SMB_HOSTC_SSRESET ((uint8_t)(1 << 3))
-#define ICH9_SMB_HOSTC_I2C_EN ((uint8_t)(1 << 2))
-#define ICH9_SMB_HOSTC_SMB_SMI_EN ((uint8_t)(1 << 1))
-#define ICH9_SMB_HOSTC_HST_EN ((uint8_t)(1 << 0))
-
-/* D31:F3 SMBus I/O and memory mapped I/O registers */
-#define ICH9_SMB_DEV 31
-#define ICH9_SMB_FUNC 3
-
-#define ICH9_SMB_HST_STS 0x00
-#define ICH9_SMB_HST_CNT 0x02
-#define ICH9_SMB_HST_CMD 0x03
-#define ICH9_SMB_XMIT_SLVA 0x04
-#define ICH9_SMB_HST_D0 0x05
-#define ICH9_SMB_HST_D1 0x06
-#define ICH9_SMB_HOST_BLOCK_DB 0x07
-
#define ICH9_LPC_SMI_NEGOTIATED_FEAT_PROP "x-smi-negotiated-features"
/* bit positions used in fw_cfg SMI feature negotiation */
diff --git a/hw/i2c/smbus_ich9.c b/hw/i2c/smbus_ich9.c
index 3980bca4c5..2c18278090 100644
--- a/hw/i2c/smbus_ich9.c
+++ b/hw/i2c/smbus_ich9.c
@@ -26,10 +26,37 @@
#include "migration/vmstate.h"
#include "qemu/module.h"
-#include "hw/southbridge/ich9.h"
#include "qom/object.h"
#include "hw/acpi/acpi_aml_interface.h"
+/* D31:F3 SMBus controller */
+
+#define ICH9_A2_SMB_REVISION 0x02
+#define ICH9_SMB_PI 0x00
+
+#define ICH9_SMB_SMBMBAR0 0x10
+#define ICH9_SMB_SMBMBAR1 0x14
+#define ICH9_SMB_SMBM_BAR 0
+#define ICH9_SMB_SMBM_SIZE (1 << 8)
+#define ICH9_SMB_SMB_BASE 0x20
+#define ICH9_SMB_SMB_BASE_BAR 4
+#define ICH9_SMB_SMB_BASE_SIZE (1 << 5)
+#define ICH9_SMB_HOSTC 0x40
+#define ICH9_SMB_HOSTC_SSRESET ((uint8_t)(1 << 3))
+#define ICH9_SMB_HOSTC_I2C_EN ((uint8_t)(1 << 2))
+#define ICH9_SMB_HOSTC_SMB_SMI_EN ((uint8_t)(1 << 1))
+#define ICH9_SMB_HOSTC_HST_EN ((uint8_t)(1 << 0))
+
+/* D31:F3 SMBus I/O and memory mapped I/O registers */
+
+#define ICH9_SMB_HST_STS 0x00
+#define ICH9_SMB_HST_CNT 0x02
+#define ICH9_SMB_HST_CMD 0x03
+#define ICH9_SMB_XMIT_SLVA 0x04
+#define ICH9_SMB_HST_D0 0x05
+#define ICH9_SMB_HST_D1 0x06
+#define ICH9_SMB_HOST_BLOCK_DB 0x07
+
static bool ich9_vmstate_need_smbus(void *opaque, int version_id)
{
return pm_smbus_vmstate_needed();
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 060358d449..7f6ced8a6e 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -236,6 +236,7 @@ static void pc_q35_init(MachineState *machine)
OBJECT(host_bus), &error_abort);
qdev_prop_set_bit(ich9, "d2p-enabled", false);
qdev_prop_set_bit(ich9, "sata-enabled", pcms->sata_enabled);
+ qdev_prop_set_bit(ich9, "smbus-enabled", pcms->smbus_enabled);
qdev_realize_and_unref(ich9, NULL, &error_fatal);
/* irq lines */
@@ -309,15 +310,8 @@ static void pc_q35_init(MachineState *machine)
}
if (pcms->smbus_enabled) {
- PCIDevice *smb;
-
+ pcms->smbus = I2C_BUS(qdev_get_child_bus(ich9, "i2c"));
/* TODO: Populate SPD eeprom data. */
- smb = pci_create_simple_multifunction(host_bus,
- PCI_DEVFN(ICH9_SMB_DEV,
- ICH9_SMB_FUNC),
- TYPE_ICH9_SMB_DEVICE);
- pcms->smbus = I2C_BUS(qdev_get_child_bus(DEVICE(smb), "i2c"));
-
smbus_eeprom_init(pcms->smbus, 8, NULL, 0);
}
diff --git a/hw/southbridge/ich9.c b/hw/southbridge/ich9.c
index 233dc1c5d7..4d2c298666 100644
--- a/hw/southbridge/ich9.c
+++ b/hw/southbridge/ich9.c
@@ -15,9 +15,11 @@
#include "hw/pci-bridge/ich_dmi_pci.h"
#include "hw/ide/ahci-pci.h"
#include "hw/ide.h"
+#include "hw/i2c/smbus_ich9.h"
#define ICH9_D2P_DEVFN PCI_DEVFN(30, 0)
#define ICH9_SATA1_DEVFN PCI_DEVFN(31, 2)
+#define ICH9_SMB_DEVFN PCI_DEVFN(31, 3)
#define SATA_PORTS 6
@@ -26,10 +28,12 @@ struct ICH9State {
I82801b11Bridge d2p;
AHCIPCIState sata0;
+ ICH9SMBState smb;
PCIBus *pci_bus;
bool d2p_enabled;
bool sata_enabled;
+ bool smbus_enabled;
};
static Property ich9_props[] = {
@@ -37,6 +41,7 @@ static Property ich9_props[] = {
TYPE_PCIE_BUS, PCIBus *),
DEFINE_PROP_BOOL("d2p-enabled", ICH9State, d2p_enabled, true),
DEFINE_PROP_BOOL("sata-enabled", ICH9State, sata_enabled, true),
+ DEFINE_PROP_BOOL("smbus-enabled", ICH9State, smbus_enabled, true),
DEFINE_PROP_END_OF_LIST(),
};
@@ -79,6 +84,18 @@ static bool ich9_realize_sata(ICH9State *s, Error **errp)
return true;
}
+static bool ich9_realize_smbus(ICH9State *s, Error **errp)
+{
+ object_initialize_child(OBJECT(s), "smb", &s->smb, TYPE_ICH9_SMB_DEVICE);
+ qdev_prop_set_int32(DEVICE(&s->smb), "addr", ICH9_SMB_DEVFN);
+ if (!qdev_realize(DEVICE(&s->smb), BUS(s->pci_bus), errp)) {
+ return false;
+ }
+ object_property_add_alias(OBJECT(s), "i2c", OBJECT(&s->smb), "i2c");
+
+ return true;
+}
+
static void ich9_init(Object *obj)
{
}
@@ -99,6 +116,10 @@ static void ich9_realize(DeviceState *dev, Error **errp)
if (s->sata_enabled && !ich9_realize_sata(s, errp)) {
return;
}
+
+ if (s->smbus_enabled && !ich9_realize_smbus(s, errp)) {
+ return;
+ }
}
static void ich9_class_init(ObjectClass *klass, void *data)
diff --git a/hw/southbridge/Kconfig b/hw/southbridge/Kconfig
index f806033d48..03e89a55d1 100644
--- a/hw/southbridge/Kconfig
+++ b/hw/southbridge/Kconfig
@@ -5,3 +5,4 @@ config ICH9
depends on PCI_EXPRESS
imply I82801B11
select AHCI_ICH9
+ select ACPI_ICH9
--
2.41.0
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 12/14] hw/southbridge/ich9: Add the USB EHCI/UHCI functions
2024-02-19 16:38 [PATCH 00/14] hw/southbridge: Extract ICH9 QOM container model Philippe Mathieu-Daudé
` (10 preceding siblings ...)
2024-02-19 16:38 ` [PATCH 11/14] hw/southbridge/ich9: Add the SMBus function Philippe Mathieu-Daudé
@ 2024-02-19 16:38 ` Philippe Mathieu-Daudé
2024-02-19 16:38 ` [PATCH 13/14] hw/southbridge/ich9: Extract LPC definitions to 'hw/isa/ich9_lpc.h' Philippe Mathieu-Daudé
2024-02-19 16:38 ` [PATCH 14/14] hw/southbridge/ich9: Add the LPC / ISA bridge function Philippe Mathieu-Daudé
13 siblings, 0 replies; 28+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-02-19 16:38 UTC (permalink / raw)
To: qemu-devel, Bernhard Beschow
Cc: Michael S. Tsirkin, Ani Sinha, Richard Henderson, Igor Mammedov,
Mark Cave-Ayland, Laurent Vivier, Thomas Huth, Marcel Apfelbaum,
Eduardo Habkost, Paolo Bonzini, BALATON Zoltan,
Philippe Mathieu-Daudé
Instantiate EHCI and UHCI in TYPE_ICH9_SOUTHBRIDGE.
Since machines can disable USB, add the 'ehci-count'
property. Machine can disable USB functions by setting
ehci-count=0.
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
include/hw/southbridge/ich9.h | 5 ---
hw/i386/pc_q35.c | 62 ++---------------------------------
hw/southbridge/ich9.c | 54 ++++++++++++++++++++++++++++++
hw/southbridge/Kconfig | 2 ++
4 files changed, 58 insertions(+), 65 deletions(-)
diff --git a/include/hw/southbridge/ich9.h b/include/hw/southbridge/ich9.h
index d4b299bf3c..7e75496b0b 100644
--- a/include/hw/southbridge/ich9.h
+++ b/include/hw/southbridge/ich9.h
@@ -103,11 +103,6 @@ struct ICH9LPCState {
#define ICH9_PCIE_DEV 28
#define ICH9_PCIE_FUNC_MAX 6
-
-/* D29:F0 USB UHCI Controller #1 */
-#define ICH9_USB_UHCI1_DEV 29
-#define ICH9_USB_UHCI1_FUNC 0
-
/* D31:F0 LPC Processor Interface */
#define ICH9_RST_CNT_IOPORT 0xCF9
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 7f6ced8a6e..e5f5bb0db1 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -50,8 +50,6 @@
#include "hw/ide/ahci-pci.h"
#include "hw/intc/ioapic.h"
#include "hw/southbridge/ich9.h"
-#include "hw/usb.h"
-#include "hw/usb/hcd-uhci.h"
#include "qapi/error.h"
#include "qemu/error-report.h"
#include "sysemu/numa.h"
@@ -61,59 +59,6 @@
#include "hw/acpi/acpi.h"
#include "target/i386/cpu.h"
-struct ehci_companions {
- const char *name;
- int func;
- int port;
-};
-
-static const struct ehci_companions ich9_1d[] = {
- { .name = TYPE_ICH9_USB_UHCI(1), .func = 0, .port = 0 },
- { .name = TYPE_ICH9_USB_UHCI(2), .func = 1, .port = 2 },
- { .name = TYPE_ICH9_USB_UHCI(3), .func = 2, .port = 4 },
-};
-
-static const struct ehci_companions ich9_1a[] = {
- { .name = TYPE_ICH9_USB_UHCI(4), .func = 0, .port = 0 },
- { .name = TYPE_ICH9_USB_UHCI(5), .func = 1, .port = 2 },
- { .name = TYPE_ICH9_USB_UHCI(6), .func = 2, .port = 4 },
-};
-
-static int ehci_create_ich9_with_companions(PCIBus *bus, int slot)
-{
- const struct ehci_companions *comp;
- PCIDevice *ehci, *uhci;
- BusState *usbbus;
- const char *name;
- int i;
-
- switch (slot) {
- case 0x1d:
- name = "ich9-usb-ehci1";
- comp = ich9_1d;
- break;
- case 0x1a:
- name = "ich9-usb-ehci2";
- comp = ich9_1a;
- break;
- default:
- return -1;
- }
-
- ehci = pci_new_multifunction(PCI_DEVFN(slot, 7), name);
- pci_realize_and_unref(ehci, bus, &error_fatal);
- usbbus = QLIST_FIRST(&ehci->qdev.child_bus);
-
- for (i = 0; i < 3; i++) {
- uhci = pci_new_multifunction(PCI_DEVFN(slot, comp[i].func),
- comp[i].name);
- qdev_prop_set_string(&uhci->qdev, "masterbus", usbbus->name);
- qdev_prop_set_uint32(&uhci->qdev, "firstport", comp[i].port);
- pci_realize_and_unref(uhci, bus, &error_fatal);
- }
- return 0;
-}
-
/* PC hardware initialisation */
static void pc_q35_init(MachineState *machine)
{
@@ -237,6 +182,8 @@ static void pc_q35_init(MachineState *machine)
qdev_prop_set_bit(ich9, "d2p-enabled", false);
qdev_prop_set_bit(ich9, "sata-enabled", pcms->sata_enabled);
qdev_prop_set_bit(ich9, "smbus-enabled", pcms->smbus_enabled);
+ /* Should we create 6 UHCI according to ich9 spec? */
+ qdev_prop_set_uint8(ich9, "ehci-count", machine_usb(machine) ? 1 : 0);
qdev_realize_and_unref(ich9, NULL, &error_fatal);
/* irq lines */
@@ -304,11 +251,6 @@ static void pc_q35_init(MachineState *machine)
idebus[1] = qdev_get_child_bus(ich9, "ide.1");
}
- if (machine_usb(machine)) {
- /* Should we create 6 UHCI according to ich9 spec? */
- ehci_create_ich9_with_companions(host_bus, 0x1d);
- }
-
if (pcms->smbus_enabled) {
pcms->smbus = I2C_BUS(qdev_get_child_bus(ich9, "i2c"));
/* TODO: Populate SPD eeprom data. */
diff --git a/hw/southbridge/ich9.c b/hw/southbridge/ich9.c
index 4d2c298666..085d75e569 100644
--- a/hw/southbridge/ich9.c
+++ b/hw/southbridge/ich9.c
@@ -16,12 +16,18 @@
#include "hw/ide/ahci-pci.h"
#include "hw/ide.h"
#include "hw/i2c/smbus_ich9.h"
+#include "hw/usb.h"
+#include "hw/usb/hcd-ehci.h"
+#include "hw/usb/hcd-uhci.h"
#define ICH9_D2P_DEVFN PCI_DEVFN(30, 0)
#define ICH9_SATA1_DEVFN PCI_DEVFN(31, 2)
#define ICH9_SMB_DEVFN PCI_DEVFN(31, 3)
+#define ICH9_EHCI_FUNC 7
#define SATA_PORTS 6
+#define EHCI_PER_FN 2
+#define UHCI_PER_FN 3
struct ICH9State {
DeviceState parent_obj;
@@ -29,11 +35,14 @@ struct ICH9State {
I82801b11Bridge d2p;
AHCIPCIState sata0;
ICH9SMBState smb;
+ EHCIPCIState ehci[EHCI_PER_FN];
+ UHCIState uhci[EHCI_PER_FN * UHCI_PER_FN];
PCIBus *pci_bus;
bool d2p_enabled;
bool sata_enabled;
bool smbus_enabled;
+ uint8_t ehci_count;
};
static Property ich9_props[] = {
@@ -42,6 +51,7 @@ static Property ich9_props[] = {
DEFINE_PROP_BOOL("d2p-enabled", ICH9State, d2p_enabled, true),
DEFINE_PROP_BOOL("sata-enabled", ICH9State, sata_enabled, true),
DEFINE_PROP_BOOL("smbus-enabled", ICH9State, smbus_enabled, true),
+ DEFINE_PROP_UINT8("ehci-count", ICH9State, ehci_count, 2),
DEFINE_PROP_END_OF_LIST(),
};
@@ -96,6 +106,46 @@ static bool ich9_realize_smbus(ICH9State *s, Error **errp)
return true;
}
+static bool ich9_realize_usb(ICH9State *s, Error **errp)
+{
+ if (!module_object_class_by_name(TYPE_ICH9_USB_UHCI(0))
+ || !module_object_class_by_name("ich9-usb-ehci0")) {
+ error_setg(errp, "USB functions not available in this build");
+ return false;
+ }
+ for (unsigned e = 0; e < s->ehci_count; e++) {
+ g_autofree gchar *ename = g_strdup_printf("ich9-usb-ehci%u", e + 1);
+ EHCIPCIState *ehci = &s->ehci[e];
+ const unsigned devid = e ? 0x1a : 0x1d;
+ BusState *masterbus;
+
+ object_initialize_child(OBJECT(s), "ehci[*]", ehci, ename);
+ qdev_prop_set_int32(DEVICE(ehci), "addr", PCI_DEVFN(devid,
+ ICH9_EHCI_FUNC));
+ if (!qdev_realize(DEVICE(ehci), BUS(s->pci_bus), errp)) {
+ return false;
+ }
+ masterbus = QLIST_FIRST(&DEVICE(ehci)->child_bus);
+
+ for (unsigned u = 0; u < UHCI_PER_FN; u++) {
+ unsigned c = UHCI_PER_FN * e + u;
+ UHCIState *uhci = &s->uhci[c];
+ g_autofree gchar *cname = g_strdup_printf("ich9-usb-uhci%u", c + 1);
+
+ object_initialize_child(OBJECT(s), "uhci[*]", uhci, cname);
+ qdev_prop_set_bit(DEVICE(uhci), "multifunction", true);
+ qdev_prop_set_int32(DEVICE(uhci), "addr", PCI_DEVFN(devid, u));
+ qdev_prop_set_string(DEVICE(uhci), "masterbus", masterbus->name);
+ qdev_prop_set_uint32(DEVICE(uhci), "firstport", 2 * u);
+ if (!qdev_realize(DEVICE(uhci), BUS(s->pci_bus), errp)) {
+ return false;
+ }
+ }
+ }
+
+ return true;
+}
+
static void ich9_init(Object *obj)
{
}
@@ -120,6 +170,10 @@ static void ich9_realize(DeviceState *dev, Error **errp)
if (s->smbus_enabled && !ich9_realize_smbus(s, errp)) {
return;
}
+
+ if (!ich9_realize_usb(s, errp)) {
+ return;
+ }
}
static void ich9_class_init(ObjectClass *klass, void *data)
diff --git a/hw/southbridge/Kconfig b/hw/southbridge/Kconfig
index 03e89a55d1..31eb125bf7 100644
--- a/hw/southbridge/Kconfig
+++ b/hw/southbridge/Kconfig
@@ -6,3 +6,5 @@ config ICH9
imply I82801B11
select AHCI_ICH9
select ACPI_ICH9
+ imply USB_EHCI_PCI
+ imply USB_UHCI
--
2.41.0
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 13/14] hw/southbridge/ich9: Extract LPC definitions to 'hw/isa/ich9_lpc.h'
2024-02-19 16:38 [PATCH 00/14] hw/southbridge: Extract ICH9 QOM container model Philippe Mathieu-Daudé
` (11 preceding siblings ...)
2024-02-19 16:38 ` [PATCH 12/14] hw/southbridge/ich9: Add the USB EHCI/UHCI functions Philippe Mathieu-Daudé
@ 2024-02-19 16:38 ` Philippe Mathieu-Daudé
2024-02-19 16:38 ` [PATCH 14/14] hw/southbridge/ich9: Add the LPC / ISA bridge function Philippe Mathieu-Daudé
13 siblings, 0 replies; 28+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-02-19 16:38 UTC (permalink / raw)
To: qemu-devel, Bernhard Beschow
Cc: Michael S. Tsirkin, Ani Sinha, Richard Henderson, Igor Mammedov,
Mark Cave-Ayland, Laurent Vivier, Thomas Huth, Marcel Apfelbaum,
Eduardo Habkost, Paolo Bonzini, BALATON Zoltan,
Philippe Mathieu-Daudé
Restrict ICH9 LPC/ISA definitions by moving them
to the "hw/isa/ich9_lpc.h" header.
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
MAINTAINERS | 1 +
include/hw/isa/ich9_lpc.h | 166 ++++++++++++++++++++++++++++++++++
include/hw/southbridge/ich9.h | 158 ++------------------------------
hw/acpi/ich9.c | 1 +
hw/acpi/ich9_tco.c | 1 +
hw/i386/acpi-build.c | 1 +
hw/i386/pc_q35.c | 1 +
hw/isa/lpc_ich9.c | 34 ++++++-
tests/qtest/tco-test.c | 2 +-
9 files changed, 208 insertions(+), 157 deletions(-)
create mode 100644 include/hw/isa/ich9_lpc.h
diff --git a/MAINTAINERS b/MAINTAINERS
index b896d953af..31ddbc565b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2613,6 +2613,7 @@ F: hw/southbridge/ich9.c
F: include/hw/acpi/ich9*.h
F: include/hw/i2c/smbus_ich9.h
F: include/hw/ide/ahci-pci.h
+F: include/hw/isa/ich9_lpc.h
F: include/hw/pci-bridge/ich_dmi_pci.h
F: include/hw/southbridge/ich9.h
diff --git a/include/hw/isa/ich9_lpc.h b/include/hw/isa/ich9_lpc.h
new file mode 100644
index 0000000000..b64d88b395
--- /dev/null
+++ b/include/hw/isa/ich9_lpc.h
@@ -0,0 +1,166 @@
+/*
+ * QEMU ICH9 PCI-to-LPC/ISA bridge emulation
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef HW_ISA_ICH9_LPC_H
+#define HW_ISA_ICH9_LPC_H
+
+#include "exec/memory.h"
+#include "hw/isa/apm.h"
+#include "hw/acpi/ich9.h"
+#include "hw/intc/ioapic.h"
+#include "hw/pci/pci_device.h"
+#include "hw/rtc/mc146818rtc.h"
+#include "qemu/notify.h"
+#include "qom/object.h"
+
+#define TYPE_ICH9_LPC_DEVICE "ICH9-LPC"
+OBJECT_DECLARE_SIMPLE_TYPE(ICH9LPCState, ICH9_LPC_DEVICE)
+
+#define ICH9_CC_SIZE (16 * 1024) /* 16KB. Chipset configuration registers */
+
+struct ICH9LPCState {
+ /* ICH9 LPC PCI to ISA bridge */
+ PCIDevice d;
+
+ /* (pci device, intx) -> pirq
+ * In real chipset case, the unused slots are never used
+ * as ICH9 supports only D25-D31 irq routing.
+ * On the other hand in qemu case, any slot/function can be populated
+ * via command line option.
+ * So fallback interrupt routing for any devices in any slots is necessary.
+ */
+ uint8_t irr[PCI_SLOT_MAX][PCI_NUM_PINS];
+
+ MC146818RtcState rtc;
+ APMState apm;
+ ICH9LPCPMRegs pm;
+ uint32_t sci_level; /* track sci level */
+ uint8_t sci_gsi;
+
+ /* 2.24 Pin Straps */
+ struct {
+ bool spkr_hi;
+ } pin_strap;
+
+ /* 10.1 Chipset Configuration registers(Memory Space)
+ which is pointed by RCBA */
+ uint8_t chip_config[ICH9_CC_SIZE];
+
+ /*
+ * 13.7.5 RST_CNT---Reset Control Register (LPC I/F---D31:F0)
+ *
+ * register contents and IO memory region
+ */
+ uint8_t rst_cnt;
+ MemoryRegion rst_cnt_mem;
+
+ /* 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
+ * endian uint64_t */
+ uint8_t smi_guest_features_le[8]; /* guest-visible, read-write, little
+ * endian uint64_t */
+ uint8_t smi_features_ok; /* guest-visible, read-only; selecting it
+ * triggers feature lockdown */
+ uint64_t smi_negotiated_features; /* guest-invisible, host endian */
+
+ MemoryRegion rcrb_mem; /* root complex register block */
+ Notifier machine_ready;
+
+ qemu_irq gsi[IOAPIC_NUM_PINS];
+};
+
+#define ICH9_MASK(bit, ms_bit, ls_bit) \
+((uint##bit##_t)(((1ULL << ((ms_bit) + 1)) - 1) & ~((1ULL << ls_bit) - 1)))
+
+#define ICH9_CC_SIZE (16 * 1024) /* 16KB. Chipset configuration registers */
+
+/* ICH9: Chipset Configuration Registers */
+#define ICH9_CC_ADDR_MASK (ICH9_CC_SIZE - 1)
+
+#define ICH9_CC
+#define ICH9_CC_D28IP 0x310C
+#define ICH9_CC_D28IP_SHIFT 4
+#define ICH9_CC_D28IP_MASK 0xf
+#define ICH9_CC_D28IP_DEFAULT 0x00214321
+#define ICH9_CC_D31IR 0x3140
+#define ICH9_CC_D30IR 0x3142
+#define ICH9_CC_D29IR 0x3144
+#define ICH9_CC_D28IR 0x3146
+#define ICH9_CC_D27IR 0x3148
+#define ICH9_CC_D26IR 0x314C
+#define ICH9_CC_D25IR 0x3150
+#define ICH9_CC_DIR_DEFAULT 0x3210
+#define ICH9_CC_D30IR_DEFAULT 0x0
+#define ICH9_CC_DIR_SHIFT 4
+#define ICH9_CC_DIR_MASK 0x7
+#define ICH9_CC_OIC 0x31FF
+#define ICH9_CC_OIC_AEN 0x1
+#define ICH9_CC_GCS 0x3410
+#define ICH9_CC_GCS_DEFAULT 0x00000020
+#define ICH9_CC_GCS_NO_REBOOT (1 << 5)
+
+/* D31:F0 LPC Processor Interface */
+#define ICH9_RST_CNT_IOPORT 0xCF9
+
+/* D31:F1 LPC controller */
+#define ICH9_A2_LPC "ICH9 A2 LPC"
+#define ICH9_A2_LPC_SAVEVM_VERSION 0
+
+#define ICH9_A2_LPC_REVISION 0x2
+#define ICH9_LPC_NB_PIRQS 8 /* PCI A-H */
+
+#define ICH9_LPC_PMBASE 0x40
+#define ICH9_LPC_PMBASE_BASE_ADDRESS_MASK ICH9_MASK(32, 15, 7)
+#define ICH9_LPC_PMBASE_RTE 0x1
+#define ICH9_LPC_PMBASE_DEFAULT 0x1
+
+#define ICH9_LPC_ACPI_CTRL 0x44
+#define ICH9_LPC_ACPI_CTRL_ACPI_EN 0x80
+#define ICH9_LPC_ACPI_CTRL_SCI_IRQ_SEL_MASK ICH9_MASK(8, 2, 0)
+#define ICH9_LPC_ACPI_CTRL_9 0x0
+#define ICH9_LPC_ACPI_CTRL_10 0x1
+#define ICH9_LPC_ACPI_CTRL_11 0x2
+#define ICH9_LPC_ACPI_CTRL_20 0x4
+#define ICH9_LPC_ACPI_CTRL_21 0x5
+#define ICH9_LPC_ACPI_CTRL_DEFAULT 0x0
+
+#define ICH9_LPC_PIRQA_ROUT 0x60
+#define ICH9_LPC_PIRQB_ROUT 0x61
+#define ICH9_LPC_PIRQC_ROUT 0x62
+#define ICH9_LPC_PIRQD_ROUT 0x63
+
+#define ICH9_LPC_PIRQE_ROUT 0x68
+#define ICH9_LPC_PIRQF_ROUT 0x69
+#define ICH9_LPC_PIRQG_ROUT 0x6a
+#define ICH9_LPC_PIRQH_ROUT 0x6b
+
+#define ICH9_LPC_PIRQ_ROUT_IRQEN 0x80
+#define ICH9_LPC_PIRQ_ROUT_MASK ICH9_MASK(8, 3, 0)
+#define ICH9_LPC_PIRQ_ROUT_DEFAULT 0x80
+
+#define ICH9_LPC_GEN_PMCON_1 0xa0
+#define ICH9_LPC_GEN_PMCON_1_SMI_LOCK (1 << 4)
+#define ICH9_LPC_GEN_PMCON_2 0xa2
+#define ICH9_LPC_GEN_PMCON_3 0xa4
+#define ICH9_LPC_GEN_PMCON_LOCK 0xa6
+
+#define ICH9_LPC_RCBA 0xf0
+#define ICH9_LPC_RCBA_BA_MASK ICH9_MASK(32, 31, 14)
+#define ICH9_LPC_RCBA_EN 0x1
+#define ICH9_LPC_RCBA_DEFAULT 0x0
+
+#define ICH9_LPC_PIC_NUM_PINS 16
+#define ICH9_LPC_IOAPIC_NUM_PINS 24
+
+/* D31:F0 power management I/O registers
+ offset from the address ICH9_LPC_PMBASE */
+
+/* FADT ACPI_ENABLE/ACPI_DISABLE */
+#define ICH9_APM_ACPI_ENABLE 0x2
+#define ICH9_APM_ACPI_DISABLE 0x3
+
+#endif
diff --git a/include/hw/southbridge/ich9.h b/include/hw/southbridge/ich9.h
index 7e75496b0b..d6c3b5ece3 100644
--- a/include/hw/southbridge/ich9.h
+++ b/include/hw/southbridge/ich9.h
@@ -1,173 +1,27 @@
+/*
+ * QEMU Intel ICH9 south bridge emulation
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
#ifndef HW_SOUTHBRIDGE_ICH9_H
#define HW_SOUTHBRIDGE_ICH9_H
-#include "hw/isa/apm.h"
-#include "hw/acpi/ich9.h"
-#include "hw/intc/ioapic.h"
-#include "hw/pci/pci.h"
-#include "hw/pci/pci_device.h"
-#include "hw/rtc/mc146818rtc.h"
-#include "exec/memory.h"
-#include "qemu/notify.h"
#include "qom/object.h"
#define TYPE_ICH9_SOUTHBRIDGE "ICH9-southbridge"
OBJECT_DECLARE_SIMPLE_TYPE(ICH9State, ICH9_SOUTHBRIDGE)
-#define ICH9_CC_SIZE (16 * 1024) /* 16KB. Chipset configuration registers */
-
-#define TYPE_ICH9_LPC_DEVICE "ICH9-LPC"
-OBJECT_DECLARE_SIMPLE_TYPE(ICH9LPCState, ICH9_LPC_DEVICE)
-
-struct ICH9LPCState {
- /* ICH9 LPC PCI to ISA bridge */
- PCIDevice d;
-
- /* (pci device, intx) -> pirq
- * In real chipset case, the unused slots are never used
- * as ICH9 supports only D25-D31 irq routing.
- * On the other hand in qemu case, any slot/function can be populated
- * via command line option.
- * So fallback interrupt routing for any devices in any slots is necessary.
- */
- uint8_t irr[PCI_SLOT_MAX][PCI_NUM_PINS];
-
- MC146818RtcState rtc;
- APMState apm;
- ICH9LPCPMRegs pm;
- uint32_t sci_level; /* track sci level */
- uint8_t sci_gsi;
-
- /* 2.24 Pin Straps */
- struct {
- bool spkr_hi;
- } pin_strap;
-
- /* 10.1 Chipset Configuration registers(Memory Space)
- which is pointed by RCBA */
- uint8_t chip_config[ICH9_CC_SIZE];
-
- /*
- * 13.7.5 RST_CNT---Reset Control Register (LPC I/F---D31:F0)
- *
- * register contents and IO memory region
- */
- uint8_t rst_cnt;
- MemoryRegion rst_cnt_mem;
-
- /* 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
- * endian uint64_t */
- uint8_t smi_guest_features_le[8]; /* guest-visible, read-write, little
- * endian uint64_t */
- uint8_t smi_features_ok; /* guest-visible, read-only; selecting it
- * triggers feature lockdown */
- uint64_t smi_negotiated_features; /* guest-invisible, host endian */
-
- MemoryRegion rcrb_mem; /* root complex register block */
- Notifier machine_ready;
-
- qemu_irq gsi[IOAPIC_NUM_PINS];
-};
-
-#define ICH9_MASK(bit, ms_bit, ls_bit) \
-((uint##bit##_t)(((1ULL << ((ms_bit) + 1)) - 1) & ~((1ULL << ls_bit) - 1)))
-
-/* ICH9: Chipset Configuration Registers */
-#define ICH9_CC_ADDR_MASK (ICH9_CC_SIZE - 1)
-
-#define ICH9_CC
-#define ICH9_CC_D28IP 0x310C
-#define ICH9_CC_D28IP_SHIFT 4
-#define ICH9_CC_D28IP_MASK 0xf
-#define ICH9_CC_D28IP_DEFAULT 0x00214321
-#define ICH9_CC_D31IR 0x3140
-#define ICH9_CC_D30IR 0x3142
-#define ICH9_CC_D29IR 0x3144
-#define ICH9_CC_D28IR 0x3146
-#define ICH9_CC_D27IR 0x3148
-#define ICH9_CC_D26IR 0x314C
-#define ICH9_CC_D25IR 0x3150
-#define ICH9_CC_DIR_DEFAULT 0x3210
-#define ICH9_CC_D30IR_DEFAULT 0x0
-#define ICH9_CC_DIR_SHIFT 4
-#define ICH9_CC_DIR_MASK 0x7
-#define ICH9_CC_OIC 0x31FF
-#define ICH9_CC_OIC_AEN 0x1
-#define ICH9_CC_GCS 0x3410
-#define ICH9_CC_GCS_DEFAULT 0x00000020
-#define ICH9_CC_GCS_NO_REBOOT (1 << 5)
-
/* D28:F[0-5] */
#define ICH9_PCIE_DEV 28
#define ICH9_PCIE_FUNC_MAX 6
-/* D31:F0 LPC Processor Interface */
-#define ICH9_RST_CNT_IOPORT 0xCF9
-
/* D31:F1 LPC controller */
-#define ICH9_A2_LPC "ICH9 A2 LPC"
-#define ICH9_A2_LPC_SAVEVM_VERSION 0
-
#define ICH9_LPC_DEV 31
#define ICH9_LPC_FUNC 0
-#define ICH9_A2_LPC_REVISION 0x2
-#define ICH9_LPC_NB_PIRQS 8 /* PCI A-H */
-
-#define ICH9_LPC_PMBASE 0x40
-#define ICH9_LPC_PMBASE_BASE_ADDRESS_MASK ICH9_MASK(32, 15, 7)
-#define ICH9_LPC_PMBASE_RTE 0x1
-#define ICH9_LPC_PMBASE_DEFAULT 0x1
-
-#define ICH9_LPC_ACPI_CTRL 0x44
-#define ICH9_LPC_ACPI_CTRL_ACPI_EN 0x80
-#define ICH9_LPC_ACPI_CTRL_SCI_IRQ_SEL_MASK ICH9_MASK(8, 2, 0)
-#define ICH9_LPC_ACPI_CTRL_9 0x0
-#define ICH9_LPC_ACPI_CTRL_10 0x1
-#define ICH9_LPC_ACPI_CTRL_11 0x2
-#define ICH9_LPC_ACPI_CTRL_20 0x4
-#define ICH9_LPC_ACPI_CTRL_21 0x5
-#define ICH9_LPC_ACPI_CTRL_DEFAULT 0x0
-
-#define ICH9_LPC_PIRQA_ROUT 0x60
-#define ICH9_LPC_PIRQB_ROUT 0x61
-#define ICH9_LPC_PIRQC_ROUT 0x62
-#define ICH9_LPC_PIRQD_ROUT 0x63
-
-#define ICH9_LPC_PIRQE_ROUT 0x68
-#define ICH9_LPC_PIRQF_ROUT 0x69
-#define ICH9_LPC_PIRQG_ROUT 0x6a
-#define ICH9_LPC_PIRQH_ROUT 0x6b
-
-#define ICH9_LPC_PIRQ_ROUT_IRQEN 0x80
-#define ICH9_LPC_PIRQ_ROUT_MASK ICH9_MASK(8, 3, 0)
-#define ICH9_LPC_PIRQ_ROUT_DEFAULT 0x80
-
-#define ICH9_LPC_GEN_PMCON_1 0xa0
-#define ICH9_LPC_GEN_PMCON_1_SMI_LOCK (1 << 4)
-#define ICH9_LPC_GEN_PMCON_2 0xa2
-#define ICH9_LPC_GEN_PMCON_3 0xa4
-#define ICH9_LPC_GEN_PMCON_LOCK 0xa6
-
-#define ICH9_LPC_RCBA 0xf0
-#define ICH9_LPC_RCBA_BA_MASK ICH9_MASK(32, 31, 14)
-#define ICH9_LPC_RCBA_EN 0x1
-#define ICH9_LPC_RCBA_DEFAULT 0x0
-
-#define ICH9_LPC_PIC_NUM_PINS 16
-#define ICH9_LPC_IOAPIC_NUM_PINS 24
-
#define ICH9_GPIO_GSI "gsi"
-/* D31:F0 power management I/O registers
- offset from the address ICH9_LPC_PMBASE */
-
-/* FADT ACPI_ENABLE/ACPI_DISABLE */
-#define ICH9_APM_ACPI_ENABLE 0x2
-#define ICH9_APM_ACPI_DISABLE 0x3
-
#define ICH9_LPC_SMI_NEGOTIATED_FEAT_PROP "x-smi-negotiated-features"
/* bit positions used in fw_cfg SMI feature negotiation */
diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 660fa6a082..5e293ffb2a 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -37,6 +37,7 @@
#include "hw/acpi/ich9_tco.h"
#include "hw/acpi/acpi_dev_interface.h"
#include "hw/southbridge/ich9.h"
+#include "hw/isa/ich9_lpc.h"
#include "hw/mem/pc-dimm.h"
#include "hw/mem/nvdimm.h"
diff --git a/hw/acpi/ich9_tco.c b/hw/acpi/ich9_tco.c
index dd4aff82e0..7499ec17db 100644
--- a/hw/acpi/ich9_tco.c
+++ b/hw/acpi/ich9_tco.c
@@ -13,6 +13,7 @@
#include "migration/vmstate.h"
#include "hw/acpi/ich9_tco.h"
+#include "hw/isa/ich9_lpc.h"
#include "trace.h"
enum {
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index d3ce96dd9f..34f3f03949 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -56,6 +56,7 @@
/* Supported chipsets: */
#include "hw/southbridge/ich9.h"
+#include "hw/isa/ich9_lpc.h"
#include "hw/acpi/pcihp.h"
#include "hw/i386/fw_cfg.h"
#include "hw/i386/pc.h"
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index e5f5bb0db1..573a5a0bc0 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -50,6 +50,7 @@
#include "hw/ide/ahci-pci.h"
#include "hw/intc/ioapic.h"
#include "hw/southbridge/ich9.h"
+#include "hw/isa/ich9_lpc.h"
#include "qapi/error.h"
#include "qemu/error-report.h"
#include "sysemu/numa.h"
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 70c6e8a093..685ac38c72 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -1,5 +1,5 @@
/*
- * QEMU ICH9 Emulation
+ * QEMU ICH9 LPC PCI-to-ISA bridge Emulation
*
* Copyright (c) 2006 Fabrice Bellard
* Copyright (c) 2009, 2010, 2011
@@ -42,6 +42,7 @@
#include "hw/pci/pci.h"
#include "hw/southbridge/ich9.h"
#include "hw/i386/pc.h"
+#include "hw/isa/ich9_lpc.h"
#include "hw/acpi/acpi.h"
#include "hw/acpi/ich9.h"
#include "hw/pci/pci_bus.h"
@@ -54,10 +55,35 @@
#include "hw/acpi/acpi_aml_interface.h"
#include "trace.h"
-/*****************************************************************************/
-/* ICH9 LPC PCI to ISA bridge */
+#define ICH9_A2_LPC_REVISION 0x2
+#define ICH9_LPC_NB_PIRQS 8 /* PCI A-H */
-/* chipset configuration register
+/* ICH9: Chipset Configuration Registers */
+#define ICH9_CC_ADDR_MASK (ICH9_CC_SIZE - 1)
+
+#define ICH9_CC
+#define ICH9_CC_D28IP 0x310C
+#define ICH9_CC_D28IP_SHIFT 4
+#define ICH9_CC_D28IP_MASK 0xf
+#define ICH9_CC_D28IP_DEFAULT 0x00214321
+#define ICH9_CC_D31IR 0x3140
+#define ICH9_CC_D30IR 0x3142
+#define ICH9_CC_D29IR 0x3144
+#define ICH9_CC_D28IR 0x3146
+#define ICH9_CC_D27IR 0x3148
+#define ICH9_CC_D26IR 0x314C
+#define ICH9_CC_D25IR 0x3150
+#define ICH9_CC_DIR_DEFAULT 0x3210
+#define ICH9_CC_D30IR_DEFAULT 0x0
+#define ICH9_CC_DIR_SHIFT 4
+#define ICH9_CC_DIR_MASK 0x7
+#define ICH9_CC_OIC 0x31FF
+#define ICH9_CC_OIC_AEN 0x1
+#define ICH9_CC_GCS 0x3410
+#define ICH9_CC_GCS_DEFAULT 0x00000020
+#define ICH9_CC_GCS_NO_REBOOT (1 << 5)
+
+/*
* to access chipset configuration registers, pci_[sg]et_{byte, word, long}
* are used.
* Although it's not pci configuration space, it's little endian as Intel.
diff --git a/tests/qtest/tco-test.c b/tests/qtest/tco-test.c
index 0547d41173..c5974e72bd 100644
--- a/tests/qtest/tco-test.c
+++ b/tests/qtest/tco-test.c
@@ -14,7 +14,7 @@
#include "libqos/pci-pc.h"
#include "qapi/qmp/qdict.h"
#include "hw/pci/pci_regs.h"
-#include "hw/southbridge/ich9.h"
+#include "hw/isa/ich9_lpc.h"
#include "hw/acpi/ich9.h"
#include "hw/acpi/ich9_tco.h"
--
2.41.0
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 14/14] hw/southbridge/ich9: Add the LPC / ISA bridge function
2024-02-19 16:38 [PATCH 00/14] hw/southbridge: Extract ICH9 QOM container model Philippe Mathieu-Daudé
` (12 preceding siblings ...)
2024-02-19 16:38 ` [PATCH 13/14] hw/southbridge/ich9: Extract LPC definitions to 'hw/isa/ich9_lpc.h' Philippe Mathieu-Daudé
@ 2024-02-19 16:38 ` Philippe Mathieu-Daudé
13 siblings, 0 replies; 28+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-02-19 16:38 UTC (permalink / raw)
To: qemu-devel, Bernhard Beschow
Cc: Michael S. Tsirkin, Ani Sinha, Richard Henderson, Igor Mammedov,
Mark Cave-Ayland, Laurent Vivier, Thomas Huth, Marcel Apfelbaum,
Eduardo Habkost, Paolo Bonzini, BALATON Zoltan,
Philippe Mathieu-Daudé
Instantiate TYPE_ICH9_LPC_DEVICE in TYPE_ICH9_SOUTHBRIDGE.
Expose the SMM property so the Q35 machine can disable it
(depending on the accelerator used).
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
include/hw/southbridge/ich9.h | 4 ----
hw/i386/pc_q35.c | 12 +++---------
hw/isa/lpc_ich9.c | 3 +++
hw/southbridge/ich9.c | 14 ++++++++++++++
hw/i386/Kconfig | 1 -
hw/southbridge/Kconfig | 1 +
6 files changed, 21 insertions(+), 14 deletions(-)
diff --git a/include/hw/southbridge/ich9.h b/include/hw/southbridge/ich9.h
index d6c3b5ece3..a8da4a8665 100644
--- a/include/hw/southbridge/ich9.h
+++ b/include/hw/southbridge/ich9.h
@@ -16,10 +16,6 @@ OBJECT_DECLARE_SIMPLE_TYPE(ICH9State, ICH9_SOUTHBRIDGE)
#define ICH9_PCIE_DEV 28
#define ICH9_PCIE_FUNC_MAX 6
-/* D31:F1 LPC controller */
-#define ICH9_LPC_DEV 31
-#define ICH9_LPC_FUNC 0
-
#define ICH9_GPIO_GSI "gsi"
#define ICH9_LPC_SMI_NEGOTIATED_FEAT_PROP "x-smi-negotiated-features"
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 573a5a0bc0..0fe43e8e2d 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -50,7 +50,6 @@
#include "hw/ide/ahci-pci.h"
#include "hw/intc/ioapic.h"
#include "hw/southbridge/ich9.h"
-#include "hw/isa/ich9_lpc.h"
#include "qapi/error.h"
#include "qemu/error-report.h"
#include "sysemu/numa.h"
@@ -69,7 +68,6 @@ static void pc_q35_init(MachineState *machine)
Object *phb;
PCIBus *host_bus;
DeviceState *ich9;
- PCIDevice *lpc;
Object *lpc_obj;
DeviceState *lpc_dev;
BusState *idebus[2] = { };
@@ -181,6 +179,7 @@ static void pc_q35_init(MachineState *machine)
object_property_set_link(OBJECT(ich9), "mch-pcie-bus",
OBJECT(host_bus), &error_abort);
qdev_prop_set_bit(ich9, "d2p-enabled", false);
+ qdev_prop_set_bit(ich9, "smm-enabled", x86_machine_is_smm_enabled(x86ms));
qdev_prop_set_bit(ich9, "sata-enabled", pcms->sata_enabled);
qdev_prop_set_bit(ich9, "smbus-enabled", pcms->smbus_enabled);
/* Should we create 6 UHCI according to ich9 spec? */
@@ -191,13 +190,8 @@ static void pc_q35_init(MachineState *machine)
gsi_state = pc_gsi_create(&x86ms->gsi, true);
/* create ISA bus */
- lpc = pci_new_multifunction(PCI_DEVFN(ICH9_LPC_DEV, ICH9_LPC_FUNC),
- TYPE_ICH9_LPC_DEVICE);
- lpc_obj = OBJECT(lpc);
- lpc_dev = DEVICE(lpc);
- qdev_prop_set_bit(lpc_dev, "smm-enabled",
- x86_machine_is_smm_enabled(x86ms));
- pci_realize_and_unref(lpc, host_bus, &error_fatal);
+ lpc_obj = object_resolve_path_component(OBJECT(ich9), "lpc");
+ lpc_dev = DEVICE(lpc_obj);
for (i = 0; i < IOAPIC_NUM_PINS; i++) {
qdev_connect_gpio_out_named(lpc_dev, ICH9_GPIO_GSI, i, x86ms->gsi[i]);
}
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 685ac38c72..4d7b5c0c64 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -55,6 +55,9 @@
#include "hw/acpi/acpi_aml_interface.h"
#include "trace.h"
+#define ICH9_LPC_DEV 31
+#define ICH9_LPC_FUNC 0
+
#define ICH9_A2_LPC_REVISION 0x2
#define ICH9_LPC_NB_PIRQS 8 /* PCI A-H */
diff --git a/hw/southbridge/ich9.c b/hw/southbridge/ich9.c
index 085d75e569..57a05b35e1 100644
--- a/hw/southbridge/ich9.c
+++ b/hw/southbridge/ich9.c
@@ -13,6 +13,7 @@
#include "hw/southbridge/ich9.h"
#include "hw/pci/pci.h"
#include "hw/pci-bridge/ich_dmi_pci.h"
+#include "hw/isa/ich9_lpc.h"
#include "hw/ide/ahci-pci.h"
#include "hw/ide.h"
#include "hw/i2c/smbus_ich9.h"
@@ -21,6 +22,7 @@
#include "hw/usb/hcd-uhci.h"
#define ICH9_D2P_DEVFN PCI_DEVFN(30, 0)
+#define ICH9_LPC_DEVFN PCI_DEVFN(31, 0)
#define ICH9_SATA1_DEVFN PCI_DEVFN(31, 2)
#define ICH9_SMB_DEVFN PCI_DEVFN(31, 3)
#define ICH9_EHCI_FUNC 7
@@ -34,6 +36,7 @@ struct ICH9State {
I82801b11Bridge d2p;
AHCIPCIState sata0;
+ ICH9LPCState lpc;
ICH9SMBState smb;
EHCIPCIState ehci[EHCI_PER_FN];
UHCIState uhci[EHCI_PER_FN * UHCI_PER_FN];
@@ -148,6 +151,13 @@ static bool ich9_realize_usb(ICH9State *s, Error **errp)
static void ich9_init(Object *obj)
{
+ ICH9State *s = ICH9_SOUTHBRIDGE(obj);
+
+ object_initialize_child(obj, "lpc", &s->lpc, TYPE_ICH9_LPC_DEVICE);
+ qdev_prop_set_int32(DEVICE(&s->lpc), "addr", ICH9_LPC_DEVFN);
+ qdev_prop_set_bit(DEVICE(&s->lpc), "multifunction", true);
+ object_property_add_alias(obj, "smm-enabled",
+ OBJECT(&s->lpc), "smm-enabled");
}
static void ich9_realize(DeviceState *dev, Error **errp)
@@ -163,6 +173,10 @@ static void ich9_realize(DeviceState *dev, Error **errp)
return;
}
+ if (!qdev_realize(DEVICE(&s->lpc), BUS(s->pci_bus), errp)) {
+ return;
+ }
+
if (s->sata_enabled && !ich9_realize_sata(s, errp)) {
return;
}
diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig
index 226d7f6916..eccc834e49 100644
--- a/hw/i386/Kconfig
+++ b/hw/i386/Kconfig
@@ -100,7 +100,6 @@ config Q35
select PC_ACPI
select PCI_EXPRESS_Q35
select ICH9
- select LPC_ICH9
select DIMM
select SMBIOS
select FW_CFG_DMA
diff --git a/hw/southbridge/Kconfig b/hw/southbridge/Kconfig
index 31eb125bf7..8ce62b703c 100644
--- a/hw/southbridge/Kconfig
+++ b/hw/southbridge/Kconfig
@@ -8,3 +8,4 @@ config ICH9
select ACPI_ICH9
imply USB_EHCI_PCI
imply USB_UHCI
+ select LPC_ICH9
--
2.41.0
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 06/14] hw/pci-bridge: Extract QOM ICH definitions to 'ich_dmi_pci.h'
2024-02-19 16:38 ` [PATCH 06/14] hw/pci-bridge: Extract QOM ICH definitions to 'ich_dmi_pci.h' Philippe Mathieu-Daudé
@ 2024-02-19 18:15 ` BALATON Zoltan
2024-02-19 18:24 ` BALATON Zoltan
2024-02-20 19:25 ` Bernhard Beschow
1 sibling, 1 reply; 28+ messages in thread
From: BALATON Zoltan @ 2024-02-19 18:15 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: qemu-devel, Bernhard Beschow, Michael S. Tsirkin, Ani Sinha,
Richard Henderson, Igor Mammedov, Mark Cave-Ayland,
Laurent Vivier, Thomas Huth, Marcel Apfelbaum, Eduardo Habkost,
Paolo Bonzini
[-- Attachment #1: Type: text/plain, Size: 3957 bytes --]
On Mon, 19 Feb 2024, Philippe Mathieu-Daudé wrote:
> Expose TYPE_ICH_DMI_PCI_BRIDGE to the new
> "hw/pci-bridge/ich_dmi_pci.h" header.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> MAINTAINERS | 1 +
> include/hw/pci-bridge/ich_dmi_pci.h | 20 ++++++++++++++++++++
> include/hw/southbridge/ich9.h | 2 --
> hw/pci-bridge/i82801b11.c | 11 ++++-------
> 4 files changed, 25 insertions(+), 9 deletions(-)
> create mode 100644 include/hw/pci-bridge/ich_dmi_pci.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1b210c5cc1..50507c3dd6 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2609,6 +2609,7 @@ F: hw/acpi/ich9*.c
> F: hw/i2c/smbus_ich9.c
> F: hw/isa/lpc_ich9.c
> F: include/hw/acpi/ich9*.h
> +F: include/hw/pci-bridge/ich_dmi_pci.h
> F: include/hw/southbridge/ich9.h
>
> PIIX4 South Bridge (i82371AB)
> diff --git a/include/hw/pci-bridge/ich_dmi_pci.h b/include/hw/pci-bridge/ich_dmi_pci.h
> new file mode 100644
> index 0000000000..7623b32b8e
> --- /dev/null
> +++ b/include/hw/pci-bridge/ich_dmi_pci.h
> @@ -0,0 +1,20 @@
> +/*
> + * QEMU ICH4 i82801b11 dmi-to-pci Bridge Emulation
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#ifndef HW_PCI_BRIDGE_ICH_D2P_H
> +#define HW_PCI_BRIDGE_ICH_D2P_H
> +
> +#include "qom/object.h"
> +#include "hw/pci/pci_bridge.h"
> +
> +#define TYPE_ICH_DMI_PCI_BRIDGE "i82801b11-bridge"
> +OBJECT_DECLARE_SIMPLE_TYPE(I82801b11Bridge, ICH_DMI_PCI_BRIDGE)
> +
> +struct I82801b11Bridge {
> + PCIBridge parent_obj;
> +};
If this class has no fields of its own why does it need its own state
struct defined? You could just set .instance_size = sizeof(PCIBridge) in
the TypeInfo i82801b11_bridge_info below and delete this struct completely
as it's not even used anywhere. One less needless QOM complication :-) For
an example see the empty via-mc97 device in hw/audio/via-ac97.c.
Then you can put the OBJECT_DECLARE_SIMPLE_TYPE in
hw/pci-bridge/i82801b11.c where this object is defined and the #define
TYPE_ICH_DMI_PCI_BRIDGE in hw/southbridge/ich9.h and then you don't need
this header at all so you don't end up with:
4 files changed, 25 insertions(+), 9 deletions(-)
but really simplifying it.
Regards,
BALATON Zoltan
> +
> +#endif
> diff --git a/include/hw/southbridge/ich9.h b/include/hw/southbridge/ich9.h
> index bee522a4cf..b2abf483e0 100644
> --- a/include/hw/southbridge/ich9.h
> +++ b/include/hw/southbridge/ich9.h
> @@ -114,8 +114,6 @@ struct ICH9LPCState {
>
> #define ICH9_D2P_SECONDARY_DEFAULT (256 - 8)
>
> -#define ICH9_D2P_A2_REVISION 0x92
> -
> /* D31:F0 LPC Processor Interface */
> #define ICH9_RST_CNT_IOPORT 0xCF9
>
> diff --git a/hw/pci-bridge/i82801b11.c b/hw/pci-bridge/i82801b11.c
> index c140919cbc..dd17e35b0a 100644
> --- a/hw/pci-bridge/i82801b11.c
> +++ b/hw/pci-bridge/i82801b11.c
> @@ -45,7 +45,7 @@
> #include "hw/pci/pci_bridge.h"
> #include "migration/vmstate.h"
> #include "qemu/module.h"
> -#include "hw/southbridge/ich9.h"
> +#include "hw/pci-bridge/ich_dmi_pci.h"
>
> /*****************************************************************************/
> /* ICH9 DMI-to-PCI bridge */
> @@ -53,11 +53,8 @@
> #define I82801ba_SSVID_SVID 0
> #define I82801ba_SSVID_SSID 0
>
> -typedef struct I82801b11Bridge {
> - /*< private >*/
> - PCIBridge parent_obj;
> - /*< public >*/
> -} I82801b11Bridge;
> +
> +#define ICH9_D2P_A2_REVISION 0x92
>
> static void i82801b11_bridge_realize(PCIDevice *d, Error **errp)
> {
> @@ -103,7 +100,7 @@ static void i82801b11_bridge_class_init(ObjectClass *klass, void *data)
> }
>
> static const TypeInfo i82801b11_bridge_info = {
> - .name = "i82801b11-bridge",
> + .name = TYPE_ICH_DMI_PCI_BRIDGE,
> .parent = TYPE_PCI_BRIDGE,
> .instance_size = sizeof(I82801b11Bridge),
> .class_init = i82801b11_bridge_class_init,
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 06/14] hw/pci-bridge: Extract QOM ICH definitions to 'ich_dmi_pci.h'
2024-02-19 18:15 ` BALATON Zoltan
@ 2024-02-19 18:24 ` BALATON Zoltan
2024-02-20 6:09 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 28+ messages in thread
From: BALATON Zoltan @ 2024-02-19 18:24 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: qemu-devel, Bernhard Beschow, Michael S. Tsirkin, Ani Sinha,
Richard Henderson, Igor Mammedov, Mark Cave-Ayland,
Laurent Vivier, Thomas Huth, Marcel Apfelbaum, Eduardo Habkost,
Paolo Bonzini
[-- Attachment #1: Type: text/plain, Size: 4357 bytes --]
On Mon, 19 Feb 2024, BALATON Zoltan wrote:
> On Mon, 19 Feb 2024, Philippe Mathieu-Daudé wrote:
>> Expose TYPE_ICH_DMI_PCI_BRIDGE to the new
>> "hw/pci-bridge/ich_dmi_pci.h" header.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> MAINTAINERS | 1 +
>> include/hw/pci-bridge/ich_dmi_pci.h | 20 ++++++++++++++++++++
>> include/hw/southbridge/ich9.h | 2 --
>> hw/pci-bridge/i82801b11.c | 11 ++++-------
>> 4 files changed, 25 insertions(+), 9 deletions(-)
>> create mode 100644 include/hw/pci-bridge/ich_dmi_pci.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 1b210c5cc1..50507c3dd6 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -2609,6 +2609,7 @@ F: hw/acpi/ich9*.c
>> F: hw/i2c/smbus_ich9.c
>> F: hw/isa/lpc_ich9.c
>> F: include/hw/acpi/ich9*.h
>> +F: include/hw/pci-bridge/ich_dmi_pci.h
>> F: include/hw/southbridge/ich9.h
>>
>> PIIX4 South Bridge (i82371AB)
>> diff --git a/include/hw/pci-bridge/ich_dmi_pci.h
>> b/include/hw/pci-bridge/ich_dmi_pci.h
>> new file mode 100644
>> index 0000000000..7623b32b8e
>> --- /dev/null
>> +++ b/include/hw/pci-bridge/ich_dmi_pci.h
>> @@ -0,0 +1,20 @@
>> +/*
>> + * QEMU ICH4 i82801b11 dmi-to-pci Bridge Emulation
>> + *
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + */
>> +
>> +#ifndef HW_PCI_BRIDGE_ICH_D2P_H
>> +#define HW_PCI_BRIDGE_ICH_D2P_H
>> +
>> +#include "qom/object.h"
>> +#include "hw/pci/pci_bridge.h"
>> +
>> +#define TYPE_ICH_DMI_PCI_BRIDGE "i82801b11-bridge"
>> +OBJECT_DECLARE_SIMPLE_TYPE(I82801b11Bridge, ICH_DMI_PCI_BRIDGE)
>> +
>> +struct I82801b11Bridge {
>> + PCIBridge parent_obj;
>> +};
>
> If this class has no fields of its own why does it need its own state struct
> defined? You could just set .instance_size = sizeof(PCIBridge) in the
> TypeInfo i82801b11_bridge_info below and delete this struct completely as
> it's not even used anywhere. One less needless QOM complication :-) For an
> example see the empty via-mc97 device in hw/audio/via-ac97.c.
>
> Then you can put the OBJECT_DECLARE_SIMPLE_TYPE in hw/pci-bridge/i82801b11.c
> where this object is defined and the #define TYPE_ICH_DMI_PCI_BRIDGE in
You don't even need OBJECT_DECLARE_SIMPLE_TYPE if there's no state struct.
But on second look what is this object at all? It's never instantiated
anywhere. Is it used somewhere?
Regards,
BALATON Zoltan
> hw/southbridge/ich9.h and then you don't need this header at all so you don't
> end up with:
>
> 4 files changed, 25 insertions(+), 9 deletions(-)
>
> but really simplifying it.
>
> Regards,
> BALATON Zoltan
>
>> +
>> +#endif
>> diff --git a/include/hw/southbridge/ich9.h b/include/hw/southbridge/ich9.h
>> index bee522a4cf..b2abf483e0 100644
>> --- a/include/hw/southbridge/ich9.h
>> +++ b/include/hw/southbridge/ich9.h
>> @@ -114,8 +114,6 @@ struct ICH9LPCState {
>>
>> #define ICH9_D2P_SECONDARY_DEFAULT (256 - 8)
>>
>> -#define ICH9_D2P_A2_REVISION 0x92
>> -
>> /* D31:F0 LPC Processor Interface */
>> #define ICH9_RST_CNT_IOPORT 0xCF9
>>
>> diff --git a/hw/pci-bridge/i82801b11.c b/hw/pci-bridge/i82801b11.c
>> index c140919cbc..dd17e35b0a 100644
>> --- a/hw/pci-bridge/i82801b11.c
>> +++ b/hw/pci-bridge/i82801b11.c
>> @@ -45,7 +45,7 @@
>> #include "hw/pci/pci_bridge.h"
>> #include "migration/vmstate.h"
>> #include "qemu/module.h"
>> -#include "hw/southbridge/ich9.h"
>> +#include "hw/pci-bridge/ich_dmi_pci.h"
>>
>> /*****************************************************************************/
>> /* ICH9 DMI-to-PCI bridge */
>> @@ -53,11 +53,8 @@
>> #define I82801ba_SSVID_SVID 0
>> #define I82801ba_SSVID_SSID 0
>>
>> -typedef struct I82801b11Bridge {
>> - /*< private >*/
>> - PCIBridge parent_obj;
>> - /*< public >*/
>> -} I82801b11Bridge;
>> +
>> +#define ICH9_D2P_A2_REVISION 0x92
>>
>> static void i82801b11_bridge_realize(PCIDevice *d, Error **errp)
>> {
>> @@ -103,7 +100,7 @@ static void i82801b11_bridge_class_init(ObjectClass
>> *klass, void *data)
>> }
>>
>> static const TypeInfo i82801b11_bridge_info = {
>> - .name = "i82801b11-bridge",
>> + .name = TYPE_ICH_DMI_PCI_BRIDGE,
>> .parent = TYPE_PCI_BRIDGE,
>> .instance_size = sizeof(I82801b11Bridge),
>> .class_init = i82801b11_bridge_class_init,
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 09/14] hw/southbridge/ich9: Add a AHCI function
2024-02-19 16:38 ` [PATCH 09/14] hw/southbridge/ich9: Add a AHCI function Philippe Mathieu-Daudé
@ 2024-02-19 18:31 ` BALATON Zoltan
2024-02-20 6:01 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 28+ messages in thread
From: BALATON Zoltan @ 2024-02-19 18:31 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: qemu-devel, Bernhard Beschow, Michael S. Tsirkin, Ani Sinha,
Richard Henderson, Igor Mammedov, Mark Cave-Ayland,
Laurent Vivier, Thomas Huth, Marcel Apfelbaum, Eduardo Habkost,
Paolo Bonzini
[-- Attachment #1: Type: text/plain, Size: 7084 bytes --]
On Mon, 19 Feb 2024, Philippe Mathieu-Daudé wrote:
> Instantiate TYPE_ICH9_AHCI in TYPE_ICH9_SOUTHBRIDGE.
>
> Since the PC machines can disable SATA (see the
> PC_MACHINE_SATA dynamic property), add the 'sata-enabled'
> property to disable it.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> MAINTAINERS | 2 ++
> include/hw/southbridge/ich9.h | 4 ----
> hw/i386/pc_q35.c | 25 ++++---------------------
> hw/southbridge/ich9.c | 35 +++++++++++++++++++++++++++++++++++
> hw/i386/Kconfig | 1 -
> hw/southbridge/Kconfig | 1 +
> 6 files changed, 42 insertions(+), 26 deletions(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d1a2eddd4c..937ebb5c96 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2607,9 +2607,11 @@ M: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> S: Supported
> F: hw/acpi/ich9*.c
> F: hw/i2c/smbus_ich9.c
> +F: hw/ide/ich.c
> F: hw/isa/lpc_ich9.c
> F: hw/southbridge/ich9.c
> F: include/hw/acpi/ich9*.h
> +F: include/hw/ide/ahci-pci.h
> F: include/hw/pci-bridge/ich_dmi_pci.h
> F: include/hw/southbridge/ich9.h
>
> diff --git a/include/hw/southbridge/ich9.h b/include/hw/southbridge/ich9.h
> index b9122d299d..ac7f9f4ff5 100644
> --- a/include/hw/southbridge/ich9.h
> +++ b/include/hw/southbridge/ich9.h
> @@ -166,10 +166,6 @@ struct ICH9LPCState {
>
> #define ICH9_GPIO_GSI "gsi"
>
> -/* D31:F2 SATA Controller #1 */
> -#define ICH9_SATA1_DEV 31
> -#define ICH9_SATA1_FUNC 2
> -
> /* D31:F0 power management I/O registers
> offset from the address ICH9_LPC_PMBASE */
>
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 2f15af540f..060358d449 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -61,9 +61,6 @@
> #include "hw/acpi/acpi.h"
> #include "target/i386/cpu.h"
>
> -/* ICH9 AHCI has 6 ports */
> -#define MAX_SATA_PORTS 6
> -
> struct ehci_companions {
> const char *name;
> int func;
> @@ -129,7 +126,7 @@ static void pc_q35_init(MachineState *machine)
> PCIDevice *lpc;
> Object *lpc_obj;
> DeviceState *lpc_dev;
> - BusState *idebus[MAX_SATA_PORTS];
> + BusState *idebus[2] = { };
> ISADevice *rtc_state;
> MemoryRegion *system_memory = get_system_memory();
> MemoryRegion *system_io = get_system_io();
> @@ -138,7 +135,6 @@ static void pc_q35_init(MachineState *machine)
> ISABus *isa_bus;
> int i;
> ram_addr_t lowmem;
> - DriveInfo *hd[MAX_SATA_PORTS];
> MachineClass *mc = MACHINE_GET_CLASS(machine);
> bool acpi_pcihp;
> bool keep_pci_slot_hpc;
> @@ -239,6 +235,7 @@ static void pc_q35_init(MachineState *machine)
> object_property_set_link(OBJECT(ich9), "mch-pcie-bus",
> OBJECT(host_bus), &error_abort);
> qdev_prop_set_bit(ich9, "d2p-enabled", false);
> + qdev_prop_set_bit(ich9, "sata-enabled", pcms->sata_enabled);
> qdev_realize_and_unref(ich9, NULL, &error_fatal);
>
> /* irq lines */
> @@ -302,22 +299,8 @@ static void pc_q35_init(MachineState *machine)
> 0xff0104);
>
> if (pcms->sata_enabled) {
Shouldn't this condition be inverted if you only leave the else leg?
Regards,.
BALATON Zoltan
> - PCIDevice *pdev;
> - AHCIPCIState *ich9;
> -
> - /* ahci and SATA device, for q35 1 ahci controller is built-in */
> - pdev = pci_create_simple_multifunction(host_bus,
> - PCI_DEVFN(ICH9_SATA1_DEV,
> - ICH9_SATA1_FUNC),
> - "ich9-ahci");
> - ich9 = ICH9_AHCI(pdev);
> - idebus[0] = qdev_get_child_bus(DEVICE(pdev), "ide.0");
> - idebus[1] = qdev_get_child_bus(DEVICE(pdev), "ide.1");
> - g_assert(MAX_SATA_PORTS == ich9->ahci.ports);
> - ide_drive_get(hd, ich9->ahci.ports);
> - ahci_ide_create_devs(&ich9->ahci, hd);
> - } else {
> - idebus[0] = idebus[1] = NULL;
> + idebus[0] = qdev_get_child_bus(ich9, "ide.0");
> + idebus[1] = qdev_get_child_bus(ich9, "ide.1");
> }
>
> if (machine_usb(machine)) {
> diff --git a/hw/southbridge/ich9.c b/hw/southbridge/ich9.c
> index 6df47e81fb..233dc1c5d7 100644
> --- a/hw/southbridge/ich9.c
> +++ b/hw/southbridge/ich9.c
> @@ -13,22 +13,30 @@
> #include "hw/southbridge/ich9.h"
> #include "hw/pci/pci.h"
> #include "hw/pci-bridge/ich_dmi_pci.h"
> +#include "hw/ide/ahci-pci.h"
> +#include "hw/ide.h"
>
> #define ICH9_D2P_DEVFN PCI_DEVFN(30, 0)
> +#define ICH9_SATA1_DEVFN PCI_DEVFN(31, 2)
> +
> +#define SATA_PORTS 6
>
> struct ICH9State {
> DeviceState parent_obj;
>
> I82801b11Bridge d2p;
> + AHCIPCIState sata0;
>
> PCIBus *pci_bus;
> bool d2p_enabled;
> + bool sata_enabled;
> };
>
> static Property ich9_props[] = {
> DEFINE_PROP_LINK("mch-pcie-bus", ICH9State, pci_bus,
> TYPE_PCIE_BUS, PCIBus *),
> DEFINE_PROP_BOOL("d2p-enabled", ICH9State, d2p_enabled, true),
> + DEFINE_PROP_BOOL("sata-enabled", ICH9State, sata_enabled, true),
> DEFINE_PROP_END_OF_LIST(),
> };
>
> @@ -48,6 +56,29 @@ static bool ich9_realize_d2p(ICH9State *s, Error **errp)
> return true;
> }
>
> +static bool ich9_realize_sata(ICH9State *s, Error **errp)
> +{
> + DriveInfo *hd[SATA_PORTS];
> +
> + object_initialize_child(OBJECT(s), "sata[0]", &s->sata0, TYPE_ICH9_AHCI);
> + qdev_prop_set_int32(DEVICE(&s->sata0), "addr", ICH9_SATA1_DEVFN);
> + if (!qdev_realize(DEVICE(&s->sata0), BUS(s->pci_bus), errp)) {
> + return false;
> + }
> + for (unsigned i = 0; i < SATA_PORTS; i++) {
> + g_autofree char *portname = g_strdup_printf("ide.%u", i);
> +
> + object_property_add_alias(OBJECT(s), portname,
> + OBJECT(&s->sata0), portname);
> + }
> +
> + g_assert(SATA_PORTS == s->sata0.ahci.ports);
> + ide_drive_get(hd, s->sata0.ahci.ports);
> + ahci_ide_create_devs(&s->sata0.ahci, hd);
> +
> + return true;
> +}
> +
> static void ich9_init(Object *obj)
> {
> }
> @@ -64,6 +95,10 @@ static void ich9_realize(DeviceState *dev, Error **errp)
> if (s->d2p_enabled && !ich9_realize_d2p(s, errp)) {
> return;
> }
> +
> + if (s->sata_enabled && !ich9_realize_sata(s, errp)) {
> + return;
> + }
> }
>
> static void ich9_class_init(ObjectClass *klass, void *data)
> diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig
> index d21638f4f9..226d7f6916 100644
> --- a/hw/i386/Kconfig
> +++ b/hw/i386/Kconfig
> @@ -101,7 +101,6 @@ config Q35
> select PCI_EXPRESS_Q35
> select ICH9
> select LPC_ICH9
> - select AHCI_ICH9
> select DIMM
> select SMBIOS
> select FW_CFG_DMA
> diff --git a/hw/southbridge/Kconfig b/hw/southbridge/Kconfig
> index db7259bf6f..f806033d48 100644
> --- a/hw/southbridge/Kconfig
> +++ b/hw/southbridge/Kconfig
> @@ -4,3 +4,4 @@ config ICH9
> bool
> depends on PCI_EXPRESS
> imply I82801B11
> + select AHCI_ICH9
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 09/14] hw/southbridge/ich9: Add a AHCI function
2024-02-19 18:31 ` BALATON Zoltan
@ 2024-02-20 6:01 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 28+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-02-20 6:01 UTC (permalink / raw)
To: BALATON Zoltan
Cc: qemu-devel, Bernhard Beschow, Michael S. Tsirkin, Ani Sinha,
Richard Henderson, Igor Mammedov, Mark Cave-Ayland,
Laurent Vivier, Thomas Huth, Marcel Apfelbaum, Eduardo Habkost,
Paolo Bonzini
On 19/2/24 19:31, BALATON Zoltan wrote:
> On Mon, 19 Feb 2024, Philippe Mathieu-Daudé wrote:
>> Instantiate TYPE_ICH9_AHCI in TYPE_ICH9_SOUTHBRIDGE.
>>
>> Since the PC machines can disable SATA (see the
>> PC_MACHINE_SATA dynamic property), add the 'sata-enabled'
>> property to disable it.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> MAINTAINERS | 2 ++
>> include/hw/southbridge/ich9.h | 4 ----
>> hw/i386/pc_q35.c | 25 ++++---------------------
>> hw/southbridge/ich9.c | 35 +++++++++++++++++++++++++++++++++++
>> hw/i386/Kconfig | 1 -
>> hw/southbridge/Kconfig | 1 +
>> 6 files changed, 42 insertions(+), 26 deletions(-)
>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>> index 2f15af540f..060358d449 100644
>> --- a/hw/i386/pc_q35.c
>> +++ b/hw/i386/pc_q35.c
>> @@ -61,9 +61,6 @@
>> #include "hw/acpi/acpi.h"
>> #include "target/i386/cpu.h"
>>
>> -/* ICH9 AHCI has 6 ports */
>> -#define MAX_SATA_PORTS 6
>> -
>> struct ehci_companions {
>> const char *name;
>> int func;
>> @@ -129,7 +126,7 @@ static void pc_q35_init(MachineState *machine)
>> PCIDevice *lpc;
>> Object *lpc_obj;
>> DeviceState *lpc_dev;
>> - BusState *idebus[MAX_SATA_PORTS];
>> + BusState *idebus[2] = { };
[*]
>> ISADevice *rtc_state;
>> MemoryRegion *system_memory = get_system_memory();
>> MemoryRegion *system_io = get_system_io();
>> @@ -138,7 +135,6 @@ static void pc_q35_init(MachineState *machine)
>> ISABus *isa_bus;
>> int i;
>> ram_addr_t lowmem;
>> - DriveInfo *hd[MAX_SATA_PORTS];
>> MachineClass *mc = MACHINE_GET_CLASS(machine);
>> bool acpi_pcihp;
>> bool keep_pci_slot_hpc;
>> @@ -239,6 +235,7 @@ static void pc_q35_init(MachineState *machine)
>> object_property_set_link(OBJECT(ich9), "mch-pcie-bus",
>> OBJECT(host_bus), &error_abort);
>> qdev_prop_set_bit(ich9, "d2p-enabled", false);
>> + qdev_prop_set_bit(ich9, "sata-enabled", pcms->sata_enabled);
>> qdev_realize_and_unref(ich9, NULL, &error_fatal);
>>
>> /* irq lines */
>> @@ -302,22 +299,8 @@ static void pc_q35_init(MachineState *machine)
>> 0xff0104);
>>
>> if (pcms->sata_enabled) {
>
> Shouldn't this condition be inverted if you only leave the else leg?
idebus[] is NULL-initialized in [*] so we can remove the else
ladder.
>
> Regards,.
> BALATON Zoltan
>
>> - PCIDevice *pdev;
>> - AHCIPCIState *ich9;
>> -
>> - /* ahci and SATA device, for q35 1 ahci controller is
>> built-in */
>> - pdev = pci_create_simple_multifunction(host_bus,
>> - PCI_DEVFN(ICH9_SATA1_DEV,
>> -
>> ICH9_SATA1_FUNC),
>> - "ich9-ahci");
>> - ich9 = ICH9_AHCI(pdev);
>> - idebus[0] = qdev_get_child_bus(DEVICE(pdev), "ide.0");
>> - idebus[1] = qdev_get_child_bus(DEVICE(pdev), "ide.1");
>> - g_assert(MAX_SATA_PORTS == ich9->ahci.ports);
>> - ide_drive_get(hd, ich9->ahci.ports);
>> - ahci_ide_create_devs(&ich9->ahci, hd);
>> - } else {
>> - idebus[0] = idebus[1] = NULL;
>> + idebus[0] = qdev_get_child_bus(ich9, "ide.0");
>> + idebus[1] = qdev_get_child_bus(ich9, "ide.1");
>> }
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 06/14] hw/pci-bridge: Extract QOM ICH definitions to 'ich_dmi_pci.h'
2024-02-19 18:24 ` BALATON Zoltan
@ 2024-02-20 6:09 ` Philippe Mathieu-Daudé
2024-02-20 12:20 ` BALATON Zoltan
0 siblings, 1 reply; 28+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-02-20 6:09 UTC (permalink / raw)
To: BALATON Zoltan
Cc: qemu-devel, Bernhard Beschow, Michael S. Tsirkin, Ani Sinha,
Richard Henderson, Igor Mammedov, Mark Cave-Ayland,
Laurent Vivier, Thomas Huth, Marcel Apfelbaum, Eduardo Habkost,
Paolo Bonzini, Manos Pitsidianakis, Markus Armbruster,
Mark Cave-Ayland, Peter Maydell
On 19/2/24 19:24, BALATON Zoltan wrote:
> On Mon, 19 Feb 2024, BALATON Zoltan wrote:
>> On Mon, 19 Feb 2024, Philippe Mathieu-Daudé wrote:
>>> Expose TYPE_ICH_DMI_PCI_BRIDGE to the new
>>> "hw/pci-bridge/ich_dmi_pci.h" header.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>> MAINTAINERS | 1 +
>>> include/hw/pci-bridge/ich_dmi_pci.h | 20 ++++++++++++++++++++
>>> include/hw/southbridge/ich9.h | 2 --
>>> hw/pci-bridge/i82801b11.c | 11 ++++-------
>>> 4 files changed, 25 insertions(+), 9 deletions(-)
>>> create mode 100644 include/hw/pci-bridge/ich_dmi_pci.h
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 1b210c5cc1..50507c3dd6 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -2609,6 +2609,7 @@ F: hw/acpi/ich9*.c
>>> F: hw/i2c/smbus_ich9.c
>>> F: hw/isa/lpc_ich9.c
>>> F: include/hw/acpi/ich9*.h
>>> +F: include/hw/pci-bridge/ich_dmi_pci.h
>>> F: include/hw/southbridge/ich9.h
>>>
>>> PIIX4 South Bridge (i82371AB)
>>> diff --git a/include/hw/pci-bridge/ich_dmi_pci.h
>>> b/include/hw/pci-bridge/ich_dmi_pci.h
>>> new file mode 100644
>>> index 0000000000..7623b32b8e
>>> --- /dev/null
>>> +++ b/include/hw/pci-bridge/ich_dmi_pci.h
>>> @@ -0,0 +1,20 @@
>>> +/*
>>> + * QEMU ICH4 i82801b11 dmi-to-pci Bridge Emulation
>>> + *
>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>> + */
>>> +
>>> +#ifndef HW_PCI_BRIDGE_ICH_D2P_H
>>> +#define HW_PCI_BRIDGE_ICH_D2P_H
>>> +
>>> +#include "qom/object.h"
>>> +#include "hw/pci/pci_bridge.h"
>>> +
>>> +#define TYPE_ICH_DMI_PCI_BRIDGE "i82801b11-bridge"
>>> +OBJECT_DECLARE_SIMPLE_TYPE(I82801b11Bridge, ICH_DMI_PCI_BRIDGE)
>>> +
>>> +struct I82801b11Bridge {
>>> + PCIBridge parent_obj;
>>> +};
>>
>> If this class has no fields of its own why does it need its own state
>> struct defined? You could just set .instance_size = sizeof(PCIBridge)
>> in the TypeInfo i82801b11_bridge_info below and delete this struct
>> completely as it's not even used anywhere. One less needless QOM
>> complication :-) For an example see the empty via-mc97 device in
>> hw/audio/via-ac97.c.
>>
>> Then you can put the OBJECT_DECLARE_SIMPLE_TYPE in
>> hw/pci-bridge/i82801b11.c where this object is defined and the #define
>> TYPE_ICH_DMI_PCI_BRIDGE in
>
> You don't even need OBJECT_DECLARE_SIMPLE_TYPE if there's no state
> struct. But on second look what is this object at all? It's never
> instantiated anywhere. Is it used somewhere?
Here my view is we should always define QOM type names in headers
and use them, in particular in the TypeInfo registration. To unify
style and copy/pasting, better use the QOM DECLARE_TYPE macros.
I envision that might help moving toward DSL and have HW modelling
checks done externally, before starting QEMU. But then this is my
view and I dunno about when we'll get that DSL in so I'm OK to
revisit this patch.
> Regards,
> BALATON Zoltan
>
>> hw/southbridge/ich9.h and then you don't need this header at all so
>> you don't end up with:
>>
>> 4 files changed, 25 insertions(+), 9 deletions(-)
>>
>> but really simplifying it.
>>
>> Regards,
>> BALATON Zoltan
>>
>>> +
>>> +#endif
>>> diff --git a/include/hw/southbridge/ich9.h
>>> b/include/hw/southbridge/ich9.h
>>> index bee522a4cf..b2abf483e0 100644
>>> --- a/include/hw/southbridge/ich9.h
>>> +++ b/include/hw/southbridge/ich9.h
>>> @@ -114,8 +114,6 @@ struct ICH9LPCState {
>>>
>>> #define ICH9_D2P_SECONDARY_DEFAULT (256 - 8)
>>>
>>> -#define ICH9_D2P_A2_REVISION 0x92
>>> -
>>> /* D31:F0 LPC Processor Interface */
>>> #define ICH9_RST_CNT_IOPORT 0xCF9
>>>
>>> diff --git a/hw/pci-bridge/i82801b11.c b/hw/pci-bridge/i82801b11.c
>>> index c140919cbc..dd17e35b0a 100644
>>> --- a/hw/pci-bridge/i82801b11.c
>>> +++ b/hw/pci-bridge/i82801b11.c
>>> @@ -45,7 +45,7 @@
>>> #include "hw/pci/pci_bridge.h"
>>> #include "migration/vmstate.h"
>>> #include "qemu/module.h"
>>> -#include "hw/southbridge/ich9.h"
>>> +#include "hw/pci-bridge/ich_dmi_pci.h"
>>>
>>> /*****************************************************************************/
>>> /* ICH9 DMI-to-PCI bridge */
>>> @@ -53,11 +53,8 @@
>>> #define I82801ba_SSVID_SVID 0
>>> #define I82801ba_SSVID_SSID 0
>>>
>>> -typedef struct I82801b11Bridge {
>>> - /*< private >*/
>>> - PCIBridge parent_obj;
>>> - /*< public >*/
>>> -} I82801b11Bridge;
>>> +
>>> +#define ICH9_D2P_A2_REVISION 0x92
>>>
>>> static void i82801b11_bridge_realize(PCIDevice *d, Error **errp)
>>> {
>>> @@ -103,7 +100,7 @@ static void
>>> i82801b11_bridge_class_init(ObjectClass *klass, void *data)
>>> }
>>>
>>> static const TypeInfo i82801b11_bridge_info = {
>>> - .name = "i82801b11-bridge",
>>> + .name = TYPE_ICH_DMI_PCI_BRIDGE,
>>> .parent = TYPE_PCI_BRIDGE,
>>> .instance_size = sizeof(I82801b11Bridge),
>>> .class_init = i82801b11_bridge_class_init,
>>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 05/14] hw/acpi/ich9_tco: Restrict ich9_generate_smi() declaration
2024-02-19 16:38 ` [PATCH 05/14] hw/acpi/ich9_tco: Restrict ich9_generate_smi() declaration Philippe Mathieu-Daudé
@ 2024-02-20 6:32 ` Philippe Mathieu-Daudé
2024-02-26 9:53 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 28+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-02-20 6:32 UTC (permalink / raw)
To: qemu-devel, Bernhard Beschow
Cc: Michael S. Tsirkin, Ani Sinha, Richard Henderson, Igor Mammedov,
Mark Cave-Ayland, Laurent Vivier, Thomas Huth, Marcel Apfelbaum,
Eduardo Habkost, Paolo Bonzini, BALATON Zoltan
On 19/2/24 17:38, Philippe Mathieu-Daudé wrote:
> Only files including "hw/acpi/ich9_tco.h" require
> the ich9_generate_smi() declaration.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> include/hw/acpi/ich9_tco.h | 1 +
> include/hw/southbridge/ich9.h | 2 --
> 2 files changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/include/hw/acpi/ich9_tco.h b/include/hw/acpi/ich9_tco.h
> index 1c99781a79..68ee64942f 100644
> --- a/include/hw/acpi/ich9_tco.h
> +++ b/include/hw/acpi/ich9_tco.h
> @@ -76,6 +76,7 @@ typedef struct TCOIORegs {
> } TCOIORegs;
>
> void ich9_acpi_pm_tco_init(TCOIORegs *tr, MemoryRegion *parent);
> +void ich9_generate_smi(void);
Bah it is only used in hw/acpi/ich9_tco.c, I'll declare it
statically there.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 06/14] hw/pci-bridge: Extract QOM ICH definitions to 'ich_dmi_pci.h'
2024-02-20 6:09 ` Philippe Mathieu-Daudé
@ 2024-02-20 12:20 ` BALATON Zoltan
2024-02-20 12:55 ` Thomas Huth
0 siblings, 1 reply; 28+ messages in thread
From: BALATON Zoltan @ 2024-02-20 12:20 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: qemu-devel, Bernhard Beschow, Michael S. Tsirkin, Ani Sinha,
Richard Henderson, Igor Mammedov, Mark Cave-Ayland,
Laurent Vivier, Thomas Huth, Marcel Apfelbaum, Eduardo Habkost,
Paolo Bonzini, Manos Pitsidianakis, Markus Armbruster,
Mark Cave-Ayland, Peter Maydell
[-- Attachment #1: Type: text/plain, Size: 3559 bytes --]
On Tue, 20 Feb 2024, Philippe Mathieu-Daudé wrote:
> On 19/2/24 19:24, BALATON Zoltan wrote:
>> On Mon, 19 Feb 2024, BALATON Zoltan wrote:
>>> On Mon, 19 Feb 2024, Philippe Mathieu-Daudé wrote:
>>>> Expose TYPE_ICH_DMI_PCI_BRIDGE to the new
>>>> "hw/pci-bridge/ich_dmi_pci.h" header.
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>> ---
>>>> MAINTAINERS | 1 +
>>>> include/hw/pci-bridge/ich_dmi_pci.h | 20 ++++++++++++++++++++
>>>> include/hw/southbridge/ich9.h | 2 --
>>>> hw/pci-bridge/i82801b11.c | 11 ++++-------
>>>> 4 files changed, 25 insertions(+), 9 deletions(-)
>>>> create mode 100644 include/hw/pci-bridge/ich_dmi_pci.h
>>>>
>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>> index 1b210c5cc1..50507c3dd6 100644
>>>> --- a/MAINTAINERS
>>>> +++ b/MAINTAINERS
>>>> @@ -2609,6 +2609,7 @@ F: hw/acpi/ich9*.c
>>>> F: hw/i2c/smbus_ich9.c
>>>> F: hw/isa/lpc_ich9.c
>>>> F: include/hw/acpi/ich9*.h
>>>> +F: include/hw/pci-bridge/ich_dmi_pci.h
>>>> F: include/hw/southbridge/ich9.h
>>>>
>>>> PIIX4 South Bridge (i82371AB)
>>>> diff --git a/include/hw/pci-bridge/ich_dmi_pci.h
>>>> b/include/hw/pci-bridge/ich_dmi_pci.h
>>>> new file mode 100644
>>>> index 0000000000..7623b32b8e
>>>> --- /dev/null
>>>> +++ b/include/hw/pci-bridge/ich_dmi_pci.h
>>>> @@ -0,0 +1,20 @@
>>>> +/*
>>>> + * QEMU ICH4 i82801b11 dmi-to-pci Bridge Emulation
>>>> + *
>>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>>> + */
>>>> +
>>>> +#ifndef HW_PCI_BRIDGE_ICH_D2P_H
>>>> +#define HW_PCI_BRIDGE_ICH_D2P_H
>>>> +
>>>> +#include "qom/object.h"
>>>> +#include "hw/pci/pci_bridge.h"
>>>> +
>>>> +#define TYPE_ICH_DMI_PCI_BRIDGE "i82801b11-bridge"
>>>> +OBJECT_DECLARE_SIMPLE_TYPE(I82801b11Bridge, ICH_DMI_PCI_BRIDGE)
>>>> +
>>>> +struct I82801b11Bridge {
>>>> + PCIBridge parent_obj;
>>>> +};
>>>
>>> If this class has no fields of its own why does it need its own state
>>> struct defined? You could just set .instance_size = sizeof(PCIBridge) in
>>> the TypeInfo i82801b11_bridge_info below and delete this struct completely
>>> as it's not even used anywhere. One less needless QOM complication :-) For
>>> an example see the empty via-mc97 device in hw/audio/via-ac97.c.
>>>
>>> Then you can put the OBJECT_DECLARE_SIMPLE_TYPE in
>>> hw/pci-bridge/i82801b11.c where this object is defined and the #define
>>> TYPE_ICH_DMI_PCI_BRIDGE in
>>
>> You don't even need OBJECT_DECLARE_SIMPLE_TYPE if there's no state struct.
>> But on second look what is this object at all? It's never instantiated
>> anywhere. Is it used somewhere?
>
> Here my view is we should always define QOM type names in headers
> and use them, in particular in the TypeInfo registration. To unify
> style and copy/pasting, better use the QOM DECLARE_TYPE macros.
> I envision that might help moving toward DSL and have HW modelling
> checks done externally, before starting QEMU. But then this is my
> view and I dunno about when we'll get that DSL in so I'm OK to
> revisit this patch.
The question here is more if we need this object at all because it wasn't
enstantiated before, and after your series it could be instantiated by a
property that's never set. So unless I misunderstood somthing this whole
thing could just be removed as dead code and let it be re-added later when
it's actually implemented following whatever conventions we'll have then.
No need to keep around empty placeholders that aren't used. Or does it
serve any purpose?
Regards,
BALATON Zoltan
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 06/14] hw/pci-bridge: Extract QOM ICH definitions to 'ich_dmi_pci.h'
2024-02-20 12:20 ` BALATON Zoltan
@ 2024-02-20 12:55 ` Thomas Huth
2024-02-26 13:46 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 28+ messages in thread
From: Thomas Huth @ 2024-02-20 12:55 UTC (permalink / raw)
To: BALATON Zoltan, Philippe Mathieu-Daudé
Cc: qemu-devel, Bernhard Beschow, Michael S. Tsirkin, Ani Sinha,
Richard Henderson, Igor Mammedov, Mark Cave-Ayland,
Laurent Vivier, Marcel Apfelbaum, Eduardo Habkost, Paolo Bonzini,
Manos Pitsidianakis, Markus Armbruster, Peter Maydell
On 20/02/2024 13.20, BALATON Zoltan wrote:
> On Tue, 20 Feb 2024, Philippe Mathieu-Daudé wrote:
>> On 19/2/24 19:24, BALATON Zoltan wrote:
>>> On Mon, 19 Feb 2024, BALATON Zoltan wrote:
>>>> On Mon, 19 Feb 2024, Philippe Mathieu-Daudé wrote:
>>>>> Expose TYPE_ICH_DMI_PCI_BRIDGE to the new
>>>>> "hw/pci-bridge/ich_dmi_pci.h" header.
>>>>>
>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>> ---
>>>>> MAINTAINERS | 1 +
>>>>> include/hw/pci-bridge/ich_dmi_pci.h | 20 ++++++++++++++++++++
>>>>> include/hw/southbridge/ich9.h | 2 --
>>>>> hw/pci-bridge/i82801b11.c | 11 ++++-------
>>>>> 4 files changed, 25 insertions(+), 9 deletions(-)
>>>>> create mode 100644 include/hw/pci-bridge/ich_dmi_pci.h
>>>>>
>>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>>> index 1b210c5cc1..50507c3dd6 100644
>>>>> --- a/MAINTAINERS
>>>>> +++ b/MAINTAINERS
>>>>> @@ -2609,6 +2609,7 @@ F: hw/acpi/ich9*.c
>>>>> F: hw/i2c/smbus_ich9.c
>>>>> F: hw/isa/lpc_ich9.c
>>>>> F: include/hw/acpi/ich9*.h
>>>>> +F: include/hw/pci-bridge/ich_dmi_pci.h
>>>>> F: include/hw/southbridge/ich9.h
>>>>>
>>>>> PIIX4 South Bridge (i82371AB)
>>>>> diff --git a/include/hw/pci-bridge/ich_dmi_pci.h
>>>>> b/include/hw/pci-bridge/ich_dmi_pci.h
>>>>> new file mode 100644
>>>>> index 0000000000..7623b32b8e
>>>>> --- /dev/null
>>>>> +++ b/include/hw/pci-bridge/ich_dmi_pci.h
>>>>> @@ -0,0 +1,20 @@
>>>>> +/*
>>>>> + * QEMU ICH4 i82801b11 dmi-to-pci Bridge Emulation
>>>>> + *
>>>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>>>> + */
>>>>> +
>>>>> +#ifndef HW_PCI_BRIDGE_ICH_D2P_H
>>>>> +#define HW_PCI_BRIDGE_ICH_D2P_H
>>>>> +
>>>>> +#include "qom/object.h"
>>>>> +#include "hw/pci/pci_bridge.h"
>>>>> +
>>>>> +#define TYPE_ICH_DMI_PCI_BRIDGE "i82801b11-bridge"
>>>>> +OBJECT_DECLARE_SIMPLE_TYPE(I82801b11Bridge, ICH_DMI_PCI_BRIDGE)
>>>>> +
>>>>> +struct I82801b11Bridge {
>>>>> + PCIBridge parent_obj;
>>>>> +};
>>>>
>>>> If this class has no fields of its own why does it need its own state
>>>> struct defined? You could just set .instance_size = sizeof(PCIBridge) in
>>>> the TypeInfo i82801b11_bridge_info below and delete this struct
>>>> completely as it's not even used anywhere. One less needless QOM
>>>> complication :-) For an example see the empty via-mc97 device in
>>>> hw/audio/via-ac97.c.
>>>>
>>>> Then you can put the OBJECT_DECLARE_SIMPLE_TYPE in
>>>> hw/pci-bridge/i82801b11.c where this object is defined and the #define
>>>> TYPE_ICH_DMI_PCI_BRIDGE in
>>>
>>> You don't even need OBJECT_DECLARE_SIMPLE_TYPE if there's no state
>>> struct. But on second look what is this object at all? It's never
>>> instantiated anywhere. Is it used somewhere?
>>
>> Here my view is we should always define QOM type names in headers
>> and use them, in particular in the TypeInfo registration. To unify
>> style and copy/pasting, better use the QOM DECLARE_TYPE macros.
>> I envision that might help moving toward DSL and have HW modelling
>> checks done externally, before starting QEMU. But then this is my
>> view and I dunno about when we'll get that DSL in so I'm OK to
>> revisit this patch.
>
> The question here is more if we need this object at all because it wasn't
> enstantiated before, and after your series it could be instantiated by a
> property that's never set. So unless I misunderstood somthing this whole
> thing could just be removed as dead code and let it be re-added later when
> it's actually implemented following whatever conventions we'll have then. No
> need to keep around empty placeholders that aren't used. Or does it serve
> any purpose?
It's apparently used by some q35 configs:
$ grep -r i82801b11 docs/
docs/config/q35-emulated.cfg: driver = "i82801b11-bridge"
Thomas
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 06/14] hw/pci-bridge: Extract QOM ICH definitions to 'ich_dmi_pci.h'
2024-02-19 16:38 ` [PATCH 06/14] hw/pci-bridge: Extract QOM ICH definitions to 'ich_dmi_pci.h' Philippe Mathieu-Daudé
2024-02-19 18:15 ` BALATON Zoltan
@ 2024-02-20 19:25 ` Bernhard Beschow
2024-02-21 8:54 ` Philippe Mathieu-Daudé
1 sibling, 1 reply; 28+ messages in thread
From: Bernhard Beschow @ 2024-02-20 19:25 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: Michael S. Tsirkin, Ani Sinha, Richard Henderson, Igor Mammedov,
Mark Cave-Ayland, Laurent Vivier, Thomas Huth, Marcel Apfelbaum,
Eduardo Habkost, Paolo Bonzini, BALATON Zoltan
Am 19. Februar 2024 16:38:46 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>Expose TYPE_ICH_DMI_PCI_BRIDGE to the new
>"hw/pci-bridge/ich_dmi_pci.h" header.
>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>---
> MAINTAINERS | 1 +
> include/hw/pci-bridge/ich_dmi_pci.h | 20 ++++++++++++++++++++
> include/hw/southbridge/ich9.h | 2 --
> hw/pci-bridge/i82801b11.c | 11 ++++-------
> 4 files changed, 25 insertions(+), 9 deletions(-)
> create mode 100644 include/hw/pci-bridge/ich_dmi_pci.h
>
>diff --git a/MAINTAINERS b/MAINTAINERS
>index 1b210c5cc1..50507c3dd6 100644
>--- a/MAINTAINERS
>+++ b/MAINTAINERS
>@@ -2609,6 +2609,7 @@ F: hw/acpi/ich9*.c
> F: hw/i2c/smbus_ich9.c
> F: hw/isa/lpc_ich9.c
> F: include/hw/acpi/ich9*.h
>+F: include/hw/pci-bridge/ich_dmi_pci.h
> F: include/hw/southbridge/ich9.h
>
> PIIX4 South Bridge (i82371AB)
>diff --git a/include/hw/pci-bridge/ich_dmi_pci.h b/include/hw/pci-bridge/ich_dmi_pci.h
>new file mode 100644
>index 0000000000..7623b32b8e
>--- /dev/null
>+++ b/include/hw/pci-bridge/ich_dmi_pci.h
Shouldn't we name the new header like its source file, i.e. i82801b11.h?
>@@ -0,0 +1,20 @@
>+/*
>+ * QEMU ICH4 i82801b11 dmi-to-pci Bridge Emulation
>+ *
>+ * SPDX-License-Identifier: GPL-2.0-or-later
>+ */
>+
>+#ifndef HW_PCI_BRIDGE_ICH_D2P_H
>+#define HW_PCI_BRIDGE_ICH_D2P_H
>+
>+#include "qom/object.h"
>+#include "hw/pci/pci_bridge.h"
>+
>+#define TYPE_ICH_DMI_PCI_BRIDGE "i82801b11-bridge"
>+OBJECT_DECLARE_SIMPLE_TYPE(I82801b11Bridge, ICH_DMI_PCI_BRIDGE)
>+
>+struct I82801b11Bridge {
>+ PCIBridge parent_obj;
>+};
>+
>+#endif
>diff --git a/include/hw/southbridge/ich9.h b/include/hw/southbridge/ich9.h
>index bee522a4cf..b2abf483e0 100644
>--- a/include/hw/southbridge/ich9.h
>+++ b/include/hw/southbridge/ich9.h
>@@ -114,8 +114,6 @@ struct ICH9LPCState {
>
> #define ICH9_D2P_SECONDARY_DEFAULT (256 - 8)
>
>-#define ICH9_D2P_A2_REVISION 0x92
>-
> /* D31:F0 LPC Processor Interface */
> #define ICH9_RST_CNT_IOPORT 0xCF9
>
>diff --git a/hw/pci-bridge/i82801b11.c b/hw/pci-bridge/i82801b11.c
>index c140919cbc..dd17e35b0a 100644
>--- a/hw/pci-bridge/i82801b11.c
>+++ b/hw/pci-bridge/i82801b11.c
>@@ -45,7 +45,7 @@
> #include "hw/pci/pci_bridge.h"
> #include "migration/vmstate.h"
> #include "qemu/module.h"
>-#include "hw/southbridge/ich9.h"
>+#include "hw/pci-bridge/ich_dmi_pci.h"
>
> /*****************************************************************************/
> /* ICH9 DMI-to-PCI bridge */
>@@ -53,11 +53,8 @@
> #define I82801ba_SSVID_SVID 0
> #define I82801ba_SSVID_SSID 0
>
>-typedef struct I82801b11Bridge {
>- /*< private >*/
>- PCIBridge parent_obj;
>- /*< public >*/
>-} I82801b11Bridge;
>+
>+#define ICH9_D2P_A2_REVISION 0x92
>
> static void i82801b11_bridge_realize(PCIDevice *d, Error **errp)
> {
>@@ -103,7 +100,7 @@ static void i82801b11_bridge_class_init(ObjectClass *klass, void *data)
> }
>
> static const TypeInfo i82801b11_bridge_info = {
>- .name = "i82801b11-bridge",
>+ .name = TYPE_ICH_DMI_PCI_BRIDGE,
> .parent = TYPE_PCI_BRIDGE,
> .instance_size = sizeof(I82801b11Bridge),
> .class_init = i82801b11_bridge_class_init,
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 06/14] hw/pci-bridge: Extract QOM ICH definitions to 'ich_dmi_pci.h'
2024-02-20 19:25 ` Bernhard Beschow
@ 2024-02-21 8:54 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 28+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-02-21 8:54 UTC (permalink / raw)
To: Bernhard Beschow, qemu-devel
Cc: Michael S. Tsirkin, Ani Sinha, Richard Henderson, Igor Mammedov,
Mark Cave-Ayland, Laurent Vivier, Thomas Huth, Marcel Apfelbaum,
Eduardo Habkost, Paolo Bonzini, BALATON Zoltan
On 20/2/24 20:25, Bernhard Beschow wrote:
>
>
> Am 19. Februar 2024 16:38:46 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>> Expose TYPE_ICH_DMI_PCI_BRIDGE to the new
>> "hw/pci-bridge/ich_dmi_pci.h" header.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> MAINTAINERS | 1 +
>> include/hw/pci-bridge/ich_dmi_pci.h | 20 ++++++++++++++++++++
>> include/hw/southbridge/ich9.h | 2 --
>> hw/pci-bridge/i82801b11.c | 11 ++++-------
>> 4 files changed, 25 insertions(+), 9 deletions(-)
>> create mode 100644 include/hw/pci-bridge/ich_dmi_pci.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 1b210c5cc1..50507c3dd6 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -2609,6 +2609,7 @@ F: hw/acpi/ich9*.c
>> F: hw/i2c/smbus_ich9.c
>> F: hw/isa/lpc_ich9.c
>> F: include/hw/acpi/ich9*.h
>> +F: include/hw/pci-bridge/ich_dmi_pci.h
>> F: include/hw/southbridge/ich9.h
>>
>> PIIX4 South Bridge (i82371AB)
>> diff --git a/include/hw/pci-bridge/ich_dmi_pci.h b/include/hw/pci-bridge/ich_dmi_pci.h
>> new file mode 100644
>> index 0000000000..7623b32b8e
>> --- /dev/null
>> +++ b/include/hw/pci-bridge/ich_dmi_pci.h
>
> Shouldn't we name the new header like its source file, i.e. i82801b11.h?
I'll rename sources to use the "ichX_FOO.c" pattern.
>> @@ -0,0 +1,20 @@
>> +/*
>> + * QEMU ICH4 i82801b11 dmi-to-pci Bridge Emulation
>> + *
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + */
>> +
>> +#ifndef HW_PCI_BRIDGE_ICH_D2P_H
>> +#define HW_PCI_BRIDGE_ICH_D2P_H
...
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 05/14] hw/acpi/ich9_tco: Restrict ich9_generate_smi() declaration
2024-02-20 6:32 ` Philippe Mathieu-Daudé
@ 2024-02-26 9:53 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 28+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-02-26 9:53 UTC (permalink / raw)
To: qemu-devel, Bernhard Beschow
Cc: Michael S. Tsirkin, Ani Sinha, Richard Henderson, Igor Mammedov,
Mark Cave-Ayland, Laurent Vivier, Thomas Huth, Marcel Apfelbaum,
Eduardo Habkost, Paolo Bonzini, BALATON Zoltan
On 20/2/24 07:32, Philippe Mathieu-Daudé wrote:
> On 19/2/24 17:38, Philippe Mathieu-Daudé wrote:
>> Only files including "hw/acpi/ich9_tco.h" require
>> the ich9_generate_smi() declaration.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> include/hw/acpi/ich9_tco.h | 1 +
>> include/hw/southbridge/ich9.h | 2 --
>> 2 files changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/include/hw/acpi/ich9_tco.h b/include/hw/acpi/ich9_tco.h
>> index 1c99781a79..68ee64942f 100644
>> --- a/include/hw/acpi/ich9_tco.h
>> +++ b/include/hw/acpi/ich9_tco.h
>> @@ -76,6 +76,7 @@ typedef struct TCOIORegs {
>> } TCOIORegs;
>> void ich9_acpi_pm_tco_init(TCOIORegs *tr, MemoryRegion *parent);
>> +void ich9_generate_smi(void);
>
> Bah it is only used in hw/acpi/ich9_tco.c, I'll declare it
> statically there.
Unfortunately can't do that now because I really don't want
to add a x86 specific dependency here:
../../hw/acpi/ich9_tco.c:35:30: error: use of undeclared identifier
'CPU_INTERRUPT_SMI'
cpu_interrupt(first_cpu, CPU_INTERRUPT_SMI);
^
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 06/14] hw/pci-bridge: Extract QOM ICH definitions to 'ich_dmi_pci.h'
2024-02-20 12:55 ` Thomas Huth
@ 2024-02-26 13:46 ` Philippe Mathieu-Daudé
2024-02-26 13:56 ` BALATON Zoltan
0 siblings, 1 reply; 28+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-02-26 13:46 UTC (permalink / raw)
To: Thomas Huth, BALATON Zoltan
Cc: qemu-devel, Bernhard Beschow, Michael S. Tsirkin, Ani Sinha,
Richard Henderson, Igor Mammedov, Mark Cave-Ayland,
Laurent Vivier, Marcel Apfelbaum, Eduardo Habkost, Paolo Bonzini,
Manos Pitsidianakis, Markus Armbruster, Peter Maydell
On 20/2/24 13:55, Thomas Huth wrote:
> On 20/02/2024 13.20, BALATON Zoltan wrote:
>> On Tue, 20 Feb 2024, Philippe Mathieu-Daudé wrote:
>>> On 19/2/24 19:24, BALATON Zoltan wrote:
>>>> On Mon, 19 Feb 2024, BALATON Zoltan wrote:
>>>>> On Mon, 19 Feb 2024, Philippe Mathieu-Daudé wrote:
>>>>>> Expose TYPE_ICH_DMI_PCI_BRIDGE to the new
>>>>>> "hw/pci-bridge/ich_dmi_pci.h" header.
>>>>>>
>>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>>> ---
>>>>>> MAINTAINERS | 1 +
>>>>>> include/hw/pci-bridge/ich_dmi_pci.h | 20 ++++++++++++++++++++
>>>>>> include/hw/southbridge/ich9.h | 2 --
>>>>>> hw/pci-bridge/i82801b11.c | 11 ++++-------
>>>>>> 4 files changed, 25 insertions(+), 9 deletions(-)
>>>>>> create mode 100644 include/hw/pci-bridge/ich_dmi_pci.h
>>>>>>
>>>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>>>> index 1b210c5cc1..50507c3dd6 100644
>>>>>> --- a/MAINTAINERS
>>>>>> +++ b/MAINTAINERS
>>>>>> @@ -2609,6 +2609,7 @@ F: hw/acpi/ich9*.c
>>>>>> F: hw/i2c/smbus_ich9.c
>>>>>> F: hw/isa/lpc_ich9.c
>>>>>> F: include/hw/acpi/ich9*.h
>>>>>> +F: include/hw/pci-bridge/ich_dmi_pci.h
>>>>>> F: include/hw/southbridge/ich9.h
>>>>>>
>>>>>> PIIX4 South Bridge (i82371AB)
>>>>>> diff --git a/include/hw/pci-bridge/ich_dmi_pci.h
>>>>>> b/include/hw/pci-bridge/ich_dmi_pci.h
>>>>>> new file mode 100644
>>>>>> index 0000000000..7623b32b8e
>>>>>> --- /dev/null
>>>>>> +++ b/include/hw/pci-bridge/ich_dmi_pci.h
>>>>>> @@ -0,0 +1,20 @@
>>>>>> +/*
>>>>>> + * QEMU ICH4 i82801b11 dmi-to-pci Bridge Emulation
>>>>>> + *
>>>>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>>>>> + */
>>>>>> +
>>>>>> +#ifndef HW_PCI_BRIDGE_ICH_D2P_H
>>>>>> +#define HW_PCI_BRIDGE_ICH_D2P_H
>>>>>> +
>>>>>> +#include "qom/object.h"
>>>>>> +#include "hw/pci/pci_bridge.h"
>>>>>> +
>>>>>> +#define TYPE_ICH_DMI_PCI_BRIDGE "i82801b11-bridge"
>>>>>> +OBJECT_DECLARE_SIMPLE_TYPE(I82801b11Bridge, ICH_DMI_PCI_BRIDGE)
>>>>>> +
>>>>>> +struct I82801b11Bridge {
>>>>>> + PCIBridge parent_obj;
>>>>>> +};
>>>>>
>>>>> If this class has no fields of its own why does it need its own
>>>>> state struct defined? You could just set .instance_size =
>>>>> sizeof(PCIBridge) in the TypeInfo i82801b11_bridge_info below and
>>>>> delete this struct completely as it's not even used anywhere. One
>>>>> less needless QOM complication :-) For an example see the empty
>>>>> via-mc97 device in hw/audio/via-ac97.c.
>>>>>
>>>>> Then you can put the OBJECT_DECLARE_SIMPLE_TYPE in
>>>>> hw/pci-bridge/i82801b11.c where this object is defined and the
>>>>> #define TYPE_ICH_DMI_PCI_BRIDGE in
>>>>
>>>> You don't even need OBJECT_DECLARE_SIMPLE_TYPE if there's no state
>>>> struct. But on second look what is this object at all? It's never
>>>> instantiated anywhere. Is it used somewhere?
>>>
>>> Here my view is we should always define QOM type names in headers
>>> and use them, in particular in the TypeInfo registration. To unify
>>> style and copy/pasting, better use the QOM DECLARE_TYPE macros.
>>> I envision that might help moving toward DSL and have HW modelling
>>> checks done externally, before starting QEMU. But then this is my
>>> view and I dunno about when we'll get that DSL in so I'm OK to
>>> revisit this patch.
>>
>> The question here is more if we need this object at all because it
>> wasn't enstantiated before, and after your series it could be
>> instantiated by a property that's never set. So unless I misunderstood
>> somthing this whole thing could just be removed as dead code and let
>> it be re-added later when it's actually implemented following whatever
>> conventions we'll have then. No need to keep around empty placeholders
>> that aren't used. Or does it serve any purpose?
This isn't a virtual hardware, and is well specified, I'm trying to
plug all the parts we have so the full chipset can be used to create
a dynamic machine.
> It's apparently used by some q35 configs:
>
> $ grep -r i82801b11 docs/
> docs/config/q35-emulated.cfg: driver = "i82801b11-bridge"
>
> Thomas
>
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 06/14] hw/pci-bridge: Extract QOM ICH definitions to 'ich_dmi_pci.h'
2024-02-26 13:46 ` Philippe Mathieu-Daudé
@ 2024-02-26 13:56 ` BALATON Zoltan
0 siblings, 0 replies; 28+ messages in thread
From: BALATON Zoltan @ 2024-02-26 13:56 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Thomas Huth, qemu-devel, Bernhard Beschow, Michael S. Tsirkin,
Ani Sinha, Richard Henderson, Igor Mammedov, Mark Cave-Ayland,
Laurent Vivier, Marcel Apfelbaum, Eduardo Habkost, Paolo Bonzini,
Manos Pitsidianakis, Markus Armbruster, Peter Maydell
[-- Attachment #1: Type: text/plain, Size: 5070 bytes --]
On Mon, 26 Feb 2024, Philippe Mathieu-Daudé wrote:
> On 20/2/24 13:55, Thomas Huth wrote:
>> On 20/02/2024 13.20, BALATON Zoltan wrote:
>>> On Tue, 20 Feb 2024, Philippe Mathieu-Daudé wrote:
>>>> On 19/2/24 19:24, BALATON Zoltan wrote:
>>>>> On Mon, 19 Feb 2024, BALATON Zoltan wrote:
>>>>>> On Mon, 19 Feb 2024, Philippe Mathieu-Daudé wrote:
>>>>>>> Expose TYPE_ICH_DMI_PCI_BRIDGE to the new
>>>>>>> "hw/pci-bridge/ich_dmi_pci.h" header.
>>>>>>>
>>>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>>>> ---
>>>>>>> MAINTAINERS | 1 +
>>>>>>> include/hw/pci-bridge/ich_dmi_pci.h | 20 ++++++++++++++++++++
>>>>>>> include/hw/southbridge/ich9.h | 2 --
>>>>>>> hw/pci-bridge/i82801b11.c | 11 ++++-------
>>>>>>> 4 files changed, 25 insertions(+), 9 deletions(-)
>>>>>>> create mode 100644 include/hw/pci-bridge/ich_dmi_pci.h
>>>>>>>
>>>>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>>>>> index 1b210c5cc1..50507c3dd6 100644
>>>>>>> --- a/MAINTAINERS
>>>>>>> +++ b/MAINTAINERS
>>>>>>> @@ -2609,6 +2609,7 @@ F: hw/acpi/ich9*.c
>>>>>>> F: hw/i2c/smbus_ich9.c
>>>>>>> F: hw/isa/lpc_ich9.c
>>>>>>> F: include/hw/acpi/ich9*.h
>>>>>>> +F: include/hw/pci-bridge/ich_dmi_pci.h
>>>>>>> F: include/hw/southbridge/ich9.h
>>>>>>>
>>>>>>> PIIX4 South Bridge (i82371AB)
>>>>>>> diff --git a/include/hw/pci-bridge/ich_dmi_pci.h
>>>>>>> b/include/hw/pci-bridge/ich_dmi_pci.h
>>>>>>> new file mode 100644
>>>>>>> index 0000000000..7623b32b8e
>>>>>>> --- /dev/null
>>>>>>> +++ b/include/hw/pci-bridge/ich_dmi_pci.h
>>>>>>> @@ -0,0 +1,20 @@
>>>>>>> +/*
>>>>>>> + * QEMU ICH4 i82801b11 dmi-to-pci Bridge Emulation
>>>>>>> + *
>>>>>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>>>>>> + */
>>>>>>> +
>>>>>>> +#ifndef HW_PCI_BRIDGE_ICH_D2P_H
>>>>>>> +#define HW_PCI_BRIDGE_ICH_D2P_H
>>>>>>> +
>>>>>>> +#include "qom/object.h"
>>>>>>> +#include "hw/pci/pci_bridge.h"
>>>>>>> +
>>>>>>> +#define TYPE_ICH_DMI_PCI_BRIDGE "i82801b11-bridge"
>>>>>>> +OBJECT_DECLARE_SIMPLE_TYPE(I82801b11Bridge, ICH_DMI_PCI_BRIDGE)
>>>>>>> +
>>>>>>> +struct I82801b11Bridge {
>>>>>>> + PCIBridge parent_obj;
>>>>>>> +};
>>>>>>
>>>>>> If this class has no fields of its own why does it need its own state
>>>>>> struct defined? You could just set .instance_size = sizeof(PCIBridge)
>>>>>> in the TypeInfo i82801b11_bridge_info below and delete this struct
>>>>>> completely as it's not even used anywhere. One less needless QOM
>>>>>> complication :-) For an example see the empty via-mc97 device in
>>>>>> hw/audio/via-ac97.c.
>>>>>>
>>>>>> Then you can put the OBJECT_DECLARE_SIMPLE_TYPE in
>>>>>> hw/pci-bridge/i82801b11.c where this object is defined and the #define
>>>>>> TYPE_ICH_DMI_PCI_BRIDGE in
>>>>>
>>>>> You don't even need OBJECT_DECLARE_SIMPLE_TYPE if there's no state
>>>>> struct. But on second look what is this object at all? It's never
>>>>> instantiated anywhere. Is it used somewhere?
>>>>
>>>> Here my view is we should always define QOM type names in headers
>>>> and use them, in particular in the TypeInfo registration. To unify
>>>> style and copy/pasting, better use the QOM DECLARE_TYPE macros.
>>>> I envision that might help moving toward DSL and have HW modelling
>>>> checks done externally, before starting QEMU. But then this is my
>>>> view and I dunno about when we'll get that DSL in so I'm OK to
>>>> revisit this patch.
>>>
>>> The question here is more if we need this object at all because it wasn't
>>> enstantiated before, and after your series it could be instantiated by a
>>> property that's never set. So unless I misunderstood somthing this whole
>>> thing could just be removed as dead code and let it be re-added later when
>>> it's actually implemented following whatever conventions we'll have then.
>>> No need to keep around empty placeholders that aren't used. Or does it
>>> serve any purpose?
>
> This isn't a virtual hardware, and is well specified, I'm trying to
> plug all the parts we have so the full chipset can be used to create
> a dynamic machine.
This isn't prevented by moving this object implementation from its
separate file to the file where the southbridge that's the sole user of
this device it implemented. The rationale is that there's no need to make
it a full object with all the headers and defines when it's not used
anywhere else than that southbridge and its "implementation" consists of
only the TypeInfo and a realize method. These could just be defined in the
southbridge as a local object and do away with all the other boilerplate
which would remove some unneeded cruft. This won't affect your modelling
of the southbridge as you can still instantiate it there but don't need to
define this empty bridge device as external object when it's not needed
outside of the south bridge. Just like the via-mc97 device. I did not put
that in a separate file and add a separate header for it because it just
seems to be overkill for an empty device that does nothing.
Regards,
BALATON Zoltan
^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2024-02-26 14:18 UTC | newest]
Thread overview: 28+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-02-19 16:38 [PATCH 00/14] hw/southbridge: Extract ICH9 QOM container model Philippe Mathieu-Daudé
2024-02-19 16:38 ` [PATCH 01/14] MAINTAINERS: Add 'ICH9 South Bridge' section Philippe Mathieu-Daudé
2024-02-19 16:38 ` [PATCH 02/14] hw/i386/q35: Add local 'lpc_obj' variable Philippe Mathieu-Daudé
2024-02-19 16:38 ` [PATCH 03/14] hw/acpi/ich9: Restrict definitions from 'hw/southbridge/ich9.h' Philippe Mathieu-Daudé
2024-02-19 16:38 ` [PATCH 04/14] hw/acpi/ich9_tco: Include 'ich9' in names Philippe Mathieu-Daudé
2024-02-19 16:38 ` [PATCH 05/14] hw/acpi/ich9_tco: Restrict ich9_generate_smi() declaration Philippe Mathieu-Daudé
2024-02-20 6:32 ` Philippe Mathieu-Daudé
2024-02-26 9:53 ` Philippe Mathieu-Daudé
2024-02-19 16:38 ` [PATCH 06/14] hw/pci-bridge: Extract QOM ICH definitions to 'ich_dmi_pci.h' Philippe Mathieu-Daudé
2024-02-19 18:15 ` BALATON Zoltan
2024-02-19 18:24 ` BALATON Zoltan
2024-02-20 6:09 ` Philippe Mathieu-Daudé
2024-02-20 12:20 ` BALATON Zoltan
2024-02-20 12:55 ` Thomas Huth
2024-02-26 13:46 ` Philippe Mathieu-Daudé
2024-02-26 13:56 ` BALATON Zoltan
2024-02-20 19:25 ` Bernhard Beschow
2024-02-21 8:54 ` Philippe Mathieu-Daudé
2024-02-19 16:38 ` [PATCH 07/14] hw/southbridge/ich9: Introduce TYPE_ICH9_SOUTHBRIDGE stub Philippe Mathieu-Daudé
2024-02-19 16:38 ` [PATCH 08/14] hw/southbridge/ich9: Add the DMI-to-PCI bridge Philippe Mathieu-Daudé
2024-02-19 16:38 ` [PATCH 09/14] hw/southbridge/ich9: Add a AHCI function Philippe Mathieu-Daudé
2024-02-19 18:31 ` BALATON Zoltan
2024-02-20 6:01 ` Philippe Mathieu-Daudé
2024-02-19 16:38 ` [PATCH 10/14] hw/i2c/smbus: Extract QOM ICH9 definitions to 'smbus_ich9.h' Philippe Mathieu-Daudé
2024-02-19 16:38 ` [PATCH 11/14] hw/southbridge/ich9: Add the SMBus function Philippe Mathieu-Daudé
2024-02-19 16:38 ` [PATCH 12/14] hw/southbridge/ich9: Add the USB EHCI/UHCI functions Philippe Mathieu-Daudé
2024-02-19 16:38 ` [PATCH 13/14] hw/southbridge/ich9: Extract LPC definitions to 'hw/isa/ich9_lpc.h' Philippe Mathieu-Daudé
2024-02-19 16:38 ` [PATCH 14/14] hw/southbridge/ich9: Add the LPC / ISA bridge function Philippe Mathieu-Daudé
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).