* [PATCH v4 0/3] drivers: Simplify handling of pci_sbdf_t in passthrough/amd
@ 2025-04-14 19:19 Andrii Sultanov
2025-04-14 19:19 ` [PATCH v4 1/3] drivers: Change amd_iommu struct to contain pci_sbdf_t, simplify code Andrii Sultanov
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Andrii Sultanov @ 2025-04-14 19:19 UTC (permalink / raw)
To: xen-devel
Cc: Andrii Sultanov, Jan Beulich, Andrew Cooper, Roger Pau Monné
Step-by-step, use pci_sbdf_t directly where appropriate instead of
handling seg and bdf separately. This removes conversions, reduces code
size and simplifies code in general.
Andrii Sultanov (3):
drivers: Change amd_iommu struct to contain pci_sbdf_t, simplify code
drivers: Change find_iommu_for_device function to take pci_sbdf_t,
simplify code
drivers: Make ioapic_sbdf and hpet_sbdf contain pci_sbdf_t
xen/drivers/passthrough/amd/iommu.h | 11 +--
xen/drivers/passthrough/amd/iommu_acpi.c | 58 ++++++++--------
xen/drivers/passthrough/amd/iommu_cmd.c | 10 +--
xen/drivers/passthrough/amd/iommu_detect.c | 18 ++---
xen/drivers/passthrough/amd/iommu_init.c | 39 +++++------
xen/drivers/passthrough/amd/iommu_intr.c | 76 ++++++++++-----------
xen/drivers/passthrough/amd/iommu_map.c | 6 +-
xen/drivers/passthrough/amd/pci_amd_iommu.c | 50 +++++++-------
8 files changed, 130 insertions(+), 138 deletions(-)
--
2.49.0
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH v4 1/3] drivers: Change amd_iommu struct to contain pci_sbdf_t, simplify code 2025-04-14 19:19 [PATCH v4 0/3] drivers: Simplify handling of pci_sbdf_t in passthrough/amd Andrii Sultanov @ 2025-04-14 19:19 ` Andrii Sultanov 2025-04-15 7:34 ` dmkhn 2025-04-15 10:36 ` Jan Beulich 2025-04-14 19:19 ` [PATCH v4 2/3] drivers: Change find_iommu_for_device function to take " Andrii Sultanov 2025-04-14 19:19 ` [PATCH v4 3/3] drivers: Make ioapic_sbdf and hpet_sbdf contain pci_sbdf_t Andrii Sultanov 2 siblings, 2 replies; 13+ messages in thread From: Andrii Sultanov @ 2025-04-14 19:19 UTC (permalink / raw) To: xen-devel Cc: Andrii Sultanov, Jan Beulich, Andrew Cooper, Roger Pau Monné From: Andrii Sultanov <sultanovandriy@gmail.com> Following on from 250d87dc3ff9 ("x86/msi: Change __msi_set_enable() to take pci_sbdf_t"), make struct amd_iommu contain pci_sbdf_t directly instead of specifying seg+bdf separately and regenerating sbdf_t from them, which simplifies code. Bloat-o-meter reports: add/remove: 0/0 grow/shrink: 4/13 up/down: 121/-377 (-256) Function old new delta _einittext 22028 22092 +64 amd_iommu_prepare 853 897 +44 __mon_lengths 2928 2936 +8 _invalidate_all_devices 133 138 +5 _hvm_dpci_msi_eoi 157 155 -2 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_msi_msg_update_ire 472 448 -24 send_iommu_command 251 224 -27 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 V4: * Dropped references to the order of seg/bdf in the commit message * Dropped unnecessary detail from the commit message * Reverted to a macro usage in one case where it was mistakenly dropped * Folded several separate seg+bdf comparisons into a single one between sbdf_t, folded separate assignments with a macro. * More code size improvements with the changes, so I've refreshed the bloat-o-meter report Changes in V3: * Dropped the union with seg+bdf/pci_sbdf_t to avoid aliasing, renamed all users appropriately 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 | 4 +-- xen/drivers/passthrough/amd/iommu_acpi.c | 16 +++++----- xen/drivers/passthrough/amd/iommu_cmd.c | 8 ++--- xen/drivers/passthrough/amd/iommu_detect.c | 18 +++++------ xen/drivers/passthrough/amd/iommu_init.c | 35 ++++++++++----------- xen/drivers/passthrough/amd/iommu_intr.c | 29 ++++++++--------- xen/drivers/passthrough/amd/iommu_map.c | 4 +-- xen/drivers/passthrough/amd/pci_amd_iommu.c | 22 ++++++------- 8 files changed, 67 insertions(+), 69 deletions(-) diff --git a/xen/drivers/passthrough/amd/iommu.h b/xen/drivers/passthrough/amd/iommu.h index 00e81b4b2a..ba541f7943 100644 --- a/xen/drivers/passthrough/amd/iommu.h +++ b/xen/drivers/passthrough/amd/iommu.h @@ -77,8 +77,8 @@ struct amd_iommu { struct list_head list; spinlock_t lock; /* protect iommu */ - u16 seg; - u16 bdf; + pci_sbdf_t sbdf; + struct msi_desc msi; u16 cap_offset; diff --git a/xen/drivers/passthrough/amd/iommu_acpi.c b/xen/drivers/passthrough/amd/iommu_acpi.c index 5bdbfb5ba8..025d9be40f 100644 --- a/xen/drivers/passthrough/amd/iommu_acpi.c +++ b/xen/drivers/passthrough/amd/iommu_acpi.c @@ -58,7 +58,7 @@ static void __init add_ivrs_mapping_entry( uint16_t bdf, uint16_t alias_id, uint8_t flags, unsigned int ext_flags, bool alloc_irt, struct amd_iommu *iommu) { - struct ivrs_mappings *ivrs_mappings = get_ivrs_mappings(iommu->seg); + struct ivrs_mappings *ivrs_mappings = get_ivrs_mappings(iommu->sbdf.seg); ASSERT( ivrs_mappings != NULL ); @@ -70,7 +70,7 @@ static void __init add_ivrs_mapping_entry( ivrs_mappings[bdf].device_flags = flags; /* Don't map an IOMMU by itself. */ - if ( iommu->bdf == bdf ) + if ( iommu->sbdf.bdf == bdf ) return; /* Allocate interrupt remapping table if needed. */ @@ -96,7 +96,7 @@ static void __init add_ivrs_mapping_entry( if ( !ivrs_mappings[alias_id].intremap_table ) panic("No memory for %pp's IRT\n", - &PCI_SBDF(iommu->seg, alias_id)); + &PCI_SBDF(iommu->sbdf.seg, alias_id)); } } @@ -112,7 +112,7 @@ static struct amd_iommu * __init find_iommu_from_bdf_cap( struct amd_iommu *iommu; for_each_amd_iommu ( iommu ) - if ( (iommu->seg == seg) && (iommu->bdf == bdf) && + if ( (iommu->sbdf.seg == seg) && (iommu->sbdf.bdf == bdf) && (iommu->cap_offset == cap_offset) ) return iommu; @@ -297,13 +297,13 @@ 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(iommu->sbdf.seg, bdf) ) continue; - req = get_ivrs_mappings(iommu->seg)[bdf].dte_requestor_id; - rc = reserve_unity_map_for_device(iommu->seg, bdf, base, length, + req = get_ivrs_mappings(iommu->sbdf.seg)[bdf].dte_requestor_id; + rc = reserve_unity_map_for_device(iommu->sbdf.seg, bdf, base, length, iw, ir, false) ?: - reserve_unity_map_for_device(iommu->seg, req, base, length, + reserve_unity_map_for_device(iommu->sbdf.seg, req, base, length, iw, ir, false); } diff --git a/xen/drivers/passthrough/amd/iommu_cmd.c b/xen/drivers/passthrough/amd/iommu_cmd.c index 83c525b84f..eefd626161 100644 --- a/xen/drivers/passthrough/amd/iommu_cmd.c +++ b/xen/drivers/passthrough/amd/iommu_cmd.c @@ -40,7 +40,7 @@ static void send_iommu_command(struct amd_iommu *iommu, IOMMU_RING_BUFFER_PTR_MASK) ) { printk_once(XENLOG_ERR "AMD IOMMU %pp: no cmd slot available\n", - &PCI_SBDF(iommu->seg, iommu->bdf)); + &iommu->sbdf); cpu_relax(); } @@ -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); } @@ -300,7 +300,7 @@ void amd_iommu_flush_iotlb(u8 devfn, const struct pci_dev *pdev, if ( !iommu_has_cap(iommu, PCI_CAP_IOTLB_SHIFT) ) return; - req_id = get_dma_requestor_id(iommu->seg, PCI_BDF(pdev->bus, devfn)); + req_id = get_dma_requestor_id(iommu->sbdf.seg, PCI_BDF(pdev->bus, devfn)); queueid = req_id; maxpend = pdev->ats.queue_depth & 0xff; diff --git a/xen/drivers/passthrough/amd/iommu_detect.c b/xen/drivers/passthrough/amd/iommu_detect.c index cede44e651..72cc554b08 100644 --- a/xen/drivers/passthrough/amd/iommu_detect.c +++ b/xen/drivers/passthrough/amd/iommu_detect.c @@ -162,8 +162,8 @@ int __init amd_iommu_detect_one_acpi( spin_lock_init(&iommu->lock); INIT_LIST_HEAD(&iommu->ats_devices); - iommu->seg = ivhd_block->pci_segment_group; - iommu->bdf = ivhd_block->header.device_id; + iommu->sbdf = PCI_SBDF(ivhd_block->pci_segment_group, + ivhd_block->header.device_id); iommu->cap_offset = ivhd_block->capability_offset; iommu->mmio_base_phys = ivhd_block->base_address; @@ -210,16 +210,16 @@ int __init amd_iommu_detect_one_acpi( /* override IOMMU HT flags */ iommu->ht_flags = ivhd_block->header.flags; - bus = PCI_BUS(iommu->bdf); - dev = PCI_SLOT(iommu->bdf); - func = PCI_FUNC(iommu->bdf); + bus = PCI_BUS(iommu->sbdf.bdf); + dev = PCI_SLOT(iommu->sbdf.bdf); + func = PCI_FUNC(iommu->sbdf.bdf); - rt = get_iommu_capabilities(iommu->seg, bus, dev, func, + rt = get_iommu_capabilities(iommu->sbdf.seg, bus, dev, func, iommu->cap_offset, iommu); if ( rt ) goto out; - rt = get_iommu_msi_capabilities(iommu->seg, bus, dev, func, iommu); + rt = get_iommu_msi_capabilities(iommu->sbdf.seg, bus, dev, func, iommu); if ( rt ) goto out; @@ -228,10 +228,10 @@ int __init amd_iommu_detect_one_acpi( if ( !iommu->domid_map ) goto out; - rt = pci_ro_device(iommu->seg, bus, PCI_DEVFN(dev, func)); + rt = pci_ro_device(iommu->sbdf.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..58d657060a 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) @@ -566,7 +564,7 @@ static void cf_check parse_event_log_entry(struct amd_iommu *iommu, u32 entry[]) printk(XENLOG_ERR "AMD-Vi: %s: %pp d%u addr %016"PRIx64 " flags %#x%s%s%s%s%s%s%s%s%s%s\n", - code_str, &PCI_SBDF(iommu->seg, device_id), + code_str, &PCI_SBDF(iommu->sbdf.seg, device_id), domain_id, addr, flags, (flags & 0xe00) ? " ??" : "", (flags & 0x100) ? " TR" : "", @@ -583,8 +581,8 @@ static void cf_check parse_event_log_entry(struct amd_iommu *iommu, u32 entry[]) amd_iommu_print_entries(iommu, device_id, daddr_to_dfn(addr)); for ( bdf = 0; bdf < ivrs_bdf_entries; bdf++ ) - if ( get_dma_requestor_id(iommu->seg, bdf) == device_id ) - pci_check_disable_device(iommu->seg, PCI_BUS(bdf), + if ( get_dma_requestor_id(iommu->sbdf.seg, bdf) == device_id ) + pci_check_disable_device(iommu->sbdf.seg, PCI_BUS(bdf), PCI_DEVFN(bdf)); } else @@ -643,7 +641,7 @@ static void cf_check parse_ppr_log_entry(struct amd_iommu *iommu, u32 entry[]) struct pci_dev *pdev; pcidevs_lock(); - pdev = pci_get_real_pdev(PCI_SBDF(iommu->seg, device_id)); + pdev = pci_get_real_pdev(PCI_SBDF(iommu->sbdf.seg, device_id)); pcidevs_unlock(); if ( pdev ) @@ -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) @@ -1095,7 +1092,7 @@ static int __init amd_iommu_init_one(struct amd_iommu *iommu, bool intr) goto error_out; /* Make sure that the device table has been successfully allocated. */ - ivrs_mappings = get_ivrs_mappings(iommu->seg); + ivrs_mappings = get_ivrs_mappings(iommu->sbdf.seg); if ( !IVRS_MAPPINGS_DEVTAB(ivrs_mappings) ) goto error_out; @@ -1363,7 +1360,7 @@ static bool __init amd_sp5100_erratum28(void) static int __init amd_iommu_prepare_one(struct amd_iommu *iommu) { - int rc = alloc_ivrs_mappings(iommu->seg); + int rc = alloc_ivrs_mappings(iommu->sbdf.seg); if ( !rc ) rc = map_iommu_mmio_region(iommu); diff --git a/xen/drivers/passthrough/amd/iommu_intr.c b/xen/drivers/passthrough/amd/iommu_intr.c index 9abdc38053..a7347fcbad 100644 --- a/xen/drivers/passthrough/amd/iommu_intr.c +++ b/xen/drivers/passthrough/amd/iommu_intr.c @@ -132,7 +132,7 @@ static int get_intremap_requestor_id(int seg, int bdf) static unsigned int alloc_intremap_entry(const struct amd_iommu *iommu, unsigned int bdf, unsigned int nr) { - const struct ivrs_mappings *ivrs_mappings = get_ivrs_mappings(iommu->seg); + const struct ivrs_mappings *ivrs_mappings = get_ivrs_mappings(iommu->sbdf.seg); unsigned long *inuse = ivrs_mappings[bdf].intremap_inuse; unsigned int nr_ents = intremap_table_entries(ivrs_mappings[bdf].intremap_table, iommu); @@ -167,7 +167,7 @@ static union irte_ptr get_intremap_entry(const struct amd_iommu *iommu, unsigned int bdf, unsigned int index) { union irte_ptr table = { - .ptr = get_ivrs_mappings(iommu->seg)[bdf].intremap_table + .ptr = get_ivrs_mappings(iommu->sbdf.seg)[bdf].intremap_table }; ASSERT(table.ptr && (index < intremap_table_entries(table.ptr, iommu))); @@ -184,7 +184,7 @@ static void free_intremap_entry(const struct amd_iommu *iommu, unsigned int bdf, unsigned int index) { union irte_ptr entry = get_intremap_entry(iommu, bdf, index); - struct ivrs_mappings *ivrs = get_ivrs_mappings(iommu->seg); + struct ivrs_mappings *ivrs = get_ivrs_mappings(iommu->sbdf.seg); if ( iommu->ctrl.ga_en ) { @@ -281,8 +281,8 @@ static int update_intremap_entry_from_ioapic( unsigned int dest, offset; bool fresh = false; - req_id = get_intremap_requestor_id(iommu->seg, bdf); - lock = get_intremap_lock(iommu->seg, req_id); + req_id = get_intremap_requestor_id(iommu->sbdf.seg, bdf); + lock = get_intremap_lock(iommu->sbdf.seg, req_id); delivery_mode = rte->delivery_mode; vector = rte->vector; @@ -419,10 +419,10 @@ static int update_intremap_entry_from_msi_msg( unsigned int dest, offset, i; bool fresh = false; - req_id = get_dma_requestor_id(iommu->seg, bdf); - alias_id = get_intremap_requestor_id(iommu->seg, bdf); + req_id = get_dma_requestor_id(iommu->sbdf.seg, bdf); + alias_id = get_intremap_requestor_id(iommu->sbdf.seg, bdf); - lock = get_intremap_lock(iommu->seg, req_id); + lock = get_intremap_lock(iommu->sbdf.seg, req_id); spin_lock_irqsave(lock, flags); if ( msg == NULL ) @@ -486,10 +486,10 @@ static int update_intremap_entry_from_msi_msg( */ if ( ( req_id != alias_id ) && - get_ivrs_mappings(iommu->seg)[alias_id].intremap_table != NULL ) + get_ivrs_mappings(iommu->sbdf.seg)[alias_id].intremap_table != NULL ) { - BUG_ON(get_ivrs_mappings(iommu->seg)[req_id].intremap_table != - get_ivrs_mappings(iommu->seg)[alias_id].intremap_table); + BUG_ON(get_ivrs_mappings(iommu->sbdf.seg)[req_id].intremap_table != + get_ivrs_mappings(iommu->sbdf.seg)[alias_id].intremap_table); } return fresh; @@ -498,16 +498,17 @@ static int update_intremap_entry_from_msi_msg( static struct amd_iommu *_find_iommu_for_device(int seg, int bdf) { struct amd_iommu *iommu; + pci_sbdf_t sbdf = PCI_SBDF(seg, bdf); 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); 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); } @@ -730,7 +731,7 @@ static void dump_intremap_table(const struct amd_iommu *iommu, if ( ivrs_mapping ) { printk(" %pp:\n", - &PCI_SBDF(iommu->seg, ivrs_mapping->dte_requestor_id)); + &PCI_SBDF(iommu->sbdf.seg, ivrs_mapping->dte_requestor_id)); ivrs_mapping = NULL; } diff --git a/xen/drivers/passthrough/amd/iommu_map.c b/xen/drivers/passthrough/amd/iommu_map.c index dde393645a..d28c475650 100644 --- a/xen/drivers/passthrough/amd/iommu_map.c +++ b/xen/drivers/passthrough/amd/iommu_map.c @@ -558,14 +558,14 @@ void amd_iommu_print_entries(const struct amd_iommu *iommu, unsigned int dev_id, if ( !dt[dev_id].tv ) { - printk("%pp: no root\n", &PCI_SBDF(iommu->seg, dev_id)); + printk("%pp: no root\n", &PCI_SBDF(iommu->sbdf.seg, dev_id)); return; } pt_mfn = _mfn(dt[dev_id].pt_root); level = dt[dev_id].paging_mode; printk("%pp root @ %"PRI_mfn" (%u levels) dfn=%"PRI_dfn"\n", - &PCI_SBDF(iommu->seg, dev_id), mfn_x(pt_mfn), level, dfn_x(dfn)); + &PCI_SBDF(iommu->sbdf.seg, dev_id), mfn_x(pt_mfn), level, dfn_x(dfn)); while ( level ) { diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c index d00697edb3..6b58e3380b 100644 --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c @@ -43,7 +43,7 @@ struct amd_iommu *find_iommu_for_device(int seg, int bdf) { unsigned int bd0 = bdf & ~PCI_FUNC(~0); - if ( ivrs_mappings[bd0].iommu && ivrs_mappings[bd0].iommu->bdf != bdf ) + if ( ivrs_mappings[bd0].iommu && ivrs_mappings[bd0].iommu->sbdf.bdf != bdf ) { struct ivrs_mappings tmp = ivrs_mappings[bd0]; @@ -121,7 +121,7 @@ static bool use_ats( { return !ivrs_dev->block_ats && iommu_has_cap(iommu, PCI_CAP_IOTLB_SHIFT) && - pci_ats_device(iommu->seg, pdev->bus, pdev->devfn); + pci_ats_device(iommu->sbdf.seg, pdev->bus, pdev->devfn); } static int __must_check amd_iommu_setup_domain_device( @@ -147,17 +147,17 @@ static int __must_check amd_iommu_setup_domain_device( if ( rc ) return rc; - req_id = get_dma_requestor_id(iommu->seg, pdev->sbdf.bdf); - ivrs_dev = &get_ivrs_mappings(iommu->seg)[req_id]; + req_id = get_dma_requestor_id(iommu->sbdf.seg, pdev->sbdf.bdf); + ivrs_dev = &get_ivrs_mappings(iommu->sbdf.seg)[req_id]; sr_flags = (iommu_hwdom_passthrough && is_hardware_domain(domain) ? 0 : SET_ROOT_VALID) | (ivrs_dev->unity_map ? SET_ROOT_WITH_UNITY_MAP : 0); /* get device-table entry */ - req_id = get_dma_requestor_id(iommu->seg, PCI_BDF(bus, devfn)); + req_id = get_dma_requestor_id(iommu->sbdf.seg, PCI_BDF(bus, devfn)); table = iommu->dev_table.buffer; dte = &table[req_id]; - ivrs_dev = &get_ivrs_mappings(iommu->seg)[req_id]; + ivrs_dev = &get_ivrs_mappings(iommu->sbdf.seg)[req_id]; if ( domain != dom_io ) { @@ -275,7 +275,7 @@ static int __must_check amd_iommu_setup_domain_device( ASSERT(pcidevs_locked()); if ( use_ats(pdev, iommu, ivrs_dev) && - !pci_ats_enabled(iommu->seg, bus, pdev->devfn) ) + !pci_ats_enabled(iommu->sbdf.seg, bus, pdev->devfn) ) { if ( devfn == pdev->devfn ) enable_ats_device(pdev, &iommu->ats_devices); @@ -418,12 +418,12 @@ static void amd_iommu_disable_domain_device(const struct domain *domain, ASSERT(pcidevs_locked()); - if ( pci_ats_device(iommu->seg, bus, pdev->devfn) && - pci_ats_enabled(iommu->seg, bus, pdev->devfn) ) + if ( pci_ats_device(iommu->sbdf.seg, bus, pdev->devfn) && + pci_ats_enabled(iommu->sbdf.seg, bus, pdev->devfn) ) disable_ats_device(pdev); BUG_ON ( iommu->dev_table.buffer == NULL ); - req_id = get_dma_requestor_id(iommu->seg, PCI_BDF(bus, devfn)); + req_id = get_dma_requestor_id(iommu->sbdf.seg, PCI_BDF(bus, devfn)); table = iommu->dev_table.buffer; dte = &table[req_id]; @@ -578,7 +578,7 @@ 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); -- 2.49.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v4 1/3] drivers: Change amd_iommu struct to contain pci_sbdf_t, simplify code 2025-04-14 19:19 ` [PATCH v4 1/3] drivers: Change amd_iommu struct to contain pci_sbdf_t, simplify code Andrii Sultanov @ 2025-04-15 7:34 ` dmkhn 2025-04-15 7:38 ` Jan Beulich 2025-04-15 7:56 ` Andriy Sultanov 2025-04-15 10:36 ` Jan Beulich 1 sibling, 2 replies; 13+ messages in thread From: dmkhn @ 2025-04-15 7:34 UTC (permalink / raw) To: Andrii Sultanov Cc: xen-devel, Jan Beulich, Andrew Cooper, Roger Pau Monné On Mon, Apr 14, 2025 at 08:19:16PM +0100, Andrii Sultanov wrote: > From: Andrii Sultanov <sultanovandriy@gmail.com> > > Following on from 250d87dc3ff9 ("x86/msi: Change __msi_set_enable() to > take pci_sbdf_t"), make struct amd_iommu contain pci_sbdf_t directly > instead of specifying seg+bdf separately and regenerating sbdf_t from them, > which simplifies code. > > Bloat-o-meter reports: > add/remove: 0/0 grow/shrink: 4/13 up/down: 121/-377 (-256) > Function old new delta > _einittext 22028 22092 +64 > amd_iommu_prepare 853 897 +44 > __mon_lengths 2928 2936 +8 > _invalidate_all_devices 133 138 +5 > _hvm_dpci_msi_eoi 157 155 -2 > 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_msi_msg_update_ire 472 448 -24 > send_iommu_command 251 224 -27 > 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 V4: > * Dropped references to the order of seg/bdf in the commit message > * Dropped unnecessary detail from the commit message > * Reverted to a macro usage in one case where it was mistakenly dropped > * Folded several separate seg+bdf comparisons into a single one between > sbdf_t, folded separate assignments with a macro. > * More code size improvements with the changes, so I've refreshed the > bloat-o-meter report > > Changes in V3: > * Dropped the union with seg+bdf/pci_sbdf_t to avoid aliasing, renamed > all users appropriately > > 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 | 4 +-- > xen/drivers/passthrough/amd/iommu_acpi.c | 16 +++++----- > xen/drivers/passthrough/amd/iommu_cmd.c | 8 ++--- > xen/drivers/passthrough/amd/iommu_detect.c | 18 +++++------ > xen/drivers/passthrough/amd/iommu_init.c | 35 ++++++++++----------- > xen/drivers/passthrough/amd/iommu_intr.c | 29 ++++++++--------- > xen/drivers/passthrough/amd/iommu_map.c | 4 +-- > xen/drivers/passthrough/amd/pci_amd_iommu.c | 22 ++++++------- > 8 files changed, 67 insertions(+), 69 deletions(-) > > diff --git a/xen/drivers/passthrough/amd/iommu.h b/xen/drivers/passthrough/amd/iommu.h > index 00e81b4b2a..ba541f7943 100644 > --- a/xen/drivers/passthrough/amd/iommu.h > +++ b/xen/drivers/passthrough/amd/iommu.h > @@ -77,8 +77,8 @@ struct amd_iommu { > struct list_head list; > spinlock_t lock; /* protect iommu */ > > - u16 seg; > - u16 bdf; > + pci_sbdf_t sbdf; > + > struct msi_desc msi; > > u16 cap_offset; > diff --git a/xen/drivers/passthrough/amd/iommu_acpi.c b/xen/drivers/passthrough/amd/iommu_acpi.c > index 5bdbfb5ba8..025d9be40f 100644 > --- a/xen/drivers/passthrough/amd/iommu_acpi.c > +++ b/xen/drivers/passthrough/amd/iommu_acpi.c > @@ -58,7 +58,7 @@ static void __init add_ivrs_mapping_entry( > uint16_t bdf, uint16_t alias_id, uint8_t flags, unsigned int ext_flags, > bool alloc_irt, struct amd_iommu *iommu) > { > - struct ivrs_mappings *ivrs_mappings = get_ivrs_mappings(iommu->seg); > + struct ivrs_mappings *ivrs_mappings = get_ivrs_mappings(iommu->sbdf.seg); > > ASSERT( ivrs_mappings != NULL ); > > @@ -70,7 +70,7 @@ static void __init add_ivrs_mapping_entry( > ivrs_mappings[bdf].device_flags = flags; > > /* Don't map an IOMMU by itself. */ > - if ( iommu->bdf == bdf ) > + if ( iommu->sbdf.bdf == bdf ) > return; > > /* Allocate interrupt remapping table if needed. */ > @@ -96,7 +96,7 @@ static void __init add_ivrs_mapping_entry( > > if ( !ivrs_mappings[alias_id].intremap_table ) > panic("No memory for %pp's IRT\n", > - &PCI_SBDF(iommu->seg, alias_id)); > + &PCI_SBDF(iommu->sbdf.seg, alias_id)); > } > } > > @@ -112,7 +112,7 @@ static struct amd_iommu * __init find_iommu_from_bdf_cap( > struct amd_iommu *iommu; > > for_each_amd_iommu ( iommu ) > - if ( (iommu->seg == seg) && (iommu->bdf == bdf) && > + if ( (iommu->sbdf.seg == seg) && (iommu->sbdf.bdf == bdf) && Perhaps something like if ( (iommu->sbdf.sbdf == PCI_SBDF(seg, bdf).sbdf && ? > (iommu->cap_offset == cap_offset) ) > return iommu; > > @@ -297,13 +297,13 @@ 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(iommu->sbdf.seg, bdf) ) > continue; > > - req = get_ivrs_mappings(iommu->seg)[bdf].dte_requestor_id; > - rc = reserve_unity_map_for_device(iommu->seg, bdf, base, length, > + req = get_ivrs_mappings(iommu->sbdf.seg)[bdf].dte_requestor_id; > + rc = reserve_unity_map_for_device(iommu->sbdf.seg, bdf, base, length, > iw, ir, false) ?: > - reserve_unity_map_for_device(iommu->seg, req, base, length, > + reserve_unity_map_for_device(iommu->sbdf.seg, req, base, length, > iw, ir, false); > } > > diff --git a/xen/drivers/passthrough/amd/iommu_cmd.c b/xen/drivers/passthrough/amd/iommu_cmd.c > index 83c525b84f..eefd626161 100644 > --- a/xen/drivers/passthrough/amd/iommu_cmd.c > +++ b/xen/drivers/passthrough/amd/iommu_cmd.c > @@ -40,7 +40,7 @@ static void send_iommu_command(struct amd_iommu *iommu, > IOMMU_RING_BUFFER_PTR_MASK) ) > { > printk_once(XENLOG_ERR "AMD IOMMU %pp: no cmd slot available\n", > - &PCI_SBDF(iommu->seg, iommu->bdf)); > + &iommu->sbdf); > cpu_relax(); > } > > @@ -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); > } > @@ -300,7 +300,7 @@ void amd_iommu_flush_iotlb(u8 devfn, const struct pci_dev *pdev, > if ( !iommu_has_cap(iommu, PCI_CAP_IOTLB_SHIFT) ) > return; > > - req_id = get_dma_requestor_id(iommu->seg, PCI_BDF(pdev->bus, devfn)); > + req_id = get_dma_requestor_id(iommu->sbdf.seg, PCI_BDF(pdev->bus, devfn)); > queueid = req_id; > maxpend = pdev->ats.queue_depth & 0xff; > > diff --git a/xen/drivers/passthrough/amd/iommu_detect.c b/xen/drivers/passthrough/amd/iommu_detect.c > index cede44e651..72cc554b08 100644 > --- a/xen/drivers/passthrough/amd/iommu_detect.c > +++ b/xen/drivers/passthrough/amd/iommu_detect.c > @@ -162,8 +162,8 @@ int __init amd_iommu_detect_one_acpi( > spin_lock_init(&iommu->lock); > INIT_LIST_HEAD(&iommu->ats_devices); > > - iommu->seg = ivhd_block->pci_segment_group; > - iommu->bdf = ivhd_block->header.device_id; > + iommu->sbdf = PCI_SBDF(ivhd_block->pci_segment_group, > + ivhd_block->header.device_id); > iommu->cap_offset = ivhd_block->capability_offset; > iommu->mmio_base_phys = ivhd_block->base_address; > > @@ -210,16 +210,16 @@ int __init amd_iommu_detect_one_acpi( > /* override IOMMU HT flags */ > iommu->ht_flags = ivhd_block->header.flags; > > - bus = PCI_BUS(iommu->bdf); > - dev = PCI_SLOT(iommu->bdf); > - func = PCI_FUNC(iommu->bdf); > + bus = PCI_BUS(iommu->sbdf.bdf); > + dev = PCI_SLOT(iommu->sbdf.bdf); > + func = PCI_FUNC(iommu->sbdf.bdf); > > - rt = get_iommu_capabilities(iommu->seg, bus, dev, func, > + rt = get_iommu_capabilities(iommu->sbdf.seg, bus, dev, func, I would update signature of get_iommu_capabilities() so it takes pci_sbdf_t as an agument instead of disaggregated PCI address. I think it will simplify the code futher. > iommu->cap_offset, iommu); > if ( rt ) > goto out; > > - rt = get_iommu_msi_capabilities(iommu->seg, bus, dev, func, iommu); > + rt = get_iommu_msi_capabilities(iommu->sbdf.seg, bus, dev, func, iommu); ... and same idea for get_iommu_msi_capabilities() What do you think? > if ( rt ) > goto out; > > @@ -228,10 +228,10 @@ int __init amd_iommu_detect_one_acpi( > if ( !iommu->domid_map ) > goto out; > > - rt = pci_ro_device(iommu->seg, bus, PCI_DEVFN(dev, func)); > + rt = pci_ro_device(iommu->sbdf.seg, bus, PCI_DEVFN(dev, func)); There's not so many users of pci_ro_device(). I think it makes sense to update pci_ro_device() to something like: int pci_ro_device(pci_sbdf_t pciaddr); But probably in a separate code change. > 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..58d657060a 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) > @@ -566,7 +564,7 @@ static void cf_check parse_event_log_entry(struct amd_iommu *iommu, u32 entry[]) > > printk(XENLOG_ERR "AMD-Vi: %s: %pp d%u addr %016"PRIx64 > " flags %#x%s%s%s%s%s%s%s%s%s%s\n", > - code_str, &PCI_SBDF(iommu->seg, device_id), > + code_str, &PCI_SBDF(iommu->sbdf.seg, device_id), > domain_id, addr, flags, > (flags & 0xe00) ? " ??" : "", > (flags & 0x100) ? " TR" : "", > @@ -583,8 +581,8 @@ static void cf_check parse_event_log_entry(struct amd_iommu *iommu, u32 entry[]) > amd_iommu_print_entries(iommu, device_id, daddr_to_dfn(addr)); > > for ( bdf = 0; bdf < ivrs_bdf_entries; bdf++ ) > - if ( get_dma_requestor_id(iommu->seg, bdf) == device_id ) > - pci_check_disable_device(iommu->seg, PCI_BUS(bdf), > + if ( get_dma_requestor_id(iommu->sbdf.seg, bdf) == device_id ) > + pci_check_disable_device(iommu->sbdf.seg, PCI_BUS(bdf), > PCI_DEVFN(bdf)); > } > else > @@ -643,7 +641,7 @@ static void cf_check parse_ppr_log_entry(struct amd_iommu *iommu, u32 entry[]) > struct pci_dev *pdev; > > pcidevs_lock(); > - pdev = pci_get_real_pdev(PCI_SBDF(iommu->seg, device_id)); > + pdev = pci_get_real_pdev(PCI_SBDF(iommu->sbdf.seg, device_id)); > pcidevs_unlock(); > > if ( pdev ) > @@ -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) > @@ -1095,7 +1092,7 @@ static int __init amd_iommu_init_one(struct amd_iommu *iommu, bool intr) > goto error_out; > > /* Make sure that the device table has been successfully allocated. */ > - ivrs_mappings = get_ivrs_mappings(iommu->seg); > + ivrs_mappings = get_ivrs_mappings(iommu->sbdf.seg); > if ( !IVRS_MAPPINGS_DEVTAB(ivrs_mappings) ) > goto error_out; > > @@ -1363,7 +1360,7 @@ static bool __init amd_sp5100_erratum28(void) > > static int __init amd_iommu_prepare_one(struct amd_iommu *iommu) > { > - int rc = alloc_ivrs_mappings(iommu->seg); > + int rc = alloc_ivrs_mappings(iommu->sbdf.seg); > > if ( !rc ) > rc = map_iommu_mmio_region(iommu); > diff --git a/xen/drivers/passthrough/amd/iommu_intr.c b/xen/drivers/passthrough/amd/iommu_intr.c > index 9abdc38053..a7347fcbad 100644 > --- a/xen/drivers/passthrough/amd/iommu_intr.c > +++ b/xen/drivers/passthrough/amd/iommu_intr.c > @@ -132,7 +132,7 @@ static int get_intremap_requestor_id(int seg, int bdf) > static unsigned int alloc_intremap_entry(const struct amd_iommu *iommu, > unsigned int bdf, unsigned int nr) > { > - const struct ivrs_mappings *ivrs_mappings = get_ivrs_mappings(iommu->seg); > + const struct ivrs_mappings *ivrs_mappings = get_ivrs_mappings(iommu->sbdf.seg); > unsigned long *inuse = ivrs_mappings[bdf].intremap_inuse; > unsigned int nr_ents = > intremap_table_entries(ivrs_mappings[bdf].intremap_table, iommu); > @@ -167,7 +167,7 @@ static union irte_ptr get_intremap_entry(const struct amd_iommu *iommu, > unsigned int bdf, unsigned int index) > { > union irte_ptr table = { > - .ptr = get_ivrs_mappings(iommu->seg)[bdf].intremap_table > + .ptr = get_ivrs_mappings(iommu->sbdf.seg)[bdf].intremap_table > }; > > ASSERT(table.ptr && (index < intremap_table_entries(table.ptr, iommu))); > @@ -184,7 +184,7 @@ static void free_intremap_entry(const struct amd_iommu *iommu, > unsigned int bdf, unsigned int index) > { > union irte_ptr entry = get_intremap_entry(iommu, bdf, index); > - struct ivrs_mappings *ivrs = get_ivrs_mappings(iommu->seg); > + struct ivrs_mappings *ivrs = get_ivrs_mappings(iommu->sbdf.seg); > > if ( iommu->ctrl.ga_en ) > { > @@ -281,8 +281,8 @@ static int update_intremap_entry_from_ioapic( > unsigned int dest, offset; > bool fresh = false; > > - req_id = get_intremap_requestor_id(iommu->seg, bdf); > - lock = get_intremap_lock(iommu->seg, req_id); > + req_id = get_intremap_requestor_id(iommu->sbdf.seg, bdf); > + lock = get_intremap_lock(iommu->sbdf.seg, req_id); > > delivery_mode = rte->delivery_mode; > vector = rte->vector; > @@ -419,10 +419,10 @@ static int update_intremap_entry_from_msi_msg( > unsigned int dest, offset, i; > bool fresh = false; > > - req_id = get_dma_requestor_id(iommu->seg, bdf); > - alias_id = get_intremap_requestor_id(iommu->seg, bdf); > + req_id = get_dma_requestor_id(iommu->sbdf.seg, bdf); > + alias_id = get_intremap_requestor_id(iommu->sbdf.seg, bdf); > > - lock = get_intremap_lock(iommu->seg, req_id); > + lock = get_intremap_lock(iommu->sbdf.seg, req_id); > spin_lock_irqsave(lock, flags); > > if ( msg == NULL ) > @@ -486,10 +486,10 @@ static int update_intremap_entry_from_msi_msg( > */ > > if ( ( req_id != alias_id ) && > - get_ivrs_mappings(iommu->seg)[alias_id].intremap_table != NULL ) > + get_ivrs_mappings(iommu->sbdf.seg)[alias_id].intremap_table != NULL ) > { > - BUG_ON(get_ivrs_mappings(iommu->seg)[req_id].intremap_table != > - get_ivrs_mappings(iommu->seg)[alias_id].intremap_table); > + BUG_ON(get_ivrs_mappings(iommu->sbdf.seg)[req_id].intremap_table != > + get_ivrs_mappings(iommu->sbdf.seg)[alias_id].intremap_table); > } > > return fresh; > @@ -498,16 +498,17 @@ static int update_intremap_entry_from_msi_msg( > static struct amd_iommu *_find_iommu_for_device(int seg, int bdf) > { > struct amd_iommu *iommu; > + pci_sbdf_t sbdf = PCI_SBDF(seg, bdf); > > 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); > 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); > } > > @@ -730,7 +731,7 @@ static void dump_intremap_table(const struct amd_iommu *iommu, > if ( ivrs_mapping ) > { > printk(" %pp:\n", > - &PCI_SBDF(iommu->seg, ivrs_mapping->dte_requestor_id)); > + &PCI_SBDF(iommu->sbdf.seg, ivrs_mapping->dte_requestor_id)); > ivrs_mapping = NULL; > } > > diff --git a/xen/drivers/passthrough/amd/iommu_map.c b/xen/drivers/passthrough/amd/iommu_map.c > index dde393645a..d28c475650 100644 > --- a/xen/drivers/passthrough/amd/iommu_map.c > +++ b/xen/drivers/passthrough/amd/iommu_map.c > @@ -558,14 +558,14 @@ void amd_iommu_print_entries(const struct amd_iommu *iommu, unsigned int dev_id, > > if ( !dt[dev_id].tv ) > { > - printk("%pp: no root\n", &PCI_SBDF(iommu->seg, dev_id)); > + printk("%pp: no root\n", &PCI_SBDF(iommu->sbdf.seg, dev_id)); > return; > } > > pt_mfn = _mfn(dt[dev_id].pt_root); > level = dt[dev_id].paging_mode; > printk("%pp root @ %"PRI_mfn" (%u levels) dfn=%"PRI_dfn"\n", > - &PCI_SBDF(iommu->seg, dev_id), mfn_x(pt_mfn), level, dfn_x(dfn)); > + &PCI_SBDF(iommu->sbdf.seg, dev_id), mfn_x(pt_mfn), level, dfn_x(dfn)); > > while ( level ) > { > diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c > index d00697edb3..6b58e3380b 100644 > --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c > +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c > @@ -43,7 +43,7 @@ struct amd_iommu *find_iommu_for_device(int seg, int bdf) > { > unsigned int bd0 = bdf & ~PCI_FUNC(~0); > > - if ( ivrs_mappings[bd0].iommu && ivrs_mappings[bd0].iommu->bdf != bdf ) > + if ( ivrs_mappings[bd0].iommu && ivrs_mappings[bd0].iommu->sbdf.bdf != bdf ) > { > struct ivrs_mappings tmp = ivrs_mappings[bd0]; > > @@ -121,7 +121,7 @@ static bool use_ats( > { > return !ivrs_dev->block_ats && > iommu_has_cap(iommu, PCI_CAP_IOTLB_SHIFT) && > - pci_ats_device(iommu->seg, pdev->bus, pdev->devfn); > + pci_ats_device(iommu->sbdf.seg, pdev->bus, pdev->devfn); Same idea about updating signature to take pci_sbdf_t. > } > > static int __must_check amd_iommu_setup_domain_device( > @@ -147,17 +147,17 @@ static int __must_check amd_iommu_setup_domain_device( > if ( rc ) > return rc; > > - req_id = get_dma_requestor_id(iommu->seg, pdev->sbdf.bdf); > - ivrs_dev = &get_ivrs_mappings(iommu->seg)[req_id]; > + req_id = get_dma_requestor_id(iommu->sbdf.seg, pdev->sbdf.bdf); > + ivrs_dev = &get_ivrs_mappings(iommu->sbdf.seg)[req_id]; > sr_flags = (iommu_hwdom_passthrough && is_hardware_domain(domain) > ? 0 : SET_ROOT_VALID) > | (ivrs_dev->unity_map ? SET_ROOT_WITH_UNITY_MAP : 0); > > /* get device-table entry */ > - req_id = get_dma_requestor_id(iommu->seg, PCI_BDF(bus, devfn)); > + req_id = get_dma_requestor_id(iommu->sbdf.seg, PCI_BDF(bus, devfn)); > table = iommu->dev_table.buffer; > dte = &table[req_id]; > - ivrs_dev = &get_ivrs_mappings(iommu->seg)[req_id]; > + ivrs_dev = &get_ivrs_mappings(iommu->sbdf.seg)[req_id]; > > if ( domain != dom_io ) > { > @@ -275,7 +275,7 @@ static int __must_check amd_iommu_setup_domain_device( > ASSERT(pcidevs_locked()); > > if ( use_ats(pdev, iommu, ivrs_dev) && > - !pci_ats_enabled(iommu->seg, bus, pdev->devfn) ) > + !pci_ats_enabled(iommu->sbdf.seg, bus, pdev->devfn) ) ... and same for pci_ats_enabled() > { > if ( devfn == pdev->devfn ) > enable_ats_device(pdev, &iommu->ats_devices); > @@ -418,12 +418,12 @@ static void amd_iommu_disable_domain_device(const struct domain *domain, > > ASSERT(pcidevs_locked()); > > - if ( pci_ats_device(iommu->seg, bus, pdev->devfn) && > - pci_ats_enabled(iommu->seg, bus, pdev->devfn) ) > + if ( pci_ats_device(iommu->sbdf.seg, bus, pdev->devfn) && > + pci_ats_enabled(iommu->sbdf.seg, bus, pdev->devfn) ) > disable_ats_device(pdev); > > BUG_ON ( iommu->dev_table.buffer == NULL ); > - req_id = get_dma_requestor_id(iommu->seg, PCI_BDF(bus, devfn)); > + req_id = get_dma_requestor_id(iommu->sbdf.seg, PCI_BDF(bus, devfn)); > table = iommu->dev_table.buffer; > dte = &table[req_id]; > > @@ -578,7 +578,7 @@ 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); > -- > 2.49.0 > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 1/3] drivers: Change amd_iommu struct to contain pci_sbdf_t, simplify code 2025-04-15 7:34 ` dmkhn @ 2025-04-15 7:38 ` Jan Beulich 2025-04-15 7:56 ` Andriy Sultanov 1 sibling, 0 replies; 13+ messages in thread From: Jan Beulich @ 2025-04-15 7:38 UTC (permalink / raw) To: dmkhn; +Cc: xen-devel, Andrew Cooper, Roger Pau Monné, Andrii Sultanov On 15.04.2025 09:34, dmkhn@proton.me wrote: > On Mon, Apr 14, 2025 at 08:19:16PM +0100, Andrii Sultanov wrote: >> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c >> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c >> @@ -43,7 +43,7 @@ struct amd_iommu *find_iommu_for_device(int seg, int bdf) >> { >> unsigned int bd0 = bdf & ~PCI_FUNC(~0); >> >> - if ( ivrs_mappings[bd0].iommu && ivrs_mappings[bd0].iommu->bdf != bdf ) >> + if ( ivrs_mappings[bd0].iommu && ivrs_mappings[bd0].iommu->sbdf.bdf != bdf ) >> { >> struct ivrs_mappings tmp = ivrs_mappings[bd0]; >> >> @@ -121,7 +121,7 @@ static bool use_ats( >> { >> return !ivrs_dev->block_ats && >> iommu_has_cap(iommu, PCI_CAP_IOTLB_SHIFT) && >> - pci_ats_device(iommu->seg, pdev->bus, pdev->devfn); >> + pci_ats_device(iommu->sbdf.seg, pdev->bus, pdev->devfn); > > Same idea about updating signature to take pci_sbdf_t. Perhaps both this and ... >> @@ -147,17 +147,17 @@ static int __must_check amd_iommu_setup_domain_device( >> if ( rc ) >> return rc; >> >> - req_id = get_dma_requestor_id(iommu->seg, pdev->sbdf.bdf); >> - ivrs_dev = &get_ivrs_mappings(iommu->seg)[req_id]; >> + req_id = get_dma_requestor_id(iommu->sbdf.seg, pdev->sbdf.bdf); >> + ivrs_dev = &get_ivrs_mappings(iommu->sbdf.seg)[req_id]; >> sr_flags = (iommu_hwdom_passthrough && is_hardware_domain(domain) >> ? 0 : SET_ROOT_VALID) >> | (ivrs_dev->unity_map ? SET_ROOT_WITH_UNITY_MAP : 0); >> >> /* get device-table entry */ >> - req_id = get_dma_requestor_id(iommu->seg, PCI_BDF(bus, devfn)); >> + req_id = get_dma_requestor_id(iommu->sbdf.seg, PCI_BDF(bus, devfn)); >> table = iommu->dev_table.buffer; >> dte = &table[req_id]; >> - ivrs_dev = &get_ivrs_mappings(iommu->seg)[req_id]; >> + ivrs_dev = &get_ivrs_mappings(iommu->sbdf.seg)[req_id]; >> >> if ( domain != dom_io ) >> { >> @@ -275,7 +275,7 @@ static int __must_check amd_iommu_setup_domain_device( >> ASSERT(pcidevs_locked()); >> >> if ( use_ats(pdev, iommu, ivrs_dev) && >> - !pci_ats_enabled(iommu->seg, bus, pdev->devfn) ) >> + !pci_ats_enabled(iommu->sbdf.seg, bus, pdev->devfn) ) > > ... and same for pci_ats_enabled() ... this would best take a pointer to a const pdev (if that works out). And yes, there's a lot of similar cleanup to be done in this area. I don't think we can demand Andrii to do it all in one go. Jan ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 1/3] drivers: Change amd_iommu struct to contain pci_sbdf_t, simplify code 2025-04-15 7:34 ` dmkhn 2025-04-15 7:38 ` Jan Beulich @ 2025-04-15 7:56 ` Andriy Sultanov 1 sibling, 0 replies; 13+ messages in thread From: Andriy Sultanov @ 2025-04-15 7:56 UTC (permalink / raw) To: dmkhn; +Cc: xen-devel, Jan Beulich, Andrew Cooper, Roger Pau Monné On 4/15/25 8:34 AM, dmkhn@proton.me wrote: > On Mon, Apr 14, 2025 at 08:19:16PM +0100, Andrii Sultanov wrote: >> From: Andrii Sultanov <sultanovandriy@gmail.com> >> >> Following on from 250d87dc3ff9 ("x86/msi: Change __msi_set_enable() to >> take pci_sbdf_t"), make struct amd_iommu contain pci_sbdf_t directly >> instead of specifying seg+bdf separately and regenerating sbdf_t from them, >> which simplifies code. >> >> Bloat-o-meter reports: >> add/remove: 0/0 grow/shrink: 4/13 up/down: 121/-377 (-256) >> Function old new delta >> _einittext 22028 22092 +64 >> amd_iommu_prepare 853 897 +44 >> __mon_lengths 2928 2936 +8 >> _invalidate_all_devices 133 138 +5 >> _hvm_dpci_msi_eoi 157 155 -2 >> 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_msi_msg_update_ire 472 448 -24 >> send_iommu_command 251 224 -27 >> 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 V4: >> * Dropped references to the order of seg/bdf in the commit message >> * Dropped unnecessary detail from the commit message >> * Reverted to a macro usage in one case where it was mistakenly dropped >> * Folded several separate seg+bdf comparisons into a single one between >> sbdf_t, folded separate assignments with a macro. >> * More code size improvements with the changes, so I've refreshed the >> bloat-o-meter report >> >> Changes in V3: >> * Dropped the union with seg+bdf/pci_sbdf_t to avoid aliasing, renamed >> all users appropriately >> >> 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 | 4 +-- >> xen/drivers/passthrough/amd/iommu_acpi.c | 16 +++++----- >> xen/drivers/passthrough/amd/iommu_cmd.c | 8 ++--- >> xen/drivers/passthrough/amd/iommu_detect.c | 18 +++++------ >> xen/drivers/passthrough/amd/iommu_init.c | 35 ++++++++++----------- >> xen/drivers/passthrough/amd/iommu_intr.c | 29 ++++++++--------- >> xen/drivers/passthrough/amd/iommu_map.c | 4 +-- >> xen/drivers/passthrough/amd/pci_amd_iommu.c | 22 ++++++------- >> 8 files changed, 67 insertions(+), 69 deletions(-) >> >> diff --git a/xen/drivers/passthrough/amd/iommu.h b/xen/drivers/passthrough/amd/iommu.h >> index 00e81b4b2a..ba541f7943 100644 >> --- a/xen/drivers/passthrough/amd/iommu.h >> +++ b/xen/drivers/passthrough/amd/iommu.h >> @@ -77,8 +77,8 @@ struct amd_iommu { >> struct list_head list; >> spinlock_t lock; /* protect iommu */ >> >> - u16 seg; >> - u16 bdf; >> + pci_sbdf_t sbdf; >> + >> struct msi_desc msi; >> >> u16 cap_offset; >> diff --git a/xen/drivers/passthrough/amd/iommu_acpi.c b/xen/drivers/passthrough/amd/iommu_acpi.c >> index 5bdbfb5ba8..025d9be40f 100644 >> --- a/xen/drivers/passthrough/amd/iommu_acpi.c >> +++ b/xen/drivers/passthrough/amd/iommu_acpi.c >> @@ -58,7 +58,7 @@ static void __init add_ivrs_mapping_entry( >> uint16_t bdf, uint16_t alias_id, uint8_t flags, unsigned int ext_flags, >> bool alloc_irt, struct amd_iommu *iommu) >> { >> - struct ivrs_mappings *ivrs_mappings = get_ivrs_mappings(iommu->seg); >> + struct ivrs_mappings *ivrs_mappings = get_ivrs_mappings(iommu->sbdf.seg); >> >> ASSERT( ivrs_mappings != NULL ); >> >> @@ -70,7 +70,7 @@ static void __init add_ivrs_mapping_entry( >> ivrs_mappings[bdf].device_flags = flags; >> >> /* Don't map an IOMMU by itself. */ >> - if ( iommu->bdf == bdf ) >> + if ( iommu->sbdf.bdf == bdf ) >> return; >> >> /* Allocate interrupt remapping table if needed. */ >> @@ -96,7 +96,7 @@ static void __init add_ivrs_mapping_entry( >> >> if ( !ivrs_mappings[alias_id].intremap_table ) >> panic("No memory for %pp's IRT\n", >> - &PCI_SBDF(iommu->seg, alias_id)); >> + &PCI_SBDF(iommu->sbdf.seg, alias_id)); >> } >> } >> >> @@ -112,7 +112,7 @@ static struct amd_iommu * __init find_iommu_from_bdf_cap( >> struct amd_iommu *iommu; >> >> for_each_amd_iommu ( iommu ) >> - if ( (iommu->seg == seg) && (iommu->bdf == bdf) && >> + if ( (iommu->sbdf.seg == seg) && (iommu->sbdf.bdf == bdf) && > Perhaps something like > > if ( (iommu->sbdf.sbdf == PCI_SBDF(seg, bdf).sbdf && > > ? > >> (iommu->cap_offset == cap_offset) ) >> return iommu; >> >> @@ -297,13 +297,13 @@ 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(iommu->sbdf.seg, bdf) ) >> continue; >> >> - req = get_ivrs_mappings(iommu->seg)[bdf].dte_requestor_id; >> - rc = reserve_unity_map_for_device(iommu->seg, bdf, base, length, >> + req = get_ivrs_mappings(iommu->sbdf.seg)[bdf].dte_requestor_id; >> + rc = reserve_unity_map_for_device(iommu->sbdf.seg, bdf, base, length, >> iw, ir, false) ?: >> - reserve_unity_map_for_device(iommu->seg, req, base, length, >> + reserve_unity_map_for_device(iommu->sbdf.seg, req, base, length, >> iw, ir, false); >> } >> >> diff --git a/xen/drivers/passthrough/amd/iommu_cmd.c b/xen/drivers/passthrough/amd/iommu_cmd.c >> index 83c525b84f..eefd626161 100644 >> --- a/xen/drivers/passthrough/amd/iommu_cmd.c >> +++ b/xen/drivers/passthrough/amd/iommu_cmd.c >> @@ -40,7 +40,7 @@ static void send_iommu_command(struct amd_iommu *iommu, >> IOMMU_RING_BUFFER_PTR_MASK) ) >> { >> printk_once(XENLOG_ERR "AMD IOMMU %pp: no cmd slot available\n", >> - &PCI_SBDF(iommu->seg, iommu->bdf)); >> + &iommu->sbdf); >> cpu_relax(); >> } >> >> @@ -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); >> } >> @@ -300,7 +300,7 @@ void amd_iommu_flush_iotlb(u8 devfn, const struct pci_dev *pdev, >> if ( !iommu_has_cap(iommu, PCI_CAP_IOTLB_SHIFT) ) >> return; >> >> - req_id = get_dma_requestor_id(iommu->seg, PCI_BDF(pdev->bus, devfn)); >> + req_id = get_dma_requestor_id(iommu->sbdf.seg, PCI_BDF(pdev->bus, devfn)); >> queueid = req_id; >> maxpend = pdev->ats.queue_depth & 0xff; >> >> diff --git a/xen/drivers/passthrough/amd/iommu_detect.c b/xen/drivers/passthrough/amd/iommu_detect.c >> index cede44e651..72cc554b08 100644 >> --- a/xen/drivers/passthrough/amd/iommu_detect.c >> +++ b/xen/drivers/passthrough/amd/iommu_detect.c >> @@ -162,8 +162,8 @@ int __init amd_iommu_detect_one_acpi( >> spin_lock_init(&iommu->lock); >> INIT_LIST_HEAD(&iommu->ats_devices); >> >> - iommu->seg = ivhd_block->pci_segment_group; >> - iommu->bdf = ivhd_block->header.device_id; >> + iommu->sbdf = PCI_SBDF(ivhd_block->pci_segment_group, >> + ivhd_block->header.device_id); >> iommu->cap_offset = ivhd_block->capability_offset; >> iommu->mmio_base_phys = ivhd_block->base_address; >> >> @@ -210,16 +210,16 @@ int __init amd_iommu_detect_one_acpi( >> /* override IOMMU HT flags */ >> iommu->ht_flags = ivhd_block->header.flags; >> >> - bus = PCI_BUS(iommu->bdf); >> - dev = PCI_SLOT(iommu->bdf); >> - func = PCI_FUNC(iommu->bdf); >> + bus = PCI_BUS(iommu->sbdf.bdf); >> + dev = PCI_SLOT(iommu->sbdf.bdf); >> + func = PCI_FUNC(iommu->sbdf.bdf); >> >> - rt = get_iommu_capabilities(iommu->seg, bus, dev, func, >> + rt = get_iommu_capabilities(iommu->sbdf.seg, bus, dev, func, > I would update signature of get_iommu_capabilities() so it takes pci_sbdf_t > as an agument instead of disaggregated PCI address. I think it will simplify > the code futher. > >> iommu->cap_offset, iommu); >> if ( rt ) >> goto out; >> >> - rt = get_iommu_msi_capabilities(iommu->seg, bus, dev, func, iommu); >> + rt = get_iommu_msi_capabilities(iommu->sbdf.seg, bus, dev, func, iommu); > ... and same idea for get_iommu_msi_capabilities() > > What do you think? > >> if ( rt ) >> goto out; >> >> @@ -228,10 +228,10 @@ int __init amd_iommu_detect_one_acpi( >> if ( !iommu->domid_map ) >> goto out; >> >> - rt = pci_ro_device(iommu->seg, bus, PCI_DEVFN(dev, func)); >> + rt = pci_ro_device(iommu->sbdf.seg, bus, PCI_DEVFN(dev, func)); > There's not so many users of pci_ro_device(). I think it makes sense to update > pci_ro_device() to something like: > > int pci_ro_device(pci_sbdf_t pciaddr); > > But probably in a separate code change. > >> 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..58d657060a 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) >> @@ -566,7 +564,7 @@ static void cf_check parse_event_log_entry(struct amd_iommu *iommu, u32 entry[]) >> >> printk(XENLOG_ERR "AMD-Vi: %s: %pp d%u addr %016"PRIx64 >> " flags %#x%s%s%s%s%s%s%s%s%s%s\n", >> - code_str, &PCI_SBDF(iommu->seg, device_id), >> + code_str, &PCI_SBDF(iommu->sbdf.seg, device_id), >> domain_id, addr, flags, >> (flags & 0xe00) ? " ??" : "", >> (flags & 0x100) ? " TR" : "", >> @@ -583,8 +581,8 @@ static void cf_check parse_event_log_entry(struct amd_iommu *iommu, u32 entry[]) >> amd_iommu_print_entries(iommu, device_id, daddr_to_dfn(addr)); >> >> for ( bdf = 0; bdf < ivrs_bdf_entries; bdf++ ) >> - if ( get_dma_requestor_id(iommu->seg, bdf) == device_id ) >> - pci_check_disable_device(iommu->seg, PCI_BUS(bdf), >> + if ( get_dma_requestor_id(iommu->sbdf.seg, bdf) == device_id ) >> + pci_check_disable_device(iommu->sbdf.seg, PCI_BUS(bdf), >> PCI_DEVFN(bdf)); >> } >> else >> @@ -643,7 +641,7 @@ static void cf_check parse_ppr_log_entry(struct amd_iommu *iommu, u32 entry[]) >> struct pci_dev *pdev; >> >> pcidevs_lock(); >> - pdev = pci_get_real_pdev(PCI_SBDF(iommu->seg, device_id)); >> + pdev = pci_get_real_pdev(PCI_SBDF(iommu->sbdf.seg, device_id)); >> pcidevs_unlock(); >> >> if ( pdev ) >> @@ -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) >> @@ -1095,7 +1092,7 @@ static int __init amd_iommu_init_one(struct amd_iommu *iommu, bool intr) >> goto error_out; >> >> /* Make sure that the device table has been successfully allocated. */ >> - ivrs_mappings = get_ivrs_mappings(iommu->seg); >> + ivrs_mappings = get_ivrs_mappings(iommu->sbdf.seg); >> if ( !IVRS_MAPPINGS_DEVTAB(ivrs_mappings) ) >> goto error_out; >> >> @@ -1363,7 +1360,7 @@ static bool __init amd_sp5100_erratum28(void) >> >> static int __init amd_iommu_prepare_one(struct amd_iommu *iommu) >> { >> - int rc = alloc_ivrs_mappings(iommu->seg); >> + int rc = alloc_ivrs_mappings(iommu->sbdf.seg); >> >> if ( !rc ) >> rc = map_iommu_mmio_region(iommu); >> diff --git a/xen/drivers/passthrough/amd/iommu_intr.c b/xen/drivers/passthrough/amd/iommu_intr.c >> index 9abdc38053..a7347fcbad 100644 >> --- a/xen/drivers/passthrough/amd/iommu_intr.c >> +++ b/xen/drivers/passthrough/amd/iommu_intr.c >> @@ -132,7 +132,7 @@ static int get_intremap_requestor_id(int seg, int bdf) >> static unsigned int alloc_intremap_entry(const struct amd_iommu *iommu, >> unsigned int bdf, unsigned int nr) >> { >> - const struct ivrs_mappings *ivrs_mappings = get_ivrs_mappings(iommu->seg); >> + const struct ivrs_mappings *ivrs_mappings = get_ivrs_mappings(iommu->sbdf.seg); >> unsigned long *inuse = ivrs_mappings[bdf].intremap_inuse; >> unsigned int nr_ents = >> intremap_table_entries(ivrs_mappings[bdf].intremap_table, iommu); >> @@ -167,7 +167,7 @@ static union irte_ptr get_intremap_entry(const struct amd_iommu *iommu, >> unsigned int bdf, unsigned int index) >> { >> union irte_ptr table = { >> - .ptr = get_ivrs_mappings(iommu->seg)[bdf].intremap_table >> + .ptr = get_ivrs_mappings(iommu->sbdf.seg)[bdf].intremap_table >> }; >> >> ASSERT(table.ptr && (index < intremap_table_entries(table.ptr, iommu))); >> @@ -184,7 +184,7 @@ static void free_intremap_entry(const struct amd_iommu *iommu, >> unsigned int bdf, unsigned int index) >> { >> union irte_ptr entry = get_intremap_entry(iommu, bdf, index); >> - struct ivrs_mappings *ivrs = get_ivrs_mappings(iommu->seg); >> + struct ivrs_mappings *ivrs = get_ivrs_mappings(iommu->sbdf.seg); >> >> if ( iommu->ctrl.ga_en ) >> { >> @@ -281,8 +281,8 @@ static int update_intremap_entry_from_ioapic( >> unsigned int dest, offset; >> bool fresh = false; >> >> - req_id = get_intremap_requestor_id(iommu->seg, bdf); >> - lock = get_intremap_lock(iommu->seg, req_id); >> + req_id = get_intremap_requestor_id(iommu->sbdf.seg, bdf); >> + lock = get_intremap_lock(iommu->sbdf.seg, req_id); >> >> delivery_mode = rte->delivery_mode; >> vector = rte->vector; >> @@ -419,10 +419,10 @@ static int update_intremap_entry_from_msi_msg( >> unsigned int dest, offset, i; >> bool fresh = false; >> >> - req_id = get_dma_requestor_id(iommu->seg, bdf); >> - alias_id = get_intremap_requestor_id(iommu->seg, bdf); >> + req_id = get_dma_requestor_id(iommu->sbdf.seg, bdf); >> + alias_id = get_intremap_requestor_id(iommu->sbdf.seg, bdf); >> >> - lock = get_intremap_lock(iommu->seg, req_id); >> + lock = get_intremap_lock(iommu->sbdf.seg, req_id); >> spin_lock_irqsave(lock, flags); >> >> if ( msg == NULL ) >> @@ -486,10 +486,10 @@ static int update_intremap_entry_from_msi_msg( >> */ >> >> if ( ( req_id != alias_id ) && >> - get_ivrs_mappings(iommu->seg)[alias_id].intremap_table != NULL ) >> + get_ivrs_mappings(iommu->sbdf.seg)[alias_id].intremap_table != NULL ) >> { >> - BUG_ON(get_ivrs_mappings(iommu->seg)[req_id].intremap_table != >> - get_ivrs_mappings(iommu->seg)[alias_id].intremap_table); >> + BUG_ON(get_ivrs_mappings(iommu->sbdf.seg)[req_id].intremap_table != >> + get_ivrs_mappings(iommu->sbdf.seg)[alias_id].intremap_table); >> } >> >> return fresh; >> @@ -498,16 +498,17 @@ static int update_intremap_entry_from_msi_msg( >> static struct amd_iommu *_find_iommu_for_device(int seg, int bdf) >> { >> struct amd_iommu *iommu; >> + pci_sbdf_t sbdf = PCI_SBDF(seg, bdf); >> >> 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); >> 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); >> } >> >> @@ -730,7 +731,7 @@ static void dump_intremap_table(const struct amd_iommu *iommu, >> if ( ivrs_mapping ) >> { >> printk(" %pp:\n", >> - &PCI_SBDF(iommu->seg, ivrs_mapping->dte_requestor_id)); >> + &PCI_SBDF(iommu->sbdf.seg, ivrs_mapping->dte_requestor_id)); >> ivrs_mapping = NULL; >> } >> >> diff --git a/xen/drivers/passthrough/amd/iommu_map.c b/xen/drivers/passthrough/amd/iommu_map.c >> index dde393645a..d28c475650 100644 >> --- a/xen/drivers/passthrough/amd/iommu_map.c >> +++ b/xen/drivers/passthrough/amd/iommu_map.c >> @@ -558,14 +558,14 @@ void amd_iommu_print_entries(const struct amd_iommu *iommu, unsigned int dev_id, >> >> if ( !dt[dev_id].tv ) >> { >> - printk("%pp: no root\n", &PCI_SBDF(iommu->seg, dev_id)); >> + printk("%pp: no root\n", &PCI_SBDF(iommu->sbdf.seg, dev_id)); >> return; >> } >> >> pt_mfn = _mfn(dt[dev_id].pt_root); >> level = dt[dev_id].paging_mode; >> printk("%pp root @ %"PRI_mfn" (%u levels) dfn=%"PRI_dfn"\n", >> - &PCI_SBDF(iommu->seg, dev_id), mfn_x(pt_mfn), level, dfn_x(dfn)); >> + &PCI_SBDF(iommu->sbdf.seg, dev_id), mfn_x(pt_mfn), level, dfn_x(dfn)); >> >> while ( level ) >> { >> diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c >> index d00697edb3..6b58e3380b 100644 >> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c >> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c >> @@ -43,7 +43,7 @@ struct amd_iommu *find_iommu_for_device(int seg, int bdf) >> { >> unsigned int bd0 = bdf & ~PCI_FUNC(~0); >> >> - if ( ivrs_mappings[bd0].iommu && ivrs_mappings[bd0].iommu->bdf != bdf ) >> + if ( ivrs_mappings[bd0].iommu && ivrs_mappings[bd0].iommu->sbdf.bdf != bdf ) >> { >> struct ivrs_mappings tmp = ivrs_mappings[bd0]; >> >> @@ -121,7 +121,7 @@ static bool use_ats( >> { >> return !ivrs_dev->block_ats && >> iommu_has_cap(iommu, PCI_CAP_IOTLB_SHIFT) && >> - pci_ats_device(iommu->seg, pdev->bus, pdev->devfn); >> + pci_ats_device(iommu->sbdf.seg, pdev->bus, pdev->devfn); > Same idea about updating signature to take pci_sbdf_t. > >> } >> >> static int __must_check amd_iommu_setup_domain_device( >> @@ -147,17 +147,17 @@ static int __must_check amd_iommu_setup_domain_device( >> if ( rc ) >> return rc; >> >> - req_id = get_dma_requestor_id(iommu->seg, pdev->sbdf.bdf); >> - ivrs_dev = &get_ivrs_mappings(iommu->seg)[req_id]; >> + req_id = get_dma_requestor_id(iommu->sbdf.seg, pdev->sbdf.bdf); >> + ivrs_dev = &get_ivrs_mappings(iommu->sbdf.seg)[req_id]; >> sr_flags = (iommu_hwdom_passthrough && is_hardware_domain(domain) >> ? 0 : SET_ROOT_VALID) >> | (ivrs_dev->unity_map ? SET_ROOT_WITH_UNITY_MAP : 0); >> >> /* get device-table entry */ >> - req_id = get_dma_requestor_id(iommu->seg, PCI_BDF(bus, devfn)); >> + req_id = get_dma_requestor_id(iommu->sbdf.seg, PCI_BDF(bus, devfn)); >> table = iommu->dev_table.buffer; >> dte = &table[req_id]; >> - ivrs_dev = &get_ivrs_mappings(iommu->seg)[req_id]; >> + ivrs_dev = &get_ivrs_mappings(iommu->sbdf.seg)[req_id]; >> >> if ( domain != dom_io ) >> { >> @@ -275,7 +275,7 @@ static int __must_check amd_iommu_setup_domain_device( >> ASSERT(pcidevs_locked()); >> >> if ( use_ats(pdev, iommu, ivrs_dev) && >> - !pci_ats_enabled(iommu->seg, bus, pdev->devfn) ) >> + !pci_ats_enabled(iommu->sbdf.seg, bus, pdev->devfn) ) > ... and same for pci_ats_enabled() > >> { >> if ( devfn == pdev->devfn ) >> enable_ats_device(pdev, &iommu->ats_devices); >> @@ -418,12 +418,12 @@ static void amd_iommu_disable_domain_device(const struct domain *domain, >> >> ASSERT(pcidevs_locked()); >> >> - if ( pci_ats_device(iommu->seg, bus, pdev->devfn) && >> - pci_ats_enabled(iommu->seg, bus, pdev->devfn) ) >> + if ( pci_ats_device(iommu->sbdf.seg, bus, pdev->devfn) && >> + pci_ats_enabled(iommu->sbdf.seg, bus, pdev->devfn) ) >> disable_ats_device(pdev); >> >> BUG_ON ( iommu->dev_table.buffer == NULL ); >> - req_id = get_dma_requestor_id(iommu->seg, PCI_BDF(bus, devfn)); >> + req_id = get_dma_requestor_id(iommu->sbdf.seg, PCI_BDF(bus, devfn)); >> table = iommu->dev_table.buffer; >> dte = &table[req_id]; >> >> @@ -578,7 +578,7 @@ 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); >> -- >> 2.49.0 >> >> I'll leave pci_ats_device and similar functions from directories above passthrough/amd for the next cleanup in a separate patch. Will change local functions like get_iommu.*_capabilities and resend. Thank you! ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 1/3] drivers: Change amd_iommu struct to contain pci_sbdf_t, simplify code 2025-04-14 19:19 ` [PATCH v4 1/3] drivers: Change amd_iommu struct to contain pci_sbdf_t, simplify code Andrii Sultanov 2025-04-15 7:34 ` dmkhn @ 2025-04-15 10:36 ` Jan Beulich 1 sibling, 0 replies; 13+ messages in thread From: Jan Beulich @ 2025-04-15 10:36 UTC (permalink / raw) To: Andrii Sultanov; +Cc: Andrew Cooper, Roger Pau Monné, xen-devel On 14.04.2025 21:19, Andrii Sultanov wrote: > @@ -578,7 +578,7 @@ 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 ) Not for this patch (optionally for a prereq one) we may want to gain sbdf_eq(), much like we have mfn_eq() and friends. Jan ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v4 2/3] drivers: Change find_iommu_for_device function to take pci_sbdf_t, simplify code 2025-04-14 19:19 [PATCH v4 0/3] drivers: Simplify handling of pci_sbdf_t in passthrough/amd Andrii Sultanov 2025-04-14 19:19 ` [PATCH v4 1/3] drivers: Change amd_iommu struct to contain pci_sbdf_t, simplify code Andrii Sultanov @ 2025-04-14 19:19 ` Andrii Sultanov 2025-04-15 7:50 ` dmkhn 2025-04-14 19:19 ` [PATCH v4 3/3] drivers: Make ioapic_sbdf and hpet_sbdf contain pci_sbdf_t Andrii Sultanov 2 siblings, 1 reply; 13+ messages in thread From: Andrii Sultanov @ 2025-04-14 19:19 UTC (permalink / raw) To: xen-devel Cc: Andrii Sultanov, Jan Beulich, Andrew Cooper, Roger Pau Monné From: Andrii Sultanov <sultanovandriy@gmail.com> 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 removes conversions in the majority of cases. Bloat-o-meter reports (on top of the first patch in the series): add/remove: 0/0 grow/shrink: 12/11 up/down: 95/-95 (0) 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 46 51 +5 _hvm_dpci_msi_eoi 155 160 +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 521 516 -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 parse_ivmd_block 1339 1312 -27 Signed-off-by: Andrii Sultanov <sultanovandriy@gmail.com> Acked-by: Jan Beulich <jbeulich@suse.com> --- Changes in V4: * After amendments to the previous commit which increased improvements there, this commit now does not improve code size anymore (but still simplifies code), so I've updated the bloat-o-meter report. Changes in V3: * Amended commit message * As the previous patch dropped the aliasing of seg and bdf, renamed users of amd_iommu as appropriate. 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 | 17 ++++++------ xen/drivers/passthrough/amd/iommu_map.c | 2 +- xen/drivers/passthrough/amd/pci_amd_iommu.c | 30 ++++++++++----------- 7 files changed, 35 insertions(+), 36 deletions(-) diff --git a/xen/drivers/passthrough/amd/iommu.h b/xen/drivers/passthrough/amd/iommu.h index ba541f7943..2599800e6a 100644 --- a/xen/drivers/passthrough/amd/iommu.h +++ b/xen/drivers/passthrough/amd/iommu.h @@ -240,7 +240,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 025d9be40f..9e4fbee953 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->sbdf.seg, bdf) ) + if ( iommu != find_iommu_for_device(PCI_SBDF(iommu->sbdf.seg, bdf)) ) continue; req = get_ivrs_mappings(iommu->sbdf.seg)[bdf].dte_requestor_id; diff --git a/xen/drivers/passthrough/amd/iommu_cmd.c b/xen/drivers/passthrough/amd/iommu_cmd.c index eefd626161..6b80c57f44 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 58d657060a..3f6d2f5db5 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 a7347fcbad..16075cd5a1 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,16 +495,15 @@ 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; - pci_sbdf_t sbdf = PCI_SBDF(seg, bdf); for_each_amd_iommu ( iommu ) 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; @@ -524,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); @@ -661,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)); @@ -691,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 d28c475650..320a2dc64c 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 6b58e3380b..3a14770855 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->sbdf.bdf != bdf ) + if ( ivrs_mappings[bd0].iommu && ivrs_mappings[bd0].iommu->sbdf.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", @@ -581,7 +581,7 @@ static int cf_check amd_iommu_add_device(u8 devfn, struct pci_dev *pdev) 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.49.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v4 2/3] drivers: Change find_iommu_for_device function to take pci_sbdf_t, simplify code 2025-04-14 19:19 ` [PATCH v4 2/3] drivers: Change find_iommu_for_device function to take " Andrii Sultanov @ 2025-04-15 7:50 ` dmkhn 0 siblings, 0 replies; 13+ messages in thread From: dmkhn @ 2025-04-15 7:50 UTC (permalink / raw) To: Andrii Sultanov Cc: xen-devel, Jan Beulich, Andrew Cooper, Roger Pau Monné On Mon, Apr 14, 2025 at 08:19:17PM +0100, Andrii Sultanov wrote: > From: Andrii Sultanov <sultanovandriy@gmail.com> > > 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 removes conversions in the majority of cases. > > Bloat-o-meter reports (on top of the first patch in the series): > add/remove: 0/0 grow/shrink: 12/11 up/down: 95/-95 (0) > 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 46 51 +5 > _hvm_dpci_msi_eoi 155 160 +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 521 516 -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 > parse_ivmd_block 1339 1312 -27 > > Signed-off-by: Andrii Sultanov <sultanovandriy@gmail.com> > > Acked-by: Jan Beulich <jbeulich@suse.com> Looks good to me: Reviewed-by: Denis Mukhin <dmukhin@ford.com> > > --- > Changes in V4: > * After amendments to the previous commit which increased improvements > there, this commit now does not improve code size anymore (but still > simplifies code), so I've updated the bloat-o-meter report. > > Changes in V3: > * Amended commit message > * As the previous patch dropped the aliasing of seg and bdf, renamed users of > amd_iommu as appropriate. > > 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 | 17 ++++++------ > xen/drivers/passthrough/amd/iommu_map.c | 2 +- > xen/drivers/passthrough/amd/pci_amd_iommu.c | 30 ++++++++++----------- > 7 files changed, 35 insertions(+), 36 deletions(-) > > diff --git a/xen/drivers/passthrough/amd/iommu.h b/xen/drivers/passthrough/amd/iommu.h > index ba541f7943..2599800e6a 100644 > --- a/xen/drivers/passthrough/amd/iommu.h > +++ b/xen/drivers/passthrough/amd/iommu.h > @@ -240,7 +240,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 025d9be40f..9e4fbee953 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->sbdf.seg, bdf) ) > + if ( iommu != find_iommu_for_device(PCI_SBDF(iommu->sbdf.seg, bdf)) ) > continue; > > req = get_ivrs_mappings(iommu->sbdf.seg)[bdf].dte_requestor_id; > diff --git a/xen/drivers/passthrough/amd/iommu_cmd.c b/xen/drivers/passthrough/amd/iommu_cmd.c > index eefd626161..6b80c57f44 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 58d657060a..3f6d2f5db5 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 a7347fcbad..16075cd5a1 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,16 +495,15 @@ 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; > - pci_sbdf_t sbdf = PCI_SBDF(seg, bdf); > > for_each_amd_iommu ( iommu ) > 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; > > @@ -524,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); > > @@ -661,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)); > @@ -691,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 d28c475650..320a2dc64c 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 6b58e3380b..3a14770855 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->sbdf.bdf != bdf ) > + if ( ivrs_mappings[bd0].iommu && ivrs_mappings[bd0].iommu->sbdf.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", > @@ -581,7 +581,7 @@ static int cf_check amd_iommu_add_device(u8 devfn, struct pci_dev *pdev) > 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.49.0 > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v4 3/3] drivers: Make ioapic_sbdf and hpet_sbdf contain pci_sbdf_t 2025-04-14 19:19 [PATCH v4 0/3] drivers: Simplify handling of pci_sbdf_t in passthrough/amd Andrii Sultanov 2025-04-14 19:19 ` [PATCH v4 1/3] drivers: Change amd_iommu struct to contain pci_sbdf_t, simplify code Andrii Sultanov 2025-04-14 19:19 ` [PATCH v4 2/3] drivers: Change find_iommu_for_device function to take " Andrii Sultanov @ 2025-04-14 19:19 ` Andrii Sultanov 2025-04-15 7:05 ` dmkhn 2025-04-15 11:12 ` Jan Beulich 2 siblings, 2 replies; 13+ messages in thread From: Andrii Sultanov @ 2025-04-14 19:19 UTC (permalink / raw) To: xen-devel Cc: Andrii Sultanov, Jan Beulich, Andrew Cooper, Roger Pau Monné From: Andrii Sultanov <sultanovandriy@gmail.com> 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 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 | 30 +++++++--------- xen/drivers/passthrough/amd/iommu_intr.c | 44 +++++++++++------------- 3 files changed, 37 insertions(+), 42 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 9e4fbee953..14845766e6 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, PCI_BDF(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, PCI_BDF(bus, dev, func) ); hpet_sbdf.init = HPET_CMDL; return 0; @@ -748,6 +746,7 @@ static u16 __init parse_ivhd_device_special( { u16 dev_length, bdf; unsigned int apic, idx; + pci_sbdf_t sbdf; dev_length = sizeof(*special); if ( header_length < (block_length + dev_length) ) @@ -757,6 +756,7 @@ static u16 __init parse_ivhd_device_special( } bdf = special->used_id; + sbdf = PCI_SBDF(seg, bdf); if ( bdf >= ivrs_bdf_entries ) { AMD_IOMMU_ERROR("IVHD: invalid Device_Entry Dev_Id %#x\n", bdf); @@ -764,7 +764,7 @@ static u16 __init parse_ivhd_device_special( } AMD_IOMMU_DEBUG("IVHD Special: %pp variety %#x handle %#x\n", - &PCI_SBDF(seg, bdf), special->variety, special->handle); + &sbdf, special->variety, special->handle); add_ivrs_mapping_entry(bdf, bdf, special->header.data_setting, 0, true, iommu); @@ -780,8 +780,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 +789,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 +804,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 +825,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 +859,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 +1135,9 @@ static int __init cf_check parse_ivrs_table(struct acpi_table_header *table) return -ENXIO; } - if ( !ioapic_sbdf[idx].seg && + if ( !ioapic_sbdf[idx].sbdf.seg && /* SB IO-APIC is always on this device in AMD systems. */ - ioapic_sbdf[idx].bdf == PCI_BDF(0, 0x14, 0) ) + ioapic_sbdf[idx].sbdf.bdf == PCI_BDF(0, 0x14, 0) ) 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 16075cd5a1..b788675be2 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,14 +688,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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v4 3/3] drivers: Make ioapic_sbdf and hpet_sbdf contain pci_sbdf_t 2025-04-14 19:19 ` [PATCH v4 3/3] drivers: Make ioapic_sbdf and hpet_sbdf contain pci_sbdf_t Andrii Sultanov @ 2025-04-15 7:05 ` dmkhn 2025-04-15 7:15 ` Andriy Sultanov 2025-04-15 11:12 ` Jan Beulich 1 sibling, 1 reply; 13+ messages in thread From: dmkhn @ 2025-04-15 7:05 UTC (permalink / raw) To: Andrii Sultanov Cc: xen-devel, Jan Beulich, Andrew Cooper, Roger Pau Monné On Mon, Apr 14, 2025 at 08:19:18PM +0100, Andrii Sultanov wrote: > From: Andrii Sultanov <sultanovandriy@gmail.com> > > 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 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 | 30 +++++++--------- > xen/drivers/passthrough/amd/iommu_intr.c | 44 +++++++++++------------- > 3 files changed, 37 insertions(+), 42 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 9e4fbee953..14845766e6 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, PCI_BDF(bus, dev, func) ); PCI_SBDF() is a variadic macro, so, IMO, the line above can be simplified as: ioapic_sbdf[idx].sbdf = PCI_SBDF( seg, bus, dev, func ); Ex: pdev_type() in xen/drivers/passthrough/pci.c Can you please double check in the modified code? > 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, PCI_BDF(bus, dev, func) ); ^^ e.g. here it can be simplified too. > hpet_sbdf.init = HPET_CMDL; > > return 0; > @@ -748,6 +746,7 @@ static u16 __init parse_ivhd_device_special( > { > u16 dev_length, bdf; > unsigned int apic, idx; > + pci_sbdf_t sbdf; > > dev_length = sizeof(*special); > if ( header_length < (block_length + dev_length) ) > @@ -757,6 +756,7 @@ static u16 __init parse_ivhd_device_special( > } > > bdf = special->used_id; > + sbdf = PCI_SBDF(seg, bdf); > if ( bdf >= ivrs_bdf_entries ) > { > AMD_IOMMU_ERROR("IVHD: invalid Device_Entry Dev_Id %#x\n", bdf); > @@ -764,7 +764,7 @@ static u16 __init parse_ivhd_device_special( > } > > AMD_IOMMU_DEBUG("IVHD Special: %pp variety %#x handle %#x\n", > - &PCI_SBDF(seg, bdf), special->variety, special->handle); > + &sbdf, special->variety, special->handle); > add_ivrs_mapping_entry(bdf, bdf, special->header.data_setting, 0, true, > iommu); > > @@ -780,8 +780,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 +789,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 +804,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 +825,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 +859,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 +1135,9 @@ static int __init cf_check parse_ivrs_table(struct acpi_table_header *table) > return -ENXIO; > } > > - if ( !ioapic_sbdf[idx].seg && > + if ( !ioapic_sbdf[idx].sbdf.seg && > /* SB IO-APIC is always on this device in AMD systems. */ > - ioapic_sbdf[idx].bdf == PCI_BDF(0, 0x14, 0) ) > + ioapic_sbdf[idx].sbdf.bdf == PCI_BDF(0, 0x14, 0) ) Looks like something like the following should work: if ( ioapic_sbdf[idx].sbdf.sbdf == PCI_SBDF(0, 0, 0x14, 0).sbdf ) What do you think? > 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 16075cd5a1..b788675be2 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,14 +688,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 > > Thanks, Denis ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 3/3] drivers: Make ioapic_sbdf and hpet_sbdf contain pci_sbdf_t 2025-04-15 7:05 ` dmkhn @ 2025-04-15 7:15 ` Andriy Sultanov 2025-04-15 7:35 ` Jan Beulich 0 siblings, 1 reply; 13+ messages in thread From: Andriy Sultanov @ 2025-04-15 7:15 UTC (permalink / raw) To: dmkhn; +Cc: xen-devel, Jan Beulich, Andrew Cooper, Roger Pau Monné On 4/15/25 8:05 AM, dmkhn@proton.me wrote: >> --- 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, PCI_BDF(bus, dev, func) ); > PCI_SBDF() is a variadic macro, so, IMO, the line above can be simplified as: > > ioapic_sbdf[idx].sbdf = PCI_SBDF( seg, bus, dev, func ); > > Ex: pdev_type() in xen/drivers/passthrough/pci.c > > Can you please double check in the modified code? > >> 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, PCI_BDF(bus, dev, func) ); > ^^ > e.g. here it can be simplified too. You are right, PCI_SBDF(sef, bus, dev, func) works here and above. Will resend. >> @@ -1139,9 +1135,9 @@ static int __init cf_check parse_ivrs_table(struct acpi_table_header *table) >> return -ENXIO; >> } >> >> - if ( !ioapic_sbdf[idx].seg && >> + if ( !ioapic_sbdf[idx].sbdf.seg && >> /* SB IO-APIC is always on this device in AMD systems. */ >> - ioapic_sbdf[idx].bdf == PCI_BDF(0, 0x14, 0) ) >> + ioapic_sbdf[idx].sbdf.bdf == PCI_BDF(0, 0x14, 0) ) > Looks like something like the following should work: > > if ( ioapic_sbdf[idx].sbdf.sbdf == PCI_SBDF(0, 0, 0x14, 0).sbdf ) > > What do you think? Will replace this one as well. Thank you! ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 3/3] drivers: Make ioapic_sbdf and hpet_sbdf contain pci_sbdf_t 2025-04-15 7:15 ` Andriy Sultanov @ 2025-04-15 7:35 ` Jan Beulich 0 siblings, 0 replies; 13+ messages in thread From: Jan Beulich @ 2025-04-15 7:35 UTC (permalink / raw) To: Andriy Sultanov; +Cc: xen-devel, Andrew Cooper, Roger Pau Monné, dmkhn On 15.04.2025 09:15, Andriy Sultanov wrote: > On 4/15/25 8:05 AM, dmkhn@proton.me wrote: >>> --- 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, PCI_BDF(bus, dev, func) ); >> PCI_SBDF() is a variadic macro, so, IMO, the line above can be simplified as: >> >> ioapic_sbdf[idx].sbdf = PCI_SBDF( seg, bus, dev, func ); >> >> Ex: pdev_type() in xen/drivers/passthrough/pci.c >> >> Can you please double check in the modified code? >> >>> 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, PCI_BDF(bus, dev, func) ); >> ^^ >> e.g. here it can be simplified too. > You are right, PCI_SBDF(sef, bus, dev, func) works here and above. Will > resend. And please also drop the stray blanks immediately next to the parentheses. Jan ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 3/3] drivers: Make ioapic_sbdf and hpet_sbdf contain pci_sbdf_t 2025-04-14 19:19 ` [PATCH v4 3/3] drivers: Make ioapic_sbdf and hpet_sbdf contain pci_sbdf_t Andrii Sultanov 2025-04-15 7:05 ` dmkhn @ 2025-04-15 11:12 ` Jan Beulich 1 sibling, 0 replies; 13+ messages in thread From: Jan Beulich @ 2025-04-15 11:12 UTC (permalink / raw) To: Andrii Sultanov; +Cc: Andrew Cooper, Roger Pau Monné, xen-devel On 14.04.2025 21:19, Andrii Sultanov wrote: > @@ -748,6 +746,7 @@ static u16 __init parse_ivhd_device_special( > { > u16 dev_length, bdf; > unsigned int apic, idx; > + pci_sbdf_t sbdf; Why does bdf need keeping around here? > @@ -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); Hmm, this one's somewhat questionable: An IO-APIC isn't a PCI device, so making its "coordinates" look like one can be confusing. > @@ -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) ) Is a local variable here warranted in the first place? Jan ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2025-04-15 11:13 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-04-14 19:19 [PATCH v4 0/3] drivers: Simplify handling of pci_sbdf_t in passthrough/amd Andrii Sultanov 2025-04-14 19:19 ` [PATCH v4 1/3] drivers: Change amd_iommu struct to contain pci_sbdf_t, simplify code Andrii Sultanov 2025-04-15 7:34 ` dmkhn 2025-04-15 7:38 ` Jan Beulich 2025-04-15 7:56 ` Andriy Sultanov 2025-04-15 10:36 ` Jan Beulich 2025-04-14 19:19 ` [PATCH v4 2/3] drivers: Change find_iommu_for_device function to take " Andrii Sultanov 2025-04-15 7:50 ` dmkhn 2025-04-14 19:19 ` [PATCH v4 3/3] drivers: Make ioapic_sbdf and hpet_sbdf contain pci_sbdf_t Andrii Sultanov 2025-04-15 7:05 ` dmkhn 2025-04-15 7:15 ` Andriy Sultanov 2025-04-15 7:35 ` Jan Beulich 2025-04-15 11:12 ` Jan Beulich
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.