Following a similar change to amd_iommu struct, make two more structs
take pci_sbdf_t directly instead of seg and bdf separately. This lets us
drop several conversions from the latter to the former and simplifies
several comparisons and assignments.
Bloat-o-meter reports:
add/remove: 0/0 grow/shrink: 1/10 up/down: 256/-320 (-64)
Function old new delta
_einittext 22092 22348 +256
parse_ivrs_hpet 248 245 -3
amd_iommu_detect_one_acpi 876 868 -8
iov_supports_xt 275 264 -11
amd_iommu_read_ioapic_from_ire 344 332 -12
amd_setup_hpet_msi 237 224 -13
amd_iommu_ioapic_update_ire 575 555 -20
reserve_unity_map_for_device 453 424 -29
_hvm_dpci_msi_eoi 160 128 -32
amd_iommu_get_supported_ivhd_type 86 30 -56
parse_ivrs_table 3966 3830 -136
Signed-off-by: Andrii Sultanov <sultanovandriy@gmail.com>
---
Changes in V5:
* Dropped PCI_BDF usage inside PCI_SBDF macros
* Joined separate seg and bdf comparisons into a single sbdf one in
parse_ivrs_table
* Dropped unnecessary bdf in parse_ivhd_device_special, using sbdf.bdf
instead
* Reverted print formatting change in amd_iommu_ioapic_update_ire
Changes in V4:
* Folded several separate seg+bdf comparisons and assignments into one
with sbdf_t
* With reshuffling in the prior commits, this commit is no longer
neutral in terms of code size
Changes in V3:
* Dropped aliasing of seg and bdf, renamed users.
Changes in V2:
* Split single commit into several patches
* Change the format specifier to %pp in amd_iommu_ioapic_update_ire
---
xen/drivers/passthrough/amd/iommu.h | 5 +--
xen/drivers/passthrough/amd/iommu_acpi.c | 40 ++++++++++------------
xen/drivers/passthrough/amd/iommu_intr.c | 43 ++++++++++++------------
3 files changed, 41 insertions(+), 47 deletions(-)
diff --git a/xen/drivers/passthrough/amd/iommu.h b/xen/drivers/passthrough/amd/iommu.h
index 2599800e6a..52f748310b 100644
--- a/xen/drivers/passthrough/amd/iommu.h
+++ b/xen/drivers/passthrough/amd/iommu.h
@@ -262,7 +262,7 @@ int cf_check amd_setup_hpet_msi(struct msi_desc *msi_desc);
void cf_check amd_iommu_dump_intremap_tables(unsigned char key);
extern struct ioapic_sbdf {
- u16 bdf, seg;
+ pci_sbdf_t sbdf;
u8 id;
bool cmdline;
u16 *pin_2_idx;
@@ -273,7 +273,8 @@ unsigned int ioapic_id_to_index(unsigned int apic_id);
unsigned int get_next_ioapic_sbdf_index(void);
extern struct hpet_sbdf {
- u16 bdf, seg, id;
+ pci_sbdf_t sbdf;
+ uint16_t id;
enum {
HPET_NONE,
HPET_CMDL,
diff --git a/xen/drivers/passthrough/amd/iommu_acpi.c b/xen/drivers/passthrough/amd/iommu_acpi.c
index a9c5432e86..c007a427f2 100644
--- a/xen/drivers/passthrough/amd/iommu_acpi.c
+++ b/xen/drivers/passthrough/amd/iommu_acpi.c
@@ -707,8 +707,7 @@ static int __init cf_check parse_ivrs_ioapic(const char *str)
}
}
- ioapic_sbdf[idx].bdf = PCI_BDF(bus, dev, func);
- ioapic_sbdf[idx].seg = seg;
+ ioapic_sbdf[idx].sbdf = PCI_SBDF(seg, bus, dev, func);
ioapic_sbdf[idx].id = id;
ioapic_sbdf[idx].cmdline = true;
@@ -734,8 +733,7 @@ static int __init cf_check parse_ivrs_hpet(const char *str)
return -EINVAL;
hpet_sbdf.id = id;
- hpet_sbdf.bdf = PCI_BDF(bus, dev, func);
- hpet_sbdf.seg = seg;
+ hpet_sbdf.sbdf = PCI_SBDF(seg, bus, dev, func);
hpet_sbdf.init = HPET_CMDL;
return 0;
@@ -746,8 +744,9 @@ static u16 __init parse_ivhd_device_special(
const struct acpi_ivrs_device8c *special, u16 seg,
u16 header_length, u16 block_length, struct amd_iommu *iommu)
{
- u16 dev_length, bdf;
+ u16 dev_length;
unsigned int apic, idx;
+ pci_sbdf_t sbdf;
dev_length = sizeof(*special);
if ( header_length < (block_length + dev_length) )
@@ -756,16 +755,16 @@ static u16 __init parse_ivhd_device_special(
return 0;
}
- bdf = special->used_id;
- if ( bdf >= ivrs_bdf_entries )
+ sbdf = PCI_SBDF(seg, special->used_id);
+ if ( sbdf.bdf >= ivrs_bdf_entries )
{
- AMD_IOMMU_ERROR("IVHD: invalid Device_Entry Dev_Id %#x\n", bdf);
+ AMD_IOMMU_ERROR("IVHD: invalid Device_Entry Dev_Id %#x\n", sbdf.bdf);
return 0;
}
AMD_IOMMU_DEBUG("IVHD Special: %pp variety %#x handle %#x\n",
- &PCI_SBDF(seg, bdf), special->variety, special->handle);
- add_ivrs_mapping_entry(bdf, bdf, special->header.data_setting, 0, true,
+ &sbdf, special->variety, special->handle);
+ add_ivrs_mapping_entry(sbdf.bdf, sbdf.bdf, special->header.data_setting, 0, true,
iommu);
switch ( special->variety )
@@ -780,8 +779,7 @@ static u16 __init parse_ivhd_device_special(
*/
for ( idx = 0; idx < nr_ioapic_sbdf; idx++ )
{
- if ( ioapic_sbdf[idx].bdf == bdf &&
- ioapic_sbdf[idx].seg == seg &&
+ if ( ioapic_sbdf[idx].sbdf.sbdf == sbdf.sbdf &&
ioapic_sbdf[idx].cmdline )
break;
}
@@ -790,7 +788,7 @@ static u16 __init parse_ivhd_device_special(
AMD_IOMMU_DEBUG("IVHD: Command line override present for IO-APIC %#x"
"(IVRS: %#x devID %pp)\n",
ioapic_sbdf[idx].id, special->handle,
- &PCI_SBDF(seg, bdf));
+ &sbdf);
break;
}
@@ -805,8 +803,7 @@ static u16 __init parse_ivhd_device_special(
special->handle);
else if ( idx != MAX_IO_APICS && ioapic_sbdf[idx].pin_2_idx )
{
- if ( ioapic_sbdf[idx].bdf == bdf &&
- ioapic_sbdf[idx].seg == seg )
+ if ( ioapic_sbdf[idx].sbdf.sbdf == sbdf.sbdf )
AMD_IOMMU_WARN("IVHD: duplicate IO-APIC %#x entries\n",
special->handle);
else
@@ -827,8 +824,7 @@ static u16 __init parse_ivhd_device_special(
}
/* set device id of ioapic */
- ioapic_sbdf[idx].bdf = bdf;
- ioapic_sbdf[idx].seg = seg;
+ ioapic_sbdf[idx].sbdf = sbdf;
ioapic_sbdf[idx].id = special->handle;
ioapic_sbdf[idx].pin_2_idx = xmalloc_array(
@@ -862,13 +858,12 @@ static u16 __init parse_ivhd_device_special(
AMD_IOMMU_DEBUG("IVHD: Command line override present for HPET %#x "
"(IVRS: %#x devID %pp)\n",
hpet_sbdf.id, special->handle,
- &PCI_SBDF(seg, bdf));
+ &sbdf);
break;
case HPET_NONE:
/* set device id of hpet */
hpet_sbdf.id = special->handle;
- hpet_sbdf.bdf = bdf;
- hpet_sbdf.seg = seg;
+ hpet_sbdf.sbdf = sbdf;
hpet_sbdf.init = HPET_IVHD;
break;
default:
@@ -1139,9 +1134,8 @@ static int __init cf_check parse_ivrs_table(struct acpi_table_header *table)
return -ENXIO;
}
- if ( !ioapic_sbdf[idx].seg &&
- /* SB IO-APIC is always on this device in AMD systems. */
- ioapic_sbdf[idx].bdf == PCI_BDF(0, 0x14, 0) )
+ /* SB IO-APIC is always on this device in AMD systems. */
+ if ( ioapic_sbdf[idx].sbdf.sbdf == PCI_SBDF(0, 0, 0x14, 0).sbdf )
sb_ioapic = 1;
if ( ioapic_sbdf[idx].pin_2_idx )
diff --git a/xen/drivers/passthrough/amd/iommu_intr.c b/xen/drivers/passthrough/amd/iommu_intr.c
index 25bf25904e..7dae89bcc0 100644
--- a/xen/drivers/passthrough/amd/iommu_intr.c
+++ b/xen/drivers/passthrough/amd/iommu_intr.c
@@ -323,7 +323,8 @@ void cf_check amd_iommu_ioapic_update_ire(
unsigned int apic, unsigned int pin, uint64_t rte)
{
struct IO_APIC_route_entry new_rte;
- int seg, bdf, rc;
+ pci_sbdf_t sbdf;
+ int rc;
struct amd_iommu *iommu;
unsigned int idx;
@@ -335,20 +336,19 @@ void cf_check amd_iommu_ioapic_update_ire(
new_rte.raw = rte;
/* get device id of ioapic devices */
- bdf = ioapic_sbdf[idx].bdf;
- seg = ioapic_sbdf[idx].seg;
- iommu = find_iommu_for_device(PCI_SBDF(seg, bdf));
+ sbdf = ioapic_sbdf[idx].sbdf;
+ iommu = find_iommu_for_device(sbdf);
if ( !iommu )
{
AMD_IOMMU_WARN("failed to find IOMMU for IO-APIC @ %04x:%04x\n",
- seg, bdf);
+ sbdf.seg, sbdf.bdf);
__ioapic_write_entry(apic, pin, true, new_rte);
return;
}
/* Update interrupt remapping entry */
rc = update_intremap_entry_from_ioapic(
- bdf, iommu, &new_rte,
+ sbdf.bdf, iommu, &new_rte,
&ioapic_sbdf[idx].pin_2_idx[pin]);
if ( rc )
@@ -369,7 +369,8 @@ unsigned int cf_check amd_iommu_read_ioapic_from_ire(
unsigned int offset;
unsigned int val = __io_apic_read(apic, reg);
unsigned int pin = (reg - 0x10) / 2;
- uint16_t seg, bdf, req_id;
+ pci_sbdf_t sbdf;
+ uint16_t req_id;
const struct amd_iommu *iommu;
union irte_ptr entry;
@@ -381,12 +382,11 @@ unsigned int cf_check amd_iommu_read_ioapic_from_ire(
if ( offset >= INTREMAP_MAX_ENTRIES )
return val;
- seg = ioapic_sbdf[idx].seg;
- bdf = ioapic_sbdf[idx].bdf;
- iommu = find_iommu_for_device(PCI_SBDF(seg, bdf));
+ sbdf = ioapic_sbdf[idx].sbdf;
+ iommu = find_iommu_for_device(sbdf);
if ( !iommu )
return val;
- req_id = get_intremap_requestor_id(seg, bdf);
+ req_id = get_intremap_requestor_id(sbdf.seg, sbdf.bdf);
entry = get_intremap_entry(iommu, req_id, offset);
if ( !(reg & 1) )
@@ -515,15 +515,15 @@ int cf_check amd_iommu_msi_msg_update_ire(
struct msi_desc *msi_desc, struct msi_msg *msg)
{
struct pci_dev *pdev = msi_desc->dev;
- int bdf, seg, rc;
+ pci_sbdf_t sbdf;
+ int rc;
struct amd_iommu *iommu;
unsigned int i, nr = 1;
u32 data;
- bdf = pdev ? pdev->sbdf.bdf : hpet_sbdf.bdf;
- seg = pdev ? pdev->seg : hpet_sbdf.seg;
+ sbdf = pdev ? pdev->sbdf : hpet_sbdf.sbdf;
- iommu = _find_iommu_for_device(PCI_SBDF(seg, bdf));
+ iommu = _find_iommu_for_device(sbdf);
if ( IS_ERR_OR_NULL(iommu) )
return PTR_ERR(iommu);
@@ -532,7 +532,7 @@ int cf_check amd_iommu_msi_msg_update_ire(
if ( msi_desc->remap_index >= 0 && !msg )
{
- update_intremap_entry_from_msi_msg(iommu, bdf, nr,
+ update_intremap_entry_from_msi_msg(iommu, sbdf.bdf, nr,
&msi_desc->remap_index,
NULL, NULL);
@@ -543,7 +543,7 @@ int cf_check amd_iommu_msi_msg_update_ire(
if ( !msg )
return 0;
- rc = update_intremap_entry_from_msi_msg(iommu, bdf, nr,
+ rc = update_intremap_entry_from_msi_msg(iommu, sbdf.bdf, nr,
&msi_desc->remap_index,
msg, &data);
if ( rc >= 0 )
@@ -660,8 +660,7 @@ bool __init cf_check iov_supports_xt(void)
if ( idx == MAX_IO_APICS )
return false;
- if ( !find_iommu_for_device(PCI_SBDF(ioapic_sbdf[idx].seg,
- ioapic_sbdf[idx].bdf)) )
+ if ( !find_iommu_for_device(ioapic_sbdf[idx].sbdf) )
{
AMD_IOMMU_WARN("no IOMMU for IO-APIC %#x (ID %x)\n",
apic, IO_APIC_ID(apic));
@@ -690,14 +689,14 @@ int __init cf_check amd_setup_hpet_msi(struct msi_desc *msi_desc)
return -ENODEV;
}
- iommu = find_iommu_for_device(PCI_SBDF(hpet_sbdf.seg, hpet_sbdf.bdf));
+ iommu = find_iommu_for_device(hpet_sbdf.sbdf);
if ( !iommu )
return -ENXIO;
- lock = get_intremap_lock(hpet_sbdf.seg, hpet_sbdf.bdf);
+ lock = get_intremap_lock(hpet_sbdf.sbdf.seg, hpet_sbdf.sbdf.bdf);
spin_lock_irqsave(lock, flags);
- msi_desc->remap_index = alloc_intremap_entry(iommu, hpet_sbdf.bdf, 1);
+ msi_desc->remap_index = alloc_intremap_entry(iommu, hpet_sbdf.sbdf.bdf, 1);
if ( msi_desc->remap_index >= INTREMAP_MAX_ENTRIES )
{
msi_desc->remap_index = -1;
--
2.49.0On 17.07.2025 09:31, Andrii Sultanov wrote:
> Following a similar change to amd_iommu struct, make two more structs
> take pci_sbdf_t directly instead of seg and bdf separately. This lets us
> drop several conversions from the latter to the former and simplifies
> several comparisons and assignments.
>
> Bloat-o-meter reports:
> add/remove: 0/0 grow/shrink: 1/10 up/down: 256/-320 (-64)
> Function old new delta
> _einittext 22092 22348 +256
> parse_ivrs_hpet 248 245 -3
> amd_iommu_detect_one_acpi 876 868 -8
> iov_supports_xt 275 264 -11
> amd_iommu_read_ioapic_from_ire 344 332 -12
> amd_setup_hpet_msi 237 224 -13
> amd_iommu_ioapic_update_ire 575 555 -20
> reserve_unity_map_for_device 453 424 -29
> _hvm_dpci_msi_eoi 160 128 -32
> amd_iommu_get_supported_ivhd_type 86 30 -56
> parse_ivrs_table 3966 3830 -136
>
> Signed-off-by: Andrii Sultanov <sultanovandriy@gmail.com>
Here the unspecific subject prefix is actually a problem: It remains unclear
what component(s) is (are) being altered.
> @@ -746,8 +744,9 @@ static u16 __init parse_ivhd_device_special(
> const struct acpi_ivrs_device8c *special, u16 seg,
> u16 header_length, u16 block_length, struct amd_iommu *iommu)
> {
> - u16 dev_length, bdf;
> + u16 dev_length;
Nit: Preferably switch to uint16_t while touching such a line.
With the adjustments (which can be done while committing, I guess):
Reviewed-by: Jan Beulich <jbeulich@suse.com>
> @@ -780,8 +779,7 @@ static u16 __init parse_ivhd_device_special(
> */
> for ( idx = 0; idx < nr_ioapic_sbdf; idx++ )
> {
> - if ( ioapic_sbdf[idx].bdf == bdf &&
> - ioapic_sbdf[idx].seg == seg &&
> + if ( ioapic_sbdf[idx].sbdf.sbdf == sbdf.sbdf &&
Just to mention - there are of course a lot of "sbdf" on lines like this now.
We may want to reconsider naming of the two global variables (not here, but
in general).
Jan
On Thu, Jul 17, 2025 at 08:31:27AM +0100, Andrii Sultanov wrote:
> Following a similar change to amd_iommu struct, make two more structs
> take pci_sbdf_t directly instead of seg and bdf separately. This lets us
> drop several conversions from the latter to the former and simplifies
> several comparisons and assignments.
>
> Bloat-o-meter reports:
> add/remove: 0/0 grow/shrink: 1/10 up/down: 256/-320 (-64)
> Function old new delta
> _einittext 22092 22348 +256
> parse_ivrs_hpet 248 245 -3
> amd_iommu_detect_one_acpi 876 868 -8
> iov_supports_xt 275 264 -11
> amd_iommu_read_ioapic_from_ire 344 332 -12
> amd_setup_hpet_msi 237 224 -13
> amd_iommu_ioapic_update_ire 575 555 -20
> reserve_unity_map_for_device 453 424 -29
> _hvm_dpci_msi_eoi 160 128 -32
> amd_iommu_get_supported_ivhd_type 86 30 -56
> parse_ivrs_table 3966 3830 -136
>
> Signed-off-by: Andrii Sultanov <sultanovandriy@gmail.com>
>
> ---
> Changes in V5:
> * Dropped PCI_BDF usage inside PCI_SBDF macros
> * Joined separate seg and bdf comparisons into a single sbdf one in
> parse_ivrs_table
> * Dropped unnecessary bdf in parse_ivhd_device_special, using sbdf.bdf
> instead
> * Reverted print formatting change in amd_iommu_ioapic_update_ire
>
> Changes in V4:
> * Folded several separate seg+bdf comparisons and assignments into one
> with sbdf_t
> * With reshuffling in the prior commits, this commit is no longer
> neutral in terms of code size
>
> Changes in V3:
> * Dropped aliasing of seg and bdf, renamed users.
>
> Changes in V2:
> * Split single commit into several patches
> * Change the format specifier to %pp in amd_iommu_ioapic_update_ire
> ---
> xen/drivers/passthrough/amd/iommu.h | 5 +--
> xen/drivers/passthrough/amd/iommu_acpi.c | 40 ++++++++++------------
> xen/drivers/passthrough/amd/iommu_intr.c | 43 ++++++++++++------------
> 3 files changed, 41 insertions(+), 47 deletions(-)
>
> diff --git a/xen/drivers/passthrough/amd/iommu.h b/xen/drivers/passthrough/amd/iommu.h
> index 2599800e6a..52f748310b 100644
> --- a/xen/drivers/passthrough/amd/iommu.h
> +++ b/xen/drivers/passthrough/amd/iommu.h
> @@ -262,7 +262,7 @@ int cf_check amd_setup_hpet_msi(struct msi_desc *msi_desc);
> void cf_check amd_iommu_dump_intremap_tables(unsigned char key);
>
> extern struct ioapic_sbdf {
> - u16 bdf, seg;
> + pci_sbdf_t sbdf;
> u8 id;
> bool cmdline;
> u16 *pin_2_idx;
> @@ -273,7 +273,8 @@ unsigned int ioapic_id_to_index(unsigned int apic_id);
> unsigned int get_next_ioapic_sbdf_index(void);
>
> extern struct hpet_sbdf {
> - u16 bdf, seg, id;
> + pci_sbdf_t sbdf;
> + uint16_t id;
> enum {
> HPET_NONE,
> HPET_CMDL,
> diff --git a/xen/drivers/passthrough/amd/iommu_acpi.c b/xen/drivers/passthrough/amd/iommu_acpi.c
> index a9c5432e86..c007a427f2 100644
> --- a/xen/drivers/passthrough/amd/iommu_acpi.c
> +++ b/xen/drivers/passthrough/amd/iommu_acpi.c
> @@ -707,8 +707,7 @@ static int __init cf_check parse_ivrs_ioapic(const char *str)
> }
> }
>
> - ioapic_sbdf[idx].bdf = PCI_BDF(bus, dev, func);
> - ioapic_sbdf[idx].seg = seg;
> + ioapic_sbdf[idx].sbdf = PCI_SBDF(seg, bus, dev, func);
> ioapic_sbdf[idx].id = id;
> ioapic_sbdf[idx].cmdline = true;
>
> @@ -734,8 +733,7 @@ static int __init cf_check parse_ivrs_hpet(const char *str)
> return -EINVAL;
>
> hpet_sbdf.id = id;
> - hpet_sbdf.bdf = PCI_BDF(bus, dev, func);
> - hpet_sbdf.seg = seg;
> + hpet_sbdf.sbdf = PCI_SBDF(seg, bus, dev, func);
> hpet_sbdf.init = HPET_CMDL;
>
> return 0;
> @@ -746,8 +744,9 @@ static u16 __init parse_ivhd_device_special(
> const struct acpi_ivrs_device8c *special, u16 seg,
> u16 header_length, u16 block_length, struct amd_iommu *iommu)
> {
> - u16 dev_length, bdf;
> + u16 dev_length;
> unsigned int apic, idx;
> + pci_sbdf_t sbdf;
>
> dev_length = sizeof(*special);
> if ( header_length < (block_length + dev_length) )
> @@ -756,16 +755,16 @@ static u16 __init parse_ivhd_device_special(
> return 0;
> }
>
> - bdf = special->used_id;
> - if ( bdf >= ivrs_bdf_entries )
> + sbdf = PCI_SBDF(seg, special->used_id);
> + if ( sbdf.bdf >= ivrs_bdf_entries )
> {
> - AMD_IOMMU_ERROR("IVHD: invalid Device_Entry Dev_Id %#x\n", bdf);
> + AMD_IOMMU_ERROR("IVHD: invalid Device_Entry Dev_Id %#x\n", sbdf.bdf);
^^
Suggest using %pp as a formatter (similar to modification below).
> return 0;
> }
>
> AMD_IOMMU_DEBUG("IVHD Special: %pp variety %#x handle %#x\n",
> - &PCI_SBDF(seg, bdf), special->variety, special->handle);
> - add_ivrs_mapping_entry(bdf, bdf, special->header.data_setting, 0, true,
> + &sbdf, special->variety, special->handle);
> + add_ivrs_mapping_entry(sbdf.bdf, sbdf.bdf, special->header.data_setting, 0, true,
> iommu);
>
> switch ( special->variety )
> @@ -780,8 +779,7 @@ static u16 __init parse_ivhd_device_special(
> */
> for ( idx = 0; idx < nr_ioapic_sbdf; idx++ )
> {
> - if ( ioapic_sbdf[idx].bdf == bdf &&
> - ioapic_sbdf[idx].seg == seg &&
> + if ( ioapic_sbdf[idx].sbdf.sbdf == sbdf.sbdf &&
> ioapic_sbdf[idx].cmdline )
> break;
> }
> @@ -790,7 +788,7 @@ static u16 __init parse_ivhd_device_special(
> AMD_IOMMU_DEBUG("IVHD: Command line override present for IO-APIC %#x"
> "(IVRS: %#x devID %pp)\n",
> ioapic_sbdf[idx].id, special->handle,
> - &PCI_SBDF(seg, bdf));
> + &sbdf);
> break;
> }
>
> @@ -805,8 +803,7 @@ static u16 __init parse_ivhd_device_special(
> special->handle);
> else if ( idx != MAX_IO_APICS && ioapic_sbdf[idx].pin_2_idx )
> {
> - if ( ioapic_sbdf[idx].bdf == bdf &&
> - ioapic_sbdf[idx].seg == seg )
> + if ( ioapic_sbdf[idx].sbdf.sbdf == sbdf.sbdf )
> AMD_IOMMU_WARN("IVHD: duplicate IO-APIC %#x entries\n",
> special->handle);
> else
> @@ -827,8 +824,7 @@ static u16 __init parse_ivhd_device_special(
> }
>
> /* set device id of ioapic */
> - ioapic_sbdf[idx].bdf = bdf;
> - ioapic_sbdf[idx].seg = seg;
> + ioapic_sbdf[idx].sbdf = sbdf;
> ioapic_sbdf[idx].id = special->handle;
>
> ioapic_sbdf[idx].pin_2_idx = xmalloc_array(
> @@ -862,13 +858,12 @@ static u16 __init parse_ivhd_device_special(
> AMD_IOMMU_DEBUG("IVHD: Command line override present for HPET %#x "
> "(IVRS: %#x devID %pp)\n",
> hpet_sbdf.id, special->handle,
> - &PCI_SBDF(seg, bdf));
> + &sbdf);
> break;
> case HPET_NONE:
> /* set device id of hpet */
> hpet_sbdf.id = special->handle;
> - hpet_sbdf.bdf = bdf;
> - hpet_sbdf.seg = seg;
> + hpet_sbdf.sbdf = sbdf;
> hpet_sbdf.init = HPET_IVHD;
> break;
> default:
> @@ -1139,9 +1134,8 @@ static int __init cf_check parse_ivrs_table(struct acpi_table_header *table)
> return -ENXIO;
> }
>
> - if ( !ioapic_sbdf[idx].seg &&
> - /* SB IO-APIC is always on this device in AMD systems. */
> - ioapic_sbdf[idx].bdf == PCI_BDF(0, 0x14, 0) )
> + /* SB IO-APIC is always on this device in AMD systems. */
> + if ( ioapic_sbdf[idx].sbdf.sbdf == PCI_SBDF(0, 0, 0x14, 0).sbdf )
> sb_ioapic = 1;
>
> if ( ioapic_sbdf[idx].pin_2_idx )
> diff --git a/xen/drivers/passthrough/amd/iommu_intr.c b/xen/drivers/passthrough/amd/iommu_intr.c
> index 25bf25904e..7dae89bcc0 100644
> --- a/xen/drivers/passthrough/amd/iommu_intr.c
> +++ b/xen/drivers/passthrough/amd/iommu_intr.c
> @@ -323,7 +323,8 @@ void cf_check amd_iommu_ioapic_update_ire(
> unsigned int apic, unsigned int pin, uint64_t rte)
> {
> struct IO_APIC_route_entry new_rte;
> - int seg, bdf, rc;
> + pci_sbdf_t sbdf;
> + int rc;
> struct amd_iommu *iommu;
> unsigned int idx;
>
> @@ -335,20 +336,19 @@ void cf_check amd_iommu_ioapic_update_ire(
> new_rte.raw = rte;
>
> /* get device id of ioapic devices */
> - bdf = ioapic_sbdf[idx].bdf;
> - seg = ioapic_sbdf[idx].seg;
> - iommu = find_iommu_for_device(PCI_SBDF(seg, bdf));
> + sbdf = ioapic_sbdf[idx].sbdf;
> + iommu = find_iommu_for_device(sbdf);
> if ( !iommu )
> {
> AMD_IOMMU_WARN("failed to find IOMMU for IO-APIC @ %04x:%04x\n",
^^
Use %pp ?
> - seg, bdf);
> + sbdf.seg, sbdf.bdf);
> __ioapic_write_entry(apic, pin, true, new_rte);
> return;
> }
>
> /* Update interrupt remapping entry */
> rc = update_intremap_entry_from_ioapic(
> - bdf, iommu, &new_rte,
> + sbdf.bdf, iommu, &new_rte,
> &ioapic_sbdf[idx].pin_2_idx[pin]);
>
> if ( rc )
> @@ -369,7 +369,8 @@ unsigned int cf_check amd_iommu_read_ioapic_from_ire(
> unsigned int offset;
> unsigned int val = __io_apic_read(apic, reg);
> unsigned int pin = (reg - 0x10) / 2;
> - uint16_t seg, bdf, req_id;
> + pci_sbdf_t sbdf;
> + uint16_t req_id;
> const struct amd_iommu *iommu;
> union irte_ptr entry;
>
> @@ -381,12 +382,11 @@ unsigned int cf_check amd_iommu_read_ioapic_from_ire(
> if ( offset >= INTREMAP_MAX_ENTRIES )
> return val;
>
> - seg = ioapic_sbdf[idx].seg;
> - bdf = ioapic_sbdf[idx].bdf;
> - iommu = find_iommu_for_device(PCI_SBDF(seg, bdf));
> + sbdf = ioapic_sbdf[idx].sbdf;
> + iommu = find_iommu_for_device(sbdf);
> if ( !iommu )
> return val;
> - req_id = get_intremap_requestor_id(seg, bdf);
> + req_id = get_intremap_requestor_id(sbdf.seg, sbdf.bdf);
> entry = get_intremap_entry(iommu, req_id, offset);
>
> if ( !(reg & 1) )
> @@ -515,15 +515,15 @@ int cf_check amd_iommu_msi_msg_update_ire(
> struct msi_desc *msi_desc, struct msi_msg *msg)
> {
> struct pci_dev *pdev = msi_desc->dev;
> - int bdf, seg, rc;
> + pci_sbdf_t sbdf;
> + int rc;
> struct amd_iommu *iommu;
> unsigned int i, nr = 1;
> u32 data;
>
> - bdf = pdev ? pdev->sbdf.bdf : hpet_sbdf.bdf;
> - seg = pdev ? pdev->seg : hpet_sbdf.seg;
> + sbdf = pdev ? pdev->sbdf : hpet_sbdf.sbdf;
I think it is worth moving initialization a bit up:
pci_sbdf_t sbdf = pdev ? pdev->sbdf : hpet_sbdf.sbdf;
>
> - iommu = _find_iommu_for_device(PCI_SBDF(seg, bdf));
> + iommu = _find_iommu_for_device(sbdf);
> if ( IS_ERR_OR_NULL(iommu) )
> return PTR_ERR(iommu);
>
> @@ -532,7 +532,7 @@ int cf_check amd_iommu_msi_msg_update_ire(
>
> if ( msi_desc->remap_index >= 0 && !msg )
> {
> - update_intremap_entry_from_msi_msg(iommu, bdf, nr,
> + update_intremap_entry_from_msi_msg(iommu, sbdf.bdf, nr,
> &msi_desc->remap_index,
> NULL, NULL);
>
> @@ -543,7 +543,7 @@ int cf_check amd_iommu_msi_msg_update_ire(
> if ( !msg )
> return 0;
>
> - rc = update_intremap_entry_from_msi_msg(iommu, bdf, nr,
> + rc = update_intremap_entry_from_msi_msg(iommu, sbdf.bdf, nr,
> &msi_desc->remap_index,
> msg, &data);
> if ( rc >= 0 )
> @@ -660,8 +660,7 @@ bool __init cf_check iov_supports_xt(void)
> if ( idx == MAX_IO_APICS )
> return false;
>
> - if ( !find_iommu_for_device(PCI_SBDF(ioapic_sbdf[idx].seg,
> - ioapic_sbdf[idx].bdf)) )
> + if ( !find_iommu_for_device(ioapic_sbdf[idx].sbdf) )
> {
> AMD_IOMMU_WARN("no IOMMU for IO-APIC %#x (ID %x)\n",
> apic, IO_APIC_ID(apic));
> @@ -690,14 +689,14 @@ int __init cf_check amd_setup_hpet_msi(struct msi_desc *msi_desc)
> return -ENODEV;
> }
>
> - iommu = find_iommu_for_device(PCI_SBDF(hpet_sbdf.seg, hpet_sbdf.bdf));
> + iommu = find_iommu_for_device(hpet_sbdf.sbdf);
> if ( !iommu )
> return -ENXIO;
>
> - lock = get_intremap_lock(hpet_sbdf.seg, hpet_sbdf.bdf);
> + lock = get_intremap_lock(hpet_sbdf.sbdf.seg, hpet_sbdf.sbdf.bdf);
> spin_lock_irqsave(lock, flags);
>
> - msi_desc->remap_index = alloc_intremap_entry(iommu, hpet_sbdf.bdf, 1);
> + msi_desc->remap_index = alloc_intremap_entry(iommu, hpet_sbdf.sbdf.bdf, 1);
> if ( msi_desc->remap_index >= INTREMAP_MAX_ENTRIES )
> {
> msi_desc->remap_index = -1;
> --
> 2.49.0
>
>
On 19.07.2025 00:03, dmkhn@proton.me wrote:
> On Thu, Jul 17, 2025 at 08:31:27AM +0100, Andrii Sultanov wrote:
>> @@ -756,16 +755,16 @@ static u16 __init parse_ivhd_device_special(
>> return 0;
>> }
>>
>> - bdf = special->used_id;
>> - if ( bdf >= ivrs_bdf_entries )
>> + sbdf = PCI_SBDF(seg, special->used_id);
>> + if ( sbdf.bdf >= ivrs_bdf_entries )
>> {
>> - AMD_IOMMU_ERROR("IVHD: invalid Device_Entry Dev_Id %#x\n", bdf);
>> + AMD_IOMMU_ERROR("IVHD: invalid Device_Entry Dev_Id %#x\n", sbdf.bdf);
>
> ^^
> Suggest using %pp as a formatter (similar to modification below).
Here using %pp may be okay, albeit I'm not sure even for this one.
>> @@ -335,20 +336,19 @@ void cf_check amd_iommu_ioapic_update_ire(
>> new_rte.raw = rte;
>>
>> /* get device id of ioapic devices */
>> - bdf = ioapic_sbdf[idx].bdf;
>> - seg = ioapic_sbdf[idx].seg;
>> - iommu = find_iommu_for_device(PCI_SBDF(seg, bdf));
>> + sbdf = ioapic_sbdf[idx].sbdf;
>> + iommu = find_iommu_for_device(sbdf);
>> if ( !iommu )
>> {
>> AMD_IOMMU_WARN("failed to find IOMMU for IO-APIC @ %04x:%04x\n",
>
> ^^
> Use %pp ?
Here I'm pretty firmly against. We're talking of an IO-APIC here, not really
a PCI device (and that's irrespective of AMD often(?) representing IO-APICs
also as PCI devices).
Jan
On 7/21/25 7:25 AM, Jan Beulich wrote:
> On 19.07.2025 00:03,dmkhn@proton.me wrote:
>> On Thu, Jul 17, 2025 at 08:31:27AM +0100, Andrii Sultanov wrote:
>>> @@ -335,20 +336,19 @@ void cf_check amd_iommu_ioapic_update_ire(
>>> new_rte.raw = rte;
>>>
>>> /* get device id of ioapic devices */
>>> - bdf = ioapic_sbdf[idx].bdf;
>>> - seg = ioapic_sbdf[idx].seg;
>>> - iommu = find_iommu_for_device(PCI_SBDF(seg, bdf));
>>> + sbdf = ioapic_sbdf[idx].sbdf;
>>> + iommu = find_iommu_for_device(sbdf);
>>> if ( !iommu )
>>> {
>>> AMD_IOMMU_WARN("failed to find IOMMU for IO-APIC @ %04x:%04x\n",
>> ^^
>> Use %pp ?
> Here I'm pretty firmly against. We're talking of an IO-APIC here, not really
> a PCI device (and that's irrespective of AMD often(?) representing IO-APICs
> also as PCI devices).
>
> Jan
Please note that this one was just reverted in V5 following some review
comments. From the changelog in the commit message:
> * Reverted print formatting change in amd_iommu_ioapic_update_ire
© 2016 - 2025 Red Hat, Inc.