[PATCH v9] xen/pt: reserve PCI slot 2 for Intel igd-passthru

[PATCH v9] xen/pt: reserve PCI slot 2 for Intel igd-passthru

[Chuck Zmudzinski]

Intel specifies that the Intel IGD must occupy slot 2 on the PCI bus,
as noted in docs/igd-assign.txt in the Qemu source code.

Currently, when the xl toolstack is used to configure a Xen HVM guest with
Intel IGD passthrough to the guest with the Qemu upstream device model,
a Qemu emulated PCI device will occupy slot 2 and the Intel IGD will occupy
a different slot. This problem often prevents the guest from booting.

The only available workaround is not good: Configure Xen HVM guests to use
the old and no longer maintained Qemu traditional device model available
from xenbits.xen.org which does reserve slot 2 for the Intel IGD.

To implement this feature in the Qemu upstream device model for Xen HVM
guests, introduce the following new functions, types, and macros:

* XEN_PT_DEVICE_CLASS declaration, based on the existing TYPE_XEN_PT_DEVICE
* XEN_PT_DEVICE_GET_CLASS macro helper function for XEN_PT_DEVICE_CLASS
* typedef XenPTQdevRealize function pointer
* XEN_PCI_IGD_SLOT_MASK, the value of slot_reserved_mask to reserve slot 2
* xen_igd_reserve_slot and xen_igd_clear_slot functions

Michael Tsirkin:
* Introduce XEN_PCI_IGD_DOMAIN, XEN_PCI_IGD_BUS, XEN_PCI_IGD_DEV, and
  XEN_PCI_IGD_FN - use them to compute the value of XEN_PCI_IGD_SLOT_MASK

The new xen_igd_reserve_slot function uses the existing slot_reserved_mask
member of PCIBus to reserve PCI slot 2 for Xen HVM guests configured using
the xl toolstack with the gfx_passthru option enabled, which sets the
igd-passthru=on option to Qemu for the Xen HVM machine type.

The new xen_igd_reserve_slot function also needs to be implemented in
hw/xen/xen_pt_stub.c to prevent FTBFS during the link stage for the case
when Qemu is configured with --enable-xen and --disable-xen-pci-passthrough,
in which case it does nothing.

The new xen_igd_clear_slot function overrides qdev->realize of the parent
PCI device class to enable the Intel IGD to occupy slot 2 on the PCI bus
since slot 2 was reserved by xen_igd_reserve_slot when the PCI bus was
created in hw/i386/pc_piix.c for the case when igd-passthru=on.

Move the call to xen_host_pci_device_get, and the associated error
handling, from xen_pt_realize to the new xen_igd_clear_slot function to
initialize the device class and vendor values which enables the checks for
the Intel IGD to succeed. The verification that the host device is an
Intel IGD to be passed through is done by checking the domain, bus, slot,
and function values as well as by checking that gfx_passthru is enabled,
the device class is VGA, and the device vendor in Intel.

Signed-off-by: Chuck Zmudzinski <brchuckz@aol.com>
---
Notes that might be helpful to reviewers of patched code in hw/xen:

The new functions and types are based on recommendations from Qemu docs:
https://qemu.readthedocs.io/en/latest/devel/qom.html

Notes that might be helpful to reviewers of patched code in hw/i386:

The small patch to hw/i386/pc_piix.c is protected by CONFIG_XEN so it does
not affect builds that do not have CONFIG_XEN defined.

xen_igd_gfx_pt_enabled() in the patched hw/i386/pc_piix.c file is an
existing function that is only true when Qemu is built with
xen-pci-passthrough enabled and the administrator has configured the Xen
HVM guest with Qemu's igd-passthru=on option.

v2: Remove From: <email address> tag at top of commit message

