* [PATCH 0/4] IOMMU/PCI: respect device specifics
@ 2021-09-15 9:11 Jan Beulich
2021-09-15 9:12 ` [PATCH 1/4] VT-d: defer "no DRHD" check when (un)mapping devices Jan Beulich
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Jan Beulich @ 2021-09-15 9:11 UTC (permalink / raw)
To: xen-devel; +Cc: Paul Durrant, Kevin Tian, Andrew Cooper
While making the first change here it occurred to me that the recent
vPCI-related discussion about hidden devices has some relevance also
elsewhere. In the course I then came to also notice a phantom device
related quirk.
1: VT-d: defer "no DRHD" check when (un)mapping devices
2: VT-d: consider hidden devices when unmapping
3: VT-d: skip IOMMU bitmap cleanup for phantom devices
4: AMD/IOMMU: consider hidden devices when flushing device I/O TLBs
Jan
(I'm sorry for the resend, but I thought I would better add the
previously missing tag.)
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 1/4] VT-d: defer "no DRHD" check when (un)mapping devices 2021-09-15 9:11 [PATCH 0/4] IOMMU/PCI: respect device specifics Jan Beulich @ 2021-09-15 9:12 ` Jan Beulich 2021-09-16 8:05 ` Tian, Kevin 2021-09-15 9:12 ` [PATCH 2/4] VT-d: consider hidden devices when unmapping Jan Beulich ` (2 subsequent siblings) 3 siblings, 1 reply; 9+ messages in thread From: Jan Beulich @ 2021-09-15 9:12 UTC (permalink / raw) To: xen-devel; +Cc: Paul Durrant, Kevin Tian If devices are to be skipped anyway (which is the case in particular for host bridges), there's no point complaining about a missing DRHD (and hence a missing association with an IOMMU). While there convert assignments to initializers and constify "drhd" local variables. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/drivers/passthrough/vtd/iommu.c +++ b/xen/drivers/passthrough/vtd/iommu.c @@ -1460,14 +1460,10 @@ static int domain_context_unmap(struct d static int domain_context_mapping(struct domain *domain, u8 devfn, struct pci_dev *pdev) { - struct acpi_drhd_unit *drhd; + const struct acpi_drhd_unit *drhd = acpi_find_matched_drhd_unit(pdev); int ret = 0; u8 seg = pdev->seg, bus = pdev->bus, secbus; - drhd = acpi_find_matched_drhd_unit(pdev); - if ( !drhd ) - return -ENODEV; - /* * Generally we assume only devices from one node to get assigned to a * given guest. But even if not, by replacing the prior value here we @@ -1476,7 +1472,7 @@ static int domain_context_mapping(struct * this or other devices may be penalized then, but some would also be * if we left other than NUMA_NO_NODE untouched here. */ - if ( drhd->iommu->node != NUMA_NO_NODE ) + if ( drhd && drhd->iommu->node != NUMA_NO_NODE ) dom_iommu(domain)->node = drhd->iommu->node; ASSERT(pcidevs_locked()); @@ -1497,6 +1493,9 @@ static int domain_context_mapping(struct break; case DEV_TYPE_PCIe_ENDPOINT: + if ( !drhd ) + return -ENODEV; + if ( iommu_debug ) printk(VTDPREFIX "%pd:PCIe: map %pp\n", domain, &PCI_SBDF3(seg, bus, devfn)); @@ -1508,6 +1507,9 @@ static int domain_context_mapping(struct break; case DEV_TYPE_PCI: + if ( !drhd ) + return -ENODEV; + if ( iommu_debug ) printk(VTDPREFIX "%pd:PCI: map %pp\n", domain, &PCI_SBDF3(seg, bus, devfn)); @@ -1651,17 +1653,12 @@ int domain_context_unmap_one( static int domain_context_unmap(struct domain *domain, u8 devfn, struct pci_dev *pdev) { - struct acpi_drhd_unit *drhd; - struct vtd_iommu *iommu; + const struct acpi_drhd_unit *drhd = acpi_find_matched_drhd_unit(pdev); + struct vtd_iommu *iommu = drhd ? drhd->iommu : NULL; int ret; u8 seg = pdev->seg, bus = pdev->bus, tmp_bus, tmp_devfn, secbus; int found = 0; - drhd = acpi_find_matched_drhd_unit(pdev); - if ( !drhd ) - return -ENODEV; - iommu = drhd->iommu; - switch ( pdev->type ) { case DEV_TYPE_PCI_HOST_BRIDGE: @@ -1676,6 +1673,9 @@ static int domain_context_unmap(struct d return 0; case DEV_TYPE_PCIe_ENDPOINT: + if ( !iommu ) + return -ENODEV; + if ( iommu_debug ) printk(VTDPREFIX "%pd:PCIe: unmap %pp\n", domain, &PCI_SBDF3(seg, bus, devfn)); @@ -1686,6 +1686,9 @@ static int domain_context_unmap(struct d break; case DEV_TYPE_PCI: + if ( !iommu ) + return -ENODEV; + if ( iommu_debug ) printk(VTDPREFIX "%pd:PCI: unmap %pp\n", domain, &PCI_SBDF3(seg, bus, devfn)); ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH 1/4] VT-d: defer "no DRHD" check when (un)mapping devices 2021-09-15 9:12 ` [PATCH 1/4] VT-d: defer "no DRHD" check when (un)mapping devices Jan Beulich @ 2021-09-16 8:05 ` Tian, Kevin 0 siblings, 0 replies; 9+ messages in thread From: Tian, Kevin @ 2021-09-16 8:05 UTC (permalink / raw) To: Jan Beulich, xen-devel; +Cc: Paul Durrant > From: Jan Beulich <jbeulich@suse.com> > Sent: Wednesday, September 15, 2021 5:12 PM > > If devices are to be skipped anyway (which is the case in particular for > host bridges), there's no point complaining about a missing DRHD (and > hence a missing association with an IOMMU). > > While there convert assignments to initializers and constify "drhd" > local variables. > > 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 > @@ -1460,14 +1460,10 @@ static int domain_context_unmap(struct d > static int domain_context_mapping(struct domain *domain, u8 devfn, > struct pci_dev *pdev) > { > - struct acpi_drhd_unit *drhd; > + const struct acpi_drhd_unit *drhd = acpi_find_matched_drhd_unit(pdev); > int ret = 0; > u8 seg = pdev->seg, bus = pdev->bus, secbus; > > - drhd = acpi_find_matched_drhd_unit(pdev); > - if ( !drhd ) > - return -ENODEV; > - > /* > * Generally we assume only devices from one node to get assigned to a > * given guest. But even if not, by replacing the prior value here we > @@ -1476,7 +1472,7 @@ static int domain_context_mapping(struct > * this or other devices may be penalized then, but some would also be > * if we left other than NUMA_NO_NODE untouched here. > */ > - if ( drhd->iommu->node != NUMA_NO_NODE ) > + if ( drhd && drhd->iommu->node != NUMA_NO_NODE ) > dom_iommu(domain)->node = drhd->iommu->node; > > ASSERT(pcidevs_locked()); > @@ -1497,6 +1493,9 @@ static int domain_context_mapping(struct > break; > > case DEV_TYPE_PCIe_ENDPOINT: > + if ( !drhd ) > + return -ENODEV; > + > if ( iommu_debug ) > printk(VTDPREFIX "%pd:PCIe: map %pp\n", > domain, &PCI_SBDF3(seg, bus, devfn)); > @@ -1508,6 +1507,9 @@ static int domain_context_mapping(struct > break; > > case DEV_TYPE_PCI: > + if ( !drhd ) > + return -ENODEV; > + > if ( iommu_debug ) > printk(VTDPREFIX "%pd:PCI: map %pp\n", > domain, &PCI_SBDF3(seg, bus, devfn)); > @@ -1651,17 +1653,12 @@ int domain_context_unmap_one( > static int domain_context_unmap(struct domain *domain, u8 devfn, > struct pci_dev *pdev) > { > - struct acpi_drhd_unit *drhd; > - struct vtd_iommu *iommu; > + const struct acpi_drhd_unit *drhd = acpi_find_matched_drhd_unit(pdev); > + struct vtd_iommu *iommu = drhd ? drhd->iommu : NULL; > int ret; > u8 seg = pdev->seg, bus = pdev->bus, tmp_bus, tmp_devfn, secbus; > int found = 0; > > - drhd = acpi_find_matched_drhd_unit(pdev); > - if ( !drhd ) > - return -ENODEV; > - iommu = drhd->iommu; > - > switch ( pdev->type ) > { > case DEV_TYPE_PCI_HOST_BRIDGE: > @@ -1676,6 +1673,9 @@ static int domain_context_unmap(struct d > return 0; > > case DEV_TYPE_PCIe_ENDPOINT: > + if ( !iommu ) > + return -ENODEV; > + > if ( iommu_debug ) > printk(VTDPREFIX "%pd:PCIe: unmap %pp\n", > domain, &PCI_SBDF3(seg, bus, devfn)); > @@ -1686,6 +1686,9 @@ static int domain_context_unmap(struct d > break; > > case DEV_TYPE_PCI: > + if ( !iommu ) > + return -ENODEV; > + > if ( iommu_debug ) > printk(VTDPREFIX "%pd:PCI: unmap %pp\n", > domain, &PCI_SBDF3(seg, bus, devfn)); ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/4] VT-d: consider hidden devices when unmapping 2021-09-15 9:11 [PATCH 0/4] IOMMU/PCI: respect device specifics Jan Beulich 2021-09-15 9:12 ` [PATCH 1/4] VT-d: defer "no DRHD" check when (un)mapping devices Jan Beulich @ 2021-09-15 9:12 ` Jan Beulich 2021-09-16 8:18 ` Tian, Kevin 2021-09-15 9:13 ` [PATCH 3/4] VT-d: skip IOMMU bitmap cleanup for phantom devices Jan Beulich 2021-09-15 9:13 ` [PATCH 4/4] AMD/IOMMU: consider hidden devices when flushing device I/O TLBs Jan Beulich 3 siblings, 1 reply; 9+ messages in thread From: Jan Beulich @ 2021-09-15 9:12 UTC (permalink / raw) To: xen-devel; +Cc: Paul Durrant, Kevin Tian Whether to clear an IOMMU's bit in the domain's bitmap should depend on all devices the domain can control. For the hardware domain this includes hidden devices, which are associated with DomXEN. While touching related logic, convert "found" to "bool". Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/drivers/passthrough/vtd/iommu.c +++ b/xen/drivers/passthrough/vtd/iommu.c @@ -1657,7 +1657,7 @@ static int domain_context_unmap(struct d struct vtd_iommu *iommu = drhd ? drhd->iommu : NULL; int ret; u8 seg = pdev->seg, bus = pdev->bus, tmp_bus, tmp_devfn, secbus; - int found = 0; + bool found; switch ( pdev->type ) { @@ -1737,23 +1737,33 @@ static int domain_context_unmap(struct d return ret; /* - * if no other devices under the same iommu owned by this domain, - * clear iommu in iommu_bitmap and clear domain_id in domid_bitmp + * If no other devices under the same iommu owned by this domain, + * clear iommu in iommu_bitmap and clear domain_id in domid_bitmap. */ - for_each_pdev ( domain, pdev ) + for ( found = false; ; domain = dom_xen ) { - if ( pdev->seg == seg && pdev->bus == bus && pdev->devfn == devfn ) - continue; - - drhd = acpi_find_matched_drhd_unit(pdev); - if ( drhd && drhd->iommu == iommu ) + for_each_pdev ( domain, pdev ) { - found = 1; - break; + if ( pdev->seg == seg && pdev->bus == bus && pdev->devfn == devfn ) + continue; + + drhd = acpi_find_matched_drhd_unit(pdev); + if ( drhd && drhd->iommu == iommu ) + { + found = true; + break; + } } + + /* + * Hidden devices are associated with DomXEN but usable by the + * hardware domain. Hence they need considering here as well. + */ + if ( found || !is_hardware_domain(domain) ) + break; } - if ( found == 0 ) + if ( !found ) { clear_bit(iommu->index, &dom_iommu(domain)->arch.vtd.iommu_bitmap); cleanup_domid_map(domain, iommu); ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH 2/4] VT-d: consider hidden devices when unmapping 2021-09-15 9:12 ` [PATCH 2/4] VT-d: consider hidden devices when unmapping Jan Beulich @ 2021-09-16 8:18 ` Tian, Kevin 2021-09-16 8:31 ` Jan Beulich 0 siblings, 1 reply; 9+ messages in thread From: Tian, Kevin @ 2021-09-16 8:18 UTC (permalink / raw) To: Jan Beulich, xen-devel; +Cc: Paul Durrant > From: Jan Beulich > Sent: Wednesday, September 15, 2021 5:13 PM > > Whether to clear an IOMMU's bit in the domain's bitmap should depend on > all devices the domain can control. For the hardware domain this > includes hidden devices, which are associated with DomXEN. > > While touching related logic, convert "found" to "bool". > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/drivers/passthrough/vtd/iommu.c > +++ b/xen/drivers/passthrough/vtd/iommu.c > @@ -1657,7 +1657,7 @@ static int domain_context_unmap(struct d > struct vtd_iommu *iommu = drhd ? drhd->iommu : NULL; > int ret; > u8 seg = pdev->seg, bus = pdev->bus, tmp_bus, tmp_devfn, secbus; > - int found = 0; > + bool found; > > switch ( pdev->type ) > { > @@ -1737,23 +1737,33 @@ static int domain_context_unmap(struct d > return ret; > > /* > - * if no other devices under the same iommu owned by this domain, > - * clear iommu in iommu_bitmap and clear domain_id in domid_bitmp > + * If no other devices under the same iommu owned by this domain, > + * clear iommu in iommu_bitmap and clear domain_id in domid_bitmap. what is changed above? > */ > - for_each_pdev ( domain, pdev ) > + for ( found = false; ; domain = dom_xen ) honesty speaking I had to read several times to understand the break condition of this loop. It'd be easier to understand if putting for_each_ pdev(){} into a function and then: found = func(domain); if (!found && is_hardware_domain(domain)) found = func(dom_xen); > { > - if ( pdev->seg == seg && pdev->bus == bus && pdev->devfn == devfn ) > - continue; > - > - drhd = acpi_find_matched_drhd_unit(pdev); > - if ( drhd && drhd->iommu == iommu ) > + for_each_pdev ( domain, pdev ) > { > - found = 1; > - break; > + if ( pdev->seg == seg && pdev->bus == bus && pdev->devfn == devfn ) > + continue; > + > + drhd = acpi_find_matched_drhd_unit(pdev); > + if ( drhd && drhd->iommu == iommu ) > + { > + found = true; > + break; > + } > } > + > + /* > + * Hidden devices are associated with DomXEN but usable by the > + * hardware domain. Hence they need considering here as well. > + */ > + if ( found || !is_hardware_domain(domain) ) > + break; > } > > - if ( found == 0 ) > + if ( !found ) > { > clear_bit(iommu->index, &dom_iommu(domain)- > >arch.vtd.iommu_bitmap); > cleanup_domid_map(domain, iommu); > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/4] VT-d: consider hidden devices when unmapping 2021-09-16 8:18 ` Tian, Kevin @ 2021-09-16 8:31 ` Jan Beulich 0 siblings, 0 replies; 9+ messages in thread From: Jan Beulich @ 2021-09-16 8:31 UTC (permalink / raw) To: Tian, Kevin; +Cc: Paul Durrant, xen-devel On 16.09.2021 10:18, Tian, Kevin wrote: >> @@ -1737,23 +1737,33 @@ static int domain_context_unmap(struct d >> return ret; >> >> /* >> - * if no other devices under the same iommu owned by this domain, >> - * clear iommu in iommu_bitmap and clear domain_id in domid_bitmp >> + * If no other devices under the same iommu owned by this domain, >> + * clear iommu in iommu_bitmap and clear domain_id in domid_bitmap. > > what is changed above? A style and a spelling correction: Capital letter at the start and a missing 'a' in domid_bitmap. >> */ >> - for_each_pdev ( domain, pdev ) >> + for ( found = false; ; domain = dom_xen ) > > honesty speaking I had to read several times to understand the break > condition of this loop. It'd be easier to understand if putting for_each_ > pdev(){} into a function and then: > > found = func(domain); > if (!found && is_hardware_domain(domain)) > found = func(dom_xen); Can do so, sure. Jan ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/4] VT-d: skip IOMMU bitmap cleanup for phantom devices 2021-09-15 9:11 [PATCH 0/4] IOMMU/PCI: respect device specifics Jan Beulich 2021-09-15 9:12 ` [PATCH 1/4] VT-d: defer "no DRHD" check when (un)mapping devices Jan Beulich 2021-09-15 9:12 ` [PATCH 2/4] VT-d: consider hidden devices when unmapping Jan Beulich @ 2021-09-15 9:13 ` Jan Beulich 2021-09-16 8:19 ` Tian, Kevin 2021-09-15 9:13 ` [PATCH 4/4] AMD/IOMMU: consider hidden devices when flushing device I/O TLBs Jan Beulich 3 siblings, 1 reply; 9+ messages in thread From: Jan Beulich @ 2021-09-15 9:13 UTC (permalink / raw) To: xen-devel; +Cc: Paul Durrant, Kevin Tian Doing the cleanup also for phantom devices is at best redundant with doing it for the corresponding real device. I couldn't force myself into checking all the code paths whether it really is: It seems better to explicitly skip this step in such cases. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/drivers/passthrough/vtd/iommu.c +++ b/xen/drivers/passthrough/vtd/iommu.c @@ -1733,7 +1733,7 @@ static int domain_context_unmap(struct d return -EINVAL; } - if ( ret || QUARANTINE_SKIP(domain) ) + if ( ret || QUARANTINE_SKIP(domain) || pdev->devfn != devfn ) return ret; /* ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH 3/4] VT-d: skip IOMMU bitmap cleanup for phantom devices 2021-09-15 9:13 ` [PATCH 3/4] VT-d: skip IOMMU bitmap cleanup for phantom devices Jan Beulich @ 2021-09-16 8:19 ` Tian, Kevin 0 siblings, 0 replies; 9+ messages in thread From: Tian, Kevin @ 2021-09-16 8:19 UTC (permalink / raw) To: Jan Beulich, xen-devel; +Cc: Paul Durrant > From: Jan Beulich <jbeulich@suse.com> > Sent: Wednesday, September 15, 2021 5:13 PM > > Doing the cleanup also for phantom devices is at best redundant with > doing it for the corresponding real device. I couldn't force myself into > checking all the code paths whether it really is: It seems better to > explicitly skip this step in such cases. > > 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 > @@ -1733,7 +1733,7 @@ static int domain_context_unmap(struct d > return -EINVAL; > } > > - if ( ret || QUARANTINE_SKIP(domain) ) > + if ( ret || QUARANTINE_SKIP(domain) || pdev->devfn != devfn ) > return ret; > > /* ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 4/4] AMD/IOMMU: consider hidden devices when flushing device I/O TLBs 2021-09-15 9:11 [PATCH 0/4] IOMMU/PCI: respect device specifics Jan Beulich ` (2 preceding siblings ...) 2021-09-15 9:13 ` [PATCH 3/4] VT-d: skip IOMMU bitmap cleanup for phantom devices Jan Beulich @ 2021-09-15 9:13 ` Jan Beulich 3 siblings, 0 replies; 9+ messages in thread From: Jan Beulich @ 2021-09-15 9:13 UTC (permalink / raw) To: xen-devel; +Cc: Paul Durrant, Andrew Cooper Hidden devices are associated with DomXEN but usable by the hardware domain. Hence they need flushing as well when all devices are to have flushes invoked. While there drop a redundant ATS-enabled check and constify the first parameter of the involved 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 @@ -308,14 +308,11 @@ void amd_iommu_flush_iotlb(u8 devfn, con flush_command_buffer(iommu, iommu_dev_iotlb_timeout); } -static void amd_iommu_flush_all_iotlbs(struct domain *d, daddr_t daddr, +static void amd_iommu_flush_all_iotlbs(const struct domain *d, daddr_t daddr, unsigned int order) { struct pci_dev *pdev; - if ( !ats_enabled ) - return; - for_each_pdev( d, pdev ) { u8 devfn = pdev->devfn; @@ -343,7 +340,16 @@ static void _amd_iommu_flush_pages(struc } if ( ats_enabled ) + { amd_iommu_flush_all_iotlbs(d, daddr, order); + + /* + * Hidden devices are associated with DomXEN but usable by the + * hardware domain. Hence they need dealing with here as well. + */ + if ( is_hardware_domain(d) ) + amd_iommu_flush_all_iotlbs(dom_xen, daddr, order); + } } void amd_iommu_flush_all_pages(struct domain *d) ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-09-16 8:32 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-15 9:11 [PATCH 0/4] IOMMU/PCI: respect device specifics Jan Beulich 2021-09-15 9:12 ` [PATCH 1/4] VT-d: defer "no DRHD" check when (un)mapping devices Jan Beulich 2021-09-16 8:05 ` Tian, Kevin 2021-09-15 9:12 ` [PATCH 2/4] VT-d: consider hidden devices when unmapping Jan Beulich 2021-09-16 8:18 ` Tian, Kevin 2021-09-16 8:31 ` Jan Beulich 2021-09-15 9:13 ` [PATCH 3/4] VT-d: skip IOMMU bitmap cleanup for phantom devices Jan Beulich 2021-09-16 8:19 ` Tian, Kevin 2021-09-15 9:13 ` [PATCH 4/4] AMD/IOMMU: consider hidden devices when flushing device I/O TLBs Jan Beulich
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.