Re: [PATCH v1 1/1] Q35 Support
Hi Joel,
Nice! I've been working on making the PIIX south bridge Xen agnostic, partly to
show how Xen enablement in Q35 could look like. Not that I'd have any use case
for it but great to see that you've actually done that!
I know you didn't intend to send this patch but I'll give you some early
comments anyway.
Am 20. Juni 2023 17:24:35 UTC schrieb Joel Upham :
>---
> hw/acpi/ich9.c| 22 +-
> hw/acpi/pcihp.c |6 +-
> hw/core/machine.c | 19 +
> hw/i386/pc_piix.c |3 +-
> hw/i386/pc_q35.c | 39 +-
> hw/i386/xen/xen-hvm.c |7 +-
> hw/i386/xen/xen_platform.c| 19 +-
> hw/isa/lpc_ich9.c | 53 +-
> hw/isa/piix3.c|2 +-
> hw/pci-host/q35.c | 28 +-
> hw/pci/pci.c | 17 +
> hw/xen/xen-host-pci-device.c | 106 +++-
> hw/xen/xen-host-pci-device.h |6 +-
> hw/xen/xen_pt.c | 49 +-
> hw/xen/xen_pt.h | 19 +-
> hw/xen/xen_pt_config_init.c | 1103 ++---
> include/hw/acpi/ich9.h|1 +
> include/hw/acpi/pcihp.h |2 +
> include/hw/boards.h |1 +
> include/hw/i386/pc.h |3 +
> include/hw/pci-host/q35.h |4 +-
> include/hw/pci/pci.h |3 +
> include/hw/southbridge/ich9.h |1 +
> include/hw/xen/xen.h |4 +-
> qemu-options.hx |1 +
> softmmu/datadir.c |1 -
> softmmu/qdev-monitor.c|3 +-
> stubs/xen-hw-stub.c |4 +-
> 28 files changed, 1395 insertions(+), 131 deletions(-)
>
>diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
>index 25e2c7243e..234706a191 100644
>--- a/hw/acpi/ich9.c
>+++ b/hw/acpi/ich9.c
>@@ -39,6 +39,8 @@
> #include "hw/southbridge/ich9.h"
> #include "hw/mem/pc-dimm.h"
> #include "hw/mem/nvdimm.h"
>+#include "hw/xen/xen.h"
>+#include "sysemu/xen.h"
>
> //#define DEBUG
>
>@@ -67,6 +69,10 @@ static void ich9_gpe_writeb(void *opaque, hwaddr addr,
>uint64_t val,
> ICH9LPCPMRegs *pm = opaque;
> acpi_gpe_ioport_writeb(>acpi_regs, addr, val);
> acpi_update_sci(>acpi_regs, pm->irq);
>+
>+if (xen_enabled()) {
>+acpi_pcihp_reset(>acpi_pci_hotplug);
>+}
> }
>
> static const MemoryRegionOps ich9_gpe_ops = {
>@@ -137,7 +143,8 @@ static int ich9_pm_post_load(void *opaque, int version_id)
> {
> ICH9LPCPMRegs *pm = opaque;
> uint32_t pm_io_base = pm->pm_io_base;
>-pm->pm_io_base = 0;
>+if (!xen_enabled())
>+pm->pm_io_base = 0;
> ich9_pm_iospace_update(pm, pm_io_base);
> return 0;
> }
>@@ -268,7 +275,10 @@ static void pm_reset(void *opaque)
> acpi_pm1_evt_reset(>acpi_regs);
> acpi_pm1_cnt_reset(>acpi_regs);
> acpi_pm_tmr_reset(>acpi_regs);
>-acpi_gpe_reset(>acpi_regs);
>+/* Noticed guest freezing in xen when this was reset after S3. */
>+if (!xen_enabled()) {
>+acpi_gpe_reset(>acpi_regs);
>+}
I wonder why this seems to work with PIIX?
I'd rather try to keep the Xen impact on the device model as low as possible,
ideally as low as in the PIIX4 ACPI device model.
>
> pm->smi_en = 0;
> if (!pm->smm_enabled) {
>@@ -316,7 +326,7 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
>qemu_irq sci_irq)
> acpi_pm_tco_init(>tco_regs, >io);
> }
>
>-if (pm->acpi_pci_hotplug.use_acpi_hotplug_bridge) {
>+if (pm->acpi_pci_hotplug.use_acpi_hotplug_bridge || xen_enabled()) {
> acpi_pcihp_init(OBJECT(lpc_pci),
> >acpi_pci_hotplug,
> pci_get_bus(lpc_pci),
>@@ -332,10 +342,14 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
>qemu_irq sci_irq)
> pm->powerdown_notifier.notify = pm_powerdown_req;
> qemu_register_powerdown_notifier(>powerdown_notifier);
>
>+if (xen_enabled()) {
>+acpi_set_pci_info(true);
>+}
>+
> legacy_acpi_cpu_hotplug_init(pci_address_space_io(lpc_pci),
> OBJECT(lpc_pci), >gpe_cpu, ICH9_CPU_HOTPLUG_IO_BASE);
>
>-if (pm->acpi_memory_hotplug.is_enabled) {
>+if (pm->acpi_memory_hotplug.is_enabled || xen_enabled()) {
> acpi_memory_hotplug_init(pci_address_space_io(lpc_pci),
> OBJECT(lpc_pci),
> >acpi_memory_hotplug,
> ACPI_MEMORY_HOTPLUG_BASE);
>diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
>index cdd6f775a1..5b065d670c 100644
>--- a/hw/acpi/pcihp.c
>+++ b/hw/acpi/pcihp.c
>@@ -40,6 +40,7 @@
> #include "qapi/error.h"
> #include "qom/qom-qobject.h"
> #include "trace.h"
>+#include "sysemu/xen.h"
>
> #define ACPI_PCIHP_SIZE 0x0018
> #define PCI_UP_BASE 0x
>@@ -84,7 +85,8 @@ static void *acpi_set_bsel(PCIBus *bus, void *opaque)
> bool is_bridge = IS_PCI_BRIDGE(br);
>
> /* hotplugged bridges can't be described in ACPI ignore them */
>-if (qbus_is_hotpluggable(BUS(bus))) {
>+/* Xen requires hotplugging to the root device, even on the Q35
Re: [PATCH v1 1/1] Q35 Support
On 21.06.23 18:35, Joel Upham wrote: Sorry, this was sent in error when I did the git send-email for the folder. This was before I broke each patch down (after looking at the Qemu submission guidance). This is my first time sending a patch in this way, so thanks for the understanding. This patch can be ignored, as they are all covered elsewhere. We've all been there (messing with git send-email), no need to feel bad :) -- Cheers, David / dhildenb
Re: [PATCH v1 1/1] Q35 Support
Sorry, this was sent in error when I did the git send-email for the folder. This was before I broke each patch down (after looking at the Qemu submission guidance). This is my first time sending a patch in this way, so thanks for the understanding. This patch can be ignored, as they are all covered elsewhere. -Joel Upham On Wed, Jun 21, 2023 at 7:10 AM David Hildenbrand wrote: > On 20.06.23 19:24, Joel Upham wrote: > > Inexpressive patch subject and non-existant patch desciption. I have no > clue what this is supposed to do, except that it involes q35 and xen ()I > guess ?. > > > --- > > hw/acpi/ich9.c| 22 +- > > hw/acpi/pcihp.c |6 +- > > hw/core/machine.c | 19 + > > hw/i386/pc_piix.c |3 +- > > hw/i386/pc_q35.c | 39 +- > > hw/i386/xen/xen-hvm.c |7 +- > > hw/i386/xen/xen_platform.c| 19 +- > > hw/isa/lpc_ich9.c | 53 +- > > hw/isa/piix3.c|2 +- > > hw/pci-host/q35.c | 28 +- > > hw/pci/pci.c | 17 + > > hw/xen/xen-host-pci-device.c | 106 +++- > > hw/xen/xen-host-pci-device.h |6 +- > > hw/xen/xen_pt.c | 49 +- > > hw/xen/xen_pt.h | 19 +- > > hw/xen/xen_pt_config_init.c | 1103 ++--- > > include/hw/acpi/ich9.h|1 + > > include/hw/acpi/pcihp.h |2 + > > include/hw/boards.h |1 + > > include/hw/i386/pc.h |3 + > > include/hw/pci-host/q35.h |4 +- > > include/hw/pci/pci.h |3 + > > include/hw/southbridge/ich9.h |1 + > > include/hw/xen/xen.h |4 +- > > qemu-options.hx |1 + > > softmmu/datadir.c |1 - > > softmmu/qdev-monitor.c|3 +- > > stubs/xen-hw-stub.c |4 +- > > 28 files changed, 1395 insertions(+), 131 deletions(-) > > > > Usually people refrain from reviewing such massive patches. Most > probably this can be broken up into reviewable pieces. > > Was this supposed to be an RFC? > > -- > Cheers, > > David / dhildenb > >
Re: [PATCH v1 1/1] Q35 Support
On 20.06.23 19:24, Joel Upham wrote: Inexpressive patch subject and non-existant patch desciption. I have no clue what this is supposed to do, except that it involes q35 and xen ()I guess ?. --- hw/acpi/ich9.c| 22 +- hw/acpi/pcihp.c |6 +- hw/core/machine.c | 19 + hw/i386/pc_piix.c |3 +- hw/i386/pc_q35.c | 39 +- hw/i386/xen/xen-hvm.c |7 +- hw/i386/xen/xen_platform.c| 19 +- hw/isa/lpc_ich9.c | 53 +- hw/isa/piix3.c|2 +- hw/pci-host/q35.c | 28 +- hw/pci/pci.c | 17 + hw/xen/xen-host-pci-device.c | 106 +++- hw/xen/xen-host-pci-device.h |6 +- hw/xen/xen_pt.c | 49 +- hw/xen/xen_pt.h | 19 +- hw/xen/xen_pt_config_init.c | 1103 ++--- include/hw/acpi/ich9.h|1 + include/hw/acpi/pcihp.h |2 + include/hw/boards.h |1 + include/hw/i386/pc.h |3 + include/hw/pci-host/q35.h |4 +- include/hw/pci/pci.h |3 + include/hw/southbridge/ich9.h |1 + include/hw/xen/xen.h |4 +- qemu-options.hx |1 + softmmu/datadir.c |1 - softmmu/qdev-monitor.c|3 +- stubs/xen-hw-stub.c |4 +- 28 files changed, 1395 insertions(+), 131 deletions(-) Usually people refrain from reviewing such massive patches. Most probably this can be broken up into reviewable pieces. Was this supposed to be an RFC? -- Cheers, David / dhildenb
[PATCH v1 1/1] Q35 Support
---
hw/acpi/ich9.c| 22 +-
hw/acpi/pcihp.c |6 +-
hw/core/machine.c | 19 +
hw/i386/pc_piix.c |3 +-
hw/i386/pc_q35.c | 39 +-
hw/i386/xen/xen-hvm.c |7 +-
hw/i386/xen/xen_platform.c| 19 +-
hw/isa/lpc_ich9.c | 53 +-
hw/isa/piix3.c|2 +-
hw/pci-host/q35.c | 28 +-
hw/pci/pci.c | 17 +
hw/xen/xen-host-pci-device.c | 106 +++-
hw/xen/xen-host-pci-device.h |6 +-
hw/xen/xen_pt.c | 49 +-
hw/xen/xen_pt.h | 19 +-
hw/xen/xen_pt_config_init.c | 1103 ++---
include/hw/acpi/ich9.h|1 +
include/hw/acpi/pcihp.h |2 +
include/hw/boards.h |1 +
include/hw/i386/pc.h |3 +
include/hw/pci-host/q35.h |4 +-
include/hw/pci/pci.h |3 +
include/hw/southbridge/ich9.h |1 +
include/hw/xen/xen.h |4 +-
qemu-options.hx |1 +
softmmu/datadir.c |1 -
softmmu/qdev-monitor.c|3 +-
stubs/xen-hw-stub.c |4 +-
28 files changed, 1395 insertions(+), 131 deletions(-)
diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 25e2c7243e..234706a191 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -39,6 +39,8 @@
#include "hw/southbridge/ich9.h"
#include "hw/mem/pc-dimm.h"
#include "hw/mem/nvdimm.h"
+#include "hw/xen/xen.h"
+#include "sysemu/xen.h"
//#define DEBUG
@@ -67,6 +69,10 @@ static void ich9_gpe_writeb(void *opaque, hwaddr addr,
uint64_t val,
ICH9LPCPMRegs *pm = opaque;
acpi_gpe_ioport_writeb(>acpi_regs, addr, val);
acpi_update_sci(>acpi_regs, pm->irq);
+
+if (xen_enabled()) {
+acpi_pcihp_reset(>acpi_pci_hotplug);
+}
}
static const MemoryRegionOps ich9_gpe_ops = {
@@ -137,7 +143,8 @@ static int ich9_pm_post_load(void *opaque, int version_id)
{
ICH9LPCPMRegs *pm = opaque;
uint32_t pm_io_base = pm->pm_io_base;
-pm->pm_io_base = 0;
+if (!xen_enabled())
+pm->pm_io_base = 0;
ich9_pm_iospace_update(pm, pm_io_base);
return 0;
}
@@ -268,7 +275,10 @@ static void pm_reset(void *opaque)
acpi_pm1_evt_reset(>acpi_regs);
acpi_pm1_cnt_reset(>acpi_regs);
acpi_pm_tmr_reset(>acpi_regs);
-acpi_gpe_reset(>acpi_regs);
+/* Noticed guest freezing in xen when this was reset after S3. */
+if (!xen_enabled()) {
+acpi_gpe_reset(>acpi_regs);
+}
pm->smi_en = 0;
if (!pm->smm_enabled) {
@@ -316,7 +326,7 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
qemu_irq sci_irq)
acpi_pm_tco_init(>tco_regs, >io);
}
-if (pm->acpi_pci_hotplug.use_acpi_hotplug_bridge) {
+if (pm->acpi_pci_hotplug.use_acpi_hotplug_bridge || xen_enabled()) {
acpi_pcihp_init(OBJECT(lpc_pci),
>acpi_pci_hotplug,
pci_get_bus(lpc_pci),
@@ -332,10 +342,14 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
qemu_irq sci_irq)
pm->powerdown_notifier.notify = pm_powerdown_req;
qemu_register_powerdown_notifier(>powerdown_notifier);
+if (xen_enabled()) {
+acpi_set_pci_info(true);
+}
+
legacy_acpi_cpu_hotplug_init(pci_address_space_io(lpc_pci),
OBJECT(lpc_pci), >gpe_cpu, ICH9_CPU_HOTPLUG_IO_BASE);
-if (pm->acpi_memory_hotplug.is_enabled) {
+if (pm->acpi_memory_hotplug.is_enabled || xen_enabled()) {
acpi_memory_hotplug_init(pci_address_space_io(lpc_pci),
OBJECT(lpc_pci),
>acpi_memory_hotplug,
ACPI_MEMORY_HOTPLUG_BASE);
diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index cdd6f775a1..5b065d670c 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -40,6 +40,7 @@
#include "qapi/error.h"
#include "qom/qom-qobject.h"
#include "trace.h"
+#include "sysemu/xen.h"
#define ACPI_PCIHP_SIZE 0x0018
#define PCI_UP_BASE 0x
@@ -84,7 +85,8 @@ static void *acpi_set_bsel(PCIBus *bus, void *opaque)
bool is_bridge = IS_PCI_BRIDGE(br);
/* hotplugged bridges can't be described in ACPI ignore them */
-if (qbus_is_hotpluggable(BUS(bus))) {
+/* Xen requires hotplugging to the root device, even on the Q35 chipset */
+if (qbus_is_hotpluggable(BUS(bus)) || xen_enabled()) {
if (!is_bridge || (!br->hotplugged && info->has_bridge_hotplug)) {
bus_bsel = g_malloc(sizeof *bus_bsel);
@@ -97,7 +99,7 @@ static void *acpi_set_bsel(PCIBus *bus, void *opaque)
return info;
}
-static void acpi_set_pci_info(bool has_bridge_hotplug)
+void acpi_set_pci_info(bool has_bridge_hotplug)
{
static bool bsel_is_set;
Object *host = acpi_get_i386_pci_host();
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 1000406211..703138d2ec 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -455,6 +455,20 @@ static void machine_set_graphics(Object
