Xen Q35 Bringup patches + support for PCIe Extended Capabilities for passed through devices

This patch adds the DSDT table for Q35 (new tools/libacpi/dsdt_q35.asl file). There are not many differences with dsdt.asl (for i440) at the moment, namely:

- BDF location of LPC Controller
- Minor changes related to FDC detection
- Addition of _OSC method to inform OSPM about PCIe features supported

As we are still using 4 PCI router links and their corresponding device/register addresses are same (offset 0x60), no need to change PCI routing descriptions.

Also, ACPI hotplug is still used to control passed through device hot (un)plug (as it was for i440).

Signed-off-by Alexey Gerasimenko <[email protected]>

---
 tools/libacpi/dsdt_q35.asl | 551 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 551 insertions(+)
 create mode 100644 tools/libacpi/dsdt_q35.asl

tools/libacpi/dsdt_q35.asl new file +0 -0

new file mode 100644
index 0000000000..cd02946a07

Quoted 34 lines · Alexey Gerasimenko

This patch adds the DSDT table for Q35 (new tools/libacpi/dsdt_q35.asl file). There are not many differences with dsdt.asl (for i440) at the moment, namely:

- BDF location of LPC Controller
- Minor changes related to FDC detection
- Addition of _OSC method to inform OSPM about PCIe features supported

As we are still using 4 PCI router links and their corresponding device/register addresses are same (offset 0x60), no need to change PCI routing descriptions.

Also, ACPI hotplug is still used to control passed through device hot (un)plug (as it was for i440).

Signed-off-by Alexey Gerasimenko <[email protected]>

---
 tools/libacpi/dsdt_q35.asl | 551

+++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 551 insertions(+) create mode 100644 tools/libacpi/dsdt_q35.asl

tools/libacpi/dsdt_q35.asl new file +6 -0

new file mode 100644
index 0000000000..cd02946a07
@@ -0,0 +1,551 @@
+/******************************************************************************
+ * DSDT for Xen with Qemu device model (for Q35 machine)
+ *
+ * Copyright (c) 2004, Intel Corporation.
+ *
+ * This program is free software; you can redistribute it and/or

Quoted 6 lines

modify
+ * it under the terms of the GNU Lesser General Public License as published
+ * by the Free Software Foundation; version 2.1 only. with the special
+ * exception on linking described in file LICENSE.

I don't see the 'LICENSE' file in Xen's directory?

Also, your email does not seem to be coming from Intel, so I have to ask, where did this file originally come from?

Quoted 44 lines · On Mon, 12 Mar 2018 15:38:03 -0400 Konrad Rzeszutek Wilk…

Quoted 34 lines · Alexey Gerasimenko

This patch adds the DSDT table for Q35 (new tools/libacpi/dsdt_q35.asl file). There are not many differences with dsdt.asl (for i440) at the moment, namely:

- BDF location of LPC Controller
- Minor changes related to FDC detection
- Addition of _OSC method to inform OSPM about PCIe features supported

As we are still using 4 PCI router links and their corresponding device/register addresses are same (offset 0x60), no need to change PCI routing descriptions.

Also, ACPI hotplug is still used to control passed through device hot (un)plug (as it was for i440).

Signed-off-by Alexey Gerasimenko <[email protected]>

---
 tools/libacpi/dsdt_q35.asl | 551

+++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 551 insertions(+) create mode 100644 tools/libacpi/dsdt_q35.asl

tools/libacpi/dsdt_q35.asl new file +6 -0

new file mode 100644
index 0000000000..cd02946a07
@@ -0,0 +1,551 @@
+/******************************************************************************
+ * DSDT for Xen with Qemu device model (for Q35 machine)
+ *
+ * Copyright (c) 2004, Intel Corporation.
+ *
+ * This program is free software; you can redistribute it and/or

Quoted 6 lines

modify
+ * it under the terms of the GNU Lesser General Public License as published
+ * by the Free Software Foundation; version 2.1 only. with the special
+ * exception on linking described in file LICENSE.

I don't see the 'LICENSE' file in Xen's directory?

Quoted 2 lines

Also, your email does not seem to be coming from Intel, so I have to ask, where did this file originally come from?

It's basically Xen's dsdt.asl with some modifications related to Q35. Currently only few modifications needed, but in the future dsdt.asl and dsdt_q35.asl will diverge more from each other -- that's the reason why a separate file was forked instead applying these changes to dsdt.asl directly, for example, as #ifdef-parts.

Quoted 48 lines · Alexey G

Quoted 45 lines · On Mon, 12 Mar 2018 15:38:03 -0400 Konrad Rzeszutek Wilk…

Quoted 34 lines · Alexey Gerasimenko

This patch adds the DSDT table for Q35 (new tools/libacpi/dsdt_q35.asl file). There are not many differences with dsdt.asl (for i440) at the moment, namely:

- BDF location of LPC Controller
- Minor changes related to FDC detection
- Addition of _OSC method to inform OSPM about PCIe features supported

As we are still using 4 PCI router links and their corresponding device/register addresses are same (offset 0x60), no need to change PCI routing descriptions.

Also, ACPI hotplug is still used to control passed through device hot (un)plug (as it was for i440).

Signed-off-by Alexey Gerasimenko <[email protected]>

---
 tools/libacpi/dsdt_q35.asl | 551

+++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 551 insertions(+) create mode 100644 tools/libacpi/dsdt_q35.asl

(quoted diff) +0 -0

b/tools/libacpi/dsdt_q35.asl new file mode 100644
index 0000000000..cd02946a07

tools/libacpi/dsdt_q35.asl new file +6 -0

@@ -0,0 +1,551 @@
+/******************************************************************************
+ * DSDT for Xen with Qemu device model (for Q35 machine)
+ *
+ * Copyright (c) 2004, Intel Corporation.
+ *
+ * This program is free software; you can redistribute it and/or

Quoted 6 lines

modify
+ * it under the terms of the GNU Lesser General Public License as published
+ * by the Free Software Foundation; version 2.1 only. with the special
+ * exception on linking described in file LICENSE.

I don't see the 'LICENSE' file in Xen's directory?

Quoted 8 lines

Quoted 2 lines

Also, your email does not seem to be coming from Intel, so I have to ask, where did this file originally come from?

It's basically Xen's dsdt.asl with some modifications related to Q35. Currently only few modifications needed, but in the future dsdt.asl and dsdt_q35.asl will diverge more from each other -- that's the reason why a separate file was forked instead applying these changes to dsdt.asl directly, for example, as #ifdef-parts.

OK, as such you should make a seperate patch that adds this file (and be completly unmodified) and make sure you CC Intel folks (Kevin, et all) so they can Ack it.

From: Alexey G [mailto:[email protected]]
Sent: Tuesday, March 13, 2018 5:20 AM

Quoted 19 lines · On Mon, 12 Mar 2018 16:32:27 -0400 Konrad Rzeszutek Wilk…

Quoted 18 lines · Alexey G

Quoted 15 lines · On Mon, 12 Mar 2018 15:38:03 -0400 Konrad Rzeszutek Wilk…

Quoted 13 lines · Alexey Gerasimenko

This patch adds the DSDT table for Q35 (new tools/libacpi/dsdt_q35.asl file). There are not many differences with dsdt.asl (for i440) at the moment, namely:

- BDF location of LPC Controller
- Minor changes related to FDC detection
- Addition of _OSC method to inform OSPM about PCIe features supported

As we are still using 4 PCI router links and their corresponding device/register addresses are same (offset 0x60), no need to change PCI routing descriptions.

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Also, ACPI hotplug is still used to control passed through device hot (un)plug (as it was for i440).

Signed-off-by Alexey Gerasimenko <[email protected]>

---
 tools/libacpi/dsdt_q35.asl | 551

+++++++++++++++++++++++++++++++++++++++++++++ 1 file changed,

551

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

tools/libacpi/dsdt_q35.asl

Quoted 9 lines

insertions(+) create mode 100644 tools/libacpi/dsdt_q35.asl

(quoted diff) +0 -0

b/tools/libacpi/dsdt_q35.asl new file mode 100644
index 0000000000..cd02946a07

tools/libacpi/dsdt_q35.asl new file +0 -0

@@ -0,0 +1,551 @@

+/************************************************************
******************

Quoted 23 lines

Quoted 14 lines

Quoted 14 lines

Quoted 11 lines

tools/libacpi/dsdt_q35.asl new file +5 -0

+ * DSDT for Xen with Qemu device model (for Q35 machine)
+ *
+ * Copyright (c) 2004, Intel Corporation.
+ *
+ * This program is free software; you can redistribute it and/or

modify
+ * it under the terms of the GNU Lesser General Public License as published
+ * by the Free Software Foundation; version 2.1 only. with the special
+ * exception on linking described in file LICENSE.

I don't see the 'LICENSE' file in Xen's directory?

Quoted 8 lines

Quoted 2 lines

Also, your email does not seem to be coming from Intel, so I have to ask, where did this file originally come from?

It's basically Xen's dsdt.asl with some modifications related to Q35. Currently only few modifications needed, but in the future dsdt.asl and dsdt_q35.asl will diverge more from each other -- that's the reason why a separate file was forked instead applying these changes to dsdt.asl directly, for example, as #ifdef-parts.

Quoted 3 lines

OK, as such you should make a seperate patch that adds this file (and be completly unmodified) and make sure you CC Intel folks (Kevin, et all) so they can Ack it.

Kevin -- I assume you mean Kevin Tian <[email protected]>? Cc'ing him.
Please let me know other persons from Intel who are also responsible, the MAINTAINERS file doesn't tell much about Intel people regarding /libacpi, unfortunately.

This patch adds the DSDT table for Q35 (new tools/libacpi/dsdt_q35.asl file). There are not many differences with dsdt.asl (for i440) at the moment, namely:

- BDF location of LPC Controller
- Minor changes related to FDC detection
- Addition of _OSC method to inform OSPM about PCIe features supported

As we are still using 4 PCI router links and their corresponding device/register addresses are same (offset 0x60), no need to change PCI routing descriptions.

Also, ACPI hotplug is still used to control passed through device hot (un)plug (as it was for i440).

Signed-off-by Alexey Gerasimenko <[email protected]>

---
 tools/libacpi/dsdt_q35.asl | 551 +++++++++++++++++++++++++++++++++++++++++++++

Quoted 13 lines · Alexey Gerasimenko

This patch adds the DSDT table for Q35 (new tools/libacpi/dsdt_q35.asl file). There are not many differences with dsdt.asl (for i440) at the moment, namely:

- BDF location of LPC Controller
- Minor changes related to FDC detection
- Addition of _OSC method to inform OSPM about PCIe features supported

As we are still using 4 PCI router links and their corresponding device/register addresses are same (offset 0x60), no need to change PCI routing descriptions.

Quoted 7 lines

Also, ACPI hotplug is still used to control passed through device hot (un)plug (as it was for i440).

Signed-off-by Alexey Gerasimenko <[email protected]>

---
 tools/libacpi/dsdt_q35.asl | 551

+++++++++++++++++++++++++++++++++++++++++++++

So this is basically a modified dupe of the current dsdt.asl? AFAICT there are a bunch of common bits, which ideally we want to have defined in a single place.

Can't you factor out the common parts of the dsdt.asl into smaller parts an include them for both dsdt.asl and dsdt_q35.asl?

I would first have a patch that extract the common parts of the dsdt into file(s), and then a second patch which creates a dsdt_q35.asl based on those common bits plus the specific q35 code.

Provide building for newly added dsdt_q35.asl file, in a way similar to dsdt.asl.

Note that '15cpu' ACPI tables are only applicable to qemu-traditional (which have no support for Q35), so we need to use 'anycpu' version only.

Quoted 3 lines · Alexey Gerasimenko

Provide building for newly added dsdt_q35.asl file, in a way similar to dsdt.asl.

Quoted 3 lines

Note that '15cpu' ACPI tables are only applicable to qemu-traditional (which have no support for Q35), so we need to use 'anycpu' version only.

You should do this in the same patch that adds dsdt_q35.asl, at the end without this the previous patch just adds dead code.

Thanks, Roger.

Quoted 6 lines

Quoted 1 line

Quoted 1 line · On 12.03.18 at 19:33, <[email protected]> wrote:

tools/firmware/hvmloader/Makefile +0 -0

@@ -75,7 +75,7 @@ rombios.o: roms.inc
 smbios.o: CFLAGS += -D__SMBIOS_DATE__="\"$(SMBIOS_REL_DATE)\""
 

Quoted 4 lines

tools/firmware/hvmloader/Makefile +1 -1

 ACPI_PATH = ../../libacpi
-DSDT_FILES = dsdt_anycpu.c dsdt_15cpu.c dsdt_anycpu_qemu_xen.c
+DSDT_FILES = dsdt_anycpu.c dsdt_15cpu.c dsdt_anycpu_qemu_xen.c

dsdt_q35_anycpu_qemu_xen.c

Unless you intend to add a second flavor, please omit the "anycpu" part from the name of the new instance.

Quoted 4 lines

tools/firmware/hvmloader/Makefile +0 -1

@@ -56,6 +56,13 @@ $(ACPI_BUILD_DIR)/dsdt_anycpu_qemu_xen.asl:
 dsdt.asl dsdt_acpi_info.asl $(MK_DSD $(MK_DSDT) --debug=$(debug)
--dm-version qemu-xen >> $@.$(TMP_SUFFIX) mv -f $@.$(TMP_SUFFIX) $@
 

tools/firmware/hvmloader/Makefile $(ACPI_BUILD_DIR)/dsdt_anycpu_qemu_xen.asl: · line 65

Quoted 8 lines

tools/firmware/hvmloader/Makefile +6 -0

+$(ACPI_BUILD_DIR)/dsdt_q35_anycpu_qemu_xen.asl: dsdt_q35.asl
 dsdt_acpi_info.asl $(MK_DSDT)
+	# Remove last bracket
+	awk 'NR > 1 {print s} {s=$$0}' $< > $@.$(TMP_SUFFIX)
+	cat dsdt_acpi_info.asl >> $@.$(TMP_SUFFIX)
+	$(MK_DSDT) --debug=$(debug) --dm-version qemu-xen >>
 $@.$(TMP_SUFFIX)
+	mv -f $@.$(TMP_SUFFIX) $@  

The commands look to be exactly the same as those for dsdt_anycpu_qemu_xen.asl - please let's not duplicate such things, but instead use a pattern rule.

This adds a new function get_pc_machine_type() which allows to determine the emulated chipset type. Supported return values:

- MACHINE_TYPE_I440
- MACHINE_TYPE_Q35
- MACHINE_TYPE_UNKNOWN, results in the error message being printed followed by calling BUG() in hvmloader.

Signed-off-by Alexey Gerasimenko <[email protected]>

---
 tools/firmware/hvmloader/pci_regs.h |  5 ++++
 tools/firmware/hvmloader/util.c     | 47 +++++++++++++++++++++++++++++++++++++
 tools/firmware/hvmloader/util.h     |  8 +++++++
 3 files changed, 60 insertions(+)

tools/firmware/hvmloader/pci_regs.h +0 -0

index 7bf2d873ab..ba498b840e 100644

tools/firmware/hvmloader/pci_regs.h +0 -0

 #endif /* __HVMLOADER_PCI_REGS_H__ */
 
 /*

tools/firmware/hvmloader/util.c +9 -0

index 0c3f2d24cd..5739a87628 100644
@@ -22,6 +22,7 @@
 #include "hypercall.h"
 #include "ctype.h"
 #include "vnuma.h"
+#include "pci_regs.h"
 #include <acpi2_0.h>
 #include <libacpi.h>
 #include <stdint.h>

@@ -735,6 +736,52 @@ void __bug(char *file, int line)
     crash();
 }
 
+
+static int machine_type = MACHINE_TYPE_UNDEFINED;
+
+int get_pc_machine_type(void)
+{
+    uint16_t vendor_id;
+    uint16_t device_id;
+

Quoted 24 lines · Alexey Gerasimenko

This adds a new function get_pc_machine_type() which allows to determine the emulated chipset type. Supported return values:

- MACHINE_TYPE_I440
- MACHINE_TYPE_Q35
- MACHINE_TYPE_UNKNOWN, results in the error message being printed followed by calling BUG() in hvmloader.

Signed-off-by Alexey Gerasimenko <[email protected]>

---
 tools/firmware/hvmloader/pci_regs.h |  5 ++++
 tools/firmware/hvmloader/util.c     | 47

+++++++++++++++++++++++++++++++++++++
tools/firmware/hvmloader/util.h | 8 +++++++ 3 files changed, 60
insertions(+)

(quoted diff) +0 -0

b/tools/firmware/hvmloader/pci_regs.h index 7bf2d873ab..ba498b840e 100644 --- a/tools/firmware/hvmloader/pci_regs.h
+++ b/tools/firmware/hvmloader/pci_regs.h

tools/firmware/hvmloader/pci_regs.h +0 -0

@@ -107,6 +107,11 @@
 
 #define PCI_INTEL_OPREGION 0xfc /* 4 bits */
 

