[PATCH v2 0/2] IOMMU/PCI: assorted follow-on to XSA-400

[PATCH v2 0/2] IOMMU/PCI: assorted follow-on to XSA-400

[Jan Beulich]

Just the two remaining patches, with the only change being a would-be-
build fix to Arm code which cannot be compiled in the first place.

1: PCI: replace stray uses of PCI_{DEVFN,BDF}2()
2: PCI: replace "secondary" flavors of PCI_{DEVFN,BDF,SBDF}()

Jan

[PATCH v2 1/2] PCI: replace stray uses of PCI_{DEVFN,BDF}2()

[Jan Beulich]

There's no good reason to use these when we already have a pci_sbdf_t
type object available. This extends to the use of PCI_BUS() in
pci_ecam_map_bus() as well.

No change to generated code (with gcc11 at least, and I have to admit
that I didn't expect compilers to necessarily be able to spot the
optimization potential on the original code).

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
---
Note that the Arm changes are "blind": I haven't been able to spot a way
to at least compile test the changes there; the code looks to be
entirely dead.
---
v2: Arm build fix (for those who actually have ways to build the Arm
    code being changed here).

--- a/xen/arch/arm/pci/ecam.c
+++ b/xen/arch/arm/pci/ecam.c
@@ -28,8 +28,7 @@ void __iomem *pci_ecam_map_bus(struct pc
         container_of(bridge->ops, const struct pci_ecam_ops, pci_ops);
     unsigned int devfn_shift = ops->bus_shift - 8;
     void __iomem *base;
-
-    unsigned int busn = PCI_BUS(sbdf.bdf);
+    unsigned int busn = sbdf.bus;
 
     if ( busn < cfg->busn_start || busn > cfg->busn_end )
         return NULL;
@@ -37,7 +36,7 @@ void __iomem *pci_ecam_map_bus(struct pc
     busn -= cfg->busn_start;
     base = cfg->win + (busn << ops->bus_shift);
 
-    return base + (PCI_DEVFN2(sbdf.bdf) << devfn_shift) + where;
+    return base + (sbdf.devfn << devfn_shift) + where;
 }
 
 bool __init pci_ecam_need_p2m_hwdom_mapping(struct domain *d,
--- a/xen/arch/x86/msi.c
+++ b/xen/arch/x86/msi.c
@@ -839,7 +839,7 @@ static int msix_capability_init(struct p
             pbus = dev->info.physfn.bus;
             pslot = PCI_SLOT(dev->info.physfn.devfn);
             pfunc = PCI_FUNC(dev->info.physfn.devfn);
-            vf = PCI_BDF2(dev->bus, dev->devfn);
+            vf = dev->sbdf.bdf;
         }
 
         table_paddr = read_pci_mem_bar(seg, pbus, pslot, pfunc, bir, vf);
--- a/xen/drivers/passthrough/vtd/qinval.c
+++ b/xen/drivers/passthrough/vtd/qinval.c
@@ -267,7 +267,7 @@ int qinval_device_iotlb_sync(struct vtd_
     qinval_entry->q.dev_iotlb_inv_dsc.lo.res_1 = 0;
     qinval_entry->q.dev_iotlb_inv_dsc.lo.max_invs_pend = pdev->ats.queue_depth;
     qinval_entry->q.dev_iotlb_inv_dsc.lo.res_2 = 0;
-    qinval_entry->q.dev_iotlb_inv_dsc.lo.sid = PCI_BDF2(pdev->bus, pdev->devfn);
+    qinval_entry->q.dev_iotlb_inv_dsc.lo.sid = pdev->sbdf.bdf;
     qinval_entry->q.dev_iotlb_inv_dsc.lo.res_3 = 0;
 
     qinval_entry->q.dev_iotlb_inv_dsc.hi.size = size;

Re: [PATCH v2 1/2] PCI: replace stray uses of PCI_{DEVFN,BDF}2()

[Durrant, Paul]

On 21/04/2022 15:26, Jan Beulich wrote:
> There's no good reason to use these when we already have a pci_sbdf_t
> type object available. This extends to the use of PCI_BUS() in
> pci_ecam_map_bus() as well.
> 
> No change to generated code (with gcc11 at least, and I have to admit
> that I didn't expect compilers to necessarily be able to spot the
> optimization potential on the original code).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>

Reviewed-by: Paul Durrant <paul@xen.org>

Re: [PATCH v2 1/2] PCI: replace stray uses of PCI_{DEVFN,BDF}2()

[Jan Beulich]

On 21.04.2022 16:26, Jan Beulich wrote:
> There's no good reason to use these when we already have a pci_sbdf_t
> type object available. This extends to the use of PCI_BUS() in
> pci_ecam_map_bus() as well.
> 
> No change to generated code (with gcc11 at least, and I have to admit
> that I didn't expect compilers to necessarily be able to spot the
> optimization potential on the original code).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> ---
> Note that the Arm changes are "blind": I haven't been able to spot a way
> to at least compile test the changes there; the code looks to be
> entirely dead.
> ---
> v2: Arm build fix (for those who actually have ways to build the Arm
>     code being changed here).

May I please get an Arm side ack (or otherwise) here? Especially the
2nd, dependent patch better wouldn't remain pending for too long, or
else there's a fair risk for it to go stale.

Thanks, Jan

> --- a/xen/arch/arm/pci/ecam.c
> +++ b/xen/arch/arm/pci/ecam.c
> @@ -28,8 +28,7 @@ void __iomem *pci_ecam_map_bus(struct pc
>          container_of(bridge->ops, const struct pci_ecam_ops, pci_ops);
>      unsigned int devfn_shift = ops->bus_shift - 8;
>      void __iomem *base;
> -
> -    unsigned int busn = PCI_BUS(sbdf.bdf);
> +    unsigned int busn = sbdf.bus;
>  
>      if ( busn < cfg->busn_start || busn > cfg->busn_end )
>          return NULL;
> @@ -37,7 +36,7 @@ void __iomem *pci_ecam_map_bus(struct pc
>      busn -= cfg->busn_start;
>      base = cfg->win + (busn << ops->bus_shift);
>  
> -    return base + (PCI_DEVFN2(sbdf.bdf) << devfn_shift) + where;
> +    return base + (sbdf.devfn << devfn_shift) + where;
>  }
>  
>  bool __init pci_ecam_need_p2m_hwdom_mapping(struct domain *d,
> --- a/xen/arch/x86/msi.c
> +++ b/xen/arch/x86/msi.c
> @@ -839,7 +839,7 @@ static int msix_capability_init(struct p
>              pbus = dev->info.physfn.bus;
>              pslot = PCI_SLOT(dev->info.physfn.devfn);
>              pfunc = PCI_FUNC(dev->info.physfn.devfn);
> -            vf = PCI_BDF2(dev->bus, dev->devfn);
> +            vf = dev->sbdf.bdf;
>          }
>  
>          table_paddr = read_pci_mem_bar(seg, pbus, pslot, pfunc, bir, vf);
> --- a/xen/drivers/passthrough/vtd/qinval.c
> +++ b/xen/drivers/passthrough/vtd/qinval.c
> @@ -267,7 +267,7 @@ int qinval_device_iotlb_sync(struct vtd_
>      qinval_entry->q.dev_iotlb_inv_dsc.lo.res_1 = 0;
>      qinval_entry->q.dev_iotlb_inv_dsc.lo.max_invs_pend = pdev->ats.queue_depth;
>      qinval_entry->q.dev_iotlb_inv_dsc.lo.res_2 = 0;
> -    qinval_entry->q.dev_iotlb_inv_dsc.lo.sid = PCI_BDF2(pdev->bus, pdev->devfn);
> +    qinval_entry->q.dev_iotlb_inv_dsc.lo.sid = pdev->sbdf.bdf;
>      qinval_entry->q.dev_iotlb_inv_dsc.lo.res_3 = 0;
>  
>      qinval_entry->q.dev_iotlb_inv_dsc.hi.size = size;
> 
> 

Re: [PATCH v2 1/2] PCI: replace stray uses of PCI_{DEVFN,BDF}2()

[Bertrand Marquis]

Hi Jan,

> On 21 Apr 2022, at 15:26, Jan Beulich <jbeulich@suse.com> wrote:
> 
> There's no good reason to use these when we already have a pci_sbdf_t
> type object available. This extends to the use of PCI_BUS() in
> pci_ecam_map_bus() as well.
> 
> No change to generated code (with gcc11 at least, and I have to admit
> that I didn't expect compilers to necessarily be able to spot the
> optimization potential on the original code).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Sorry I missed it.

Cheers
Bertrand


> ---
> Note that the Arm changes are "blind": I haven't been able to spot a way
> to at least compile test the changes there; the code looks to be
> entirely dead.
> ---
> v2: Arm build fix (for those who actually have ways to build the Arm
>    code being changed here).
> 
> --- a/xen/arch/arm/pci/ecam.c
> +++ b/xen/arch/arm/pci/ecam.c
> @@ -28,8 +28,7 @@ void __iomem *pci_ecam_map_bus(struct pc
>         container_of(bridge->ops, const struct pci_ecam_ops, pci_ops);
>     unsigned int devfn_shift = ops->bus_shift - 8;
>     void __iomem *base;
> -
> -    unsigned int busn = PCI_BUS(sbdf.bdf);
> +    unsigned int busn = sbdf.bus;
> 
>     if ( busn < cfg->busn_start || busn > cfg->busn_end )
>         return NULL;
> @@ -37,7 +36,7 @@ void __iomem *pci_ecam_map_bus(struct pc
>     busn -= cfg->busn_start;
>     base = cfg->win + (busn << ops->bus_shift);
> 
> -    return base + (PCI_DEVFN2(sbdf.bdf) << devfn_shift) + where;
> +    return base + (sbdf.devfn << devfn_shift) + where;
> }
> 
> bool __init pci_ecam_need_p2m_hwdom_mapping(struct domain *d,
> --- a/xen/arch/x86/msi.c
> +++ b/xen/arch/x86/msi.c
> @@ -839,7 +839,7 @@ static int msix_capability_init(struct p
>             pbus = dev->info.physfn.bus;
>             pslot = PCI_SLOT(dev->info.physfn.devfn);
>             pfunc = PCI_FUNC(dev->info.physfn.devfn);
> -            vf = PCI_BDF2(dev->bus, dev->devfn);
> +            vf = dev->sbdf.bdf;
>         }
> 
>         table_paddr = read_pci_mem_bar(seg, pbus, pslot, pfunc, bir, vf);
> --- a/xen/drivers/passthrough/vtd/qinval.c
> +++ b/xen/drivers/passthrough/vtd/qinval.c
> @@ -267,7 +267,7 @@ int qinval_device_iotlb_sync(struct vtd_
>     qinval_entry->q.dev_iotlb_inv_dsc.lo.res_1 = 0;
>     qinval_entry->q.dev_iotlb_inv_dsc.lo.max_invs_pend = pdev->ats.queue_depth;
>     qinval_entry->q.dev_iotlb_inv_dsc.lo.res_2 = 0;
> -    qinval_entry->q.dev_iotlb_inv_dsc.lo.sid = PCI_BDF2(pdev->bus, pdev->devfn);
> +    qinval_entry->q.dev_iotlb_inv_dsc.lo.sid = pdev->sbdf.bdf;
>     qinval_entry->q.dev_iotlb_inv_dsc.lo.res_3 = 0;
> 
>     qinval_entry->q.dev_iotlb_inv_dsc.hi.size = size;
> 

[PATCH v2 2/2] PCI: replace "secondary" flavors of PCI_{DEVFN,BDF,SBDF}()

[Jan Beulich]

At their use sites the numeric suffixes are at least odd to read, first
and foremost for PCI_DEVFN2() where the suffix doesn't even match the
number of arguments. Make use of count_args() such that a single flavor
each suffices (leaving aside helper macros, which aren't supposed to be
used from the outside).

In parse_ppr_log_entry() take the opportunity and drop two local
variables and convert an assignment to an initializer.

In VT-d code fold a number of bus+devfn comparison pairs into a single
BDF comparison.

No change to generated code for the vast majority of the adjustments.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>

--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4902,7 +4902,7 @@ int cf_check mmcfg_intercept_write(
     if ( pci_conf_write_intercept(mmio_ctxt->seg, mmio_ctxt->bdf,
                                   offset, bytes, p_data) >= 0 )
         pci_mmcfg_write(mmio_ctxt->seg, PCI_BUS(mmio_ctxt->bdf),
-                        PCI_DEVFN2(mmio_ctxt->bdf), offset, bytes,
+                        PCI_DEVFN(mmio_ctxt->bdf), offset, bytes,
                         *(uint32_t *)p_data);
 
     return X86EMUL_OKAY;
--- a/xen/arch/x86/pci.c
+++ b/xen/arch/x86/pci.c
@@ -90,7 +90,7 @@ int pci_conf_write_intercept(unsigned in
 
     pcidevs_lock();
 
-    pdev = pci_get_pdev(seg, PCI_BUS(bdf), PCI_DEVFN2(bdf));
+    pdev = pci_get_pdev(seg, PCI_BUS(bdf), PCI_DEVFN(bdf));
     if ( pdev )
         rc = pci_msi_conf_write_intercept(pdev, reg, size, data);
 
--- a/xen/arch/x86/x86_64/mmconfig-shared.c
+++ b/xen/arch/x86/x86_64/mmconfig-shared.c
@@ -313,7 +313,7 @@ static int __init pci_mmcfg_check_hostbr
     for (i = 0; !name && i < ARRAY_SIZE(pci_mmcfg_probes); i++) {
         bus =  pci_mmcfg_probes[i].bus;
         devfn = pci_mmcfg_probes[i].devfn;
-        l = pci_conf_read32(PCI_SBDF3(0, bus, devfn), 0);
+        l = pci_conf_read32(PCI_SBDF(0, bus, devfn), 0);
         vendor = l & 0xffff;
         device = (l >> 16) & 0xffff;
 
--- a/xen/common/compat/memory.c
+++ b/xen/common/compat/memory.c
@@ -27,8 +27,8 @@ static int cf_check get_reserved_device_
     xen_pfn_t start, xen_ulong_t nr, u32 id, void *ctxt)
 {
     struct get_reserved_device_memory *grdm = ctxt;
-    uint32_t sbdf = PCI_SBDF3(grdm->map.dev.pci.seg, grdm->map.dev.pci.bus,
-                              grdm->map.dev.pci.devfn).sbdf;
+    uint32_t sbdf = PCI_SBDF(grdm->map.dev.pci.seg, grdm->map.dev.pci.bus,
+                             grdm->map.dev.pci.devfn).sbdf;
 
     if ( !(grdm->map.flags & XENMEM_RDM_ALL) && (sbdf != id) )
         return 0;
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -1055,8 +1055,8 @@ static int cf_check get_reserved_device_
     xen_pfn_t start, xen_ulong_t nr, u32 id, void *ctxt)
 {
     struct get_reserved_device_memory *grdm = ctxt;
-    uint32_t sbdf = PCI_SBDF3(grdm->map.dev.pci.seg, grdm->map.dev.pci.bus,
-                              grdm->map.dev.pci.devfn).sbdf;
+    uint32_t sbdf = PCI_SBDF(grdm->map.dev.pci.seg, grdm->map.dev.pci.bus,
+                             grdm->map.dev.pci.devfn).sbdf;
 
     if ( !(grdm->map.flags & XENMEM_RDM_ALL) && (sbdf != id) )
         return 0;
--- a/xen/drivers/passthrough/amd/iommu_acpi.c
+++ b/xen/drivers/passthrough/amd/iommu_acpi.c
@@ -96,7 +96,7 @@ static void __init add_ivrs_mapping_entr
 
             if ( !ivrs_mappings[alias_id].intremap_table )
                 panic("No memory for %pp's IRT\n",
-                      &PCI_SBDF2(iommu->seg, alias_id));
+                      &PCI_SBDF(iommu->seg, alias_id));
         }
     }
 
@@ -790,7 +790,7 @@ static u16 __init parse_ivhd_device_spec
     }
 
     AMD_IOMMU_DEBUG("IVHD Special: %pp variety %#x handle %#x\n",
-                    &PCI_SBDF2(seg, bdf), special->variety, special->handle);
+                    &PCI_SBDF(seg, bdf), special->variety, special->handle);
     add_ivrs_mapping_entry(bdf, bdf, special->header.data_setting, 0, true,
                            iommu);
 
@@ -816,7 +816,7 @@ static u16 __init parse_ivhd_device_spec
             AMD_IOMMU_DEBUG("IVHD: Command line override present for IO-APIC %#x"
                             "(IVRS: %#x devID %pp)\n",
                             ioapic_sbdf[idx].id, special->handle,
-                            &PCI_SBDF2(seg, bdf));
+                            &PCI_SBDF(seg, bdf));
             break;
         }
 
@@ -888,7 +888,7 @@ static u16 __init parse_ivhd_device_spec
             AMD_IOMMU_DEBUG("IVHD: Command line override present for HPET %#x "
                             "(IVRS: %#x devID %pp)\n",
                             hpet_sbdf.id, special->handle,
-                            &PCI_SBDF2(seg, bdf));
+                            &PCI_SBDF(seg, bdf));
             break;
         case HPET_NONE:
             /* set device id of hpet */
--- 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 am
                      IOMMU_RING_BUFFER_PTR_MASK) )
     {
         printk_once(XENLOG_ERR "AMD IOMMU %pp: no cmd slot available\n",
-                    &PCI_SBDF2(iommu->seg, iommu->bdf));
+                    &PCI_SBDF(iommu->seg, iommu->bdf));
         cpu_relax();
     }
 
@@ -84,7 +84,7 @@ static void flush_command_buffer(struct
             threshold |= threshold << 1;
             printk(XENLOG_WARNING
                    "AMD IOMMU %pp: %scompletion wait taking too long\n",
-                   &PCI_SBDF2(iommu->seg, iommu->bdf),
+                   &PCI_SBDF(iommu->seg, iommu->bdf),
                    timeout_base ? "iotlb " : "");
             timeout = 0;
         }
@@ -94,7 +94,7 @@ static void flush_command_buffer(struct
     if ( !timeout )
         printk(XENLOG_WARNING
                "AMD IOMMU %pp: %scompletion wait took %lums\n",
-               &PCI_SBDF2(iommu->seg, iommu->bdf),
+               &PCI_SBDF(iommu->seg, iommu->bdf),
                timeout_base ? "iotlb " : "",
                (NOW() - start) / 10000000);
 }
@@ -292,14 +292,14 @@ void amd_iommu_flush_iotlb(u8 devfn, con
     if ( !iommu )
     {
         AMD_IOMMU_WARN("can't find IOMMU for %pp\n",
-                       &PCI_SBDF3(pdev->seg, pdev->bus, devfn));
+                       &PCI_SBDF(pdev->seg, pdev->bus, devfn));
         return;
     }
 
     if ( !iommu_has_cap(iommu, PCI_CAP_IOTLB_SHIFT) )
         return;
 
-    req_id = get_dma_requestor_id(iommu->seg, PCI_BDF2(pdev->bus, devfn));
+    req_id = get_dma_requestor_id(iommu->seg, PCI_BDF(pdev->bus, devfn));
     queueid = req_id;
     maxpend = pdev->ats.queue_depth & 0xff;
 
--- 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_SBDF2(iommu->seg, iommu->bdf), rt);
+               &PCI_SBDF(iommu->seg, iommu->bdf), rt);
 
     list_add_tail(&iommu->list, &amd_iommu_head);
     rt = 0;
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -562,7 +562,7 @@ static void cf_check parse_event_log_ent
 
         printk(XENLOG_ERR "AMD-Vi: %s: %pp d%u addr %016"PRIx64
                " flags %#x%s%s%s%s%s%s%s%s%s%s\n",
-               code_str, &PCI_SBDF2(iommu->seg, device_id),
+               code_str, &PCI_SBDF(iommu->seg, device_id),
                domain_id, addr, flags,
                (flags & 0xe00) ? " ??" : "",
                (flags & 0x100) ? " TR" : "",
@@ -578,7 +578,7 @@ static void cf_check parse_event_log_ent
         for ( bdf = 0; bdf < ivrs_bdf_entries; bdf++ )
             if ( get_dma_requestor_id(iommu->seg, bdf) == device_id )
                 pci_check_disable_device(iommu->seg, PCI_BUS(bdf),
-                                         PCI_DEVFN2(bdf));
+                                         PCI_DEVFN(bdf));
     }
     else
         printk(XENLOG_ERR "%s %08x %08x %08x %08x\n",
@@ -631,18 +631,13 @@ static void iommu_check_event_log(struct
 
 static void cf_check parse_ppr_log_entry(struct amd_iommu *iommu, u32 entry[])
 {
-
-    u16 device_id;
-    u8 bus, devfn;
-    struct pci_dev *pdev;
-
     /* here device_id is physical value */
-    device_id = iommu_get_devid_from_cmd(entry[0]);
-    bus = PCI_BUS(device_id);
-    devfn = PCI_DEVFN2(device_id);
+    uint16_t device_id = iommu_get_devid_from_cmd(entry[0]);
+    struct pci_dev *pdev;
 
     pcidevs_lock();
-    pdev = pci_get_real_pdev(iommu->seg, bus, devfn);
+    pdev = pci_get_real_pdev(iommu->seg, PCI_BUS(device_id),
+                             PCI_DEVFN(device_id));
     pcidevs_unlock();
 
     if ( pdev )
@@ -751,12 +746,12 @@ static bool_t __init set_iommu_interrupt
 
     pcidevs_lock();
     iommu->msi.dev = pci_get_pdev(iommu->seg, PCI_BUS(iommu->bdf),
-                                  PCI_DEVFN2(iommu->bdf));
+                                  PCI_DEVFN(iommu->bdf));
     pcidevs_unlock();
     if ( !iommu->msi.dev )
     {
         AMD_IOMMU_WARN("no pdev for %pp\n",
-                       &PCI_SBDF2(iommu->seg, iommu->bdf));
+                       &PCI_SBDF(iommu->seg, iommu->bdf));
         return 0;
     }
 
@@ -778,7 +773,7 @@ static bool_t __init set_iommu_interrupt
         hw_irq_controller *handler;
         u16 control;
 
-        control = pci_conf_read16(PCI_SBDF2(iommu->seg, iommu->bdf),
+        control = pci_conf_read16(PCI_SBDF(iommu->seg, iommu->bdf),
                                   iommu->msi.msi_attrib.pos + PCI_MSI_FLAGS);
 
         iommu->msi.msi.nvec = 1;
@@ -842,22 +837,22 @@ static void amd_iommu_erratum_746_workar
          (boot_cpu_data.x86_model > 0x1f) )
         return;
 
-    pci_conf_write32(PCI_SBDF2(iommu->seg, iommu->bdf), 0xf0, 0x90);
-    value = pci_conf_read32(PCI_SBDF2(iommu->seg, iommu->bdf), 0xf4);
+    pci_conf_write32(PCI_SBDF(iommu->seg, iommu->bdf), 0xf0, 0x90);
+    value = pci_conf_read32(PCI_SBDF(iommu->seg, iommu->bdf), 0xf4);
 
     if ( value & (1 << 2) )
         return;
 
     /* Select NB indirect register 0x90 and enable writing */
-    pci_conf_write32(PCI_SBDF2(iommu->seg, iommu->bdf), 0xf0, 0x90 | (1 << 8));
+    pci_conf_write32(PCI_SBDF(iommu->seg, iommu->bdf), 0xf0, 0x90 | (1 << 8));
 
-    pci_conf_write32(PCI_SBDF2(iommu->seg, iommu->bdf), 0xf4, value | (1 << 2));
+    pci_conf_write32(PCI_SBDF(iommu->seg, iommu->bdf), 0xf4, value | (1 << 2));
     printk(XENLOG_INFO
            "AMD-Vi: Applying erratum 746 workaround for IOMMU at %pp\n",
-           &PCI_SBDF2(iommu->seg, iommu->bdf));
+           &PCI_SBDF(iommu->seg, iommu->bdf));
 
     /* Clear the enable writing bit */
-    pci_conf_write32(PCI_SBDF2(iommu->seg, iommu->bdf), 0xf0, 0x90);
+    pci_conf_write32(PCI_SBDF(iommu->seg, iommu->bdf), 0xf0, 0x90);
 }
 
 static void enable_iommu(struct amd_iommu *iommu)
@@ -1288,7 +1283,7 @@ static int __init cf_check amd_iommu_set
                 if ( !pci_init )
                     continue;
                 pcidevs_lock();
-                pdev = pci_get_pdev(seg, PCI_BUS(bdf), PCI_DEVFN2(bdf));
+                pdev = pci_get_pdev(seg, PCI_BUS(bdf), PCI_DEVFN(bdf));
                 pcidevs_unlock();
             }
 
--- a/xen/drivers/passthrough/amd/iommu_intr.c
+++ b/xen/drivers/passthrough/amd/iommu_intr.c
@@ -604,7 +604,7 @@ static struct amd_iommu *_find_iommu_for
     if ( iommu )
         return iommu;
 
-    AMD_IOMMU_DEBUG("No IOMMU for MSI dev = %pp\n", &PCI_SBDF2(seg, bdf));
+    AMD_IOMMU_DEBUG("No IOMMU for MSI dev = %pp\n", &PCI_SBDF(seg, bdf));
     return ERR_PTR(-EINVAL);
 }
 
