Re: [PATCH v3 1/3] drivers: Change amd_iommu struct to contain pci_sbdf_t, simplify code

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.03.2025 16:30, Andrii Sultanov wrote:
> Following on from 250d87dc3ff9 ("x86/msi: Change __msi_set_enable() to
> take pci_sbdf_t"), struct amd_iommu has its seg and bdf fields
> backwards with relation to pci_sbdf_t.

This being backwards isn't relevant anymore, is it?

> Instead of regenerating sbdf_t
> from seg+bdf, make the struct contain pci_sbdf_t directly, which simplifies
> code.
> 
> I've avoided aliasing pci_sbdf_t's fields here, so all the usages of
> amd_iommu->seg|bdf dropped down a namespace to amd_iommu->sbdf.seg|bdf
> and required renaming.

Is this really meant to be part of the description? Feels like it was more
meant to be a remark in the post-commit-message area.

> Bloat-o-meter reports:
> add/remove: 0/0 grow/shrink: 6/11 up/down: 135/-327 (-192)
> Function                                     old     new   delta
> _einittext                                 22028   22092     +64
> amd_iommu_prepare                            853     897     +44
> _hvm_dpci_msi_eoi                            157     168     +11
> __mon_lengths                               2928    2936      +8
> _invalidate_all_devices                      133     138      +5
> amd_iommu_get_reserved_device_memory         521     524      +3
> amd_iommu_domain_destroy                      46      43      -3
> build_info                                   752     744      -8
> amd_iommu_add_device                         856     844     -12
> amd_iommu_msi_enable                          33      20     -13
> update_intremap_entry_from_msi_msg           879     859     -20
> amd_iommu_get_supported_ivhd_type             86      54     -32
> amd_iommu_detect_one_acpi                    918     886     -32
> iterate_ivrs_mappings                        169     129     -40
> flush_command_buffer                         460     417     -43
> set_iommu_interrupt_handler                  421     377     -44
> enable_iommu                                1745    1665     -80
> 
> Resolves: https://gitlab.com/xen-project/xen/-/issues/198
> 
> Reported-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Signed-off-by: Andrii Sultanov <sultanovandriy@xxxxxxxxx>
> 
> ---
> 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    | 26 +++++++--------
>  xen/drivers/passthrough/amd/iommu_map.c     |  6 ++--
>  xen/drivers/passthrough/amd/pci_amd_iommu.c | 22 ++++++-------
>  8 files changed, 66 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..4defa0a44d 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));
> +                    &PCI_SBDF(iommu->sbdf.seg, iommu->sbdf.bdf));

Simply &iommu->sbdf? Much like you do e.g. ...

> @@ -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,

... here?

> --- 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.seg = ivhd_block->pci_segment_group;
> +    iommu->sbdf.bdf = ivhd_block->header.device_id;

Could this now be done in a single assignment, using PCI_SBDF()?

> @@ -500,7 +500,7 @@ static struct amd_iommu *_find_iommu_for_device(int seg, 
> int bdf)
>      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 )
>              return NULL;

Similarly here making a local sbdf up front would allow the two comparisons
to be folded.

> --- 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 )
>      {
> @@ -730,7 +730,7 @@ int cf_check amd_iommu_get_reserved_device_memory(
>               * the same alias ID.
>               */
>              if ( bdf != req && ivrs_mappings[req].iommu &&
> -                 func(0, 0, PCI_SBDF(seg, req).sbdf, ctxt) )
> +                 func(0, 0, sbdf.sbdf, ctxt) )

sbdf was initialized from PCI_SBDF(seg, bdf), not PCI_SBDF(seg, req), so this
looks wrong to me.

> @@ -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->seg == iommu->sbdf.seg && pdev->sbdf.bdf == 
> iommu->sbdf.bdf )

Fold the two comparisons into one again?

Jan