* [PATCH 0/8] IOMMU: assorted follow-on to XSA-400 @ 2022-04-11 9:34 Jan Beulich 2022-04-11 9:35 ` [PATCH 1/8] IOMMU/x86: drop locking from quarantine_init() hooks Jan Beulich ` (7 more replies) 0 siblings, 8 replies; 36+ messages in thread From: Jan Beulich @ 2022-04-11 9:34 UTC (permalink / raw) To: xen-devel; +Cc: Kevin Tian, Paul Durrant, Andrew Cooper, Roger Pau Monné 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 1/8] IOMMU/x86: drop locking from quarantine_init() hooks 2022-04-11 9:34 [PATCH 0/8] IOMMU: assorted follow-on to XSA-400 Jan Beulich @ 2022-04-11 9:35 ` Jan Beulich 2022-04-11 10:01 ` Andrew Cooper ` (2 more replies) 2022-04-11 9:36 ` [PATCH 2/8] VT-d: drop ROOT_ENTRY_NR Jan Beulich ` (6 subsequent siblings) 7 siblings, 3 replies; 36+ messages in thread From: Jan Beulich @ 2022-04-11 9:35 UTC (permalink / raw) To: xen-devel; +Cc: Kevin Tian, Paul Durrant, Andrew Cooper, Roger Pau Monné 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); ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 1/8] IOMMU/x86: drop locking from quarantine_init() hooks 2022-04-11 9:35 ` [PATCH 1/8] IOMMU/x86: drop locking from quarantine_init() hooks Jan Beulich @ 2022-04-11 10:01 ` Andrew Cooper 2022-04-11 10:18 ` Jan Beulich 2022-04-12 13:14 ` Jan Beulich 2022-04-12 11:05 ` Roger Pau Monné 2022-04-20 6:22 ` Tian, Kevin 2 siblings, 2 replies; 36+ messages in thread From: Andrew Cooper @ 2022-04-11 10:01 UTC (permalink / raw) To: Jan Beulich, xen-devel; +Cc: Kevin Tian, Paul Durrant, Roger Pau Monne 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 1/8] IOMMU/x86: drop locking from quarantine_init() hooks 2022-04-11 10:01 ` Andrew Cooper @ 2022-04-11 10:18 ` Jan Beulich 2022-04-12 13:14 ` Jan Beulich 1 sibling, 0 replies; 36+ messages in thread From: Jan Beulich @ 2022-04-11 10:18 UTC (permalink / raw) To: Andrew Cooper; +Cc: Kevin Tian, Paul Durrant, Roger Pau Monne, xen-devel 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 1/8] IOMMU/x86: drop locking from quarantine_init() hooks 2022-04-11 10:01 ` Andrew Cooper 2022-04-11 10:18 ` Jan Beulich @ 2022-04-12 13:14 ` Jan Beulich 1 sibling, 0 replies; 36+ messages in thread From: Jan Beulich @ 2022-04-12 13:14 UTC (permalink / raw) To: Andrew Cooper; +Cc: Kevin Tian, Paul Durrant, Roger Pau Monne, xen-devel 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 1/8] IOMMU/x86: drop locking from quarantine_init() hooks 2022-04-11 9:35 ` [PATCH 1/8] IOMMU/x86: drop locking from quarantine_init() hooks Jan Beulich 2022-04-11 10:01 ` Andrew Cooper @ 2022-04-12 11:05 ` Roger Pau Monné 2022-04-12 12:17 ` Jan Beulich 2022-04-20 6:22 ` Tian, Kevin 2 siblings, 1 reply; 36+ messages in thread From: Roger Pau Monné @ 2022-04-12 11:05 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Kevin Tian, Paul Durrant, Andrew Cooper 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 1/8] IOMMU/x86: drop locking from quarantine_init() hooks 2022-04-12 11:05 ` Roger Pau Monné @ 2022-04-12 12:17 ` Jan Beulich 2022-04-12 12:54 ` Roger Pau Monné 0 siblings, 1 reply; 36+ messages in thread From: Jan Beulich @ 2022-04-12 12:17 UTC (permalink / raw) To: Roger Pau Monné; +Cc: xen-devel, Kevin Tian, Paul Durrant, Andrew Cooper 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 1/8] IOMMU/x86: drop locking from quarantine_init() hooks 2022-04-12 12:17 ` Jan Beulich @ 2022-04-12 12:54 ` Roger Pau Monné 2022-04-12 13:12 ` Jan Beulich 0 siblings, 1 reply; 36+ messages in thread From: Roger Pau Monné @ 2022-04-12 12:54 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Kevin Tian, Paul Durrant, Andrew Cooper 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 1/8] IOMMU/x86: drop locking from quarantine_init() hooks 2022-04-12 12:54 ` Roger Pau Monné @ 2022-04-12 13:12 ` Jan Beulich 0 siblings, 0 replies; 36+ messages in thread From: Jan Beulich @ 2022-04-12 13:12 UTC (permalink / raw) To: Roger Pau Monné; +Cc: xen-devel, Kevin Tian, Paul Durrant, Andrew Cooper 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* RE: [PATCH 1/8] IOMMU/x86: drop locking from quarantine_init() hooks 2022-04-11 9:35 ` [PATCH 1/8] IOMMU/x86: drop locking from quarantine_init() hooks Jan Beulich 2022-04-11 10:01 ` Andrew Cooper 2022-04-12 11:05 ` Roger Pau Monné @ 2022-04-20 6:22 ` Tian, Kevin 2 siblings, 0 replies; 36+ messages in thread From: Tian, Kevin @ 2022-04-20 6:22 UTC (permalink / raw) To: Beulich, Jan, xen-devel Cc: Paul Durrant, Cooper, Andrew, Pau Monné, Roger > 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> ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 2/8] VT-d: drop ROOT_ENTRY_NR 2022-04-11 9:34 [PATCH 0/8] IOMMU: assorted follow-on to XSA-400 Jan Beulich 2022-04-11 9:35 ` [PATCH 1/8] IOMMU/x86: drop locking from quarantine_init() hooks Jan Beulich @ 2022-04-11 9:36 ` Jan Beulich 2022-04-12 8:20 ` Roger Pau Monné 2022-04-20 6:22 ` Tian, Kevin 2022-04-11 9:36 ` [PATCH 3/8] VT-d: plug memory leaks in iommu_alloc() Jan Beulich ` (5 subsequent siblings) 7 siblings, 2 replies; 36+ messages in thread From: Jan Beulich @ 2022-04-11 9:36 UTC (permalink / raw) To: xen-devel; +Cc: Kevin Tian, Paul Durrant, Roger Pau Monné 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) ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 2/8] VT-d: drop ROOT_ENTRY_NR 2022-04-11 9:36 ` [PATCH 2/8] VT-d: drop ROOT_ENTRY_NR Jan Beulich @ 2022-04-12 8:20 ` Roger Pau Monné 2022-04-20 6:22 ` Tian, Kevin 1 sibling, 0 replies; 36+ messages in thread From: Roger Pau Monné @ 2022-04-12 8:20 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Kevin Tian, Paul Durrant 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* RE: [PATCH 2/8] VT-d: drop ROOT_ENTRY_NR 2022-04-11 9:36 ` [PATCH 2/8] VT-d: drop ROOT_ENTRY_NR Jan Beulich 2022-04-12 8:20 ` Roger Pau Monné @ 2022-04-20 6:22 ` Tian, Kevin 1 sibling, 0 replies; 36+ messages in thread From: Tian, Kevin @ 2022-04-20 6:22 UTC (permalink / raw) To: Beulich, Jan, xen-devel; +Cc: Paul Durrant, Pau Monné, Roger > 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) ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 3/8] VT-d: plug memory leaks in iommu_alloc() 2022-04-11 9:34 [PATCH 0/8] IOMMU: assorted follow-on to XSA-400 Jan Beulich 2022-04-11 9:35 ` [PATCH 1/8] IOMMU/x86: drop locking from quarantine_init() hooks Jan Beulich 2022-04-11 9:36 ` [PATCH 2/8] VT-d: drop ROOT_ENTRY_NR Jan Beulich @ 2022-04-11 9:36 ` Jan Beulich 2022-04-12 8:29 ` Roger Pau Monné 2022-04-20 6:23 ` Tian, Kevin 2022-04-11 9:37 ` [PATCH 4/8] VT-d: refuse to use IOMMU with reserved CAP.ND value Jan Beulich ` (4 subsequent siblings) 7 siblings, 2 replies; 36+ messages in thread From: Jan Beulich @ 2022-04-11 9:36 UTC (permalink / raw) To: xen-devel; +Cc: Kevin Tian, Paul Durrant, Roger Pau Monné 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/8] VT-d: plug memory leaks in iommu_alloc() 2022-04-11 9:36 ` [PATCH 3/8] VT-d: plug memory leaks in iommu_alloc() Jan Beulich @ 2022-04-12 8:29 ` Roger Pau Monné 2022-04-20 6:23 ` Tian, Kevin 1 sibling, 0 replies; 36+ messages in thread From: Roger Pau Monné @ 2022-04-12 8:29 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Kevin Tian, Paul Durrant 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* RE: [PATCH 3/8] VT-d: plug memory leaks in iommu_alloc() 2022-04-11 9:36 ` [PATCH 3/8] VT-d: plug memory leaks in iommu_alloc() Jan Beulich 2022-04-12 8:29 ` Roger Pau Monné @ 2022-04-20 6:23 ` Tian, Kevin 1 sibling, 0 replies; 36+ messages in thread From: Tian, Kevin @ 2022-04-20 6:23 UTC (permalink / raw) To: Beulich, Jan, xen-devel; +Cc: Paul Durrant, Pau Monné, Roger > 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 4/8] VT-d: refuse to use IOMMU with reserved CAP.ND value 2022-04-11 9:34 [PATCH 0/8] IOMMU: assorted follow-on to XSA-400 Jan Beulich ` (2 preceding siblings ...) 2022-04-11 9:36 ` [PATCH 3/8] VT-d: plug memory leaks in iommu_alloc() Jan Beulich @ 2022-04-11 9:37 ` Jan Beulich 2022-04-12 9:22 ` Roger Pau Monné 2022-04-20 6:23 ` Tian, Kevin 2022-04-11 9:37 ` [PATCH 5/8] AMD/IOMMU: replace a few PCI_BDF2() Jan Beulich ` (3 subsequent siblings) 7 siblings, 2 replies; 36+ messages in thread From: Jan Beulich @ 2022-04-11 9:37 UTC (permalink / raw) To: xen-devel; +Cc: Kevin Tian, Paul Durrant, Roger Pau Monné 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. */ ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 4/8] VT-d: refuse to use IOMMU with reserved CAP.ND value 2022-04-11 9:37 ` [PATCH 4/8] VT-d: refuse to use IOMMU with reserved CAP.ND value Jan Beulich @ 2022-04-12 9:22 ` Roger Pau Monné 2022-04-12 10:35 ` Jan Beulich 2022-04-20 6:23 ` Tian, Kevin 1 sibling, 1 reply; 36+ messages in thread From: Roger Pau Monné @ 2022-04-12 9:22 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Kevin Tian, Paul Durrant 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 4/8] VT-d: refuse to use IOMMU with reserved CAP.ND value 2022-04-12 9:22 ` Roger Pau Monné @ 2022-04-12 10:35 ` Jan Beulich 0 siblings, 0 replies; 36+ messages in thread From: Jan Beulich @ 2022-04-12 10:35 UTC (permalink / raw) To: Roger Pau Monné; +Cc: xen-devel, Kevin Tian, Paul Durrant 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* RE: [PATCH 4/8] VT-d: refuse to use IOMMU with reserved CAP.ND value 2022-04-11 9:37 ` [PATCH 4/8] VT-d: refuse to use IOMMU with reserved CAP.ND value Jan Beulich 2022-04-12 9:22 ` Roger Pau Monné @ 2022-04-20 6:23 ` Tian, Kevin 1 sibling, 0 replies; 36+ messages in thread From: Tian, Kevin @ 2022-04-20 6:23 UTC (permalink / raw) To: Beulich, Jan, xen-devel; +Cc: Paul Durrant, Pau Monné, Roger > 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. */ ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 5/8] AMD/IOMMU: replace a few PCI_BDF2() 2022-04-11 9:34 [PATCH 0/8] IOMMU: assorted follow-on to XSA-400 Jan Beulich ` (3 preceding siblings ...) 2022-04-11 9:37 ` [PATCH 4/8] VT-d: refuse to use IOMMU with reserved CAP.ND value Jan Beulich @ 2022-04-11 9:37 ` Jan Beulich 2022-04-12 9:37 ` Roger Pau Monné 2022-04-11 9:38 ` [PATCH 6/8] IOMMU: log appropriate SBDF Jan Beulich ` (2 subsequent siblings) 7 siblings, 1 reply; 36+ messages in thread From: Jan Beulich @ 2022-04-11 9:37 UTC (permalink / raw) To: xen-devel; +Cc: Paul Durrant, Andrew Cooper, Roger Pau Monné 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", ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 5/8] AMD/IOMMU: replace a few PCI_BDF2() 2022-04-11 9:37 ` [PATCH 5/8] AMD/IOMMU: replace a few PCI_BDF2() Jan Beulich @ 2022-04-12 9:37 ` Roger Pau Monné 0 siblings, 0 replies; 36+ messages in thread From: Roger Pau Monné @ 2022-04-12 9:37 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Paul Durrant, Andrew Cooper 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 6/8] IOMMU: log appropriate SBDF 2022-04-11 9:34 [PATCH 0/8] IOMMU: assorted follow-on to XSA-400 Jan Beulich ` (4 preceding siblings ...) 2022-04-11 9:37 ` [PATCH 5/8] AMD/IOMMU: replace a few PCI_BDF2() Jan Beulich @ 2022-04-11 9:38 ` Jan Beulich 2022-04-12 10:05 ` Roger Pau Monné 2022-04-11 9:40 ` [PATCH 7/8] PCI: replace stray uses of PCI_{DEVFN,BDF}2() Jan Beulich 2022-04-11 9:42 ` [PATCH 8/8] PCI: replace "secondary" flavors of PCI_{DEVFN,BDF,SBDF}() Jan Beulich 7 siblings, 1 reply; 36+ messages in thread From: Jan Beulich @ 2022-04-11 9:38 UTC (permalink / raw) To: xen-devel; +Cc: Paul Durrant, Andrew Cooper, Roger Pau Monné 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; } ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 6/8] IOMMU: log appropriate SBDF 2022-04-11 9:38 ` [PATCH 6/8] IOMMU: log appropriate SBDF Jan Beulich @ 2022-04-12 10:05 ` Roger Pau Monné 2022-04-12 10:39 ` Jan Beulich 0 siblings, 1 reply; 36+ messages in thread From: Roger Pau Monné @ 2022-04-12 10:05 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Paul Durrant, Andrew Cooper 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 6/8] IOMMU: log appropriate SBDF 2022-04-12 10:05 ` Roger Pau Monné @ 2022-04-12 10:39 ` Jan Beulich 0 siblings, 0 replies; 36+ messages in thread From: Jan Beulich @ 2022-04-12 10:39 UTC (permalink / raw) To: Roger Pau Monné; +Cc: xen-devel, Paul Durrant, Andrew Cooper 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 7/8] PCI: replace stray uses of PCI_{DEVFN,BDF}2() 2022-04-11 9:34 [PATCH 0/8] IOMMU: assorted follow-on to XSA-400 Jan Beulich ` (5 preceding siblings ...) 2022-04-11 9:38 ` [PATCH 6/8] IOMMU: log appropriate SBDF Jan Beulich @ 2022-04-11 9:40 ` Jan Beulich 2022-04-12 10:07 ` Roger Pau Monné ` (2 more replies) 2022-04-11 9:42 ` [PATCH 8/8] PCI: replace "secondary" flavors of PCI_{DEVFN,BDF,SBDF}() Jan Beulich 7 siblings, 3 replies; 36+ messages in thread From: Jan Beulich @ 2022-04-11 9:40 UTC (permalink / raw) To: xen-devel Cc: Kevin Tian, Paul Durrant, Julien Grall, Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis, Andrew Cooper, Wei Liu, Roger Pau Monné 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; ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 7/8] PCI: replace stray uses of PCI_{DEVFN,BDF}2() 2022-04-11 9:40 ` [PATCH 7/8] PCI: replace stray uses of PCI_{DEVFN,BDF}2() Jan Beulich @ 2022-04-12 10:07 ` Roger Pau Monné 2022-04-13 13:48 ` Bertrand Marquis 2022-04-20 6:29 ` Tian, Kevin 2 siblings, 0 replies; 36+ messages in thread From: Roger Pau Monné @ 2022-04-12 10:07 UTC (permalink / raw) To: Jan Beulich Cc: xen-devel, Kevin Tian, Paul Durrant, Julien Grall, Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis, Andrew Cooper, Wei Liu 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 7/8] PCI: replace stray uses of PCI_{DEVFN,BDF}2() 2022-04-11 9:40 ` [PATCH 7/8] PCI: replace stray uses of PCI_{DEVFN,BDF}2() Jan Beulich 2022-04-12 10:07 ` Roger Pau Monné @ 2022-04-13 13:48 ` Bertrand Marquis 2022-04-13 13:55 ` Jan Beulich 2022-04-13 13:58 ` Roger Pau Monné 2022-04-20 6:29 ` Tian, Kevin 2 siblings, 2 replies; 36+ messages in thread From: Bertrand Marquis @ 2022-04-13 13:48 UTC (permalink / raw) To: Jan Beulich Cc: xen-devel, Kevin Tian, Paul Durrant, Julien Grall, Stefano Stabellini, Volodymyr Babchuk, Andrew Cooper, Wei Liu, Roger Pau Monné 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 7/8] PCI: replace stray uses of PCI_{DEVFN,BDF}2() 2022-04-13 13:48 ` Bertrand Marquis @ 2022-04-13 13:55 ` Jan Beulich 2022-04-13 13:58 ` Roger Pau Monné 1 sibling, 0 replies; 36+ messages in thread From: Jan Beulich @ 2022-04-13 13:55 UTC (permalink / raw) To: Bertrand Marquis Cc: xen-devel, Kevin Tian, Paul Durrant, Julien Grall, Stefano Stabellini, Volodymyr Babchuk, Andrew Cooper, Wei Liu, Roger Pau Monné 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 7/8] PCI: replace stray uses of PCI_{DEVFN,BDF}2() 2022-04-13 13:48 ` Bertrand Marquis 2022-04-13 13:55 ` Jan Beulich @ 2022-04-13 13:58 ` Roger Pau Monné 2022-04-13 14:13 ` Bertrand Marquis 1 sibling, 1 reply; 36+ messages in thread From: Roger Pau Monné @ 2022-04-13 13:58 UTC (permalink / raw) To: Bertrand Marquis Cc: Jan Beulich, xen-devel, Kevin Tian, Paul Durrant, Julien Grall, Stefano Stabellini, Volodymyr Babchuk, Andrew Cooper, Wei Liu 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 7/8] PCI: replace stray uses of PCI_{DEVFN,BDF}2() 2022-04-13 13:58 ` Roger Pau Monné @ 2022-04-13 14:13 ` Bertrand Marquis 2022-04-13 14:38 ` Jan Beulich 0 siblings, 1 reply; 36+ messages in thread From: Bertrand Marquis @ 2022-04-13 14:13 UTC (permalink / raw) To: Roger Pau Monné Cc: Jan Beulich, xen-devel, Kevin Tian, Paul Durrant, Julien Grall, Stefano Stabellini, Volodymyr Babchuk, Andrew Cooper, Wei Liu 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 7/8] PCI: replace stray uses of PCI_{DEVFN,BDF}2() 2022-04-13 14:13 ` Bertrand Marquis @ 2022-04-13 14:38 ` Jan Beulich 0 siblings, 0 replies; 36+ messages in thread From: Jan Beulich @ 2022-04-13 14:38 UTC (permalink / raw) To: Bertrand Marquis Cc: xen-devel, Kevin Tian, Paul Durrant, Julien Grall, Stefano Stabellini, Volodymyr Babchuk, Andrew Cooper, Wei Liu, Roger Pau Monné 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* RE: [PATCH 7/8] PCI: replace stray uses of PCI_{DEVFN,BDF}2() 2022-04-11 9:40 ` [PATCH 7/8] PCI: replace stray uses of PCI_{DEVFN,BDF}2() Jan Beulich 2022-04-12 10:07 ` Roger Pau Monné 2022-04-13 13:48 ` Bertrand Marquis @ 2022-04-20 6:29 ` Tian, Kevin 2 siblings, 0 replies; 36+ messages in thread From: Tian, Kevin @ 2022-04-20 6:29 UTC (permalink / raw) To: Beulich, Jan, xen-devel Cc: Paul Durrant, Julien Grall, Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis, Cooper, Andrew, Wei Liu, Pau Monné, Roger > 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; ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 8/8] PCI: replace "secondary" flavors of PCI_{DEVFN,BDF,SBDF}() 2022-04-11 9:34 [PATCH 0/8] IOMMU: assorted follow-on to XSA-400 Jan Beulich ` (6 preceding siblings ...) 2022-04-11 9:40 ` [PATCH 7/8] PCI: replace stray uses of PCI_{DEVFN,BDF}2() Jan Beulich @ 2022-04-11 9:42 ` Jan Beulich 2022-04-12 10:49 ` Roger Pau Monné 2022-04-20 6:37 ` Tian, Kevin 7 siblings, 2 replies; 36+ messages in thread From: Jan Beulich @ 2022-04-11 9:42 UTC (permalink / raw) To: xen-devel Cc: Kevin Tian, Roger Pau Monné, Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini, Wei Liu 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) ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 8/8] PCI: replace "secondary" flavors of PCI_{DEVFN,BDF,SBDF}() 2022-04-11 9:42 ` [PATCH 8/8] PCI: replace "secondary" flavors of PCI_{DEVFN,BDF,SBDF}() Jan Beulich @ 2022-04-12 10:49 ` Roger Pau Monné 2022-04-20 6:37 ` Tian, Kevin 1 sibling, 0 replies; 36+ messages in thread From: Roger Pau Monné @ 2022-04-12 10:49 UTC (permalink / raw) To: Jan Beulich Cc: xen-devel, Kevin Tian, Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini, Wei Liu 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* RE: [PATCH 8/8] PCI: replace "secondary" flavors of PCI_{DEVFN,BDF,SBDF}() 2022-04-11 9:42 ` [PATCH 8/8] PCI: replace "secondary" flavors of PCI_{DEVFN,BDF,SBDF}() Jan Beulich 2022-04-12 10:49 ` Roger Pau Monné @ 2022-04-20 6:37 ` Tian, Kevin 1 sibling, 0 replies; 36+ messages in thread From: Tian, Kevin @ 2022-04-20 6:37 UTC (permalink / raw) To: Beulich, Jan, xen-devel Cc: Pau Monné, Roger, Cooper, Andrew, George Dunlap, Julien Grall, Stefano Stabellini, Wei Liu > 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) > ^ permalink raw reply [flat|nested] 36+ messages in thread
end of thread, other threads:[~2022-04-20 6:37 UTC | newest] Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-04-11 9:34 [PATCH 0/8] IOMMU: assorted follow-on to XSA-400 Jan Beulich 2022-04-11 9:35 ` [PATCH 1/8] IOMMU/x86: drop locking from quarantine_init() hooks Jan Beulich 2022-04-11 10:01 ` Andrew Cooper 2022-04-11 10:18 ` Jan Beulich 2022-04-12 13:14 ` Jan Beulich 2022-04-12 11:05 ` Roger Pau Monné 2022-04-12 12:17 ` Jan Beulich 2022-04-12 12:54 ` Roger Pau Monné 2022-04-12 13:12 ` Jan Beulich 2022-04-20 6:22 ` Tian, Kevin 2022-04-11 9:36 ` [PATCH 2/8] VT-d: drop ROOT_ENTRY_NR Jan Beulich 2022-04-12 8:20 ` Roger Pau Monné 2022-04-20 6:22 ` Tian, Kevin 2022-04-11 9:36 ` [PATCH 3/8] VT-d: plug memory leaks in iommu_alloc() Jan Beulich 2022-04-12 8:29 ` Roger Pau Monné 2022-04-20 6:23 ` Tian, Kevin 2022-04-11 9:37 ` [PATCH 4/8] VT-d: refuse to use IOMMU with reserved CAP.ND value Jan Beulich 2022-04-12 9:22 ` Roger Pau Monné 2022-04-12 10:35 ` Jan Beulich 2022-04-20 6:23 ` Tian, Kevin 2022-04-11 9:37 ` [PATCH 5/8] AMD/IOMMU: replace a few PCI_BDF2() Jan Beulich 2022-04-12 9:37 ` Roger Pau Monné 2022-04-11 9:38 ` [PATCH 6/8] IOMMU: log appropriate SBDF Jan Beulich 2022-04-12 10:05 ` Roger Pau Monné 2022-04-12 10:39 ` Jan Beulich 2022-04-11 9:40 ` [PATCH 7/8] PCI: replace stray uses of PCI_{DEVFN,BDF}2() Jan Beulich 2022-04-12 10:07 ` Roger Pau Monné 2022-04-13 13:48 ` Bertrand Marquis 2022-04-13 13:55 ` Jan Beulich 2022-04-13 13:58 ` Roger Pau Monné 2022-04-13 14:13 ` Bertrand Marquis 2022-04-13 14:38 ` Jan Beulich 2022-04-20 6:29 ` Tian, Kevin 2022-04-11 9:42 ` [PATCH 8/8] PCI: replace "secondary" flavors of PCI_{DEVFN,BDF,SBDF}() Jan Beulich 2022-04-12 10:49 ` Roger Pau Monné 2022-04-20 6:37 ` Tian, Kevin
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.