@@ -814,7 +814,7 @@ static void dump_intremap_table(const st
         if ( ivrs_mapping )
         {
             printk("  %pp:\n",
-                   &PCI_SBDF2(iommu->seg, ivrs_mapping->dte_requestor_id));
+                   &PCI_SBDF(iommu->seg, ivrs_mapping->dte_requestor_id));
             ivrs_mapping = NULL;
         }
 
--- a/xen/drivers/passthrough/amd/iommu_map.c
+++ b/xen/drivers/passthrough/amd/iommu_map.c
@@ -534,7 +534,7 @@ int cf_check amd_iommu_get_reserved_devi
 
     for ( bdf = 0; bdf < ivrs_bdf_entries; ++bdf )
     {
-        pci_sbdf_t sbdf = PCI_SBDF2(seg, bdf);
+        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;
@@ -563,7 +563,7 @@ int cf_check amd_iommu_get_reserved_devi
              * the same alias ID.
              */
             if ( bdf != req && ivrs_mappings[req].iommu &&
-                 func(0, 0, PCI_SBDF2(seg, req).sbdf, ctxt) )
+                 func(0, 0, PCI_SBDF(seg, req).sbdf, ctxt) )
                 continue;
 
             if ( global == pending )
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -53,7 +53,7 @@ struct amd_iommu *find_iommu_for_device(
             ivrs_mappings[bdf] = tmp;
 
             printk(XENLOG_WARNING "%pp not found in ACPI tables;"
-                   " using same IOMMU as function 0\n", &PCI_SBDF2(seg, bdf));
+                   " using same IOMMU as function 0\n", &PCI_SBDF(seg, bdf));
 
             /* write iommu field last */
             ivrs_mappings[bdf].iommu = ivrs_mappings[bd0].iommu;
@@ -144,7 +144,7 @@ static int __must_check amd_iommu_setup_
                | (ivrs_dev->unity_map ? SET_ROOT_WITH_UNITY_MAP : 0);
 
     /* get device-table entry */
-    req_id = get_dma_requestor_id(iommu->seg, PCI_BDF2(bus, devfn));
+    req_id = get_dma_requestor_id(iommu->seg, PCI_BDF(bus, devfn));
     table = iommu->dev_table.buffer;
     dte = &table[req_id];
     ivrs_dev = &get_ivrs_mappings(iommu->seg)[req_id];
@@ -202,7 +202,7 @@ static int __must_check amd_iommu_setup_
          * presence.  But let's deal with that case only if it is actually
          * found in the wild.
          */
-        if ( req_id != PCI_BDF2(bus, devfn) &&
+        if ( req_id != PCI_BDF(bus, devfn) &&
              (sr_flags & SET_ROOT_WITH_UNITY_MAP) )
             rc = -EOPNOTSUPP;
         else
@@ -231,7 +231,7 @@ static int __must_check amd_iommu_setup_
              (any_pdev_behind_iommu(pdev->domain, pdev, iommu) ||
               pdev->phantom_stride) )
             AMD_IOMMU_WARN(" %pp: reassignment may cause %pd data corruption\n",
-                           &PCI_SBDF3(pdev->seg, bus, devfn), pdev->domain);
+                           &PCI_SBDF(pdev->seg, bus, devfn), pdev->domain);
 
         /*
          * Check remaining settings are still in place from an earlier call
@@ -414,7 +414,7 @@ static void amd_iommu_disable_domain_dev
         disable_ats_device(pdev);
 
     BUG_ON ( iommu->dev_table.buffer == NULL );
-    req_id = get_dma_requestor_id(iommu->seg, PCI_BDF2(bus, devfn));
+    req_id = get_dma_requestor_id(iommu->seg, PCI_BDF(bus, devfn));
     table = iommu->dev_table.buffer;
     dte = &table[req_id];
 
@@ -461,7 +461,7 @@ static int cf_check reassign_device(
     if ( !iommu )
     {
         AMD_IOMMU_WARN("failed to find IOMMU: %pp cannot be assigned to %pd\n",
-                       &PCI_SBDF3(pdev->seg, pdev->bus, devfn), target);
+                       &PCI_SBDF(pdev->seg, pdev->bus, devfn), target);
         return -ENODEV;
     }
 
@@ -488,7 +488,7 @@ static int cf_check reassign_device(
     if ( !is_hardware_domain(source) )
     {
         const struct ivrs_mappings *ivrs_mappings = get_ivrs_mappings(pdev->seg);
-        unsigned int bdf = PCI_BDF2(pdev->bus, devfn);
+        unsigned int bdf = PCI_BDF(pdev->bus, devfn);
 
         rc = amd_iommu_reserve_domain_unity_unmap(
                  source,
@@ -498,7 +498,7 @@ static int cf_check reassign_device(
     }
 
     AMD_IOMMU_DEBUG("Re-assign %pp from %pd to %pd\n",
-                    &PCI_SBDF3(pdev->seg, pdev->bus, devfn), source, target);
+                    &PCI_SBDF(pdev->seg, pdev->bus, devfn), source, target);
 
     return 0;
 }
@@ -507,7 +507,7 @@ static int cf_check amd_iommu_assign_dev
     struct domain *d, u8 devfn, struct pci_dev *pdev, u32 flag)
 {
     struct ivrs_mappings *ivrs_mappings = get_ivrs_mappings(pdev->seg);
-    int bdf = PCI_BDF2(pdev->bus, devfn);
+    unsigned int bdf = PCI_BDF(pdev->bus, devfn);
     int req_id = get_dma_requestor_id(pdev->seg, bdf);
     int rc = amd_iommu_reserve_domain_unity_map(
                  d, ivrs_mappings[req_id].unity_map, flag);
@@ -575,12 +575,12 @@ static int cf_check amd_iommu_add_device
         }
 
         AMD_IOMMU_WARN("no IOMMU for %pp; cannot be handed to %pd\n",
-                        &PCI_SBDF3(pdev->seg, pdev->bus, devfn), pdev->domain);
+                        &PCI_SBDF(pdev->seg, pdev->bus, devfn), pdev->domain);
         return -ENODEV;
     }
 
     ivrs_mappings = get_ivrs_mappings(pdev->seg);
-    bdf = PCI_BDF2(pdev->bus, devfn);
+    bdf = PCI_BDF(pdev->bus, devfn);
     if ( !ivrs_mappings ||
          !ivrs_mappings[ivrs_mappings[bdf].dte_requestor_id].valid )
         return -EPERM;
@@ -618,7 +618,7 @@ static int cf_check amd_iommu_add_device
              ivrs_mappings[ivrs_mappings[bdf].dte_requestor_id].unity_map,
              0) )
         AMD_IOMMU_WARN("%pd: unity mapping failed for %pp\n",
-                       pdev->domain, &PCI_SBDF2(pdev->seg, bdf));
+                       pdev->domain, &PCI_SBDF(pdev->seg, bdf));
 
     if ( iommu_quarantine && pdev->arch.pseudo_domid == DOMID_INVALID )
     {
@@ -651,20 +651,20 @@ static int cf_check amd_iommu_remove_dev
     if ( !iommu )
     {
         AMD_IOMMU_WARN("failed to find IOMMU: %pp cannot be removed from %pd\n",
-                        &PCI_SBDF3(pdev->seg, pdev->bus, devfn), pdev->domain);
+                        &PCI_SBDF(pdev->seg, pdev->bus, devfn), pdev->domain);
         return -ENODEV;
     }
 
     amd_iommu_disable_domain_device(pdev->domain, iommu, devfn, pdev);
 
     ivrs_mappings = get_ivrs_mappings(pdev->seg);
-    bdf = PCI_BDF2(pdev->bus, devfn);
+    bdf = PCI_BDF(pdev->bus, devfn);
 
     if ( amd_iommu_reserve_domain_unity_unmap(
              pdev->domain,
              ivrs_mappings[ivrs_mappings[bdf].dte_requestor_id].unity_map) )
         AMD_IOMMU_WARN("%pd: unity unmapping failed for %pp\n",
-                       pdev->domain, &PCI_SBDF2(pdev->seg, bdf));
+                       pdev->domain, &PCI_SBDF(pdev->seg, bdf));
 
     amd_iommu_quarantine_teardown(pdev);
 
@@ -681,7 +681,7 @@ static int cf_check amd_iommu_remove_dev
 
 static int cf_check amd_iommu_group_id(u16 seg, u8 bus, u8 devfn)
 {
-    int bdf = PCI_BDF2(bus, devfn);
+    unsigned int bdf = PCI_BDF(bus, devfn);
 
     return (bdf < ivrs_bdf_entries) ? get_dma_requestor_id(seg, bdf) : bdf;
 }
--- a/xen/drivers/passthrough/ats.h
+++ b/xen/drivers/passthrough/ats.h
@@ -35,7 +35,7 @@ static inline int pci_ats_enabled(int se
     pos = pci_find_ext_capability(seg, bus, devfn, PCI_EXT_CAP_ID_ATS);
     BUG_ON(!pos);
 
-    value = pci_conf_read16(PCI_SBDF3(seg, bus, devfn), pos + ATS_REG_CTL);
+    value = pci_conf_read16(PCI_SBDF(seg, bus, devfn), pos + ATS_REG_CTL);
 
     return value & ATS_ENABLE;
 }
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -553,7 +553,7 @@ int __init pci_ro_device(int seg, int bu
         memset(pseg->ro_map, 0, sz);
     }
 
-    __set_bit(PCI_BDF2(bus, devfn), pseg->ro_map);
+    __set_bit(PCI_BDF(bus, devfn), pseg->ro_map);
     _pci_hide_device(pdev);
 
     return 0;
@@ -957,7 +957,7 @@ static int deassign_device(struct domain
  out:
     if ( ret )
         printk(XENLOG_G_ERR "%pd: deassign (%pp) failed (%d)\n",
-               d, &PCI_SBDF3(seg, bus, devfn), ret);
+               d, &PCI_SBDF(seg, bus, devfn), ret);
 
     return ret;
 }
@@ -1406,7 +1406,7 @@ static int iommu_add_device(struct pci_d
         rc = iommu_call(hd->platform_ops, add_device, devfn, pci_to_dev(pdev));
         if ( rc )
             printk(XENLOG_WARNING "IOMMU: add %pp failed (%d)\n",
-                   &PCI_SBDF3(pdev->seg, pdev->bus, devfn), rc);
+                   &PCI_SBDF(pdev->seg, pdev->bus, devfn), rc);
     }
 }
 
@@ -1452,7 +1452,7 @@ static int iommu_remove_device(struct pc
             continue;
 
         printk(XENLOG_ERR "IOMMU: remove %pp failed (%d)\n",
-               &PCI_SBDF3(pdev->seg, pdev->bus, devfn), rc);
+               &PCI_SBDF(pdev->seg, pdev->bus, devfn), rc);
         return rc;
     }
 
@@ -1536,7 +1536,7 @@ static int assign_device(struct domain *
  done:
     if ( rc )
         printk(XENLOG_G_WARNING "%pd: assign (%pp) failed (%d)\n",
-               d, &PCI_SBDF3(seg, bus, devfn), rc);
+               d, &PCI_SBDF(seg, bus, devfn), rc);
     /* The device is assigned to dom_io so mark it as quarantined */
     else if ( d == dom_io )
         pdev->quarantine = true;
@@ -1647,7 +1647,7 @@ int iommu_do_pci_domctl(
 
         seg = domctl->u.get_device_group.machine_sbdf >> 16;
         bus = PCI_BUS(domctl->u.get_device_group.machine_sbdf);
-        devfn = PCI_DEVFN2(domctl->u.get_device_group.machine_sbdf);
+        devfn = PCI_DEVFN(domctl->u.get_device_group.machine_sbdf);
         max_sdevs = domctl->u.get_device_group.max_sdevs;
         sdevs = domctl->u.get_device_group.sdev_array;
 
@@ -1697,7 +1697,7 @@ int iommu_do_pci_domctl(
 
         seg = machine_sbdf >> 16;
         bus = PCI_BUS(machine_sbdf);
-        devfn = PCI_DEVFN2(machine_sbdf);
+        devfn = PCI_DEVFN(machine_sbdf);
 
         pcidevs_lock();
         ret = device_assigned(seg, bus, devfn);
@@ -1706,7 +1706,7 @@ int iommu_do_pci_domctl(
             if ( ret )
             {
                 printk(XENLOG_G_INFO "%pp already assigned, or non-existent\n",
-                       &PCI_SBDF3(seg, bus, devfn));
+                       &PCI_SBDF(seg, bus, devfn));
                 ret = -EINVAL;
             }
         }
@@ -1742,7 +1742,7 @@ int iommu_do_pci_domctl(
 
         seg = machine_sbdf >> 16;
         bus = PCI_BUS(machine_sbdf);
-        devfn = PCI_DEVFN2(machine_sbdf);
+        devfn = PCI_DEVFN(machine_sbdf);
 
         pcidevs_lock();
         ret = deassign_device(d, seg, bus, devfn);
--- a/xen/drivers/passthrough/vtd/dmar.c
+++ b/xen/drivers/passthrough/vtd/dmar.c
@@ -222,7 +222,7 @@ struct acpi_drhd_unit *acpi_find_matched
             continue;
 
         for (i = 0; i < drhd->scope.devices_cnt; i++)
-            if ( drhd->scope.devices[i] == PCI_BDF2(bus, devfn) )
+            if ( drhd->scope.devices[i] == PCI_BDF(bus, devfn) )
                 return drhd;
 
         if ( test_bit(bus, drhd->scope.buses) )
@@ -1062,7 +1062,7 @@ int cf_check intel_iommu_get_reserved_de
 
         rc = func(PFN_DOWN(rmrr->base_address),
                   PFN_UP(rmrr->end_address) - PFN_DOWN(rmrr->base_address),
-                  PCI_SBDF2(rmrr->segment, bdf).sbdf, ctxt);
+                  PCI_SBDF(rmrr->segment, bdf).sbdf, ctxt);
 
         if ( unlikely(rc < 0) )
             return rc;
--- a/xen/drivers/passthrough/vtd/intremap.c
+++ b/xen/drivers/passthrough/vtd/intremap.c
@@ -498,7 +498,7 @@ static void set_msi_source_id(struct pci
         case 4: sq = SQ_13_IGNORE_1; break;
         default: sq = SQ_ALL_16; break;
         }
-        set_ire_sid(ire, SVT_VERIFY_SID_SQ, sq, PCI_BDF2(bus, devfn));
+        set_ire_sid(ire, SVT_VERIFY_SID_SQ, sq, PCI_BDF(bus, devfn));
         break;
 
     case DEV_TYPE_PCI:
@@ -508,7 +508,7 @@ static void set_msi_source_id(struct pci
         if ( ret == 0 ) /* integrated PCI device */
         {
             set_ire_sid(ire, SVT_VERIFY_SID_SQ, SQ_ALL_16,
-                        PCI_BDF2(bus, devfn));
+                        PCI_BDF(bus, devfn));
         }
         else if ( ret == 1 ) /* find upstream bridge */
         {
@@ -517,7 +517,7 @@ static void set_msi_source_id(struct pci
                             (bus << 8) | pdev->bus);
             else
                 set_ire_sid(ire, SVT_VERIFY_SID_SQ, SQ_ALL_16,
-                            PCI_BDF2(bus, devfn));
+                            PCI_BDF(bus, devfn));
         }
         else
             dprintk(XENLOG_WARNING VTDPREFIX,
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -938,21 +938,21 @@ static int iommu_page_fault_do_one(struc
                "DMAR:[%s] Request device [%pp] "
                "fault addr %"PRIx64"\n",
                (type ? "DMA Read" : "DMA Write"),
-               &PCI_SBDF2(seg, source_id), addr);
+               &PCI_SBDF(seg, source_id), addr);
         kind = "DMAR";
         break;
     case INTR_REMAP:
         printk(XENLOG_G_WARNING VTDPREFIX
                "INTR-REMAP: Request device [%pp] "
                "fault index %"PRIx64"\n",
-               &PCI_SBDF2(seg, source_id), addr >> 48);
+               &PCI_SBDF(seg, source_id), addr >> 48);
         kind = "INTR-REMAP";
         break;
     default:
         printk(XENLOG_G_WARNING VTDPREFIX
                "UNKNOWN: Request device [%pp] "
                "fault addr %"PRIx64"\n",
-               &PCI_SBDF2(seg, source_id), addr);
+               &PCI_SBDF(seg, source_id), addr);
         kind = "UNKNOWN";
         break;
     }
