[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v1] drivers: Change amd_iommu struct to contain pci_sbdf_t, simplify code
On Thu, 13 Mar 2025 at 19:59, Jason Andryuk <jason.andryuk@xxxxxxx> wrote: > > On 2025-03-13 14:57, Andrii Sultanov wrote: > > Following on from 250d87dc, struct amd_iommu has its seg and bdf fields > > backwards with relation to pci_sbdf_t. Swap them around, and simplify the > > expressions regenerating an sbdf_t from seg+bdf. > > > > Simplify ioapic_sbdf and bpet_sbdf along the way. Adjust functions > > taking seg and bdf fields of these structs to take pci_sbdf_t instead. > > Simplify comparisons similarly. > > It's rather large. Can this be sensibly split into smaller patches? Will do. > > diff --git a/xen/drivers/passthrough/amd/iommu.h > > b/xen/drivers/passthrough/amd/iommu.h > > index 00e81b4b2a..6903b1bc5d 100644 > > --- a/xen/drivers/passthrough/amd/iommu.h > > +++ b/xen/drivers/passthrough/amd/iommu.h > > @@ -77,8 +77,14 @@ struct amd_iommu { > > struct list_head list; > > spinlock_t lock; /* protect iommu */ > > > > - u16 seg; > > - u16 bdf; > > + union { > > + struct { > > + uint16_t bdf; > > + uint16_t seg; > > Are these still needed by the end of this patch? Yes - otherwise the patch would be larger as bdf and seg would be one namespace deeper - /iommu->seg/iommu->sbdf.seg/ > > + }; > > + 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..57efb7ddda 100644 > > --- a/xen/drivers/passthrough/amd/iommu_acpi.c > > +++ b/xen/drivers/passthrough/amd/iommu_acpi.c > > @@ -107,12 +107,12 @@ static void __init add_ivrs_mapping_entry( > > > @@ -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, .bdf = bdf }; > > Maybe retain the /* XXX */ to highlight that segment is hardcoded to 0. Will do > > + 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)); > > Please drop () for just &sbdf. Will do here and below. > > 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, > > Another candidate for conversion? This function is always used with seg and bdf coming from two different places, and it doesn't convert them to pci_sbdf_t internally, so this is unnecessary and would only increase code size.* * This particular example would be neutral: add/remove: 0/0 grow/shrink: 3/3 up/down: 51/-51 (0) Function old new delta parse_ivmd_block 1271 1296 +25 register_range_for_device 281 299 +18 __mon_lengths 2928 2936 +8 build_info 752 744 -8 parse_ivrs_table 3966 3953 -13 reserve_unity_map_for_device 453 423 -30 But would still increase the diff. > > 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 > > > @@ -916,8 +916,8 @@ static int __init parse_ivhd_block(const struct > > acpi_ivrs_hardware *ivhd_block) > > ivhd_block->pci_segment_group, ivhd_block->info, > > ivhd_block->iommu_attr); > > > > - iommu = find_iommu_from_bdf_cap(ivhd_block->pci_segment_group, > > - ivhd_block->header.device_id, > > + iommu = find_iommu_from_bdf_cap(PCI_SBDF(ivhd_block->pci_segment_group, > > + ivhd_block->header.device_id), > > Please indent to match the end of "PCI_SBDF(". Will do. > > ivhd_block->capability_offset); > > if ( !iommu ) > > { > > diff --git a/xen/drivers/passthrough/amd/iommu_cmd.c > > b/xen/drivers/passthrough/amd/iommu_cmd.c > > index 83c525b84f..dc3d2394a1 100644 > > --- a/xen/drivers/passthrough/amd/iommu_cmd.c > > +++ b/xen/drivers/passthrough/amd/iommu_cmd.c > > @@ -85,7 +85,7 @@ static void flush_command_buffer(struct amd_iommu *iommu, > > threshold |= threshold << 1; > > printk(XENLOG_WARNING > > "AMD IOMMU %pp: %scompletion wait taking too long\n", > > - &PCI_SBDF(iommu->seg, iommu->bdf), > > + &(iommu->sbdf), > > Please drop (). > > > 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), > > Please drop (). > > > timeout_base ? "iotlb " : "", > > (NOW() - start) / 10000000); > > } > > > diff --git a/xen/drivers/passthrough/amd/iommu_detect.c > > b/xen/drivers/passthrough/amd/iommu_detect.c > > index cede44e651..7d60389500 100644 > > --- a/xen/drivers/passthrough/amd/iommu_detect.c > > +++ b/xen/drivers/passthrough/amd/iommu_detect.c > > @@ -231,7 +231,7 @@ int __init amd_iommu_detect_one_acpi( > > rt = pci_ro_device(iommu->seg, bus, PCI_DEVFN(dev, func)); > > if ( rt ) > > printk(XENLOG_ERR "Could not mark config space of %pp read-only > > (%d)\n", > > - &PCI_SBDF(iommu->seg, iommu->bdf), rt); > > + &(iommu->sbdf), rt); > > Please drop (). > > > > > 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..e2c205a857 100644 > > --- a/xen/drivers/passthrough/amd/iommu_init.c > > +++ b/xen/drivers/passthrough/amd/iommu_init.c > > > @@ -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)); > > Please drop (). > > > return 0; > > } > > > > > > @@ -1543,14 +1540,14 @@ static void invalidate_all_domain_pages(void) > > static int cf_check _invalidate_all_devices( > > u16 seg, struct ivrs_mappings *ivrs_mappings) > > { > > - unsigned int bdf; > > + pci_sbdf_t sbdf = { .seg = seg, .bdf = 0 }; > > .bdf = 0 isn't necessary as it will be set to 0 by default. > > u16 req_id; > > struct amd_iommu *iommu; > > > > - for ( bdf = 0; bdf < ivrs_bdf_entries; bdf++ ) > > + for ( /* sbdf.bdf = 0 */ ; sbdf.bdf < ivrs_bdf_entries; sbdf.bdf++ ) > > I'd either set it or just drop the comment. Will drop _invalidate_all_devices hunk entirely, as suggested by Andrew in the sibling reply. > > { > > - iommu = find_iommu_for_device(seg, bdf); > > - req_id = ivrs_mappings[bdf].dte_requestor_id; > > + iommu = find_iommu_for_device(sbdf); > > + req_id = ivrs_mappings[sbdf.bdf].dte_requestor_id; > > if ( iommu ) > > { > > /* > > diff --git a/xen/drivers/passthrough/amd/iommu_intr.c > > b/xen/drivers/passthrough/amd/iommu_intr.c > > index 9abdc38053..0c91125ec0 100644 > > --- a/xen/drivers/passthrough/amd/iommu_intr.c > > +++ b/xen/drivers/passthrough/amd/iommu_intr.c > > > diff --git a/xen/drivers/passthrough/amd/iommu_map.c > > b/xen/drivers/passthrough/amd/iommu_map.c > > index dde393645a..17070904fa 100644 > > --- a/xen/drivers/passthrough/amd/iommu_map.c > > +++ b/xen/drivers/passthrough/amd/iommu_map.c > > @@ -694,17 +694,16 @@ int amd_iommu_reserve_domain_unity_unmap(struct > > domain *d, > > int cf_check amd_iommu_get_reserved_device_memory( > > iommu_grdm_t *func, void *ctxt) > > { > > - unsigned int seg = 0 /* XXX */, bdf; > > - const struct ivrs_mappings *ivrs_mappings = get_ivrs_mappings(seg); > > + pci_sbdf_t sbdf = {0}; > > Just " = {};" > > > + const struct ivrs_mappings *ivrs_mappings = > > get_ivrs_mappings(sbdf.seg); > > /* At least for global entries, avoid reporting them multiple times. > > */ > > enum { pending, processing, done } global = pending; > > > > - for ( bdf = 0; bdf < ivrs_bdf_entries; ++bdf ) > > + for ( /* sbdf.bdf = 0 */ ; sbdf.bdf < ivrs_bdf_entries; ++sbdf.bdf ) > > Like earlier, change to code or remove comment. > > > { > > - pci_sbdf_t sbdf = PCI_SBDF(seg, bdf); > > - const struct ivrs_unity_map *um = ivrs_mappings[bdf].unity_map; > > - unsigned int req = ivrs_mappings[bdf].dte_requestor_id; > > - const struct amd_iommu *iommu = ivrs_mappings[bdf].iommu; > > + const struct ivrs_unity_map *um = > > ivrs_mappings[sbdf.bdf].unity_map; > > + unsigned int req = ivrs_mappings[sbdf.bdf].dte_requestor_id; > > + const struct amd_iommu *iommu = ivrs_mappings[sbdf.bdf].iommu; > > int rc; > > > > if ( !iommu ) Will drop the entire amd_iommu_get_reserved_device_memory hunk as suggested by Andrew. > > diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c > > b/xen/drivers/passthrough/amd/pci_amd_iommu.c > > index d00697edb3..16bab0f948 100644 > > --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c > > +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c > > @@ -32,35 +32,35 @@ static bool __read_mostly init_done; > > > > static const struct iommu_init_ops _iommu_init_ops; > > > > -struct amd_iommu *find_iommu_for_device(int seg, int bdf) > > +struct amd_iommu *find_iommu_for_device(pci_sbdf_t sbdf) > > { > > - struct ivrs_mappings *ivrs_mappings = get_ivrs_mappings(seg); > > + struct ivrs_mappings *ivrs_mappings = get_ivrs_mappings(sbdf.seg); > > Adding: > unsigned int bdf = sbdf.bdf > > here would eliminate all the sbdf.bdf use below. > > Thanks, > Jason > > > > > - if ( !ivrs_mappings || bdf >= ivrs_bdf_entries ) > > + if ( !ivrs_mappings || sbdf.bdf >= ivrs_bdf_entries ) > > return NULL; > > > > - if ( unlikely(!ivrs_mappings[bdf].iommu) && likely(init_done) ) > > + if ( unlikely(!ivrs_mappings[sbdf.bdf].iommu) && likely(init_done) ) > > { > > - unsigned int bd0 = bdf & ~PCI_FUNC(~0); > > + unsigned int bd0 = sbdf.bdf & ~PCI_FUNC(~0); > > > > - if ( ivrs_mappings[bd0].iommu && ivrs_mappings[bd0].iommu->bdf != > > bdf ) > > + if ( ivrs_mappings[bd0].iommu && ivrs_mappings[bd0].iommu->bdf != > > sbdf.bdf ) > > { > > struct ivrs_mappings tmp = ivrs_mappings[bd0]; > > > > tmp.iommu = NULL; > > if ( tmp.dte_requestor_id == bd0 ) > > - tmp.dte_requestor_id = bdf; > > - ivrs_mappings[bdf] = tmp; > > + tmp.dte_requestor_id = sbdf.bdf; > > + ivrs_mappings[sbdf.bdf] = tmp; > > > > printk(XENLOG_WARNING "%pp not found in ACPI tables;" > > - " using same IOMMU as function 0\n", &PCI_SBDF(seg, > > bdf)); > > + " using same IOMMU as function 0\n", &sbdf); > > > > /* write iommu field last */ > > - ivrs_mappings[bdf].iommu = ivrs_mappings[bd0].iommu; > > + ivrs_mappings[sbdf.bdf].iommu = ivrs_mappings[bd0].iommu; > > } > > } > > > > - return ivrs_mappings[bdf].iommu; > > + return ivrs_mappings[sbdf.bdf].iommu; > > } > > > > /* Thank you!
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |