Am 12. Januar 2023 20:11:54 UTC schrieb Chuck Zmudzinski <brchu...@aol.com>:
>On 1/12/23 2:18 PM, Bernhard Beschow wrote:
>>
>>
>> Am 11. Januar 2023 15:40:24 UTC schrieb Chuck Zmudzinski <brchu...@aol.com>:
>>>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 <brchu...@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 brchu...@aol.com instead of brchu...@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.
>>>
>>>I see to do that would be to resolve the two pc_xen_hvm*
>>>functions in pc_piix.c that are guarded by CONFIG_XEN and
>>>move them to an appropriate place such as xen-hvm.c.
>>>
>>>That is along the lines of the work that Bernhard and Philippe
>>>are doing, so I am Cc'ing them. My first inclination is just
>>>to defer to them: I think eventually the little patch I propose
>>>here to pc_piix.c is eventually going to be moved out of pc_piix.c
>>>by Bernhard in a future patch.
>>>
>>>What they have been doing is very conservative, and I expect
>>>if and when Bernhard gets here to resolve those functions, they
>>>will do it in a way that keeps the dependency of the xenfv machine
>>>type on the pc machine type and the pc_init1 function.
>>>
>>>What I would propose would be to break the dependency of xenfv
>>>on the pc_init1 function. That is, I would propose having a
>>>xenfv_init function in xen-hvm.c, and the first version would
>>>be the current version of pc_init1, so xenfv would still depend
>>>on many i440fx type things, but with the change xen developers
>>>would be free to tweak xenfv_init without affecting the users
>>>of the pc machine type.
>>>
>>>Would that be a good idea? If I get posiive feedback for this
>>>idea, I will put it on the table, probably initially as an RFC
>>>patch.
>>
>> In various patches I've been decoupling 1/ PIIX3 from Xen and 2/ QEMU's Xen
>> integration code from the PC machine. My idea is to confine all wiring for a
>> PIIX based PC machine using Xen in pc_piix.c. The pc_xen_hvm* functions seem
>> to do exactly that, so I'd leave them there, at least for now.
>>
>> What I would like to avoid is for the Xen integration code to make
>> assumptions that an x86 or PC machine is always based on i440fx or PIIX3.
>
>I think what you are saying is that if I try to move the logic of my patch to
>xen-hvm.c, as Michael suggests, I should not move or copy any piix3 code to
>the Xen integration code, but access it via an appropriate qom interface to
>the code in pc_piix.c and only move Xen specific things to the Xen integration
>code such as the content of my patch. I can try to do that for a v9 of my
>patch. It might take me a little while (I am not a professional coder), so I
>will just leave v8 of my patch as is for now until I have a patch ready to
>move it out of pc_piix.c the qom way.
I think the change Michael suggests is very minimalistic: Move the if condition
around xen_igd_reserve_slot() into the function itself and always call it there
unconditionally -- basically turning three lines into one. Since
xen_igd_reserve_slot() seems very problem specific, Michael further suggests to
rename it to something more general. All in all no big changes required.
Best regards,
Bernhard
>
>Thanks,
>
>Chuck
>
>>
>> I like Michael's idea of going one step further, both in terms of the
>> approach and the reasoning.
>>
>> Best regards,
>> Bernhard
>>
>>>Also, thanks, Michael, for your other suggestions for this patch
>>>about using macros for the devfn constants.
>>>
>>>Chuck
>>>
>>>>
>>>>> 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.
>>>>
>>>>> + 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
>>>>
>>>
>