@@ -961,7 +961,7 @@ static int iommu_page_fault_do_one(struc
            kind, fault_reason, reason);
 
     if ( iommu_verbose && fault_type == DMA_REMAP )
-        print_vtd_entries(iommu, PCI_BUS(source_id), PCI_DEVFN2(source_id),
+        print_vtd_entries(iommu, PCI_BUS(source_id), PCI_DEVFN(source_id),
                           addr >> PAGE_SHIFT);
 
     return 0;
@@ -1039,7 +1039,7 @@ static void __do_iommu_page_fault(struct
                                 source_id, guest_addr);
 
         pci_check_disable_device(iommu->drhd->segment,
-                                 PCI_BUS(source_id), PCI_DEVFN2(source_id));
+                                 PCI_BUS(source_id), PCI_DEVFN(source_id));
 
         fault_index++;
         if ( fault_index > cap_num_fault_regs(iommu->cap) )
@@ -1541,7 +1541,7 @@ int domain_context_mapping_one(
                 check_cleanup_domid_map(domain, pdev, iommu);
             printk(XENLOG_ERR
                    "%pp: unexpected context entry %016lx_%016lx (expected %016lx_%016lx)\n",
-                   &PCI_SBDF3(seg, bus, devfn),
+                   &PCI_SBDF(seg, bus, devfn),
                    (uint64_t)(res >> 64), (uint64_t)res,
                    (uint64_t)(old >> 64), (uint64_t)old);
             rc = -EILSEQ;
@@ -1571,7 +1571,7 @@ int domain_context_mapping_one(
         if ( !(mode & (MAP_OWNER_DYING | MAP_SINGLE_DEVICE)) )
             printk(XENLOG_WARNING VTDPREFIX
                    " %pp: reassignment may cause %pd data corruption\n",
-                   &PCI_SBDF3(seg, bus, devfn), prev_dom);
+                   &PCI_SBDF(seg, bus, devfn), prev_dom);
 
         write_atomic(&context->lo, lctxt.lo);
         /* No barrier should be needed between these two. */
@@ -1581,7 +1581,7 @@ int domain_context_mapping_one(
     iommu_sync_cache(context, sizeof(struct context_entry));
     spin_unlock(&iommu->lock);
 
-    rc = iommu_flush_context_device(iommu, prev_did, PCI_BDF2(bus, devfn),
+    rc = iommu_flush_context_device(iommu, prev_did, PCI_BDF(bus, devfn),
                                     DMA_CCMD_MASK_NOBIT, !prev_dom);
     flush_dev_iotlb = !!find_ats_dev_drhd(iommu);
     ret = iommu_flush_iotlb_dsi(iommu, prev_did, !prev_dom, flush_dev_iotlb);
@@ -1688,7 +1688,7 @@ static int domain_context_mapping(struct
     case DEV_TYPE_PCI_HOST_BRIDGE:
         if ( iommu_debug )
             printk(VTDPREFIX "%pd:Hostbridge: skip %pp map\n",
-                   domain, &PCI_SBDF3(seg, bus, devfn));
+                   domain, &PCI_SBDF(seg, bus, devfn));
         if ( !is_hardware_domain(domain) )
             return -EPERM;
         break;
@@ -1712,7 +1712,7 @@ static int domain_context_mapping(struct
 
         if ( iommu_debug )
             printk(VTDPREFIX "%pd:PCIe: map %pp\n",
-                   domain, &PCI_SBDF3(seg, bus, devfn));
+                   domain, &PCI_SBDF(seg, bus, devfn));
         ret = domain_context_mapping_one(domain, drhd->iommu, bus, devfn, pdev,
                                          DEVICE_DOMID(domain, pdev), pgd_maddr,
                                          mode);
@@ -1737,7 +1737,7 @@ static int domain_context_mapping(struct
 
         if ( iommu_debug )
             printk(VTDPREFIX "%pd:PCI: map %pp\n",
-                   domain, &PCI_SBDF3(seg, bus, devfn));
+                   domain, &PCI_SBDF(seg, bus, devfn));
 
         ret = domain_context_mapping_one(domain, drhd->iommu, bus, devfn,
                                          pdev, DEVICE_DOMID(domain, pdev),
@@ -1802,7 +1802,7 @@ static int domain_context_mapping(struct
 
     default:
         dprintk(XENLOG_ERR VTDPREFIX, "%pd:unknown(%u): %pp\n",
-                domain, pdev->type, &PCI_SBDF3(seg, bus, devfn));
+                domain, pdev->type, &PCI_SBDF(seg, bus, devfn));
         ret = -EINVAL;
         break;
     }
@@ -1851,7 +1851,7 @@ int domain_context_unmap_one(
     iommu_sync_cache(context, sizeof(struct context_entry));
 
     rc = iommu_flush_context_device(iommu, iommu_domid,
-                                    PCI_BDF2(bus, devfn),
+                                    PCI_BDF(bus, devfn),
                                     DMA_CCMD_MASK_NOBIT, 0);
 
     flush_dev_iotlb = !!find_ats_dev_drhd(iommu);
@@ -1910,7 +1910,7 @@ static const struct acpi_drhd_unit *doma
     case DEV_TYPE_PCI_HOST_BRIDGE:
         if ( iommu_debug )
             printk(VTDPREFIX "%pd:Hostbridge: skip %pp unmap\n",
-                   domain, &PCI_SBDF3(seg, bus, devfn));
+                   domain, &PCI_SBDF(seg, bus, devfn));
         return ERR_PTR(is_hardware_domain(domain) ? 0 : -EPERM);
 
     case DEV_TYPE_PCIe_BRIDGE:
@@ -1924,7 +1924,7 @@ static const struct acpi_drhd_unit *doma
 
         if ( iommu_debug )
             printk(VTDPREFIX "%pd:PCIe: unmap %pp\n",
-                   domain, &PCI_SBDF3(seg, bus, devfn));
+                   domain, &PCI_SBDF(seg, bus, devfn));
         ret = domain_context_unmap_one(domain, iommu, bus, devfn);
         if ( !ret && devfn == pdev->devfn && ats_device(pdev, drhd) > 0 )
             disable_ats_device(pdev);
@@ -1937,7 +1937,7 @@ static const struct acpi_drhd_unit *doma
 
         if ( iommu_debug )
             printk(VTDPREFIX "%pd:PCI: unmap %pp\n",
-                   domain, &PCI_SBDF3(seg, bus, devfn));
+                   domain, &PCI_SBDF(seg, bus, devfn));
         ret = domain_context_unmap_one(domain, iommu, bus, devfn);
         if ( ret )
             break;
@@ -1970,7 +1970,7 @@ static const struct acpi_drhd_unit *doma
 
     default:
         dprintk(XENLOG_ERR VTDPREFIX, "%pd:unknown(%u): %pp\n",
-                domain, pdev->type, &PCI_SBDF3(seg, bus, devfn));
+                domain, pdev->type, &PCI_SBDF(seg, bus, devfn));
         return ERR_PTR(-EINVAL);
     }
 