v3: Changed the test for the Intel IGD in xen_igd_clear_slot:

    if (is_igd_vga_passthrough(&s->real_device) &&
        (s->real_device.vendor_id == PCI_VENDOR_ID_INTEL)) {

    is changed to

    if (xen_igd_gfx_pt_enabled() && (s->hostaddr.slot == 2)
        && (s->hostaddr.function == 0)) {

    I hoped that I could use the test in v2, since it matches the
    other tests for the Intel IGD in Qemu and Xen, but those tests
    do not work because the necessary data structures are not set with
    their values yet. So instead use the test that the administrator
    has enabled gfx_passthru and the device address on the host is
    02.0. This test does detect the Intel IGD correctly.

v4: Use brchuckz@aol.com instead of brchuckz@netscape.net for the author's
    email address to match the address used by the same author in commits
    be9c61da and c0e86b76
    
    Change variable for XEN_PT_DEVICE_CLASS: xptc changed to xpdc

v5: The patch of xen_pt.c was re-worked to allow a more consistent test
    for the Intel IGD that uses the same criteria as in other places.
    This involved moving the call to xen_host_pci_device_get from
    xen_pt_realize to xen_igd_clear_slot and updating the checks for the
    Intel IGD in xen_igd_clear_slot:
    
    if (xen_igd_gfx_pt_enabled() && (s->hostaddr.slot == 2)
        && (s->hostaddr.function == 0)) {

    is changed to

    if (is_igd_vga_passthrough(&s->real_device) &&
        s->real_device.domain == 0 && s->real_device.bus == 0 &&
        s->real_device.dev == 2 && s->real_device.func == 0 &&
        s->real_device.vendor_id == PCI_VENDOR_ID_INTEL) {

    Added an explanation for the move of xen_host_pci_device_get from
    xen_pt_realize to xen_igd_clear_slot to the commit message.

    Rebase.

v6: Fix logging by removing these lines from the move from xen_pt_realize
    to xen_igd_clear_slot that was done in v5:

    XEN_PT_LOG(d, "Assigning real physical device %02x:%02x.%d"
               " to devfn 0x%x\n",
               s->hostaddr.bus, s->hostaddr.slot, s->hostaddr.function,
               s->dev.devfn);

    This log needs to be in xen_pt_realize because s->dev.devfn is not
    set yet in xen_igd_clear_slot.

v7: The v7 that was posted to the mailing list was incorrect. v8 is what
    v7 was intended to be.

v8: Inhibit out of context log message and needless processing by
    adding 2 lines at the top of the new xen_igd_clear_slot function:

    if (!(pci_bus->slot_reserved_mask & XEN_PCI_IGD_SLOT_MASK))
        return;

    Rebase. This removed an unnecessary header file from xen_pt.h 

v9: Move check for xen_igd_gfx_pt_enabled() from pc_piix.c to xen_pt.c

    Move #include "hw/pci/pci_bus.h" from xen_pt.h to xen_pt.c

    Introduce macros for the IGD devfn constants and use them to compute
    the value of XEN_PCI_IGD_SLOT_MASK

    Also use the new macros at an appropriate place in xen_pt_realize

    Add Cc: to stable - This has been broken for a long time, ever since
                        support for igd-passthru was added to Qemu 7
                        years ago.

    Mention new macros in the commit message (Michael Tsirkin)

    N.B.: I could not follow the suggestion to move the statement
    pci_bus->slot_reserved_mask &= ~XEN_PCI_IGD_SLOT_MASK; to after
    pci_qdev_realize for symmetry. Doing that results in an error when
    creating the guest:
    
    libxl: error: libxl_qmp.c:1837:qmp_ev_parse_error_messages: Domain 4:PCI: slot 2 function 0 not available for xen-pci-passthrough, reserved
    libxl: error: libxl_pci.c:1809:device_pci_add_done: Domain 4:libxl__device_pci_add failed for PCI device 0:0:2.0 (rc -28)
    libxl: error: libxl_create.c:1921:domcreate_attach_devices: Domain 4:unable to add pci devices

 hw/i386/pc_piix.c    |  1 +
 hw/xen/xen_pt.c      | 61 ++++++++++++++++++++++++++++++++++++--------
 hw/xen/xen_pt.h      | 20 +++++++++++++++
 hw/xen/xen_pt_stub.c |  4 +++
 4 files changed, 75 insertions(+), 11 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index b48047f50c..8fc96eb63b 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -405,6 +405,7 @@ static void pc_xen_hvm_init(MachineState *machine)
     }
 
     pc_xen_hvm_init_pci(machine);
+    xen_igd_reserve_slot(pcms->bus);
     pci_create_simple(pcms->bus, -1, "xen-platform");
 }
 #endif
diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index 0ec7e52183..51f100f64a 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -57,6 +57,7 @@
 #include <sys/ioctl.h>
 
 #include "hw/pci/pci.h"
+#include "hw/pci/pci_bus.h"
 #include "hw/qdev-properties.h"
 #include "hw/qdev-properties-system.h"
 #include "hw/xen/xen.h"
@@ -780,15 +781,6 @@ static void xen_pt_realize(PCIDevice *d, Error **errp)
                s->hostaddr.bus, s->hostaddr.slot, s->hostaddr.function,
                s->dev.devfn);
 
-    xen_host_pci_device_get(&s->real_device,
-                            s->hostaddr.domain, s->hostaddr.bus,
-                            s->hostaddr.slot, s->hostaddr.function,
-                            errp);
-    if (*errp) {
-        error_append_hint(errp, "Failed to \"open\" the real pci device");
-        return;
-    }
-
     s->is_virtfn = s->real_device.is_virtfn;
     if (s->is_virtfn) {
         XEN_PT_LOG(d, "%04x:%02x:%02x.%d is a SR-IOV Virtual Function\n",
@@ -803,8 +795,10 @@ static void xen_pt_realize(PCIDevice *d, Error **errp)
     s->io_listener = xen_pt_io_listener;
 
     /* Setup VGA bios for passthrough GFX */
-    if ((s->real_device.domain == 0) && (s->real_device.bus == 0) &&
-        (s->real_device.dev == 2) && (s->real_device.func == 0)) {
+    if ((s->real_device.domain == XEN_PCI_IGD_DOMAIN) &&
+        (s->real_device.bus == XEN_PCI_IGD_BUS) &&
+        (s->real_device.dev == XEN_PCI_IGD_DEV) &&
+        (s->real_device.func == XEN_PCI_IGD_FN)) {
         if (!is_igd_vga_passthrough(&s->real_device)) {
             error_setg(errp, "Need to enable igd-passthru if you're trying"
                     " to passthrough IGD GFX");
@@ -950,11 +944,55 @@ static void xen_pci_passthrough_instance_init(Object *obj)
     PCI_DEVICE(obj)->cap_present |= QEMU_PCI_CAP_EXPRESS;
 }
 
+void xen_igd_reserve_slot(PCIBus *pci_bus)
+{
+    if (!xen_igd_gfx_pt_enabled())
+        return;
+
+    XEN_PT_LOG(0, "Reserving PCI slot 2 for IGD\n");
+    pci_bus->slot_reserved_mask |= XEN_PCI_IGD_SLOT_MASK;
+}
+
+static void xen_igd_clear_slot(DeviceState *qdev, Error **errp)
+{
+    ERRP_GUARD();
+    PCIDevice *pci_dev = (PCIDevice *)qdev;
+    XenPCIPassthroughState *s = XEN_PT_DEVICE(pci_dev);
+    XenPTDeviceClass *xpdc = XEN_PT_DEVICE_GET_CLASS(s);
+    PCIBus *pci_bus = pci_get_bus(pci_dev);
+
+    if (!(pci_bus->slot_reserved_mask & XEN_PCI_IGD_SLOT_MASK))
+        return;
+
+    xen_host_pci_device_get(&s->real_device,
+                            s->hostaddr.domain, s->hostaddr.bus,
+                            s->hostaddr.slot, s->hostaddr.function,
+                            errp);
+    if (*errp) {
+        error_append_hint(errp, "Failed to \"open\" the real pci device");
+        return;
+    }
+
+    if (is_igd_vga_passthrough(&s->real_device) &&
+        s->real_device.domain == XEN_PCI_IGD_DOMAIN &&
+        s->real_device.bus == XEN_PCI_IGD_BUS &&
+        s->real_device.dev == XEN_PCI_IGD_DEV &&
+        s->real_device.func == XEN_PCI_IGD_FN &&
+        s->real_device.vendor_id == PCI_VENDOR_ID_INTEL) {
+        pci_bus->slot_reserved_mask &= ~XEN_PCI_IGD_SLOT_MASK;
+        XEN_PT_LOG(pci_dev, "Intel IGD found, using slot 2\n");
+    }
+    xpdc->pci_qdev_realize(qdev, errp);
+}
+
 static void xen_pci_passthrough_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
+    XenPTDeviceClass *xpdc = XEN_PT_DEVICE_CLASS(klass);
+    xpdc->pci_qdev_realize = dc->realize;
+    dc->realize = xen_igd_clear_slot;
     k->realize = xen_pt_realize;
     k->exit = xen_pt_unregister_device;
     k->config_read = xen_pt_pci_read_config;
@@ -977,6 +1015,7 @@ static const TypeInfo xen_pci_passthrough_info = {
     .instance_size = sizeof(XenPCIPassthroughState),
     .instance_finalize = xen_pci_passthrough_finalize,
     .class_init = xen_pci_passthrough_class_init,
+    .class_size = sizeof(XenPTDeviceClass),
     .instance_init = xen_pci_passthrough_instance_init,
     .interfaces = (InterfaceInfo[]) {
         { INTERFACE_CONVENTIONAL_PCI_DEVICE },
diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
index cf10fc7bbf..e184699740 100644
--- a/hw/xen/xen_pt.h
+++ b/hw/xen/xen_pt.h
@@ -40,7 +40,20 @@ typedef struct XenPTReg XenPTReg;
 #define TYPE_XEN_PT_DEVICE "xen-pci-passthrough"
 OBJECT_DECLARE_SIMPLE_TYPE(XenPCIPassthroughState, XEN_PT_DEVICE)
 
+#define XEN_PT_DEVICE_CLASS(klass) \
+    OBJECT_CLASS_CHECK(XenPTDeviceClass, klass, TYPE_XEN_PT_DEVICE)
+#define XEN_PT_DEVICE_GET_CLASS(obj) \
+    OBJECT_GET_CLASS(XenPTDeviceClass, obj, TYPE_XEN_PT_DEVICE)
+
+typedef void (*XenPTQdevRealize)(DeviceState *qdev, Error **errp);
+
+typedef struct XenPTDeviceClass {
+    PCIDeviceClass parent_class;
+    XenPTQdevRealize pci_qdev_realize;
+} XenPTDeviceClass;
+
 uint32_t igd_read_opregion(XenPCIPassthroughState *s);
+void xen_igd_reserve_slot(PCIBus *pci_bus);
 void igd_write_opregion(XenPCIPassthroughState *s, uint32_t val);
 void xen_igd_passthrough_isa_bridge_create(XenPCIPassthroughState *s,
                                            XenHostPCIDevice *dev);
@@ -75,6 +88,13 @@ typedef int (*xen_pt_conf_byte_read)
 
 #define XEN_PCI_INTEL_OPREGION 0xfc
 
+#define XEN_PCI_IGD_DOMAIN 0
+#define XEN_PCI_IGD_BUS 0
+#define XEN_PCI_IGD_DEV 2
+#define XEN_PCI_IGD_FN 0
+#define XEN_PCI_IGD_SLOT_MASK \
+    (1UL << PCI_SLOT(PCI_DEVFN(XEN_PCI_IGD_DEV, XEN_PCI_IGD_FN)))
+
 typedef enum {
     XEN_PT_GRP_TYPE_HARDWIRED = 0,  /* 0 Hardwired reg group */
     XEN_PT_GRP_TYPE_EMU,            /* emul reg group */
diff --git a/hw/xen/xen_pt_stub.c b/hw/xen/xen_pt_stub.c
index 2d8cac8d54..5c108446a8 100644
--- a/hw/xen/xen_pt_stub.c
+++ b/hw/xen/xen_pt_stub.c
@@ -20,3 +20,7 @@ void xen_igd_gfx_pt_set(bool value, Error **errp)
         error_setg(errp, "Xen PCI passthrough support not built in");
     }
 }
+
+void xen_igd_reserve_slot(PCIBus *pci_bus)
+{
+}
-- 
2.39.0

Re: [PATCH v9] xen/pt: reserve PCI slot 2 for Intel igd-passthru

[Stefano Stabellini]

On Sat, 14 Jan 2023, Chuck Zmudzinski wrote:
> Intel specifies that the Intel IGD must occupy slot 2 on the PCI bus,
> as noted in docs/igd-assign.txt in the Qemu source code.
> 
> Currently, when the xl toolstack is used to configure a Xen HVM guest with
> Intel IGD passthrough to the guest with the Qemu upstream device model,
> a Qemu emulated PCI device will occupy slot 2 and the Intel IGD will occupy
> a different slot. This problem often prevents the guest from booting.
> 
> The only available workaround is not good: Configure Xen HVM guests to use
> the old and no longer maintained Qemu traditional device model available
> from xenbits.xen.org which does reserve slot 2 for the Intel IGD.
> 
> To implement this feature in the Qemu upstream device model for Xen HVM
> guests, introduce the following new functions, types, and macros:
> 
> * XEN_PT_DEVICE_CLASS declaration, based on the existing TYPE_XEN_PT_DEVICE
> * XEN_PT_DEVICE_GET_CLASS macro helper function for XEN_PT_DEVICE_CLASS
> * typedef XenPTQdevRealize function pointer
> * XEN_PCI_IGD_SLOT_MASK, the value of slot_reserved_mask to reserve slot 2
> * xen_igd_reserve_slot and xen_igd_clear_slot functions
> 
> Michael Tsirkin:
> * Introduce XEN_PCI_IGD_DOMAIN, XEN_PCI_IGD_BUS, XEN_PCI_IGD_DEV, and
>   XEN_PCI_IGD_FN - use them to compute the value of XEN_PCI_IGD_SLOT_MASK
> 
> The new xen_igd_reserve_slot function uses the existing slot_reserved_mask
> member of PCIBus to reserve PCI slot 2 for Xen HVM guests configured using
> the xl toolstack with the gfx_passthru option enabled, which sets the
> igd-passthru=on option to Qemu for the Xen HVM machine type.
> 
> The new xen_igd_reserve_slot function also needs to be implemented in
> hw/xen/xen_pt_stub.c to prevent FTBFS during the link stage for the case
> when Qemu is configured with --enable-xen and --disable-xen-pci-passthrough,
> in which case it does nothing.
> 
> The new xen_igd_clear_slot function overrides qdev->realize of the parent
> PCI device class to enable the Intel IGD to occupy slot 2 on the PCI bus
> since slot 2 was reserved by xen_igd_reserve_slot when the PCI bus was
> created in hw/i386/pc_piix.c for the case when igd-passthru=on.
> 
> Move the call to xen_host_pci_device_get, and the associated error
> handling, from xen_pt_realize to the new xen_igd_clear_slot function to
> initialize the device class and vendor values which enables the checks for
> the Intel IGD to succeed. The verification that the host device is an
> Intel IGD to be passed through is done by checking the domain, bus, slot,
> and function values as well as by checking that gfx_passthru is enabled,
> the device class is VGA, and the device vendor in Intel.
> 
> Signed-off-by: Chuck Zmudzinski <brchuckz@aol.com>

Hi Chuck,

The approach looks OK in principle to me. I only have one question: for
other PCI devices (not Intel IGD), where is xen_host_pci_device_get
called now?

It looks like that xen_igd_reserve_slot would return without setting
slot_reserved_mask, hence xen_igd_clear_slot would also return without
calling xen_host_pci_device_get. And xen_pt_realize doesn't call
xen_host_pci_device_get any longer.

Am I missing something?


> ---
> Notes that might be helpful to reviewers of patched code in hw/xen:
> 
> The new functions and types are based on recommendations from Qemu docs:
> https://qemu.readthedocs.io/en/latest/devel/qom.html
> 
> Notes that might be helpful to reviewers of patched code in hw/i386:
> 
> The small patch to hw/i386/pc_piix.c is protected by CONFIG_XEN so it does
> not affect builds that do not have CONFIG_XEN defined.
> 
> xen_igd_gfx_pt_enabled() in the patched hw/i386/pc_piix.c file is an
> existing function that is only true when Qemu is built with
> xen-pci-passthrough enabled and the administrator has configured the Xen
> HVM guest with Qemu's igd-passthru=on option.
> 
> v2: Remove From: <email address> tag at top of commit message
> 
> v3: Changed the test for the Intel IGD in xen_igd_clear_slot:
> 
>     if (is_igd_vga_passthrough(&s->real_device) &&
>         (s->real_device.vendor_id == PCI_VENDOR_ID_INTEL)) {
> 
>     is changed to
> 
>     if (xen_igd_gfx_pt_enabled() && (s->hostaddr.slot == 2)
>         && (s->hostaddr.function == 0)) {
> 
>     I hoped that I could use the test in v2, since it matches the
>     other tests for the Intel IGD in Qemu and Xen, but those tests
>     do not work because the necessary data structures are not set with
>     their values yet. So instead use the test that the administrator
>     has enabled gfx_passthru and the device address on the host is
>     02.0. This test does detect the Intel IGD correctly.
> 
> v4: Use brchuckz@aol.com instead of brchuckz@netscape.net for the author's
>     email address to match the address used by the same author in commits
>     be9c61da and c0e86b76
>     
>     Change variable for XEN_PT_DEVICE_CLASS: xptc changed to xpdc
> 
> v5: The patch of xen_pt.c was re-worked to allow a more consistent test
>     for the Intel IGD that uses the same criteria as in other places.
>     This involved moving the call to xen_host_pci_device_get from
>     xen_pt_realize to xen_igd_clear_slot and updating the checks for the
>     Intel IGD in xen_igd_clear_slot:
>     
>     if (xen_igd_gfx_pt_enabled() && (s->hostaddr.slot == 2)
>         && (s->hostaddr.function == 0)) {
> 
>     is changed to
> 
>     if (is_igd_vga_passthrough(&s->real_device) &&
>         s->real_device.domain == 0 && s->real_device.bus == 0 &&
>         s->real_device.dev == 2 && s->real_device.func == 0 &&
>         s->real_device.vendor_id == PCI_VENDOR_ID_INTEL) {
> 
>     Added an explanation for the move of xen_host_pci_device_get from
>     xen_pt_realize to xen_igd_clear_slot to the commit message.
> 
>     Rebase.
> 
> v6: Fix logging by removing these lines from the move from xen_pt_realize
>     to xen_igd_clear_slot that was done in v5:
> 
>     XEN_PT_LOG(d, "Assigning real physical device %02x:%02x.%d"
>                " to devfn 0x%x\n",
>                s->hostaddr.bus, s->hostaddr.slot, s->hostaddr.function,
>                s->dev.devfn);
> 
>     This log needs to be in xen_pt_realize because s->dev.devfn is not
>     set yet in xen_igd_clear_slot.
> 
> v7: The v7 that was posted to the mailing list was incorrect. v8 is what
>     v7 was intended to be.
> 
> v8: Inhibit out of context log message and needless processing by
>     adding 2 lines at the top of the new xen_igd_clear_slot function:
> 
>     if (!(pci_bus->slot_reserved_mask & XEN_PCI_IGD_SLOT_MASK))
>         return;
> 
>     Rebase. This removed an unnecessary header file from xen_pt.h 
> 
> v9: Move check for xen_igd_gfx_pt_enabled() from pc_piix.c to xen_pt.c
> 
>     Move #include "hw/pci/pci_bus.h" from xen_pt.h to xen_pt.c
> 
>     Introduce macros for the IGD devfn constants and use them to compute
>     the value of XEN_PCI_IGD_SLOT_MASK
> 
>     Also use the new macros at an appropriate place in xen_pt_realize
> 
>     Add Cc: to stable - This has been broken for a long time, ever since
>                         support for igd-passthru was added to Qemu 7
>                         years ago.
> 
>     Mention new macros in the commit message (Michael Tsirkin)
> 
>     N.B.: I could not follow the suggestion to move the statement
>     pci_bus->slot_reserved_mask &= ~XEN_PCI_IGD_SLOT_MASK; to after
>     pci_qdev_realize for symmetry. Doing that results in an error when
>     creating the guest:
>     
>     libxl: error: libxl_qmp.c:1837:qmp_ev_parse_error_messages: Domain 4:PCI: slot 2 function 0 not available for xen-pci-passthrough, reserved
>     libxl: error: libxl_pci.c:1809:device_pci_add_done: Domain 4:libxl__device_pci_add failed for PCI device 0:0:2.0 (rc -28)
>     libxl: error: libxl_create.c:1921:domcreate_attach_devices: Domain 4:unable to add pci devices
> 
>  hw/i386/pc_piix.c    |  1 +
>  hw/xen/xen_pt.c      | 61 ++++++++++++++++++++++++++++++++++++--------
>  hw/xen/xen_pt.h      | 20 +++++++++++++++
>  hw/xen/xen_pt_stub.c |  4 +++
>  4 files changed, 75 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index b48047f50c..8fc96eb63b 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -405,6 +405,7 @@ static void pc_xen_hvm_init(MachineState *machine)
>      }
>  
>      pc_xen_hvm_init_pci(machine);
> +    xen_igd_reserve_slot(pcms->bus);
>      pci_create_simple(pcms->bus, -1, "xen-platform");
>  }
>  #endif
> diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
> index 0ec7e52183..51f100f64a 100644
> --- a/hw/xen/xen_pt.c
> +++ b/hw/xen/xen_pt.c
> @@ -57,6 +57,7 @@
>  #include <sys/ioctl.h>
>  
>  #include "hw/pci/pci.h"
> +#include "hw/pci/pci_bus.h"
>  #include "hw/qdev-properties.h"
>  #include "hw/qdev-properties-system.h"
>  #include "hw/xen/xen.h"
> @@ -780,15 +781,6 @@ static void xen_pt_realize(PCIDevice *d, Error **errp)
>                 s->hostaddr.bus, s->hostaddr.slot, s->hostaddr.function,
>                 s->dev.devfn);
>  
> -    xen_host_pci_device_get(&s->real_device,
> -                            s->hostaddr.domain, s->hostaddr.bus,
> -                            s->hostaddr.slot, s->hostaddr.function,
> -                            errp);
> -    if (*errp) {
> -        error_append_hint(errp, "Failed to \"open\" the real pci device");
> -        return;
> -    }
> -
>      s->is_virtfn = s->real_device.is_virtfn;
>      if (s->is_virtfn) {
>          XEN_PT_LOG(d, "%04x:%02x:%02x.%d is a SR-IOV Virtual Function\n",
> @@ -803,8 +795,10 @@ static void xen_pt_realize(PCIDevice *d, Error **errp)
>      s->io_listener = xen_pt_io_listener;
>  
>      /* Setup VGA bios for passthrough GFX */
> -    if ((s->real_device.domain == 0) && (s->real_device.bus == 0) &&
> -        (s->real_device.dev == 2) && (s->real_device.func == 0)) {
> +    if ((s->real_device.domain == XEN_PCI_IGD_DOMAIN) &&
> +        (s->real_device.bus == XEN_PCI_IGD_BUS) &&
> +        (s->real_device.dev == XEN_PCI_IGD_DEV) &&
> +        (s->real_device.func == XEN_PCI_IGD_FN)) {
>          if (!is_igd_vga_passthrough(&s->real_device)) {
>              error_setg(errp, "Need to enable igd-passthru if you're trying"
>                      " to passthrough IGD GFX");
> @@ -950,11 +944,55 @@ static void xen_pci_passthrough_instance_init(Object *obj)
>      PCI_DEVICE(obj)->cap_present |= QEMU_PCI_CAP_EXPRESS;
>  }
>  
> +void xen_igd_reserve_slot(PCIBus *pci_bus)
> +{
> +    if (!xen_igd_gfx_pt_enabled())
> +        return;
> +
> +    XEN_PT_LOG(0, "Reserving PCI slot 2 for IGD\n");
> +    pci_bus->slot_reserved_mask |= XEN_PCI_IGD_SLOT_MASK;
> +}
> +
> +static void xen_igd_clear_slot(DeviceState *qdev, Error **errp)
> +{
> +    ERRP_GUARD();
> +    PCIDevice *pci_dev = (PCIDevice *)qdev;
> +    XenPCIPassthroughState *s = XEN_PT_DEVICE(pci_dev);
> +    XenPTDeviceClass *xpdc = XEN_PT_DEVICE_GET_CLASS(s);
> +    PCIBus *pci_bus = pci_get_bus(pci_dev);
> +
> +    if (!(pci_bus->slot_reserved_mask & XEN_PCI_IGD_SLOT_MASK))
> +        return;
> +
> +    xen_host_pci_device_get(&s->real_device,
> +                            s->hostaddr.domain, s->hostaddr.bus,
> +                            s->hostaddr.slot, s->hostaddr.function,
> +                            errp);
> +    if (*errp) {
> +        error_append_hint(errp, "Failed to \"open\" the real pci device");
> +        return;
> +    }
> +
> +    if (is_igd_vga_passthrough(&s->real_device) &&
> +        s->real_device.domain == XEN_PCI_IGD_DOMAIN &&
> +        s->real_device.bus == XEN_PCI_IGD_BUS &&
> +        s->real_device.dev == XEN_PCI_IGD_DEV &&
> +        s->real_device.func == XEN_PCI_IGD_FN &&
> +        s->real_device.vendor_id == PCI_VENDOR_ID_INTEL) {
> +        pci_bus->slot_reserved_mask &= ~XEN_PCI_IGD_SLOT_MASK;
> +        XEN_PT_LOG(pci_dev, "Intel IGD found, using slot 2\n");
> +    }
> +    xpdc->pci_qdev_realize(qdev, errp);
> +}
> +
>  static void xen_pci_passthrough_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>  
> +    XenPTDeviceClass *xpdc = XEN_PT_DEVICE_CLASS(klass);
> +    xpdc->pci_qdev_realize = dc->realize;
> +    dc->realize = xen_igd_clear_slot;
>      k->realize = xen_pt_realize;
>      k->exit = xen_pt_unregister_device;
>      k->config_read = xen_pt_pci_read_config;
> @@ -977,6 +1015,7 @@ static const TypeInfo xen_pci_passthrough_info = {
>      .instance_size = sizeof(XenPCIPassthroughState),
>      .instance_finalize = xen_pci_passthrough_finalize,
>      .class_init = xen_pci_passthrough_class_init,
> +    .class_size = sizeof(XenPTDeviceClass),
>      .instance_init = xen_pci_passthrough_instance_init,
>      .interfaces = (InterfaceInfo[]) {
>          { INTERFACE_CONVENTIONAL_PCI_DEVICE },
> diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
> index cf10fc7bbf..e184699740 100644
> --- a/hw/xen/xen_pt.h
> +++ b/hw/xen/xen_pt.h
> @@ -40,7 +40,20 @@ typedef struct XenPTReg XenPTReg;
>  #define TYPE_XEN_PT_DEVICE "xen-pci-passthrough"
>  OBJECT_DECLARE_SIMPLE_TYPE(XenPCIPassthroughState, XEN_PT_DEVICE)
>  
> +#define XEN_PT_DEVICE_CLASS(klass) \
> +    OBJECT_CLASS_CHECK(XenPTDeviceClass, klass, TYPE_XEN_PT_DEVICE)
> +#define XEN_PT_DEVICE_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(XenPTDeviceClass, obj, TYPE_XEN_PT_DEVICE)
> +
> +typedef void (*XenPTQdevRealize)(DeviceState *qdev, Error **errp);
> +
> +typedef struct XenPTDeviceClass {
> +    PCIDeviceClass parent_class;
> +    XenPTQdevRealize pci_qdev_realize;
> +} XenPTDeviceClass;
> +
>  uint32_t igd_read_opregion(XenPCIPassthroughState *s);
> +void xen_igd_reserve_slot(PCIBus *pci_bus);
>  void igd_write_opregion(XenPCIPassthroughState *s, uint32_t val);
>  void xen_igd_passthrough_isa_bridge_create(XenPCIPassthroughState *s,
>                                             XenHostPCIDevice *dev);
> @@ -75,6 +88,13 @@ typedef int (*xen_pt_conf_byte_read)
>  
>  #define XEN_PCI_INTEL_OPREGION 0xfc
>  
> +#define XEN_PCI_IGD_DOMAIN 0
> +#define XEN_PCI_IGD_BUS 0
> +#define XEN_PCI_IGD_DEV 2
> +#define XEN_PCI_IGD_FN 0
> +#define XEN_PCI_IGD_SLOT_MASK \
> +    (1UL << PCI_SLOT(PCI_DEVFN(XEN_PCI_IGD_DEV, XEN_PCI_IGD_FN)))
> +
>  typedef enum {
>      XEN_PT_GRP_TYPE_HARDWIRED = 0,  /* 0 Hardwired reg group */
>      XEN_PT_GRP_TYPE_EMU,            /* emul reg group */
> diff --git a/hw/xen/xen_pt_stub.c b/hw/xen/xen_pt_stub.c
> index 2d8cac8d54..5c108446a8 100644
> --- a/hw/xen/xen_pt_stub.c
> +++ b/hw/xen/xen_pt_stub.c
> @@ -20,3 +20,7 @@ void xen_igd_gfx_pt_set(bool value, Error **errp)
>          error_setg(errp, "Xen PCI passthrough support not built in");
>      }
>  }
> +
> +void xen_igd_reserve_slot(PCIBus *pci_bus)
> +{
> +}
> -- 
> 2.39.0
> 

Re: [PATCH v9] xen/pt: reserve PCI slot 2 for Intel igd-passthru

[Chuck Zmudzinski]

On 1/20/23 4:34 PM, Stefano Stabellini wrote:
> On Sat, 14 Jan 2023, Chuck Zmudzinski wrote:
> > Intel specifies that the Intel IGD must occupy slot 2 on the PCI bus,
> > as noted in docs/igd-assign.txt in the Qemu source code.
> > 
> > Currently, when the xl toolstack is used to configure a Xen HVM guest with
> > Intel IGD passthrough to the guest with the Qemu upstream device model,
> > a Qemu emulated PCI device will occupy slot 2 and the Intel IGD will occupy
> > a different slot. This problem often prevents the guest from booting.
> > 
> > The only available workaround is not good: Configure Xen HVM guests to use
> > the old and no longer maintained Qemu traditional device model available
> > from xenbits.xen.org which does reserve slot 2 for the Intel IGD.
> > 
> > To implement this feature in the Qemu upstream device model for Xen HVM
> > guests, introduce the following new functions, types, and macros:
> > 
> > * XEN_PT_DEVICE_CLASS declaration, based on the existing TYPE_XEN_PT_DEVICE
> > * XEN_PT_DEVICE_GET_CLASS macro helper function for XEN_PT_DEVICE_CLASS
> > * typedef XenPTQdevRealize function pointer
> > * XEN_PCI_IGD_SLOT_MASK, the value of slot_reserved_mask to reserve slot 2
> > * xen_igd_reserve_slot and xen_igd_clear_slot functions
> > 
> > Michael Tsirkin:
> > * Introduce XEN_PCI_IGD_DOMAIN, XEN_PCI_IGD_BUS, XEN_PCI_IGD_DEV, and
> >   XEN_PCI_IGD_FN - use them to compute the value of XEN_PCI_IGD_SLOT_MASK
> > 
> > The new xen_igd_reserve_slot function uses the existing slot_reserved_mask
> > member of PCIBus to reserve PCI slot 2 for Xen HVM guests configured using
> > the xl toolstack with the gfx_passthru option enabled, which sets the
> > igd-passthru=on option to Qemu for the Xen HVM machine type.
> > 
> > The new xen_igd_reserve_slot function also needs to be implemented in
> > hw/xen/xen_pt_stub.c to prevent FTBFS during the link stage for the case
> > when Qemu is configured with --enable-xen and --disable-xen-pci-passthrough,
> > in which case it does nothing.
> > 
> > The new xen_igd_clear_slot function overrides qdev->realize of the parent
> > PCI device class to enable the Intel IGD to occupy slot 2 on the PCI bus
> > since slot 2 was reserved by xen_igd_reserve_slot when the PCI bus was
> > created in hw/i386/pc_piix.c for the case when igd-passthru=on.
> > 
> > Move the call to xen_host_pci_device_get, and the associated error
> > handling, from xen_pt_realize to the new xen_igd_clear_slot function to
> > initialize the device class and vendor values which enables the checks for
> > the Intel IGD to succeed. The verification that the host device is an
> > Intel IGD to be passed through is done by checking the domain, bus, slot,
> > and function values as well as by checking that gfx_passthru is enabled,
> > the device class is VGA, and the device vendor in Intel.
> > 
> > Signed-off-by: Chuck Zmudzinski <brchuckz@aol.com>
>
> Hi Chuck,
>
> The approach looks OK in principle to me. I only have one question: for
> other PCI devices (not Intel IGD), where is xen_host_pci_device_get
> called now?

I think you are right that there might be a problem for the
devices being added after the Intel IGD. I think I only tested
the case when the Intel IGD is the last device being added.
I do expect if I add the Intel IGD first, then there will be
problems when the subsequent type XEN_PT devices are
added because xen_pt_realize will not like it if
xen_host_pci_device_get does not get called. I will check
this over the weekend and, if a change is needed, I will
post it in v10.

I also think there is the same problem when the bit in
slot_reserved_mask is never set, which happens when
the guest has pci devices passed through but without
configuring Qemu with the igd-passthru=on option for
the xenfv machine type. I will also test this over the
weekend.

> It looks like that xen_igd_reserve_slot would return without setting
> slot_reserved_mask, hence xen_igd_clear_slot would also return without
> calling xen_host_pci_device_get. And xen_pt_realize doesn't call
> xen_host_pci_device_get any longer.
>
> Am I missing something?

No, you are not missing anything. You are pointing to some
cases that I need to test that probably would not work.
I think the fix is to have this at the beginning of
xen_igd_clear_slot instead of what I have now:

    xen_host_pci_device_get(&s->real_device,
                            s->hostaddr.domain, s->hostaddr.bus,
                            s->hostaddr.slot, s->hostaddr.function,
                            errp);
    if (*errp) {
        error_append_hint(errp, "Failed to \"open\" the real pci device");
        return;
    }

    if (!(pci_bus->slot_reserved_mask & XEN_PCI_IGD_SLOT_MASK))
        return;

That way xen_host_pci_device_get would still get called
when xen_igd_clear_slot returns because the bit in the
mask was not set.

Thanks for your careful review of the patch. I think you
did find a real mistake that needs to be fixed in v10 which
I hope to post with the above mentioned change early next
week.

Chuck
>
>
> > ---
> > Notes that might be helpful to reviewers of patched code in hw/xen:
> > 
> > The new functions and types are based on recommendations from Qemu docs:
> > https://qemu.readthedocs.io/en/latest/devel/qom.html
> > 
> > Notes that might be helpful to reviewers of patched code in hw/i386:
> > 
> > The small patch to hw/i386/pc_piix.c is protected by CONFIG_XEN so it does
> > not affect builds that do not have CONFIG_XEN defined.
> > 
> > xen_igd_gfx_pt_enabled() in the patched hw/i386/pc_piix.c file is an
> > existing function that is only true when Qemu is built with
> > xen-pci-passthrough enabled and the administrator has configured the Xen
> > HVM guest with Qemu's igd-passthru=on option.
> > 
> > v2: Remove From: <email address> tag at top of commit message
> > 
> > v3: Changed the test for the Intel IGD in xen_igd_clear_slot:
> > 
> >     if (is_igd_vga_passthrough(&s->real_device) &&
> >         (s->real_device.vendor_id == PCI_VENDOR_ID_INTEL)) {
> > 
> >     is changed to
> > 
> >     if (xen_igd_gfx_pt_enabled() && (s->hostaddr.slot == 2)
> >         && (s->hostaddr.function == 0)) {
> > 
> >     I hoped that I could use the test in v2, since it matches the
> >     other tests for the Intel IGD in Qemu and Xen, but those tests
> >     do not work because the necessary data structures are not set with
> >     their values yet. So instead use the test that the administrator
> >     has enabled gfx_passthru and the device address on the host is
> >     02.0. This test does detect the Intel IGD correctly.
> > 
> > v4: Use brchuckz@aol.com instead of brchuckz@netscape.net for the author's
> >     email address to match the address used by the same author in commits
> >     be9c61da and c0e86b76
> >     
> >     Change variable for XEN_PT_DEVICE_CLASS: xptc changed to xpdc
> > 
> > v5: The patch of xen_pt.c was re-worked to allow a more consistent test
> >     for the Intel IGD that uses the same criteria as in other places.
> >     This involved moving the call to xen_host_pci_device_get from
> >     xen_pt_realize to xen_igd_clear_slot and updating the checks for the
> >     Intel IGD in xen_igd_clear_slot:
> >     
> >     if (xen_igd_gfx_pt_enabled() && (s->hostaddr.slot == 2)
> >         && (s->hostaddr.function == 0)) {
> > 
> >     is changed to
> > 
> >     if (is_igd_vga_passthrough(&s->real_device) &&
> >         s->real_device.domain == 0 && s->real_device.bus == 0 &&
> >         s->real_device.dev == 2 && s->real_device.func == 0 &&
> >         s->real_device.vendor_id == PCI_VENDOR_ID_INTEL) {
> > 
> >     Added an explanation for the move of xen_host_pci_device_get from
> >     xen_pt_realize to xen_igd_clear_slot to the commit message.
> > 
> >     Rebase.
> > 
> > v6: Fix logging by removing these lines from the move from xen_pt_realize
> >     to xen_igd_clear_slot that was done in v5:
> > 
> >     XEN_PT_LOG(d, "Assigning real physical device %02x:%02x.%d"
> >                " to devfn 0x%x\n",
> >                s->hostaddr.bus, s->hostaddr.slot, s->hostaddr.function,
> >                s->dev.devfn);
> > 
> >     This log needs to be in xen_pt_realize because s->dev.devfn is not
> >     set yet in xen_igd_clear_slot.
> > 
> > v7: The v7 that was posted to the mailing list was incorrect. v8 is what
> >     v7 was intended to be.
> > 
> > v8: Inhibit out of context log message and needless processing by
> >     adding 2 lines at the top of the new xen_igd_clear_slot function:
> > 
> >     if (!(pci_bus->slot_reserved_mask & XEN_PCI_IGD_SLOT_MASK))
> >         return;
> > 
> >     Rebase. This removed an unnecessary header file from xen_pt.h 
> > 
> > v9: Move check for xen_igd_gfx_pt_enabled() from pc_piix.c to xen_pt.c
> > 
> >     Move #include "hw/pci/pci_bus.h" from xen_pt.h to xen_pt.c
> > 
> >     Introduce macros for the IGD devfn constants and use them to compute
> >     the value of XEN_PCI_IGD_SLOT_MASK
> > 
> >     Also use the new macros at an appropriate place in xen_pt_realize
> > 
> >     Add Cc: to stable - This has been broken for a long time, ever since
> >                         support for igd-passthru was added to Qemu 7
> >                         years ago.
> > 
> >     Mention new macros in the commit message (Michael Tsirkin)
> > 
> >     N.B.: I could not follow the suggestion to move the statement
> >     pci_bus->slot_reserved_mask &= ~XEN_PCI_IGD_SLOT_MASK; to after
> >     pci_qdev_realize for symmetry. Doing that results in an error when
> >     creating the guest:
> >     
> >     libxl: error: libxl_qmp.c:1837:qmp_ev_parse_error_messages: Domain 4:PCI: slot 2 function 0 not available for xen-pci-passthrough, reserved
> >     libxl: error: libxl_pci.c:1809:device_pci_add_done: Domain 4:libxl__device_pci_add failed for PCI device 0:0:2.0 (rc -28)
> >     libxl: error: libxl_create.c:1921:domcreate_attach_devices: Domain 4:unable to add pci devices
> > 
> >  hw/i386/pc_piix.c    |  1 +
> >  hw/xen/xen_pt.c      | 61 ++++++++++++++++++++++++++++++++++++--------
> >  hw/xen/xen_pt.h      | 20 +++++++++++++++
> >  hw/xen/xen_pt_stub.c |  4 +++
> >  4 files changed, 75 insertions(+), 11 deletions(-)
> > 
> > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > index b48047f50c..8fc96eb63b 100644
> > --- a/hw/i386/pc_piix.c
> > +++ b/hw/i386/pc_piix.c
> > @@ -405,6 +405,7 @@ static void pc_xen_hvm_init(MachineState *machine)
> >      }
> >  
> >      pc_xen_hvm_init_pci(machine);
> > +    xen_igd_reserve_slot(pcms->bus);
> >      pci_create_simple(pcms->bus, -1, "xen-platform");
> >  }
> >  #endif
> > diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
> > index 0ec7e52183..51f100f64a 100644
> > --- a/hw/xen/xen_pt.c
> > +++ b/hw/xen/xen_pt.c
> > @@ -57,6 +57,7 @@
> >  #include <sys/ioctl.h>
> >  
> >  #include "hw/pci/pci.h"
> > +#include "hw/pci/pci_bus.h"
> >  #include "hw/qdev-properties.h"
> >  #include "hw/qdev-properties-system.h"
> >  #include "hw/xen/xen.h"
> > @@ -780,15 +781,6 @@ static void xen_pt_realize(PCIDevice *d, Error **errp)
> >                 s->hostaddr.bus, s->hostaddr.slot, s->hostaddr.function,
> >                 s->dev.devfn);
> >  
> > -    xen_host_pci_device_get(&s->real_device,
> > -                            s->hostaddr.domain, s->hostaddr.bus,
> > -                            s->hostaddr.slot, s->hostaddr.function,
> > -                            errp);
> > -    if (*errp) {
> > -        error_append_hint(errp, "Failed to \"open\" the real pci device");
> > -        return;
> > -    }
> > -
> >      s->is_virtfn = s->real_device.is_virtfn;
> >      if (s->is_virtfn) {
> >          XEN_PT_LOG(d, "%04x:%02x:%02x.%d is a SR-IOV Virtual Function\n",
> > @@ -803,8 +795,10 @@ static void xen_pt_realize(PCIDevice *d, Error **errp)
> >      s->io_listener = xen_pt_io_listener;
> >  
> >      /* Setup VGA bios for passthrough GFX */
> > -    if ((s->real_device.domain == 0) && (s->real_device.bus == 0) &&
> > -        (s->real_device.dev == 2) && (s->real_device.func == 0)) {
> > +    if ((s->real_device.domain == XEN_PCI_IGD_DOMAIN) &&
> > +        (s->real_device.bus == XEN_PCI_IGD_BUS) &&
> > +        (s->real_device.dev == XEN_PCI_IGD_DEV) &&
> > +        (s->real_device.func == XEN_PCI_IGD_FN)) {
> >          if (!is_igd_vga_passthrough(&s->real_device)) {
> >              error_setg(errp, "Need to enable igd-passthru if you're trying"
> >                      " to passthrough IGD GFX");
> > @@ -950,11 +944,55 @@ static void xen_pci_passthrough_instance_init(Object *obj)
> >      PCI_DEVICE(obj)->cap_present |= QEMU_PCI_CAP_EXPRESS;
> >  }
> >  
> > +void xen_igd_reserve_slot(PCIBus *pci_bus)
> > +{
> > +    if (!xen_igd_gfx_pt_enabled())
> > +        return;
> > +
> > +    XEN_PT_LOG(0, "Reserving PCI slot 2 for IGD\n");
> > +    pci_bus->slot_reserved_mask |= XEN_PCI_IGD_SLOT_MASK;
> > +}
> > +
> > +static void xen_igd_clear_slot(DeviceState *qdev, Error **errp)
> > +{
> > +    ERRP_GUARD();
> > +    PCIDevice *pci_dev = (PCIDevice *)qdev;
> > +    XenPCIPassthroughState *s = XEN_PT_DEVICE(pci_dev);
> > +    XenPTDeviceClass *xpdc = XEN_PT_DEVICE_GET_CLASS(s);
> > +    PCIBus *pci_bus = pci_get_bus(pci_dev);
> > +
> > +    if (!(pci_bus->slot_reserved_mask & XEN_PCI_IGD_SLOT_MASK))
> > +        return;
> > +
> > +    xen_host_pci_device_get(&s->real_device,
> > +                            s->hostaddr.domain, s->hostaddr.bus,
> > +                            s->hostaddr.slot, s->hostaddr.function,
> > +                            errp);
> > +    if (*errp) {
> > +        error_append_hint(errp, "Failed to \"open\" the real pci device");
> > +        return;
> > +    }
> > +
> > +    if (is_igd_vga_passthrough(&s->real_device) &&
> > +        s->real_device.domain == XEN_PCI_IGD_DOMAIN &&
> > +        s->real_device.bus == XEN_PCI_IGD_BUS &&
> > +        s->real_device.dev == XEN_PCI_IGD_DEV &&
> > +        s->real_device.func == XEN_PCI_IGD_FN &&
> > +        s->real_device.vendor_id == PCI_VENDOR_ID_INTEL) {
> > +        pci_bus->slot_reserved_mask &= ~XEN_PCI_IGD_SLOT_MASK;
> > +        XEN_PT_LOG(pci_dev, "Intel IGD found, using slot 2\n");
> > +    }
> > +    xpdc->pci_qdev_realize(qdev, errp);
> > +}
> > +
> >  static void xen_pci_passthrough_class_init(ObjectClass *klass, void *data)
> >  {
> >      DeviceClass *dc = DEVICE_CLASS(klass);
> >      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> >  
> > +    XenPTDeviceClass *xpdc = XEN_PT_DEVICE_CLASS(klass);
> > +    xpdc->pci_qdev_realize = dc->realize;
> > +    dc->realize = xen_igd_clear_slot;
> >      k->realize = xen_pt_realize;
> >      k->exit = xen_pt_unregister_device;
> >      k->config_read = xen_pt_pci_read_config;
> > @@ -977,6 +1015,7 @@ static const TypeInfo xen_pci_passthrough_info = {
> >      .instance_size = sizeof(XenPCIPassthroughState),
> >      .instance_finalize = xen_pci_passthrough_finalize,
> >      .class_init = xen_pci_passthrough_class_init,
> > +    .class_size = sizeof(XenPTDeviceClass),
> >      .instance_init = xen_pci_passthrough_instance_init,
> >      .interfaces = (InterfaceInfo[]) {
> >          { INTERFACE_CONVENTIONAL_PCI_DEVICE },
> > diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
> > index cf10fc7bbf..e184699740 100644
> > --- a/hw/xen/xen_pt.h
> > +++ b/hw/xen/xen_pt.h
> > @@ -40,7 +40,20 @@ typedef struct XenPTReg XenPTReg;
> >  #define TYPE_XEN_PT_DEVICE "xen-pci-passthrough"
> >  OBJECT_DECLARE_SIMPLE_TYPE(XenPCIPassthroughState, XEN_PT_DEVICE)
> >  
> > +#define XEN_PT_DEVICE_CLASS(klass) \
> > +    OBJECT_CLASS_CHECK(XenPTDeviceClass, klass, TYPE_XEN_PT_DEVICE)
> > +#define XEN_PT_DEVICE_GET_CLASS(obj) \
> > +    OBJECT_GET_CLASS(XenPTDeviceClass, obj, TYPE_XEN_PT_DEVICE)
> > +
> > +typedef void (*XenPTQdevRealize)(DeviceState *qdev, Error **errp);
> > +
> > +typedef struct XenPTDeviceClass {
> > +    PCIDeviceClass parent_class;
> > +    XenPTQdevRealize pci_qdev_realize;
> > +} XenPTDeviceClass;
> > +
> >  uint32_t igd_read_opregion(XenPCIPassthroughState *s);
> > +void xen_igd_reserve_slot(PCIBus *pci_bus);
> >  void igd_write_opregion(XenPCIPassthroughState *s, uint32_t val);
> >  void xen_igd_passthrough_isa_bridge_create(XenPCIPassthroughState *s,
> >                                             XenHostPCIDevice *dev);
> > @@ -75,6 +88,13 @@ typedef int (*xen_pt_conf_byte_read)
> >  
> >  #define XEN_PCI_INTEL_OPREGION 0xfc
> >  
> > +#define XEN_PCI_IGD_DOMAIN 0
> > +#define XEN_PCI_IGD_BUS 0
> > +#define XEN_PCI_IGD_DEV 2
> > +#define XEN_PCI_IGD_FN 0
> > +#define XEN_PCI_IGD_SLOT_MASK \
> > +    (1UL << PCI_SLOT(PCI_DEVFN(XEN_PCI_IGD_DEV, XEN_PCI_IGD_FN)))
> > +
> >  typedef enum {
> >      XEN_PT_GRP_TYPE_HARDWIRED = 0,  /* 0 Hardwired reg group */
> >      XEN_PT_GRP_TYPE_EMU,            /* emul reg group */
> > diff --git a/hw/xen/xen_pt_stub.c b/hw/xen/xen_pt_stub.c
> > index 2d8cac8d54..5c108446a8 100644
> > --- a/hw/xen/xen_pt_stub.c
> > +++ b/hw/xen/xen_pt_stub.c
> > @@ -20,3 +20,7 @@ void xen_igd_gfx_pt_set(bool value, Error **errp)
> >          error_setg(errp, "Xen PCI passthrough support not built in");
> >      }
> >  }
> > +
> > +void xen_igd_reserve_slot(PCIBus *pci_bus)
> > +{
> > +}
> > -- 
> > 2.39.0
> > 

Re: [PATCH v9] xen/pt: reserve PCI slot 2 for Intel igd-passthru

[Chuck Zmudzinski]

On 1/20/2023 4:34 PM, Stefano Stabellini wrote:
> On Sat, 14 Jan 2023, Chuck Zmudzinski wrote:
> > Intel specifies that the Intel IGD must occupy slot 2 on the PCI bus,
> > as noted in docs/igd-assign.txt in the Qemu source code.
> > 
> > Currently, when the xl toolstack is used to configure a Xen HVM guest with
> > Intel IGD passthrough to the guest with the Qemu upstream device model,
> > a Qemu emulated PCI device will occupy slot 2 and the Intel IGD will occupy
> > a different slot. This problem often prevents the guest from booting.
> > 
> > The only available workaround is not good: Configure Xen HVM guests to use
> > the old and no longer maintained Qemu traditional device model available
> > from xenbits.xen.org which does reserve slot 2 for the Intel IGD.
> > 
> > To implement this feature in the Qemu upstream device model for Xen HVM
> > guests, introduce the following new functions, types, and macros:
> > 
> > * XEN_PT_DEVICE_CLASS declaration, based on the existing TYPE_XEN_PT_DEVICE
> > * XEN_PT_DEVICE_GET_CLASS macro helper function for XEN_PT_DEVICE_CLASS
> > * typedef XenPTQdevRealize function pointer
> > * XEN_PCI_IGD_SLOT_MASK, the value of slot_reserved_mask to reserve slot 2
> > * xen_igd_reserve_slot and xen_igd_clear_slot functions
> > 
> > Michael Tsirkin:
> > * Introduce XEN_PCI_IGD_DOMAIN, XEN_PCI_IGD_BUS, XEN_PCI_IGD_DEV, and
> >   XEN_PCI_IGD_FN - use them to compute the value of XEN_PCI_IGD_SLOT_MASK
> > 
> > The new xen_igd_reserve_slot function uses the existing slot_reserved_mask
> > member of PCIBus to reserve PCI slot 2 for Xen HVM guests configured using
> > the xl toolstack with the gfx_passthru option enabled, which sets the
> > igd-passthru=on option to Qemu for the Xen HVM machine type.
> > 
> > The new xen_igd_reserve_slot function also needs to be implemented in
> > hw/xen/xen_pt_stub.c to prevent FTBFS during the link stage for the case
> > when Qemu is configured with --enable-xen and --disable-xen-pci-passthrough,
> > in which case it does nothing.
> > 
> > The new xen_igd_clear_slot function overrides qdev->realize of the parent
> > PCI device class to enable the Intel IGD to occupy slot 2 on the PCI bus
> > since slot 2 was reserved by xen_igd_reserve_slot when the PCI bus was
> > created in hw/i386/pc_piix.c for the case when igd-passthru=on.
> > 
> > Move the call to xen_host_pci_device_get, and the associated error
> > handling, from xen_pt_realize to the new xen_igd_clear_slot function to
> > initialize the device class and vendor values which enables the checks for
> > the Intel IGD to succeed. The verification that the host device is an
> > Intel IGD to be passed through is done by checking the domain, bus, slot,
> > and function values as well as by checking that gfx_passthru is enabled,
> > the device class is VGA, and the device vendor in Intel.
> > 
> > Signed-off-by: Chuck Zmudzinski <brchuckz@aol.com>
>
> Hi Chuck,
>
> The approach looks OK in principle to me. I only have one question: for
> other PCI devices (not Intel IGD), where is xen_host_pci_device_get
> called now?
>
> It looks like that xen_igd_reserve_slot would return without setting
> slot_reserved_mask, hence xen_igd_clear_slot would also return without
> calling xen_host_pci_device_get. And xen_pt_realize doesn't call
> xen_host_pci_device_get any longer.
>
> Am I missing something?

Thanks for catching this. With v9 guest creation fails when the bit in
slot_reserved_mask that reserves slot 2 is not set.

It fails because v9 not only fails to call xen_host_pci_device_get when the
bit in slot_reserved_mask that reserves slot 2 is not set, it also does not
call xpdc->pci_qdev_realize either. So I uploaded v10 to fix that here:

https://lore.kernel.org/qemu-devel/d473914c4d2dc38ae87dca4b898d75b44751c9cb.1674297794.git.brchuckz@aol.com/

Tests with v10 show it is now working for all cases.

Thanks,

Chuck