[PATCH 0/8] IOMMU: assorted follow-on to XSA-400

[PATCH 0/8] IOMMU: assorted follow-on to XSA-400

[Jan Beulich]

Considering how long it took to get the XSA-400 series ready, it is
probably quite natural that in the course of doing so some further
items were found which want dealing with. There we go ...

1: IOMMU/x86: drop locking from quarantine_init() hooks
2: VT-d: drop ROOT_ENTRY_NR
3: VT-d: plug memory leaks in iommu_alloc()
4: VT-d: refuse to use IOMMU with reserved CAP.ND value
5: AMD/IOMMU: replace a few PCI_BDF2()
6: IOMMU: log appropriate SBDF
7: PCI: replace stray uses of PCI_{DEVFN,BDF}2()
8: PCI: replace "secondary" flavors of PCI_{DEVFN,BDF,SBDF}()

Jan

[PATCH 1/8] IOMMU/x86: drop locking from quarantine_init() hooks

[Jan Beulich]

Prior extension of these functions to enable per-device quarantine page
tables already didn't add more locking there, but merely left in place
what had been there before. But really locking is unnecessary here:
We're running with pcidevs_lock held (i.e. multiple invocations of the
same function [or their teardown equivalents] are impossible, and hence
there are no "local" races), while all consuming of the data being
populated here can't race anyway due to happening sequentially
afterwards. See also the comment in struct arch_pci_dev.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/passthrough/amd/iommu_map.c
+++ b/xen/drivers/passthrough/amd/iommu_map.c
@@ -699,15 +699,11 @@ int cf_check amd_iommu_quarantine_init(s
         union amd_iommu_pte *root;
         struct page_info *pgs[IOMMU_MAX_PT_LEVELS] = {};
 
-        spin_lock(&hd->arch.mapping_lock);
-
         root = __map_domain_page(pdev->arch.amd.root_table);
         rc = fill_qpt(root, level - 1, pgs);
         unmap_domain_page(root);
 
         pdev->arch.leaf_mfn = page_to_mfn(pgs[0]);
-
-        spin_unlock(&hd->arch.mapping_lock);
     }
 
     page_list_move(&pdev->arch.pgtables_list, &hd->arch.pgtables.list);
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -3054,15 +3054,11 @@ static int cf_check intel_iommu_quaranti
         struct dma_pte *root;
         struct page_info *pgs[6] = {};
 
-        spin_lock(&hd->arch.mapping_lock);
-
         root = map_vtd_domain_page(pdev->arch.vtd.pgd_maddr);
         rc = fill_qpt(root, level - 1, pgs);
         unmap_vtd_domain_page(root);
 
         pdev->arch.leaf_mfn = page_to_mfn(pgs[0]);
-
-        spin_unlock(&hd->arch.mapping_lock);
     }
 
     page_list_move(&pdev->arch.pgtables_list, &hd->arch.pgtables.list);

[PATCH 2/8] VT-d: drop ROOT_ENTRY_NR

[Jan Beulich]

It's not only misplaced, but entirely unused.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/passthrough/vtd/iommu.h
+++ b/xen/drivers/passthrough/vtd/iommu.h
@@ -204,7 +204,6 @@ struct context_entry {
         __uint128_t full;
     };
 };
-#define ROOT_ENTRY_NR (PAGE_SIZE_4K/sizeof(struct root_entry))
 #define context_present(c) ((c).lo & 1)
 #define context_fault_disable(c) (((c).lo >> 1) & 1)
 #define context_translation_type(c) (((c).lo >> 2) & 3)

[PATCH 3/8] VT-d: plug memory leaks in iommu_alloc()

[Jan Beulich]

While 97af062b89d5 ("IOMMU/x86: maintain a per-device pseudo domain ID")
took care of not making things worse, plugging pre-existing leaks wasn't
the purpose of that change; they're not security relevant after all.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1238,8 +1238,9 @@ int __init iommu_alloc(struct acpi_drhd_
     drhd->iommu = iommu;
 
     iommu->reg = ioremap(drhd->address, PAGE_SIZE);
+    rc = -ENOMEM;
     if ( !iommu->reg )
-        return -ENOMEM;
+        goto free;
     iommu->index = nr_iommus++;
 
     iommu->cap = dmar_readq(iommu->reg, DMAR_CAP_REG);
@@ -1260,8 +1261,9 @@ int __init iommu_alloc(struct acpi_drhd_
         printk(VTDPREFIX "cap = %"PRIx64" ecap = %"PRIx64"\n",
                iommu->cap, iommu->ecap);
     }
+    rc = -ENODEV;
     if ( !(iommu->cap + 1) || !(iommu->ecap + 1) )
-        return -ENODEV;
+        goto free;
 
     quirk_iommu_caps(iommu);
 
@@ -1272,7 +1274,8 @@ int __init iommu_alloc(struct acpi_drhd_
     {
         printk(XENLOG_ERR VTDPREFIX "IOMMU: unsupported\n");
         print_iommu_regs(drhd);
-        return -ENODEV;
+        rc = -ENODEV;
+        goto free;
     }
 
     /* Calculate number of pagetable levels: 3 or 4. */
@@ -1283,7 +1286,8 @@ int __init iommu_alloc(struct acpi_drhd_
     {
         printk(XENLOG_ERR VTDPREFIX "IOMMU: unsupported sagaw %x\n", sagaw);
         print_iommu_regs(drhd);
-        return -ENODEV;
+        rc = -ENODEV;
+        goto free;
     }
     iommu->nr_pt_levels = agaw_to_level(agaw);
 
@@ -1298,8 +1302,9 @@ int __init iommu_alloc(struct acpi_drhd_
         iommu->domid_bitmap = xzalloc_array(unsigned long,
                                             BITS_TO_LONGS(nr_dom));
         iommu->domid_map = xzalloc_array(domid_t, nr_dom);
+        rc = -ENOMEM;
         if ( !iommu->domid_bitmap || !iommu->domid_map )
-            return -ENOMEM;
+            goto free;
 
         /*
          * If Caching mode is set, then invalid translations are tagged

[PATCH 4/8] VT-d: refuse to use IOMMU with reserved CAP.ND value

[Jan Beulich]

The field taking the value 7 (resulting in 18-bit DIDs when using the
calculation in cap_ndoms(), when the DID fields are only 16 bits wide)
is reserved. Instead of misbehaving in case we would encounter such an
IOMMU, refuse to use it.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1267,8 +1267,11 @@ int __init iommu_alloc(struct acpi_drhd_
 
     quirk_iommu_caps(iommu);
 
+    nr_dom = cap_ndoms(iommu->cap);
+
     if ( cap_fault_reg_offset(iommu->cap) +
          cap_num_fault_regs(iommu->cap) * PRIMARY_FAULT_REG_LEN > PAGE_SIZE ||
+         ((nr_dom - 1) >> 16) /* I.e. cap.nd > 6 */ ||
          (has_register_based_invalidation(iommu) &&
           ecap_iotlb_offset(iommu->ecap) >= PAGE_SIZE) )
     {
@@ -1294,8 +1297,6 @@ int __init iommu_alloc(struct acpi_drhd_
     if ( !ecap_coherent(iommu->ecap) )
         iommu_non_coherent = true;
 
-    nr_dom = cap_ndoms(iommu->cap);
-
     if ( nr_dom <= DOMID_MASK * 2 + cap_caching_mode(iommu->cap) )
     {
         /* Allocate domain id (bit) maps. */

[PATCH 5/8] AMD/IOMMU: replace a few PCI_BDF2()

[Jan Beulich]

struct pci_dev has the wanted value directly available; use it. Note
that this fixes a - imo benign - mistake in reassign_device(): The unity
map removal ought to be based on the passed in devfn (as is the case on
the establishing side). This is benign because the mappings would be
removed anyway a little later, when the "main" device gets processed.
While there also limit the scope of two variables in that function.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/passthrough/amd/iommu_cmd.c
+++ b/xen/drivers/passthrough/amd/iommu_cmd.c
@@ -287,7 +287,7 @@ void amd_iommu_flush_iotlb(u8 devfn, con
     if ( !pci_ats_enabled(pdev->seg, pdev->bus, pdev->devfn) )
         return;
 
-    iommu = find_iommu_for_device(pdev->seg, PCI_BDF2(pdev->bus, pdev->devfn));
+    iommu = find_iommu_for_device(pdev->seg, pdev->sbdf.bdf);
 
     if ( !iommu )
     {
--- a/xen/drivers/passthrough/amd/iommu_intr.c
+++ b/xen/drivers/passthrough/amd/iommu_intr.c
@@ -617,7 +617,7 @@ int cf_check amd_iommu_msi_msg_update_ir
     unsigned int i, nr = 1;
     u32 data;
 
-    bdf = pdev ? PCI_BDF2(pdev->bus, pdev->devfn) : hpet_sbdf.bdf;
+    bdf = pdev ? pdev->sbdf.bdf : hpet_sbdf.bdf;
     seg = pdev ? pdev->seg : hpet_sbdf.seg;
 
     iommu = _find_iommu_for_device(seg, bdf);
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -455,11 +455,9 @@ static int cf_check reassign_device(
     struct pci_dev *pdev)
 {
     struct amd_iommu *iommu;
-    int bdf, rc;
-    const struct ivrs_mappings *ivrs_mappings = get_ivrs_mappings(pdev->seg);
+    int rc;
 
-    bdf = PCI_BDF2(pdev->bus, pdev->devfn);
-    iommu = find_iommu_for_device(pdev->seg, bdf);
+    iommu = find_iommu_for_device(pdev->seg, pdev->sbdf.bdf);
     if ( !iommu )
     {
         AMD_IOMMU_WARN("failed to find IOMMU: %pp cannot be assigned to %pd\n",
@@ -489,6 +487,9 @@ 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);
+
         rc = amd_iommu_reserve_domain_unity_unmap(
                  source,
                  ivrs_mappings[get_dma_requestor_id(pdev->seg, bdf)].unity_map);
@@ -558,13 +559,11 @@ static int cf_check amd_iommu_add_device
     if ( !pdev->domain )
         return -EINVAL;
 
-    bdf = PCI_BDF2(pdev->bus, pdev->devfn);
-
     for_each_amd_iommu(iommu)
-        if ( pdev->seg == iommu->seg && bdf == iommu->bdf )
+        if ( pdev->seg == iommu->seg && pdev->sbdf.bdf == iommu->bdf )
             return is_hardware_domain(pdev->domain) ? 0 : -ENODEV;
 
-    iommu = find_iommu_for_device(pdev->seg, bdf);
+    iommu = find_iommu_for_device(pdev->seg, pdev->sbdf.bdf);
     if ( unlikely(!iommu) )
     {
         /* Filter bridge devices. */
@@ -648,8 +647,7 @@ static int cf_check amd_iommu_remove_dev
     if ( !pdev->domain )
         return -EINVAL;
 
-    bdf = PCI_BDF2(pdev->bus, pdev->devfn);
-    iommu = find_iommu_for_device(pdev->seg, bdf);
+    iommu = find_iommu_for_device(pdev->seg, pdev->sbdf.bdf);
     if ( !iommu )
     {
         AMD_IOMMU_WARN("failed to find IOMMU: %pp cannot be removed from %pd\n",

[PATCH 6/8] IOMMU: log appropriate SBDF

[Jan Beulich]

To handle phantom devices, several functions are passed separate "devfn"
arguments besides a PCI device. In such cases we want to log the phantom
device's coordinates instead of the main one's. (Note that not all of
the instances being changed are fallout from the referenced commit.)

Fixes: 1ee1441835f4 ("print: introduce a format specifier for pci_sbdf_t")
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/passthrough/amd/iommu_cmd.c
+++ b/xen/drivers/passthrough/amd/iommu_cmd.c
@@ -291,7 +291,8 @@ void amd_iommu_flush_iotlb(u8 devfn, con
 
     if ( !iommu )
     {
-        AMD_IOMMU_WARN("can't find IOMMU for %pp\n", &pdev->sbdf);
+        AMD_IOMMU_WARN("can't find IOMMU for %pp\n",
+                       &PCI_SBDF3(pdev->seg, pdev->bus, devfn));
         return;
     }
 
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -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",
-                       &pdev->sbdf, target);
+                       &PCI_SBDF3(pdev->seg, pdev->bus, devfn), target);
         return -ENODEV;
     }
 
@@ -497,8 +497,8 @@ static int cf_check reassign_device(
             return rc;
     }
 
-    AMD_IOMMU_DEBUG("Re-assign %pp from dom%d to dom%d\n",
-                    &pdev->sbdf, source->domain_id, target->domain_id);
+    AMD_IOMMU_DEBUG("Re-assign %pp from %pd to %pd\n",
+                    &PCI_SBDF3(pdev->seg, pdev->bus, devfn), source, target);
 
     return 0;
 }
@@ -575,7 +575,7 @@ static int cf_check amd_iommu_add_device
         }
 
         AMD_IOMMU_WARN("no IOMMU for %pp; cannot be handed to %pd\n",
-                        &pdev->sbdf, pdev->domain);
+                        &PCI_SBDF3(pdev->seg, pdev->bus, devfn), pdev->domain);
         return -ENODEV;
     }
 
@@ -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, &pdev->sbdf);
+                       pdev->domain, &PCI_SBDF2(pdev->seg, bdf));
 
     if ( iommu_quarantine && pdev->arch.pseudo_domid == DOMID_INVALID )
     {
@@ -651,7 +651,7 @@ static int cf_check amd_iommu_remove_dev
     if ( !iommu )
     {
         AMD_IOMMU_WARN("failed to find IOMMU: %pp cannot be removed from %pd\n",
-                        &pdev->sbdf, pdev->domain);
+                        &PCI_SBDF3(pdev->seg, pdev->bus, devfn), pdev->domain);
         return -ENODEV;
     }
 
@@ -664,7 +664,7 @@ static int cf_check amd_iommu_remove_dev
              pdev->domain,
              ivrs_mappings[ivrs_mappings[bdf].dte_requestor_id].unity_map) )
         AMD_IOMMU_WARN("%pd: unity unmapping failed for %pp\n",
-                       pdev->domain, &pdev->sbdf);
+                       pdev->domain, &PCI_SBDF2(pdev->seg, bdf));
 
     amd_iommu_quarantine_teardown(pdev);
 
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -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",
-                   &pdev->sbdf, rc);
+                   &PCI_SBDF3(pdev->seg, pdev->bus, devfn), rc);
     }
 }
 
@@ -1451,7 +1451,8 @@ static int iommu_remove_device(struct pc
         if ( !rc )
             continue;
 
-        printk(XENLOG_ERR "IOMMU: remove %pp failed (%d)\n", &pdev->sbdf, rc);
+        printk(XENLOG_ERR "IOMMU: remove %pp failed (%d)\n",
+               &PCI_SBDF3(pdev->seg, pdev->bus, devfn), rc);
         return rc;
     }
 

[PATCH 7/8] 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>
---
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.

--- 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.df << 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 8/8] 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>

--- 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 1/8] IOMMU/x86: drop locking from quarantine_init() hooks

[Andrew Cooper]

On 11/04/2022 10:35, Jan Beulich wrote:
> Prior extension of these functions to enable per-device quarantine page
> tables already didn't add more locking there, but merely left in place
> what had been there before. But really locking is unnecessary here:
> We're running with pcidevs_lock held (i.e. multiple invocations of the
> same function [or their teardown equivalents] are impossible, and hence
> there are no "local" races), while all consuming of the data being
> populated here can't race anyway due to happening sequentially
> afterwards. See also the comment in struct arch_pci_dev.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

It is only legitimate to drop these calls if you delete the mapping_lock
entirely.  Otherwise you're breaking the semantics of mapping_lock.

Your argument of "well this is already guarded by the pci lock" means
that these are uncontended lock/unlock operations, and therefore not
interesting to drop in the first place.

This patch is specifically setting us up for an XSA in the future when
the behaviour of the the PCI lock changes, the fix for which will be
revert this patch.

~Andrew

Re: [PATCH 1/8] IOMMU/x86: drop locking from quarantine_init() hooks

[Jan Beulich]

On 11.04.2022 12:01, Andrew Cooper wrote:
> On 11/04/2022 10:35, Jan Beulich wrote:
>> Prior extension of these functions to enable per-device quarantine page
>> tables already didn't add more locking there, but merely left in place
>> what had been there before. But really locking is unnecessary here:
>> We're running with pcidevs_lock held (i.e. multiple invocations of the
>> same function [or their teardown equivalents] are impossible, and hence
>> there are no "local" races), while all consuming of the data being
>> populated here can't race anyway due to happening sequentially
>> afterwards. See also the comment in struct arch_pci_dev.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> It is only legitimate to drop these calls if you delete the mapping_lock
> entirely.  Otherwise you're breaking the semantics of mapping_lock.

Not for all domains other than DomIO. And DomIO is, well, special. As
is at what times quarantine_init() is actually being invoked.

> Your argument of "well this is already guarded by the pci lock" means
> that these are uncontended lock/unlock operations, and therefore not
> interesting to drop in the first place.
> 
> This patch is specifically setting us up for an XSA in the future when
> the behaviour of the the PCI lock changes, the fix for which will be
> revert this patch.

That wouldn't suffice then. As said in the description, and as discussed
during the development of the XSA-400 series, further locking would need
adding then.

Jan

Re: [PATCH 2/8] VT-d: drop ROOT_ENTRY_NR

[Roger Pau Monné]

On Mon, Apr 11, 2022 at 11:36:23AM +0200, Jan Beulich wrote:
> It's not only misplaced, but entirely unused.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

Re: [PATCH 3/8] VT-d: plug memory leaks in iommu_alloc()

[Roger Pau Monné]

On Mon, Apr 11, 2022 at 11:36:43AM +0200, Jan Beulich wrote:
> While 97af062b89d5 ("IOMMU/x86: maintain a per-device pseudo domain ID")
> took care of not making things worse, plugging pre-existing leaks wasn't
> the purpose of that change; they're not security relevant after all.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

Re: [PATCH 4/8] VT-d: refuse to use IOMMU with reserved CAP.ND value

[Roger Pau Monné]

On Mon, Apr 11, 2022 at 11:37:28AM +0200, Jan Beulich wrote:
> The field taking the value 7 (resulting in 18-bit DIDs when using the
> calculation in cap_ndoms(), when the DID fields are only 16 bits wide)
> is reserved. Instead of misbehaving in case we would encounter such an
> IOMMU, refuse to use it.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

I would maybe prefer to get more specific error message rather than
"IOMMU: unsupported" and a dump of the iommu registers.

Thanks, Roger.

Re: [PATCH 5/8] AMD/IOMMU: replace a few PCI_BDF2()

[Roger Pau Monné]

On Mon, Apr 11, 2022 at 11:37:52AM +0200, Jan Beulich wrote:
> struct pci_dev has the wanted value directly available; use it. Note
> that this fixes a - imo benign - mistake in reassign_device(): The unity
> map removal ought to be based on the passed in devfn (as is the case on
> the establishing side). This is benign because the mappings would be
> removed anyway a little later, when the "main" device gets processed.
> While there also limit the scope of two variables in that function.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

Re: [PATCH 6/8] IOMMU: log appropriate SBDF

[Roger Pau Monné]

On Mon, Apr 11, 2022 at 11:38:28AM +0200, Jan Beulich wrote:
> To handle phantom devices, several functions are passed separate "devfn"
> arguments besides a PCI device. In such cases we want to log the phantom
> device's coordinates instead of the main one's. (Note that not all of
> the instances being changed are fallout from the referenced commit.)
> 
> Fixes: 1ee1441835f4 ("print: introduce a format specifier for pci_sbdf_t")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

I'm unsure it matters much to have the logs from failures to find an
IOMMU using pdev->sbdf or devfn, as the parent device should have been
added before attempting to add any phantom functions, and hence would
have already failed to find an IOMMU.

> 
> --- a/xen/drivers/passthrough/amd/iommu_cmd.c
> +++ b/xen/drivers/passthrough/amd/iommu_cmd.c
> @@ -291,7 +291,8 @@ void amd_iommu_flush_iotlb(u8 devfn, con
>  
>      if ( !iommu )
>      {
> -        AMD_IOMMU_WARN("can't find IOMMU for %pp\n", &pdev->sbdf);
> +        AMD_IOMMU_WARN("can't find IOMMU for %pp\n",
> +                       &PCI_SBDF3(pdev->seg, pdev->bus, devfn));

Hm, the call to find_iommu_for_device() is explicitly using
pdev->devfn, so while the iommu of the phantom function is tied to
it's parent, it's unclear to me that logging the SBDF of the phantom
function will make this clearer for a user reading the logs.

>          return;
>      }
>  
> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> @@ -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",
> -                       &pdev->sbdf, target);
> +                       &PCI_SBDF3(pdev->seg, pdev->bus, devfn), target);

IIRC we would first reassign the parent, and then the phantom
functions, so we would always hit this error first with devfn ==
pdev->sbdf.bdf.

Thanks, Roger.

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

[Roger Pau Monné]

On Mon, Apr 11, 2022 at 11:40:24AM +0200, 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>

Thanks, Roger.

Re: [PATCH 4/8] VT-d: refuse to use IOMMU with reserved CAP.ND value

[Jan Beulich]

On 12.04.2022 11:22, Roger Pau Monné wrote:
> On Mon, Apr 11, 2022 at 11:37:28AM +0200, Jan Beulich wrote:
>> The field taking the value 7 (resulting in 18-bit DIDs when using the
>> calculation in cap_ndoms(), when the DID fields are only 16 bits wide)
>> is reserved. Instead of misbehaving in case we would encounter such an
>> IOMMU, refuse to use it.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

> I would maybe prefer to get more specific error message rather than
> "IOMMU: unsupported" and a dump of the iommu registers.

Perhaps, but this extends to other properties being checked as well then,
I would say, and hence may want to be the subject of yet another patch.

Jan

Re: [PATCH 6/8] IOMMU: log appropriate SBDF

[Jan Beulich]

On 12.04.2022 12:05, Roger Pau Monné wrote:
> On Mon, Apr 11, 2022 at 11:38:28AM +0200, Jan Beulich wrote:
>> To handle phantom devices, several functions are passed separate "devfn"
>> arguments besides a PCI device. In such cases we want to log the phantom
>> device's coordinates instead of the main one's. (Note that not all of
>> the instances being changed are fallout from the referenced commit.)
>>
>> Fixes: 1ee1441835f4 ("print: introduce a format specifier for pci_sbdf_t")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

> I'm unsure it matters much to have the logs from failures to find an
> IOMMU using pdev->sbdf or devfn, as the parent device should have been
> added before attempting to add any phantom functions, and hence would
> have already failed to find an IOMMU.

That's the expectation, unless something unexpected is going on. Hence
better have precise information in the log.

>> --- a/xen/drivers/passthrough/amd/iommu_cmd.c
>> +++ b/xen/drivers/passthrough/amd/iommu_cmd.c
>> @@ -291,7 +291,8 @@ void amd_iommu_flush_iotlb(u8 devfn, con
>>  
>>      if ( !iommu )
>>      {
>> -        AMD_IOMMU_WARN("can't find IOMMU for %pp\n", &pdev->sbdf);
>> +        AMD_IOMMU_WARN("can't find IOMMU for %pp\n",
>> +                       &PCI_SBDF3(pdev->seg, pdev->bus, devfn));
> 
> Hm, the call to find_iommu_for_device() is explicitly using
> pdev->devfn, so while the iommu of the phantom function is tied to
> it's parent, it's unclear to me that logging the SBDF of the phantom
> function will make this clearer for a user reading the logs.

The phantom function may not be possible to find an IOMMU for, so
using the base device for the lookup is unavoidable. For the message
here and ...

>> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
>> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
>> @@ -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",
>> -                       &pdev->sbdf, target);
>> +                       &PCI_SBDF3(pdev->seg, pdev->bus, devfn), target);
> 
> IIRC we would first reassign the parent, and then the phantom
> functions, so we would always hit this error first with devfn ==
> pdev->sbdf.bdf.

... here what I said further up applies.

Jan

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

[Roger Pau Monné]

On Mon, Apr 11, 2022 at 11:42:05AM +0200, 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>

Nice.

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

Re: [PATCH 1/8] IOMMU/x86: drop locking from quarantine_init() hooks

[Roger Pau Monné]

On Mon, Apr 11, 2022 at 11:35:54AM +0200, Jan Beulich wrote:
> Prior extension of these functions to enable per-device quarantine page
> tables already didn't add more locking there, but merely left in place
> what had been there before. But really locking is unnecessary here:
> We're running with pcidevs_lock held (i.e. multiple invocations of the
> same function [or their teardown equivalents] are impossible, and hence
> there are no "local" races), while all consuming of the data being
> populated here can't race anyway due to happening sequentially
> afterwards. See also the comment in struct arch_pci_dev.

I would explicitly say that none of the code in the locked region
touches any data in the domain_iommu struct, so taking the
mapping_lock is unneeded.

Long term we might wish to implemented a per-device lock that could be
used here, instead of relying on the pcidevs lock.

> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

Re: [PATCH 1/8] IOMMU/x86: drop locking from quarantine_init() hooks

[Jan Beulich]

On 12.04.2022 13:05, Roger Pau Monné wrote:
> On Mon, Apr 11, 2022 at 11:35:54AM +0200, Jan Beulich wrote:
>> Prior extension of these functions to enable per-device quarantine page
>> tables already didn't add more locking there, but merely left in place
>> what had been there before. But really locking is unnecessary here:
>> We're running with pcidevs_lock held (i.e. multiple invocations of the
>> same function [or their teardown equivalents] are impossible, and hence
>> there are no "local" races), while all consuming of the data being
>> populated here can't race anyway due to happening sequentially
>> afterwards. See also the comment in struct arch_pci_dev.
> 
> I would explicitly say that none of the code in the locked region
> touches any data in the domain_iommu struct, so taking the
> mapping_lock is unneeded.

But that would limit what the mapping_lock protects more than it actually
does: The entire page tables hanging off of the root table are also
protected by that lock. It's just that for a pdev, after having
installed identity mappings, the root doesn't hang off of hd. But in
principle - i.e. if the per-device mappings weren't static once created -
the lock would be the one to hold whenever any of these page tables was
modified.

> Long term we might wish to implemented a per-device lock that could be
> used here, instead of relying on the pcidevs lock.

Well, I would want to avoid this unless a need arises to not hold the
pcidevs lock here. Or, of course, if a need arose to dynamically alter
these page tables.

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

Thanks.

Jan

Re: [PATCH 1/8] IOMMU/x86: drop locking from quarantine_init() hooks

[Roger Pau Monné]