@@ -2181,9 +2181,7 @@ static int cf_check intel_iommu_add_devi
 
     for_each_rmrr_device ( rmrr, bdf, i )
     {
-        if ( rmrr->segment == pdev->seg &&
-             PCI_BUS(bdf) == pdev->bus &&
-             PCI_DEVFN2(bdf) == devfn )
+        if ( rmrr->segment == pdev->seg && bdf == PCI_BDF(pdev->bus, devfn) )
         {
             /*
              * iommu_add_device() is only called for the hardware
@@ -2239,9 +2237,7 @@ static int cf_check intel_iommu_remove_d
 
     for_each_rmrr_device ( rmrr, bdf, i )
     {
-        if ( rmrr->segment != pdev->seg ||
-             PCI_BUS(bdf) != pdev->bus ||
-             PCI_DEVFN2(bdf) != devfn )
+        if ( rmrr->segment != pdev->seg || bdf != PCI_BDF(pdev->bus, devfn) )
             continue;
 
         /*
@@ -2668,8 +2664,7 @@ static int cf_check reassign_device_owne
 
         for_each_rmrr_device( rmrr, bdf, i )
             if ( rmrr->segment == pdev->seg &&
-                 PCI_BUS(bdf) == pdev->bus &&
-                 PCI_DEVFN2(bdf) == devfn )
+                 bdf == PCI_BDF(pdev->bus, devfn) )
             {
                 /*
                  * Any RMRR flag is always ignored when remove a device,
@@ -2713,9 +2708,7 @@ static int cf_check intel_iommu_assign_d
      */
     for_each_rmrr_device( rmrr, bdf, i )
     {
-        if ( rmrr->segment == seg &&
-             PCI_BUS(bdf) == bus &&
-             PCI_DEVFN2(bdf) == devfn &&
+        if ( rmrr->segment == seg && bdf == PCI_BDF(bus, devfn) &&
              rmrr->scope.devices_cnt > 1 )
         {
             bool_t relaxed = !!(flag & XEN_DOMCTL_DEV_RDM_RELAXED);
@@ -2725,7 +2718,7 @@ static int cf_check intel_iommu_assign_d
                    " with shared RMRR at %"PRIx64" for %pd.\n",
                    relaxed ? XENLOG_WARNING : XENLOG_ERR,
                    relaxed ? "risky" : "disallowed",
-                   &PCI_SBDF3(seg, bus, devfn), rmrr->base_address, d);
+                   &PCI_SBDF(seg, bus, devfn), rmrr->base_address, d);
             if ( !relaxed )
                 return -EPERM;
         }
@@ -2737,9 +2730,7 @@ static int cf_check intel_iommu_assign_d
     /* Setup rmrr identity mapping */
     for_each_rmrr_device( rmrr, bdf, i )
     {
-        if ( rmrr->segment == seg &&
-             PCI_BUS(bdf) == bus &&
-             PCI_DEVFN2(bdf) == devfn )
+        if ( rmrr->segment == seg && bdf == PCI_BDF(bus, devfn) )
         {
             ret = iommu_identity_mapping(d, p2m_access_rw, rmrr->base_address,
                                          rmrr->end_address, flag);
@@ -2762,9 +2753,7 @@ static int cf_check intel_iommu_assign_d
 
     for_each_rmrr_device( rmrr, bdf, i )
     {
-        if ( rmrr->segment == seg &&
-             PCI_BUS(bdf) == bus &&
-             PCI_DEVFN2(bdf) == devfn )
+        if ( rmrr->segment == seg && bdf == PCI_BDF(bus, devfn) )
         {
             int rc = iommu_identity_mapping(d, p2m_access_x,
                                             rmrr->base_address,
@@ -2791,7 +2780,7 @@ static int cf_check intel_iommu_group_id
     if ( find_upstream_bridge(seg, &bus, &devfn, &secbus) < 0 )
         return -ENODEV;
 
-    return PCI_BDF2(bus, devfn);
+    return PCI_BDF(bus, devfn);
 }
 
 static int __must_check cf_check vtd_suspend(void)
--- a/xen/drivers/passthrough/vtd/quirks.c
+++ b/xen/drivers/passthrough/vtd/quirks.c
@@ -115,7 +115,7 @@ bool is_azalia_tlb_enabled(const struct
         return true;
 
     /* Check for the specific device. */
-    sbdf = PCI_SBDF2(drhd->segment, drhd->scope.devices[0]);
+    sbdf = PCI_SBDF(drhd->segment, drhd->scope.devices[0]);
     if ( pci_conf_read16(sbdf, PCI_VENDOR_ID) != PCI_VENDOR_ID_INTEL ||
          pci_conf_read16(sbdf, PCI_DEVICE_ID) != 0x3a3e )
         return true;
@@ -446,7 +446,7 @@ int me_wifi_quirk(struct domain *domain,
             return 0;
 
         /* if device is WLAN device, map ME phantom device 0:3.7 */
-        id = pci_conf_read32(PCI_SBDF3(0, bus, devfn), 0);
+        id = pci_conf_read32(PCI_SBDF(0, bus, devfn), 0);
         switch (id)
         {
             case 0x42328086:
@@ -470,7 +470,7 @@ int me_wifi_quirk(struct domain *domain,
             return 0;
 
         /* if device is WLAN device, map ME phantom device 0:22.7 */
-        id = pci_conf_read32(PCI_SBDF3(0, bus, devfn), 0);
+        id = pci_conf_read32(PCI_SBDF(0, bus, devfn), 0);
         switch (id)
         {
             case 0x00878086:        /* Kilmer Peak */
--- a/xen/drivers/passthrough/vtd/utils.c
+++ b/xen/drivers/passthrough/vtd/utils.c
@@ -96,7 +96,7 @@ void print_vtd_entries(struct vtd_iommu
     u32 l_index, level;
 
     printk("print_vtd_entries: iommu #%u dev %pp gmfn %"PRI_gfn"\n",
-           iommu->index, &PCI_SBDF3(iommu->drhd->segment, bus, devfn),
+           iommu->index, &PCI_SBDF(iommu->drhd->segment, bus, devfn),
            gmfn);
 
     if ( iommu->root_maddr == 0 )
--- a/xen/drivers/pci/pci.c
+++ b/xen/drivers/pci/pci.c
@@ -46,12 +46,12 @@ int pci_find_next_cap(u16 seg, u8 bus, u
 
     while ( ttl-- )
     {
-        pos = pci_conf_read8(PCI_SBDF3(seg, bus, devfn), pos);
+        pos = pci_conf_read8(PCI_SBDF(seg, bus, devfn), pos);
         if ( pos < 0x40 )
             break;
 
         pos &= ~3;
-        id = pci_conf_read8(PCI_SBDF3(seg, bus, devfn), pos + PCI_CAP_LIST_ID);
+        id = pci_conf_read8(PCI_SBDF(seg, bus, devfn), pos + PCI_CAP_LIST_ID);
 
         if ( id == 0xff )
             break;
@@ -93,7 +93,7 @@ int pci_find_next_ext_capability(int seg
     int ttl = 480; /* 3840 bytes, minimum 8 bytes per capability */
     int pos = max(start, 0x100);
 
-    header = pci_conf_read32(PCI_SBDF3(seg, bus, devfn), pos);
+    header = pci_conf_read32(PCI_SBDF(seg, bus, devfn), pos);
 
     /*
      * If we have no capabilities, this is indicated by cap ID,
@@ -109,7 +109,7 @@ int pci_find_next_ext_capability(int seg
         pos = PCI_EXT_CAP_NEXT(header);
         if ( pos < 0x100 )
             break;
-        header = pci_conf_read32(PCI_SBDF3(seg, bus, devfn), pos);
+        header = pci_conf_read32(PCI_SBDF(seg, bus, devfn), pos);
     }
     return 0;
 }
@@ -162,7 +162,7 @@ const char *__init parse_pci_seg(const c
     else
         func = 0;
     if ( seg != (seg_p ? (u16)seg : 0) ||
-         bus != PCI_BUS(PCI_BDF2(bus, 0)) ||
+         bus != PCI_BUS(PCI_BDF(bus, 0)) ||
          dev != PCI_SLOT(PCI_DEVFN(dev, 0)) ||
          func != PCI_FUNC(PCI_DEVFN(0, func)) )
         return NULL;
--- a/xen/drivers/video/vga.c
+++ b/xen/drivers/video/vga.c
@@ -122,9 +122,9 @@ void __init video_endboot(void)
                 pcidevs_unlock();
 
                 if ( !pdev ||
-                     pci_conf_read16(PCI_SBDF3(0, bus, devfn),
+                     pci_conf_read16(PCI_SBDF(0, bus, devfn),
                                      PCI_CLASS_DEVICE) != 0x0300 ||
-                     !(pci_conf_read16(PCI_SBDF3(0, bus, devfn), PCI_COMMAND) &
+                     !(pci_conf_read16(PCI_SBDF(0, bus, devfn), PCI_COMMAND) &
                        (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) )
                     continue;
 
@@ -136,12 +136,12 @@ void __init video_endboot(void)
                         b = 0;
                         break;
                     case 1:
-                        switch ( pci_conf_read8(PCI_SBDF3(0, b, df),
+                        switch ( pci_conf_read8(PCI_SBDF(0, b, df),
                                                 PCI_HEADER_TYPE) )
                         {
                         case PCI_HEADER_TYPE_BRIDGE:
                         case PCI_HEADER_TYPE_CARDBUS:
-                            if ( pci_conf_read16(PCI_SBDF3(0, b, df),
+                            if ( pci_conf_read16(PCI_SBDF(0, b, df),
                                                  PCI_BRIDGE_CONTROL) &
                                  PCI_BRIDGE_CTL_VGA )
                                 continue;
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -29,16 +29,21 @@
 #define PCI_BUS(bdf)    (((bdf) >> 8) & 0xff)
 #define PCI_SLOT(bdf)   (((bdf) >> 3) & 0x1f)
 #define PCI_FUNC(bdf)   ((bdf) & 0x07)
-#define PCI_DEVFN(d,f)  ((((d) & 0x1f) << 3) | ((f) & 0x07))
-#define PCI_DEVFN2(bdf) ((bdf) & 0xff)
-#define PCI_BDF(b,d,f)  ((((b) & 0xff) << 8) | PCI_DEVFN(d,f))
-#define PCI_BDF2(b,df)  ((((b) & 0xff) << 8) | ((df) & 0xff))
-#define PCI_SBDF(s,b,d,f) \
-    ((pci_sbdf_t){ .sbdf = (((s) & 0xffff) << 16) | PCI_BDF(b, d, f) })
-#define PCI_SBDF2(s,bdf) \
+
+#define PCI_DEVFN1_(df)   ((df) & 0xff)
+#define PCI_DEVFN2_(d, f) ((((d) & 0x1f) << 3) | ((f) & 7))
+#define PCI_SBDF4_(s, b, d, f...) \
+    ((pci_sbdf_t){ .sbdf = (((s) & 0xffff) << 16) | PCI_BDF(b, d, ##f) })
+#define PCI_SBDF3_ PCI_SBDF4_
+#define PCI_SBDF2_(s, bdf) \
     ((pci_sbdf_t){ .sbdf = (((s) & 0xffff) << 16) | ((bdf) & 0xffff) })
-#define PCI_SBDF3(s,b,df) \
-    ((pci_sbdf_t){ .sbdf = (((s) & 0xffff) << 16) | PCI_BDF2(b, df) })
+
+#define PCI__(what, nr) PCI_##what##nr##_
+#define PCI_(what, nr)  PCI__(what, nr)
+
+#define PCI_DEVFN(d, f...)   PCI_(DEVFN, count_args(d, ##f))(d, ##f)
+#define PCI_BDF(b, d, f...)  ((((b) & 0xff) << 8) | PCI_DEVFN(d, ##f))
+#define PCI_SBDF(s, b, d...) PCI_(SBDF, count_args(s, b, ##d))(s, b, ##d)
 
 #define ECAM_REG_OFFSET(addr)  ((addr) & 0x00000fff)
 

Re: [PATCH v2 2/2] PCI: replace "secondary" flavors of PCI_{DEVFN,BDF,SBDF}()

[Durrant, Paul]

On 21/04/2022 15:27, Jan Beulich wrote:
> At their use sites the numeric suffixes are at least odd to read, first
> and foremost for PCI_DEVFN2() where the suffix doesn't even match the
> number of arguments. Make use of count_args() such that a single flavor
> each suffices (leaving aside helper macros, which aren't supposed to be
> used from the outside).
> 
> In parse_ppr_log_entry() take the opportunity and drop two local
> variables and convert an assignment to an initializer.
> 
> In VT-d code fold a number of bus+devfn comparison pairs into a single
> BDF comparison.
> 
> No change to generated code for the vast majority of the adjustments.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>

Reviewed-by: Paul Durrant <paul@xen.org>