* [PATCH 1/5] pci: Introduce parse_pci_sbdf{_seg}()
[not found] <cover.1779116255.git.teddy.astie@vates.tech>
@ 2026-05-18 15:21 ` Teddy Astie
2026-05-20 2:39 ` dmukhin
2026-05-18 15:21 ` [PATCH 2/5] vtd: Use pci_sbdf_t in acpi_parse_dev_scope() Teddy Astie
` (4 subsequent siblings)
5 siblings, 1 reply; 23+ messages in thread
From: Teddy Astie @ 2026-05-18 15:21 UTC (permalink / raw)
To: xen-devel
Cc: Teddy Astie, Andrew Cooper, Anthony PERARD, Michal Orzel,
Jan Beulich, Julien Grall, Roger Pau Monné,
Stefano Stabellini
[-- Attachment #1: Type: text/plain, Size: 2031 bytes --]
In many places, we're parsing a PCI string into individual parts
(seg, bus, dev, fn) and then transform it into a pci_sbdf_t using PCI_SBDF
macro. Rather than converting from parts to pci_sbdf_t and vice versa,
introduce a new function that parses a PCI string into a pci_sbdf_t structure
directly.
Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
---
xen/drivers/pci/pci.c | 18 ++++++++++++++++++
xen/include/xen/pci.h | 3 +++
2 files changed, 21 insertions(+)
diff --git a/xen/drivers/pci/pci.c b/xen/drivers/pci/pci.c
index 084be3880c..1d06cb035b 100644
--- a/xen/drivers/pci/pci.c
+++ b/xen/drivers/pci/pci.c
@@ -202,3 +202,21 @@ const char *__init parse_pci_seg(const char *s, unsigned int *seg_p,
return s;
}
+
+const char *parse_pci_sbdf(const char *s, pci_sbdf_t *sbdf)
+{
+ unsigned int seg, bus, dev, func;
+ const char *out = parse_pci(s, &seg, &bus, &dev, &func);
+
+ *sbdf = PCI_SBDF(seg, bus, dev, func);
+ return out;
+}
+
+const char *parse_pci_sbdf_seg(const char *s, pci_sbdf_t *sbdf, bool *def_seg)
+{
+ unsigned int seg, bus, dev, func;
+ const char *out = parse_pci_seg(s, &seg, &bus, &dev, &func, def_seg);
+
+ *sbdf = PCI_SBDF(seg, bus, dev, func);
+ return out;
+}
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index afb6bbf50d..7bfc59cd75 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -280,6 +280,9 @@ const char *parse_pci_seg(const char *s, unsigned int *seg_p,
unsigned int *bus_p, unsigned int *dev_p,
unsigned int *func_p, bool *def_seg);
+const char *parse_pci_sbdf(const char *s, pci_sbdf_t *sbdf);
+const char *parse_pci_sbdf_seg(const char *s, pci_sbdf_t *sbdf, bool *def_seg);
+
#define PCI_BAR_VF (1u << 0)
#define PCI_BAR_LAST (1u << 1)
#define PCI_BAR_ROM (1u << 2)
--
2.52.0
--
Teddy Astie | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 2/5] vtd: Use pci_sbdf_t in acpi_parse_dev_scope()
[not found] <cover.1779116255.git.teddy.astie@vates.tech>
2026-05-18 15:21 ` [PATCH 1/5] pci: Introduce parse_pci_sbdf{_seg}() Teddy Astie
@ 2026-05-18 15:21 ` Teddy Astie
2026-05-20 3:00 ` dmukhin
2026-05-18 15:21 ` [PATCH 3/5] pci: Use pci_sbdf_t in pci_device_detect() Teddy Astie
` (3 subsequent siblings)
5 siblings, 1 reply; 23+ messages in thread
From: Teddy Astie @ 2026-05-18 15:21 UTC (permalink / raw)
To: xen-devel; +Cc: Teddy Astie, Jan Beulich, Andrew Cooper, Roger Pau Monné
[-- Attachment #1: Type: text/plain, Size: 5828 bytes --]
Use a dedicated pci_sbdf_t struct that we update instead of recreating
one each time we need it.
Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
---
xen/drivers/passthrough/vtd/dmar.c | 42 ++++++++++++------------------
1 file changed, 16 insertions(+), 26 deletions(-)
diff --git a/xen/drivers/passthrough/vtd/dmar.c b/xen/drivers/passthrough/vtd/dmar.c
index 2a756831a6..c36f4bbd7b 100644
--- a/xen/drivers/passthrough/vtd/dmar.c
+++ b/xen/drivers/passthrough/vtd/dmar.c
@@ -310,7 +310,7 @@ static int __init acpi_parse_dev_scope(
{
struct acpi_ioapic_unit *acpi_ioapic_unit;
const struct acpi_dmar_device_scope *acpi_scope;
- u16 bus, sub_bus, sec_bus;
+ u16 sub_bus, sec_bus;
const struct acpi_dmar_pci_path *path;
struct acpi_drhd_unit *drhd = type == DMAR_TYPE ?
container_of(scope, struct acpi_drhd_unit, scope) : NULL;
@@ -332,29 +332,26 @@ static int __init acpi_parse_dev_scope(
while ( start < end )
{
+ pci_sbdf_t dev_sbdf;
acpi_scope = start;
path = (const void *)(acpi_scope + 1);
depth = (acpi_scope->length - sizeof(*acpi_scope)) / sizeof(*path);
- bus = acpi_scope->bus;
+ dev_sbdf = PCI_SBDF(seg, acpi_scope->bus, path->dev, path->fn);
while ( --depth > 0 )
{
- bus = pci_conf_read8(PCI_SBDF(seg, bus, path->dev, path->fn),
- PCI_SECONDARY_BUS);
+ dev_sbdf.bus = pci_conf_read8(dev_sbdf, PCI_SECONDARY_BUS);
path++;
}
switch ( acpi_scope->entry_type )
{
case ACPI_DMAR_SCOPE_TYPE_BRIDGE:
- sec_bus = pci_conf_read8(PCI_SBDF(seg, bus, path->dev, path->fn),
- PCI_SECONDARY_BUS);
- sub_bus = pci_conf_read8(PCI_SBDF(seg, bus, path->dev, path->fn),
- PCI_SUBORDINATE_BUS);
+ sec_bus = pci_conf_read8(dev_sbdf, PCI_SECONDARY_BUS);
+ sub_bus = pci_conf_read8(dev_sbdf, PCI_SUBORDINATE_BUS);
if ( iommu_verbose )
printk(VTDPREFIX " bridge: %pp start=%x sec=%x sub=%x\n",
- &PCI_SBDF(seg, bus, path->dev, path->fn),
- acpi_scope->bus, sec_bus, sub_bus);
+ &dev_sbdf, acpi_scope->bus, sec_bus, sub_bus);
dmar_scope_add_buses(scope, sec_bus, sub_bus);
gfx_only = false;
@@ -362,8 +359,7 @@ static int __init acpi_parse_dev_scope(
case ACPI_DMAR_SCOPE_TYPE_HPET:
if ( iommu_verbose )
- printk(VTDPREFIX " MSI HPET: %pp\n",
- &PCI_SBDF(seg, bus, path->dev, path->fn));
+ printk(VTDPREFIX " MSI HPET: %pp\n", &dev_sbdf);
if ( drhd )
{
@@ -374,9 +370,7 @@ static int __init acpi_parse_dev_scope(
if ( !acpi_hpet_unit )
goto out;
acpi_hpet_unit->id = acpi_scope->enumeration_id;
- acpi_hpet_unit->bus = bus;
- acpi_hpet_unit->dev = path->dev;
- acpi_hpet_unit->func = path->fn;
+ acpi_hpet_unit->bdf = dev_sbdf.bdf;
list_add(&acpi_hpet_unit->list, &drhd->hpet_list);
gfx_only = false;
@@ -386,16 +380,15 @@ static int __init acpi_parse_dev_scope(
case ACPI_DMAR_SCOPE_TYPE_ENDPOINT:
if ( iommu_verbose )
- printk(VTDPREFIX " endpoint: %pp\n",
- &PCI_SBDF(seg, bus, path->dev, path->fn));
+ printk(VTDPREFIX " endpoint: %pp\n", &dev_sbdf);
- if ( drhd && pci_device_detect(seg, bus, path->dev, path->fn) )
+ if ( drhd && pci_device_detect(seg, dev_sbdf.bus, dev_sbdf.dev, dev_sbdf.fn) )
{
- if ( pci_conf_read8(PCI_SBDF(seg, bus, path->dev, path->fn),
+ if ( pci_conf_read8(dev_sbdf,
PCI_CLASS_DEVICE + 1) != 0x03
/* PCI_BASE_CLASS_DISPLAY */ )
gfx_only = false;
- else if ( !seg && !bus && path->dev == 2 && !path->fn )
+ else if ( !seg && !dev_sbdf.bus && path->dev == 2 && !path->fn )
igd_drhd_address = drhd->address;
}
@@ -403,8 +396,7 @@ static int __init acpi_parse_dev_scope(
case ACPI_DMAR_SCOPE_TYPE_IOAPIC:
if ( iommu_verbose )
- printk(VTDPREFIX " IOAPIC: %pp\n",
- &PCI_SBDF(seg, bus, path->dev, path->fn));
+ printk(VTDPREFIX " IOAPIC: %pp\n", &dev_sbdf);
if ( drhd )
{
@@ -413,9 +405,7 @@ static int __init acpi_parse_dev_scope(
if ( !acpi_ioapic_unit )
goto out;
acpi_ioapic_unit->apic_id = acpi_scope->enumeration_id;
- acpi_ioapic_unit->ioapic.bdf.bus = bus;
- acpi_ioapic_unit->ioapic.bdf.dev = path->dev;
- acpi_ioapic_unit->ioapic.bdf.func = path->fn;
+ acpi_ioapic_unit->ioapic.info = dev_sbdf.bdf;
list_add(&acpi_ioapic_unit->list, &drhd->ioapic_list);
gfx_only = false;
@@ -431,7 +421,7 @@ static int __init acpi_parse_dev_scope(
gfx_only = false;
continue;
}
- scope->devices[didx++] = PCI_BDF(bus, path->dev, path->fn);
+ scope->devices[didx++] = dev_sbdf.bdf;
start += acpi_scope->length;
}
--
2.52.0
--
Teddy Astie | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 3/5] pci: Use pci_sbdf_t in pci_device_detect()
[not found] <cover.1779116255.git.teddy.astie@vates.tech>
2026-05-18 15:21 ` [PATCH 1/5] pci: Introduce parse_pci_sbdf{_seg}() Teddy Astie
2026-05-18 15:21 ` [PATCH 2/5] vtd: Use pci_sbdf_t in acpi_parse_dev_scope() Teddy Astie
@ 2026-05-18 15:21 ` Teddy Astie
2026-05-20 3:21 ` dmukhin
2026-05-18 15:21 ` [PATCH 4/5] pci: Parse into pci_sbdf_t directly Teddy Astie
` (2 subsequent siblings)
5 siblings, 1 reply; 23+ messages in thread
From: Teddy Astie @ 2026-05-18 15:21 UTC (permalink / raw)
To: xen-devel
Cc: Teddy Astie, Andrew Cooper, Anthony PERARD, Michal Orzel,
Jan Beulich, Julien Grall, Roger Pau Monné,
Stefano Stabellini
[-- Attachment #1: Type: text/plain, Size: 10153 bytes --]
Use a single pci_sbdf_t instead of each of its part as individual parameters.
Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
---
xen/drivers/char/ehci-dbgp.c | 35 ++++++++++++++----------------
xen/drivers/passthrough/pci.c | 16 +++++++-------
xen/drivers/passthrough/vtd/dmar.c | 33 +++++++++++-----------------
xen/include/xen/pci.h | 2 +-
4 files changed, 38 insertions(+), 48 deletions(-)
diff --git a/xen/drivers/char/ehci-dbgp.c b/xen/drivers/char/ehci-dbgp.c
index a5c79f56fc..39a047eb3f 100644
--- a/xen/drivers/char/ehci-dbgp.c
+++ b/xen/drivers/char/ehci-dbgp.c
@@ -681,16 +681,14 @@ static int dbgp_control_msg(struct ehci_dbgp *dbgp, unsigned int devnum,
return ret;
}
-static unsigned int __init __find_dbgp(u8 bus, u8 slot, u8 func)
+static unsigned int __init __find_dbgp(pci_sbdf_t sbdf)
{
- uint32_t class = pci_conf_read32(PCI_SBDF(0, bus, slot, func),
- PCI_CLASS_REVISION);
+ uint32_t class = pci_conf_read32(sbdf, PCI_CLASS_REVISION);
if ( (class >> 8) != PCI_CLASS_SERIAL_USB_EHCI )
return 0;
- return pci_find_cap_offset(PCI_SBDF(0, bus, slot, func),
- PCI_CAP_ID_EHCI_DEBUG);
+ return pci_find_cap_offset(sbdf, PCI_CAP_ID_EHCI_DEBUG);
}
static unsigned int __init find_dbgp(struct ehci_dbgp *dbgp,
@@ -704,20 +702,20 @@ static unsigned int __init find_dbgp(struct ehci_dbgp *dbgp,
{
for ( func = 0; func < 8; func++ )
{
+ pci_sbdf_t sbdf = PCI_SBDF(0, bus, slot, func);
unsigned int cap;
- if ( !pci_device_detect(0, bus, slot, func) )
+ if ( !pci_device_detect(sbdf) )
{
if ( !func )
break;
continue;
}
- cap = __find_dbgp(bus, slot, func);
+ cap = __find_dbgp(sbdf);
if ( !cap || ehci_num-- )
{
- if ( !func && !(pci_conf_read8(PCI_SBDF(0, bus, slot, func),
- PCI_HEADER_TYPE) & 0x80) )
+ if ( !func && !(pci_conf_read8(sbdf, PCI_HEADER_TYPE) & 0x80) )
break;
continue;
}
@@ -1510,25 +1508,24 @@ void __init ehci_dbgp_init(void)
}
else if ( strncmp(opt_dbgp + 4, "@pci", 4) == 0 )
{
- unsigned int bus, slot, func;
-
- e = parse_pci(opt_dbgp + 8, NULL, &bus, &slot, &func);
+ pci_sbdf_t sbdf = PCI_SBDF(0, 0, 0, 0);
+
+ e = parse_pci_sbdf(opt_dbgp + 8, &sbdf);
if ( !e || *e )
return;
- dbgp->bus = bus;
- dbgp->slot = slot;
- dbgp->func = func;
+ dbgp->bus = sbdf.bus;
+ dbgp->slot = sbdf.dev;
+ dbgp->func = sbdf.fn;
- if ( !pci_device_detect(0, bus, slot, func) )
+ if ( !pci_device_detect(sbdf) )
return;
- dbgp->cap = __find_dbgp(bus, slot, func);
+ dbgp->cap = __find_dbgp(sbdf);
if ( !dbgp->cap )
return;
- dbgp_printk("Using EHCI debug port on %02x:%02x.%u\n",
- bus, slot, func);
+ dbgp_printk("Using EHCI debug port on %pp\n", &sbdf);
}
else
return;
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 464bb0fee4..7b2898bd5a 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -1166,11 +1166,11 @@ out:
return ret;
}
-bool __init pci_device_detect(u16 seg, u8 bus, u8 dev, u8 func)
+bool __init pci_device_detect(pci_sbdf_t sbdf)
{
u32 vendor;
- vendor = pci_conf_read32(PCI_SBDF(seg, bus, dev, func), PCI_VENDOR_ID);
+ vendor = pci_conf_read32(sbdf, PCI_VENDOR_ID);
/* some broken boards return 0 or ~0 if a slot is empty: */
if ( (vendor == 0xffffffffU) || (vendor == 0x00000000U) ||
(vendor == 0x0000ffffU) || (vendor == 0xffff0000U) )
@@ -1221,24 +1221,24 @@ static int __init cf_check _scan_pci_devices(struct pci_seg *pseg, void *arg)
{
for ( func = 0; func < 8; func++ )
{
- if ( !pci_device_detect(pseg->nr, bus, dev, func) )
+ pci_sbdf_t sbdf = PCI_SBDF(pseg->nr, bus, dev, func);
+
+ if ( !pci_device_detect(sbdf) )
{
if ( !func )
break;
continue;
}
- pdev = alloc_pdev(pseg, bus, PCI_DEVFN(dev, func));
+ pdev = alloc_pdev(pseg, bus, sbdf.devfn);
if ( !pdev )
{
printk(XENLOG_WARNING "%pp: alloc_pdev failed\n",
- &PCI_SBDF(pseg->nr, bus, dev, func));
+ &sbdf);
return -ENOMEM;
}
- if ( !func && !(pci_conf_read8(PCI_SBDF(pseg->nr, bus, dev,
- func),
- PCI_HEADER_TYPE) & 0x80) )
+ if ( !func && !(pci_conf_read8(sbdf, PCI_HEADER_TYPE) & 0x80) )
break;
}
}
diff --git a/xen/drivers/passthrough/vtd/dmar.c b/xen/drivers/passthrough/vtd/dmar.c
index c36f4bbd7b..9f9b639eba 100644
--- a/xen/drivers/passthrough/vtd/dmar.c
+++ b/xen/drivers/passthrough/vtd/dmar.c
@@ -382,7 +382,7 @@ static int __init acpi_parse_dev_scope(
if ( iommu_verbose )
printk(VTDPREFIX " endpoint: %pp\n", &dev_sbdf);
- if ( drhd && pci_device_detect(seg, dev_sbdf.bus, dev_sbdf.dev, dev_sbdf.fn) )
+ if ( drhd && pci_device_detect(dev_sbdf) )
{
if ( pci_conf_read8(dev_sbdf,
PCI_CLASS_DEVICE + 1) != 0x03
@@ -505,7 +505,6 @@ acpi_parse_one_drhd(struct acpi_dmar_header *header)
acpi_register_drhd_unit(dmaru);
else
{
- u8 b, d, f;
unsigned int i = 0;
union {
const void *raw;
@@ -519,18 +518,16 @@ acpi_parse_one_drhd(struct acpi_dmar_header *header)
for ( p.raw = dev_scope_start; i < dmaru->scope.devices_cnt;
i++, p.raw += p.scope->length )
{
+ pci_sbdf_t sbdf = PCI_SBDF(drhd->segment, dmaru->scope.devices[i]);
+
if ( p.scope->entry_type == ACPI_DMAR_SCOPE_TYPE_IOAPIC ||
p.scope->entry_type == ACPI_DMAR_SCOPE_TYPE_HPET )
continue;
- b = PCI_BUS(dmaru->scope.devices[i]);
- d = PCI_SLOT(dmaru->scope.devices[i]);
- f = PCI_FUNC(dmaru->scope.devices[i]);
-
- if ( !pci_device_detect(drhd->segment, b, d, f) )
+ if ( !pci_device_detect(sbdf) )
printk(XENLOG_WARNING VTDPREFIX
" Non-existent device (%pp) in this DRHD's scope!\n",
- &PCI_SBDF(drhd->segment, b, d, f));
+ &sbdf);
}
acpi_register_drhd_unit(dmaru);
@@ -559,17 +556,14 @@ static int __init register_one_rmrr(struct acpi_rmrr_unit *rmrru)
for ( ; i < rmrru->scope.devices_cnt; i++ )
{
- u8 b = PCI_BUS(rmrru->scope.devices[i]);
- u8 d = PCI_SLOT(rmrru->scope.devices[i]);
- u8 f = PCI_FUNC(rmrru->scope.devices[i]);
+ pci_sbdf_t sbdf = PCI_SBDF(rmrru->segment, rmrru->scope.devices[i]);
- if ( pci_device_detect(rmrru->segment, b, d, f) == 0 )
+ if ( pci_device_detect(sbdf) == 0 )
{
dprintk(XENLOG_WARNING VTDPREFIX,
" Non-existent device (%pp) is reported"
" in RMRR [%"PRIx64", %"PRIx64"]'s scope!\n",
- &PCI_SBDF(rmrru->segment, b, d, f),
- rmrru->base_address, rmrru->end_address);
+ &sbdf, rmrru->base_address, rmrru->end_address);
ignore = true;
}
else
@@ -753,15 +747,13 @@ static int __init register_one_satc(struct acpi_satc_unit *satcu)
for ( ; i < satcu->scope.devices_cnt; i++ )
{
- uint8_t b = PCI_BUS(satcu->scope.devices[i]);
- uint8_t d = PCI_SLOT(satcu->scope.devices[i]);
- uint8_t f = PCI_FUNC(satcu->scope.devices[i]);
+ pci_sbdf_t sbdf = PCI_SBDF(satcu->segment, satcu->scope.devices[i]);
- if ( !pci_device_detect(satcu->segment, b, d, f) )
+ if ( !pci_device_detect(sbdf) )
{
dprintk(XENLOG_WARNING VTDPREFIX,
" Non-existent device (%pp) is reported in SATC scope!\n",
- &PCI_SBDF(satcu->segment, b, d, f));
+ &sbdf);
ignore = true;
}
else
@@ -1184,8 +1176,9 @@ int cf_check intel_iommu_get_reserved_device_memory(
static int __init cf_check parse_rmrr_param(const char *str)
{
const char *s = str, *cur, *stmp;
- unsigned int seg, bus, dev, func, dev_count;
+ unsigned int dev_count;
unsigned long start, end;
+ pci_sbdf_t sbdf;
do {
if ( nr_rmrr >= MAX_USER_RMRR )
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index 7bfc59cd75..d816dcad05 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -218,7 +218,7 @@ static always_inline bool pcidevs_trylock(void)
#endif
bool pci_known_segment(u16 seg);
-bool pci_device_detect(u16 seg, u8 bus, u8 dev, u8 func);
+bool pci_device_detect(pci_sbdf_t sbdf);
int scan_pci_devices(void);
enum pdev_type pdev_type(u16 seg, u8 bus, u8 devfn);
int find_upstream_bridge(u16 seg, u8 *bus, u8 *devfn, u8 *secbus);
--
2.52.0
--
Teddy Astie | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 4/5] pci: Parse into pci_sbdf_t directly
[not found] <cover.1779116255.git.teddy.astie@vates.tech>
` (2 preceding siblings ...)
2026-05-18 15:21 ` [PATCH 3/5] pci: Use pci_sbdf_t in pci_device_detect() Teddy Astie
@ 2026-05-18 15:21 ` Teddy Astie
2026-05-20 3:31 ` dmukhin
2026-05-18 15:21 ` [PATCH 5/5] RFC: pci: Migrate pci_mmcfg_{read,write} to pci.c Teddy Astie
2026-05-18 17:20 ` [PATCH 0/5] Small PCI refactoring Teddy Astie
5 siblings, 1 reply; 23+ messages in thread
From: Teddy Astie @ 2026-05-18 15:21 UTC (permalink / raw)
To: xen-devel
Cc: Teddy Astie, Andrew Cooper, Anthony PERARD, Michal Orzel,
Jan Beulich, Julien Grall, Roger Pau Monné,
Stefano Stabellini, Jason Andryuk
[-- Attachment #1: Type: text/plain, Size: 8442 bytes --]
Use the newly introduced parse_pci_sbdf() and parse_pci_sbdf_seg() in order
to parse into a pci_sbdf_t directly instead of reconstructing it afterward.
Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
---
xen/drivers/char/ns16550.c | 24 +++++++++++-----------
xen/drivers/char/xhci-dbc.c | 6 +++---
xen/drivers/passthrough/amd/iommu_acpi.c | 26 ++++++++++++------------
xen/drivers/passthrough/vtd/dmar.c | 7 +++----
4 files changed, 31 insertions(+), 32 deletions(-)
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 878da27f2e..fa2d0e5991 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -1572,22 +1572,22 @@ static bool __init parse_positional(struct ns16550 *uart, char **str)
#ifdef CONFIG_HAS_PCI
if ( *conf == ',' && *++conf != ',' )
{
- unsigned int b, d, f;
+ pci_sbdf_t sbdf;
- conf = parse_pci(conf, NULL, &b, &d, &f);
+ conf = parse_pci_sbdf(conf, &sbdf);
if ( !conf )
PARSE_ERR_RET("Bad port PCI coordinates");
- uart->pci_device = PCI_SBDF(0, b, d, f);
+ uart->pci_device = sbdf;
uart->ps_bdf_enable = true;
}
if ( *conf == ',' && *++conf != ',' )
{
- unsigned int b, d, f;
+ pci_sbdf_t sbdf;
- if ( !parse_pci(conf, NULL, &b, &d, &f) )
+ if ( !parse_pci_sbdf(conf, &sbdf) )
PARSE_ERR_RET("Bad bridge PCI coordinates");
- uart->pci_bridge = PCI_SBDF(0, b, d, f);
+ uart->pci_bridge = sbdf;
uart->pb_bdf_enable = true;
}
#endif
@@ -1671,22 +1671,22 @@ static bool __init parse_namevalue_pairs(char *str, struct ns16550 *uart)
case port_bdf:
{
- unsigned int b, d, f;
+ pci_sbdf_t sbdf;
- if ( !parse_pci(param_value, NULL, &b, &d, &f) )
+ if ( !parse_pci_sbdf(param_value, &sbdf) )
PARSE_ERR_RET("Bad port PCI coordinates\n");
- uart->pci_device = PCI_SBDF(0, b, d, f);
+ uart->pci_device = sbdf;
uart->ps_bdf_enable = true;
break;
}
case bridge_bdf:
{
- unsigned int b, d, f;
+ pci_sbdf_t sbdf;
- if ( !parse_pci(param_value, NULL, &b, &d, &f) )
+ if ( !parse_pci_sbdf(param_value, &sbdf) )
PARSE_ERR_RET("Bad bridge PCI coordinates\n");
- uart->pci_bridge = PCI_SBDF(0, b, d, f);
+ uart->pci_bridge = sbdf;
uart->pb_bdf_enable = true;
break;
}
diff --git a/xen/drivers/char/xhci-dbc.c b/xen/drivers/char/xhci-dbc.c
index c1ff528de6..c7fd554be0 100644
--- a/xen/drivers/char/xhci-dbc.c
+++ b/xen/drivers/char/xhci-dbc.c
@@ -1357,9 +1357,9 @@ static int __init cf_check xhci_parse_dbgp(const char *opt_dbgp)
}
else if ( strncmp(opt_dbgp + 4, "@pci", 4) == 0 )
{
- unsigned int bus, slot, func;
+ pci_sbdf_t sbdf;
- e = parse_pci(opt_dbgp + 8, NULL, &bus, &slot, &func);
+ e = parse_pci_sbdf(opt_dbgp + 8, &sbdf);
if ( !e || (*e && *e != ',') )
{
printk(XENLOG_ERR
@@ -1368,7 +1368,7 @@ static int __init cf_check xhci_parse_dbgp(const char *opt_dbgp)
return -EINVAL;
}
- dbc->sbdf = PCI_SBDF(0, bus, slot, func);
+ dbc->sbdf = sbdf;
}
opt = e;
diff --git a/xen/drivers/passthrough/amd/iommu_acpi.c b/xen/drivers/passthrough/amd/iommu_acpi.c
index 39ae637959..7b40da33ae 100644
--- a/xen/drivers/passthrough/amd/iommu_acpi.c
+++ b/xen/drivers/passthrough/amd/iommu_acpi.c
@@ -682,8 +682,8 @@ static int __init cf_check parse_ivrs_ioapic(const char *str)
{
const char *s = str;
unsigned long id;
- unsigned int seg, bus, dev, func;
unsigned int idx;
+ pci_sbdf_t sbdf;
if ( *s != '[' )
return -EINVAL;
@@ -692,7 +692,7 @@ static int __init cf_check parse_ivrs_ioapic(const char *str)
if ( *s != ']' || *++s != '=' )
return -EINVAL;
- s = parse_pci(s + 1, &seg, &bus, &dev, &func);
+ s = parse_pci_sbdf(s + 1, &sbdf);
if ( !s || *s )
return -EINVAL;
@@ -707,7 +707,7 @@ static int __init cf_check parse_ivrs_ioapic(const char *str)
}
}
- ioapic_sbdf[idx].sbdf = PCI_SBDF(seg, bus, dev, func);
+ ioapic_sbdf[idx].sbdf = sbdf;
ioapic_sbdf[idx].id = id;
ioapic_sbdf[idx].cmdline = true;
@@ -719,7 +719,7 @@ static int __init cf_check parse_ivrs_hpet(const char *str)
{
const char *s = str;
unsigned long id;
- unsigned int seg, bus, dev, func;
+ pci_sbdf_t sbdf;
if ( *s != '[' )
return -EINVAL;
@@ -728,12 +728,12 @@ static int __init cf_check parse_ivrs_hpet(const char *str)
if ( id != (typeof(hpet_sbdf.id))id || *s != ']' || *++s != '=' )
return -EINVAL;
- s = parse_pci(s + 1, &seg, &bus, &dev, &func);
+ s = parse_pci_sbdf(s + 1, &sbdf);
if ( !s || *s )
return -EINVAL;
hpet_sbdf.id = id;
- hpet_sbdf.sbdf = PCI_SBDF(seg, bus, dev, func);
+ hpet_sbdf.sbdf = sbdf;
hpet_sbdf.init = HPET_CMDL;
return 0;
@@ -1399,13 +1399,13 @@ static int __init cf_check parse_ivmd_param(const char *s)
}
do {
- unsigned int seg, bus, dev, func;
+ pci_sbdf_t sbdf;
if ( nr_ivmd >= ARRAY_SIZE(user_ivmds) )
return -E2BIG;
- s = parse_pci(s + 1, &seg, &bus, &dev, &func);
- if ( !s || seg )
+ s = parse_pci_sbdf(s + 1, &sbdf);
+ if ( !s || sbdf.seg )
return -EINVAL;
user_ivmds[nr_ivmd].start_address = start << PAGE_SHIFT;
@@ -1413,16 +1413,16 @@ static int __init cf_check parse_ivmd_param(const char *s)
user_ivmds[nr_ivmd].header.flags = ACPI_IVMD_UNITY |
ACPI_IVMD_READ | ACPI_IVMD_WRITE;
user_ivmds[nr_ivmd].header.length = sizeof(*user_ivmds);
- user_ivmds[nr_ivmd].header.device_id = PCI_BDF(bus, dev, func);
+ user_ivmds[nr_ivmd].header.device_id = sbdf.bdf;
user_ivmds[nr_ivmd].header.type = ACPI_IVRS_TYPE_MEMORY_ONE;
if ( *s == '-' )
{
- s = parse_pci(s + 1, &seg, &bus, &dev, &func);
- if ( !s || seg )
+ s = parse_pci_sbdf(s + 1, &sbdf);
+ if ( !s || sbdf.seg )
return -EINVAL;
- user_ivmds[nr_ivmd].aux_data = PCI_BDF(bus, dev, func);
+ user_ivmds[nr_ivmd].aux_data = sbdf.bdf;
if ( user_ivmds[nr_ivmd].aux_data <
user_ivmds[nr_ivmd].header.device_id )
return -EINVAL;
diff --git a/xen/drivers/passthrough/vtd/dmar.c b/xen/drivers/passthrough/vtd/dmar.c
index 9f9b639eba..dafe1b62f6 100644
--- a/xen/drivers/passthrough/vtd/dmar.c
+++ b/xen/drivers/passthrough/vtd/dmar.c
@@ -1215,7 +1215,7 @@ static int __init cf_check parse_rmrr_param(const char *str)
do {
bool def_seg = false;
- stmp = parse_pci_seg(s + 1, &seg, &bus, &dev, &func, &def_seg);
+ stmp = parse_pci_sbdf_seg(s + 1, &sbdf, &def_seg);
if ( !stmp )
return -EINVAL;
@@ -1224,12 +1224,11 @@ static int __init cf_check parse_rmrr_param(const char *str)
* Segment will be replaced with one from first device.
*/
if ( user_rmrrs[nr_rmrr].dev_count && def_seg )
- seg = PCI_SEG(user_rmrrs[nr_rmrr].sbdf[0]);
+ sbdf.seg = PCI_SEG(user_rmrrs[nr_rmrr].sbdf[0]);
/* Keep sbdf's even if they differ and later report an error. */
dev_count = user_rmrrs[nr_rmrr].dev_count;
- user_rmrrs[nr_rmrr].sbdf[dev_count] =
- PCI_SBDF(seg, bus, dev, func).sbdf;
+ user_rmrrs[nr_rmrr].sbdf[dev_count] = sbdf.sbdf;
user_rmrrs[nr_rmrr].dev_count++;
s = stmp;
--
2.52.0
--
Teddy Astie | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 5/5] RFC: pci: Migrate pci_mmcfg_{read,write} to pci.c
[not found] <cover.1779116255.git.teddy.astie@vates.tech>
` (3 preceding siblings ...)
2026-05-18 15:21 ` [PATCH 4/5] pci: Parse into pci_sbdf_t directly Teddy Astie
@ 2026-05-18 15:21 ` Teddy Astie
2026-05-18 17:35 ` Andrew Cooper
2026-05-18 17:20 ` [PATCH 0/5] Small PCI refactoring Teddy Astie
5 siblings, 1 reply; 23+ messages in thread
From: Teddy Astie @ 2026-05-18 15:21 UTC (permalink / raw)
To: xen-devel
Cc: Teddy Astie, Jan Beulich, Andrew Cooper, Roger Pau Monné,
Anthony PERARD, Michal Orzel, Julien Grall, Stefano Stabellini
[-- Attachment #1: Type: text/plain, Size: 15829 bytes --]
Key parts of MMCFG access bits are in mmconfig_64.c (in particular
pci_mmcfg_{read,write}()) while PCI configuration primitives (used accross the
codebase) are in pci.c.
This leads to situations where the compiler cannot optimize the `switch (len)`
for MMCFG access for all pci_conf_{read,write}N(), because they are not from
the same file.
Move the pci_mmcfg_{read,write} in pci.c and hint the compiler to inline these
functions such that it's more likely that the compiler eliminates the
`switch (len)``.
Also take the opportunity to migrate to pci_sbdf_t to reduce the parameter count
and drop many parameter domains checks.
On GCC 16.1, this leads to codegen where pci_conf_{read,write}N() doesn't call
pci_mmcfg_{read,write}() anymore and directly perform the MMIO RW.
<pci_conf_read32>:
55 push %rbp
48 89 e5 mov %rsp,%rbp
53 push %rbx
89 f8 mov %edi,%eax
89 f3 mov %esi,%ebx
c1 ef 10 shr $0x10,%edi
81 fe ff 00 00 00 cmp $0xff,%esi
77 26 ja ffff82d040301fab <pci_conf_read32+0x3a>
85 ff test %edi,%edi
75 22 jne ffff82d040301fab <pci_conf_read32+0x3a>
0f b7 f8 movzwl %ax,%edi
c1 e7 08 shl $0x8,%edi
83 e3 fc and $0xfffffffc,%ebx
09 df or %ebx,%edi
81 cf 00 00 00 80 or $0x80000000,%edi
ba 04 00 00 00 mov $0x4,%edx
be 00 00 00 00 mov $0x0,%esi
e8 2a 1c 03 00 call ffff82d040333bd3 <pci_conf_read>
eb 22 jmp ffff82d040301fcd <pci_conf_read32+0x5c>
81 fb ff 0f 00 00 cmp $0xfff,%ebx
77 24 ja ffff82d040301fd7 <pci_conf_read32+0x66>
0f b6 d0 movzbl %al,%edx
0f b6 f4 movzbl %ah,%esi
0f b7 ff movzwl %di,%edi
e8 f5 fd ff ff call ffff82d040301db6 <pci_dev_base>
48 85 c0 test %rax,%rax
74 18 je ffff82d040301fde <pci_conf_read32+0x6d>
89 db mov %ebx,%ebx
48 01 d8 add %rbx,%rax
8b 00 mov (%rax),%eax
48 8b 5d f8 mov -0x8(%rbp),%rbx
c9 leave
e9 89 12 f0 ff jmp ffff82d040203260 <__x86_return_thunk>
b8 ff ff ff ff mov $0xffffffff,%eax
eb ef jmp ffff82d040301fcd <pci_conf_read32+0x5c>
b8 ff ff ff ff mov $0xffffffff,%eax
eb e8 jmp ffff82d040301fcd <pci_conf_read32+0x5c>
Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
---
xen/arch/x86/pv/ro-page-fault.c | 3 +-
xen/arch/x86/x86_64/mmconfig.h | 43 -----------
xen/arch/x86/x86_64/mmconfig_64.c | 106 +++++---------------------
xen/arch/x86/x86_64/pci.c | 122 ++++++++++++++++++++++++++++--
xen/include/xen/pci.h | 6 +-
5 files changed, 138 insertions(+), 142 deletions(-)
diff --git a/xen/arch/x86/pv/ro-page-fault.c b/xen/arch/x86/pv/ro-page-fault.c
index d89306d34f..647041560f 100644
--- a/xen/arch/x86/pv/ro-page-fault.c
+++ b/xen/arch/x86/pv/ro-page-fault.c
@@ -331,8 +331,7 @@ static int cf_check mmcfg_intercept_write(
offset &= 0xfff;
if ( pci_conf_write_intercept(mmio_ctxt->seg, mmio_ctxt->bdf,
offset, bytes, p_data) >= 0 )
- pci_mmcfg_write(mmio_ctxt->seg, PCI_BUS(mmio_ctxt->bdf),
- PCI_DEVFN(mmio_ctxt->bdf), offset, bytes,
+ pci_mmcfg_write(PCI_SBDF(mmio_ctxt->seg, mmio_ctxt->bdf), offset, bytes,
*(uint32_t *)p_data);
return X86EMUL_OKAY;
diff --git a/xen/arch/x86/x86_64/mmconfig.h b/xen/arch/x86/x86_64/mmconfig.h
index 27c0ae5cb1..c1786a3ceb 100644
--- a/xen/arch/x86/x86_64/mmconfig.h
+++ b/xen/arch/x86/x86_64/mmconfig.h
@@ -23,49 +23,6 @@
extern unsigned int pci_probe;
-/*
- * AMD Fam10h CPUs are buggy, and cannot access MMIO config space
- * on their northbrige except through the * %eax register. As such, you MUST
- * NOT use normal IOMEM accesses, you need to only use the magic mmio-config
- * accessor functions.
- * In fact just use pci_config_*, nothing else please.
- */
-static inline unsigned char mmio_config_readb(void __iomem *pos)
-{
- u8 val;
- asm volatile("movb (%1),%%al" : "=a" (val) : "r" (pos));
- return val;
-}
-
-static inline unsigned short mmio_config_readw(void __iomem *pos)
-{
- u16 val;
- asm volatile("movw (%1),%%ax" : "=a" (val) : "r" (pos));
- return val;
-}
-
-static inline unsigned int mmio_config_readl(void __iomem *pos)
-{
- u32 val;
- asm volatile("movl (%1),%%eax" : "=a" (val) : "r" (pos));
- return val;
-}
-
-static inline void mmio_config_writeb(void __iomem *pos, u8 val)
-{
- asm volatile("movb %%al,(%1)" :: "a" (val), "r" (pos) : "memory");
-}
-
-static inline void mmio_config_writew(void __iomem *pos, u16 val)
-{
- asm volatile("movw %%ax,(%1)" :: "a" (val), "r" (pos) : "memory");
-}
-
-static inline void mmio_config_writel(void __iomem *pos, u32 val)
-{
- asm volatile("movl %%eax,(%1)" :: "a" (val), "r" (pos) : "memory");
-}
-
/* function prototypes */
struct acpi_table_header;
int cf_check acpi_parse_mcfg(struct acpi_table_header *header);
diff --git a/xen/arch/x86/x86_64/mmconfig_64.c b/xen/arch/x86/x86_64/mmconfig_64.c
index 940cf6d747..483dff9c2c 100644
--- a/xen/arch/x86/x86_64/mmconfig_64.c
+++ b/xen/arch/x86/x86_64/mmconfig_64.c
@@ -26,93 +26,6 @@ struct mmcfg_virt {
static struct mmcfg_virt *pci_mmcfg_virt;
static unsigned int mmcfg_pci_segment_shift;
-static char __iomem *get_virt(unsigned int seg, unsigned int *bus)
-{
- struct acpi_mcfg_allocation *cfg;
- int cfg_num;
-
- for (cfg_num = 0; cfg_num < pci_mmcfg_config_num; cfg_num++) {
- cfg = pci_mmcfg_virt[cfg_num].cfg;
- if (cfg->pci_segment == seg &&
- (cfg->start_bus_number <= *bus) &&
- (cfg->end_bus_number >= *bus)) {
- *bus -= cfg->start_bus_number;
- return pci_mmcfg_virt[cfg_num].virt;
- }
- }
-
- /* Fall back to type 0 */
- return NULL;
-}
-
-static char __iomem *pci_dev_base(unsigned int seg, unsigned int bus, unsigned int devfn)
-{
- char __iomem *addr;
-
- addr = get_virt(seg, &bus);
- if (!addr)
- return NULL;
- return addr + ((bus << 20) | (devfn << 12));
-}
-
-int pci_mmcfg_read(unsigned int seg, unsigned int bus,
- unsigned int devfn, int reg, int len, u32 *value)
-{
- char __iomem *addr;
-
- /* Why do we have this when nobody checks it. How about a BUG()!? -AK */
- if (unlikely((bus > 255) || (devfn > 255) || (reg > 4095))) {
-err: *value = -1;
- return -EINVAL;
- }
-
- addr = pci_dev_base(seg, bus, devfn);
- if (!addr)
- goto err;
-
- switch (len) {
- case 1:
- *value = mmio_config_readb(addr + reg);
- break;
- case 2:
- *value = mmio_config_readw(addr + reg);
- break;
- case 4:
- *value = mmio_config_readl(addr + reg);
- break;
- }
-
- return 0;
-}
-
-int pci_mmcfg_write(unsigned int seg, unsigned int bus,
- unsigned int devfn, int reg, int len, u32 value)
-{
- char __iomem *addr;
-
- /* Why do we have this when nobody checks it. How about a BUG()!? -AK */
- if (unlikely((bus > 255) || (devfn > 255) || (reg > 4095)))
- return -EINVAL;
-
- addr = pci_dev_base(seg, bus, devfn);
- if (!addr)
- return -EINVAL;
-
- switch (len) {
- case 1:
- mmio_config_writeb(addr + reg, value);
- break;
- case 2:
- mmio_config_writew(addr + reg, value);
- break;
- case 4:
- mmio_config_writel(addr + reg, value);
- break;
- }
-
- return 0;
-}
-
static void __iomem *mcfg_ioremap(const struct acpi_mcfg_allocation *cfg,
unsigned long idx, unsigned int prot)
{
@@ -133,6 +46,25 @@ static void __iomem *mcfg_ioremap(const struct acpi_mcfg_allocation *cfg,
return (void __iomem *) virt;
}
+char __iomem *pci_mmcfg_base(unsigned int seg, unsigned int *bus)
+{
+ struct acpi_mcfg_allocation *cfg;
+ int cfg_num;
+
+ for (cfg_num = 0; cfg_num < pci_mmcfg_config_num; cfg_num++) {
+ cfg = pci_mmcfg_virt[cfg_num].cfg;
+ if (cfg->pci_segment == seg &&
+ (cfg->start_bus_number <= *bus) &&
+ (cfg->end_bus_number >= *bus)) {
+ *bus -= cfg->start_bus_number;
+ return pci_mmcfg_virt[cfg_num].virt;
+ }
+ }
+
+ /* Fall back to type 0 */
+ return NULL;
+}
+
int pci_mmcfg_arch_enable(unsigned int idx)
{
const typeof(pci_mmcfg_config[0]) *cfg = pci_mmcfg_virt[idx].cfg;
diff --git a/xen/arch/x86/x86_64/pci.c b/xen/arch/x86/x86_64/pci.c
index 8d33429103..c37e3edade 100644
--- a/xen/arch/x86/x86_64/pci.c
+++ b/xen/arch/x86/x86_64/pci.c
@@ -11,13 +11,123 @@
#define PCI_CONF_ADDRESS(sbdf, reg) \
(0x80000000U | ((sbdf).bdf << 8) | ((reg) & ~3))
+/*
+ * AMD Fam10h CPUs are buggy, and cannot access MMIO config space
+ * on their northbrige except through the * %eax register. As such, you MUST
+ * NOT use normal IOMEM accesses, you need to only use the magic mmio-config
+ * accessor functions.
+ * In fact just use pci_config_*, nothing else please.
+ */
+static inline unsigned char mmio_config_readb(void __iomem *pos)
+{
+ u8 val;
+ asm volatile("movb (%1),%%al" : "=a" (val) : "r" (pos));
+ return val;
+}
+
+static inline unsigned short mmio_config_readw(void __iomem *pos)
+{
+ u16 val;
+ asm volatile("movw (%1),%%ax" : "=a" (val) : "r" (pos));
+ return val;
+}
+
+static inline unsigned int mmio_config_readl(void __iomem *pos)
+{
+ u32 val;
+ asm volatile("movl (%1),%%eax" : "=a" (val) : "r" (pos));
+ return val;
+}
+
+static inline void mmio_config_writeb(void __iomem *pos, u8 val)
+{
+ asm volatile("movb %%al,(%1)" :: "a" (val), "r" (pos) : "memory");
+}
+
+static inline void mmio_config_writew(void __iomem *pos, u16 val)
+{
+ asm volatile("movw %%ax,(%1)" :: "a" (val), "r" (pos) : "memory");
+}
+
+static inline void mmio_config_writel(void __iomem *pos, u32 val)
+{
+ asm volatile("movl %%eax,(%1)" :: "a" (val), "r" (pos) : "memory");
+}
+
+static char __iomem *pci_dev_base(unsigned int seg, unsigned int bus, unsigned int devfn)
+{
+ char __iomem *addr;
+
+ addr = pci_mmcfg_base(seg, &bus);
+ if (!addr)
+ return NULL;
+ return addr + ((bus << 20) | (devfn << 12));
+}
+
+static inline
+int pci_mmcfg_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int len, u32 *value)
+{
+ char __iomem *addr;
+
+ /* Why do we have this when nobody checks it. How about a BUG()!? -AK */
+ if (unlikely(reg > 4095)) {
+err: *value = -1;
+ return -EINVAL;
+ }
+
+ addr = pci_dev_base(sbdf.seg, sbdf.bus, sbdf.devfn);
+ if (!addr)
+ goto err;
+
+ switch (len) {
+ case 1:
+ *value = mmio_config_readb(addr + reg);
+ break;
+ case 2:
+ *value = mmio_config_readw(addr + reg);
+ break;
+ case 4:
+ *value = mmio_config_readl(addr + reg);
+ break;
+ }
+
+ return 0;
+}
+
+inline int pci_mmcfg_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int len, u32 value)
+{
+ char __iomem *addr;
+
+ /* Why do we have this when nobody checks it. How about a BUG()!? -AK */
+ if (unlikely(reg > 4095))
+ return -EINVAL;
+
+ addr = pci_dev_base(sbdf.seg, sbdf.bus, sbdf.devfn);
+ if (!addr)
+ return -EINVAL;
+
+ switch (len) {
+ case 1:
+ mmio_config_writeb(addr + reg, value);
+ break;
+ case 2:
+ mmio_config_writew(addr + reg, value);
+ break;
+ case 4:
+ mmio_config_writel(addr + reg, value);
+ break;
+ }
+
+ return 0;
+}
+
uint8_t pci_conf_read8(pci_sbdf_t sbdf, unsigned int reg)
{
uint32_t value;
if ( sbdf.seg || reg > 255 )
{
- pci_mmcfg_read(sbdf.seg, sbdf.bus, sbdf.devfn, reg, 1, &value);
+ pci_mmcfg_read(sbdf, reg, 1, &value);
return value;
}
@@ -30,7 +140,7 @@ uint16_t pci_conf_read16(pci_sbdf_t sbdf, unsigned int reg)
{
uint32_t value;
- pci_mmcfg_read(sbdf.seg, sbdf.bus, sbdf.devfn, reg, 2, &value);
+ pci_mmcfg_read(sbdf, reg, 2, &value);
return value;
}
@@ -43,7 +153,7 @@ uint32_t pci_conf_read32(pci_sbdf_t sbdf, unsigned int reg)
{
uint32_t value;
- pci_mmcfg_read(sbdf.seg, sbdf.bus, sbdf.devfn, reg, 4, &value);
+ pci_mmcfg_read(sbdf, reg, 4, &value);
return value;
}
@@ -53,7 +163,7 @@ uint32_t pci_conf_read32(pci_sbdf_t sbdf, unsigned int reg)
void pci_conf_write8(pci_sbdf_t sbdf, unsigned int reg, uint8_t data)
{
if ( sbdf.seg || reg > 255 )
- pci_mmcfg_write(sbdf.seg, sbdf.bus, sbdf.devfn, reg, 1, data);
+ pci_mmcfg_write(sbdf, reg, 1, data);
else
pci_conf_write(PCI_CONF_ADDRESS(sbdf, reg), reg & 3, 1, data);
}
@@ -61,7 +171,7 @@ void pci_conf_write8(pci_sbdf_t sbdf, unsigned int reg, uint8_t data)
void pci_conf_write16(pci_sbdf_t sbdf, unsigned int reg, uint16_t data)
{
if ( sbdf.seg || reg > 255 )
- pci_mmcfg_write(sbdf.seg, sbdf.bus, sbdf.devfn, reg, 2, data);
+ pci_mmcfg_write(sbdf, reg, 2, data);
else
pci_conf_write(PCI_CONF_ADDRESS(sbdf, reg), reg & 2, 2, data);
}
@@ -69,7 +179,7 @@ void pci_conf_write16(pci_sbdf_t sbdf, unsigned int reg, uint16_t data)
void pci_conf_write32(pci_sbdf_t sbdf, unsigned int reg, uint32_t data)
{
if ( sbdf.seg || reg > 255 )
- pci_mmcfg_write(sbdf.seg, sbdf.bus, sbdf.devfn, reg, 4, data);
+ pci_mmcfg_write(sbdf, reg, 4, data);
else
pci_conf_write(PCI_CONF_ADDRESS(sbdf, reg), 0, 4, data);
}
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index d816dcad05..b3c91fea9c 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -258,10 +258,8 @@ void pci_conf_write16(pci_sbdf_t sbdf, unsigned int reg, uint16_t data);
void pci_conf_write32(pci_sbdf_t sbdf, unsigned int reg, uint32_t data);
uint32_t pci_conf_read(uint32_t cf8, uint8_t offset, uint8_t bytes);
void pci_conf_write(uint32_t cf8, uint8_t offset, uint8_t bytes, uint32_t data);
-int pci_mmcfg_read(unsigned int seg, unsigned int bus,
- unsigned int devfn, int reg, int len, u32 *value);
-int pci_mmcfg_write(unsigned int seg, unsigned int bus,
- unsigned int devfn, int reg, int len, u32 value);
+char *pci_mmcfg_base(unsigned int seg, unsigned int *bus);
+int pci_mmcfg_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int len, u32 value);
unsigned int pci_find_cap_offset(pci_sbdf_t sbdf, unsigned int cap);
unsigned int pci_find_next_cap_ttl(pci_sbdf_t sbdf, unsigned int pos,
const unsigned int caps[], unsigned int n,
--
2.52.0
--
Teddy Astie | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 0/5] Small PCI refactoring
[not found] <cover.1779116255.git.teddy.astie@vates.tech>
` (4 preceding siblings ...)
2026-05-18 15:21 ` [PATCH 5/5] RFC: pci: Migrate pci_mmcfg_{read,write} to pci.c Teddy Astie
@ 2026-05-18 17:20 ` Teddy Astie
2026-05-19 17:41 ` Andrew Cooper
2026-05-20 3:34 ` dmukhin
5 siblings, 2 replies; 23+ messages in thread
From: Teddy Astie @ 2026-05-18 17:20 UTC (permalink / raw)
To: xen-devel
Cc: Andrew Cooper, Anthony PERARD, Michal Orzel, Jan Beulich,
Julien Grall, Roger Pau Monné, Stefano Stabellini,
Jason Andryuk
[-- Attachment #1.1.1: Type: text/plain, Size: 4524 bytes --]
Le 18/05/2026 à 17:22, Teddy Astie a écrit :
> The goal of this series is to make some refactoring of some
> pci primitives to improve codegen and make code less verbose.
>
> A big chunk of it is converting many places where (seg, bus, dev, fn)
> is split into multiples variables and convert it into being just
> pci_sbdf_t, in particular in some PCI function parameters to reduce
> parameter count which usually translate into less registers to pass
> to the function. Moreover, we also avoid translating back and forth
> between pci_sbdf_t and individual (seg, bus, dev, fn).
>
> Latest patch attempts to improve codegen of pci_conf_{read,write}N()
> by making them inline specialized variants of pci_mmcfg_{read,write}()
> in order to eliminate a particular `switch (len)` at compile time.
>
> No intended functional change, aside some parts of the codebase that will
> now correctly handle PCI segment when parsed while it was previously
> ignored (e.g dbgp).
>
> Bloat-o-meter is pretty telling
>
> add/remove: 4/2 grow/shrink: 11/14 up/down: 529/-1470 (-941)
> Function old new delta
> pci_mmcfg_base - 106 +106
> parse_pci_sbdf_seg - 88 +88
> parse_pci_sbdf - 85 +85
> pci_dev_base - 56 +56
> pci_conf_read16 92 118 +26
> pci_conf_read8 92 117 +25
> ehci_dbgp_init 629 652 +23
> pci_conf_read32 94 116 +22
> pci_conf_write32 88 109 +21
> symbols_names 106435 106452 +17
> symbols_sorted_offsets 59672 59688 +16
> pci_conf_write16 94 109 +15
> pci_conf_write8 94 108 +14
> reserve_unity_map_for_device 434 445 +11
> symbols_offsets 31920 31924 +4
> mmcfg_intercept_write 194 193 -1
> add_one_user_rmrr 653 644 -9
> __find_dbgp 87 60 -27
> pci_device_detect 89 55 -34
> pci_mmcfg_write 197 152 -45
> _scan_pci_devices 286 241 -45
> parse_ivrs_ioapic 292 235 -57
> parse_rmrr_param 484 420 -64
> register_one_rmrr 389 324 -65
> parse_ivrs_hpet 249 184 -65
> parse_ivmd_param 651 570 -81
> acpi_parse_dmar 2625 2520 -105
> get_virt 106 - -106
> pci_mmcfg_read 189 - -189
> ns16550_init 3205 3002 -203
> acpi_parse_dev_scope 1465 1091 -374
>
> Teddy Astie (5):
> pci: Introduce parse_pci_sbdf{_seg}()
> vtd: Use pci_sbdf_t in acpi_parse_dev_scope()
> pci: Use pci_sbdf_t in pci_device_detect()
> pci: Parse into pci_sbdf_t directly
> RFC: pci: Migrate pci_mmcfg_{read,write}() to pci.c
>
> xen/arch/x86/pv/ro-page-fault.c | 3 +-
> xen/arch/x86/x86_64/mmconfig.h | 43 --------
> xen/arch/x86/x86_64/mmconfig_64.c | 106 ++++----------------
> xen/arch/x86/x86_64/pci.c | 122 +++++++++++++++++++++--
> xen/drivers/char/ehci-dbgp.c | 35 +++----
> xen/drivers/char/ns16550.c | 24 ++---
> xen/drivers/char/xhci-dbc.c | 6 +-
> xen/drivers/passthrough/amd/iommu_acpi.c | 26 ++---
> xen/drivers/passthrough/pci.c | 16 +--
> xen/drivers/passthrough/vtd/dmar.c | 80 ++++++---------
> xen/drivers/pci/pci.c | 18 ++++
> xen/include/xen/pci.h | 11 +-
> 12 files changed, 243 insertions(+), 247 deletions(-)
>
There are some issues with our mail provider for the cover letter and
some whitespace issues in one of the patch, will resend tomorrow
(hopefully correctly this time).
Teddy
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 2489 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 665 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 5/5] RFC: pci: Migrate pci_mmcfg_{read,write} to pci.c
2026-05-18 15:21 ` [PATCH 5/5] RFC: pci: Migrate pci_mmcfg_{read,write} to pci.c Teddy Astie
@ 2026-05-18 17:35 ` Andrew Cooper
2026-05-19 6:02 ` Jan Beulich
2026-05-19 13:42 ` Teddy Astie
0 siblings, 2 replies; 23+ messages in thread
From: Andrew Cooper @ 2026-05-18 17:35 UTC (permalink / raw)
To: Teddy Astie, xen-devel
Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné, Anthony PERARD,
Michal Orzel, Julien Grall, Stefano Stabellini
On 18/05/2026 4:21 pm, Teddy Astie wrote:
> Key parts of MMCFG access bits are in mmconfig_64.c (in particular
> pci_mmcfg_{read,write}()) while PCI configuration primitives (used accross the
> codebase) are in pci.c.
> This leads to situations where the compiler cannot optimize the `switch (len)`
> for MMCFG access for all pci_conf_{read,write}N(), because they are not from
> the same file.
>
> Move the pci_mmcfg_{read,write} in pci.c and hint the compiler to inline these
> functions such that it's more likely that the compiler eliminates the
> `switch (len)``.
>
> Also take the opportunity to migrate to pci_sbdf_t to reduce the parameter count
> and drop many parameter domains checks.
>
> On GCC 16.1, this leads to codegen where pci_conf_{read,write}N() doesn't call
> pci_mmcfg_{read,write}() anymore and directly perform the MMIO RW.
>
> <pci_conf_read32>:
> 55 push %rbp
> 48 89 e5 mov %rsp,%rbp
> 53 push %rbx
> 89 f8 mov %edi,%eax
> 89 f3 mov %esi,%ebx
> c1 ef 10 shr $0x10,%edi
> 81 fe ff 00 00 00 cmp $0xff,%esi
> 77 26 ja ffff82d040301fab <pci_conf_read32+0x3a>
> 85 ff test %edi,%edi
> 75 22 jne ffff82d040301fab <pci_conf_read32+0x3a>
> 0f b7 f8 movzwl %ax,%edi
> c1 e7 08 shl $0x8,%edi
> 83 e3 fc and $0xfffffffc,%ebx
> 09 df or %ebx,%edi
> 81 cf 00 00 00 80 or $0x80000000,%edi
> ba 04 00 00 00 mov $0x4,%edx
> be 00 00 00 00 mov $0x0,%esi
> e8 2a 1c 03 00 call ffff82d040333bd3 <pci_conf_read>
> eb 22 jmp ffff82d040301fcd <pci_conf_read32+0x5c>
> 81 fb ff 0f 00 00 cmp $0xfff,%ebx
> 77 24 ja ffff82d040301fd7 <pci_conf_read32+0x66>
> 0f b6 d0 movzbl %al,%edx
> 0f b6 f4 movzbl %ah,%esi
> 0f b7 ff movzwl %di,%edi
> e8 f5 fd ff ff call ffff82d040301db6 <pci_dev_base>
> 48 85 c0 test %rax,%rax
> 74 18 je ffff82d040301fde <pci_conf_read32+0x6d>
> 89 db mov %ebx,%ebx
> 48 01 d8 add %rbx,%rax
> 8b 00 mov (%rax),%eax
> 48 8b 5d f8 mov -0x8(%rbp),%rbx
> c9 leave
> e9 89 12 f0 ff jmp ffff82d040203260 <__x86_return_thunk>
> b8 ff ff ff ff mov $0xffffffff,%eax
> eb ef jmp ffff82d040301fcd <pci_conf_read32+0x5c>
> b8 ff ff ff ff mov $0xffffffff,%eax
> eb e8 jmp ffff82d040301fcd <pci_conf_read32+0x5c>
This is not the whole function because it's missing a pop %rbx. Also,
right at the bottom here are the -1's from bad error paths (discussed
later).
But, this should be after the ---. Disassembly this long isn't
interesting to stay in the commit message.
> diff --git a/xen/arch/x86/x86_64/mmconfig_64.c b/xen/arch/x86/x86_64/mmconfig_64.c
> index 940cf6d747..483dff9c2c 100644
> --- a/xen/arch/x86/x86_64/mmconfig_64.c
> +++ b/xen/arch/x86/x86_64/mmconfig_64.c
> @@ -133,6 +46,25 @@ static void __iomem *mcfg_ioremap(const struct acpi_mcfg_allocation *cfg,
> return (void __iomem *) virt;
> }
>
> +char __iomem *pci_mmcfg_base(unsigned int seg, unsigned int *bus)
> +{
> + struct acpi_mcfg_allocation *cfg;
> + int cfg_num;
> +
> + for (cfg_num = 0; cfg_num < pci_mmcfg_config_num; cfg_num++) {
> + cfg = pci_mmcfg_virt[cfg_num].cfg;
> + if (cfg->pci_segment == seg &&
> + (cfg->start_bus_number <= *bus) &&
> + (cfg->end_bus_number >= *bus)) {
> + *bus -= cfg->start_bus_number;
> + return pci_mmcfg_virt[cfg_num].virt;
> + }
> + }
> +
> + /* Fall back to type 0 */
> + return NULL;
> +}
This is a horrid function. Accessing and modifying bus like that causes
poor code generation, and by now having this in a separate translation
unit, the optimiser can't fold it into it's single caller and undo the
poor decisions which went into writing this function.
Instead, you want:
void __iomem *pci_mmcfg_base(pci_sbdf_t sbdf)
{
...
}
base which takes care of the bus adjustment internally. This can be
broken out into a separate patch, and take the opportunity to write it
to Xen style.
> diff --git a/xen/arch/x86/x86_64/pci.c b/xen/arch/x86/x86_64/pci.c
> index 8d33429103..c37e3edade 100644
> --- a/xen/arch/x86/x86_64/pci.c
> +++ b/xen/arch/x86/x86_64/pci.c
> @@ -11,13 +11,123 @@
> #define PCI_CONF_ADDRESS(sbdf, reg) \
> (0x80000000U | ((sbdf).bdf << 8) | ((reg) & ~3))
>
> +/*
> + * AMD Fam10h CPUs are buggy, and cannot access MMIO config space
> + * on their northbrige except through the * %eax register. As such, you MUST
> + * NOT use normal IOMEM accesses, you need to only use the magic mmio-config
> + * accessor functions.
> + * In fact just use pci_config_*, nothing else please.
I know you're just copying an existing comment, but it's mostly an
opinion and not terribly helpful in place.
"AMD Fam10h CPUs can only make MMCFG accesses via MOV %eax/%ax/%al",
would be better, except...
... this claim cannot be true. It's been made since the K8 RevF BKWG
and exists even into the latest PPRs, but that's simply not how
load/store ops work in the pipeline.
It was added to Linux in
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=3320ad994afb2c44ad34b3b34c3c5cf0da297331
but without adequate justification. I've made some enquiries.
> + */
> +static inline unsigned char mmio_config_readb(void __iomem *pos)
> +{
> + u8 val;
> + asm volatile("movb (%1),%%al" : "=a" (val) : "r" (pos));
> + return val;
> +}
These need corrections, either in this patch or a followup.
Switch to Xen types, and correct the memory operand constraint to be "m"
(*(uint8_t *)ptr).
The Fam10h BKWG states that any memory encoding is acceptable, and this
allows the optimiser more flexibility (which will get used).
> +
> +static inline unsigned short mmio_config_readw(void __iomem *pos)
> +{
> + u16 val;
> + asm volatile("movw (%1),%%ax" : "=a" (val) : "r" (pos));
> + return val;
> +}
> +
> +static inline unsigned int mmio_config_readl(void __iomem *pos)
> +{
> + u32 val;
> + asm volatile("movl (%1),%%eax" : "=a" (val) : "r" (pos));
> + return val;
> +}
> +
> +static inline void mmio_config_writeb(void __iomem *pos, u8 val)
> +{
> + asm volatile("movb %%al,(%1)" :: "a" (val), "r" (pos) : "memory");
> +}
> +
> +static inline void mmio_config_writew(void __iomem *pos, u16 val)
> +{
> + asm volatile("movw %%ax,(%1)" :: "a" (val), "r" (pos) : "memory");
> +}
> +
> +static inline void mmio_config_writel(void __iomem *pos, u32 val)
> +{
> + asm volatile("movl %%eax,(%1)" :: "a" (val), "r" (pos) : "memory");
> +}
> +
> +static char __iomem *pci_dev_base(unsigned int seg, unsigned int bus, unsigned int devfn)
> +{
> + char __iomem *addr;
> +
> + addr = pci_mmcfg_base(seg, &bus);
> + if (!addr)
> + return NULL;
> + return addr + ((bus << 20) | (devfn << 12));
> +}
> +
> +static inline
> +int pci_mmcfg_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int len, u32 *value)
> +{
> + char __iomem *addr;
> +
> + /* Why do we have this when nobody checks it. How about a BUG()!? -AK */
> + if (unlikely(reg > 4095)) {
> +err: *value = -1;
> + return -EINVAL;
> + }
> +
> + addr = pci_dev_base(sbdf.seg, sbdf.bus, sbdf.devfn);
> + if (!addr)
> + goto err;
> +
> + switch (len) {
> + case 1:
> + *value = mmio_config_readb(addr + reg);
> + break;
> + case 2:
> + *value = mmio_config_readw(addr + reg);
> + break;
> + case 4:
> + *value = mmio_config_readl(addr + reg);
> + break;
> + }
> +
> + return 0;
> +}
Again, for this patch or a later cleanup, drop the output-by-pointer and
return value directly. The optimiser is hopefully doing this already
but it also makes the function simpler.
At best, we want ASSERT_UNREACHBLE()'s in the error paths (including no
mapping), and to consistently return -1. Returning 0 for a bad length
is bogus.
Strictly speaking we also need to check reg & (len - 1) because accesses
must be naturally aligned, but even with ASSERT_UNREACHABLE() and a
failsafe -1, that's still logic emitted and I'm not sure if it's worth
having. Amongst other things you really need to know that len is 1, 2
or 4 before the alignment check reads correctly.
> +
> +inline int pci_mmcfg_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int len, u32 value)
> +{
> + char __iomem *addr;
> +
> + /* Why do we have this when nobody checks it. How about a BUG()!? -AK */
> + if (unlikely(reg > 4095))
> + return -EINVAL;
> +
> + addr = pci_dev_base(sbdf.seg, sbdf.bus, sbdf.devfn);
> + if (!addr)
> + return -EINVAL;
> +
> + switch (len) {
> + case 1:
> + mmio_config_writeb(addr + reg, value);
> + break;
> + case 2:
> + mmio_config_writew(addr + reg, value);
> + break;
> + case 4:
> + mmio_config_writel(addr + reg, value);
> + break;
> + }
> +
> + return 0;
> +}
> +
> uint8_t pci_conf_read8(pci_sbdf_t sbdf, unsigned int reg)
> {
> uint32_t value;
>
> if ( sbdf.seg || reg > 255 )
> {
> - pci_mmcfg_read(sbdf.seg, sbdf.bus, sbdf.devfn, reg, 1, &value);
> + pci_mmcfg_read(sbdf, reg, 1, &value);
> return value;
> }
Not for this patch, but we also need to junk the if() condition here and
elsewhere.
We should be using MMCFG unilaterally if it's available; the IO port
pairs require use of a global spinlock, and behind the scenes all the
CPU is doing is translating it back into MMCFG-like accesses.
At this juncture we should probably change it at the start of the 4.23
dev window to give it maximum time to settle before getting into a
release, so probably best to tack it on as a final commit in this series?
~Andrew
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 5/5] RFC: pci: Migrate pci_mmcfg_{read,write} to pci.c
2026-05-18 17:35 ` Andrew Cooper
@ 2026-05-19 6:02 ` Jan Beulich
2026-05-19 17:15 ` Andrew Cooper
2026-05-19 13:42 ` Teddy Astie
1 sibling, 1 reply; 23+ messages in thread
From: Jan Beulich @ 2026-05-19 6:02 UTC (permalink / raw)
To: Andrew Cooper, Teddy Astie
Cc: Roger Pau Monné, Anthony PERARD, Michal Orzel, Julien Grall,
Stefano Stabellini, xen-devel
On 18.05.2026 19:35, Andrew Cooper wrote:
> On 18/05/2026 4:21 pm, Teddy Astie wrote:
>> --- a/xen/arch/x86/x86_64/mmconfig_64.c
>> +++ b/xen/arch/x86/x86_64/mmconfig_64.c
>> @@ -133,6 +46,25 @@ static void __iomem *mcfg_ioremap(const struct acpi_mcfg_allocation *cfg,
>> return (void __iomem *) virt;
>> }
>>
>> +char __iomem *pci_mmcfg_base(unsigned int seg, unsigned int *bus)
>> +{
>> + struct acpi_mcfg_allocation *cfg;
>> + int cfg_num;
>> +
>> + for (cfg_num = 0; cfg_num < pci_mmcfg_config_num; cfg_num++) {
>> + cfg = pci_mmcfg_virt[cfg_num].cfg;
>> + if (cfg->pci_segment == seg &&
>> + (cfg->start_bus_number <= *bus) &&
>> + (cfg->end_bus_number >= *bus)) {
>> + *bus -= cfg->start_bus_number;
>> + return pci_mmcfg_virt[cfg_num].virt;
>> + }
>> + }
>> +
>> + /* Fall back to type 0 */
>> + return NULL;
>> +}
>
> This is a horrid function. Accessing and modifying bus like that causes
> poor code generation, and by now having this in a separate translation
> unit, the optimiser can't fold it into it's single caller and undo the
> poor decisions which went into writing this function.
>
> Instead, you want:
>
> void __iomem *pci_mmcfg_base(pci_sbdf_t sbdf)
> {
> ...
> }
>
> base which takes care of the bus adjustment internally.
If the updated bus number need passing back to the caller, what do you
mean by this? With two values to pass back, and without resorting to
returning a larger struct by value, one pointer parameter is going to
be needed, isn't it?
>> diff --git a/xen/arch/x86/x86_64/pci.c b/xen/arch/x86/x86_64/pci.c
>> index 8d33429103..c37e3edade 100644
>> --- a/xen/arch/x86/x86_64/pci.c
>> +++ b/xen/arch/x86/x86_64/pci.c
>> @@ -11,13 +11,123 @@
>> #define PCI_CONF_ADDRESS(sbdf, reg) \
>> (0x80000000U | ((sbdf).bdf << 8) | ((reg) & ~3))
>>
>> +/*
>> + * AMD Fam10h CPUs are buggy, and cannot access MMIO config space
>> + * on their northbrige except through the * %eax register. As such, you MUST
>> + * NOT use normal IOMEM accesses, you need to only use the magic mmio-config
>> + * accessor functions.
>> + * In fact just use pci_config_*, nothing else please.
>
> I know you're just copying an existing comment, but it's mostly an
> opinion and not terribly helpful in place.
>
> "AMD Fam10h CPUs can only make MMCFG accesses via MOV %eax/%ax/%al",
> would be better, except...
>
> ... this claim cannot be true. It's been made since the K8 RevF BKWG
> and exists even into the latest PPRs, but that's simply not how
> load/store ops work in the pipeline.
How do you know what special casing there exists (or has existed), or
what (e.g.) bogus(?) SMM intercepts there may be? I'm pretty sure the
Linux change was in response to things indeed not working otherwise.
>> +static inline
>> +int pci_mmcfg_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int len, u32 *value)
>> +{
>> + char __iomem *addr;
>> +
>> + /* Why do we have this when nobody checks it. How about a BUG()!? -AK */
>> + if (unlikely(reg > 4095)) {
>> +err: *value = -1;
Nit: Style. Yet as the comment suggests: Perhaps time to drop or replace
by ASSERT() / BUG_ON()?
>> + return -EINVAL;
>> + }
>> +
>> + addr = pci_dev_base(sbdf.seg, sbdf.bus, sbdf.devfn);
>> + if (!addr)
>> + goto err;
>> +
>> + switch (len) {
>> + case 1:
>> + *value = mmio_config_readb(addr + reg);
>> + break;
>> + case 2:
>> + *value = mmio_config_readw(addr + reg);
>> + break;
>> + case 4:
>> + *value = mmio_config_readl(addr + reg);
>> + break;
>> + }
>> +
>> + return 0;
>> +}
>
> Again, for this patch or a later cleanup, drop the output-by-pointer and
> return value directly. The optimiser is hopefully doing this already
> but it also makes the function simpler.
>
> At best, we want ASSERT_UNREACHBLE()'s in the error paths (including no
> mapping), and to consistently return -1. Returning 0 for a bad length
> is bogus.
This looks to contradict the earlier paragraph: Do you want to return the
value, or do you want to return a success indicator?
Jan
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 5/5] RFC: pci: Migrate pci_mmcfg_{read,write} to pci.c
2026-05-18 17:35 ` Andrew Cooper
2026-05-19 6:02 ` Jan Beulich
@ 2026-05-19 13:42 ` Teddy Astie
1 sibling, 0 replies; 23+ messages in thread
From: Teddy Astie @ 2026-05-19 13:42 UTC (permalink / raw)
To: Andrew Cooper, xen-devel
Cc: Jan Beulich, Roger Pau Monné, Anthony PERARD, Michal Orzel,
Julien Grall, Stefano Stabellini
[-- Attachment #1.1.1: Type: text/plain, Size: 13131 bytes --]
Le 18/05/2026 à 19:38, Andrew Cooper a écrit :
> On 18/05/2026 4:21 pm, Teddy Astie wrote:
>> Key parts of MMCFG access bits are in mmconfig_64.c (in particular
>> pci_mmcfg_{read,write}()) while PCI configuration primitives (used accross the
>> codebase) are in pci.c.
>> This leads to situations where the compiler cannot optimize the `switch (len)`
>> for MMCFG access for all pci_conf_{read,write}N(), because they are not from
>> the same file.
>>
>> Move the pci_mmcfg_{read,write} in pci.c and hint the compiler to inline these
>> functions such that it's more likely that the compiler eliminates the
>> `switch (len)``.
>>
>> Also take the opportunity to migrate to pci_sbdf_t to reduce the parameter count
>> and drop many parameter domains checks.
>>
>> On GCC 16.1, this leads to codegen where pci_conf_{read,write}N() doesn't call
>> pci_mmcfg_{read,write}() anymore and directly perform the MMIO RW.
>>
>> <pci_conf_read32>:
>> 55 push %rbp
>> 48 89 e5 mov %rsp,%rbp
>> 53 push %rbx
>> 89 f8 mov %edi,%eax
>> 89 f3 mov %esi,%ebx
>> c1 ef 10 shr $0x10,%edi
>> 81 fe ff 00 00 00 cmp $0xff,%esi
>> 77 26 ja ffff82d040301fab <pci_conf_read32+0x3a>
>> 85 ff test %edi,%edi
>> 75 22 jne ffff82d040301fab <pci_conf_read32+0x3a>
>> 0f b7 f8 movzwl %ax,%edi
>> c1 e7 08 shl $0x8,%edi
>> 83 e3 fc and $0xfffffffc,%ebx
>> 09 df or %ebx,%edi
>> 81 cf 00 00 00 80 or $0x80000000,%edi
>> ba 04 00 00 00 mov $0x4,%edx
>> be 00 00 00 00 mov $0x0,%esi
>> e8 2a 1c 03 00 call ffff82d040333bd3 <pci_conf_read>
>> eb 22 jmp ffff82d040301fcd <pci_conf_read32+0x5c>
>> 81 fb ff 0f 00 00 cmp $0xfff,%ebx
>> 77 24 ja ffff82d040301fd7 <pci_conf_read32+0x66>
>> 0f b6 d0 movzbl %al,%edx
>> 0f b6 f4 movzbl %ah,%esi
>> 0f b7 ff movzwl %di,%edi
>> e8 f5 fd ff ff call ffff82d040301db6 <pci_dev_base>
>> 48 85 c0 test %rax,%rax
>> 74 18 je ffff82d040301fde <pci_conf_read32+0x6d>
>> 89 db mov %ebx,%ebx
>> 48 01 d8 add %rbx,%rax
>> 8b 00 mov (%rax),%eax
>> 48 8b 5d f8 mov -0x8(%rbp),%rbx
>> c9 leave
>> e9 89 12 f0 ff jmp ffff82d040203260 <__x86_return_thunk>
>> b8 ff ff ff ff mov $0xffffffff,%eax
>> eb ef jmp ffff82d040301fcd <pci_conf_read32+0x5c>
>> b8 ff ff ff ff mov $0xffffffff,%eax
>> eb e8 jmp ffff82d040301fcd <pci_conf_read32+0x5c>
>
> This is not the whole function because it's missing a pop %rbx. Also,
> right at the bottom here are the -1's from bad error paths (discussed
> later).
>
> But, this should be after the ---. Disassembly this long isn't
> interesting to stay in the commit message.
>
Yes, I guess I could just summary it as the function is now inlined
properly.
>
>> diff --git a/xen/arch/x86/x86_64/mmconfig_64.c b/xen/arch/x86/x86_64/mmconfig_64.c
>> index 940cf6d747..483dff9c2c 100644
>> --- a/xen/arch/x86/x86_64/mmconfig_64.c
>> +++ b/xen/arch/x86/x86_64/mmconfig_64.c
>> @@ -133,6 +46,25 @@ static void __iomem *mcfg_ioremap(const struct acpi_mcfg_allocation *cfg,
>> return (void __iomem *) virt;
>> }
>>
>> +char __iomem *pci_mmcfg_base(unsigned int seg, unsigned int *bus)
>> +{
>> + struct acpi_mcfg_allocation *cfg;
>> + int cfg_num;
>> +
>> + for (cfg_num = 0; cfg_num < pci_mmcfg_config_num; cfg_num++) {
>> + cfg = pci_mmcfg_virt[cfg_num].cfg;
>> + if (cfg->pci_segment == seg &&
>> + (cfg->start_bus_number <= *bus) &&
>> + (cfg->end_bus_number >= *bus)) {
>> + *bus -= cfg->start_bus_number;
>> + return pci_mmcfg_virt[cfg_num].virt;
>> + }
>> + }
>> +
>> + /* Fall back to type 0 */
>> + return NULL;
>> +}
>
> This is a horrid function. Accessing and modifying bus like that causes
> poor code generation, and by now having this in a separate translation
> unit, the optimiser can't fold it into it's single caller and undo the
> poor decisions which went into writing this function.
>
> Instead, you want:
>
> void __iomem *pci_mmcfg_base(pci_sbdf_t sbdf)
> {
> ...
> }
>
> base which takes care of the bus adjustment internally. This can be
> broken out into a separate patch, and take the opportunity to write it
> to Xen style.
>
That's a good idea, IIUC you want this function to now return a pointer
to the ECAM MMIO region for this SBDF. So we won't have to compute it
afterward.
>> diff --git a/xen/arch/x86/x86_64/pci.c b/xen/arch/x86/x86_64/pci.c
>> index 8d33429103..c37e3edade 100644
>> --- a/xen/arch/x86/x86_64/pci.c
>> +++ b/xen/arch/x86/x86_64/pci.c
>> @@ -11,13 +11,123 @@
>> #define PCI_CONF_ADDRESS(sbdf, reg) \
>> (0x80000000U | ((sbdf).bdf << 8) | ((reg) & ~3))
>>
>> +/*
>> + * AMD Fam10h CPUs are buggy, and cannot access MMIO config space
>> + * on their northbrige except through the * %eax register. As such, you MUST
>> + * NOT use normal IOMEM accesses, you need to only use the magic mmio-config
>> + * accessor functions.
>> + * In fact just use pci_config_*, nothing else please.
>
> I know you're just copying an existing comment, but it's mostly an
> opinion and not terribly helpful in place.
>
> "AMD Fam10h CPUs can only make MMCFG accesses via MOV %eax/%ax/%al",
> would be better, except...
>
> ... this claim cannot be true. It's been made since the K8 RevF BKWG
> and exists even into the latest PPRs, but that's simply not how
> load/store ops work in the pipeline.
>
> It was added to Linux in
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=3320ad994afb2c44ad34b3b34c3c5cf0da297331
> but without adequate justification. I've made some enquiries.
>
Recent AMD PPR state that you must use eax/ax/al to access MMIO
configuration space, so I suppose it's not actually a bug (or was in the
past one, but just became a part of the spec ?)
> MMIO configuration space accesses must use the uncacheable (UC) memory
> type.
> Instructions used to read MMIO configuration space are required to
> take the following form:
> mov eax/ax/al, any_address_mode;
> Instructions used to write MMIO configuration space are required to
> take the following form:
> mov any_address_mode, eax/ax/al;
> No other source/target registers may be used other than eax/ax/al.
I can't find anything regarding Intel though.
If that happens to not be the case in practice, and it's fine to use
whichever register, that would be preferable; some other parts of the
code rely on just volatile pointers to do it.
>> + */
>> +static inline unsigned char mmio_config_readb(void __iomem *pos)
>> +{
>> + u8 val;
>> + asm volatile("movb (%1),%%al" : "=a" (val) : "r" (pos));
>> + return val;
>> +}
>
> These need corrections, either in this patch or a followup.
>
> Switch to Xen types, and correct the memory operand constraint to be "m"
> (*(uint8_t *)ptr).
>
> The Fam10h BKWG states that any memory encoding is acceptable, and this
> allows the optimiser more flexibility (which will get used).
>
Looks good to me.
I was thinking about using a macro here, but I didn't like the end
result. So I guess I will stay with these inline functions.
Agree with styling change (I guess there are (too) many places that want
such refactor).
>> +
>> +static inline unsigned short mmio_config_readw(void __iomem *pos)
>> +{
>> + u16 val;
>> + asm volatile("movw (%1),%%ax" : "=a" (val) : "r" (pos));
>> + return val;
>> +}
>> +
>> +static inline unsigned int mmio_config_readl(void __iomem *pos)
>> +{
>> + u32 val;
>> + asm volatile("movl (%1),%%eax" : "=a" (val) : "r" (pos));
>> + return val;
>> +}
>> +
>> +static inline void mmio_config_writeb(void __iomem *pos, u8 val)
>> +{
>> + asm volatile("movb %%al,(%1)" :: "a" (val), "r" (pos) : "memory");
>> +}
>> +
>> +static inline void mmio_config_writew(void __iomem *pos, u16 val)
>> +{
>> + asm volatile("movw %%ax,(%1)" :: "a" (val), "r" (pos) : "memory");
>> +}
>> +
>> +static inline void mmio_config_writel(void __iomem *pos, u32 val)
>> +{
>> + asm volatile("movl %%eax,(%1)" :: "a" (val), "r" (pos) : "memory");
>> +}
>> +
>> +static char __iomem *pci_dev_base(unsigned int seg, unsigned int bus, unsigned int devfn)
>> +{
>> + char __iomem *addr;
>> +
>> + addr = pci_mmcfg_base(seg, &bus);
>> + if (!addr)
>> + return NULL;
>> + return addr + ((bus << 20) | (devfn << 12));
>> +}
>> +
>> +static inline
>> +int pci_mmcfg_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int len, u32 *value)
>> +{
>> + char __iomem *addr;
>> +
>> + /* Why do we have this when nobody checks it. How about a BUG()!? -AK */
>> + if (unlikely(reg > 4095)) {
>> +err: *value = -1;
>> + return -EINVAL;
>> + }
>> +
>> + addr = pci_dev_base(sbdf.seg, sbdf.bus, sbdf.devfn);
>> + if (!addr)
>> + goto err;
>> +
>> + switch (len) {
>> + case 1:
>> + *value = mmio_config_readb(addr + reg);
>> + break;
>> + case 2:
>> + *value = mmio_config_readw(addr + reg);
>> + break;
>> + case 4:
>> + *value = mmio_config_readl(addr + reg);
>> + break;
>> + }
>> +
>> + return 0;
>> +}
>
> Again, for this patch or a later cleanup, drop the output-by-pointer and
> return value directly. The optimiser is hopefully doing this already
> but it also makes the function simpler.
>
> At best, we want ASSERT_UNREACHBLE()'s in the error paths (including no
> mapping), and to consistently return -1. Returning 0 for a bad length
> is bogus.
>
> Strictly speaking we also need to check reg & (len - 1) because accesses
> must be naturally aligned, but even with ASSERT_UNREACHABLE() and a
> failsafe -1, that's still logic emitted and I'm not sure if it's worth
> having. Amongst other things you really need to know that len is 1, 2
> or 4 before the alignment check reads correctly.
>
Making this function inline itself could actually work at allowing the
compiler to eliminate many of these checks, but it's not ideal.
We can eventually leave alignment/length asserts to debug code.
>> +
>> +inline int pci_mmcfg_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int len, u32 value)
>> +{
>> + char __iomem *addr;
>> +
>> + /* Why do we have this when nobody checks it. How about a BUG()!? -AK */
>> + if (unlikely(reg > 4095))
>> + return -EINVAL;
>> +
>> + addr = pci_dev_base(sbdf.seg, sbdf.bus, sbdf.devfn);
>> + if (!addr)
>> + return -EINVAL;
>> +
>> + switch (len) {
>> + case 1:
>> + mmio_config_writeb(addr + reg, value);
>> + break;
>> + case 2:
>> + mmio_config_writew(addr + reg, value);
>> + break;
>> + case 4:
>> + mmio_config_writel(addr + reg, value);
>> + break;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> uint8_t pci_conf_read8(pci_sbdf_t sbdf, unsigned int reg)
>> {
>> uint32_t value;
>>
>> if ( sbdf.seg || reg > 255 )
>> {
>> - pci_mmcfg_read(sbdf.seg, sbdf.bus, sbdf.devfn, reg, 1, &value);
>> + pci_mmcfg_read(sbdf, reg, 1, &value);
>> return value;
>> }
>
> Not for this patch, but we also need to junk the if() condition here and
> elsewhere.
>
> We should be using MMCFG unilaterally if it's available; the IO port
> pairs require use of a global spinlock, and behind the scenes all the
> CPU is doing is translating it back into MMCFG-like accesses.
>
> At this juncture we should probably change it at the start of the 4.23
> dev window to give it maximum time to settle before getting into a
> release, so probably best to tack it on as a final commit in this series?
>
It's actually the plan for [1]. I was thinking of splitting this
specific patch and merge it into a new one dedicated to MMCFG
refactoring and keep the PCI refactoring around pci_sbdf_t separate (the
more I look to PCI subsystem, the more stuff I find to improve there).
> ~Andrew
>
Teddy
[1]
https://lore.kernel.org/xen-devel/cover.1767804090.git.teddy.astie@vates.tech/
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 2489 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 665 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 5/5] RFC: pci: Migrate pci_mmcfg_{read,write} to pci.c
2026-05-19 6:02 ` Jan Beulich
@ 2026-05-19 17:15 ` Andrew Cooper
0 siblings, 0 replies; 23+ messages in thread
From: Andrew Cooper @ 2026-05-19 17:15 UTC (permalink / raw)
To: Jan Beulich, Teddy Astie
Cc: Andrew Cooper, Roger Pau Monné, Anthony PERARD, Michal Orzel,
Julien Grall, Stefano Stabellini, xen-devel
On 19/05/2026 7:02 am, Jan Beulich wrote:
> On 18.05.2026 19:35, Andrew Cooper wrote:
>> On 18/05/2026 4:21 pm, Teddy Astie wrote:
>>> --- a/xen/arch/x86/x86_64/mmconfig_64.c
>>> +++ b/xen/arch/x86/x86_64/mmconfig_64.c
>>> @@ -133,6 +46,25 @@ static void __iomem *mcfg_ioremap(const struct acpi_mcfg_allocation *cfg,
>>> return (void __iomem *) virt;
>>> }
>>>
>>> +char __iomem *pci_mmcfg_base(unsigned int seg, unsigned int *bus)
>>> +{
>>> + struct acpi_mcfg_allocation *cfg;
>>> + int cfg_num;
>>> +
>>> + for (cfg_num = 0; cfg_num < pci_mmcfg_config_num; cfg_num++) {
>>> + cfg = pci_mmcfg_virt[cfg_num].cfg;
>>> + if (cfg->pci_segment == seg &&
>>> + (cfg->start_bus_number <= *bus) &&
>>> + (cfg->end_bus_number >= *bus)) {
>>> + *bus -= cfg->start_bus_number;
>>> + return pci_mmcfg_virt[cfg_num].virt;
>>> + }
>>> + }
>>> +
>>> + /* Fall back to type 0 */
>>> + return NULL;
>>> +}
>> This is a horrid function. Accessing and modifying bus like that causes
>> poor code generation, and by now having this in a separate translation
>> unit, the optimiser can't fold it into it's single caller and undo the
>> poor decisions which went into writing this function.
>>
>> Instead, you want:
>>
>> void __iomem *pci_mmcfg_base(pci_sbdf_t sbdf)
>> {
>> ...
>> }
>>
>> base which takes care of the bus adjustment internally.
> If the updated bus number need passing back to the caller, what do you
> mean by this? With two values to pass back, and without resorting to
> returning a larger struct by value, one pointer parameter is going to
> be needed, isn't it?
With an API like this, the bus number does not need passing back. The
caller just accesses pci_mmcfg_base(sbdf) + reg (after the NULL check of
course).
Also, I'm pretty sure that it will be cleaner to merge the two functions
than to leave them split.
>
>>> diff --git a/xen/arch/x86/x86_64/pci.c b/xen/arch/x86/x86_64/pci.c
>>> index 8d33429103..c37e3edade 100644
>>> --- a/xen/arch/x86/x86_64/pci.c
>>> +++ b/xen/arch/x86/x86_64/pci.c
>>> @@ -11,13 +11,123 @@
>>> #define PCI_CONF_ADDRESS(sbdf, reg) \
>>> (0x80000000U | ((sbdf).bdf << 8) | ((reg) & ~3))
>>>
>>> +/*
>>> + * AMD Fam10h CPUs are buggy, and cannot access MMIO config space
>>> + * on their northbrige except through the * %eax register. As such, you MUST
>>> + * NOT use normal IOMEM accesses, you need to only use the magic mmio-config
>>> + * accessor functions.
>>> + * In fact just use pci_config_*, nothing else please.
>> I know you're just copying an existing comment, but it's mostly an
>> opinion and not terribly helpful in place.
>>
>> "AMD Fam10h CPUs can only make MMCFG accesses via MOV %eax/%ax/%al",
>> would be better, except...
>>
>> ... this claim cannot be true. It's been made since the K8 RevF BKWG
>> and exists even into the latest PPRs, but that's simply not how
>> load/store ops work in the pipeline.
> How do you know what special casing there exists (or has existed), or
> what (e.g.) bogus(?) SMM intercepts there may be? I'm pretty sure the
> Linux change was in response to things indeed not working otherwise.
I did see you elsewhere on the PR which merged this, but not on this
patch specifically.
I have it on good authority that AMD CPUs can't trap MMIO into SMM.
(I'm not aware of Intel CPUs being able to trap MMIO like this either,
whereas both Intel and AMD explicitly can trap IO ports into SMM.)
Hence the aformentioned enquiries.
>>> + return -EINVAL;
>>> + }
>>> +
>>> + addr = pci_dev_base(sbdf.seg, sbdf.bus, sbdf.devfn);
>>> + if (!addr)
>>> + goto err;
>>> +
>>> + switch (len) {
>>> + case 1:
>>> + *value = mmio_config_readb(addr + reg);
>>> + break;
>>> + case 2:
>>> + *value = mmio_config_readw(addr + reg);
>>> + break;
>>> + case 4:
>>> + *value = mmio_config_readl(addr + reg);
>>> + break;
>>> + }
>>> +
>>> + return 0;
>>> +}
>> Again, for this patch or a later cleanup, drop the output-by-pointer and
>> return value directly. The optimiser is hopefully doing this already
>> but it also makes the function simpler.
>>
>> At best, we want ASSERT_UNREACHBLE()'s in the error paths (including no
>> mapping), and to consistently return -1. Returning 0 for a bad length
>> is bogus.
> This looks to contradict the earlier paragraph: Do you want to return the
> value, or do you want to return a success indicator?
Despite returning -EINVAL, the single callers don't look at the return
value, and only take the *value value, which is generally -1.
For a bad length, 0 is returned (indistinguishable from success if
anyone were to care) and *value is left uninitialised.
So yes, -1 on the failsafe real error path is fine; after all it can
occur for many other non-error reasons too in PCI Config Space accesses.
~Andrew
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 0/5] Small PCI refactoring
2026-05-18 17:20 ` [PATCH 0/5] Small PCI refactoring Teddy Astie
@ 2026-05-19 17:41 ` Andrew Cooper
2026-05-20 6:30 ` Jan Beulich
2026-05-20 3:34 ` dmukhin
1 sibling, 1 reply; 23+ messages in thread
From: Andrew Cooper @ 2026-05-19 17:41 UTC (permalink / raw)
To: Teddy Astie, xen-devel
Cc: Andrew Cooper, Anthony PERARD, Michal Orzel, Jan Beulich,
Julien Grall, Roger Pau Monné, Stefano Stabellini,
Jason Andryuk
On 18/05/2026 6:20 pm, Teddy Astie wrote:
> Le 18/05/2026 à 17:22, Teddy Astie a écrit :
>> Teddy Astie (5):
>> pci: Introduce parse_pci_sbdf{_seg}()
>> vtd: Use pci_sbdf_t in acpi_parse_dev_scope()
>> pci: Use pci_sbdf_t in pci_device_detect()
>> pci: Parse into pci_sbdf_t directly
>> RFC: pci: Migrate pci_mmcfg_{read,write}() to pci.c
>>
>> xen/arch/x86/pv/ro-page-fault.c | 3 +-
>> xen/arch/x86/x86_64/mmconfig.h | 43 --------
>> xen/arch/x86/x86_64/mmconfig_64.c | 106 ++++----------------
>> xen/arch/x86/x86_64/pci.c | 122 +++++++++++++++++++++--
Looking at this further, I think all of x86/x86_64/pci.c wants merging
upwards into x86/pci.c
The arguments about constant-propagating switch (len) apply to
dispatching to pci_conf_{read,write}() too.
One of the two external callers of pci_conf_read() should be converted
to pci_conf_read32 right away, and the other probably could do with
turning into an if/else chain too.
~Andrew
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/5] pci: Introduce parse_pci_sbdf{_seg}()
2026-05-18 15:21 ` [PATCH 1/5] pci: Introduce parse_pci_sbdf{_seg}() Teddy Astie
@ 2026-05-20 2:39 ` dmukhin
2026-05-20 10:00 ` Teddy Astie
0 siblings, 1 reply; 23+ messages in thread
From: dmukhin @ 2026-05-20 2:39 UTC (permalink / raw)
To: Teddy Astie
Cc: xen-devel, Andrew Cooper, Anthony PERARD, Michal Orzel,
Jan Beulich, Julien Grall, Roger Pau Monné,
Stefano Stabellini
On Mon, May 18, 2026 at 05:21:25PM +0200, Teddy Astie wrote:
> In many places, we're parsing a PCI string into individual parts
> (seg, bus, dev, fn) and then transform it into a pci_sbdf_t using PCI_SBDF
> macro. Rather than converting from parts to pci_sbdf_t and vice versa,
> introduce a new function that parses a PCI string into a pci_sbdf_t structure
> directly.
>
> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
> ---
> xen/drivers/pci/pci.c | 18 ++++++++++++++++++
> xen/include/xen/pci.h | 3 +++
> 2 files changed, 21 insertions(+)
>
> diff --git a/xen/drivers/pci/pci.c b/xen/drivers/pci/pci.c
> index 084be3880c..1d06cb035b 100644
> --- a/xen/drivers/pci/pci.c
> +++ b/xen/drivers/pci/pci.c
> @@ -202,3 +202,21 @@ const char *__init parse_pci_seg(const char *s, unsigned int *seg_p,
>
> return s;
> }
> +
> +const char *parse_pci_sbdf(const char *s, pci_sbdf_t *sbdf)
> +{
> + unsigned int seg, bus, dev, func;
> + const char *out = parse_pci(s, &seg, &bus, &dev, &func);
IMO, both parse_pci() and parse_pci_seg() should be merged into
parse_pci_sbdf() and parse_pci_sbdf_seg() at the end of the series,
since there will be no remaining consumers of the old APIs.
What do you think?
> +
> + *sbdf = PCI_SBDF(seg, bus, dev, func);
> + return out;
> +}
> +
> +const char *parse_pci_sbdf_seg(const char *s, pci_sbdf_t *sbdf, bool *def_seg)
> +{
> + unsigned int seg, bus, dev, func;
> + const char *out = parse_pci_seg(s, &seg, &bus, &dev, &func, def_seg);
> +
> + *sbdf = PCI_SBDF(seg, bus, dev, func);
> + return out;
> +}
> diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
> index afb6bbf50d..7bfc59cd75 100644
> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -280,6 +280,9 @@ const char *parse_pci_seg(const char *s, unsigned int *seg_p,
> unsigned int *bus_p, unsigned int *dev_p,
> unsigned int *func_p, bool *def_seg);
>
> +const char *parse_pci_sbdf(const char *s, pci_sbdf_t *sbdf);
> +const char *parse_pci_sbdf_seg(const char *s, pci_sbdf_t *sbdf, bool *def_seg);
> +
> #define PCI_BAR_VF (1u << 0)
> #define PCI_BAR_LAST (1u << 1)
> #define PCI_BAR_ROM (1u << 2)
> --
> 2.52.0
>
>
>
> --
> Teddy Astie | Vates XCP-ng Developer
>
> XCP-ng & Xen Orchestra - Vates solutions
>
> web: https://vates.tech
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/5] vtd: Use pci_sbdf_t in acpi_parse_dev_scope()
2026-05-18 15:21 ` [PATCH 2/5] vtd: Use pci_sbdf_t in acpi_parse_dev_scope() Teddy Astie
@ 2026-05-20 3:00 ` dmukhin
2026-05-20 3:23 ` dmukhin
2026-05-20 10:08 ` Teddy Astie
0 siblings, 2 replies; 23+ messages in thread
From: dmukhin @ 2026-05-20 3:00 UTC (permalink / raw)
To: Teddy Astie; +Cc: xen-devel, Jan Beulich, Andrew Cooper, Roger Pau Monné
On Mon, May 18, 2026 at 05:21:26PM +0200, Teddy Astie wrote:
> Use a dedicated pci_sbdf_t struct that we update instead of recreating
> one each time we need it.
>
> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
> ---
> xen/drivers/passthrough/vtd/dmar.c | 42 ++++++++++++------------------
> 1 file changed, 16 insertions(+), 26 deletions(-)
>
> diff --git a/xen/drivers/passthrough/vtd/dmar.c b/xen/drivers/passthrough/vtd/dmar.c
> index 2a756831a6..c36f4bbd7b 100644
> --- a/xen/drivers/passthrough/vtd/dmar.c
> +++ b/xen/drivers/passthrough/vtd/dmar.c
> @@ -310,7 +310,7 @@ static int __init acpi_parse_dev_scope(
> {
> struct acpi_ioapic_unit *acpi_ioapic_unit;
> const struct acpi_dmar_device_scope *acpi_scope;
> - u16 bus, sub_bus, sec_bus;
> + u16 sub_bus, sec_bus;
> const struct acpi_dmar_pci_path *path;
> struct acpi_drhd_unit *drhd = type == DMAR_TYPE ?
> container_of(scope, struct acpi_drhd_unit, scope) : NULL;
> @@ -332,29 +332,26 @@ static int __init acpi_parse_dev_scope(
>
> while ( start < end )
> {
> + pci_sbdf_t dev_sbdf;
> acpi_scope = start;
> path = (const void *)(acpi_scope + 1);
> depth = (acpi_scope->length - sizeof(*acpi_scope)) / sizeof(*path);
> - bus = acpi_scope->bus;
> + dev_sbdf = PCI_SBDF(seg, acpi_scope->bus, path->dev, path->fn);
`dev_sbdf` calculation depends on `path` which is updated in `while()` loop
below.
>
> while ( --depth > 0 )
> {
> - bus = pci_conf_read8(PCI_SBDF(seg, bus, path->dev, path->fn),
> - PCI_SECONDARY_BUS);
> + dev_sbdf.bus = pci_conf_read8(dev_sbdf, PCI_SECONDARY_BUS);
> path++;
> }
>
> switch ( acpi_scope->entry_type )
> {
> case ACPI_DMAR_SCOPE_TYPE_BRIDGE:
> - sec_bus = pci_conf_read8(PCI_SBDF(seg, bus, path->dev, path->fn),
> - PCI_SECONDARY_BUS);
> - sub_bus = pci_conf_read8(PCI_SBDF(seg, bus, path->dev, path->fn),
> - PCI_SUBORDINATE_BUS);
> + sec_bus = pci_conf_read8(dev_sbdf, PCI_SECONDARY_BUS);
> + sub_bus = pci_conf_read8(dev_sbdf, PCI_SUBORDINATE_BUS);
> if ( iommu_verbose )
> printk(VTDPREFIX " bridge: %pp start=%x sec=%x sub=%x\n",
> - &PCI_SBDF(seg, bus, path->dev, path->fn),
> - acpi_scope->bus, sec_bus, sub_bus);
> + &dev_sbdf, acpi_scope->bus, sec_bus, sub_bus);
>
> dmar_scope_add_buses(scope, sec_bus, sub_bus);
> gfx_only = false;
> @@ -362,8 +359,7 @@ static int __init acpi_parse_dev_scope(
>
> case ACPI_DMAR_SCOPE_TYPE_HPET:
> if ( iommu_verbose )
> - printk(VTDPREFIX " MSI HPET: %pp\n",
> - &PCI_SBDF(seg, bus, path->dev, path->fn));
> + printk(VTDPREFIX " MSI HPET: %pp\n", &dev_sbdf);
>
> if ( drhd )
> {
> @@ -374,9 +370,7 @@ static int __init acpi_parse_dev_scope(
> if ( !acpi_hpet_unit )
> goto out;
> acpi_hpet_unit->id = acpi_scope->enumeration_id;
> - acpi_hpet_unit->bus = bus;
> - acpi_hpet_unit->dev = path->dev;
> - acpi_hpet_unit->func = path->fn;
> + acpi_hpet_unit->bdf = dev_sbdf.bdf;
> list_add(&acpi_hpet_unit->list, &drhd->hpet_list);
>
> gfx_only = false;
> @@ -386,16 +380,15 @@ static int __init acpi_parse_dev_scope(
>
> case ACPI_DMAR_SCOPE_TYPE_ENDPOINT:
> if ( iommu_verbose )
> - printk(VTDPREFIX " endpoint: %pp\n",
> - &PCI_SBDF(seg, bus, path->dev, path->fn));
> + printk(VTDPREFIX " endpoint: %pp\n", &dev_sbdf);
>
> - if ( drhd && pci_device_detect(seg, bus, path->dev, path->fn) )
> + if ( drhd && pci_device_detect(seg, dev_sbdf.bus, dev_sbdf.dev, dev_sbdf.fn) )
Looks like `pci_device_detect()` also needs some refactoring...
(Probably out of scope for this series, though)
> {
> - if ( pci_conf_read8(PCI_SBDF(seg, bus, path->dev, path->fn),
> + if ( pci_conf_read8(dev_sbdf,
> PCI_CLASS_DEVICE + 1) != 0x03
> /* PCI_BASE_CLASS_DISPLAY */ )
> gfx_only = false;
> - else if ( !seg && !bus && path->dev == 2 && !path->fn )
> + else if ( !seg && !dev_sbdf.bus && path->dev == 2 && !path->fn )
> igd_drhd_address = drhd->address;
> }
>
> @@ -403,8 +396,7 @@ static int __init acpi_parse_dev_scope(
>
> case ACPI_DMAR_SCOPE_TYPE_IOAPIC:
> if ( iommu_verbose )
> - printk(VTDPREFIX " IOAPIC: %pp\n",
> - &PCI_SBDF(seg, bus, path->dev, path->fn));
> + printk(VTDPREFIX " IOAPIC: %pp\n", &dev_sbdf);
>
> if ( drhd )
> {
> @@ -413,9 +405,7 @@ static int __init acpi_parse_dev_scope(
> if ( !acpi_ioapic_unit )
> goto out;
> acpi_ioapic_unit->apic_id = acpi_scope->enumeration_id;
> - acpi_ioapic_unit->ioapic.bdf.bus = bus;
> - acpi_ioapic_unit->ioapic.bdf.dev = path->dev;
> - acpi_ioapic_unit->ioapic.bdf.func = path->fn;
> + acpi_ioapic_unit->ioapic.info = dev_sbdf.bdf;
> list_add(&acpi_ioapic_unit->list, &drhd->ioapic_list);
>
> gfx_only = false;
> @@ -431,7 +421,7 @@ static int __init acpi_parse_dev_scope(
> gfx_only = false;
> continue;
> }
> - scope->devices[didx++] = PCI_BDF(bus, path->dev, path->fn);
> + scope->devices[didx++] = dev_sbdf.bdf;
> start += acpi_scope->length;
> }
>
> --
> 2.52.0
>
>
>
> --
> Teddy Astie | Vates XCP-ng Developer
>
> XCP-ng & Xen Orchestra - Vates solutions
>
> web: https://vates.tech
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/5] pci: Use pci_sbdf_t in pci_device_detect()
2026-05-18 15:21 ` [PATCH 3/5] pci: Use pci_sbdf_t in pci_device_detect() Teddy Astie
@ 2026-05-20 3:21 ` dmukhin
2026-05-20 6:37 ` Jan Beulich
0 siblings, 1 reply; 23+ messages in thread
From: dmukhin @ 2026-05-20 3:21 UTC (permalink / raw)
To: Teddy Astie
Cc: xen-devel, Andrew Cooper, Anthony PERARD, Michal Orzel,
Jan Beulich, Julien Grall, Roger Pau Monné,
Stefano Stabellini
On Mon, May 18, 2026 at 05:21:27PM +0200, Teddy Astie wrote:
> Use a single pci_sbdf_t instead of each of its part as individual parameters.
>
> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
> ---
> xen/drivers/char/ehci-dbgp.c | 35 ++++++++++++++----------------
> xen/drivers/passthrough/pci.c | 16 +++++++-------
> xen/drivers/passthrough/vtd/dmar.c | 33 +++++++++++-----------------
> xen/include/xen/pci.h | 2 +-
> 4 files changed, 38 insertions(+), 48 deletions(-)
>
> diff --git a/xen/drivers/char/ehci-dbgp.c b/xen/drivers/char/ehci-dbgp.c
> index a5c79f56fc..39a047eb3f 100644
> --- a/xen/drivers/char/ehci-dbgp.c
> +++ b/xen/drivers/char/ehci-dbgp.c
> @@ -681,16 +681,14 @@ static int dbgp_control_msg(struct ehci_dbgp *dbgp, unsigned int devnum,
> return ret;
> }
>
> -static unsigned int __init __find_dbgp(u8 bus, u8 slot, u8 func)
> +static unsigned int __init __find_dbgp(pci_sbdf_t sbdf)
> {
> - uint32_t class = pci_conf_read32(PCI_SBDF(0, bus, slot, func),
> - PCI_CLASS_REVISION);
> + uint32_t class = pci_conf_read32(sbdf, PCI_CLASS_REVISION);
>
> if ( (class >> 8) != PCI_CLASS_SERIAL_USB_EHCI )
> return 0;
>
> - return pci_find_cap_offset(PCI_SBDF(0, bus, slot, func),
> - PCI_CAP_ID_EHCI_DEBUG);
> + return pci_find_cap_offset(sbdf, PCI_CAP_ID_EHCI_DEBUG);
> }
>
> static unsigned int __init find_dbgp(struct ehci_dbgp *dbgp,
> @@ -704,20 +702,20 @@ static unsigned int __init find_dbgp(struct ehci_dbgp *dbgp,
> {
> for ( func = 0; func < 8; func++ )
> {
> + pci_sbdf_t sbdf = PCI_SBDF(0, bus, slot, func);
> unsigned int cap;
>
> - if ( !pci_device_detect(0, bus, slot, func) )
> + if ( !pci_device_detect(sbdf) )
> {
> if ( !func )
> break;
> continue;
> }
>
> - cap = __find_dbgp(bus, slot, func);
> + cap = __find_dbgp(sbdf);
> if ( !cap || ehci_num-- )
> {
> - if ( !func && !(pci_conf_read8(PCI_SBDF(0, bus, slot, func),
> - PCI_HEADER_TYPE) & 0x80) )
> + if ( !func && !(pci_conf_read8(sbdf, PCI_HEADER_TYPE) & 0x80) )
> break;
> continue;
> }
> @@ -1510,25 +1508,24 @@ void __init ehci_dbgp_init(void)
> }
> else if ( strncmp(opt_dbgp + 4, "@pci", 4) == 0 )
> {
> - unsigned int bus, slot, func;
> -
> - e = parse_pci(opt_dbgp + 8, NULL, &bus, &slot, &func);
> + pci_sbdf_t sbdf = PCI_SBDF(0, 0, 0, 0);
> +
> + e = parse_pci_sbdf(opt_dbgp + 8, &sbdf);
The original logic was ignoring PCI segment, now the full PCI address
is allowed.
Looks like
docs/misc/xen-command-line.pandoc
needs update; but the documentation also says that debug port should be
in PCI segment 0...
> if ( !e || *e )
> return;
>
> - dbgp->bus = bus;
> - dbgp->slot = slot;
> - dbgp->func = func;
> + dbgp->bus = sbdf.bus;
> + dbgp->slot = sbdf.dev;
> + dbgp->func = sbdf.fn;
>
> - if ( !pci_device_detect(0, bus, slot, func) )
> + if ( !pci_device_detect(sbdf) )
> return;
>
> - dbgp->cap = __find_dbgp(bus, slot, func);
> + dbgp->cap = __find_dbgp(sbdf);
> if ( !dbgp->cap )
> return;
>
> - dbgp_printk("Using EHCI debug port on %02x:%02x.%u\n",
> - bus, slot, func);
> + dbgp_printk("Using EHCI debug port on %pp\n", &sbdf);
> }
> else
> return;
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index 464bb0fee4..7b2898bd5a 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -1166,11 +1166,11 @@ out:
> return ret;
> }
>
> -bool __init pci_device_detect(u16 seg, u8 bus, u8 dev, u8 func)
> +bool __init pci_device_detect(pci_sbdf_t sbdf)
> {
> u32 vendor;
>
> - vendor = pci_conf_read32(PCI_SBDF(seg, bus, dev, func), PCI_VENDOR_ID);
> + vendor = pci_conf_read32(sbdf, PCI_VENDOR_ID);
> /* some broken boards return 0 or ~0 if a slot is empty: */
> if ( (vendor == 0xffffffffU) || (vendor == 0x00000000U) ||
> (vendor == 0x0000ffffU) || (vendor == 0xffff0000U) )
> @@ -1221,24 +1221,24 @@ static int __init cf_check _scan_pci_devices(struct pci_seg *pseg, void *arg)
> {
> for ( func = 0; func < 8; func++ )
> {
> - if ( !pci_device_detect(pseg->nr, bus, dev, func) )
> + pci_sbdf_t sbdf = PCI_SBDF(pseg->nr, bus, dev, func);
> +
> + if ( !pci_device_detect(sbdf) )
> {
> if ( !func )
> break;
> continue;
> }
>
> - pdev = alloc_pdev(pseg, bus, PCI_DEVFN(dev, func));
> + pdev = alloc_pdev(pseg, bus, sbdf.devfn);
> if ( !pdev )
> {
> printk(XENLOG_WARNING "%pp: alloc_pdev failed\n",
> - &PCI_SBDF(pseg->nr, bus, dev, func));
> + &sbdf);
> return -ENOMEM;
> }
>
> - if ( !func && !(pci_conf_read8(PCI_SBDF(pseg->nr, bus, dev,
> - func),
> - PCI_HEADER_TYPE) & 0x80) )
> + if ( !func && !(pci_conf_read8(sbdf, PCI_HEADER_TYPE) & 0x80) )
> break;
> }
> }
> diff --git a/xen/drivers/passthrough/vtd/dmar.c b/xen/drivers/passthrough/vtd/dmar.c
> index c36f4bbd7b..9f9b639eba 100644
> --- a/xen/drivers/passthrough/vtd/dmar.c
> +++ b/xen/drivers/passthrough/vtd/dmar.c
> @@ -382,7 +382,7 @@ static int __init acpi_parse_dev_scope(
> if ( iommu_verbose )
> printk(VTDPREFIX " endpoint: %pp\n", &dev_sbdf);
>
> - if ( drhd && pci_device_detect(seg, dev_sbdf.bus, dev_sbdf.dev, dev_sbdf.fn) )
> + if ( drhd && pci_device_detect(dev_sbdf) )
> {
> if ( pci_conf_read8(dev_sbdf,
> PCI_CLASS_DEVICE + 1) != 0x03
> @@ -505,7 +505,6 @@ acpi_parse_one_drhd(struct acpi_dmar_header *header)
> acpi_register_drhd_unit(dmaru);
> else
> {
> - u8 b, d, f;
> unsigned int i = 0;
> union {
> const void *raw;
> @@ -519,18 +518,16 @@ acpi_parse_one_drhd(struct acpi_dmar_header *header)
> for ( p.raw = dev_scope_start; i < dmaru->scope.devices_cnt;
> i++, p.raw += p.scope->length )
> {
> + pci_sbdf_t sbdf = PCI_SBDF(drhd->segment, dmaru->scope.devices[i]);
> +
> if ( p.scope->entry_type == ACPI_DMAR_SCOPE_TYPE_IOAPIC ||
> p.scope->entry_type == ACPI_DMAR_SCOPE_TYPE_HPET )
> continue;
>
> - b = PCI_BUS(dmaru->scope.devices[i]);
> - d = PCI_SLOT(dmaru->scope.devices[i]);
> - f = PCI_FUNC(dmaru->scope.devices[i]);
> -
> - if ( !pci_device_detect(drhd->segment, b, d, f) )
> + if ( !pci_device_detect(sbdf) )
> printk(XENLOG_WARNING VTDPREFIX
> " Non-existent device (%pp) in this DRHD's scope!\n",
> - &PCI_SBDF(drhd->segment, b, d, f));
> + &sbdf);
> }
>
> acpi_register_drhd_unit(dmaru);
> @@ -559,17 +556,14 @@ static int __init register_one_rmrr(struct acpi_rmrr_unit *rmrru)
>
> for ( ; i < rmrru->scope.devices_cnt; i++ )
> {
> - u8 b = PCI_BUS(rmrru->scope.devices[i]);
> - u8 d = PCI_SLOT(rmrru->scope.devices[i]);
> - u8 f = PCI_FUNC(rmrru->scope.devices[i]);
> + pci_sbdf_t sbdf = PCI_SBDF(rmrru->segment, rmrru->scope.devices[i]);
>
> - if ( pci_device_detect(rmrru->segment, b, d, f) == 0 )
> + if ( pci_device_detect(sbdf) == 0 )
> {
> dprintk(XENLOG_WARNING VTDPREFIX,
> " Non-existent device (%pp) is reported"
> " in RMRR [%"PRIx64", %"PRIx64"]'s scope!\n",
> - &PCI_SBDF(rmrru->segment, b, d, f),
> - rmrru->base_address, rmrru->end_address);
> + &sbdf, rmrru->base_address, rmrru->end_address);
> ignore = true;
> }
> else
> @@ -753,15 +747,13 @@ static int __init register_one_satc(struct acpi_satc_unit *satcu)
>
> for ( ; i < satcu->scope.devices_cnt; i++ )
> {
> - uint8_t b = PCI_BUS(satcu->scope.devices[i]);
> - uint8_t d = PCI_SLOT(satcu->scope.devices[i]);
> - uint8_t f = PCI_FUNC(satcu->scope.devices[i]);
> + pci_sbdf_t sbdf = PCI_SBDF(satcu->segment, satcu->scope.devices[i]);
>
> - if ( !pci_device_detect(satcu->segment, b, d, f) )
> + if ( !pci_device_detect(sbdf) )
> {
> dprintk(XENLOG_WARNING VTDPREFIX,
> " Non-existent device (%pp) is reported in SATC scope!\n",
> - &PCI_SBDF(satcu->segment, b, d, f));
> + &sbdf);
> ignore = true;
> }
> else
> @@ -1184,8 +1176,9 @@ int cf_check intel_iommu_get_reserved_device_memory(
> static int __init cf_check parse_rmrr_param(const char *str)
> {
> const char *s = str, *cur, *stmp;
> - unsigned int seg, bus, dev, func, dev_count;
> + unsigned int dev_count;
> unsigned long start, end;
> + pci_sbdf_t sbdf;
>
> do {
> if ( nr_rmrr >= MAX_USER_RMRR )
> diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
> index 7bfc59cd75..d816dcad05 100644
> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -218,7 +218,7 @@ static always_inline bool pcidevs_trylock(void)
> #endif
>
> bool pci_known_segment(u16 seg);
> -bool pci_device_detect(u16 seg, u8 bus, u8 dev, u8 func);
> +bool pci_device_detect(pci_sbdf_t sbdf);
> int scan_pci_devices(void);
> enum pdev_type pdev_type(u16 seg, u8 bus, u8 devfn);
> int find_upstream_bridge(u16 seg, u8 *bus, u8 *devfn, u8 *secbus);
> --
> 2.52.0
>
>
>
> --
> Teddy Astie | Vates XCP-ng Developer
>
> XCP-ng & Xen Orchestra - Vates solutions
>
> web: https://vates.tech
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/5] vtd: Use pci_sbdf_t in acpi_parse_dev_scope()
2026-05-20 3:00 ` dmukhin
@ 2026-05-20 3:23 ` dmukhin
2026-05-20 6:32 ` Jan Beulich
2026-05-20 10:08 ` Teddy Astie
1 sibling, 1 reply; 23+ messages in thread
From: dmukhin @ 2026-05-20 3:23 UTC (permalink / raw)
To: dmukhin
Cc: Teddy Astie, xen-devel, Jan Beulich, Andrew Cooper,
Roger Pau Monné
On Tue, May 19, 2026 at 08:00:07PM -0700, dmukhin@ford.com wrote:
> On Mon, May 18, 2026 at 05:21:26PM +0200, Teddy Astie wrote:
> > Use a dedicated pci_sbdf_t struct that we update instead of recreating
> > one each time we need it.
> >
> > Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
> > ---
> > xen/drivers/passthrough/vtd/dmar.c | 42 ++++++++++++------------------
> > 1 file changed, 16 insertions(+), 26 deletions(-)
> >
> > diff --git a/xen/drivers/passthrough/vtd/dmar.c b/xen/drivers/passthrough/vtd/dmar.c
> > index 2a756831a6..c36f4bbd7b 100644
> > --- a/xen/drivers/passthrough/vtd/dmar.c
> > +++ b/xen/drivers/passthrough/vtd/dmar.c
> > @@ -310,7 +310,7 @@ static int __init acpi_parse_dev_scope(
> > {
> > struct acpi_ioapic_unit *acpi_ioapic_unit;
> > const struct acpi_dmar_device_scope *acpi_scope;
> > - u16 bus, sub_bus, sec_bus;
> > + u16 sub_bus, sec_bus;
> > const struct acpi_dmar_pci_path *path;
> > struct acpi_drhd_unit *drhd = type == DMAR_TYPE ?
> > container_of(scope, struct acpi_drhd_unit, scope) : NULL;
> > @@ -332,29 +332,26 @@ static int __init acpi_parse_dev_scope(
> >
> > while ( start < end )
> > {
> > + pci_sbdf_t dev_sbdf;
> > acpi_scope = start;
> > path = (const void *)(acpi_scope + 1);
> > depth = (acpi_scope->length - sizeof(*acpi_scope)) / sizeof(*path);
> > - bus = acpi_scope->bus;
> > + dev_sbdf = PCI_SBDF(seg, acpi_scope->bus, path->dev, path->fn);
>
> `dev_sbdf` calculation depends on `path` which is updated in `while()` loop
> below.
>
> >
> > while ( --depth > 0 )
> > {
> > - bus = pci_conf_read8(PCI_SBDF(seg, bus, path->dev, path->fn),
> > - PCI_SECONDARY_BUS);
> > + dev_sbdf.bus = pci_conf_read8(dev_sbdf, PCI_SECONDARY_BUS);
> > path++;
> > }
> >
> > switch ( acpi_scope->entry_type )
> > {
> > case ACPI_DMAR_SCOPE_TYPE_BRIDGE:
> > - sec_bus = pci_conf_read8(PCI_SBDF(seg, bus, path->dev, path->fn),
> > - PCI_SECONDARY_BUS);
> > - sub_bus = pci_conf_read8(PCI_SBDF(seg, bus, path->dev, path->fn),
> > - PCI_SUBORDINATE_BUS);
> > + sec_bus = pci_conf_read8(dev_sbdf, PCI_SECONDARY_BUS);
> > + sub_bus = pci_conf_read8(dev_sbdf, PCI_SUBORDINATE_BUS);
> > if ( iommu_verbose )
> > printk(VTDPREFIX " bridge: %pp start=%x sec=%x sub=%x\n",
> > - &PCI_SBDF(seg, bus, path->dev, path->fn),
> > - acpi_scope->bus, sec_bus, sub_bus);
> > + &dev_sbdf, acpi_scope->bus, sec_bus, sub_bus);
> >
> > dmar_scope_add_buses(scope, sec_bus, sub_bus);
> > gfx_only = false;
> > @@ -362,8 +359,7 @@ static int __init acpi_parse_dev_scope(
> >
> > case ACPI_DMAR_SCOPE_TYPE_HPET:
> > if ( iommu_verbose )
> > - printk(VTDPREFIX " MSI HPET: %pp\n",
> > - &PCI_SBDF(seg, bus, path->dev, path->fn));
> > + printk(VTDPREFIX " MSI HPET: %pp\n", &dev_sbdf);
> >
> > if ( drhd )
> > {
> > @@ -374,9 +370,7 @@ static int __init acpi_parse_dev_scope(
> > if ( !acpi_hpet_unit )
> > goto out;
> > acpi_hpet_unit->id = acpi_scope->enumeration_id;
> > - acpi_hpet_unit->bus = bus;
> > - acpi_hpet_unit->dev = path->dev;
> > - acpi_hpet_unit->func = path->fn;
> > + acpi_hpet_unit->bdf = dev_sbdf.bdf;
> > list_add(&acpi_hpet_unit->list, &drhd->hpet_list);
> >
> > gfx_only = false;
> > @@ -386,16 +380,15 @@ static int __init acpi_parse_dev_scope(
> >
> > case ACPI_DMAR_SCOPE_TYPE_ENDPOINT:
> > if ( iommu_verbose )
> > - printk(VTDPREFIX " endpoint: %pp\n",
> > - &PCI_SBDF(seg, bus, path->dev, path->fn));
> > + printk(VTDPREFIX " endpoint: %pp\n", &dev_sbdf);
> >
> > - if ( drhd && pci_device_detect(seg, bus, path->dev, path->fn) )
> > + if ( drhd && pci_device_detect(seg, dev_sbdf.bus, dev_sbdf.dev, dev_sbdf.fn) )
>
> Looks like `pci_device_detect()` also needs some refactoring...
> (Probably out of scope for this series, though)
Oh, cool, that is exactly patch 3/5
>
> > {
> > - if ( pci_conf_read8(PCI_SBDF(seg, bus, path->dev, path->fn),
> > + if ( pci_conf_read8(dev_sbdf,
> > PCI_CLASS_DEVICE + 1) != 0x03
> > /* PCI_BASE_CLASS_DISPLAY */ )
> > gfx_only = false;
> > - else if ( !seg && !bus && path->dev == 2 && !path->fn )
> > + else if ( !seg && !dev_sbdf.bus && path->dev == 2 && !path->fn )
> > igd_drhd_address = drhd->address;
> > }
> >
> > @@ -403,8 +396,7 @@ static int __init acpi_parse_dev_scope(
> >
> > case ACPI_DMAR_SCOPE_TYPE_IOAPIC:
> > if ( iommu_verbose )
> > - printk(VTDPREFIX " IOAPIC: %pp\n",
> > - &PCI_SBDF(seg, bus, path->dev, path->fn));
> > + printk(VTDPREFIX " IOAPIC: %pp\n", &dev_sbdf);
> >
> > if ( drhd )
> > {
> > @@ -413,9 +405,7 @@ static int __init acpi_parse_dev_scope(
> > if ( !acpi_ioapic_unit )
> > goto out;
> > acpi_ioapic_unit->apic_id = acpi_scope->enumeration_id;
> > - acpi_ioapic_unit->ioapic.bdf.bus = bus;
> > - acpi_ioapic_unit->ioapic.bdf.dev = path->dev;
> > - acpi_ioapic_unit->ioapic.bdf.func = path->fn;
> > + acpi_ioapic_unit->ioapic.info = dev_sbdf.bdf;
> > list_add(&acpi_ioapic_unit->list, &drhd->ioapic_list);
> >
> > gfx_only = false;
> > @@ -431,7 +421,7 @@ static int __init acpi_parse_dev_scope(
> > gfx_only = false;
> > continue;
> > }
> > - scope->devices[didx++] = PCI_BDF(bus, path->dev, path->fn);
> > + scope->devices[didx++] = dev_sbdf.bdf;
> > start += acpi_scope->length;
> > }
> >
> > --
> > 2.52.0
> >
> >
> >
> > --
> > Teddy Astie | Vates XCP-ng Developer
> >
> > XCP-ng & Xen Orchestra - Vates solutions
> >
> > web: https://vates.tech
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/5] pci: Parse into pci_sbdf_t directly
2026-05-18 15:21 ` [PATCH 4/5] pci: Parse into pci_sbdf_t directly Teddy Astie
@ 2026-05-20 3:31 ` dmukhin
0 siblings, 0 replies; 23+ messages in thread
From: dmukhin @ 2026-05-20 3:31 UTC (permalink / raw)
To: Teddy Astie
Cc: xen-devel, Andrew Cooper, Anthony PERARD, Michal Orzel,
Jan Beulich, Julien Grall, Roger Pau Monné,
Stefano Stabellini, Jason Andryuk
On Mon, May 18, 2026 at 05:21:28PM +0200, Teddy Astie wrote:
> Use the newly introduced parse_pci_sbdf() and parse_pci_sbdf_seg() in order
> to parse into a pci_sbdf_t directly instead of reconstructing it afterward.
>
> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
> ---
> xen/drivers/char/ns16550.c | 24 +++++++++++-----------
> xen/drivers/char/xhci-dbc.c | 6 +++---
> xen/drivers/passthrough/amd/iommu_acpi.c | 26 ++++++++++++------------
> xen/drivers/passthrough/vtd/dmar.c | 7 +++----
> 4 files changed, 31 insertions(+), 32 deletions(-)
>
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index 878da27f2e..fa2d0e5991 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -1572,22 +1572,22 @@ static bool __init parse_positional(struct ns16550 *uart, char **str)
> #ifdef CONFIG_HAS_PCI
> if ( *conf == ',' && *++conf != ',' )
> {
> - unsigned int b, d, f;
> + pci_sbdf_t sbdf;
>
> - conf = parse_pci(conf, NULL, &b, &d, &f);
> + conf = parse_pci_sbdf(conf, &sbdf);
Original logic considered only devices from PCI segment 0, now
all segments are allowed.
I think docs should be updated.
> if ( !conf )
> PARSE_ERR_RET("Bad port PCI coordinates");
Unrelated to the patch: I think it will be good to print the bad
string value in the error message.
> - uart->pci_device = PCI_SBDF(0, b, d, f);
> + uart->pci_device = sbdf;
> uart->ps_bdf_enable = true;
> }
>
> if ( *conf == ',' && *++conf != ',' )
> {
> - unsigned int b, d, f;
> + pci_sbdf_t sbdf;
>
> - if ( !parse_pci(conf, NULL, &b, &d, &f) )
> + if ( !parse_pci_sbdf(conf, &sbdf) )
> PARSE_ERR_RET("Bad bridge PCI coordinates");
> - uart->pci_bridge = PCI_SBDF(0, b, d, f);
> + uart->pci_bridge = sbdf;
> uart->pb_bdf_enable = true;
> }
> #endif
> @@ -1671,22 +1671,22 @@ static bool __init parse_namevalue_pairs(char *str, struct ns16550 *uart)
>
> case port_bdf:
> {
> - unsigned int b, d, f;
> + pci_sbdf_t sbdf;
>
> - if ( !parse_pci(param_value, NULL, &b, &d, &f) )
> + if ( !parse_pci_sbdf(param_value, &sbdf) )
> PARSE_ERR_RET("Bad port PCI coordinates\n");
> - uart->pci_device = PCI_SBDF(0, b, d, f);
> + uart->pci_device = sbdf;
> uart->ps_bdf_enable = true;
> break;
> }
>
> case bridge_bdf:
> {
> - unsigned int b, d, f;
> + pci_sbdf_t sbdf;
>
> - if ( !parse_pci(param_value, NULL, &b, &d, &f) )
> + if ( !parse_pci_sbdf(param_value, &sbdf) )
> PARSE_ERR_RET("Bad bridge PCI coordinates\n");
> - uart->pci_bridge = PCI_SBDF(0, b, d, f);
> + uart->pci_bridge = sbdf;
> uart->pb_bdf_enable = true;
> break;
> }
> diff --git a/xen/drivers/char/xhci-dbc.c b/xen/drivers/char/xhci-dbc.c
> index c1ff528de6..c7fd554be0 100644
> --- a/xen/drivers/char/xhci-dbc.c
> +++ b/xen/drivers/char/xhci-dbc.c
> @@ -1357,9 +1357,9 @@ static int __init cf_check xhci_parse_dbgp(const char *opt_dbgp)
> }
> else if ( strncmp(opt_dbgp + 4, "@pci", 4) == 0 )
> {
> - unsigned int bus, slot, func;
> + pci_sbdf_t sbdf;
>
> - e = parse_pci(opt_dbgp + 8, NULL, &bus, &slot, &func);
> + e = parse_pci_sbdf(opt_dbgp + 8, &sbdf);
> if ( !e || (*e && *e != ',') )
> {
> printk(XENLOG_ERR
> @@ -1368,7 +1368,7 @@ static int __init cf_check xhci_parse_dbgp(const char *opt_dbgp)
> return -EINVAL;
> }
>
> - dbc->sbdf = PCI_SBDF(0, bus, slot, func);
> + dbc->sbdf = sbdf;
> }
> opt = e;
>
> diff --git a/xen/drivers/passthrough/amd/iommu_acpi.c b/xen/drivers/passthrough/amd/iommu_acpi.c
> index 39ae637959..7b40da33ae 100644
> --- a/xen/drivers/passthrough/amd/iommu_acpi.c
> +++ b/xen/drivers/passthrough/amd/iommu_acpi.c
> @@ -682,8 +682,8 @@ static int __init cf_check parse_ivrs_ioapic(const char *str)
> {
> const char *s = str;
> unsigned long id;
> - unsigned int seg, bus, dev, func;
> unsigned int idx;
> + pci_sbdf_t sbdf;
>
> if ( *s != '[' )
> return -EINVAL;
> @@ -692,7 +692,7 @@ static int __init cf_check parse_ivrs_ioapic(const char *str)
> if ( *s != ']' || *++s != '=' )
> return -EINVAL;
>
> - s = parse_pci(s + 1, &seg, &bus, &dev, &func);
> + s = parse_pci_sbdf(s + 1, &sbdf);
> if ( !s || *s )
> return -EINVAL;
>
> @@ -707,7 +707,7 @@ static int __init cf_check parse_ivrs_ioapic(const char *str)
> }
> }
>
> - ioapic_sbdf[idx].sbdf = PCI_SBDF(seg, bus, dev, func);
> + ioapic_sbdf[idx].sbdf = sbdf;
> ioapic_sbdf[idx].id = id;
> ioapic_sbdf[idx].cmdline = true;
>
> @@ -719,7 +719,7 @@ static int __init cf_check parse_ivrs_hpet(const char *str)
> {
> const char *s = str;
> unsigned long id;
> - unsigned int seg, bus, dev, func;
> + pci_sbdf_t sbdf;
>
> if ( *s != '[' )
> return -EINVAL;
> @@ -728,12 +728,12 @@ static int __init cf_check parse_ivrs_hpet(const char *str)
> if ( id != (typeof(hpet_sbdf.id))id || *s != ']' || *++s != '=' )
> return -EINVAL;
>
> - s = parse_pci(s + 1, &seg, &bus, &dev, &func);
> + s = parse_pci_sbdf(s + 1, &sbdf);
> if ( !s || *s )
> return -EINVAL;
>
> hpet_sbdf.id = id;
> - hpet_sbdf.sbdf = PCI_SBDF(seg, bus, dev, func);
> + hpet_sbdf.sbdf = sbdf;
> hpet_sbdf.init = HPET_CMDL;
>
> return 0;
> @@ -1399,13 +1399,13 @@ static int __init cf_check parse_ivmd_param(const char *s)
> }
>
> do {
> - unsigned int seg, bus, dev, func;
> + pci_sbdf_t sbdf;
>
> if ( nr_ivmd >= ARRAY_SIZE(user_ivmds) )
> return -E2BIG;
>
> - s = parse_pci(s + 1, &seg, &bus, &dev, &func);
> - if ( !s || seg )
> + s = parse_pci_sbdf(s + 1, &sbdf);
> + if ( !s || sbdf.seg )
> return -EINVAL;
>
> user_ivmds[nr_ivmd].start_address = start << PAGE_SHIFT;
> @@ -1413,16 +1413,16 @@ static int __init cf_check parse_ivmd_param(const char *s)
> user_ivmds[nr_ivmd].header.flags = ACPI_IVMD_UNITY |
> ACPI_IVMD_READ | ACPI_IVMD_WRITE;
> user_ivmds[nr_ivmd].header.length = sizeof(*user_ivmds);
> - user_ivmds[nr_ivmd].header.device_id = PCI_BDF(bus, dev, func);
> + user_ivmds[nr_ivmd].header.device_id = sbdf.bdf;
> user_ivmds[nr_ivmd].header.type = ACPI_IVRS_TYPE_MEMORY_ONE;
>
> if ( *s == '-' )
> {
> - s = parse_pci(s + 1, &seg, &bus, &dev, &func);
> - if ( !s || seg )
> + s = parse_pci_sbdf(s + 1, &sbdf);
> + if ( !s || sbdf.seg )
> return -EINVAL;
>
> - user_ivmds[nr_ivmd].aux_data = PCI_BDF(bus, dev, func);
> + user_ivmds[nr_ivmd].aux_data = sbdf.bdf;
> if ( user_ivmds[nr_ivmd].aux_data <
> user_ivmds[nr_ivmd].header.device_id )
> return -EINVAL;
> diff --git a/xen/drivers/passthrough/vtd/dmar.c b/xen/drivers/passthrough/vtd/dmar.c
> index 9f9b639eba..dafe1b62f6 100644
> --- a/xen/drivers/passthrough/vtd/dmar.c
> +++ b/xen/drivers/passthrough/vtd/dmar.c
> @@ -1215,7 +1215,7 @@ static int __init cf_check parse_rmrr_param(const char *str)
> do {
> bool def_seg = false;
>
> - stmp = parse_pci_seg(s + 1, &seg, &bus, &dev, &func, &def_seg);
> + stmp = parse_pci_sbdf_seg(s + 1, &sbdf, &def_seg);
> if ( !stmp )
> return -EINVAL;
>
> @@ -1224,12 +1224,11 @@ static int __init cf_check parse_rmrr_param(const char *str)
> * Segment will be replaced with one from first device.
> */
> if ( user_rmrrs[nr_rmrr].dev_count && def_seg )
> - seg = PCI_SEG(user_rmrrs[nr_rmrr].sbdf[0]);
> + sbdf.seg = PCI_SEG(user_rmrrs[nr_rmrr].sbdf[0]);
>
> /* Keep sbdf's even if they differ and later report an error. */
> dev_count = user_rmrrs[nr_rmrr].dev_count;
> - user_rmrrs[nr_rmrr].sbdf[dev_count] =
> - PCI_SBDF(seg, bus, dev, func).sbdf;
> + user_rmrrs[nr_rmrr].sbdf[dev_count] = sbdf.sbdf;
>
> user_rmrrs[nr_rmrr].dev_count++;
> s = stmp;
> --
> 2.52.0
>
>
>
> --
> Teddy Astie | Vates XCP-ng Developer
>
> XCP-ng & Xen Orchestra - Vates solutions
>
> web: https://vates.tech
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 0/5] Small PCI refactoring
2026-05-18 17:20 ` [PATCH 0/5] Small PCI refactoring Teddy Astie
2026-05-19 17:41 ` Andrew Cooper
@ 2026-05-20 3:34 ` dmukhin
1 sibling, 0 replies; 23+ messages in thread
From: dmukhin @ 2026-05-20 3:34 UTC (permalink / raw)
To: Teddy Astie
Cc: xen-devel, Andrew Cooper, Anthony PERARD, Michal Orzel,
Jan Beulich, Julien Grall, Roger Pau Monné,
Stefano Stabellini, Jason Andryuk
On Mon, May 18, 2026 at 07:20:15PM +0200, Teddy Astie wrote:
> Le 18/05/2026 à 17:22, Teddy Astie a écrit :
> > The goal of this series is to make some refactoring of some
> > pci primitives to improve codegen and make code less verbose.
> >
> > A big chunk of it is converting many places where (seg, bus, dev, fn)
> > is split into multiples variables and convert it into being just
> > pci_sbdf_t, in particular in some PCI function parameters to reduce
> > parameter count which usually translate into less registers to pass
> > to the function. Moreover, we also avoid translating back and forth
> > between pci_sbdf_t and individual (seg, bus, dev, fn).
> >
> > Latest patch attempts to improve codegen of pci_conf_{read,write}N()
> > by making them inline specialized variants of pci_mmcfg_{read,write}()
> > in order to eliminate a particular `switch (len)` at compile time.
> >
> > No intended functional change, aside some parts of the codebase that will
> > now correctly handle PCI segment when parsed while it was previously
> > ignored (e.g dbgp).
> >
I would schedule a full CI cycle against the series for smoke testing.
--
Denis
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 0/5] Small PCI refactoring
2026-05-19 17:41 ` Andrew Cooper
@ 2026-05-20 6:30 ` Jan Beulich
0 siblings, 0 replies; 23+ messages in thread
From: Jan Beulich @ 2026-05-20 6:30 UTC (permalink / raw)
To: Andrew Cooper, Teddy Astie
Cc: Anthony PERARD, Michal Orzel, Julien Grall, Roger Pau Monné,
Stefano Stabellini, Jason Andryuk, xen-devel
On 19.05.2026 19:41, Andrew Cooper wrote:
> On 18/05/2026 6:20 pm, Teddy Astie wrote:
>> Le 18/05/2026 à 17:22, Teddy Astie a écrit :
>>> Teddy Astie (5):
>>> pci: Introduce parse_pci_sbdf{_seg}()
>>> vtd: Use pci_sbdf_t in acpi_parse_dev_scope()
>>> pci: Use pci_sbdf_t in pci_device_detect()
>>> pci: Parse into pci_sbdf_t directly
>>> RFC: pci: Migrate pci_mmcfg_{read,write}() to pci.c
>>>
>>> xen/arch/x86/pv/ro-page-fault.c | 3 +-
>>> xen/arch/x86/x86_64/mmconfig.h | 43 --------
>>> xen/arch/x86/x86_64/mmconfig_64.c | 106 ++++----------------
>>> xen/arch/x86/x86_64/pci.c | 122 +++++++++++++++++++++--
>
> Looking at this further, I think all of x86/x86_64/pci.c wants merging
> upwards into x86/pci.c
+1
Jan
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/5] vtd: Use pci_sbdf_t in acpi_parse_dev_scope()
2026-05-20 3:23 ` dmukhin
@ 2026-05-20 6:32 ` Jan Beulich
0 siblings, 0 replies; 23+ messages in thread
From: Jan Beulich @ 2026-05-20 6:32 UTC (permalink / raw)
To: dmukhin, Teddy Astie; +Cc: xen-devel, Andrew Cooper, Roger Pau Monné
On 20.05.2026 05:23, dmukhin@ford.com wrote:
> On Tue, May 19, 2026 at 08:00:07PM -0700, dmukhin@ford.com wrote:
>> On Mon, May 18, 2026 at 05:21:26PM +0200, Teddy Astie wrote:
>>> @@ -386,16 +380,15 @@ static int __init acpi_parse_dev_scope(
>>>
>>> case ACPI_DMAR_SCOPE_TYPE_ENDPOINT:
>>> if ( iommu_verbose )
>>> - printk(VTDPREFIX " endpoint: %pp\n",
>>> - &PCI_SBDF(seg, bus, path->dev, path->fn));
>>> + printk(VTDPREFIX " endpoint: %pp\n", &dev_sbdf);
>>>
>>> - if ( drhd && pci_device_detect(seg, bus, path->dev, path->fn) )
>>> + if ( drhd && pci_device_detect(seg, dev_sbdf.bus, dev_sbdf.dev, dev_sbdf.fn) )
>>
>> Looks like `pci_device_detect()` also needs some refactoring...
>> (Probably out of scope for this series, though)
>
> Oh, cool, that is exactly patch 3/5
Might better be done the other way around. At least then there wouldn't be a
line length issue here, afaict.
Jan
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/5] pci: Use pci_sbdf_t in pci_device_detect()
2026-05-20 3:21 ` dmukhin
@ 2026-05-20 6:37 ` Jan Beulich
0 siblings, 0 replies; 23+ messages in thread
From: Jan Beulich @ 2026-05-20 6:37 UTC (permalink / raw)
To: dmukhin
Cc: xen-devel, Andrew Cooper, Anthony PERARD, Michal Orzel,
Julien Grall, Roger Pau Monné, Stefano Stabellini,
Teddy Astie
On 20.05.2026 05:21, dmukhin@ford.com wrote:
> On Mon, May 18, 2026 at 05:21:27PM +0200, Teddy Astie wrote:
>> @@ -1510,25 +1508,24 @@ void __init ehci_dbgp_init(void)
>> }
>> else if ( strncmp(opt_dbgp + 4, "@pci", 4) == 0 )
>> {
>> - unsigned int bus, slot, func;
>> -
>> - e = parse_pci(opt_dbgp + 8, NULL, &bus, &slot, &func);
>> + pci_sbdf_t sbdf = PCI_SBDF(0, 0, 0, 0);
>> +
>> + e = parse_pci_sbdf(opt_dbgp + 8, &sbdf);
>
> The original logic was ignoring PCI segment,
Not quite - it was demanding it to be either absent or 0.
> now the full PCI address is allowed.
This behavioral change (if indeed intended) definitely needs mentioning /
justifying in the description.
Jan
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/5] pci: Introduce parse_pci_sbdf{_seg}()
2026-05-20 2:39 ` dmukhin
@ 2026-05-20 10:00 ` Teddy Astie
2026-05-21 1:32 ` dmukhin
0 siblings, 1 reply; 23+ messages in thread
From: Teddy Astie @ 2026-05-20 10:00 UTC (permalink / raw)
To: dmukhin
Cc: xen-devel, Andrew Cooper, Anthony PERARD, Michal Orzel,
Jan Beulich, Julien Grall, Roger Pau Monné,
Stefano Stabellini
[-- Attachment #1.1.1: Type: text/plain, Size: 3137 bytes --]
Le 20/05/2026 à 04:43, dmukhin@ford.com a écrit :
> On Mon, May 18, 2026 at 05:21:25PM +0200, Teddy Astie wrote:
>> In many places, we're parsing a PCI string into individual parts
>> (seg, bus, dev, fn) and then transform it into a pci_sbdf_t using PCI_SBDF
>> macro. Rather than converting from parts to pci_sbdf_t and vice versa,
>> introduce a new function that parses a PCI string into a pci_sbdf_t structure
>> directly.
>>
>> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
>> ---
>> xen/drivers/pci/pci.c | 18 ++++++++++++++++++
>> xen/include/xen/pci.h | 3 +++
>> 2 files changed, 21 insertions(+)
>>
>> diff --git a/xen/drivers/pci/pci.c b/xen/drivers/pci/pci.c
>> index 084be3880c..1d06cb035b 100644
>> --- a/xen/drivers/pci/pci.c
>> +++ b/xen/drivers/pci/pci.c
>> @@ -202,3 +202,21 @@ const char *__init parse_pci_seg(const char *s, unsigned int *seg_p,
>>
>> return s;
>> }
>> +
>> +const char *parse_pci_sbdf(const char *s, pci_sbdf_t *sbdf)
>> +{
>> + unsigned int seg, bus, dev, func;
>> + const char *out = parse_pci(s, &seg, &bus, &dev, &func);
>
> IMO, both parse_pci() and parse_pci_seg() should be merged into
> parse_pci_sbdf() and parse_pci_sbdf_seg() at the end of the series,
> since there will be no remaining consumers of the old APIs.
>
> What do you think?
>
That was my plan, but parse_phantom_dev() (in
xen/drivers/passthrough/pci.c) wants to parse the PCI string without the
function part (i.e XXXX:YY:ZZ) which can't be expressed with a full SBDF
parse function.
It currently works by passing NULL to `func_p`, which has special
handling in parse_pci.
We could eventually allow omitting PCI function and make default it to
zero, so that we will be able migrate parse_phantom_dev to this new
function (so it now allows parsing full SBDF, but ignore the function
part of it).
>> +
>> + *sbdf = PCI_SBDF(seg, bus, dev, func);
>> + return out;
>> +}
>> +
>> +const char *parse_pci_sbdf_seg(const char *s, pci_sbdf_t *sbdf, bool *def_seg)
>> +{
>> + unsigned int seg, bus, dev, func;
>> + const char *out = parse_pci_seg(s, &seg, &bus, &dev, &func, def_seg);
>> +
>> + *sbdf = PCI_SBDF(seg, bus, dev, func);
>> + return out;
>> +}
>> diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
>> index afb6bbf50d..7bfc59cd75 100644
>> --- a/xen/include/xen/pci.h
>> +++ b/xen/include/xen/pci.h
>> @@ -280,6 +280,9 @@ const char *parse_pci_seg(const char *s, unsigned int *seg_p,
>> unsigned int *bus_p, unsigned int *dev_p,
>> unsigned int *func_p, bool *def_seg);
>>
>> +const char *parse_pci_sbdf(const char *s, pci_sbdf_t *sbdf);
>> +const char *parse_pci_sbdf_seg(const char *s, pci_sbdf_t *sbdf, bool *def_seg);
>> +
>> #define PCI_BAR_VF (1u << 0)
>> #define PCI_BAR_LAST (1u << 1)
>> #define PCI_BAR_ROM (1u << 2)
>> --
>> 2.52.0
>>
>>
>>
>> --
>> Teddy Astie | Vates XCP-ng Developer
>>
>> XCP-ng & Xen Orchestra - Vates solutions
>>
>> web: https://vates.tech
>
Teddy
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 2489 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 665 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/5] vtd: Use pci_sbdf_t in acpi_parse_dev_scope()
2026-05-20 3:00 ` dmukhin
2026-05-20 3:23 ` dmukhin
@ 2026-05-20 10:08 ` Teddy Astie
1 sibling, 0 replies; 23+ messages in thread
From: Teddy Astie @ 2026-05-20 10:08 UTC (permalink / raw)
To: dmukhin; +Cc: xen-devel, Jan Beulich, Andrew Cooper, Roger Pau Monné
[-- Attachment #1.1.1: Type: text/plain, Size: 6716 bytes --]
Le 20/05/2026 à 05:03, dmukhin@ford.com a écrit :
> On Mon, May 18, 2026 at 05:21:26PM +0200, Teddy Astie wrote:
>> Use a dedicated pci_sbdf_t struct that we update instead of recreating
>> one each time we need it.
>>
>> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
>> ---
>> xen/drivers/passthrough/vtd/dmar.c | 42 ++++++++++++------------------
>> 1 file changed, 16 insertions(+), 26 deletions(-)
>>
>> diff --git a/xen/drivers/passthrough/vtd/dmar.c b/xen/drivers/passthrough/vtd/dmar.c
>> index 2a756831a6..c36f4bbd7b 100644
>> --- a/xen/drivers/passthrough/vtd/dmar.c
>> +++ b/xen/drivers/passthrough/vtd/dmar.c
>> @@ -310,7 +310,7 @@ static int __init acpi_parse_dev_scope(
>> {
>> struct acpi_ioapic_unit *acpi_ioapic_unit;
>> const struct acpi_dmar_device_scope *acpi_scope;
>> - u16 bus, sub_bus, sec_bus;
>> + u16 sub_bus, sec_bus;
>> const struct acpi_dmar_pci_path *path;
>> struct acpi_drhd_unit *drhd = type == DMAR_TYPE ?
>> container_of(scope, struct acpi_drhd_unit, scope) : NULL;
>> @@ -332,29 +332,26 @@ static int __init acpi_parse_dev_scope(
>>
>> while ( start < end )
>> {
>> + pci_sbdf_t dev_sbdf;
>> acpi_scope = start;
>> path = (const void *)(acpi_scope + 1);
>> depth = (acpi_scope->length - sizeof(*acpi_scope)) / sizeof(*path);
>> - bus = acpi_scope->bus;
>> + dev_sbdf = PCI_SBDF(seg, acpi_scope->bus, path->dev, path->fn);
>
> `dev_sbdf` calculation depends on `path` which is updated in `while()` loop
> below.
>
Ah yes indeed, good catch.
Fixed locally.
>>
>> while ( --depth > 0 )
>> {
>> - bus = pci_conf_read8(PCI_SBDF(seg, bus, path->dev, path->fn),
>> - PCI_SECONDARY_BUS);
>> + dev_sbdf.bus = pci_conf_read8(dev_sbdf, PCI_SECONDARY_BUS);
>> path++;
>> }
>>
>> switch ( acpi_scope->entry_type )
>> {
>> case ACPI_DMAR_SCOPE_TYPE_BRIDGE:
>> - sec_bus = pci_conf_read8(PCI_SBDF(seg, bus, path->dev, path->fn),
>> - PCI_SECONDARY_BUS);
>> - sub_bus = pci_conf_read8(PCI_SBDF(seg, bus, path->dev, path->fn),
>> - PCI_SUBORDINATE_BUS);
>> + sec_bus = pci_conf_read8(dev_sbdf, PCI_SECONDARY_BUS);
>> + sub_bus = pci_conf_read8(dev_sbdf, PCI_SUBORDINATE_BUS);
>> if ( iommu_verbose )
>> printk(VTDPREFIX " bridge: %pp start=%x sec=%x sub=%x\n",
>> - &PCI_SBDF(seg, bus, path->dev, path->fn),
>> - acpi_scope->bus, sec_bus, sub_bus);
>> + &dev_sbdf, acpi_scope->bus, sec_bus, sub_bus);
>>
>> dmar_scope_add_buses(scope, sec_bus, sub_bus);
>> gfx_only = false;
>> @@ -362,8 +359,7 @@ static int __init acpi_parse_dev_scope(
>>
>> case ACPI_DMAR_SCOPE_TYPE_HPET:
>> if ( iommu_verbose )
>> - printk(VTDPREFIX " MSI HPET: %pp\n",
>> - &PCI_SBDF(seg, bus, path->dev, path->fn));
>> + printk(VTDPREFIX " MSI HPET: %pp\n", &dev_sbdf);
>>
>> if ( drhd )
>> {
>> @@ -374,9 +370,7 @@ static int __init acpi_parse_dev_scope(
>> if ( !acpi_hpet_unit )
>> goto out;
>> acpi_hpet_unit->id = acpi_scope->enumeration_id;
>> - acpi_hpet_unit->bus = bus;
>> - acpi_hpet_unit->dev = path->dev;
>> - acpi_hpet_unit->func = path->fn;
>> + acpi_hpet_unit->bdf = dev_sbdf.bdf;
>> list_add(&acpi_hpet_unit->list, &drhd->hpet_list);
>>
>> gfx_only = false;
>> @@ -386,16 +380,15 @@ static int __init acpi_parse_dev_scope(
>>
>> case ACPI_DMAR_SCOPE_TYPE_ENDPOINT:
>> if ( iommu_verbose )
>> - printk(VTDPREFIX " endpoint: %pp\n",
>> - &PCI_SBDF(seg, bus, path->dev, path->fn));
>> + printk(VTDPREFIX " endpoint: %pp\n", &dev_sbdf);
>>
>> - if ( drhd && pci_device_detect(seg, bus, path->dev, path->fn) )
>> + if ( drhd && pci_device_detect(seg, dev_sbdf.bus, dev_sbdf.dev, dev_sbdf.fn) )
>
> Looks like `pci_device_detect()` also needs some refactoring...
> (Probably out of scope for this series, though)
>
>> {
>> - if ( pci_conf_read8(PCI_SBDF(seg, bus, path->dev, path->fn),
>> + if ( pci_conf_read8(dev_sbdf,
>> PCI_CLASS_DEVICE + 1) != 0x03
>> /* PCI_BASE_CLASS_DISPLAY */ )
>> gfx_only = false;
>> - else if ( !seg && !bus && path->dev == 2 && !path->fn )
>> + else if ( !seg && !dev_sbdf.bus && path->dev == 2 && !path->fn )
>> igd_drhd_address = drhd->address;
>> }
>>
>> @@ -403,8 +396,7 @@ static int __init acpi_parse_dev_scope(
>>
>> case ACPI_DMAR_SCOPE_TYPE_IOAPIC:
>> if ( iommu_verbose )
>> - printk(VTDPREFIX " IOAPIC: %pp\n",
>> - &PCI_SBDF(seg, bus, path->dev, path->fn));
>> + printk(VTDPREFIX " IOAPIC: %pp\n", &dev_sbdf);
>>
>> if ( drhd )
>> {
>> @@ -413,9 +405,7 @@ static int __init acpi_parse_dev_scope(
>> if ( !acpi_ioapic_unit )
>> goto out;
>> acpi_ioapic_unit->apic_id = acpi_scope->enumeration_id;
>> - acpi_ioapic_unit->ioapic.bdf.bus = bus;
>> - acpi_ioapic_unit->ioapic.bdf.dev = path->dev;
>> - acpi_ioapic_unit->ioapic.bdf.func = path->fn;
>> + acpi_ioapic_unit->ioapic.info = dev_sbdf.bdf;
>> list_add(&acpi_ioapic_unit->list, &drhd->ioapic_list);
>>
>> gfx_only = false;
>> @@ -431,7 +421,7 @@ static int __init acpi_parse_dev_scope(
>> gfx_only = false;
>> continue;
>> }
>> - scope->devices[didx++] = PCI_BDF(bus, path->dev, path->fn);
>> + scope->devices[didx++] = dev_sbdf.bdf;
>> start += acpi_scope->length;
>> }
>>
>> --
>> 2.52.0
>>
>>
>>
>> --
>> Teddy Astie | Vates XCP-ng Developer
>>
>> XCP-ng & Xen Orchestra - Vates solutions
>>
>> web: https://vates.tech
>
Teddy
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 2489 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 665 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/5] pci: Introduce parse_pci_sbdf{_seg}()
2026-05-20 10:00 ` Teddy Astie
@ 2026-05-21 1:32 ` dmukhin
0 siblings, 0 replies; 23+ messages in thread
From: dmukhin @ 2026-05-21 1:32 UTC (permalink / raw)
To: Teddy Astie
Cc: dmukhin, xen-devel, Andrew Cooper, Anthony PERARD, Michal Orzel,
Jan Beulich, Julien Grall, Roger Pau Monné,
Stefano Stabellini
On Wed, May 20, 2026 at 12:00:07PM +0200, Teddy Astie wrote:
> Le 20/05/2026 à 04:43, dmukhin@ford.com a écrit :
> > On Mon, May 18, 2026 at 05:21:25PM +0200, Teddy Astie wrote:
> > > In many places, we're parsing a PCI string into individual parts
> > > (seg, bus, dev, fn) and then transform it into a pci_sbdf_t using PCI_SBDF
> > > macro. Rather than converting from parts to pci_sbdf_t and vice versa,
> > > introduce a new function that parses a PCI string into a pci_sbdf_t structure
> > > directly.
> > >
> > > Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
> > > ---
> > > xen/drivers/pci/pci.c | 18 ++++++++++++++++++
> > > xen/include/xen/pci.h | 3 +++
> > > 2 files changed, 21 insertions(+)
> > >
> > > diff --git a/xen/drivers/pci/pci.c b/xen/drivers/pci/pci.c
> > > index 084be3880c..1d06cb035b 100644
> > > --- a/xen/drivers/pci/pci.c
> > > +++ b/xen/drivers/pci/pci.c
> > > @@ -202,3 +202,21 @@ const char *__init parse_pci_seg(const char *s, unsigned int *seg_p,
> > > return s;
> > > }
> > > +
> > > +const char *parse_pci_sbdf(const char *s, pci_sbdf_t *sbdf)
> > > +{
> > > + unsigned int seg, bus, dev, func;
> > > + const char *out = parse_pci(s, &seg, &bus, &dev, &func);
> >
> > IMO, both parse_pci() and parse_pci_seg() should be merged into
> > parse_pci_sbdf() and parse_pci_sbdf_seg() at the end of the series,
> > since there will be no remaining consumers of the old APIs.
> >
> > What do you think?
> >
>
> That was my plan, but parse_phantom_dev() (in xen/drivers/passthrough/pci.c)
> wants to parse the PCI string without the function part (i.e XXXX:YY:ZZ)
> which can't be expressed with a full SBDF parse function.
I see.
Perhaps adding "SBD" parsing in-place in parse_phantom_dev() is OK?
Like duplicate a small amount of code but drop old APIs.
>
> It currently works by passing NULL to `func_p`, which has special handling
> in parse_pci.
>
> We could eventually allow omitting PCI function and make default it to zero,
> so that we will be able migrate parse_phantom_dev to this new function (so
> it now allows parsing full SBDF, but ignore the function part of it).
>
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2026-05-21 1:33 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <cover.1779116255.git.teddy.astie@vates.tech>
2026-05-18 15:21 ` [PATCH 1/5] pci: Introduce parse_pci_sbdf{_seg}() Teddy Astie
2026-05-20 2:39 ` dmukhin
2026-05-20 10:00 ` Teddy Astie
2026-05-21 1:32 ` dmukhin
2026-05-18 15:21 ` [PATCH 2/5] vtd: Use pci_sbdf_t in acpi_parse_dev_scope() Teddy Astie
2026-05-20 3:00 ` dmukhin
2026-05-20 3:23 ` dmukhin
2026-05-20 6:32 ` Jan Beulich
2026-05-20 10:08 ` Teddy Astie
2026-05-18 15:21 ` [PATCH 3/5] pci: Use pci_sbdf_t in pci_device_detect() Teddy Astie
2026-05-20 3:21 ` dmukhin
2026-05-20 6:37 ` Jan Beulich
2026-05-18 15:21 ` [PATCH 4/5] pci: Parse into pci_sbdf_t directly Teddy Astie
2026-05-20 3:31 ` dmukhin
2026-05-18 15:21 ` [PATCH 5/5] RFC: pci: Migrate pci_mmcfg_{read,write} to pci.c Teddy Astie
2026-05-18 17:35 ` Andrew Cooper
2026-05-19 6:02 ` Jan Beulich
2026-05-19 17:15 ` Andrew Cooper
2026-05-19 13:42 ` Teddy Astie
2026-05-18 17:20 ` [PATCH 0/5] Small PCI refactoring Teddy Astie
2026-05-19 17:41 ` Andrew Cooper
2026-05-20 6:30 ` Jan Beulich
2026-05-20 3:34 ` dmukhin
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.