tools/firmware/hvmloader/pci_regs.h line 114

Quoted 5 lines

tools/firmware/hvmloader/pci_regs.h +5 -0

+#define PCI_VENDOR_ID_INTEL              0x8086
+#define PCI_DEVICE_ID_INTEL_82441        0x1237
+#define PCI_DEVICE_ID_INTEL_Q35_MCH      0x29c0
+
+  

Too many blank lines.

tools/firmware/hvmloader/util.c void __bug(char *file, int line) · line 740

Quoted 6 lines

tools/firmware/hvmloader/util.c +2 -0

@@ -735,6 +736,52 @@ void __bug(char *file, int line)
     crash();
 }
 
+    /* only Intel platforms are emulated currently */
+    if (vendor_id == PCI_VENDOR_ID_INTEL)  

Coding style.

Ditto.

And this patch should be folded into its user, unless the patch that uses it is very big on its own.

Quoted 25 lines · On Tue, 13 Mar 2018 17:26:04 +0000 Wei Liu <[email protected]> wrote:

Quoted 24 lines · Alexey Gerasimenko

This adds a new function get_pc_machine_type() which allows to determine the emulated chipset type. Supported return values:

- MACHINE_TYPE_I440
- MACHINE_TYPE_Q35
- MACHINE_TYPE_UNKNOWN, results in the error message being printed followed by calling BUG() in hvmloader.

Signed-off-by Alexey Gerasimenko <[email protected]>

---
 tools/firmware/hvmloader/pci_regs.h |  5 ++++
 tools/firmware/hvmloader/util.c     | 47

+++++++++++++++++++++++++++++++++++++
tools/firmware/hvmloader/util.h | 8 +++++++ 3 files changed, 60
insertions(+)

(quoted diff) +0 -0

b/tools/firmware/hvmloader/pci_regs.h index 7bf2d873ab..ba498b840e 100644 --- a/tools/firmware/hvmloader/pci_regs.h
+++ b/tools/firmware/hvmloader/pci_regs.h

tools/firmware/hvmloader/pci_regs.h +0 -0

@@ -107,6 +107,11 @@
 
 #define PCI_INTEL_OPREGION 0xfc /* 4 bits */
 

Quoted 7 lines

tools/firmware/hvmloader/pci_regs.h line 114

Quoted 5 lines

tools/firmware/hvmloader/pci_regs.h +5 -0

+#define PCI_VENDOR_ID_INTEL              0x8086
+#define PCI_DEVICE_ID_INTEL_82441        0x1237
+#define PCI_DEVICE_ID_INTEL_Q35_MCH      0x29c0
+
+  

Too many blank lines.

Will fix.

Quoted 10 lines

tools/firmware/hvmloader/util.c void __bug(char *file, int line) · line 740

Quoted 6 lines

tools/firmware/hvmloader/util.c +2 -0

@@ -735,6 +736,52 @@ void __bug(char *file, int line)
     crash();
 }
 
+    /* only Intel platforms are emulated currently */
+    if (vendor_id == PCI_VENDOR_ID_INTEL)  

Coding style.

Ditto.

Will fix.

Quoted 2 lines

And this patch should be folded into its user, unless the patch that uses it is very big on its own.

Hmm, looks like I overfollowed the recommendation about making atomic patches for easier review. There are multiple users of these function, it was made in a separate patch just because of this. In the next version I'll merge it with some of the patches which use this function then.

This adds a new function get_pc_machine_type() which allows to determine the emulated chipset type. Supported return values:

- MACHINE_TYPE_I440
- MACHINE_TYPE_Q35
- MACHINE_TYPE_UNKNOWN, results in the error message being printed followed by calling BUG() in hvmloader.

Signed-off-by Alexey Gerasimenko <[email protected]>

---
 tools/firmware/hvmloader/pci_regs.h |  5 ++++
 tools/firmware/hvmloader/util.c     | 47 +++++++++++++++++++++++++++++++++++++
 tools/firmware/hvmloader/util.h     |  8 +++++++
 3 files changed, 60 insertions(+)

tools/firmware/hvmloader/pci_regs.h +5 -0

index 7bf2d873ab..ba498b840e 100644
@@ -107,6 +107,11 @@
 
 #define PCI_INTEL_OPREGION 0xfc /* 4 bits */
 
+#define PCI_VENDOR_ID_INTEL              0x8086
+#define PCI_DEVICE_ID_INTEL_82441        0x1237
+#define PCI_DEVICE_ID_INTEL_Q35_MCH      0x29c0
+
+
 #endif /* __HVMLOADER_PCI_REGS_H__ */
 
 /*

tools/firmware/hvmloader/util.c +1 -0

index 0c3f2d24cd..5739a87628 100644
@@ -22,6 +22,7 @@
 #include "hypercall.h"
 #include "ctype.h"
 #include "vnuma.h"
+#include "pci_regs.h"
 #include <acpi2_0.h>
 #include <libacpi.h>
 #include <stdint.h>

tools/firmware/hvmloader/util.c +4 -0

+{
+    uint16_t vendor_id;
+    uint16_t device_id;
+

tools/firmware/hvmloader/util.c +13 -0

+    {
+        switch (device_id)
+        {
+        case PCI_DEVICE_ID_INTEL_82441:
+            machine_type = MACHINE_TYPE_I440;
+            printf("Detected i440 chipset\n");
+            break;
+
+        case PCI_DEVICE_ID_INTEL_Q35_MCH:
+            machine_type = MACHINE_TYPE_Q35;
+            printf("Detected Q35 chipset\n");
+            break;
+

tools/firmware/hvmloader/util.c +5 -0

+    }
+
+    return machine_type;
+}
+
 static void validate_hvm_info(struct hvm_info_table *t)
 {
     uint8_t *ptr = (uint8_t *)t;

tools/firmware/hvmloader/util.h +0 -0

index 7bca6418d2..7c77bedb00 100644

Quoted 7 lines · Alexey Gerasimenko

This adds a new function get_pc_machine_type() which allows to determine the emulated chipset type. Supported return values:

- MACHINE_TYPE_I440
- MACHINE_TYPE_Q35
- MACHINE_TYPE_UNKNOWN, results in the error message being printed followed by calling BUG() in hvmloader.

This is not correct, the return values are strictly MACHINE_TYPE_I440 or MACHINE_TYPE_Q35. Everything else ends up in a BUG().

Also makes me wonder whether this should instead be init_machine_type, and users should just read machine_type directly.

Quoted 10 lines

 tools/firmware/hvmloader/pci_regs.h |  5 ++++
 tools/firmware/hvmloader/util.c     | 47

+++++++++++++++++++++++++++++++++++++
tools/firmware/hvmloader/util.h | 8 +++++++ 3 files changed, 60
insertions(+)

(quoted diff) +0 -0

b/tools/firmware/hvmloader/pci_regs.h index 7bf2d873ab..ba498b840e 100644 --- a/tools/firmware/hvmloader/pci_regs.h
+++ b/tools/firmware/hvmloader/pci_regs.h

Quoted 1 line

+static int machine_type = MACHINE_TYPE_UNDEFINED;

There's no need to init this, _UNDEFINED is 0 which is the default value.

Quoted 1 line

+int get_pc_machine_type(void)

You introduce a function that's not used anywhere, and the commit log doesn't mention why this is needed at all. In general I prefer functions to be introduced with at least a caller, or else it needs to be described in the commit message why this is not the case.

Quoted 9 lines

tools/firmware/hvmloader/util.c +9 -0

+{
+    uint16_t vendor_id;
+    uint16_t device_id;
+
+    if (machine_type != MACHINE_TYPE_UNDEFINED)
+        return machine_type;
+
+    machine_type = MACHINE_TYPE_UNKNOWN;
+

tools/firmware/hvmloader/util.c void __bug(char *file, int line) · line 756

Quoted 5 lines

tools/firmware/hvmloader/util.c +5 -0

+    vendor_id = pci_readw(0, PCI_VENDOR_ID);
+    device_id = pci_readw(0, PCI_DEVICE_ID);
+
+    /* only Intel platforms are emulated currently */
+    if (vendor_id == PCI_VENDOR_ID_INTEL)  

Should this maybe be a BUG_ON(vendor_id != PCI_VENDOR_ID_INTEL) then? Note that in this case you end up with a BUG later anyway.

Quoted 19 lines

tools/firmware/hvmloader/util.c +19 -0

+    {
+        switch (device_id)
+        {
+        case PCI_DEVICE_ID_INTEL_82441:
+            machine_type = MACHINE_TYPE_I440;
+            printf("Detected i440 chipset\n");
+            break;
+
+        case PCI_DEVICE_ID_INTEL_Q35_MCH:
+            machine_type = MACHINE_TYPE_Q35;
+            printf("Detected Q35 chipset\n");
+            break;
+
+        default:
+            break;
+        }
+    }
+
+    if (machine_type == MACHINE_TYPE_UNKNOWN)

tools/firmware/hvmloader/util.c void __bug(char *file, int line) · line 779

Quoted 5 lines

(quoted diff) +2 -0

+    {
+        printf("Unknown emulated chipset encountered, VID=%04Xh,

DID=%04Xh\n",

tools/firmware/hvmloader/util.c +2 -0

+               vendor_id, device_id);
+        BUG();  

Why not place this in the default switch label? That would allow you to get rid of the MACHINE_TYPE_UNKNOWN define also.

Quoted 15 lines

tools/firmware/hvmloader/util.c +5 -0

+    }
+
+    return machine_type;
+}
+
 static void validate_hvm_info(struct hvm_info_table *t)
 {
     uint8_t *ptr = (uint8_t *)t;

(quoted diff) +0 -0

b/tools/firmware/hvmloader/util.h index 7bca6418d2..7c77bedb00 100644

tools/firmware/hvmloader/util.h +0 -0

@@ -100,6 +100,14 @@ void pci_write(uint32_t devfn, uint32_t reg,
 uint32_t len, uint32_t val); #define pci_writew(devfn, reg, val)
 pci_write(devfn, reg, 2, (uint16_t)(val)) #define pci_writel(devfn,

tools/firmware/hvmloader/util.h void pci_write(uint32_t devfn, uint32_t reg, uint32_t len, uint32_t val); · line 107

Quoted 6 lines

tools/firmware/hvmloader/util.h +5 -0

 reg, val) pci_write(devfn, reg, 4, (uint32_t)(val)) 
+/* Emulated machine types */
+#define MACHINE_TYPE_UNDEFINED      0
+#define MACHINE_TYPE_I440           1
+#define MACHINE_TYPE_Q35            2
+#define MACHINE_TYPE_UNKNOWN        (-1)  

An enum seems better suited for this.

In order to turn on ACPI for OS, we need to write a chipset-specific value to SMI_CMD register (sort of imitation of the APM->ACPI switch on real systems). Modify acpi_enable_sci() function to support both i440 and Q35 emulation.

Signed-off-by Alexey Gerasimenko <[email protected]>

---
 tools/firmware/hvmloader/hvmloader.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

tools/firmware/hvmloader/hvmloader.c +0 -0