On Tue, Apr 12, 2022 at 02:17:16PM +0200, Jan Beulich wrote:
> On 12.04.2022 13:05, Roger Pau Monné wrote:
> > On Mon, Apr 11, 2022 at 11:35:54AM +0200, Jan Beulich wrote:
> >> Prior extension of these functions to enable per-device quarantine page
> >> tables already didn't add more locking there, but merely left in place
> >> what had been there before. But really locking is unnecessary here:
> >> We're running with pcidevs_lock held (i.e. multiple invocations of the
> >> same function [or their teardown equivalents] are impossible, and hence
> >> there are no "local" races), while all consuming of the data being
> >> populated here can't race anyway due to happening sequentially
> >> afterwards. See also the comment in struct arch_pci_dev.
> > 
> > I would explicitly say that none of the code in the locked region
> > touches any data in the domain_iommu struct, so taking the
> > mapping_lock is unneeded.
> 
> But that would limit what the mapping_lock protects more than it actually
> does: The entire page tables hanging off of the root table are also
> protected by that lock.

Right, but at the point where fill_qpt() gets called
hd->arch.amd.root_table == NULL, and hence it seems completely
pointless to wrap this in a mapping_lock locked region.

> It's just that for a pdev, after having
> installed identity mappings, the root doesn't hang off of hd. But in
> principle - i.e. if the per-device mappings weren't static once created -
> the lock would be the one to hold whenever any of these page tables was
> modified.

The lock would need to be held if pages tables are modified while
being set in hd->arch.amd.root_table, or at least that's my
understanding.

This is a special case anyway, as the page tables are not per-domain
but per-device, but it seems clear to me that if the page tables are
not set in hd->arch.amd.root_table the lock in hd->arch.mapping_lock
is not supposed to be protecting them.

> > Long term we might wish to implemented a per-device lock that could be
> > used here, instead of relying on the pcidevs lock.
> 
> Well, I would want to avoid this unless a need arises to not hold the
> pcidevs lock here. Or, of course, if a need arose to dynamically alter
> these page tables.

I think it's likely we will need such lock for other purposes if we
ever manage to convert the pcidevs lock into a rwlock, so my comment
was not so much as it's required for the use case here, but a side
effect if we ever manage to change pcidevs lock.

Thanks, Roger.

Re: [PATCH 1/8] IOMMU/x86: drop locking from quarantine_init() hooks

[Jan Beulich]

On 12.04.2022 14:54, Roger Pau Monné wrote:
> On Tue, Apr 12, 2022 at 02:17:16PM +0200, Jan Beulich wrote:
>> On 12.04.2022 13:05, Roger Pau Monné wrote:
>>> On Mon, Apr 11, 2022 at 11:35:54AM +0200, Jan Beulich wrote:
>>>> Prior extension of these functions to enable per-device quarantine page
>>>> tables already didn't add more locking there, but merely left in place
>>>> what had been there before. But really locking is unnecessary here:
>>>> We're running with pcidevs_lock held (i.e. multiple invocations of the
>>>> same function [or their teardown equivalents] are impossible, and hence
>>>> there are no "local" races), while all consuming of the data being
>>>> populated here can't race anyway due to happening sequentially
>>>> afterwards. See also the comment in struct arch_pci_dev.
>>>
>>> I would explicitly say that none of the code in the locked region
>>> touches any data in the domain_iommu struct, so taking the
>>> mapping_lock is unneeded.
>>
>> But that would limit what the mapping_lock protects more than it actually
>> does: The entire page tables hanging off of the root table are also
>> protected by that lock.
> 
> Right, but at the point where fill_qpt() gets called
> hd->arch.amd.root_table == NULL, and hence it seems completely
> pointless to wrap this in a mapping_lock locked region.
> 
>> It's just that for a pdev, after having
>> installed identity mappings, the root doesn't hang off of hd. But in
>> principle - i.e. if the per-device mappings weren't static once created -
>> the lock would be the one to hold whenever any of these page tables was
>> modified.
> 
> The lock would need to be held if pages tables are modified while
> being set in hd->arch.amd.root_table, or at least that's my
> understanding.
> 
> This is a special case anyway, as the page tables are not per-domain
> but per-device, but it seems clear to me that if the page tables are
> not set in hd->arch.amd.root_table the lock in hd->arch.mapping_lock
> is not supposed to be protecting them.

There are multiple models possible, one being that for per-device
page tables DomIO's lock protects all of them. Hence my hesitance to
say something along these lines in the description.

>>> Long term we might wish to implemented a per-device lock that could be
>>> used here, instead of relying on the pcidevs lock.
>>
>> Well, I would want to avoid this unless a need arises to not hold the
>> pcidevs lock here. Or, of course, if a need arose to dynamically alter
>> these page tables.
> 
> I think it's likely we will need such lock for other purposes if we
> ever manage to convert the pcidevs lock into a rwlock, so my comment
> was not so much as it's required for the use case here, but a side
> effect if we ever manage to change pcidevs lock.

Such a need would further depend on whether the code paths leading here
would hold the lock in read or write mode. But yes, it is reasonable to
expect that it would only be read mode.

Jan

Re: [PATCH 1/8] IOMMU/x86: drop locking from quarantine_init() hooks

[Jan Beulich]

