[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!



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.