On 2025-04-08 02:24, Jan Beulich wrote:
On 07.04.2025 21:26, Jason Andryuk wrote:
A PCI device's irq field is an 8-bit number. A value of 0xff indicates
that the device is not connected.
Nit: "... that the device IRQ is not ..."
Additionally, the Linux ACPI code can
convert these 0xff values to IRQ_NOTCONNECTED(0x80000000) because
"0x80000000 is guaranteed to be outside the available range of
interrupts and easy to distinguish from other possible incorrect
values." When the hypercall to assign that IRQ fails, device
passthrough as a whole fails.
Add checking for a valid IRQ and skip the IRQ handling for PCI devices
outside that range. This allows for passthrough of devices without
legacy IRQs.
Which makes the code here even more Linux-centric, I guess.
The actual check is limited to the 8-bit value which is not Linux-specific.
A couple of related notes, yet most not directly affecting this patch:
--- a/tools/libs/light/libxl_pci.c
+++ b/tools/libs/light/libxl_pci.c
@@ -26,6 +26,9 @@
#define PCI_BDF_XSPATH "%04x-%02x-%02x-%01x"
#define PCI_PT_QDEV_ID "pci-pt-%02x_%02x.%01x"
+/* PCI Interrupt Line is an 8-bit value, 0xff means disconnected. */
+#define PCI_IRQ_LINE_LIMIT 0xff
+
static unsigned int pci_encode_bdf(libxl_device_pci *pci)
{
unsigned int value;
@@ -1495,7 +1498,8 @@ static void pci_add_dm_done(libxl__egc *egc,
LOGED(ERROR, domainid, "Couldn't open %s", sysfs_path);
goto out_no_irq;
}
- if ((fscanf(f, "%u", &irq) == 1) && irq) {
+ if ((fscanf(f, "%u", &irq) == 1) &&
For this, "irq" ought to be unsigned int. Same below.
+ irq > 0 && irq < PCI_IRQ_LINE_LIMIT) {
Not sure about this in libxl's style, but it feels inconsistent to have
parentheses around one relational expression but then not around the
others. Personally I'd drop them all, but the alternative clearly is to
add missing ones.
I'll drop them, and then it all fits on one line.
r = xc_physdev_map_pirq(ctx->xch, domid, irq, &irq);
if (r < 0) {
LOGED(ERROR, domainid, "xc_physdev_map_pirq irq=%d
(error=%d)",
@@ -2257,7 +2261,8 @@ skip_bar:
goto skip_legacy_irq;
}
- if ((fscanf(f, "%u", &irq) == 1) && irq) {
+ if ((fscanf(f, "%u", &irq) == 1) &&
+ irq > 0 && irq < PCI_IRQ_LINE_LIMIT) {
rc = xc_physdev_unmap_pirq(ctx->xch, domid, irq);
if (rc < 0) {
/*
This is doing things in sensible order: unmap, then remove permissions.
The map side though adds permissions only after mapping. That's kind of
necessary because the value to pass into xc_domain_irq_permission() is
an output of xc_physdev_map_pirq(). Yet then the latter should have
failed for lack of permissions, unless permissions were granted another
way? In which case what's the point of granting permissions here?
Permissions are checked later when binding - XEN_DOMCTL_bind_pt_irq or
EVTCHNOP_bind_pirq. But I agree the ordering is a unusual.
Regards,
Jason
