* [PATCH v2 0/3] PCI: re-work pci_get_pdev() and friends
@ 2022-08-11 10:50 Jan Beulich
2022-08-11 10:51 ` [PATCH v2 1/3] PCI: simplify (and thus correct) pci_get_pdev{,_by_domain}() Jan Beulich
` (3 more replies)
0 siblings, 4 replies; 17+ messages in thread
From: Jan Beulich @ 2022-08-11 10:50 UTC (permalink / raw)
To: xen-devel@lists.xenproject.org
Cc: Paul Durrant, Roger Pau Monné, Andrew Cooper, Rahul Singh
The two previously submitted bug fixes can actually be had as a side
effect of eliminating a bogus feature, the last use of which had
disappeared a while ago. Further cleanup then follows along the
lines of what had also been discussed in the context of the earlier
attempts.
1: simplify (and thus correct) pci_get_pdev{,_by_domain}()
2: fold pci_get_pdev{,_by_domain}()
3: bring pci_get_real_pdev() in line with pci_get_pdev()
Jan
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 1/3] PCI: simplify (and thus correct) pci_get_pdev{,_by_domain}()
2022-08-11 10:50 [PATCH v2 0/3] PCI: re-work pci_get_pdev() and friends Jan Beulich
@ 2022-08-11 10:51 ` Jan Beulich
2022-08-11 13:11 ` Andrew Cooper
2022-08-11 16:15 ` Rahul Singh
2022-08-11 10:52 ` [PATCH v2 2/3] PCI: fold pci_get_pdev{,_by_domain}() Jan Beulich
` (2 subsequent siblings)
3 siblings, 2 replies; 17+ messages in thread
From: Jan Beulich @ 2022-08-11 10:51 UTC (permalink / raw)
To: xen-devel@lists.xenproject.org
Cc: Paul Durrant, Roger Pau Monné, Andrew Cooper, Rahul Singh
The last "wildcard" use of either function went away with f591755823a7
("IOMMU/PCI: don't let domain cleanup continue when device de-assignment
failed"). Don't allow them to be called this way anymore. Besides
simplifying the code this also fixes two bugs:
1) When seg != -1, the outer loops should have been terminated after the
first iteration, or else a device with the same BDF but on another
segment could be found / returned.
Reported-by: Rahul Singh <rahul.singh@arm.com>
2) When seg == -1 calling get_pseg() is bogus. The function (taking a
u16) would look for segment 0xffff, which might exist. If it exists,
we might then find / return a wrong device.
In pci_get_pdev_by_domain() also switch from using the per-segment list
to using the per-domain one, with the exception of the hardware domain
(see the code comment there).
While there also constify "pseg" and drop "pdev"'s already previously
unnecessary initializer.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Full rework, with even the title changed.
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -576,30 +576,19 @@ int __init pci_ro_device(int seg, int bu
return 0;
}
-struct pci_dev *pci_get_pdev(int seg, int bus, int devfn)
+struct pci_dev *pci_get_pdev(uint16_t seg, uint8_t bus, uint8_t devfn)
{
- struct pci_seg *pseg = get_pseg(seg);
- struct pci_dev *pdev = NULL;
+ const struct pci_seg *pseg = get_pseg(seg);
+ struct pci_dev *pdev;
ASSERT(pcidevs_locked());
- ASSERT(seg != -1 || bus == -1);
- ASSERT(bus != -1 || devfn == -1);
if ( !pseg )
- {
- if ( seg == -1 )
- radix_tree_gang_lookup(&pci_segments, (void **)&pseg, 0, 1);
- if ( !pseg )
- return NULL;
- }
+ return NULL;
- do {
- list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
- if ( (pdev->bus == bus || bus == -1) &&
- (pdev->devfn == devfn || devfn == -1) )
- return pdev;
- } while ( radix_tree_gang_lookup(&pci_segments, (void **)&pseg,
- pseg->nr + 1, 1) );
+ list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
+ if ( pdev->bus == bus && pdev->devfn == devfn )
+ return pdev;
return NULL;
}
@@ -625,31 +614,33 @@ struct pci_dev *pci_get_real_pdev(int se
return pdev;
}
-struct pci_dev *pci_get_pdev_by_domain(const struct domain *d, int seg,
- int bus, int devfn)
+struct pci_dev *pci_get_pdev_by_domain(const struct domain *d, uint16_t seg,
+ uint8_t bus, uint8_t devfn)
{
- struct pci_seg *pseg = get_pseg(seg);
- struct pci_dev *pdev = NULL;
-
- ASSERT(seg != -1 || bus == -1);
- ASSERT(bus != -1 || devfn == -1);
+ struct pci_dev *pdev;
- if ( !pseg )
+ /*
+ * The hardware domain owns the majority of the devices in the system.
+ * When there are multiple segments, traversing the per-segment list is
+ * likely going to be faster, whereas for a single segment the difference
+ * shouldn't be that large.
+ */
+ if ( is_hardware_domain(d) )
{
- if ( seg == -1 )
- radix_tree_gang_lookup(&pci_segments, (void **)&pseg, 0, 1);
+ const struct pci_seg *pseg = get_pseg(seg);
+
if ( !pseg )
return NULL;
- }
- do {
list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
- if ( (pdev->bus == bus || bus == -1) &&
- (pdev->devfn == devfn || devfn == -1) &&
- (pdev->domain == d) )
+ if ( pdev->bus == bus && pdev->devfn == devfn &&
+ pdev->domain == d )
+ return pdev;
+ }
+ else
+ list_for_each_entry ( pdev, &d->pdev_list, domain_list )
+ if ( pdev->bus == bus && pdev->devfn == devfn )
return pdev;
- } while ( radix_tree_gang_lookup(&pci_segments, (void **)&pseg,
- pseg->nr + 1, 1) );
return NULL;
}
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -177,10 +177,10 @@ int pci_add_device(u16 seg, u8 bus, u8 d
int pci_remove_device(u16 seg, u8 bus, u8 devfn);
int pci_ro_device(int seg, int bus, int devfn);
int pci_hide_device(unsigned int seg, unsigned int bus, unsigned int devfn);
-struct pci_dev *pci_get_pdev(int seg, int bus, int devfn);
+struct pci_dev *pci_get_pdev(uint16_t seg, uint8_t bus, uint8_t devfn);
struct pci_dev *pci_get_real_pdev(int seg, int bus, int devfn);
-struct pci_dev *pci_get_pdev_by_domain(const struct domain *, int seg,
- int bus, int devfn);
+struct pci_dev *pci_get_pdev_by_domain(const struct domain *, uint16_t seg,
+ uint8_t bus, uint8_t devfn);
void pci_check_disable_device(u16 seg, u8 bus, u8 devfn);
uint8_t pci_conf_read8(pci_sbdf_t sbdf, unsigned int reg);
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 2/3] PCI: fold pci_get_pdev{,_by_domain}()
2022-08-11 10:50 [PATCH v2 0/3] PCI: re-work pci_get_pdev() and friends Jan Beulich
2022-08-11 10:51 ` [PATCH v2 1/3] PCI: simplify (and thus correct) pci_get_pdev{,_by_domain}() Jan Beulich
@ 2022-08-11 10:52 ` Jan Beulich
2022-08-11 13:21 ` Andrew Cooper
2022-08-11 16:17 ` Rahul Singh
2022-08-11 10:52 ` [PATCH v2 3/3] PCI: bring pci_get_real_pdev() in line with pci_get_pdev() Jan Beulich
2022-08-11 16:37 ` [PATCH] x86/msi: Switch msi_info to using pci_sbdf_t Andrew Cooper
3 siblings, 2 replies; 17+ messages in thread
From: Jan Beulich @ 2022-08-11 10:52 UTC (permalink / raw)
To: xen-devel@lists.xenproject.org
Cc: Paul Durrant, Roger Pau Monné, Rahul Singh, Andrew Cooper,
George Dunlap, Julien Grall, Stefano Stabellini, Wei Liu
Rename the latter, subsuming the functionality of the former when passed
NULL as first argument.
Since this requires touching all call sites anyway, take the opportunity
and fold the remaining three parameters into a single pci_sbdf_t one.
No functional change intended. In particular the locking related
assertion needs to continue to be kept silent when a non-NULL domain
pointer is passed - both vpci_read() and vpci_write() call the function
without holding the lock (adding respective locking to vPCI [or finding
an alternative to doing so] is the topic of a separate series).
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: New.
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -2162,7 +2162,7 @@ int map_domain_pirq(
if ( !cpu_has_apic )
goto done;
- pdev = pci_get_pdev_by_domain(d, msi->seg, msi->bus, msi->devfn);
+ pdev = pci_get_pdev(d, PCI_SBDF(msi->seg, msi->bus, msi->devfn));
if ( !pdev )
goto done;
--- a/xen/arch/x86/msi.c
+++ b/xen/arch/x86/msi.c
@@ -683,7 +683,8 @@ static u64 read_pci_mem_bar(u16 seg, u8
if ( vf >= 0 )
{
- struct pci_dev *pdev = pci_get_pdev(seg, bus, PCI_DEVFN(slot, func));
+ struct pci_dev *pdev = pci_get_pdev(NULL,
+ PCI_SBDF(seg, bus, slot, func));
unsigned int pos = pci_find_ext_capability(seg, bus,
PCI_DEVFN(slot, func),
PCI_EXT_CAP_ID_SRIOV);
@@ -1000,7 +1001,7 @@ static int __pci_enable_msi(struct msi_i
struct msi_desc *old_desc;
ASSERT(pcidevs_locked());
- pdev = pci_get_pdev(msi->seg, msi->bus, msi->devfn);
+ pdev = pci_get_pdev(NULL, PCI_SBDF(msi->seg, msi->bus, msi->devfn));
if ( !pdev )
return -ENODEV;
@@ -1055,7 +1056,7 @@ static int __pci_enable_msix(struct msi_
struct msi_desc *old_desc;
ASSERT(pcidevs_locked());
- pdev = pci_get_pdev(msi->seg, msi->bus, msi->devfn);
+ pdev = pci_get_pdev(NULL, PCI_SBDF(msi->seg, msi->bus, msi->devfn));
if ( !pdev || !pdev->msix )
return -ENODEV;
@@ -1146,7 +1147,7 @@ int pci_prepare_msix(u16 seg, u8 bus, u8
return 0;
pcidevs_lock();
- pdev = pci_get_pdev(seg, bus, devfn);
+ pdev = pci_get_pdev(NULL, PCI_SBDF(seg, bus, devfn));
if ( !pdev )
rc = -ENODEV;
else if ( pdev->msix->used_entries != !!off )
--- a/xen/arch/x86/pci.c
+++ b/xen/arch/x86/pci.c
@@ -90,7 +90,7 @@ int pci_conf_write_intercept(unsigned in
pcidevs_lock();
- pdev = pci_get_pdev(seg, PCI_BUS(bdf), PCI_DEVFN(bdf));
+ pdev = pci_get_pdev(NULL, PCI_SBDF(seg, bdf));
if ( pdev )
rc = pci_msi_conf_write_intercept(pdev, reg, size, data);
--- a/xen/arch/x86/physdev.c
+++ b/xen/arch/x86/physdev.c
@@ -531,7 +531,8 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_H
break;
pcidevs_lock();
- pdev = pci_get_pdev(0, restore_msi.bus, restore_msi.devfn);
+ pdev = pci_get_pdev(NULL,
+ PCI_SBDF(0, restore_msi.bus, restore_msi.devfn));
ret = pdev ? pci_restore_msi_state(pdev) : -ENODEV;
pcidevs_unlock();
break;
@@ -546,7 +547,7 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_H
break;
pcidevs_lock();
- pdev = pci_get_pdev(dev.seg, dev.bus, dev.devfn);
+ pdev = pci_get_pdev(NULL, PCI_SBDF(dev.seg, dev.bus, dev.devfn));
ret = pdev ? pci_restore_msi_state(pdev) : -ENODEV;
pcidevs_unlock();
break;
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -447,7 +447,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xe
}
pcidevs_lock();
- pdev = pci_get_pdev(dev.seg, dev.bus, dev.devfn);
+ pdev = pci_get_pdev(NULL, PCI_SBDF(dev.seg, dev.bus, dev.devfn));
if ( !pdev )
node = XEN_INVALID_DEV;
else if ( pdev->node == NUMA_NO_NODE )
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -748,8 +748,7 @@ static bool_t __init set_iommu_interrupt
}
pcidevs_lock();
- iommu->msi.dev = pci_get_pdev(iommu->seg, PCI_BUS(iommu->bdf),
- PCI_DEVFN(iommu->bdf));
+ iommu->msi.dev = pci_get_pdev(NULL, PCI_SBDF(iommu->seg, iommu->bdf));
pcidevs_unlock();
if ( !iommu->msi.dev )
{
@@ -1286,7 +1285,7 @@ static int __init cf_check amd_iommu_set
if ( !pci_init )
continue;
pcidevs_lock();
- pdev = pci_get_pdev(seg, PCI_BUS(bdf), PCI_DEVFN(bdf));
+ pdev = pci_get_pdev(NULL, PCI_SBDF(seg, bdf));
pcidevs_unlock();
}
--- a/xen/drivers/passthrough/amd/iommu_map.c
+++ b/xen/drivers/passthrough/amd/iommu_map.c
@@ -727,7 +727,7 @@ int cf_check amd_iommu_get_reserved_devi
const struct pci_dev *pdev;
pcidevs_lock();
- pdev = pci_get_pdev(seg, sbdf.bus, sbdf.devfn);
+ pdev = pci_get_pdev(NULL, sbdf);
pcidevs_unlock();
if ( pdev )
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -576,23 +576,6 @@ int __init pci_ro_device(int seg, int bu
return 0;
}
-struct pci_dev *pci_get_pdev(uint16_t seg, uint8_t bus, uint8_t devfn)
-{
- const struct pci_seg *pseg = get_pseg(seg);
- struct pci_dev *pdev;
-
- ASSERT(pcidevs_locked());
-
- if ( !pseg )
- return NULL;
-
- list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
- if ( pdev->bus == bus && pdev->devfn == devfn )
- return pdev;
-
- return NULL;
-}
-
struct pci_dev *pci_get_real_pdev(int seg, int bus, int devfn)
{
struct pci_dev *pdev;
@@ -601,12 +584,12 @@ struct pci_dev *pci_get_real_pdev(int se
if ( seg < 0 || bus < 0 || devfn < 0 )
return NULL;
- for ( pdev = pci_get_pdev(seg, bus, devfn), stride = 4;
+ for ( pdev = pci_get_pdev(NULL, PCI_SBDF(seg, bus, devfn)), stride = 4;
!pdev && stride; stride >>= 1 )
{
if ( !(devfn & (8 - stride)) )
continue;
- pdev = pci_get_pdev(seg, bus, devfn & ~(8 - stride));
+ pdev = pci_get_pdev(NULL, PCI_SBDF(seg, bus, devfn & ~(8 - stride)));
if ( pdev && stride != pdev->phantom_stride )
pdev = NULL;
}
@@ -614,32 +597,33 @@ struct pci_dev *pci_get_real_pdev(int se
return pdev;
}
-struct pci_dev *pci_get_pdev_by_domain(const struct domain *d, uint16_t seg,
- uint8_t bus, uint8_t devfn)
+struct pci_dev *pci_get_pdev(const struct domain *d, pci_sbdf_t sbdf)
{
struct pci_dev *pdev;
+ ASSERT(d || pcidevs_locked());
+
/*
* The hardware domain owns the majority of the devices in the system.
* When there are multiple segments, traversing the per-segment list is
* likely going to be faster, whereas for a single segment the difference
* shouldn't be that large.
*/
- if ( is_hardware_domain(d) )
+ if ( !d || is_hardware_domain(d) )
{
- const struct pci_seg *pseg = get_pseg(seg);
+ const struct pci_seg *pseg = get_pseg(sbdf.seg);
if ( !pseg )
return NULL;
list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
- if ( pdev->bus == bus && pdev->devfn == devfn &&
- pdev->domain == d )
+ if ( pdev->sbdf.bdf == sbdf.bdf &&
+ (!d || pdev->domain == d) )
return pdev;
}
else
list_for_each_entry ( pdev, &d->pdev_list, domain_list )
- if ( pdev->bus == bus && pdev->devfn == devfn )
+ if ( pdev->sbdf.bdf == sbdf.bdf )
return pdev;
return NULL;
@@ -746,7 +730,9 @@ int pci_add_device(u16 seg, u8 bus, u8 d
else if ( info->is_virtfn )
{
pcidevs_lock();
- pdev = pci_get_pdev(seg, info->physfn.bus, info->physfn.devfn);
+ pdev = pci_get_pdev(NULL,
+ PCI_SBDF(seg, info->physfn.bus,
+ info->physfn.devfn));
if ( pdev )
pf_is_extfn = pdev->info.is_extfn;
pcidevs_unlock();
@@ -924,7 +910,7 @@ static int deassign_device(struct domain
return -EINVAL;
ASSERT(pcidevs_locked());
- pdev = pci_get_pdev_by_domain(d, seg, bus, devfn);
+ pdev = pci_get_pdev(d, PCI_SBDF(seg, bus, devfn));
if ( !pdev )
return -ENODEV;
@@ -1201,7 +1187,8 @@ static int __hwdom_init cf_check _setup_
{
for ( devfn = 0; devfn < 256; devfn++ )
{
- struct pci_dev *pdev = pci_get_pdev(pseg->nr, bus, devfn);
+ struct pci_dev *pdev = pci_get_pdev(NULL,
+ PCI_SBDF(pseg->nr, bus, devfn));
if ( !pdev )
continue;
@@ -1475,7 +1462,7 @@ static int device_assigned(u16 seg, u8 b
int rc = 0;
ASSERT(pcidevs_locked());
- pdev = pci_get_pdev(seg, bus, devfn);
+ pdev = pci_get_pdev(NULL, PCI_SBDF(seg, bus, devfn));
if ( !pdev )
rc = -ENODEV;
@@ -1506,7 +1493,7 @@ static int assign_device(struct domain *
/* device_assigned() should already have cleared the device for assignment */
ASSERT(pcidevs_locked());
- pdev = pci_get_pdev(seg, bus, devfn);
+ pdev = pci_get_pdev(NULL, PCI_SBDF(seg, bus, devfn));
ASSERT(pdev && (pdev->domain == hardware_domain ||
pdev->domain == dom_io));
--- a/xen/drivers/passthrough/vtd/quirks.c
+++ b/xen/drivers/passthrough/vtd/quirks.c
@@ -417,7 +417,7 @@ static int __must_check map_me_phantom_f
int rc;
/* find ME VT-d engine base on a real ME device */
- pdev = pci_get_pdev(0, 0, PCI_DEVFN(dev, 0));
+ pdev = pci_get_pdev(NULL, PCI_SBDF(0, 0, dev, 0));
drhd = acpi_find_matched_drhd_unit(pdev);
/* map or unmap ME phantom function */
--- a/xen/drivers/video/vga.c
+++ b/xen/drivers/video/vga.c
@@ -118,7 +118,7 @@ void __init video_endboot(void)
u8 b = bus, df = devfn, sb;
pcidevs_lock();
- pdev = pci_get_pdev(0, bus, devfn);
+ pdev = pci_get_pdev(NULL, PCI_SBDF(0, bus, devfn));
pcidevs_unlock();
if ( !pdev ||
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -325,7 +325,7 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsi
}
/* Find the PCI dev matching the address. */
- pdev = pci_get_pdev_by_domain(d, sbdf.seg, sbdf.bus, sbdf.devfn);
+ pdev = pci_get_pdev(d, sbdf);
if ( !pdev )
return vpci_read_hw(sbdf, reg, size);
@@ -435,7 +435,7 @@ void vpci_write(pci_sbdf_t sbdf, unsigne
* Find the PCI dev matching the address.
* Passthrough everything that's not trapped.
*/
- pdev = pci_get_pdev_by_domain(d, sbdf.seg, sbdf.bus, sbdf.devfn);
+ pdev = pci_get_pdev(d, sbdf);
if ( !pdev )
{
vpci_write_hw(sbdf, reg, size, data);
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -177,10 +177,8 @@ int pci_add_device(u16 seg, u8 bus, u8 d
int pci_remove_device(u16 seg, u8 bus, u8 devfn);
int pci_ro_device(int seg, int bus, int devfn);
int pci_hide_device(unsigned int seg, unsigned int bus, unsigned int devfn);
-struct pci_dev *pci_get_pdev(uint16_t seg, uint8_t bus, uint8_t devfn);
+struct pci_dev *pci_get_pdev(const struct domain *d, pci_sbdf_t sbdf);
struct pci_dev *pci_get_real_pdev(int seg, int bus, int devfn);
-struct pci_dev *pci_get_pdev_by_domain(const struct domain *, uint16_t seg,
- uint8_t bus, uint8_t devfn);
void pci_check_disable_device(u16 seg, u8 bus, u8 devfn);
uint8_t pci_conf_read8(pci_sbdf_t sbdf, unsigned int reg);
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 3/3] PCI: bring pci_get_real_pdev() in line with pci_get_pdev()
2022-08-11 10:50 [PATCH v2 0/3] PCI: re-work pci_get_pdev() and friends Jan Beulich
2022-08-11 10:51 ` [PATCH v2 1/3] PCI: simplify (and thus correct) pci_get_pdev{,_by_domain}() Jan Beulich
2022-08-11 10:52 ` [PATCH v2 2/3] PCI: fold pci_get_pdev{,_by_domain}() Jan Beulich
@ 2022-08-11 10:52 ` Jan Beulich
2022-08-11 13:28 ` Andrew Cooper
2022-08-11 16:16 ` Rahul Singh
2022-08-11 16:37 ` [PATCH] x86/msi: Switch msi_info to using pci_sbdf_t Andrew Cooper
3 siblings, 2 replies; 17+ messages in thread
From: Jan Beulich @ 2022-08-11 10:52 UTC (permalink / raw)
To: xen-devel@lists.xenproject.org
Cc: Paul Durrant, Roger Pau Monné, Andrew Cooper, Rahul Singh
Fold the three parameters into a single pci_sbdf_t one.
No functional change intended, despite the "(8 - stride)" ->
"stride" replacement (not really sure why it was written the more
complicated way originally).
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: New.
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -639,8 +639,7 @@ static void cf_check parse_ppr_log_entry
struct pci_dev *pdev;
pcidevs_lock();
- pdev = pci_get_real_pdev(iommu->seg, PCI_BUS(device_id),
- PCI_DEVFN(device_id));
+ pdev = pci_get_real_pdev(PCI_SBDF(iommu->seg, device_id));
pcidevs_unlock();
if ( pdev )
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -576,20 +576,18 @@ int __init pci_ro_device(int seg, int bu
return 0;
}
-struct pci_dev *pci_get_real_pdev(int seg, int bus, int devfn)
+struct pci_dev *pci_get_real_pdev(pci_sbdf_t sbdf)
{
struct pci_dev *pdev;
int stride;
- if ( seg < 0 || bus < 0 || devfn < 0 )
- return NULL;
-
- for ( pdev = pci_get_pdev(NULL, PCI_SBDF(seg, bus, devfn)), stride = 4;
+ for ( pdev = pci_get_pdev(NULL, sbdf), stride = 4;
!pdev && stride; stride >>= 1 )
{
- if ( !(devfn & (8 - stride)) )
+ if ( !(sbdf.devfn & stride) )
continue;
- pdev = pci_get_pdev(NULL, PCI_SBDF(seg, bus, devfn & ~(8 - stride)));
+ sbdf.devfn &= ~stride;
+ pdev = pci_get_pdev(NULL, sbdf);
if ( pdev && stride != pdev->phantom_stride )
pdev = NULL;
}
@@ -1074,7 +1072,7 @@ void pci_check_disable_device(u16 seg, u
u16 cword;
pcidevs_lock();
- pdev = pci_get_real_pdev(seg, bus, devfn);
+ pdev = pci_get_real_pdev(PCI_SBDF(seg, bus, devfn));
if ( pdev )
{
if ( now < pdev->fault.time ||
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -178,7 +178,7 @@ int pci_remove_device(u16 seg, u8 bus, u
int pci_ro_device(int seg, int bus, int devfn);
int pci_hide_device(unsigned int seg, unsigned int bus, unsigned int devfn);
struct pci_dev *pci_get_pdev(const struct domain *d, pci_sbdf_t sbdf);
-struct pci_dev *pci_get_real_pdev(int seg, int bus, int devfn);
+struct pci_dev *pci_get_real_pdev(pci_sbdf_t sbdf);
void pci_check_disable_device(u16 seg, u8 bus, u8 devfn);
uint8_t pci_conf_read8(pci_sbdf_t sbdf, unsigned int reg);
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/3] PCI: simplify (and thus correct) pci_get_pdev{,_by_domain}()
2022-08-11 10:51 ` [PATCH v2 1/3] PCI: simplify (and thus correct) pci_get_pdev{,_by_domain}() Jan Beulich
@ 2022-08-11 13:11 ` Andrew Cooper
2022-08-11 13:21 ` Jan Beulich
2022-08-11 16:15 ` Rahul Singh
1 sibling, 1 reply; 17+ messages in thread
From: Andrew Cooper @ 2022-08-11 13:11 UTC (permalink / raw)
To: Jan Beulich, xen-devel@lists.xenproject.org
Cc: Paul Durrant, Roger Pau Monne, Rahul Singh
On 11/08/2022 11:51, Jan Beulich wrote:
> The last "wildcard" use of either function went away with f591755823a7
> ("IOMMU/PCI: don't let domain cleanup continue when device de-assignment
> failed"). Don't allow them to be called this way anymore. Besides
> simplifying the code this also fixes two bugs:
>
> 1) When seg != -1, the outer loops should have been terminated after the
> first iteration, or else a device with the same BDF but on another
> segment could be found / returned.
>
> Reported-by: Rahul Singh <rahul.singh@arm.com>
>
> 2) When seg == -1 calling get_pseg() is bogus. The function (taking a
> u16) would look for segment 0xffff, which might exist. If it exists,
> we might then find / return a wrong device.
>
> In pci_get_pdev_by_domain() also switch from using the per-segment list
> to using the per-domain one, with the exception of the hardware domain
> (see the code comment there).
>
> While there also constify "pseg" and drop "pdev"'s already previously
> unnecessary initializer.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
I'm not totally convinced that special casing hwdom is right, because
quarantine devices are domio not hwdom. But I also can't identify a
case where it's definitely wrong either.
> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -177,10 +177,10 @@ int pci_add_device(u16 seg, u8 bus, u8 d
> int pci_remove_device(u16 seg, u8 bus, u8 devfn);
> int pci_ro_device(int seg, int bus, int devfn);
> int pci_hide_device(unsigned int seg, unsigned int bus, unsigned int devfn);
> -struct pci_dev *pci_get_pdev(int seg, int bus, int devfn);
> +struct pci_dev *pci_get_pdev(uint16_t seg, uint8_t bus, uint8_t devfn);
I was going to make a request, but I can't quite get it to compile...
Passing sbdf as 3 parameters is a waste, and it would be great if we
could take this opportunity to improve.
Sadly,
-struct pci_dev *pci_get_pdev(uint16_t seg, uint8_t bus, uint8_t devfn);
+struct pci_dev *pci_get_pdev(pci_sbdf_t sbdf);
+
+#define pci_get_pdev(...) \
+ ({ \
+ count_args(__VA_ARGS__) == 1 \
+ ? pci_get_pdev(__VA_ARGS__) \
+ : pci_get_pdev(PCI_SBDF(__VA_ARGS__)); \
+ })
this doesn't quite compile as a transition plan, and I'm stuck for
further ideas.
~Andrew
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/3] PCI: fold pci_get_pdev{,_by_domain}()
2022-08-11 10:52 ` [PATCH v2 2/3] PCI: fold pci_get_pdev{,_by_domain}() Jan Beulich
@ 2022-08-11 13:21 ` Andrew Cooper
2022-08-11 13:26 ` Jan Beulich
2022-08-11 16:17 ` Rahul Singh
1 sibling, 1 reply; 17+ messages in thread
From: Andrew Cooper @ 2022-08-11 13:21 UTC (permalink / raw)
To: Jan Beulich, xen-devel@lists.xenproject.org
Cc: Paul Durrant, Roger Pau Monne, Rahul Singh, George Dunlap,
Julien Grall, Stefano Stabellini, Wei Liu
On 11/08/2022 11:52, Jan Beulich wrote:
> Rename the latter, subsuming the functionality of the former when passed
> NULL as first argument.
>
> Since this requires touching all call sites anyway, take the opportunity
> and fold the remaining three parameters into a single pci_sbdf_t one.
>
> No functional change intended. In particular the locking related
> assertion needs to continue to be kept silent when a non-NULL domain
> pointer is passed - both vpci_read() and vpci_write() call the function
> without holding the lock (adding respective locking to vPCI [or finding
> an alternative to doing so] is the topic of a separate series).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: New.
>
> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -2162,7 +2162,7 @@ int map_domain_pirq(
> if ( !cpu_has_apic )
> goto done;
>
> - pdev = pci_get_pdev_by_domain(d, msi->seg, msi->bus, msi->devfn);
> + pdev = pci_get_pdev(d, PCI_SBDF(msi->seg, msi->bus, msi->devfn));
Oh, I should really have read this patch before trying to do the sbdf
conversion in patch 1.
However, it occurs to me that this:
diff --git a/xen/arch/x86/include/asm/msi.h b/xen/arch/x86/include/asm/msi.h
index 117379318f2c..6f0ab845017c 100644
--- a/xen/arch/x86/include/asm/msi.h
+++ b/xen/arch/x86/include/asm/msi.h
@@ -59,9 +59,14 @@
#define FIX_MSIX_MAX_PAGES 512
struct msi_info {
- u16 seg;
- u8 bus;
- u8 devfn;
+ union {
+ struct {
+ u8 devfn;
+ u8 bus;
+ u16 seg;
+ };
+ pci_sbdf_t sbdf;
+ };
int irq;
int entry_nr;
uint64_t table_base;
will simplify several hunks in this patch, because you can just pass
msi->sbdf rather than reconstructing it by reversing 32 bits worth of
data from their in-memory representation.
Preferably with something to this effect included, Reviewed-by: Andrew
Cooper <andrew.cooper3@citrix.com>
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/3] PCI: simplify (and thus correct) pci_get_pdev{,_by_domain}()
2022-08-11 13:11 ` Andrew Cooper
@ 2022-08-11 13:21 ` Jan Beulich
0 siblings, 0 replies; 17+ messages in thread
From: Jan Beulich @ 2022-08-11 13:21 UTC (permalink / raw)
To: Andrew Cooper
Cc: Paul Durrant, Roger Pau Monne, Rahul Singh,
xen-devel@lists.xenproject.org
On 11.08.2022 15:11, Andrew Cooper wrote:
> On 11/08/2022 11:51, Jan Beulich wrote:
>> The last "wildcard" use of either function went away with f591755823a7
>> ("IOMMU/PCI: don't let domain cleanup continue when device de-assignment
>> failed"). Don't allow them to be called this way anymore. Besides
>> simplifying the code this also fixes two bugs:
>>
>> 1) When seg != -1, the outer loops should have been terminated after the
>> first iteration, or else a device with the same BDF but on another
>> segment could be found / returned.
>>
>> Reported-by: Rahul Singh <rahul.singh@arm.com>
>>
>> 2) When seg == -1 calling get_pseg() is bogus. The function (taking a
>> u16) would look for segment 0xffff, which might exist. If it exists,
>> we might then find / return a wrong device.
>>
>> In pci_get_pdev_by_domain() also switch from using the per-segment list
>> to using the per-domain one, with the exception of the hardware domain
>> (see the code comment there).
>>
>> While there also constify "pseg" and drop "pdev"'s already previously
>> unnecessary initializer.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Thanks.
> I'm not totally convinced that special casing hwdom is right, because
> quarantine devices are domio not hwdom. But I also can't identify a
> case where it's definitely wrong either.
>
>> --- a/xen/include/xen/pci.h
>> +++ b/xen/include/xen/pci.h
>> @@ -177,10 +177,10 @@ int pci_add_device(u16 seg, u8 bus, u8 d
>> int pci_remove_device(u16 seg, u8 bus, u8 devfn);
>> int pci_ro_device(int seg, int bus, int devfn);
>> int pci_hide_device(unsigned int seg, unsigned int bus, unsigned int devfn);
>> -struct pci_dev *pci_get_pdev(int seg, int bus, int devfn);
>> +struct pci_dev *pci_get_pdev(uint16_t seg, uint8_t bus, uint8_t devfn);
>
> I was going to make a request, but I can't quite get it to compile...
>
> Passing sbdf as 3 parameters is a waste, and it would be great if we
> could take this opportunity to improve.
>
> Sadly,
>
> -struct pci_dev *pci_get_pdev(uint16_t seg, uint8_t bus, uint8_t devfn);
> +struct pci_dev *pci_get_pdev(pci_sbdf_t sbdf);
> +
> +#define pci_get_pdev(...) \
> + ({ \
> + count_args(__VA_ARGS__) == 1 \
> + ? pci_get_pdev(__VA_ARGS__) \
> + : pci_get_pdev(PCI_SBDF(__VA_ARGS__)); \
> + })
>
> this doesn't quite compile as a transition plan, and I'm stuck for
> further ideas.
Just look at patch 2.
Jan
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/3] PCI: fold pci_get_pdev{,_by_domain}()
2022-08-11 13:21 ` Andrew Cooper
@ 2022-08-11 13:26 ` Jan Beulich
2022-08-11 15:09 ` Andrew Cooper
0 siblings, 1 reply; 17+ messages in thread
From: Jan Beulich @ 2022-08-11 13:26 UTC (permalink / raw)
To: Andrew Cooper
Cc: Paul Durrant, Roger Pau Monne, Rahul Singh, George Dunlap,
Julien Grall, Stefano Stabellini, Wei Liu,
xen-devel@lists.xenproject.org
On 11.08.2022 15:21, Andrew Cooper wrote:
> On 11/08/2022 11:52, Jan Beulich wrote:
>> --- a/xen/arch/x86/irq.c
>> +++ b/xen/arch/x86/irq.c
>> @@ -2162,7 +2162,7 @@ int map_domain_pirq(
>> if ( !cpu_has_apic )
>> goto done;
>>
>> - pdev = pci_get_pdev_by_domain(d, msi->seg, msi->bus, msi->devfn);
>> + pdev = pci_get_pdev(d, PCI_SBDF(msi->seg, msi->bus, msi->devfn));
>
> Oh, I should really have read this patch before trying to do the sbdf
> conversion in patch 1.
>
> However, it occurs to me that this:
>
> diff --git a/xen/arch/x86/include/asm/msi.h b/xen/arch/x86/include/asm/msi.h
> index 117379318f2c..6f0ab845017c 100644
> --- a/xen/arch/x86/include/asm/msi.h
> +++ b/xen/arch/x86/include/asm/msi.h
> @@ -59,9 +59,14 @@
> #define FIX_MSIX_MAX_PAGES 512
>
> struct msi_info {
> - u16 seg;
> - u8 bus;
> - u8 devfn;
> + union {
> + struct {
> + u8 devfn;
> + u8 bus;
> + u16 seg;
> + };
> + pci_sbdf_t sbdf;
> + };
> int irq;
> int entry_nr;
> uint64_t table_base;
>
> will simplify several hunks in this patch, because you can just pass
> msi->sbdf rather than reconstructing it by reversing 32 bits worth of
> data from their in-memory representation.
No, I'm strictly against introducing a 2nd instance of such aliasing
(we already have one in struct pci_dev, and that's bad enough). There
could be _only_ an "sbdf" field here, yes, but that'll have knock-on
effects elsewhere, so wants to be a separate change. And there are far
more places where imo we'll want to use pci_sbdf_t.
> Preferably with something to this effect included, Reviewed-by: Andrew
> Cooper <andrew.cooper3@citrix.com>
Thanks.
Jan
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 3/3] PCI: bring pci_get_real_pdev() in line with pci_get_pdev()
2022-08-11 10:52 ` [PATCH v2 3/3] PCI: bring pci_get_real_pdev() in line with pci_get_pdev() Jan Beulich
@ 2022-08-11 13:28 ` Andrew Cooper
2022-08-11 16:16 ` Rahul Singh
1 sibling, 0 replies; 17+ messages in thread
From: Andrew Cooper @ 2022-08-11 13:28 UTC (permalink / raw)
To: Jan Beulich, xen-devel@lists.xenproject.org
Cc: Paul Durrant, Roger Pau Monne, Rahul Singh
On 11/08/2022 11:52, Jan Beulich wrote:
> Fold the three parameters into a single pci_sbdf_t one.
>
> No functional change intended, despite the "(8 - stride)" ->
> "stride" replacement (not really sure why it was written the more
> complicated way originally).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/3] PCI: fold pci_get_pdev{,_by_domain}()
2022-08-11 13:26 ` Jan Beulich
@ 2022-08-11 15:09 ` Andrew Cooper
2022-08-11 15:41 ` Jan Beulich
0 siblings, 1 reply; 17+ messages in thread
From: Andrew Cooper @ 2022-08-11 15:09 UTC (permalink / raw)
To: Jan Beulich
Cc: Paul Durrant, Roger Pau Monne, Rahul Singh, George Dunlap,
Julien Grall, Stefano Stabellini, Wei Liu,
xen-devel@lists.xenproject.org
On 11/08/2022 14:26, Jan Beulich wrote:
> On 11.08.2022 15:21, Andrew Cooper wrote:
>> On 11/08/2022 11:52, Jan Beulich wrote:
>>> --- a/xen/arch/x86/irq.c
>>> +++ b/xen/arch/x86/irq.c
>>> @@ -2162,7 +2162,7 @@ int map_domain_pirq(
>>> if ( !cpu_has_apic )
>>> goto done;
>>>
>>> - pdev = pci_get_pdev_by_domain(d, msi->seg, msi->bus, msi->devfn);
>>> + pdev = pci_get_pdev(d, PCI_SBDF(msi->seg, msi->bus, msi->devfn));
>> Oh, I should really have read this patch before trying to do the sbdf
>> conversion in patch 1.
>>
>> However, it occurs to me that this:
>>
>> diff --git a/xen/arch/x86/include/asm/msi.h b/xen/arch/x86/include/asm/msi.h
>> index 117379318f2c..6f0ab845017c 100644
>> --- a/xen/arch/x86/include/asm/msi.h
>> +++ b/xen/arch/x86/include/asm/msi.h
>> @@ -59,9 +59,14 @@
>> #define FIX_MSIX_MAX_PAGES 512
>>
>> struct msi_info {
>> - u16 seg;
>> - u8 bus;
>> - u8 devfn;
>> + union {
>> + struct {
>> + u8 devfn;
>> + u8 bus;
>> + u16 seg;
>> + };
>> + pci_sbdf_t sbdf;
>> + };
>> int irq;
>> int entry_nr;
>> uint64_t table_base;
>>
>> will simplify several hunks in this patch, because you can just pass
>> msi->sbdf rather than reconstructing it by reversing 32 bits worth of
>> data from their in-memory representation.
> No, I'm strictly against introducing a 2nd instance of such aliasing
> (we already have one in struct pci_dev, and that's bad enough). There
> could be _only_ an "sbdf" field here, yes, but that'll have knock-on
> effects elsewhere, so wants to be a separate change. And there are far
> more places where imo we'll want to use pci_sbdf_t.
What's the likelihood of getting to that before 4.17 goes out?
I'd prefer to see it fixed, and obviously even more conversion to sbdf_t
is better.
Basically, I'm happy for the conversion to not be in this patch, as long
it's not going to get forgotten.
~Andrew
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/3] PCI: fold pci_get_pdev{,_by_domain}()
2022-08-11 15:09 ` Andrew Cooper
@ 2022-08-11 15:41 ` Jan Beulich
0 siblings, 0 replies; 17+ messages in thread
From: Jan Beulich @ 2022-08-11 15:41 UTC (permalink / raw)
To: Andrew Cooper
Cc: Paul Durrant, Roger Pau Monne, Rahul Singh, George Dunlap,
Julien Grall, Stefano Stabellini, Wei Liu,
xen-devel@lists.xenproject.org
On 11.08.2022 17:09, Andrew Cooper wrote:
> On 11/08/2022 14:26, Jan Beulich wrote:
>> On 11.08.2022 15:21, Andrew Cooper wrote:
>>> On 11/08/2022 11:52, Jan Beulich wrote:
>>>> --- a/xen/arch/x86/irq.c
>>>> +++ b/xen/arch/x86/irq.c
>>>> @@ -2162,7 +2162,7 @@ int map_domain_pirq(
>>>> if ( !cpu_has_apic )
>>>> goto done;
>>>>
>>>> - pdev = pci_get_pdev_by_domain(d, msi->seg, msi->bus, msi->devfn);
>>>> + pdev = pci_get_pdev(d, PCI_SBDF(msi->seg, msi->bus, msi->devfn));
>>> Oh, I should really have read this patch before trying to do the sbdf
>>> conversion in patch 1.
>>>
>>> However, it occurs to me that this:
>>>
>>> diff --git a/xen/arch/x86/include/asm/msi.h b/xen/arch/x86/include/asm/msi.h
>>> index 117379318f2c..6f0ab845017c 100644
>>> --- a/xen/arch/x86/include/asm/msi.h
>>> +++ b/xen/arch/x86/include/asm/msi.h
>>> @@ -59,9 +59,14 @@
>>> #define FIX_MSIX_MAX_PAGES 512
>>>
>>> struct msi_info {
>>> - u16 seg;
>>> - u8 bus;
>>> - u8 devfn;
>>> + union {
>>> + struct {
>>> + u8 devfn;
>>> + u8 bus;
>>> + u16 seg;
>>> + };
>>> + pci_sbdf_t sbdf;
>>> + };
>>> int irq;
>>> int entry_nr;
>>> uint64_t table_base;
>>>
>>> will simplify several hunks in this patch, because you can just pass
>>> msi->sbdf rather than reconstructing it by reversing 32 bits worth of
>>> data from their in-memory representation.
>> No, I'm strictly against introducing a 2nd instance of such aliasing
>> (we already have one in struct pci_dev, and that's bad enough). There
>> could be _only_ an "sbdf" field here, yes, but that'll have knock-on
>> effects elsewhere, so wants to be a separate change. And there are far
>> more places where imo we'll want to use pci_sbdf_t.
>
> What's the likelihood of getting to that before 4.17 goes out?
Well, I can try to get to making a patch tomorrow, just in time to meet
the submission deadline. But that's not really a promise ...
Jan
> I'd prefer to see it fixed, and obviously even more conversion to sbdf_t
> is better.
>
> Basically, I'm happy for the conversion to not be in this patch, as long
> it's not going to get forgotten.
>
> ~Andrew
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/3] PCI: simplify (and thus correct) pci_get_pdev{,_by_domain}()
2022-08-11 10:51 ` [PATCH v2 1/3] PCI: simplify (and thus correct) pci_get_pdev{,_by_domain}() Jan Beulich
2022-08-11 13:11 ` Andrew Cooper
@ 2022-08-11 16:15 ` Rahul Singh
1 sibling, 0 replies; 17+ messages in thread
From: Rahul Singh @ 2022-08-11 16:15 UTC (permalink / raw)
To: Jan Beulich
Cc: xen-devel@lists.xenproject.org, Paul Durrant,
Roger Pau Monné, Andrew Cooper
Hi Jan,
> On 11 Aug 2022, at 11:51 am, Jan Beulich <jbeulich@suse.com> wrote:
>
> The last "wildcard" use of either function went away with f591755823a7
> ("IOMMU/PCI: don't let domain cleanup continue when device de-assignment
> failed"). Don't allow them to be called this way anymore. Besides
> simplifying the code this also fixes two bugs:
>
> 1) When seg != -1, the outer loops should have been terminated after the
> first iteration, or else a device with the same BDF but on another
> segment could be found / returned.
>
> Reported-by: Rahul Singh <rahul.singh@arm.com>
>
> 2) When seg == -1 calling get_pseg() is bogus. The function (taking a
> u16) would look for segment 0xffff, which might exist. If it exists,
> we might then find / return a wrong device.
>
> In pci_get_pdev_by_domain() also switch from using the per-segment list
> to using the per-domain one, with the exception of the hardware domain
> (see the code comment there).
>
> While there also constify "pseg" and drop "pdev"'s already previously
> unnecessary initializer.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
I tested the patch series on ARM and it works as expected.
Reviewed-by: Rahul Singh <rahul.singh@arm.com>
Tested-by: Rahul Singh <rahul.singh@arm.com>
Regards,
Rahul
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 3/3] PCI: bring pci_get_real_pdev() in line with pci_get_pdev()
2022-08-11 10:52 ` [PATCH v2 3/3] PCI: bring pci_get_real_pdev() in line with pci_get_pdev() Jan Beulich
2022-08-11 13:28 ` Andrew Cooper
@ 2022-08-11 16:16 ` Rahul Singh
1 sibling, 0 replies; 17+ messages in thread
From: Rahul Singh @ 2022-08-11 16:16 UTC (permalink / raw)
To: Jan Beulich
Cc: xen-devel@lists.xenproject.org, Paul Durrant,
Roger Pau Monné, Andrew Cooper
Hi Jan,
> On 11 Aug 2022, at 11:52 am, Jan Beulich <jbeulich@suse.com> wrote:
>
> Fold the three parameters into a single pci_sbdf_t one.
>
> No functional change intended, despite the "(8 - stride)" ->
> "stride" replacement (not really sure why it was written the more
> complicated way originally).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Rahul Singh <rahul.singh@arm.com>
Tested-by: Rahul Singh <rahul.singh@arm.com>
Regards,
Rahul
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/3] PCI: fold pci_get_pdev{,_by_domain}()
2022-08-11 10:52 ` [PATCH v2 2/3] PCI: fold pci_get_pdev{,_by_domain}() Jan Beulich
2022-08-11 13:21 ` Andrew Cooper
@ 2022-08-11 16:17 ` Rahul Singh
1 sibling, 0 replies; 17+ messages in thread
From: Rahul Singh @ 2022-08-11 16:17 UTC (permalink / raw)
To: Jan Beulich
Cc: xen-devel@lists.xenproject.org, Paul Durrant,
Roger Pau Monné, Andrew Cooper, George Dunlap, Julien Grall,
Stefano Stabellini, Wei Liu
Hi Jan,
> On 11 Aug 2022, at 11:52 am, Jan Beulich <jbeulich@suse.com> wrote:
>
> Rename the latter, subsuming the functionality of the former when passed
> NULL as first argument.
>
> Since this requires touching all call sites anyway, take the opportunity
> and fold the remaining three parameters into a single pci_sbdf_t one.
>
> No functional change intended. In particular the locking related
> assertion needs to continue to be kept silent when a non-NULL domain
> pointer is passed - both vpci_read() and vpci_write() call the function
> without holding the lock (adding respective locking to vPCI [or finding
> an alternative to doing so] is the topic of a separate series).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Rahul Singh <rahul.singh@arm.com>
Tested-by: Rahul Singh <rahul.singh@arm.com>
Regards,
Rahul
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] x86/msi: Switch msi_info to using pci_sbdf_t
2022-08-11 10:50 [PATCH v2 0/3] PCI: re-work pci_get_pdev() and friends Jan Beulich
` (2 preceding siblings ...)
2022-08-11 10:52 ` [PATCH v2 3/3] PCI: bring pci_get_real_pdev() in line with pci_get_pdev() Jan Beulich
@ 2022-08-11 16:37 ` Andrew Cooper
2022-08-12 6:45 ` Jan Beulich
3 siblings, 1 reply; 17+ messages in thread
From: Andrew Cooper @ 2022-08-11 16:37 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné, Wei Liu
This reorders the fields in msi_info, but removes all the under-the-hood
parameter shuffling required to call pci_get_pdev().
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
---
xen/arch/x86/hvm/vmsi.c | 4 +---
xen/arch/x86/include/asm/msi.h | 4 +---
xen/arch/x86/irq.c | 2 +-
xen/arch/x86/msi.c | 4 ++--
xen/arch/x86/physdev.c | 10 +++++-----
xen/drivers/char/ns16550.c | 4 ++--
xen/xsm/flask/hooks.c | 2 +-
7 files changed, 13 insertions(+), 17 deletions(-)
diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
index 198fbd67084b..75f92885dc5e 100644
--- a/xen/arch/x86/hvm/vmsi.c
+++ b/xen/arch/x86/hvm/vmsi.c
@@ -750,9 +750,7 @@ static int vpci_msi_enable(const struct pci_dev *pdev, unsigned int nr,
paddr_t table_base)
{
struct msi_info msi_info = {
- .seg = pdev->seg,
- .bus = pdev->bus,
- .devfn = pdev->devfn,
+ .sbdf = pdev->sbdf,
.table_base = table_base,
.entry_nr = nr,
};
diff --git a/xen/arch/x86/include/asm/msi.h b/xen/arch/x86/include/asm/msi.h
index 117379318f2c..fe670895eed2 100644
--- a/xen/arch/x86/include/asm/msi.h
+++ b/xen/arch/x86/include/asm/msi.h
@@ -59,9 +59,7 @@
#define FIX_MSIX_MAX_PAGES 512
struct msi_info {
- u16 seg;
- u8 bus;
- u8 devfn;
+ pci_sbdf_t sbdf;
int irq;
int entry_nr;
uint64_t table_base;
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index bf8b52d1114e..cd0c8a30a864 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -2162,7 +2162,7 @@ int map_domain_pirq(
if ( !cpu_has_apic )
goto done;
- pdev = pci_get_pdev(d, PCI_SBDF(msi->seg, msi->bus, msi->devfn));
+ pdev = pci_get_pdev(d, msi->sbdf);
if ( !pdev )
goto done;
diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
index 62c4fbcfbe55..d0bf63df1def 100644
--- a/xen/arch/x86/msi.c
+++ b/xen/arch/x86/msi.c
@@ -1001,7 +1001,7 @@ static int __pci_enable_msi(struct msi_info *msi, struct msi_desc **desc)
struct msi_desc *old_desc;
ASSERT(pcidevs_locked());
- pdev = pci_get_pdev(NULL, PCI_SBDF(msi->seg, msi->bus, msi->devfn));
+ pdev = pci_get_pdev(NULL, msi->sbdf);
if ( !pdev )
return -ENODEV;
@@ -1056,7 +1056,7 @@ static int __pci_enable_msix(struct msi_info *msi, struct msi_desc **desc)
struct msi_desc *old_desc;
ASSERT(pcidevs_locked());
- pdev = pci_get_pdev(NULL, PCI_SBDF(msi->seg, msi->bus, msi->devfn));
+ pdev = pci_get_pdev(NULL, msi->sbdf);
if ( !pdev || !pdev->msix )
return -ENODEV;
diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
index 3a50d9d0c969..2f1d955a96bd 100644
--- a/xen/arch/x86/physdev.c
+++ b/xen/arch/x86/physdev.c
@@ -312,21 +312,21 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
{
case MAP_PIRQ_TYPE_MSI_SEG:
map.type = MAP_PIRQ_TYPE_MSI;
- msi.seg = map.bus >> 16;
+ msi.sbdf.seg = map.bus >> 16;
break;
case MAP_PIRQ_TYPE_MULTI_MSI:
if ( map.table_base )
return -EINVAL;
- msi.seg = map.bus >> 16;
+ msi.sbdf.seg = map.bus >> 16;
break;
default:
- msi.seg = 0;
+ msi.sbdf.seg = 0;
break;
}
- msi.bus = map.bus;
- msi.devfn = map.devfn;
+ msi.sbdf.bus = map.bus;
+ msi.sbdf.devfn = map.devfn;
msi.entry_nr = map.entry_nr;
msi.table_base = map.table_base;
ret = physdev_map_pirq(map.domid, map.type, &map.index, &map.pirq,
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index cd3573e43df3..01a05c9aa859 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -435,8 +435,8 @@ static void __init cf_check ns16550_init_postirq(struct serial_port *port)
if ( uart->msi )
{
struct msi_info msi = {
- .bus = uart->ps_bdf[0],
- .devfn = PCI_DEVFN(uart->ps_bdf[1], uart->ps_bdf[2]),
+ .sbdf = PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1],
+ uart->ps_bdf[2]),
.irq = rc = uart->irq,
.entry_nr = 1
};
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 8c9cd0f2972d..8bd56644efe4 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -918,7 +918,7 @@ static int flask_map_domain_msi (
{
#ifdef CONFIG_HAS_PCI_MSI
const struct msi_info *msi = data;
- uint32_t machine_bdf = (msi->seg << 16) | (msi->bus << 8) | msi->devfn;
+ uint32_t machine_bdf = msi->sbdf.sbdf;
AVC_AUDIT_DATA_INIT(ad, DEV);
ad->device = machine_bdf;
--
2.11.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] x86/msi: Switch msi_info to using pci_sbdf_t
2022-08-11 16:37 ` [PATCH] x86/msi: Switch msi_info to using pci_sbdf_t Andrew Cooper
@ 2022-08-12 6:45 ` Jan Beulich
2022-08-12 10:22 ` Andrew Cooper
0 siblings, 1 reply; 17+ messages in thread
From: Jan Beulich @ 2022-08-12 6:45 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Wei Liu, Xen-devel
On 11.08.2022 18:37, Andrew Cooper wrote:
> This reorders the fields in msi_info, but removes all the under-the-hood
> parameter shuffling required to call pci_get_pdev().
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Oh, you've made the requested change yourself - thanks!
Reviewed-by: Jan Beulich <jbeulich@suse.com>
While not the primary goal as per the description, I'm particularly happy
to see ...
> --- a/xen/xsm/flask/hooks.c
> +++ b/xen/xsm/flask/hooks.c
> @@ -918,7 +918,7 @@ static int flask_map_domain_msi (
> {
> #ifdef CONFIG_HAS_PCI_MSI
> const struct msi_info *msi = data;
> - uint32_t machine_bdf = (msi->seg << 16) | (msi->bus << 8) | msi->devfn;
> + uint32_t machine_bdf = msi->sbdf.sbdf;
... this open-coding go away.
Jan
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] x86/msi: Switch msi_info to using pci_sbdf_t
2022-08-12 6:45 ` Jan Beulich
@ 2022-08-12 10:22 ` Andrew Cooper
0 siblings, 0 replies; 17+ messages in thread
From: Andrew Cooper @ 2022-08-12 10:22 UTC (permalink / raw)
To: Jan Beulich; +Cc: Roger Pau Monne, Wei Liu, Xen-devel
On 12/08/2022 07:45, Jan Beulich wrote:
> On 11.08.2022 18:37, Andrew Cooper wrote:
>> This reorders the fields in msi_info, but removes all the under-the-hood
>> parameter shuffling required to call pci_get_pdev().
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Oh, you've made the requested change yourself - thanks!
I looked at the code and decided it was simple enough.
While doing it, it became clear that uart->ps_bdf[0..2] is in desperate
need too, but that was more complicated than I had time for.
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
Thanks.
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2022-08-12 10:22 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-11 10:50 [PATCH v2 0/3] PCI: re-work pci_get_pdev() and friends Jan Beulich
2022-08-11 10:51 ` [PATCH v2 1/3] PCI: simplify (and thus correct) pci_get_pdev{,_by_domain}() Jan Beulich
2022-08-11 13:11 ` Andrew Cooper
2022-08-11 13:21 ` Jan Beulich
2022-08-11 16:15 ` Rahul Singh
2022-08-11 10:52 ` [PATCH v2 2/3] PCI: fold pci_get_pdev{,_by_domain}() Jan Beulich
2022-08-11 13:21 ` Andrew Cooper
2022-08-11 13:26 ` Jan Beulich
2022-08-11 15:09 ` Andrew Cooper
2022-08-11 15:41 ` Jan Beulich
2022-08-11 16:17 ` Rahul Singh
2022-08-11 10:52 ` [PATCH v2 3/3] PCI: bring pci_get_real_pdev() in line with pci_get_pdev() Jan Beulich
2022-08-11 13:28 ` Andrew Cooper
2022-08-11 16:16 ` Rahul Singh
2022-08-11 16:37 ` [PATCH] x86/msi: Switch msi_info to using pci_sbdf_t Andrew Cooper
2022-08-12 6:45 ` Jan Beulich
2022-08-12 10:22 ` Andrew Cooper
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.