On 11.04.2022 12:01, Andrew Cooper wrote:
> On 11/04/2022 10:35, Jan Beulich wrote:
>> Prior extension of these functions to enable per-device quarantine page
>> tables already didn't add more locking there, but merely left in place
>> what had been there before. But really locking is unnecessary here:
>> We're running with pcidevs_lock held (i.e. multiple invocations of the
>> same function [or their teardown equivalents] are impossible, and hence
>> there are no "local" races), while all consuming of the data being
>> populated here can't race anyway due to happening sequentially
>> afterwards. See also the comment in struct arch_pci_dev.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> It is only legitimate to drop these calls if you delete the mapping_lock
> entirely.  Otherwise you're breaking the semantics of mapping_lock.
> 
> Your argument of "well this is already guarded by the pci lock" means
> that these are uncontended lock/unlock operations, and therefore not
> interesting to drop in the first place.
> 
> This patch is specifically setting us up for an XSA in the future when
> the behaviour of the the PCI lock changes, the fix for which will be
> revert this patch.

Further to my earlier reply, may I remind you that changes to the PCI
lock won't go unnoticed here, as there are respective ASSERT()s in
place.

Jan

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

[Bertrand Marquis]

Hi Jan,

> On 11 Apr 2022, at 10:40, 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>
> ---
> 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.
> 
> --- 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.df << devfn_shift) + where;

I think this should be sbdf.bdf instead (typo you removed the b).

Cheers
Bertrand

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

[Jan Beulich]

On 13.04.2022 15:48, Bertrand Marquis wrote:
>> On 11 Apr 2022, at 10:40, Jan Beulich <jbeulich@suse.com> wrote:
>> --- 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.df << devfn_shift) + where;
> 
> I think this should be sbdf.bdf instead (typo you removed the b).

I don't think so, no - the transformation is to remove the PCI_DEVFN2(),
which was another way to drop the bus part of the coordinates. Patch
context also shows that the bus part if taken care of by other means.

Jan

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

[Roger Pau Monné]

On Wed, Apr 13, 2022 at 01:48:14PM +0000, Bertrand Marquis wrote:
> Hi Jan,
> 
> > On 11 Apr 2022, at 10:40, 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>
> > ---
> > 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.
> > 
> > --- 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.df << devfn_shift) + where;
> 
> I think this should be sbdf.bdf instead (typo you removed the b).

I don't think so, notice PCI_DEVFN2(sbdf.bdf) which is extracting the
devfn from sbdf.bdf. That's not needed, as you can just get the devfn
directly from sbdf.df.

Or else the original code is wrong, and the PCI_DEVFN2 shouldn't be
there.

Thanks, Roger.

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

[Bertrand Marquis]

Hi,

> On 13 Apr 2022, at 14:58, Roger Pau Monné <roger.pau@citrix.com> wrote:
> 
> On Wed, Apr 13, 2022 at 01:48:14PM +0000, Bertrand Marquis wrote:
>> Hi Jan,
>> 
>>> On 11 Apr 2022, at 10:40, 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>
>>> ---
>>> 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.
>>> 
>>> --- 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.df << devfn_shift) + where;
>> 
>> I think this should be sbdf.bdf instead (typo you removed the b).
> 
> I don't think so, notice PCI_DEVFN2(sbdf.bdf) which is extracting the
> devfn from sbdf.bdf. That's not needed, as you can just get the devfn
> directly from sbdf.df.
> 
> Or else the original code is wrong, and the PCI_DEVFN2 shouldn't be
> there.

There is not df field in the sbdf structure so it should be devfn instead.

Cheers
Bertrand

> 
> Thanks, Roger.

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

[Jan Beulich]

On 13.04.2022 16:13, Bertrand Marquis wrote:
>> On 13 Apr 2022, at 14:58, Roger Pau Monné <roger.pau@citrix.com> wrote:
>> On Wed, Apr 13, 2022 at 01:48:14PM +0000, Bertrand Marquis wrote:
>>>> On 11 Apr 2022, at 10:40, 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>
>>>> ---
>>>> 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.

Note this remark.

>>>> --- 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.df << devfn_shift) + where;
>>>
>>> I think this should be sbdf.bdf instead (typo you removed the b).
>>
>> I don't think so, notice PCI_DEVFN2(sbdf.bdf) which is extracting the
>> devfn from sbdf.bdf. That's not needed, as you can just get the devfn
>> directly from sbdf.df.
>>
>> Or else the original code is wrong, and the PCI_DEVFN2 shouldn't be
>> there.
> 
> There is not df field in the sbdf structure so it should be devfn instead.

Yes indeed, thanks for noticing. But really (see the remark further up)
this is what happens if code in the tree can't even be built-tested.

Jan

RE: [PATCH 1/8] IOMMU/x86: drop locking from quarantine_init() hooks

[Tian, Kevin]

> From: Jan Beulich <jbeulich@suse.com>
> Sent: Monday, April 11, 2022 5:36 PM
> 
> Prior extension of these functions to enable per-device quarantine page
> tables already didn't add more locking there, but merely left in place
> what had been there before. But really locking is unnecessary here:
> We're running with pcidevs_lock held (i.e. multiple invocations of the
> same function [or their teardown equivalents] are impossible, and hence
> there are no "local" races), while all consuming of the data being
> populated here can't race anyway due to happening sequentially
> afterwards. See also the comment in struct arch_pci_dev.

Better add some explanation (as you explained in other replies)
why above rationale around pcidevs_lock leads to the drop of
a different lock (mapping_lock).

> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

RE: [PATCH 2/8] VT-d: drop ROOT_ENTRY_NR

[Tian, Kevin]

> From: Jan Beulich <jbeulich@suse.com>
> Sent: Monday, April 11, 2022 5:36 PM
> 
> It's not only misplaced, but entirely unused.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

