* [PATCH v2 1/3] drivers: Change amd_iommu struct to contain pci_sbdf_t, simplify code
2025-03-16 10:08 [PATCH v2 0/3] drivers: Simplify handling of pci_sbdf_t in passthrough/amd Andrii Sultanov
@ 2025-03-16 10:08 ` Andrii Sultanov
2025-03-17 15:55 ` Jan Beulich
2025-03-16 10:08 ` [PATCH v2 2/3] drivers: Change find_iommu_for_device function to take " Andrii Sultanov
2025-03-16 10:08 ` [PATCH v2 3/3] drivers: Make ioapic_sbdf and hpet_sbdf contain pci_sbdf_t Andrii Sultanov
2 siblings, 1 reply; 7+ messages in thread
From: Andrii Sultanov @ 2025-03-16 10:08 UTC (permalink / raw)
To: xen-devel
Cc: Andrii Sultanov, Jan Beulich, Andrew Cooper, Roger Pau Monné
Following on from 250d87dc3ff9 ("x86/msi: Change __msi_set_enable() to
take pci_sbdf_t"), struct amd_iommu has its seg and bdf fields
backwards with relation to pci_sbdf_t. Swap them around, and simplify the
expressions regenerating an sbdf_t from seg+bdf.
Bloat-o-meter reports:
add/remove: 0/0 grow/shrink: 6/11 up/down: 135/-327 (-192)
Function old new delta
_einittext 22028 22092 +64
amd_iommu_prepare 853 897 +44
_hvm_dpci_msi_eoi 157 168 +11
__mon_lengths 2928 2936 +8
_invalidate_all_devices 133 138 +5
amd_iommu_get_reserved_device_memory 521 524 +3
amd_iommu_domain_destroy 46 43 -3
build_info 752 744 -8
amd_iommu_add_device 856 844 -12
amd_iommu_msi_enable 33 20 -13
update_intremap_entry_from_msi_msg 879 859 -20
amd_iommu_get_supported_ivhd_type 86 54 -32
amd_iommu_detect_one_acpi 918 886 -32
iterate_ivrs_mappings 169 129 -40
flush_command_buffer 460 417 -43
set_iommu_interrupt_handler 421 377 -44
enable_iommu 1745 1665 -80
Resolves: https://gitlab.com/xen-project/xen/-/issues/198
Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Andrii Sultanov <sultanovandriy@gmail.com>
---
Changes in V2:
* Split single commit into several patches
* Added the commit title of the referenced patch
* Dropped brackets around &(iommu->sbdf) and &(sbdf)
---
xen/drivers/passthrough/amd/iommu.h | 10 ++++++++--
xen/drivers/passthrough/amd/iommu_cmd.c | 4 ++--
xen/drivers/passthrough/amd/iommu_detect.c | 2 +-
xen/drivers/passthrough/amd/iommu_init.c | 23 ++++++++++------------
xen/drivers/passthrough/amd/iommu_map.c | 2 +-
5 files changed, 22 insertions(+), 19 deletions(-)
diff --git a/xen/drivers/passthrough/amd/iommu.h b/xen/drivers/passthrough/amd/iommu.h
index 00e81b4b2a..a30c8c97ea 100644
--- a/xen/drivers/passthrough/amd/iommu.h
+++ b/xen/drivers/passthrough/amd/iommu.h
@@ -77,8 +77,14 @@ struct amd_iommu {
struct list_head list;
spinlock_t lock; /* protect iommu */
- u16 seg;
- u16 bdf;
+ union {
+ struct {
+ uint16_t bdf;
+ uint16_t seg;
+ };
+ pci_sbdf_t sbdf;
+ };
+
struct msi_desc msi;
u16 cap_offset;
diff --git a/xen/drivers/passthrough/amd/iommu_cmd.c b/xen/drivers/passthrough/amd/iommu_cmd.c
index 83c525b84f..d7162e36d6 100644
--- a/xen/drivers/passthrough/amd/iommu_cmd.c
+++ b/xen/drivers/passthrough/amd/iommu_cmd.c
@@ -85,7 +85,7 @@ static void flush_command_buffer(struct amd_iommu *iommu,
threshold |= threshold << 1;
printk(XENLOG_WARNING
"AMD IOMMU %pp: %scompletion wait taking too long\n",
- &PCI_SBDF(iommu->seg, iommu->bdf),
+ &iommu->sbdf,
timeout_base ? "iotlb " : "");
timeout = 0;
}
@@ -95,7 +95,7 @@ static void flush_command_buffer(struct amd_iommu *iommu,
if ( !timeout )
printk(XENLOG_WARNING
"AMD IOMMU %pp: %scompletion wait took %lums\n",
- &PCI_SBDF(iommu->seg, iommu->bdf),
+ &iommu->sbdf,
timeout_base ? "iotlb " : "",
(NOW() - start) / 10000000);
}
diff --git a/xen/drivers/passthrough/amd/iommu_detect.c b/xen/drivers/passthrough/amd/iommu_detect.c
index cede44e651..8567da2b24 100644
--- a/xen/drivers/passthrough/amd/iommu_detect.c
+++ b/xen/drivers/passthrough/amd/iommu_detect.c
@@ -231,7 +231,7 @@ int __init amd_iommu_detect_one_acpi(
rt = pci_ro_device(iommu->seg, bus, PCI_DEVFN(dev, func));
if ( rt )
printk(XENLOG_ERR "Could not mark config space of %pp read-only (%d)\n",
- &PCI_SBDF(iommu->seg, iommu->bdf), rt);
+ &iommu->sbdf, rt);
list_add_tail(&iommu->list, &amd_iommu_head);
rt = 0;
diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c
index bb25b55c85..ea930426be 100644
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -409,9 +409,7 @@ static void iommu_reset_log(struct amd_iommu *iommu,
static void amd_iommu_msi_enable(struct amd_iommu *iommu, int flag)
{
- pci_sbdf_t sbdf = { .seg = iommu->seg, .bdf = iommu->bdf };
-
- __msi_set_enable(sbdf, iommu->msi.msi_attrib.pos, flag);
+ __msi_set_enable(iommu->sbdf, iommu->msi.msi_attrib.pos, flag);
}
static void cf_check iommu_msi_unmask(struct irq_desc *desc)
@@ -752,12 +750,11 @@ static bool __init set_iommu_interrupt_handler(struct amd_iommu *iommu)
}
pcidevs_lock();
- iommu->msi.dev = pci_get_pdev(NULL, PCI_SBDF(iommu->seg, iommu->bdf));
+ iommu->msi.dev = pci_get_pdev(NULL, iommu->sbdf);
pcidevs_unlock();
if ( !iommu->msi.dev )
{
- AMD_IOMMU_WARN("no pdev for %pp\n",
- &PCI_SBDF(iommu->seg, iommu->bdf));
+ AMD_IOMMU_WARN("no pdev for %pp\n", &iommu->sbdf);
return 0;
}
@@ -779,7 +776,7 @@ static bool __init set_iommu_interrupt_handler(struct amd_iommu *iommu)
hw_irq_controller *handler;
u16 control;
- control = pci_conf_read16(PCI_SBDF(iommu->seg, iommu->bdf),
+ control = pci_conf_read16(iommu->sbdf,
iommu->msi.msi_attrib.pos + PCI_MSI_FLAGS);
iommu->msi.msi.nvec = 1;
@@ -843,22 +840,22 @@ static void amd_iommu_erratum_746_workaround(struct amd_iommu *iommu)
(boot_cpu_data.x86_model > 0x1f) )
return;
- pci_conf_write32(PCI_SBDF(iommu->seg, iommu->bdf), 0xf0, 0x90);
- value = pci_conf_read32(PCI_SBDF(iommu->seg, iommu->bdf), 0xf4);
+ pci_conf_write32(iommu->sbdf, 0xf0, 0x90);
+ value = pci_conf_read32(iommu->sbdf, 0xf4);
if ( value & (1 << 2) )
return;
/* Select NB indirect register 0x90 and enable writing */
- pci_conf_write32(PCI_SBDF(iommu->seg, iommu->bdf), 0xf0, 0x90 | (1 << 8));
+ pci_conf_write32(iommu->sbdf, 0xf0, 0x90 | (1 << 8));
- pci_conf_write32(PCI_SBDF(iommu->seg, iommu->bdf), 0xf4, value | (1 << 2));
+ pci_conf_write32(iommu->sbdf, 0xf4, value | (1 << 2));
printk(XENLOG_INFO
"AMD-Vi: Applying erratum 746 workaround for IOMMU at %pp\n",
- &PCI_SBDF(iommu->seg, iommu->bdf));
+ &iommu->sbdf);
/* Clear the enable writing bit */
- pci_conf_write32(PCI_SBDF(iommu->seg, iommu->bdf), 0xf0, 0x90);
+ pci_conf_write32(iommu->sbdf, 0xf0, 0x90);
}
static void enable_iommu(struct amd_iommu *iommu)
diff --git a/xen/drivers/passthrough/amd/iommu_map.c b/xen/drivers/passthrough/amd/iommu_map.c
index dde393645a..ea68edc0f2 100644
--- a/xen/drivers/passthrough/amd/iommu_map.c
+++ b/xen/drivers/passthrough/amd/iommu_map.c
@@ -730,7 +730,7 @@ int cf_check amd_iommu_get_reserved_device_memory(
* the same alias ID.
*/
if ( bdf != req && ivrs_mappings[req].iommu &&
- func(0, 0, PCI_SBDF(seg, req).sbdf, ctxt) )
+ func(0, 0, sbdf.sbdf, ctxt) )
continue;
if ( global == pending )
--
2.48.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v2 1/3] drivers: Change amd_iommu struct to contain pci_sbdf_t, simplify code
2025-03-16 10:08 ` [PATCH v2 1/3] drivers: Change amd_iommu struct to contain pci_sbdf_t, simplify code Andrii Sultanov
@ 2025-03-17 15:55 ` Jan Beulich
0 siblings, 0 replies; 7+ messages in thread
From: Jan Beulich @ 2025-03-17 15:55 UTC (permalink / raw)
To: Andrii Sultanov; +Cc: Andrew Cooper, Roger Pau Monné, xen-devel
On 16.03.2025 11:08, Andrii Sultanov wrote:
> --- a/xen/drivers/passthrough/amd/iommu.h
> +++ b/xen/drivers/passthrough/amd/iommu.h
> @@ -77,8 +77,14 @@ struct amd_iommu {
> struct list_head list;
> spinlock_t lock; /* protect iommu */
>
> - u16 seg;
> - u16 bdf;
> + union {
> + struct {
> + uint16_t bdf;
> + uint16_t seg;
> + };
> + pci_sbdf_t sbdf;
> + };
As before: I'm firmly opposed to this kind of secondary aliasing. It's bad
enough that we have something similar in struct pci_dev.
> --- a/xen/drivers/passthrough/amd/iommu_map.c
> +++ b/xen/drivers/passthrough/amd/iommu_map.c
> @@ -730,7 +730,7 @@ int cf_check amd_iommu_get_reserved_device_memory(
> * the same alias ID.
> */
> if ( bdf != req && ivrs_mappings[req].iommu &&
> - func(0, 0, PCI_SBDF(seg, req).sbdf, ctxt) )
> + func(0, 0, sbdf.sbdf, ctxt) )
> continue;
This looks unrelated to the struct amd_iommu change here? And this is a
change I would ack right away, if sent in isolation or combined with
other, similar changes.
Jan
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 2/3] drivers: Change find_iommu_for_device function to take pci_sbdf_t, simplify code
2025-03-16 10:08 [PATCH v2 0/3] drivers: Simplify handling of pci_sbdf_t in passthrough/amd Andrii Sultanov
2025-03-16 10:08 ` [PATCH v2 1/3] drivers: Change amd_iommu struct to contain pci_sbdf_t, simplify code Andrii Sultanov
@ 2025-03-16 10:08 ` Andrii Sultanov
2025-03-17 16:07 ` Jan Beulich
2025-03-16 10:08 ` [PATCH v2 3/3] drivers: Make ioapic_sbdf and hpet_sbdf contain pci_sbdf_t Andrii Sultanov
2 siblings, 1 reply; 7+ messages in thread
From: Andrii Sultanov @ 2025-03-16 10:08 UTC (permalink / raw)
To: xen-devel
Cc: Andrii Sultanov, Jan Beulich, Andrew Cooper, Roger Pau Monné
Following a similar change to amd_iommu struct, change the
find_iommu_for_device function to take pci_sbdf_t as a single parameter.
This avoids splitting a single argument into two registers in the majority
of cases.
Bloat-o-meter reports (on top of the first patch in the series):
add/remove: 0/0 grow/shrink: 11/13 up/down: 90/-154 (-64)
Function old new delta
amd_iommu_get_supported_ivhd_type 54 86 +32
parse_ivrs_table 3955 3966 +11
amd_iommu_assign_device 271 282 +11
__mon_lengths 2928 2936 +8
update_intremap_entry_from_msi_msg 859 864 +5
iov_supports_xt 270 275 +5
amd_setup_hpet_msi 232 237 +5
amd_iommu_domain_destroy 43 48 +5
find_iommu_for_device 242 246 +4
amd_iommu_ioapic_update_ire 572 575 +3
allocate_domain_resources 82 83 +1
amd_iommu_read_ioapic_from_ire 347 344 -3
reassign_device 843 838 -5
amd_iommu_remove_device 352 347 -5
amd_iommu_get_reserved_device_memory 524 519 -5
amd_iommu_flush_iotlb 359 354 -5
amd_iommu_add_device 844 839 -5
amd_iommu_setup_domain_device 1478 1472 -6
build_info 752 744 -8
amd_iommu_detect_one_acpi 886 876 -10
register_range_for_device 297 281 -16
amd_iommu_msi_msg_update_ire 472 448 -24
parse_ivmd_block 1339 1312 -27
_hvm_dpci_msi_eoi 168 133 -35
Signed-off-by: Andrii Sultanov <sultanovandriy@gmail.com>
---
> This avoids splitting a single argument into two registers
And adds a few conversions into a pci_sbdf_t... these are addressed further
in the series. - not part of the commit message in case said commit isn't taken.
Changes in V2:
* Split single commit into several patches
* Dropped brackets around &(iommu->sbdf) and &(sbdf)
* Dropped most of the hunk in _invalidate_all_devices - it was
bloat-equivalent to the existing code - just convert with PCI_SBDF
instead
* Dropped the hunk in get_intremap_requestor_id (iommu_intr.c) and
amd_iommu_get_reserved_device_memory (iommu_map.c) as they were only
increasing the code size.
* Kept "/* XXX */" where appropriate
* Fixed a slip-up in register_range_for_iommu_devices where iommu->sbdf
replaced the usage of *different* seg and bdf.
---
xen/drivers/passthrough/amd/iommu.h | 2 +-
xen/drivers/passthrough/amd/iommu_acpi.c | 14 ++++-----
xen/drivers/passthrough/amd/iommu_cmd.c | 2 +-
xen/drivers/passthrough/amd/iommu_init.c | 4 +--
xen/drivers/passthrough/amd/iommu_intr.c | 20 ++++++-------
xen/drivers/passthrough/amd/iommu_map.c | 2 +-
xen/drivers/passthrough/amd/pci_amd_iommu.c | 32 ++++++++++-----------
7 files changed, 38 insertions(+), 38 deletions(-)
diff --git a/xen/drivers/passthrough/amd/iommu.h b/xen/drivers/passthrough/amd/iommu.h
index a30c8c97ea..a9e64b8304 100644
--- a/xen/drivers/passthrough/amd/iommu.h
+++ b/xen/drivers/passthrough/amd/iommu.h
@@ -246,7 +246,7 @@ void amd_iommu_flush_intremap(struct amd_iommu *iommu, uint16_t bdf);
void amd_iommu_flush_all_caches(struct amd_iommu *iommu);
/* find iommu for bdf */
-struct amd_iommu *find_iommu_for_device(int seg, int bdf);
+struct amd_iommu *find_iommu_for_device(pci_sbdf_t sbdf);
/* interrupt remapping */
bool cf_check iov_supports_xt(void);
diff --git a/xen/drivers/passthrough/amd/iommu_acpi.c b/xen/drivers/passthrough/amd/iommu_acpi.c
index 5bdbfb5ba8..ed5a3700c0 100644
--- a/xen/drivers/passthrough/amd/iommu_acpi.c
+++ b/xen/drivers/passthrough/amd/iommu_acpi.c
@@ -239,17 +239,17 @@ static int __init register_range_for_device(
unsigned int bdf, paddr_t base, paddr_t limit,
bool iw, bool ir, bool exclusion)
{
- int seg = 0; /* XXX */
- struct ivrs_mappings *ivrs_mappings = get_ivrs_mappings(seg);
+ pci_sbdf_t sbdf = { .seg = 0 /* XXX */, .bdf = bdf };
+ struct ivrs_mappings *ivrs_mappings = get_ivrs_mappings(sbdf.seg);
struct amd_iommu *iommu;
u16 req;
int rc = 0;
- iommu = find_iommu_for_device(seg, bdf);
+ iommu = find_iommu_for_device(sbdf);
if ( !iommu )
{
AMD_IOMMU_WARN("IVMD: no IOMMU for device %pp - ignoring constrain\n",
- &PCI_SBDF(seg, bdf));
+ &sbdf);
return 0;
}
req = ivrs_mappings[bdf].dte_requestor_id;
@@ -263,9 +263,9 @@ static int __init register_range_for_device(
paddr_t length = limit + PAGE_SIZE - base;
/* reserve unity-mapped page entries for device */
- rc = reserve_unity_map_for_device(seg, bdf, base, length, iw, ir,
+ rc = reserve_unity_map_for_device(sbdf.seg, bdf, base, length, iw, ir,
false) ?:
- reserve_unity_map_for_device(seg, req, base, length, iw, ir,
+ reserve_unity_map_for_device(sbdf.seg, req, base, length, iw, ir,
false);
}
else
@@ -297,7 +297,7 @@ static int __init register_range_for_iommu_devices(
/* reserve unity-mapped page entries for devices */
for ( bdf = rc = 0; !rc && bdf < ivrs_bdf_entries; bdf++ )
{
- if ( iommu != find_iommu_for_device(iommu->seg, bdf) )
+ if ( iommu != find_iommu_for_device(PCI_SBDF(iommu->seg, bdf)) )
continue;
req = get_ivrs_mappings(iommu->seg)[bdf].dte_requestor_id;
diff --git a/xen/drivers/passthrough/amd/iommu_cmd.c b/xen/drivers/passthrough/amd/iommu_cmd.c
index d7162e36d6..facc9d2f6d 100644
--- a/xen/drivers/passthrough/amd/iommu_cmd.c
+++ b/xen/drivers/passthrough/amd/iommu_cmd.c
@@ -288,7 +288,7 @@ void amd_iommu_flush_iotlb(u8 devfn, const struct pci_dev *pdev,
if ( !pci_ats_enabled(pdev->seg, pdev->bus, pdev->devfn) )
return;
- iommu = find_iommu_for_device(pdev->seg, pdev->sbdf.bdf);
+ iommu = find_iommu_for_device(pdev->sbdf);
if ( !iommu )
{
diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c
index ea930426be..01beea1d29 100644
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -1540,13 +1540,13 @@ static void invalidate_all_domain_pages(void)
static int cf_check _invalidate_all_devices(
u16 seg, struct ivrs_mappings *ivrs_mappings)
{
- unsigned int bdf;
+ unsigned int bdf;
u16 req_id;
struct amd_iommu *iommu;
for ( bdf = 0; bdf < ivrs_bdf_entries; bdf++ )
{
- iommu = find_iommu_for_device(seg, bdf);
+ iommu = find_iommu_for_device(PCI_SBDF(seg, bdf));
req_id = ivrs_mappings[bdf].dte_requestor_id;
if ( iommu )
{
diff --git a/xen/drivers/passthrough/amd/iommu_intr.c b/xen/drivers/passthrough/amd/iommu_intr.c
index 9abdc38053..4d7dc2fdb4 100644
--- a/xen/drivers/passthrough/amd/iommu_intr.c
+++ b/xen/drivers/passthrough/amd/iommu_intr.c
@@ -337,7 +337,7 @@ void cf_check amd_iommu_ioapic_update_ire(
/* get device id of ioapic devices */
bdf = ioapic_sbdf[idx].bdf;
seg = ioapic_sbdf[idx].seg;
- iommu = find_iommu_for_device(seg, bdf);
+ iommu = find_iommu_for_device(PCI_SBDF(seg, bdf));
if ( !iommu )
{
AMD_IOMMU_WARN("failed to find IOMMU for IO-APIC @ %04x:%04x\n",
@@ -383,7 +383,7 @@ unsigned int cf_check amd_iommu_read_ioapic_from_ire(
seg = ioapic_sbdf[idx].seg;
bdf = ioapic_sbdf[idx].bdf;
- iommu = find_iommu_for_device(seg, bdf);
+ iommu = find_iommu_for_device(PCI_SBDF(seg, bdf));
if ( !iommu )
return val;
req_id = get_intremap_requestor_id(seg, bdf);
@@ -495,19 +495,19 @@ static int update_intremap_entry_from_msi_msg(
return fresh;
}
-static struct amd_iommu *_find_iommu_for_device(int seg, int bdf)
+static struct amd_iommu *_find_iommu_for_device(pci_sbdf_t sbdf)
{
struct amd_iommu *iommu;
for_each_amd_iommu ( iommu )
- if ( iommu->seg == seg && iommu->bdf == bdf )
+ if ( iommu->sbdf.sbdf == sbdf.sbdf )
return NULL;
- iommu = find_iommu_for_device(seg, bdf);
+ iommu = find_iommu_for_device(sbdf);
if ( iommu )
return iommu;
- AMD_IOMMU_DEBUG("No IOMMU for MSI dev = %pp\n", &PCI_SBDF(seg, bdf));
+ AMD_IOMMU_DEBUG("No IOMMU for MSI dev = %pp\n", &sbdf);
return ERR_PTR(-EINVAL);
}
@@ -523,7 +523,7 @@ int cf_check amd_iommu_msi_msg_update_ire(
bdf = pdev ? pdev->sbdf.bdf : hpet_sbdf.bdf;
seg = pdev ? pdev->seg : hpet_sbdf.seg;
- iommu = _find_iommu_for_device(seg, bdf);
+ iommu = _find_iommu_for_device(PCI_SBDF(seg, bdf));
if ( IS_ERR_OR_NULL(iommu) )
return PTR_ERR(iommu);
@@ -660,8 +660,8 @@ bool __init cf_check iov_supports_xt(void)
if ( idx == MAX_IO_APICS )
return false;
- if ( !find_iommu_for_device(ioapic_sbdf[idx].seg,
- ioapic_sbdf[idx].bdf) )
+ if ( !find_iommu_for_device(PCI_SBDF(ioapic_sbdf[idx].seg,
+ ioapic_sbdf[idx].bdf)) )
{
AMD_IOMMU_WARN("no IOMMU for IO-APIC %#x (ID %x)\n",
apic, IO_APIC_ID(apic));
@@ -690,7 +690,7 @@ int __init cf_check amd_setup_hpet_msi(struct msi_desc *msi_desc)
return -ENODEV;
}
- iommu = find_iommu_for_device(hpet_sbdf.seg, hpet_sbdf.bdf);
+ iommu = find_iommu_for_device(PCI_SBDF(hpet_sbdf.seg, hpet_sbdf.bdf));
if ( !iommu )
return -ENXIO;
diff --git a/xen/drivers/passthrough/amd/iommu_map.c b/xen/drivers/passthrough/amd/iommu_map.c
index ea68edc0f2..48d6498868 100644
--- a/xen/drivers/passthrough/amd/iommu_map.c
+++ b/xen/drivers/passthrough/amd/iommu_map.c
@@ -717,7 +717,7 @@ int cf_check amd_iommu_get_reserved_device_memory(
pcidevs_unlock();
if ( pdev )
- iommu = find_iommu_for_device(seg, bdf);
+ iommu = find_iommu_for_device(sbdf);
if ( !iommu )
continue;
}
diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index d00697edb3..16bab0f948 100644
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -32,35 +32,35 @@ static bool __read_mostly init_done;
static const struct iommu_init_ops _iommu_init_ops;
-struct amd_iommu *find_iommu_for_device(int seg, int bdf)
+struct amd_iommu *find_iommu_for_device(pci_sbdf_t sbdf)
{
- struct ivrs_mappings *ivrs_mappings = get_ivrs_mappings(seg);
+ struct ivrs_mappings *ivrs_mappings = get_ivrs_mappings(sbdf.seg);
- if ( !ivrs_mappings || bdf >= ivrs_bdf_entries )
+ if ( !ivrs_mappings || sbdf.bdf >= ivrs_bdf_entries )
return NULL;
- if ( unlikely(!ivrs_mappings[bdf].iommu) && likely(init_done) )
+ if ( unlikely(!ivrs_mappings[sbdf.bdf].iommu) && likely(init_done) )
{
- unsigned int bd0 = bdf & ~PCI_FUNC(~0);
+ unsigned int bd0 = sbdf.bdf & ~PCI_FUNC(~0);
- if ( ivrs_mappings[bd0].iommu && ivrs_mappings[bd0].iommu->bdf != bdf )
+ if ( ivrs_mappings[bd0].iommu && ivrs_mappings[bd0].iommu->bdf != sbdf.bdf )
{
struct ivrs_mappings tmp = ivrs_mappings[bd0];
tmp.iommu = NULL;
if ( tmp.dte_requestor_id == bd0 )
- tmp.dte_requestor_id = bdf;
- ivrs_mappings[bdf] = tmp;
+ tmp.dte_requestor_id = sbdf.bdf;
+ ivrs_mappings[sbdf.bdf] = tmp;
printk(XENLOG_WARNING "%pp not found in ACPI tables;"
- " using same IOMMU as function 0\n", &PCI_SBDF(seg, bdf));
+ " using same IOMMU as function 0\n", &sbdf);
/* write iommu field last */
- ivrs_mappings[bdf].iommu = ivrs_mappings[bd0].iommu;
+ ivrs_mappings[sbdf.bdf].iommu = ivrs_mappings[bd0].iommu;
}
}
- return ivrs_mappings[bdf].iommu;
+ return ivrs_mappings[sbdf.bdf].iommu;
}
/*
@@ -107,7 +107,7 @@ static bool any_pdev_behind_iommu(const struct domain *d,
if ( pdev == exclude )
continue;
- if ( find_iommu_for_device(pdev->seg, pdev->sbdf.bdf) == iommu )
+ if ( find_iommu_for_device(pdev->sbdf) == iommu )
return true;
}
@@ -468,7 +468,7 @@ static int cf_check reassign_device(
struct amd_iommu *iommu;
int rc;
- iommu = find_iommu_for_device(pdev->seg, pdev->sbdf.bdf);
+ iommu = find_iommu_for_device(pdev->sbdf);
if ( !iommu )
{
AMD_IOMMU_WARN("failed to find IOMMU: %pp cannot be assigned to %pd\n",
@@ -578,10 +578,10 @@ static int cf_check amd_iommu_add_device(u8 devfn, struct pci_dev *pdev)
return -EINVAL;
for_each_amd_iommu(iommu)
- if ( pdev->seg == iommu->seg && pdev->sbdf.bdf == iommu->bdf )
+ if ( pdev->sbdf.sbdf == iommu->sbdf.sbdf )
return is_hardware_domain(pdev->domain) ? 0 : -ENODEV;
- iommu = find_iommu_for_device(pdev->seg, pdev->sbdf.bdf);
+ iommu = find_iommu_for_device(pdev->sbdf);
if ( unlikely(!iommu) )
{
/* Filter bridge devices. */
@@ -666,7 +666,7 @@ static int cf_check amd_iommu_remove_device(u8 devfn, struct pci_dev *pdev)
if ( !pdev->domain )
return -EINVAL;
- iommu = find_iommu_for_device(pdev->seg, pdev->sbdf.bdf);
+ iommu = find_iommu_for_device(pdev->sbdf);
if ( !iommu )
{
AMD_IOMMU_WARN("failed to find IOMMU: %pp cannot be removed from %pd\n",
--
2.48.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v2 2/3] drivers: Change find_iommu_for_device function to take pci_sbdf_t, simplify code
2025-03-16 10:08 ` [PATCH v2 2/3] drivers: Change find_iommu_for_device function to take " Andrii Sultanov
@ 2025-03-17 16:07 ` Jan Beulich
0 siblings, 0 replies; 7+ messages in thread
From: Jan Beulich @ 2025-03-17 16:07 UTC (permalink / raw)
To: Andrii Sultanov; +Cc: Andrew Cooper, Roger Pau Monné, xen-devel
On 16.03.2025 11:08, Andrii Sultanov wrote:
> Following a similar change to amd_iommu struct, change the
> find_iommu_for_device function to take pci_sbdf_t as a single parameter.
> This avoids splitting a single argument into two registers in the majority
> of cases.
>
> Bloat-o-meter reports (on top of the first patch in the series):
> add/remove: 0/0 grow/shrink: 11/13 up/down: 90/-154 (-64)
> Function old new delta
> amd_iommu_get_supported_ivhd_type 54 86 +32
> parse_ivrs_table 3955 3966 +11
> amd_iommu_assign_device 271 282 +11
> __mon_lengths 2928 2936 +8
> update_intremap_entry_from_msi_msg 859 864 +5
> iov_supports_xt 270 275 +5
> amd_setup_hpet_msi 232 237 +5
> amd_iommu_domain_destroy 43 48 +5
> find_iommu_for_device 242 246 +4
> amd_iommu_ioapic_update_ire 572 575 +3
> allocate_domain_resources 82 83 +1
> amd_iommu_read_ioapic_from_ire 347 344 -3
> reassign_device 843 838 -5
> amd_iommu_remove_device 352 347 -5
> amd_iommu_get_reserved_device_memory 524 519 -5
> amd_iommu_flush_iotlb 359 354 -5
> amd_iommu_add_device 844 839 -5
> amd_iommu_setup_domain_device 1478 1472 -6
> build_info 752 744 -8
> amd_iommu_detect_one_acpi 886 876 -10
> register_range_for_device 297 281 -16
> amd_iommu_msi_msg_update_ire 472 448 -24
> parse_ivmd_block 1339 1312 -27
> _hvm_dpci_msi_eoi 168 133 -35
>
> Signed-off-by: Andrii Sultanov <sultanovandriy@gmail.com>
>
> ---
>> This avoids splitting a single argument into two registers
> And adds a few conversions into a pci_sbdf_t... these are addressed further
> in the series. - not part of the commit message in case said commit isn't taken.
What is "said commit" here? Everything done in a patch should be in the
commit message. If parts are later dropped, the commit message would need
refining. With the commit message amended (assuming I guessed correctly
that this is about a few secondary changes done here):
Acked-by: Jan Beulich <jbeulich@suse.com>
Jan
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 3/3] drivers: Make ioapic_sbdf and hpet_sbdf contain pci_sbdf_t
2025-03-16 10:08 [PATCH v2 0/3] drivers: Simplify handling of pci_sbdf_t in passthrough/amd Andrii Sultanov
2025-03-16 10:08 ` [PATCH v2 1/3] drivers: Change amd_iommu struct to contain pci_sbdf_t, simplify code Andrii Sultanov
2025-03-16 10:08 ` [PATCH v2 2/3] drivers: Change find_iommu_for_device function to take " Andrii Sultanov
@ 2025-03-16 10:08 ` Andrii Sultanov
2025-03-17 16:17 ` Jan Beulich
2 siblings, 1 reply; 7+ messages in thread
From: Andrii Sultanov @ 2025-03-16 10:08 UTC (permalink / raw)
To: xen-devel
Cc: Andrii Sultanov, Jan Beulich, Andrew Cooper, Roger Pau Monné
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.
Signed-off-by: Andrii Sultanov <sultanovandriy@gmail.com>
---
This particular commit does not have any effect on code size:
add/remove: 0/0 grow/shrink: 2/6 up/down: 96/-96 (0)
Function old new delta
_einittext 22092 22156 +64
_hvm_dpci_msi_eoi 133 165 +32
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
amd_iommu_get_supported_ivhd_type 86 54 -32
So we don't have to take it - but imho it significantly simplifies
source code and makes it more readable.
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 | 17 ++++++++--
xen/drivers/passthrough/amd/iommu_intr.c | 40 +++++++++++-------------
2 files changed, 34 insertions(+), 23 deletions(-)
diff --git a/xen/drivers/passthrough/amd/iommu.h b/xen/drivers/passthrough/amd/iommu.h
index a9e64b8304..6903b1bc5d 100644
--- a/xen/drivers/passthrough/amd/iommu.h
+++ b/xen/drivers/passthrough/amd/iommu.h
@@ -268,7 +268,13 @@ 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;
+ union {
+ struct {
+ uint16_t bdf;
+ uint16_t seg;
+ };
+ pci_sbdf_t sbdf;
+ };
u8 id;
bool cmdline;
u16 *pin_2_idx;
@@ -279,7 +285,14 @@ 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;
+ union {
+ struct {
+ uint16_t bdf;
+ uint16_t seg;
+ };
+ pci_sbdf_t sbdf;
+ };
+ uint16_t id;
enum {
HPET_NONE,
HPET_CMDL,
diff --git a/xen/drivers/passthrough/amd/iommu_intr.c b/xen/drivers/passthrough/amd/iommu_intr.c
index 4d7dc2fdb4..e31ab12b60 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,18 @@ 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);
+ AMD_IOMMU_WARN("failed to find IOMMU for IO-APIC @ %pp\n", &sbdf);
__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 +368,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 +381,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 +514,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 +531,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 +542,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 +659,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,7 +688,7 @@ 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;
--
2.48.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v2 3/3] drivers: Make ioapic_sbdf and hpet_sbdf contain pci_sbdf_t
2025-03-16 10:08 ` [PATCH v2 3/3] drivers: Make ioapic_sbdf and hpet_sbdf contain pci_sbdf_t Andrii Sultanov
@ 2025-03-17 16:17 ` Jan Beulich
0 siblings, 0 replies; 7+ messages in thread
From: Jan Beulich @ 2025-03-17 16:17 UTC (permalink / raw)
To: Andrii Sultanov; +Cc: Andrew Cooper, Roger Pau Monné, xen-devel
On 16.03.2025 11:08, 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.
>
> Signed-off-by: Andrii Sultanov <sultanovandriy@gmail.com>
>
> ---
>
> This particular commit does not have any effect on code size:
> add/remove: 0/0 grow/shrink: 2/6 up/down: 96/-96 (0)
> Function old new delta
> _einittext 22092 22156 +64
I'm puzzled by this: _einittext being last (and not itself a function);
how can it grow? What follows it (as per the linker script) is
.altinstr_replacement, yet I wouldn't expect that to change in size.
> --- a/xen/drivers/passthrough/amd/iommu.h
> +++ b/xen/drivers/passthrough/amd/iommu.h
> @@ -268,7 +268,13 @@ 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;
> + union {
> + struct {
> + uint16_t bdf;
> + uint16_t seg;
> + };
> + pci_sbdf_t sbdf;
> + };
> u8 id;
> bool cmdline;
> u16 *pin_2_idx;
> @@ -279,7 +285,14 @@ 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;
> + union {
> + struct {
> + uint16_t bdf;
> + uint16_t seg;
> + };
> + pci_sbdf_t sbdf;
> + };
> + uint16_t id;
> enum {
> HPET_NONE,
> HPET_CMDL,
Oh, yet more fragile aliasing. No, please don't. Just have a single pci_sbdf_t
there.
Jan
^ permalink raw reply [flat|nested] 7+ messages in thread