index f603f68ded..070698440e 100644
@@ -257,9 +257,16 @@ static const struct bios_config *detect_bios(void)
 static void acpi_enable_sci(void)

In order to turn on ACPI for OS, we need to write a chipset-specific value to SMI_CMD register (sort of imitation of the APM->ACPI switch on real systems). Modify acpi_enable_sci() function to support both i440 and Q35 emulation.

Signed-off-by Alexey Gerasimenko <[email protected]>

---
 tools/firmware/hvmloader/hvmloader.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

tools/firmware/hvmloader/hvmloader.c +1 -0

index f603f68ded..070698440e 100644
@@ -257,9 +257,16 @@ static const struct bios_config *detect_bios(void)
 static void acpi_enable_sci(void)
 {
     uint8_t pm1a_cnt_val;
+    uint8_t acpi_enable_val;

Quoted 19 lines · Alexey Gerasimenko

In order to turn on ACPI for OS, we need to write a chipset-specific value to SMI_CMD register (sort of imitation of the APM->ACPI switch on real systems). Modify acpi_enable_sci() function to support both i440 and Q35 emulation.

Signed-off-by Alexey Gerasimenko <[email protected]>

---
 tools/firmware/hvmloader/hvmloader.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

(quoted diff) +0 -0

b/tools/firmware/hvmloader/hvmloader.c index f603f68ded..070698440e 100644 --- a/tools/firmware/hvmloader/hvmloader.c
+++ b/tools/firmware/hvmloader/hvmloader.c

tools/firmware/hvmloader/hvmloader.c +1 -0

@@ -257,9 +257,16 @@ static const struct bios_config
 *detect_bios(void) static void acpi_enable_sci(void)
 {
     uint8_t pm1a_cnt_val;
+    uint8_t acpi_enable_val;

tools/firmware/hvmloader/hvmloader.c static const struct bios_config · line 269

Quoted 10 lines

tools/firmware/hvmloader/hvmloader.c +7 -1

 
-#define PIIX4_SMI_CMD_IOPORT 0xb2
+#define SMI_CMD_IOPORT       0xb2
 #define PIIX4_ACPI_ENABLE    0xf1
+#define ICH9_ACPI_ENABLE     0x02
+
+    if (get_pc_machine_type() == MACHINE_TYPE_Q35)
+        acpi_enable_val = ICH9_ACPI_ENABLE;
+    else
+        acpi_enable_val = PIIX4_ACPI_ENABLE;  

Coding style, but I would rather:

switch ( get_pc_machine_type() )
{
case MACHINE_TYPE_Q35:
...
case MACHINE_TYPE_I440:
...
default:
BUG();
}

I think storing the machine type in a global variable is better than calling get_pc_machine_type each time.

Allows to select Q35 DSDT table in hvmloader_acpi_build_tables(). Function get_pc_machine_type() is used to select a proper table (i440/q35).

As we are bound to the qemu-xen device model for Q35, no need to initialize config->dsdt_15cpu/config->dsdt_15cpu_len fields.

Signed-off-by Alexey Gerasimenko <[email protected]>

---
 tools/firmware/hvmloader/util.c | 13 +++++++++++--
 tools/firmware/hvmloader/util.h |  2 ++
 2 files changed, 13 insertions(+), 2 deletions(-)

tools/firmware/hvmloader/util.c +0 -0

index 5739a87628..d8db9e3c8e 100644

tools/firmware/hvmloader/util.c +10 -0

+        {
+            config->dsdt_anycpu = dsdt_q35_anycpu_qemu_xen;
+            config->dsdt_anycpu_len = dsdt_q35_anycpu_qemu_xen_len;
+        }
+        else
+        {
+            config->dsdt_anycpu = dsdt_anycpu_qemu_xen;
+            config->dsdt_anycpu_len = dsdt_anycpu_qemu_xen_len;
+        }
+
         config->dsdt_15cpu = NULL;
         config->dsdt_15cpu_len = 0;
     }

tools/firmware/hvmloader/util.h +0 -0

index 7c77bedb00..fd2d885c96 100644

Quoted 13 lines · Alexey Gerasimenko

Allows to select Q35 DSDT table in hvmloader_acpi_build_tables(). Function get_pc_machine_type() is used to select a proper table (i440/q35).

As we are bound to the qemu-xen device model for Q35, no need to initialize config->dsdt_15cpu/config->dsdt_15cpu_len fields.

Signed-off-by Alexey Gerasimenko <[email protected]>

---
 tools/firmware/hvmloader/util.c | 13 +++++++++++--
 tools/firmware/hvmloader/util.h |  2 ++
 2 files changed, 13 insertions(+), 2 deletions(-)

tools/firmware/hvmloader/util.c void hvmloader_acpi_build_tables(struct · line 958

Quoted 11 lines

(quoted diff) +0 -0

b/tools/firmware/hvmloader/util.c index 5739a87628..d8db9e3c8e 100644

tools/firmware/hvmloader/util.c +1 -2

@@ -955,8 +955,17 @@ void hvmloader_acpi_build_tables(struct
 acpi_config *config, }
     else if ( !strncmp(s, "qemu_xen", 9) )
     {
-        config->dsdt_anycpu = dsdt_anycpu_qemu_xen;
-        config->dsdt_anycpu_len = dsdt_anycpu_qemu_xen_len;
+        if (get_pc_machine_type() == MACHINE_TYPE_Q35)  

Coding style (missing spaces between parentheses), and I would prefer a switch here.

IMO you should add a BUG_ON(Q35) in the qemu_xen_traditional condition above this one..

Quoted 20 lines

tools/firmware/hvmloader/util.c +10 -0

+        {
+            config->dsdt_anycpu = dsdt_q35_anycpu_qemu_xen;
+            config->dsdt_anycpu_len = dsdt_q35_anycpu_qemu_xen_len;
+        }
+        else
+        {
+            config->dsdt_anycpu = dsdt_anycpu_qemu_xen;
+            config->dsdt_anycpu_len = dsdt_anycpu_qemu_xen_len;
+        }
+
         config->dsdt_15cpu = NULL;
         config->dsdt_15cpu_len = 0;
     }

(quoted diff) +0 -0

b/tools/firmware/hvmloader/util.h index 7c77bedb00..fd2d885c96 100644

tools/firmware/hvmloader/util.h +0 -0

@@ -288,7 +288,9 @@ bool check_overlap(uint64_t start, uint64_t size,
                    uint64_t reserved_start, uint64_t reserved_size);
 

tools/firmware/hvmloader/util.h bool check_overlap(uint64_t start, uint64_t size, · line 293

Quoted 4 lines

tools/firmware/hvmloader/util.h +0 -0

 extern const unsigned char dsdt_anycpu_qemu_xen[], dsdt_anycpu[],
 dsdt_15cpu[]; +extern const unsigned char dsdt_q35_anycpu_qemu_xen[];
 extern const int dsdt_anycpu_qemu_xen_len, dsdt_anycpu_len,
 dsdt_15cpu_len; +extern const int dsdt_q35_anycpu_qemu_xen_len;  

Since you are adding this, maybe unsigned int? (or size_t?)

This patch does following:

1. Move PCI-device specific initialization out of pci_setup function to the newly created class_specific_pci_device_setup function to simplify code.

2. PCI-device specific initialization extended with LPC controller initialization

3. Initialize PIRQA...{PIRQD, PIRQH} routing accordingly to the emulated south bridge (either located on PCI_ISA_DEVFN or PCI_ICH9_LPC_DEVFN).

Signed-off-by Alexey Gerasimenko <[email protected]>

---
 tools/firmware/hvmloader/config.h |   1 +
 tools/firmware/hvmloader/pci.c    | 162 ++++++++++++++++++++++++--------------
 2 files changed, 104 insertions(+), 59 deletions(-)

tools/firmware/hvmloader/config.h +1 -0

index 6e00413f2e..6fde6b7b60 100644
@@ -52,6 +52,7 @@ extern uint8_t ioapic_version;
 
 #define PCI_ISA_DEVFN       0x08    /* dev 1, fn 0 */
 #define PCI_ISA_IRQ_MASK    0x0c20U /* ISA IRQs 5,10,11 are PCI connected */
+#define PCI_ICH9_LPC_DEVFN  0xf8    /* dev 31, fn 0 */
 
 /* MMIO hole: Hardcoded defaults, which can be dynamically expanded. */
 #define PCI_MEM_END         0xfc000000

tools/firmware/hvmloader/pci.c +0 -0

index 0b708bf578..033bd20992 100644

tools/firmware/hvmloader/pci.c +64 -0

+        /* If emulated VGA is found, preserve it as primary VGA. */
+        if ( (vendor_id == 0x1234) && (device_id == 0x1111) )
+        {
+            vga_devfn = devfn;
+            virtual_vga = VGA_std;
+        }
+        else if ( (vendor_id == 0x1013) && (device_id == 0xb8) )
+        {
+            vga_devfn = devfn;
+            virtual_vga = VGA_cirrus;
+        }
+        else if ( virtual_vga == VGA_none )
+        {
+            vga_devfn = devfn;
+            virtual_vga = VGA_pt;
+            if ( vendor_id == 0x8086 )
+            {
+                igd_opregion_pgbase = mem_hole_alloc(IGD_OPREGION_PAGES);
+                /*
+                 * Write the the OpRegion offset to give the opregion
+                 * address to the device model. The device model will trap
+                 * and map the OpRegion at the give address.
+                 */
+                pci_writel(vga_devfn, PCI_INTEL_OPREGION,
+                           igd_opregion_pgbase << PAGE_SHIFT);
+            }
+        }
+        break;
+
+    case 0x0680:
+        /* PIIX4 ACPI PM. Special device with special PCI config space. */
+        ASSERT((vendor_id == 0x8086) && (device_id == 0x7113));
+        pci_writew(devfn, 0x20, 0x0000); /* No smb bus IO enable */
+        pci_writew(devfn, 0xd2, 0x0000); /* No smb bus IO enable */
+        pci_writew(devfn, 0x22, 0x0000);
+        pci_writew(devfn, 0x3c, 0x0009); /* Hardcoded IRQ9 */
+        pci_writew(devfn, 0x3d, 0x0001);
+        pci_writel(devfn, 0x40, ACPI_PM1A_EVT_BLK_ADDRESS_V1 | 1);
+        pci_writeb(devfn, 0x80, 0x01); /* enable PM io space */
+        break;
+
+    case 0x0601:
+        /* LPC bridge */
+        if (vendor_id == 0x8086 && device_id == 0x2918)
+        {
+            pci_writeb(devfn, 0x3c, 0x09); /* Hardcoded IRQ9 */
+            pci_writeb(devfn, 0x3d, 0x01);
+            pci_writel(devfn, 0x40, ACPI_PM1A_EVT_BLK_ADDRESS_V1 | 1);
+            pci_writeb(devfn, 0x44, 0x80); /* enable PM io space */
+            outl(SCI_EN_IOPORT, inl(SCI_EN_IOPORT) | GBL_SMI_EN | APMC_EN);
+        }
+        break;
+
+    case 0x0101:
+        if ( vendor_id == 0x8086 )
+        {
+            /* Intel ICHs since PIIX3: enable IDE legacy mode. */
+            pci_writew(devfn, 0x40, 0x8000); /* enable IDE0 */
+            pci_writew(devfn, 0x42, 0x8000); /* enable IDE1 */
+        }
+        break;
+    }
+}
+

tools/firmware/hvmloader/pci.c +2 -0

     /* Resources assignable to PCI devices via BARs. */
     struct resource {

@@ -130,13 +210,28 @@ void pci_setup(void)
     if ( s )
         mmio_hole_size = strtoll(s, NULL, 0);
 
+    /* check if we are on Q35 and set the flag if it is the case */
+    is_running_on_q35 = get_pc_machine_type() == MACHINE_TYPE_Q35;

Quoted 39 lines · Alexey Gerasimenko

This patch does following:

1. Move PCI-device specific initialization out of pci_setup function to the newly created class_specific_pci_device_setup function to simplify code.

2. PCI-device specific initialization extended with LPC controller initialization

3. Initialize PIRQA...{PIRQD, PIRQH} routing accordingly to the emulated south bridge (either located on PCI_ISA_DEVFN or PCI_ICH9_LPC_DEVFN).

Signed-off-by Alexey Gerasimenko <[email protected]>

---
 tools/firmware/hvmloader/config.h |   1 +
 tools/firmware/hvmloader/pci.c    | 162

++++++++++++++++++++++++-------------- 2 files changed, 104 insertions(+), 59 deletions(-)

(quoted diff) +0 -0

b/tools/firmware/hvmloader/config.h index 6e00413f2e..6fde6b7b60 100644 --- a/tools/firmware/hvmloader/config.h
+++ b/tools/firmware/hvmloader/config.h

tools/firmware/hvmloader/config.h +0 -0

@@ -52,6 +52,7 @@ extern uint8_t ioapic_version;
 
 #define PCI_ISA_DEVFN       0x08    /* dev 1, fn 0 */
 #define PCI_ISA_IRQ_MASK    0x0c20U /* ISA IRQs 5,10,11 are PCI

connected */ +#define PCI_ICH9_LPC_DEVFN 0xf8 /* dev 31, fn 0 */

/* MMIO hole: Hardcoded defaults, which can be dynamically
expanded. */ #define PCI_MEM_END 0xfc000000

(quoted diff) +0 -0

b/tools/firmware/hvmloader/pci.c index 0b708bf578..033bd20992 100644

tools/firmware/hvmloader/pci.c +0 -0

@@ -35,6 +35,7 @@ unsigned long pci_mem_end = PCI_MEM_END;
 uint64_t pci_hi_mem_start = 0, pci_hi_mem_end = 0;
 

tools/firmware/hvmloader/pci.c unsigned long pci_mem_end = PCI_MEM_END; · line 38

Quoted 2 lines

tools/firmware/hvmloader/pci.c +1 -0

 enum virtual_vga virtual_vga = VGA_none;
+uint32_t vga_devfn = 256;  

uint8_t should be enough to store a devfn. Also this should be static maybe?

Quoted 7 lines

tools/firmware/hvmloader/pci.c +0 -0

 unsigned long igd_opregion_pgbase = 0;
 
 /* Check if the specified range conflicts with any reserved device

memory. */ @@ -76,14 +77,93 @@ static int find_next_rmrr(uint32_t base) return next_rmrr;
}

tools/firmware/hvmloader/pci.c static int find_next_rmrr(uint32_t base) · line 82

Quoted 3 lines

tools/firmware/hvmloader/pci.c +3 -0

+#define SCI_EN_IOPORT  (ACPI_PM1A_EVT_BLK_ADDRESS_V1 + 0x30)
+#define GBL_SMI_EN      (1 << 0)
+#define APMC_EN         (1 << 5)  

Alignment.

tools/firmware/hvmloader/pci.c static int find_next_rmrr(uint32_t base) · line 92

Quoted 10 lines

tools/firmware/hvmloader/pci.c +4 -0

+
+static void class_specific_pci_device_setup(uint16_t vendor_id,
+                                            uint16_t device_id,
+                                            uint8_t bus, uint8_t

devfn) +{

tools/firmware/hvmloader/pci.c +5 -0

+    uint16_t class;
+
+    class = pci_readw(devfn, PCI_CLASS_DEVICE);
+
+    switch ( class )  

switch ( pci_readw(devfn, PCI_CLASS_DEVICE) ) ?

I don't see class being used elsewhere.

Also why is vendor_id/device_id provided by the caller but not class? It seems kind of pointless.

Why not fetch vendor/device from the function itself and move the (vendor_id == 0xffff) && (device_id == 0xffff) check inside the function?

Also in this case I think it would be better to have a non-functional patch that introduces class_specific_pci_device_setup and a second patch that adds support for ICH9.

Having code movement and new code in the same patch makes it harder to very what you are actually moving vs introducing.

tools/firmware/hvmloader/pci.c

Quoted 2 lines

tools/firmware/hvmloader/pci.c +2 -0

+    {
+    case 0x0300:  

All this values need to be defines documented somewhere.

Quoted 74 lines

tools/firmware/hvmloader/pci.c +18 -0

+        /* If emulated VGA is found, preserve it as primary VGA. */
+        if ( (vendor_id == 0x1234) && (device_id == 0x1111) )
+        {
+            vga_devfn = devfn;
+            virtual_vga = VGA_std;
+        }
+        else if ( (vendor_id == 0x1013) && (device_id == 0xb8) )
+        {
+            vga_devfn = devfn;
+            virtual_vga = VGA_cirrus;
+        }
+        else if ( virtual_vga == VGA_none )
+        {
+            vga_devfn = devfn;
+            virtual_vga = VGA_pt;
+            if ( vendor_id == 0x8086 )
+            {
+                igd_opregion_pgbase =

mem_hole_alloc(IGD_OPREGION_PAGES);

tools/firmware/hvmloader/pci.c +2 -0

+                /*
+                 * Write the the OpRegion offset to give the

opregion
+ * address to the device model. The device model
will trap

tools/firmware/hvmloader/pci.c +10 -0

+                 * and map the OpRegion at the give address.
+                 */
+                pci_writel(vga_devfn, PCI_INTEL_OPREGION,
+                           igd_opregion_pgbase << PAGE_SHIFT);
+            }
+        }
+        break;
+
+    case 0x0680:
+        /* PIIX4 ACPI PM. Special device with special PCI config

space. */

tools/firmware/hvmloader/pci.c +17 -0

+        ASSERT((vendor_id == 0x8086) && (device_id == 0x7113));
+        pci_writew(devfn, 0x20, 0x0000); /* No smb bus IO enable */
+        pci_writew(devfn, 0xd2, 0x0000); /* No smb bus IO enable */
+        pci_writew(devfn, 0x22, 0x0000);
+        pci_writew(devfn, 0x3c, 0x0009); /* Hardcoded IRQ9 */
+        pci_writew(devfn, 0x3d, 0x0001);
+        pci_writel(devfn, 0x40, ACPI_PM1A_EVT_BLK_ADDRESS_V1 | 1);
+        pci_writeb(devfn, 0x80, 0x01); /* enable PM io space */
+        break;
+
+    case 0x0601:
+        /* LPC bridge */
+        if (vendor_id == 0x8086 && device_id == 0x2918)
+        {
+            pci_writeb(devfn, 0x3c, 0x09); /* Hardcoded IRQ9 */
+            pci_writeb(devfn, 0x3d, 0x01);
+            pci_writel(devfn, 0x40, ACPI_PM1A_EVT_BLK_ADDRESS_V1 |

1);

tools/firmware/hvmloader/pci.c +2 -0

+            pci_writeb(devfn, 0x44, 0x80); /* enable PM io space */
+            outl(SCI_EN_IOPORT, inl(SCI_EN_IOPORT) | GBL_SMI_EN |

APMC_EN);

tools/firmware/hvmloader/pci.c +14 -0

+        }
+        break;
+
+    case 0x0101:
+        if ( vendor_id == 0x8086 )
+        {
+            /* Intel ICHs since PIIX3: enable IDE legacy mode. */
+            pci_writew(devfn, 0x40, 0x8000); /* enable IDE0 */
+            pci_writew(devfn, 0x42, 0x8000); /* enable IDE1 */
+        }
+        break;
+    }
+}
+
 void pci_setup(void)
 {
     uint8_t is_64bar, using_64bar, bar64_relocate = 0;
     uint32_t devfn, bar_reg, cmd, bar_data, bar_data_upper;

tools/firmware/hvmloader/pci.c static int find_next_rmrr(uint32_t base) · line 166

Quoted 6 lines

     uint64_t base, bar_sz, bar_sz_upper, mmio_total = 0;

tools/firmware/hvmloader/pci.c +2 -2

-    uint32_t vga_devfn = 256;
-    uint16_t class, vendor_id, device_id;
+    uint16_t vendor_id, device_id;
     unsigned int bar, pin, link, isa_irq;
+    int is_running_on_q35 = 0;  

bool is_running_on_q35 = (get_pc_machine_type() == MACHINE_TYPE_Q35);

Quoted 7 lines

 
     /* Resources assignable to PCI devices via BARs. */
     struct resource {

tools/firmware/hvmloader/pci.c +0 -0

@@ -130,13 +210,28 @@ void pci_setup(void)
     if ( s )
         mmio_hole_size = strtoll(s, NULL, 0);
 

tools/firmware/hvmloader/pci.c void pci_setup(void) · line 223

Quoted 12 lines

tools/firmware/hvmloader/pci.c +5 -1

+    /* check if we are on Q35 and set the flag if it is the case */
+    is_running_on_q35 = get_pc_machine_type() == MACHINE_TYPE_Q35;
+
     /* Program PCI-ISA bridge with appropriate link routes. */
     isa_irq = 0;
     for ( link = 0; link < 4; link++ )
     {
         do { isa_irq = (isa_irq + 1) & 15;
         } while ( !(PCI_ISA_IRQ_MASK & (1U << isa_irq)) );
-        pci_writeb(PCI_ISA_DEVFN, 0x60 + link, isa_irq);
+
+        if (is_running_on_q35)  

Coding style.

tools/firmware/hvmloader/pci.c void pci_setup(void) · line 228

Quoted 5 lines

tools/firmware/hvmloader/pci.c +5 -0

+        {
+            pci_writeb(PCI_ICH9_LPC_DEVFN, 0x60 + link, isa_irq);
+
+            /* PIRQE..PIRQH are unused */
+            pci_writeb(PCI_ICH9_LPC_DEVFN, 0x68 + link, 0x80);  

According to the spec 0x80 is the default value for this registers, do you really need to write it?

Is maybe QEMU not correctly setting the default value?

tools/firmware/hvmloader/pci.c void pci_setup(void) · line 232

Quoted 4 lines

tools/firmware/hvmloader/pci.c +4 -0

+        }
+        else
+        {
+            pci_writeb(PCI_ISA_DEVFN, 0x60 + link, isa_irq);  

Is all this magic described somewhere that you can reference?

Quoted 1 line · On Mon, 19 Mar 2018 15:30:14 +0000 Roger Pau Monné <[email protected]> wrote:

On Tue, Mar 13, 2018 at 04:33:51AM +1000, Alexey Gerasimenko wrote:

Quoted 4 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+    {
+    case 0x0300:  

All this values need to be defines documented somewhere.

Agree... although it was not me who introduced all these hardcoded PCI class values. :) I'll change these numbers into newly added pci_regs.h #defines in the non-functional patch.

Quoted 10 lines

tools/firmware/hvmloader/pci.c void pci_setup(void) · line 228

Quoted 5 lines

tools/firmware/hvmloader/pci.c +5 -0

+        {
+            pci_writeb(PCI_ICH9_LPC_DEVFN, 0x60 + link, isa_irq);
+
+            /* PIRQE..PIRQH are unused */
+            pci_writeb(PCI_ICH9_LPC_DEVFN, 0x68 + link, 0x80);  

According to the spec 0x80 is the default value for this registers, do you really need to write it?

Is maybe QEMU not correctly setting the default value?

Won't agree here. We're initializing PIRQ[n] routing in this fragment, it's better not to rely on any values but simply initialize all PIRQ[n]_ROUT registers, this makes it explicit.

Even if it is unnecessary due to defaults it's more obvious to set these registers to our own values than to force a reader to either look up their emulation in QEMU code or read the ICH9 pdf to confirm assumptions.

Quoted 10 lines · Alexey G

Quoted 6 lines · On Mon, 19 Mar 2018 15:30:14 +0000 Roger Pau Monné <[email protected]> wrote:

Review

Quoted 2 lines · Alexey Gerasimenko

(quoted diff) +2 -0

+    {
+    case 0x0300:    

All this values need to be defines documented somewhere.

Quoted 3 lines

Agree... although it was not me who introduced all these hardcoded PCI class values. :) I'll change these numbers into newly added pci_regs.h #defines in the non-functional patch.

Right. I've realized that later. If you place this code moment in a separate patch without any other modifications I won't complain about the lack of defines (although it would be nice to have them :)).

Quoted 12 lines

Quoted 11 lines

Quoted 6 lines

tools/firmware/hvmloader/pci.c +5 -0

+        {
+            pci_writeb(PCI_ICH9_LPC_DEVFN, 0x60 + link, isa_irq);
+
+            /* PIRQE..PIRQH are unused */
+            pci_writeb(PCI_ICH9_LPC_DEVFN, 0x68 + link,

0x80);

According to the spec 0x80 is the default value for this registers, do you really need to write it?

Is maybe QEMU not correctly setting the default value?

Quoted 8 lines

Won't agree here. We're initializing PIRQ[n] routing in this fragment, it's better not to rely on any values but simply initialize all PIRQ[n]_ROUT registers, this makes it explicit.

Even if it is unnecessary due to defaults it's more obvious to set these registers to our own values than to force a reader to either look up their emulation in QEMU code or read the ICH9 pdf to confirm assumptions.

But if you start doing this, you should do it for all the registers. Why is PIRQE..PIRQH routing special that you need to re-write the default value? But not SIRQ_CNTL for example?

I think a comment noting that the default value for those registers is what we expect (0x80 - Interrupt Routing Disabled) would be better.

Thanks, Roger.

Much like normal PCI BARs or other chipset-specific memory-mapped resources, MMCONFIG area needs space in MMIO hole, so we must allocate it manually.

The actual MMCONFIG size depends on a number of PCI buses available which should be covered by ECAM. Possible options are 64MB, 128MB and 256MB. As we are limited to the bus 0 currently, thus using lowest possible setting (64MB), #defined via PCI_MAX_MCFG_BUSES in hvmloader/config.h. When multiple PCI buses support for Xen will be implemented, PCI_MAX_MCFG_BUSES may be changed to calculation of the number of buses according to results of the PCI devices enumeration.

The way to allocate MMCONFIG range in MMIO hole is similar to how other PCI BARs are allocated. The patch extends 'bars' structure to make it universal for any arbitrary BAR type -- either IO, MMIO, ROM or a chipset-specific resource.

One important new field is addr_mask, which tells which bits of the base address can (should) be written. Different address types (ROM, MMIO BAR, PCIEXBAR) will have different addr_mask values.

For every assignable BAR range we store its size, PCI device BDF (devfn actually) to which it belongs, BAR type (mem/io/mem64) and corresponding register offset in device PCI conf space. This way we can insert MMCONFIG entry into bars array in the same manner like for any other BARs. In this case, the devfn field will point to MCH PCI device and bar_reg will contain PCIEXBAR register offset. It will be assigned a slot in MMIO hole later in a very same way like for plain PCI BARs, with respect to its size alignment.

Also, to reduce code complexity, all long mem/mem64 BAR flags checks are replaced by simple bars[i] field probing, eg.:

Signed-off-by Alexey Gerasimenko <[email protected]>

---
 tools/firmware/hvmloader/config.h   |   4 ++
 tools/firmware/hvmloader/pci.c      | 127 ++++++++++++++++++++++++++++--------
 tools/firmware/hvmloader/pci_regs.h |   2 +
 3 files changed, 106 insertions(+), 27 deletions(-)

tools/firmware/hvmloader/config.h +0 -0

index 6fde6b7b60..5443ecd804 100644

tools/firmware/hvmloader/config.h +1 -0

+
 #define ACPI_TIS_HDR_ADDRESS 0xFED40F00UL
 
 extern unsigned long pci_mem_start, pci_mem_end;

tools/firmware/hvmloader/pci.c +2 -1

index 033bd20992..6de124bbd5 100644
@@ -158,9 +158,10 @@ static void class_specific_pci_device_setup(uint16_t vendor_id,
 
 void pci_setup(void)
 {
-    uint8_t is_64bar, using_64bar, bar64_relocate = 0;
+    uint8_t is_64bar, using_64bar, bar64_relocate = 0, is_mem;
     uint32_t devfn, bar_reg, cmd, bar_data, bar_data_upper;
     uint64_t base, bar_sz, bar_sz_upper, mmio_total = 0;
+    uint64_t addr_mask;
     uint16_t vendor_id, device_id;
     unsigned int bar, pin, link, isa_irq;
     int is_running_on_q35 = 0;

@@ -172,10 +173,14 @@ void pci_setup(void)
 
     /* Create a list of device BARs in descending order of size. */

tools/firmware/hvmloader/pci.c +14 -4

     } *bars = (struct bars *)scratch_start;
     unsigned int i, nr_bars = 0;
     uint64_t mmio_hole_size = 0;

@@ -259,13 +264,21 @@ void pci_setup(void)
                 bar_reg = PCI_ROM_ADDRESS;
 
             bar_data = pci_readl(devfn, bar_reg);
+
+            is_mem = !!(((bar_data & PCI_BASE_ADDRESS_SPACE) ==
+                       PCI_BASE_ADDRESS_SPACE_MEMORY) ||
+                       (bar_reg == PCI_ROM_ADDRESS));
+
             if ( bar_reg != PCI_ROM_ADDRESS )
             {
-                is_64bar = !!((bar_data & (PCI_BASE_ADDRESS_SPACE |
-                             PCI_BASE_ADDRESS_MEM_TYPE_MASK)) ==
-                             (PCI_BASE_ADDRESS_SPACE_MEMORY |
+                is_64bar = !!(is_mem &&
+                             ((bar_data & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
                              PCI_BASE_ADDRESS_MEM_TYPE_64));
+
                 pci_writel(devfn, bar_reg, ~0);
+
+                addr_mask = is_mem ? PCI_BASE_ADDRESS_MEM_MASK
+                                   : PCI_BASE_ADDRESS_IO_MASK;
             }
             else
             {

@@ -273,28 +286,35 @@ void pci_setup(void)
                 pci_writel(devfn, bar_reg,
                            (bar_data | PCI_ROM_ADDRESS_MASK) &
                            ~PCI_ROM_ADDRESS_ENABLE);
+
+                addr_mask = PCI_ROM_ADDRESS_MASK;
             }
+
             bar_sz = pci_readl(devfn, bar_reg);
             pci_writel(devfn, bar_reg, bar_data);
 
             if ( bar_reg != PCI_ROM_ADDRESS )
-                bar_sz &= (((bar_data & PCI_BASE_ADDRESS_SPACE) ==

tools/firmware/hvmloader/pci.c +13 -7

+            {
                 bar_data_upper = pci_readl(devfn, bar_reg + 4);
                 pci_writel(devfn, bar_reg + 4, ~0);
                 bar_sz_upper = pci_readl(devfn, bar_reg + 4);
                 pci_writel(devfn, bar_reg + 4, bar_data_upper);
                 bar_sz = (bar_sz_upper << 32) | bar_sz;
             }
+
             bar_sz &= ~(bar_sz - 1);
             if ( bar_sz == 0 )
                 continue;
 
+            /* leave only memtype/enable bits etc */
+            bar_data &= ~addr_mask;
+
             for ( i = 0; i < nr_bars; i++ )
                 if ( bars[i].bar_sz < bar_sz )
                     break;

@@ -302,14 +322,15 @@ void pci_setup(void)
             if ( i != nr_bars )
                 memmove(&bars[i+1], &bars[i], (nr_bars-i) * sizeof(*bars));
 
-            bars[i].is_64bar = is_64bar;
-            bars[i].devfn   = devfn;
-            bars[i].bar_reg = bar_reg;
-            bars[i].bar_sz  = bar_sz;
+            bars[i].is_64bar  = is_64bar;
+            bars[i].is_mem    = is_mem;
+            bars[i].devfn     = devfn;
+            bars[i].bar_reg   = bar_reg;
+            bars[i].bar_sz    = bar_sz;
+            bars[i].addr_mask = addr_mask;
+            bars[i].bar_data  = bar_data;
 
-            if ( ((bar_data & PCI_BASE_ADDRESS_SPACE) ==
-                  PCI_BASE_ADDRESS_SPACE_MEMORY) ||
-                 (bar_reg == PCI_ROM_ADDRESS) )
+            if ( is_mem )
                 mmio_total += bar_sz;
 
             nr_bars++;

@@ -339,6 +360,63 @@ void pci_setup(void)
         pci_writew(devfn, PCI_COMMAND, cmd);
     }

tools/firmware/hvmloader/pci.c +12 -0

+
+        switch (PCI_MAX_MCFG_BUSES)
+        {
+        case 64:
+            bar_data = PCIEXBAR_64_BUSES | PCIEXBAR_ENABLE;
+            bar_sz = MB(64);
+            break;
+
+        case 128:
+            bar_data = PCIEXBAR_128_BUSES | PCIEXBAR_ENABLE;
+            bar_sz = MB(128);
+            break;

Quoted 12 lines · Alexey Gerasimenko

Much like normal PCI BARs or other chipset-specific memory-mapped resources, MMCONFIG area needs space in MMIO hole, so we must allocate it manually.

The actual MMCONFIG size depends on a number of PCI buses available which should be covered by ECAM. Possible options are 64MB, 128MB and 256MB. As we are limited to the bus 0 currently, thus using lowest possible setting (64MB), #defined via PCI_MAX_MCFG_BUSES in hvmloader/config.h. When multiple PCI buses support for Xen will be implemented, PCI_MAX_MCFG_BUSES may be changed to calculation of the number of buses according to results of the PCI devices enumeration.

Quoted 4 lines

The way to allocate MMCONFIG range in MMIO hole is similar to how other PCI BARs are allocated. The patch extends 'bars' structure to make it universal for any arbitrary BAR type -- either IO, MMIO, ROM or a chipset-specific resource.

I'm not sure this is fully correct. The IOREQ interface can differentiate PCI devices and forward config space accesses to different emulators (see IOREQ_TYPE_PCI_CONFIG). With this change you will forward all MCFG accesses to QEMU, which will likely be wrong if there are multiple PCI-device emulators for the same domain.

Ie: AFAICT Xen needs to know about the MCFG emulation and detect accesses to it in order to forward them to the right emulators.

Adding Paul who knows more about all this.

Quoted 13 lines

One important new field is addr_mask, which tells which bits of the base address can (should) be written. Different address types (ROM, MMIO BAR, PCIEXBAR) will have different addr_mask values.

For every assignable BAR range we store its size, PCI device BDF (devfn actually) to which it belongs, BAR type (mem/io/mem64) and corresponding register offset in device PCI conf space. This way we can insert MMCONFIG entry into bars array in the same manner like for any other BARs. In this case, the devfn field will point to MCH PCI device and bar_reg will contain PCIEXBAR register offset. It will be assigned a slot in MMIO hole later in a very same way like for plain PCI BARs, with respect to its size alignment.

tools/firmware/hvmloader/pci.c void pci_setup(void) · line 622

Quoted 6 lines

Also, to reduce code complexity, all long mem/mem64 BAR flags checks are replaced by simple bars[i] field probing, eg.:

tools/firmware/hvmloader/pci.c +1 -3

-        if ( (bar_reg == PCI_ROM_ADDRESS) ||
-             ((bar_data & PCI_BASE_ADDRESS_SPACE) ==
-              PCI_BASE_ADDRESS_SPACE_MEMORY) )
+        if ( bars[i].is_mem )  

This should be a separate change IMO.

Quoted 16 lines

 tools/firmware/hvmloader/config.h   |   4 ++
 tools/firmware/hvmloader/pci.c      | 127

++++++++++++++++++++++++++++--------
tools/firmware/hvmloader/pci_regs.h | 2 + 3 files changed, 106
insertions(+), 27 deletions(-)

(quoted diff) +0 -0

b/tools/firmware/hvmloader/config.h index 6fde6b7b60..5443ecd804 100644 --- a/tools/firmware/hvmloader/config.h
+++ b/tools/firmware/hvmloader/config.h

(quoted diff) +0 -0

@@ -53,10 +53,14 @@ extern uint8_t ioapic_version;
 #define PCI_ISA_DEVFN       0x08    /* dev 1, fn 0 */
 #define PCI_ISA_IRQ_MASK    0x0c20U /* ISA IRQs 5,10,11 are PCI

connected */ #define PCI_ICH9_LPC_DEVFN 0xf8 /* dev 31, fn 0 */
+#define PCI_MCH_DEVFN 0 /* bus 0, dev 0, func 0 */

tools/firmware/hvmloader/config.h extern uint8_t ioapic_version; · line 62

Quoted 5 lines

/* MMIO hole: Hardcoded defaults, which can be dynamically
expanded. */ #define PCI_MEM_END 0xfc000000

tools/firmware/hvmloader/config.h +2 -0

+/* possible values are: 64, 128, 256 */
+#define PCI_MAX_MCFG_BUSES  64  

What the reasoning for this value? Do we know which devices need ECAM areas?

Quoted 21 lines

#define ACPI_TIS_HDR_ADDRESS 0xFED40F00UL

extern unsigned long pci_mem_start, pci_mem_end;

(quoted diff) +0 -0

b/tools/firmware/hvmloader/pci.c index 033bd20992..6de124bbd5 100644

tools/firmware/hvmloader/pci.c +2 -1

@@ -158,9 +158,10 @@ static void
 class_specific_pci_device_setup(uint16_t vendor_id, 
 void pci_setup(void)
 {
-    uint8_t is_64bar, using_64bar, bar64_relocate = 0;
+    uint8_t is_64bar, using_64bar, bar64_relocate = 0, is_mem;
     uint32_t devfn, bar_reg, cmd, bar_data, bar_data_upper;
     uint64_t base, bar_sz, bar_sz_upper, mmio_total = 0;
+    uint64_t addr_mask;
     uint16_t vendor_id, device_id;
     unsigned int bar, pin, link, isa_irq;
     int is_running_on_q35 = 0;

@@ -172,10 +173,14 @@ void pci_setup(void)
 

tools/firmware/hvmloader/pci.c void pci_setup(void) · line 183

Quoted 12 lines

tools/firmware/hvmloader/pci.c +1 -1

     /* Create a list of device BARs in descending order of size. */
     struct bars {
-        uint32_t is_64bar;
         uint32_t devfn;
         uint32_t bar_reg;
         uint64_t bar_sz;
+        uint64_t addr_mask; /* which bits of the base address can

be written */

tools/firmware/hvmloader/pci.c +4 -0

+        uint32_t bar_data;  /* initial value - BAR flags here */
+        uint8_t  is_64bar;
+        uint8_t  is_mem;
+        uint8_t  padding[2];  

Why are you manually adding a padding here? Also why not make this fields bool?

Quoted 39 lines

     } *bars = (struct bars *)scratch_start;
     unsigned int i, nr_bars = 0;
     uint64_t mmio_hole_size = 0;

tools/firmware/hvmloader/pci.c +14 -3

@@ -259,13 +264,21 @@ void pci_setup(void)
                 bar_reg = PCI_ROM_ADDRESS;
 
             bar_data = pci_readl(devfn, bar_reg);
+
+            is_mem = !!(((bar_data & PCI_BASE_ADDRESS_SPACE) ==
+                       PCI_BASE_ADDRESS_SPACE_MEMORY) ||
+                       (bar_reg == PCI_ROM_ADDRESS));
+
             if ( bar_reg != PCI_ROM_ADDRESS )
             {
-                is_64bar = !!((bar_data & (PCI_BASE_ADDRESS_SPACE |
-                             PCI_BASE_ADDRESS_MEM_TYPE_MASK)) ==
-                             (PCI_BASE_ADDRESS_SPACE_MEMORY |
+                is_64bar = !!(is_mem &&
+                             ((bar_data &
 PCI_BASE_ADDRESS_MEM_TYPE_MASK) == PCI_BASE_ADDRESS_MEM_TYPE_64));
+
                 pci_writel(devfn, bar_reg, ~0);
+
+                addr_mask = is_mem ? PCI_BASE_ADDRESS_MEM_MASK
+                                   : PCI_BASE_ADDRESS_IO_MASK;
             }
             else
             {

@@ -273,28 +286,35 @@ void pci_setup(void)
                 pci_writel(devfn, bar_reg,
                            (bar_data | PCI_ROM_ADDRESS_MASK) &
                            ~PCI_ROM_ADDRESS_ENABLE);
+
+                addr_mask = PCI_ROM_ADDRESS_MASK;
             }
+
             bar_sz = pci_readl(devfn, bar_reg);
             pci_writel(devfn, bar_reg, bar_data);
 

tools/firmware/hvmloader/pci.c void pci_setup(void) · line 302

Quoted 12 lines

tools/firmware/hvmloader/pci.c +2 -4

             if ( bar_reg != PCI_ROM_ADDRESS )
-                bar_sz &= (((bar_data & PCI_BASE_ADDRESS_SPACE) ==
-                            PCI_BASE_ADDRESS_SPACE_MEMORY) ?
-                           PCI_BASE_ADDRESS_MEM_MASK :
-                           (PCI_BASE_ADDRESS_IO_MASK & 0xffff));
+                bar_sz &= is_mem ? PCI_BASE_ADDRESS_MEM_MASK :
+                                   (PCI_BASE_ADDRESS_IO_MASK &

0xffff); else

                 bar_sz &= PCI_ROM_ADDRESS_MASK;

tools/firmware/hvmloader/pci.c +2 -1

-            if (is_64bar) {
+
+            if (is_64bar)  

Coding style (spaces between parentheses).

Quoted 45 lines

tools/firmware/hvmloader/pci.c +13 -7

+            {
                 bar_data_upper = pci_readl(devfn, bar_reg + 4);
                 pci_writel(devfn, bar_reg + 4, ~0);
                 bar_sz_upper = pci_readl(devfn, bar_reg + 4);
                 pci_writel(devfn, bar_reg + 4, bar_data_upper);
                 bar_sz = (bar_sz_upper << 32) | bar_sz;
             }
+
             bar_sz &= ~(bar_sz - 1);
             if ( bar_sz == 0 )
                 continue;
 
+            /* leave only memtype/enable bits etc */
+            bar_data &= ~addr_mask;
+
             for ( i = 0; i < nr_bars; i++ )
                 if ( bars[i].bar_sz < bar_sz )
                     break;

@@ -302,14 +322,15 @@ void pci_setup(void)
             if ( i != nr_bars )
                 memmove(&bars[i+1], &bars[i], (nr_bars-i) *
 sizeof(*bars)); 
-            bars[i].is_64bar = is_64bar;
-            bars[i].devfn   = devfn;
-            bars[i].bar_reg = bar_reg;
-            bars[i].bar_sz  = bar_sz;
+            bars[i].is_64bar  = is_64bar;
+            bars[i].is_mem    = is_mem;
+            bars[i].devfn     = devfn;
+            bars[i].bar_reg   = bar_reg;
+            bars[i].bar_sz    = bar_sz;
+            bars[i].addr_mask = addr_mask;
+            bars[i].bar_data  = bar_data;
 
-            if ( ((bar_data & PCI_BASE_ADDRESS_SPACE) ==
-                  PCI_BASE_ADDRESS_SPACE_MEMORY) ||
-                 (bar_reg == PCI_ROM_ADDRESS) )
+            if ( is_mem )
                 mmio_total += bar_sz;
 
             nr_bars++;

@@ -339,6 +360,63 @@ void pci_setup(void)
         pci_writew(devfn, PCI_COMMAND, cmd);
     }
 

tools/firmware/hvmloader/pci.c void pci_setup(void) · line 371

Quoted 10 lines

tools/firmware/hvmloader/pci.c +2 -0

+    /*
+     *  Calculate MMCONFIG area size and squeeze it into the bars

array

tools/firmware/hvmloader/pci.c +7 -0

+     *  for assigning a slot in the MMIO hole
+     */
+    if (is_running_on_q35)
+    {
+        /* disable PCIEXBAR decoding for now */
+        pci_writel(PCI_MCH_DEVFN, PCI_MCH_PCIEXBAR, 0);
+        pci_writel(PCI_MCH_DEVFN, PCI_MCH_PCIEXBAR + 4, 0);  

I'm afraid I will need some context here, where is the description for the config space of dev 0 fn 0? I don't seem to be able to find it in the ich9 spec.

tools/firmware/hvmloader/pci.c void pci_setup(void) · line 376

Quoted 5 lines

tools/firmware/hvmloader/pci.c +5 -0

+
+#define PCIEXBAR_64_BUSES    (2 << 1)
+#define PCIEXBAR_128_BUSES   (1 << 1)
+#define PCIEXBAR_256_BUSES   (0 << 1)
+#define PCIEXBAR_ENABLE      (1 << 0)  

Why those strange definitions? (0 << 1)? (2 << 1) instead of (1 << 2)?

Quoted 18 lines

tools/firmware/hvmloader/pci.c +18 -0

+
+        switch (PCI_MAX_MCFG_BUSES)
+        {
+        case 64:
+            bar_data = PCIEXBAR_64_BUSES | PCIEXBAR_ENABLE;
+            bar_sz = MB(64);
+            break;
+
+        case 128:
+            bar_data = PCIEXBAR_128_BUSES | PCIEXBAR_ENABLE;
+            bar_sz = MB(128);
+            break;
+
+        case 256:
+            bar_data = PCIEXBAR_256_BUSES | PCIEXBAR_ENABLE;
+            bar_sz = MB(256);
+            break;
+

tools/firmware/hvmloader/pci.c void pci_setup(void) · line 398

Quoted 4 lines

tools/firmware/hvmloader/pci.c +4 -0

+        default:
+            /* unsupported number of buses specified */
+            BUG();
+        }  

I don't see how PCI_MAX_MCFG_BUSES should be used. Is the user supposed to know what value to use at compile time? What about distro packagers?

Quoted 24 lines · On Mon, 19 Mar 2018 15:58:02 +0000 Roger Pau Monné <[email protected]> wrote:

Quoted 12 lines · Alexey Gerasimenko

Much like normal PCI BARs or other chipset-specific memory-mapped resources, MMCONFIG area needs space in MMIO hole, so we must allocate it manually.

The actual MMCONFIG size depends on a number of PCI buses available which should be covered by ECAM. Possible options are 64MB, 128MB and 256MB. As we are limited to the bus 0 currently, thus using lowest possible setting (64MB), #defined via PCI_MAX_MCFG_BUSES in hvmloader/config.h. When multiple PCI buses support for Xen will be implemented, PCI_MAX_MCFG_BUSES may be changed to calculation of the number of buses according to results of the PCI devices enumeration.

Quoted 4 lines

The way to allocate MMCONFIG range in MMIO hole is similar to how other PCI BARs are allocated. The patch extends 'bars' structure to make it universal for any arbitrary BAR type -- either IO, MMIO, ROM or a chipset-specific resource.

I'm not sure this is fully correct. The IOREQ interface can differentiate PCI devices and forward config space accesses to different emulators (see IOREQ_TYPE_PCI_CONFIG). With this change you will forward all MCFG accesses to QEMU, which will likely be wrong if there are multiple PCI-device emulators for the same domain.

Quoted 4 lines

Ie: AFAICT Xen needs to know about the MCFG emulation and detect accesses to it in order to forward them to the right emulators.

Adding Paul who knows more about all this.

In which use cases multiple PCI-device emulators are used for a single HVM domain? Is it a proprietary setup?

I assume it is somehow related to this code in xen-hvm.c:

                /* Fake a write to port 0xCF8 so that
                 * the config space access will target the
                 * correct device model.
                 */
                val = (1u << 31) | ((req->addr & 0x0f00) <...>
                do_outp(0xcf8, 4, val);

if yes, similar thing can be made for IOREQ_TYPE_COPY accesses to the emulated MMCONFIG if needed.

In HVM+QEMU case we are not limited to merely passed through devices, most of the observable PCI config space devices belong to one particular QEMU instance. This dictates the overall emulated MMCONFIG layout for a domain which should be in sync to what QEMU emulates via CF8h/CFCh accesses... and between multiple device model instances (if there are any, still not sure what multiple PCI-device emulators you mentioned really are).

Basically, we have an emulated MMCONFIG area of 64/128/256MB size in the MMIO hole of the guest HVM domain. (BTW, this area itself can be considered a feature of the chipset the device model emulates.) It can be relocated to some other place in MMIO hole, this means that QEMU will trap accesses to the specific to the emulated chipset PCIEXBAR register and will issue same MMIO unmap/map calls as for any normal emulated MMIO range.

On the other hand, it won't be easy to provide emulated MMCONFIG translation into IOREQ_TYPE_PCI_CONFIG from Xen side. Xen should know current emulated MMCONFIG area position and size in order to translate (or not) accesses to it into corresponding BDF/reg pair (+whether that area is enabled for decoding or not). This will likely require to introduce new hypercall(s).

The question is if there will be any difference or benefit at all.

It's basically the same emulated MMIO range after all, but in one case we trap accesses to it in Xen and translate them into IOREQ_TYPE_PCI_CONFIG requests.
We have to provide some infrastructure to let Xen know where the device model/guest expects to use the MMCONFIG area (and its size). The device model will need to use this infrastructure, informing Xen of any changes. Also, due to MMCONFIG nature there might be some pitfalls like a necessity to send multiple IOREQ_TYPE_PCI_CONFIG ioreqs caused by a single memory read/write operation.

In another case, we still have an emulated MMIO range, but Xen will send plain IOREQ_TYPE_COPY requests to QEMU which it handles itself. In such case, all code to work with MMCONFIG accesses is available for reuse right away (mmcfg -> pci_* translation in QEMU), no new functionality required neither in Xen or QEMU.

Quoted 16 lines

Quoted 16 lines

 tools/firmware/hvmloader/config.h   |   4 ++
 tools/firmware/hvmloader/pci.c      | 127

++++++++++++++++++++++++++++--------
tools/firmware/hvmloader/pci_regs.h | 2 + 3 files changed, 106
insertions(+), 27 deletions(-)

(quoted diff) +0 -0

b/tools/firmware/hvmloader/config.h index 6fde6b7b60..5443ecd804 100644 --- a/tools/firmware/hvmloader/config.h
+++ b/tools/firmware/hvmloader/config.h

(quoted diff) +0 -0

@@ -53,10 +53,14 @@ extern uint8_t ioapic_version;
 #define PCI_ISA_DEVFN       0x08    /* dev 1, fn 0 */
 #define PCI_ISA_IRQ_MASK    0x0c20U /* ISA IRQs 5,10,11 are PCI

connected */ #define PCI_ICH9_LPC_DEVFN 0xf8 /* dev 31, fn 0 */
+#define PCI_MCH_DEVFN 0 /* bus 0, dev 0, func 0 */

Quoted 8 lines

tools/firmware/hvmloader/config.h extern uint8_t ioapic_version; · line 62

Quoted 5 lines

/* MMIO hole: Hardcoded defaults, which can be dynamically
expanded. */ #define PCI_MEM_END 0xfc000000

tools/firmware/hvmloader/config.h +2 -0

+/* possible values are: 64, 128, 256 */
+#define PCI_MAX_MCFG_BUSES  64  

What the reasoning for this value? Do we know which devices need ECAM areas?

Yes, Xen is limited to bus 0 emulation currently, the description states "When multiple PCI buses support for Xen will be implemented, PCI_MAX_MCFG_BUSES may be changed to calculation of the number of buses according to results of the PCI devices enumeration".

I think it might be better to replace 'switch (PCI_MAX_MCFG_BUSES)' with the real code right away, i.e. change it to

'switch (max_bus_num, aligned up to 64/128/256 boundary)', where max_bus_num should be set in PCI device enumeration code in pci_setup(). As we are limited to bus 0 currently, we'll just set it to 0 for now, before/after the PCI device enumeration loop (which should became multi-bus capable eventually).

-----Original Message-----
From: Roger Pau Monne
Sent: 20 March 2018 08:51
To: Alexey G <[email protected]>
Cc: [email protected]; Andrew Cooper <[email protected]>; Ian Jackson <[email protected]>; Jan Beulich <[email protected]>; Wei Liu <[email protected]>; Paul Durrant <[email protected]>
Subject: Re: [Xen-devel] [RFC PATCH 07/12] hvmloader: allocate MMCONFIG area in the MMIO hole + minor code refactoring

Quoted 8 lines · Alexey G

Quoted 5 lines · On Mon, 19 Mar 2018 15:58:02 +0000 Roger Pau Monné <[email protected]> wrote:

Quoted 4 lines · Alexey Gerasimenko

Much like normal PCI BARs or other chipset-specific memory-mapped resources, MMCONFIG area needs space in MMIO hole, so we must allocate it manually.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

The actual MMCONFIG size depends on a number of PCI buses available which should be covered by ECAM. Possible options are 64MB, 128MB and 256MB. As we are limited to the bus 0 currently, thus using lowest possible setting (64MB), #defined via PCI_MAX_MCFG_BUSES in hvmloader/config.h. When multiple PCI buses support for Xen will be implemented, PCI_MAX_MCFG_BUSES may be changed to calculation

of the

Quoted 10 lines

Quoted 10 lines

Quoted 6 lines

number of buses according to results of the PCI devices enumeration.

The way to allocate MMCONFIG range in MMIO hole is similar to how other PCI BARs are allocated. The patch extends 'bars' structure to make it universal for any arbitrary BAR type -- either IO, MMIO, ROM or a chipset-specific resource.

I'm not sure this is fully correct. The IOREQ interface can differentiate PCI devices and forward config space accesses to different emulators (see IOREQ_TYPE_PCI_CONFIG). With this change

you

Quoted 6 lines

Quoted 6 lines

will forward all MCFG accesses to QEMU, which will likely be wrong if there are multiple PCI-device emulators for the same domain.

Ie: AFAICT Xen needs to know about the MCFG emulation and detect accesses to it in order to forward them to the right emulators.

Quoted 4 lines

Quoted 1 line

Adding Paul who knows more about all this.

In which use cases multiple PCI-device emulators are used for a single HVM domain? Is it a proprietary setup?

Likely. I think XenGT might be using it. It's a feature of the IOREQ implementation in Xen.

Traditional PCI config space accesses are not IO port space accesses. The IOREQ code in Xen detects accesses to ports 0xcf8/0xcfc and IOREQ servers can register devices they would like to receive configuration space accesses for. QEMU is already making use of this, see for example xen_map_pcidev in the QEMU code.

By treating MCFG accesses as MMIO you are bypassing the IOREQ PCI layer, and thus a IOREQ server could register a PCI device and only receive PCI configuration accesses from the IO port space, while MCFG accesses would be forwarded somewhere else.

I think you need to make the IOREQ code aware of the MCFG area and XEN_DMOP_IO_RANGE_PCI needs to forward both IO space and MCFG accesses
to the right IOREQ server.

Quoted 10 lines

I assume it is somehow related to this code in xen-hvm.c:

                /* Fake a write to port 0xCF8 so that
                 * the config space access will target the
                 * correct device model.
                 */
                val = (1u << 31) | ((req->addr & 0x0f00) <...>
                do_outp(0xcf8, 4, val);

if yes, similar thing can be made for IOREQ_TYPE_COPY accesses to the emulated MMCONFIG if needed.

I have to admit I don't know that much about QEMU, and I have no idea what the chunk above is supposed to accomplish.

Quoted 5 lines

In HVM+QEMU case we are not limited to merely passed through devices, most of the observable PCI config space devices belong to one particular QEMU instance. This dictates the overall emulated MMCONFIG layout for a domain which should be in sync to what QEMU emulates via

CF8h/CFCh

Quoted 3 lines

accesses... and between multiple device model instances (if there are any, still not sure what multiple PCI-device emulators you mentioned really are).

In newer versions of Xen (>4.5 IIRC, Paul knows more), QEMU doesn't directly trap accesses to the 0xcf8/0xcfc IO ports, it's Xen instead the one that detects and decodes such accesses, and then forwards them to the IOREQ server that has been registered to handle them.

You cannot simply forward all MCFG accesses to QEMU as MMIO accesses, Xen needs to decode them and they need to be handled as IOREQ_TYPE_PCI_CONFIG requests, not IOREQ_TYPE_COPY IMO.

Quoted 9 lines

Basically, we have an emulated MMCONFIG area of 64/128/256MB size in the MMIO hole of the guest HVM domain. (BTW, this area itself can be considered a feature of the chipset the device model emulates.) It can be relocated to some other place in MMIO hole, this means that QEMU will trap accesses to the specific to the emulated chipset PCIEXBAR register and will issue same MMIO unmap/map calls as for any normal emulated MMIO range.

Quoted 6 lines

On the other hand, it won't be easy to provide emulated MMCONFIG translation into IOREQ_TYPE_PCI_CONFIG from Xen side. Xen should know current emulated MMCONFIG area position and size in order to translate (or not) accesses to it into corresponding BDF/reg pair (+whether that area is enabled for decoding or not). This will likely require to introduce new hypercall(s).

Yes, you will have to introduce new hypercalls to tell Xen the position/size of the MCFG hole. Likely you want to tell it the start address, the pci segment, start bus and end bus. I know pci segment and start bus is always going to be 0 ATM, but it would be nice to have a complete interface.

By your comment above I think you want an interface that allows you to remove/add those MCFG areas at runtime.

Quoted 1 line

The question is if there will be any difference or benefit at all.

IMO it's not about benefits or differences, it's about correctness. Xen currently detects accesses to the PCI configuration space from IO ports and for consistency it should also detect accesses to this space by any other means.

Quoted 8 lines

It's basically the same emulated MMIO range after all, but in one case we trap accesses to it in Xen and translate them into IOREQ_TYPE_PCI_CONFIG requests.
We have to provide some infrastructure to let Xen know where the device model/guest expects to use the MMCONFIG area (and its size). The device model will need to use this infrastructure, informing Xen of any changes. Also, due to MMCONFIG nature there might be some pitfalls like a necessity to send multiple IOREQ_TYPE_PCI_CONFIG ioreqs caused

by

Quoted 1 line

a single memory read/write operation.

This seems all fine. Why do you expect MCFG access to create multiple IOREQ_TYPE_PCI_CONFIG but not multiple IOREQ_TYPE_COPY?

Quoted 5 lines

In another case, we still have an emulated MMIO range, but Xen will send plain IOREQ_TYPE_COPY requests to QEMU which it handles itself. In such case, all code to work with MMCONFIG accesses is available for reuse right away (mmcfg -> pci_* translation in QEMU), no new functionality required neither in Xen or QEMU.

As I tried to argument above, I think this is not correct, but I would also like that Paul expresses his opinion as the IOREQ maintainer.

Quoted 26 lines

Quoted 17 lines

Quoted 17 lines

 tools/firmware/hvmloader/config.h   |   4 ++
 tools/firmware/hvmloader/pci.c      | 127

++++++++++++++++++++++++++++--------
tools/firmware/hvmloader/pci_regs.h | 2 + 3 files changed, 106
insertions(+), 27 deletions(-)

(quoted diff) +0 -0

b/tools/firmware/hvmloader/config.h index 6fde6b7b60..5443ecd804 100644 --- a/tools/firmware/hvmloader/config.h
+++ b/tools/firmware/hvmloader/config.h

(quoted diff) +0 -0

@@ -53,10 +53,14 @@ extern uint8_t ioapic_version;
 #define PCI_ISA_DEVFN       0x08    /* dev 1, fn 0 */
 #define PCI_ISA_IRQ_MASK    0x0c20U /* ISA IRQs 5,10,11 are PCI

connected */ #define PCI_ICH9_LPC_DEVFN 0xf8 /* dev 31, fn 0 */
+#define PCI_MCH_DEVFN 0 /* bus 0, dev 0, func 0 */

Quoted 8 lines

tools/firmware/hvmloader/config.h extern uint8_t ioapic_version; · line 62

Quoted 5 lines

/* MMIO hole: Hardcoded defaults, which can be dynamically
expanded. */ #define PCI_MEM_END 0xfc000000

tools/firmware/hvmloader/config.h +2 -0

+/* possible values are: 64, 128, 256 */
+#define PCI_MAX_MCFG_BUSES  64

What the reasoning for this value? Do we know which devices need ECAM areas?

Quoted 3 lines

Yes, Xen is limited to bus 0 emulation currently, the description states "When multiple PCI buses support for Xen will be implemented, PCI_MAX_MCFG_BUSES may be changed to calculation of the number of

buses

Quoted 10 lines

according to results of the PCI devices enumeration".

I think it might be better to replace 'switch (PCI_MAX_MCFG_BUSES)' with the real code right away, i.e. change it to

'switch (max_bus_num, aligned up to 64/128/256 boundary)', where max_bus_num should be set in PCI device enumeration code in pci_setup(). As we are limited to bus 0 currently, we'll just set it to 0 for now, before/after the PCI device enumeration loop (which should became multi-bus capable eventually).

I guess this is all pretty much hardcoded to bus 0 in several places, so I'm not sure it's worth to add PCI_MAX_MCFG_BUSES. IMO if something like this should be added it should be PCI_MAX_BUSES, and several places should be changed to make use of it. Or ideally we should find a way to detect this at runtime, without needed any hardcoded defines.

I think it would be good if you can add a note comment describing the different MCFG sizes supported by the Q35 chipset (64/128/256).

Thanks, Roger.

Quoted 33 lines · Alexey G

Quoted 30 lines · On Mon, 19 Mar 2018 15:58:02 +0000 Roger Pau Monné <[email protected]> wrote:

Quoted 13 lines · Alexey Gerasimenko

Much like normal PCI BARs or other chipset-specific memory-mapped resources, MMCONFIG area needs space in MMIO hole, so we must allocate it manually.

The actual MMCONFIG size depends on a number of PCI buses available which should be covered by ECAM. Possible options are 64MB, 128MB and 256MB. As we are limited to the bus 0 currently, thus using lowest possible setting (64MB), #defined via PCI_MAX_MCFG_BUSES in hvmloader/config.h. When multiple PCI buses support for Xen will be implemented, PCI_MAX_MCFG_BUSES may be changed to calculation of the number of buses according to results of the PCI devices enumeration.

Quoted 4 lines

The way to allocate MMCONFIG range in MMIO hole is similar to how other PCI BARs are allocated. The patch extends 'bars' structure to make it universal for any arbitrary BAR type -- either IO, MMIO, ROM or a chipset-specific resource.

I'm not sure this is fully correct. The IOREQ interface can differentiate PCI devices and forward config space accesses to different emulators (see IOREQ_TYPE_PCI_CONFIG). With this change you will forward all MCFG accesses to QEMU, which will likely be wrong if there are multiple PCI-device emulators for the same domain.

Ie: AFAICT Xen needs to know about the MCFG emulation and detect accesses to it in order to forward them to the right emulators.

Quoted 4 lines

Quoted 1 line

Adding Paul who knows more about all this.

In which use cases multiple PCI-device emulators are used for a single HVM domain? Is it a proprietary setup?

Likely. I think XenGT might be using it. It's a feature of the IOREQ implementation in Xen.

Traditional PCI config space accesses are not IO port space accesses.

The IOREQ code in Xen detects accesses to ports 0xcf8/0xcfc and IOREQ servers can register devices they would like to receive configuration space accesses for. QEMU is already making use of this, see for

example xen_map_pcidev in the QEMU code.

By treating MCFG accesses as MMIO you are bypassing the IOREQ PCI layer, and thus a IOREQ server could register a PCI device and only receive PCI configuration accesses from the IO port space, while MCFG accesses would be forwarded somewhere else.

I think you need to make the IOREQ code aware of the MCFG area and XEN_DMOP_IO_RANGE_PCI needs to forward both IO space and MCFG accesses to the right IOREQ server.

Quoted 9 lines

I assume it is somehow related to this code in xen-hvm.c:

                /* Fake a write to port 0xCF8 so that
                 * the config space access will target the
                 * correct device model.
                 */
                val = (1u << 31) | ((req->addr & 0x0f00) <...>
                do_outp(0xcf8, 4, val);

if yes, similar thing can be made for IOREQ_TYPE_COPY accesses to the emulated MMCONFIG if needed.

I have to admit I don't know that much about QEMU, and I have no idea what the chunk above is supposed to accomplish.

Quoted 8 lines

In HVM+QEMU case we are not limited to merely passed through devices, most of the observable PCI config space devices belong to one particular QEMU instance. This dictates the overall emulated MMCONFIG layout for a domain which should be in sync to what QEMU emulates via CF8h/CFCh accesses... and between multiple device model instances (if there are any, still not sure what multiple PCI-device emulators you mentioned really are).

In newer versions of Xen (>4.5 IIRC, Paul knows more), QEMU doesn't directly trap accesses to the 0xcf8/0xcfc IO ports, it's Xen instead the one that detects and decodes such accesses, and then forwards them to the IOREQ server that has been registered to handle them.

You cannot simply forward all MCFG accesses to QEMU as MMIO accesses, Xen needs to decode them and they need to be handled as IOREQ_TYPE_PCI_CONFIG requests, not IOREQ_TYPE_COPY IMO.

Quoted 9 lines

Basically, we have an emulated MMCONFIG area of 64/128/256MB size in the MMIO hole of the guest HVM domain. (BTW, this area itself can be considered a feature of the chipset the device model emulates.) It can be relocated to some other place in MMIO hole, this means that QEMU will trap accesses to the specific to the emulated chipset PCIEXBAR register and will issue same MMIO unmap/map calls as for any normal emulated MMIO range.

Quoted 6 lines

On the other hand, it won't be easy to provide emulated MMCONFIG translation into IOREQ_TYPE_PCI_CONFIG from Xen side. Xen should know current emulated MMCONFIG area position and size in order to translate (or not) accesses to it into corresponding BDF/reg pair (+whether that area is enabled for decoding or not). This will likely require to introduce new hypercall(s).

Yes, you will have to introduce new hypercalls to tell Xen the position/size of the MCFG hole. Likely you want to tell it the start address, the pci segment, start bus and end bus. I know pci segment and start bus is always going to be 0 ATM, but it would be nice to have a complete interface.

By your comment above I think you want an interface that allows you to remove/add those MCFG areas at runtime.

Quoted 1 line

The question is if there will be any difference or benefit at all.

IMO it's not about benefits or differences, it's about correctness. Xen currently detects accesses to the PCI configuration space from IO ports and for consistency it should also detect accesses to this space by any other means.

Quoted 9 lines

It's basically the same emulated MMIO range after all, but in one case we trap accesses to it in Xen and translate them into IOREQ_TYPE_PCI_CONFIG requests.
We have to provide some infrastructure to let Xen know where the device model/guest expects to use the MMCONFIG area (and its size). The device model will need to use this infrastructure, informing Xen of any changes. Also, due to MMCONFIG nature there might be some pitfalls like a necessity to send multiple IOREQ_TYPE_PCI_CONFIG ioreqs caused by a single memory read/write operation.

This seems all fine. Why do you expect MCFG access to create multiple IOREQ_TYPE_PCI_CONFIG but not multiple IOREQ_TYPE_COPY?

Quoted 5 lines

In another case, we still have an emulated MMIO range, but Xen will send plain IOREQ_TYPE_COPY requests to QEMU which it handles itself. In such case, all code to work with MMCONFIG accesses is available for reuse right away (mmcfg -> pci_* translation in QEMU), no new functionality required neither in Xen or QEMU.

As I tried to argument above, I think this is not correct, but I would also like that Paul expresses his opinion as the IOREQ maintainer.

Quoted 30 lines

Quoted 16 lines

Quoted 16 lines

 tools/firmware/hvmloader/config.h   |   4 ++
 tools/firmware/hvmloader/pci.c      | 127

++++++++++++++++++++++++++++--------
tools/firmware/hvmloader/pci_regs.h | 2 + 3 files changed, 106
insertions(+), 27 deletions(-)

(quoted diff) +0 -0

b/tools/firmware/hvmloader/config.h index 6fde6b7b60..5443ecd804 100644 --- a/tools/firmware/hvmloader/config.h
+++ b/tools/firmware/hvmloader/config.h

(quoted diff) +0 -0

@@ -53,10 +53,14 @@ extern uint8_t ioapic_version;
 #define PCI_ISA_DEVFN       0x08    /* dev 1, fn 0 */
 #define PCI_ISA_IRQ_MASK    0x0c20U /* ISA IRQs 5,10,11 are PCI

connected */ #define PCI_ICH9_LPC_DEVFN 0xf8 /* dev 31, fn 0
*/ +#define PCI_MCH_DEVFN 0 /* bus 0, dev 0, func 0 */

Quoted 8 lines

tools/firmware/hvmloader/config.h extern uint8_t ioapic_version; · line 62

Quoted 5 lines

/* MMIO hole: Hardcoded defaults, which can be dynamically
expanded. */ #define PCI_MEM_END 0xfc000000

tools/firmware/hvmloader/config.h +2 -0

+/* possible values are: 64, 128, 256 */
+#define PCI_MAX_MCFG_BUSES  64    

What the reasoning for this value? Do we know which devices need ECAM areas?

Yes, Xen is limited to bus 0 emulation currently, the description states "When multiple PCI buses support for Xen will be implemented, PCI_MAX_MCFG_BUSES may be changed to calculation of the number of buses according to results of the PCI devices enumeration".

Quoted 8 lines

I think it might be better to replace 'switch (PCI_MAX_MCFG_BUSES)' with the real code right away, i.e. change it to

'switch (max_bus_num, aligned up to 64/128/256 boundary)', where max_bus_num should be set in PCI device enumeration code in pci_setup(). As we are limited to bus 0 currently, we'll just set it to 0 for now, before/after the PCI device enumeration loop (which should became multi-bus capable eventually).

I guess this is all pretty much hardcoded to bus 0 in several places, so I'm not sure it's worth to add PCI_MAX_MCFG_BUSES. IMO if something like this should be added it should be PCI_MAX_BUSES, and several places should be changed to make use of it. Or ideally we should find a way to detect this at runtime, without needed any hardcoded defines.

I think it would be good if you can add a note comment describing the different MCFG sizes supported by the Q35 chipset (64/128/256).

tools/firmware/hvmloader/config.h extern uint8_t ioapic_version; · line 62

Quoted 2 lines

tools/firmware/hvmloader/config.h +2 -0

+/* possible values are: 64, 128, 256 */
+#define PCI_MAX_MCFG_BUSES  64    

Quoted 34 lines · On Tue, 20 Mar 2018 08:50:48 +0000 Roger Pau Monné <[email protected]> wrote:

Quoted 33 lines · Alexey G

Quoted 30 lines · On Mon, 19 Mar 2018 15:58:02 +0000 Roger Pau Monné <[email protected]> wrote:

Quoted 13 lines · Alexey Gerasimenko

Much like normal PCI BARs or other chipset-specific memory-mapped resources, MMCONFIG area needs space in MMIO hole, so we must allocate it manually.

The actual MMCONFIG size depends on a number of PCI buses available which should be covered by ECAM. Possible options are 64MB, 128MB and 256MB. As we are limited to the bus 0 currently, thus using lowest possible setting (64MB), #defined via PCI_MAX_MCFG_BUSES in hvmloader/config.h. When multiple PCI buses support for Xen will be implemented, PCI_MAX_MCFG_BUSES may be changed to calculation of the number of buses according to results of the PCI devices enumeration.

Quoted 4 lines

The way to allocate MMCONFIG range in MMIO hole is similar to how other PCI BARs are allocated. The patch extends 'bars' structure to make it universal for any arbitrary BAR type -- either IO, MMIO, ROM or a chipset-specific resource.

I'm not sure this is fully correct. The IOREQ interface can differentiate PCI devices and forward config space accesses to different emulators (see IOREQ_TYPE_PCI_CONFIG). With this change you will forward all MCFG accesses to QEMU, which will likely be wrong if there are multiple PCI-device emulators for the same domain.

Ie: AFAICT Xen needs to know about the MCFG emulation and detect accesses to it in order to forward them to the right emulators.

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

Adding Paul who knows more about all this.

In which use cases multiple PCI-device emulators are used for a single HVM domain? Is it a proprietary setup?

Likely. I think XenGT might be using it. It's a feature of the IOREQ implementation in Xen.

According to public slides for the feature, both PCI conf and MMIO accesses can be routed to the designated device model. It looks like for this particular setup it doesn't really matter which particular ioreq type must be used for MMCONFIG accesses -- either IOREQ_TYPE_PCI_CONFIG or IOREQ_TYPE_COPY (MMIO accesses) should be acceptable.

The only thing which matters is ioreq routing itself -- making decisions to which device model the PCI conf/MMIO ioreq should be sent.

Quoted 1 line

Traditional PCI config space accesses are not IO port space accesses.

(assuming 'not' mistyped here)

Quoted 3 lines

The IOREQ code in Xen detects accesses to ports 0xcf8/0xcfc and IOREQ servers can register devices they would like to receive configuration space accesses for. QEMU is already making use of this, see for

That's one of the reasons why current IOREQ_TYPE_PCI_CONFIG implementation is a bit inconvenient for MMCONFIG MMIO accesses -- it's too much CF8h/CFCh-centric in its implementation, might be painful to change something in the code which was intended for CF8h/CFCh handling (and not for MMIO processing).

Quoted 6 lines

example xen_map_pcidev in the QEMU code.

By treating MCFG accesses as MMIO you are bypassing the IOREQ PCI layer, and thus a IOREQ server could register a PCI device and only receive PCI configuration accesses from the IO port space, while MCFG accesses would be forwarded somewhere else.

It will be handled by IOREQ too, just using a different IOREQ type (MMIO one). The basic question is why do we have to stick to PCI conf space ioreqs for emulating MMIO accesses to MMCONFIG.

Quoted 3 lines

I think you need to make the IOREQ code aware of the MCFG area and XEN_DMOP_IO_RANGE_PCI needs to forward both IO space and MCFG accesses to the right IOREQ server.

Right now there is no way to inform Xen where the emulated MMCONFIG area is located in order to make this decision, based on the address within MMCONFIG range. A new dmop/hypercall is needed (with args similar to pci_mmcfg_reserved) along with its usage in QEMU.

I'll try to summarize two different approaches to MMCONFIG handling. For both approaches the final PCI config host interface for a passed through device in QEMU will remain same as at the moment -- xen_host_pci_* functions in /hw/xen.

Approach #1. Informing Xen about MMCONFIG area changes and letting Xen to translate MMIO accesses to _PCI_CONFIG ioreqs:

1. QEMU will trap accesses to PCIEXBAR, calling Xen via dmop/hypercall to let the latter know of any MMCONFIG area address/size/status changes

2. Xen will trap MMIO accesses to the current MMCONFIG location and convert memory accesses into one or several _PCI_CONFIG ioreqs and send them to a chosen device model

3. QEMU will receive _PCI_CONFIG ioreqs with SBDF and 12-bit offsets inside which it needs to somehow pass to pci_host_config_{read,write}_common() for emulation. It might require few hacks to make the gears turn (due to QEMU pci conf read/write model).
At the moment emulated CF8h/CFCh ports play a special role in all this -- xen-hvm.c writes an AMD-style value to the emulated CF8h port "so that the config space access will target the correct device model" (quote). Not sure about this and why it's is needed if Xen actually makes the decision to which DM the PCI conf ioreq should be sent.

One minor note: these new 'set_mmconfig_' dmops/hypercalls have to be triggered inside the chipset-specific emulation code in QEMU (PCIEXBAR handling in Q35 case). If there will be another machine which needs to emulate MMCONFIG control differently -- we have no choice but to insert these dmops/hypercalls into another chipset-specific emulation code as well, eg. inside HECBASE emulation code.

Approach #2. Handling MMCONFIG area inside QEMU using usual MMIO emulation:

1. QEMU will trap accesses to PCIEXBAR (or whatever else possibly supported in the future like HECBASE), eventually asking Xen to map the MMCONFIG MMIO range for ioreq servicing just like it does for any other emulated MMIO range, via map_io_range_to_ioreq_server(). All changes in MMCONFIG placement/status will lead to remapping/unmapping the MMIO range.

2. Xen will trap MMIO accesses to this area and forward them to QEMU as MMIO (IOREQ_TYPE_COPY) ioreqs

3. QEMU will receive these accesses and pass them to the existing MMCONFIG emulation -- pcie_mmcfg_data_read/write handlers, finally resulting in same xen_host_pci_* function calls as before.

This approach works "right out of the box", no changes needed for either Xen or QEMU. As both _PCI_CONFIG and MMIO type ioreqs are processed, either method can be used to access PCI/extended config space -- CF8/CFC port I/O or MMIO accesses to MMCONFIG.

IOREQ routing for multiple device emulators can be supported too. In fact, the same mmconfig dmops/hypercalls can be added to let Xen know where MMCONFIG area resides, Xen will use this information to forward MMCONFIG MMIO ioreqs accordingly to BDF of the address. The difference with the approach #1 is that these interfaces are now completely optional when we use MMIO ioreqs for MMCONFIG on vanilla Xen/QEMU.

The question is why IOREQ_TYPE_COPY -> IOREQ_TYPE_PCI_CONFIG translation is a must have thing at all? It won't make handling simpler. For current QEMU implementation IOREQ_TYPE_COPY (MMIO accesses for MMCONFIG) would be preferable as it allows to use the existing code.

I think it will be safe to use MMCONFIG emulation on MMIO level for now and later extend it with 'set_mmconfig_' dmop/hypercall for the 'multiple device emulators' IOREQ_TYPE_COPY routing to work same as for PCI conf, so it can be used by XenGT etc on Q35 as well.

-----Original Message-----

Quoted 4 lines

The question is why IOREQ_TYPE_COPY -> IOREQ_TYPE_PCI_CONFIG translation is a must have thing at all? It won't make handling simpler. For current QEMU implementation IOREQ_TYPE_COPY (MMIO accesses for MMCONFIG) would be preferable as it allows to use the existing code.

Granted it's likely easier to implement, but it's also incorrect. You seem to have in mind the picture of a single IOREQ server (QEMU) handling all the devices.

Although this is the most common scenario, it's not the only one supported by Xen. Your proposed solution breaks the usage of multiple IOREQ servers as PCI device emulators.

Quoted 4 lines

I think it will be safe to use MMCONFIG emulation on MMIO level for now and later extend it with 'set_mmconfig_' dmop/hypercall for the 'multiple device emulators' IOREQ_TYPE_COPY routing to work same as for PCI conf, so it can be used by XenGT etc on Q35 as well.

I'm afraid this kind of issues would have been fairly easier to identify if a design document for this feature was sent to the list prior to it's implementation.

Regarding whether to accept something like this, I'm not really in favor, but IMO it depends on how much new code is added to handle this incorrect usage that would then go away (or would have to be changed) in order to handle the proper implementation.

Thanks, Roger.

Quoted 4 lines

Although this is the most common scenario, it's not the only one supported by Xen. Your proposed solution breaks the usage of multiple IOREQ servers as PCI device emulators.

Indeed it will, and that is not acceptable even in the short term.

Quoted 5 lines

Quoted 5 lines

I think it will be safe to use MMCONFIG emulation on MMIO level for now and later extend it with 'set_mmconfig_' dmop/hypercall for the 'multiple device emulators' IOREQ_TYPE_COPY routing to work same as for PCI conf, so it can be used by XenGT etc on Q35 as well.

Introducing known breakage is not really on, particularly when it can be avoided with a reasonable amount of extra work.

-----Original Message-----
From: Alexey G [mailto:[email protected]]
Sent: 21 March 2018 14:35
To: Paul Durrant <[email protected]>
Cc: Roger Pau Monne <[email protected]>; xen- [email protected]; Andrew Cooper <[email protected]>; Ian Jackson <[email protected]>; Jan Beulich <[email protected]>; Wei Liu <[email protected]>
Subject: Re: [Xen-devel] [RFC PATCH 07/12] hvmloader: allocate MMCONFIG area in the MMIO hole + minor code refactoring

Quoted 6 lines · On Wed, 21 Mar 2018 09:36:04 +0000 Paul Durrant <[email protected]> wrote:

Quoted 4 lines

Although this is the most common scenario, it's not the only one supported by Xen. Your proposed solution breaks the usage of multiple IOREQ servers as PCI device emulators.

Indeed it will, and that is not acceptable even in the short term.

Hmm, what exactly you are rejecting? QEMU's usage of established (and provided by Xen) interfaces for QEMU to use? Any particular reason why QEMU can use map_io_range_to_ioreq_server() in one case and can't in another? It's API available for QEMU after all.

If we actually switch to the emulated MMCONFIG range informing approach for Xen (via a new dmop/hypercall), who should prevent QEMU to actually map this range via map_io_range_to_ioreq_server? QEMU itself? Or Xen?

How to will look, "QEMU asks us to map this range as emulated MMIO, but he previously told us that emulated PCIEXBAR register points there, so we won't allow him to do it"?

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

I think it will be safe to use MMCONFIG emulation on MMIO level for now and later extend it with 'set_mmconfig_' dmop/hypercall for the 'multiple device emulators' IOREQ_TYPE_COPY routing to work same as for PCI conf, so it can be used by XenGT etc on Q35 as well.

Quoted 2 lines

Introducing known breakage is not really on, particularly when it can be avoided with a reasonable amount of extra work.

It's hard to break something which doesn't exist. :) Multiple device emulators feature do not support translation/routing of MMCONFIG MMIO accesses currently, it must be designed first.

On Wed, Mar 21, 2018 at 10:58:40AM +1000, Alexey G wrote:

Quoted 6 lines

According to public slides for the feature, both PCI conf and MMIO accesses can be routed to the designated device model. It looks like for this particular setup it doesn't really matter which particular ioreq type must be used for MMCONFIG accesses -- either IOREQ_TYPE_PCI_CONFIG or IOREQ_TYPE_COPY (MMIO accesses) should be acceptable.

Isn't that going to be quite messy? How is the IOREQ server supposed to decode a MCFG access received as IOREQ_TYPE_COPY?

I don't think the IOREQ server needs to know the start of the MCFG region, in which case it won't be able to detect and decode the access if it's of type IOREQ_TYPE_COPY.

MCFG accesses need to be sent to the IOREQ server as IOREQ_TYPE_PCI_CONFIG, or else you are forcing each IOREQ server to know the position of the MCFG area in order to do the decoding. In your case this would work because QEMU controls the position of the MCFG region, but there's no need for other IOREQ servers to know the position of the MCFG area.

Quoted 3 lines

The only thing which matters is ioreq routing itself -- making decisions to which device model the PCI conf/MMIO ioreq should be sent.

Hm, see above, but I'm fairly sure you need to forward those MCFG accesses as IOREQ_TYPE_PCI_CONFIG to the IOREQ server.

Quoted 4 lines

Quoted 2 lines

Traditional PCI config space accesses are not IO port space accesses.

(assuming 'not' mistyped here)

Not really, this should instead be:

"Traditional PCI config space accesses are not forwarded to the IOREQ server as IO port space accesses (IOREQ_TYPE_PIO) but rather as PCI config space accesses (IOREQ_TYPE_PCI_CONFIG)."

Sorry for the confusion.

Quoted 10 lines

Quoted 4 lines

The IOREQ code in Xen detects accesses to ports 0xcf8/0xcfc and IOREQ servers can register devices they would like to receive configuration space accesses for. QEMU is already making use of this, see for

That's one of the reasons why current IOREQ_TYPE_PCI_CONFIG implementation is a bit inconvenient for MMCONFIG MMIO accesses -- it's too much CF8h/CFCh-centric in its implementation, might be painful to change something in the code which was intended for CF8h/CFCh handling (and not for MMIO processing).

I'm not sure I follow. Do you mean that changes should be made to the ioreq struct in order to forward MCFG accesses using IOREQ_TYPE_PCI_CONFIG as it's type?

Quoted 3 lines

It will be handled by IOREQ too, just using a different IOREQ type (MMIO one). The basic question is why do we have to stick to PCI conf space ioreqs for emulating MMIO accesses to MMCONFIG.