[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v8] xen/pt: reserve PCI slot 2 for Intel igd-passthru
From: |
Chuck Zmudzinski |
Subject: |
Re: [PATCH v8] xen/pt: reserve PCI slot 2 for Intel igd-passthru |
Date: |
Fri, 13 Jan 2023 21:55:22 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.6.0 |
On 1/10/23 3:16 AM, Michael S. Tsirkin wrote:
> On Tue, Jan 10, 2023 at 02:08:34AM -0500, 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
>>
>> 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
>>
>> hw/i386/pc_piix.c | 3 +++
>> hw/xen/xen_pt.c | 49 ++++++++++++++++++++++++++++++++++++--------
>> hw/xen/xen_pt.h | 16 +++++++++++++++
>> hw/xen/xen_pt_stub.c | 4 ++++
>> 4 files changed, 63 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index b48047f50c..bc5efa4f59 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -405,6 +405,9 @@ static void pc_xen_hvm_init(MachineState *machine)
>> }
>>
>> pc_xen_hvm_init_pci(machine);
>> + if (xen_igd_gfx_pt_enabled()) {
>> + xen_igd_reserve_slot(pcms->bus);
>> + }
>> pci_create_simple(pcms->bus, -1, "xen-platform");
>> }
>> #endif
>
> I would even maybe go further and move the whole logic into
> xen_igd_reserve_slot. And I would even just name it
> xen_hvm_init_reserved_slots without worrying about the what
> or why at the pc level. At this point it will be up to Xen maintainers.
>
>> diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
>> index 0ec7e52183..eff38155ef 100644
>> --- a/hw/xen/xen_pt.c
>> +++ b/hw/xen/xen_pt.c
>> @@ -780,15 +780,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",
>> @@ -950,11 +941,50 @@ 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)
>> +{
>> + 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 == 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) {
>
> how about macros for these?
>
> #define XEN_PCI_IGD_DOMAIN 0
> #define XEN_PCI_IGD_BUS 0
> #define XEN_PCI_IGD_DEV 2
> #define XEN_PCI_IGD_FN 0
>
>> + pci_bus->slot_reserved_mask &= ~XEN_PCI_IGD_SLOT_MASK;
>
> If you are going to do this, you should set it back
> either after pci_qdev_realize or in unrealize,
> for symmetry.
I presume you are talking about the log here. The clearing of
the bit must be done before pci_qdev_realize because the slot
is assigned in pci_qdev_realize. If the bit is not cleared *before*
calling pci_qdev_realize, the igd will not be assigned slot 2.
Doing that would defeat the whole purpose of the patch.
I suppose I could move the log message after pci_qdev_realize
for symmetry, that would not change how the patch works.
>
>> + 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 +1007,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..8c25932b4b 100644
>> --- a/hw/xen/xen_pt.h
>> +++ b/hw/xen/xen_pt.h
>> @@ -2,6 +2,7 @@
>> #define XEN_PT_H
>>
>> #include "hw/xen/xen_common.h"
>> +#include "hw/pci/pci_bus.h"
>> #include "xen-host-pci-device.h"
>> #include "qom/object.h"
>>
>> @@ -40,7 +41,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 +89,8 @@ typedef int (*xen_pt_conf_byte_read)
>>
>> #define XEN_PCI_INTEL_OPREGION 0xfc
>>
>> +#define XEN_PCI_IGD_SLOT_MASK 0x4UL /* Intel IGD slot_reserved_mask */
>> +
>
> I think you want to calculate this based on dev fn:
>
> #define XEN_PCI_IGD_SLOT_MASK \
> (0x1 << 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 v8] xen/pt: reserve PCI slot 2 for Intel igd-passthru, (continued)
- Re: [PATCH v8] xen/pt: reserve PCI slot 2 for Intel igd-passthru, Chuck Zmudzinski, 2023/01/17
- Re: [PATCH v8] xen/pt: reserve PCI slot 2 for Intel igd-passthru, Igor Mammedov, 2023/01/20
- Re: [PATCH v8] xen/pt: reserve PCI slot 2 for Intel igd-passthru, Igor Mammedov, 2023/01/17
- Re: [PATCH v8] xen/pt: reserve PCI slot 2 for Intel igd-passthru, Chuck Zmudzinski, 2023/01/17
- Re: [PATCH v8] xen/pt: reserve PCI slot 2 for Intel igd-passthru, Alex Williamson, 2023/01/17
- Re: [PATCH v8] xen/pt: reserve PCI slot 2 for Intel igd-passthru, Chuck Zmudzinski, 2023/01/18
- Re: [PATCH v8] xen/pt: reserve PCI slot 2 for Intel igd-passthru, Stefano Stabellini, 2023/01/20
- Re: [PATCH v8] xen/pt: reserve PCI slot 2 for Intel igd-passthru, Chuck Zmudzinski, 2023/01/20
Re: [PATCH v8] xen/pt: reserve PCI slot 2 for Intel igd-passthru,
Chuck Zmudzinski <=