> 
> --- a/xen/drivers/passthrough/vtd/iommu.h
> +++ b/xen/drivers/passthrough/vtd/iommu.h
> @@ -204,7 +204,6 @@ struct context_entry {
>          __uint128_t full;
>      };
>  };
> -#define ROOT_ENTRY_NR (PAGE_SIZE_4K/sizeof(struct root_entry))
>  #define context_present(c) ((c).lo & 1)
>  #define context_fault_disable(c) (((c).lo >> 1) & 1)
>  #define context_translation_type(c) (((c).lo >> 2) & 3)

RE: [PATCH 3/8] VT-d: plug memory leaks in iommu_alloc()

[Tian, Kevin]

> From: Jan Beulich <jbeulich@suse.com>
> Sent: Monday, April 11, 2022 5:37 PM
> 
> While 97af062b89d5 ("IOMMU/x86: maintain a per-device pseudo domain
> ID")
> took care of not making things worse, plugging pre-existing leaks wasn't
> the purpose of that change; they're not security relevant after all.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

> 
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1238,8 +1238,9 @@ int __init iommu_alloc(struct acpi_drhd_
>      drhd->iommu = iommu;
> 
>      iommu->reg = ioremap(drhd->address, PAGE_SIZE);
> +    rc = -ENOMEM;
>      if ( !iommu->reg )
> -        return -ENOMEM;
> +        goto free;
>      iommu->index = nr_iommus++;
> 
>      iommu->cap = dmar_readq(iommu->reg, DMAR_CAP_REG);
> @@ -1260,8 +1261,9 @@ int __init iommu_alloc(struct acpi_drhd_
>          printk(VTDPREFIX "cap = %"PRIx64" ecap = %"PRIx64"\n",
>                 iommu->cap, iommu->ecap);
>      }
> +    rc = -ENODEV;
>      if ( !(iommu->cap + 1) || !(iommu->ecap + 1) )
> -        return -ENODEV;
> +        goto free;
> 
>      quirk_iommu_caps(iommu);
> 
> @@ -1272,7 +1274,8 @@ int __init iommu_alloc(struct acpi_drhd_
>      {
>          printk(XENLOG_ERR VTDPREFIX "IOMMU: unsupported\n");
>          print_iommu_regs(drhd);
> -        return -ENODEV;
> +        rc = -ENODEV;
> +        goto free;
>      }
> 
>      /* Calculate number of pagetable levels: 3 or 4. */
> @@ -1283,7 +1286,8 @@ int __init iommu_alloc(struct acpi_drhd_
>      {
>          printk(XENLOG_ERR VTDPREFIX "IOMMU: unsupported sagaw %x\n",
> sagaw);
>          print_iommu_regs(drhd);
> -        return -ENODEV;
> +        rc = -ENODEV;
> +        goto free;
>      }
>      iommu->nr_pt_levels = agaw_to_level(agaw);
> 
> @@ -1298,8 +1302,9 @@ int __init iommu_alloc(struct acpi_drhd_
>          iommu->domid_bitmap = xzalloc_array(unsigned long,
>                                              BITS_TO_LONGS(nr_dom));
>          iommu->domid_map = xzalloc_array(domid_t, nr_dom);
> +        rc = -ENOMEM;
>          if ( !iommu->domid_bitmap || !iommu->domid_map )
> -            return -ENOMEM;
> +            goto free;
> 
>          /*
>           * If Caching mode is set, then invalid translations are tagged

RE: [PATCH 4/8] VT-d: refuse to use IOMMU with reserved CAP.ND value

[Tian, Kevin]

> From: Jan Beulich <jbeulich@suse.com>
> Sent: Monday, April 11, 2022 5:37 PM
> 
> The field taking the value 7 (resulting in 18-bit DIDs when using the
> calculation in cap_ndoms(), when the DID fields are only 16 bits wide)
> is reserved. Instead of misbehaving in case we would encounter such an
> IOMMU, refuse to use it.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

> 
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1267,8 +1267,11 @@ int __init iommu_alloc(struct acpi_drhd_
> 
>      quirk_iommu_caps(iommu);
> 
> +    nr_dom = cap_ndoms(iommu->cap);
> +
>      if ( cap_fault_reg_offset(iommu->cap) +
>           cap_num_fault_regs(iommu->cap) * PRIMARY_FAULT_REG_LEN >
> PAGE_SIZE ||
> +         ((nr_dom - 1) >> 16) /* I.e. cap.nd > 6 */ ||
>           (has_register_based_invalidation(iommu) &&
>            ecap_iotlb_offset(iommu->ecap) >= PAGE_SIZE) )
>      {
> @@ -1294,8 +1297,6 @@ int __init iommu_alloc(struct acpi_drhd_
>      if ( !ecap_coherent(iommu->ecap) )
>          iommu_non_coherent = true;
> 
> -    nr_dom = cap_ndoms(iommu->cap);
> -
>      if ( nr_dom <= DOMID_MASK * 2 + cap_caching_mode(iommu->cap) )
>      {
>          /* Allocate domain id (bit) maps. */

RE: [PATCH 7/8] PCI: replace stray uses of PCI_{DEVFN,BDF}2()

[Tian, Kevin]

> From: Jan Beulich <jbeulich@suse.com>
> Sent: Monday, April 11, 2022 5:40 PM
> 
> 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: 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.
> 
> --- 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.df << 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 8/8] PCI: replace "secondary" flavors of PCI_{DEVFN,BDF,SBDF}()

[Tian, Kevin]

> From: Jan Beulich <jbeulich@suse.com>
> Sent: Monday, April 11, 2022 5:42 PM
> 
> 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: 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)
>