Re: [PATCH v1] drivers: Change amd_iommu struct to contain pci_sbdf_t, simplify code

[Jason Andryuk <jason.andryuk@xxxxxxx>]

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;
>   }
>   
>   /*