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?
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?
+ };
+ 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.
+ 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.
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?
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(".
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.
{
- 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 )
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;
}
/*
