* [PATCH v2 0/3] IOMMU/PCI: respect device specifics
@ 2021-09-17 10:58 Jan Beulich
2021-09-17 11:00 ` [PATCH v2 1/3] VT-d: consider hidden devices when unmapping Jan Beulich
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Jan Beulich @ 2021-09-17 10:58 UTC (permalink / raw)
To: xen-devel; +Cc: Paul Durrant, Kevin Tian, Andrew Cooper
It has occurred to me that the recent vPCI-related discussion about
hidden devices has some relevance also elsewhere. In the course of
addressing review comments of what is now patch 1 I then came to
notice yet another quirk.
1: VT-d: consider hidden devices when unmapping
2: VT-d: PCI segments are up to 16 bits wide
3: AMD/IOMMU: consider hidden devices when flushing device I/O TLBs
Jan
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH v2 1/3] VT-d: consider hidden devices when unmapping 2021-09-17 10:58 [PATCH v2 0/3] IOMMU/PCI: respect device specifics Jan Beulich @ 2021-09-17 11:00 ` Jan Beulich 2021-09-18 0:48 ` Tian, Kevin 2021-09-17 11:00 ` [PATCH v2 2/3] VT-d: PCI segments are up to 16 bits wide Jan Beulich 2021-09-17 11:00 ` [PATCH v2 3/3] AMD/IOMMU: consider hidden devices when flushing device I/O TLBs Jan Beulich 2 siblings, 1 reply; 8+ messages in thread From: Jan Beulich @ 2021-09-17 11:00 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 the "current device" exclusion check to a simple pointer comparison, - convert "found" to "bool", - adjust style and correct a typo in an existing comment. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- v2: Introduce helper function. --- a/xen/drivers/passthrough/vtd/iommu.c +++ b/xen/drivers/passthrough/vtd/iommu.c @@ -1650,6 +1650,27 @@ int domain_context_unmap_one( return rc; } +static bool any_pdev_behind_iommu(const struct domain *d, + const struct pci_dev *exclude, + const struct vtd_iommu *iommu) +{ + const struct pci_dev *pdev; + + for_each_pdev ( d, pdev ) + { + const struct acpi_drhd_unit *drhd; + + if ( pdev == exclude ) + continue; + + drhd = acpi_find_matched_drhd_unit(pdev); + if ( drhd && drhd->iommu == iommu ) + return true; + } + + return false; +} + static int domain_context_unmap(struct domain *domain, u8 devfn, struct pci_dev *pdev) { @@ -1657,7 +1678,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 +1758,18 @@ 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 ) - { - if ( pdev->seg == seg && pdev->bus == bus && pdev->devfn == devfn ) - continue; - - drhd = acpi_find_matched_drhd_unit(pdev); - if ( drhd && drhd->iommu == iommu ) - { - found = 1; - break; - } - } + found = any_pdev_behind_iommu(domain, pdev, iommu); + /* + * 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) ) + found = any_pdev_behind_iommu(dom_xen, pdev, iommu); - 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] 8+ messages in thread
* RE: [PATCH v2 1/3] VT-d: consider hidden devices when unmapping 2021-09-17 11:00 ` [PATCH v2 1/3] VT-d: consider hidden devices when unmapping Jan Beulich @ 2021-09-18 0:48 ` Tian, Kevin 0 siblings, 0 replies; 8+ messages in thread From: Tian, Kevin @ 2021-09-18 0:48 UTC (permalink / raw) To: Jan Beulich, xen-devel; +Cc: Paul Durrant > From: Jan Beulich <jbeulich@suse.com> > Sent: Friday, September 17, 2021 7:00 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 the "current device" exclusion check to a simple pointer > comparison, > - convert "found" to "bool", > - adjust style and correct a typo in an existing comment. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Kevin Tian <kevin.tian@intel.com> > --- > v2: Introduce helper function. > > --- a/xen/drivers/passthrough/vtd/iommu.c > +++ b/xen/drivers/passthrough/vtd/iommu.c > @@ -1650,6 +1650,27 @@ int domain_context_unmap_one( > return rc; > } > > +static bool any_pdev_behind_iommu(const struct domain *d, > + const struct pci_dev *exclude, > + const struct vtd_iommu *iommu) > +{ > + const struct pci_dev *pdev; > + > + for_each_pdev ( d, pdev ) > + { > + const struct acpi_drhd_unit *drhd; > + > + if ( pdev == exclude ) > + continue; > + > + drhd = acpi_find_matched_drhd_unit(pdev); > + if ( drhd && drhd->iommu == iommu ) > + return true; > + } > + > + return false; > +} > + > static int domain_context_unmap(struct domain *domain, u8 devfn, > struct pci_dev *pdev) > { > @@ -1657,7 +1678,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 +1758,18 @@ 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 ) > - { > - if ( pdev->seg == seg && pdev->bus == bus && pdev->devfn == devfn ) > - continue; > - > - drhd = acpi_find_matched_drhd_unit(pdev); > - if ( drhd && drhd->iommu == iommu ) > - { > - found = 1; > - break; > - } > - } > + found = any_pdev_behind_iommu(domain, pdev, iommu); > + /* > + * 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) ) > + found = any_pdev_behind_iommu(dom_xen, pdev, iommu); > > - 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] 8+ messages in thread
* [PATCH v2 2/3] VT-d: PCI segments are up to 16 bits wide 2021-09-17 10:58 [PATCH v2 0/3] IOMMU/PCI: respect device specifics Jan Beulich 2021-09-17 11:00 ` [PATCH v2 1/3] VT-d: consider hidden devices when unmapping Jan Beulich @ 2021-09-17 11:00 ` Jan Beulich 2021-09-18 0:49 ` Tian, Kevin 2021-09-17 11:00 ` [PATCH v2 3/3] AMD/IOMMU: consider hidden devices when flushing device I/O TLBs Jan Beulich 2 siblings, 1 reply; 8+ messages in thread From: Jan Beulich @ 2021-09-17 11:00 UTC (permalink / raw) To: xen-devel; +Cc: Paul Durrant, Kevin Tian We shouldn't silently truncate respective values. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- Strictly speaking we shouldn't use uint<N>_t here at all. I wasn't sure though whether going straight to unsigned int wouldn't be viewed as introducing inconsistencies. --- v2: New. --- a/xen/drivers/passthrough/vtd/iommu.c +++ b/xen/drivers/passthrough/vtd/iommu.c @@ -1462,7 +1462,8 @@ static int domain_context_mapping(struct { const struct acpi_drhd_unit *drhd = acpi_find_matched_drhd_unit(pdev); int ret = 0; - u8 seg = pdev->seg, bus = pdev->bus, secbus; + uint16_t seg = pdev->seg; + uint8_t bus = pdev->bus, secbus; /* * Generally we assume only devices from one node to get assigned to a @@ -1677,7 +1678,8 @@ static int domain_context_unmap(struct d 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; + uint16_t seg = pdev->seg; + uint8_t bus = pdev->bus, tmp_bus, tmp_devfn, secbus; bool found; switch ( pdev->type ) ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH v2 2/3] VT-d: PCI segments are up to 16 bits wide 2021-09-17 11:00 ` [PATCH v2 2/3] VT-d: PCI segments are up to 16 bits wide Jan Beulich @ 2021-09-18 0:49 ` Tian, Kevin 0 siblings, 0 replies; 8+ messages in thread From: Tian, Kevin @ 2021-09-18 0:49 UTC (permalink / raw) To: Jan Beulich, xen-devel; +Cc: Paul Durrant > From: Jan Beulich <jbeulich@suse.com> > Sent: Friday, September 17, 2021 7:00 PM > > We shouldn't silently truncate respective values. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Kevin Tian <kevin.tian@intel.com> > --- > Strictly speaking we shouldn't use uint<N>_t here at all. I wasn't sure > though whether going straight to unsigned int wouldn't be viewed as > introducing inconsistencies. > --- > v2: New. > > --- a/xen/drivers/passthrough/vtd/iommu.c > +++ b/xen/drivers/passthrough/vtd/iommu.c > @@ -1462,7 +1462,8 @@ static int domain_context_mapping(struct > { > const struct acpi_drhd_unit *drhd = acpi_find_matched_drhd_unit(pdev); > int ret = 0; > - u8 seg = pdev->seg, bus = pdev->bus, secbus; > + uint16_t seg = pdev->seg; > + uint8_t bus = pdev->bus, secbus; > > /* > * Generally we assume only devices from one node to get assigned to a > @@ -1677,7 +1678,8 @@ static int domain_context_unmap(struct d > 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; > + uint16_t seg = pdev->seg; > + uint8_t bus = pdev->bus, tmp_bus, tmp_devfn, secbus; > bool found; > > switch ( pdev->type ) ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 3/3] AMD/IOMMU: consider hidden devices when flushing device I/O TLBs 2021-09-17 10:58 [PATCH v2 0/3] IOMMU/PCI: respect device specifics Jan Beulich 2021-09-17 11:00 ` [PATCH v2 1/3] VT-d: consider hidden devices when unmapping Jan Beulich 2021-09-17 11:00 ` [PATCH v2 2/3] VT-d: PCI segments are up to 16 bits wide Jan Beulich @ 2021-09-17 11:00 ` Jan Beulich 2021-10-11 8:04 ` Ping: " Jan Beulich 2 siblings, 1 reply; 8+ messages in thread From: Jan Beulich @ 2021-09-17 11:00 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] 8+ messages in thread
* Ping: [PATCH v2 3/3] AMD/IOMMU: consider hidden devices when flushing device I/O TLBs 2021-09-17 11:00 ` [PATCH v2 3/3] AMD/IOMMU: consider hidden devices when flushing device I/O TLBs Jan Beulich @ 2021-10-11 8:04 ` Jan Beulich 2021-10-11 9:45 ` Durrant, Paul 0 siblings, 1 reply; 8+ messages in thread From: Jan Beulich @ 2021-10-11 8:04 UTC (permalink / raw) To: Andrew Cooper; +Cc: Paul Durrant, xen-devel On 17.09.2021 13:00, Jan Beulich wrote: > 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> The VT-d side equivalent having gone in a while ago, I think it would be good to have the AMD side on par. Jan > --- 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] 8+ messages in thread
* Re: Ping: [PATCH v2 3/3] AMD/IOMMU: consider hidden devices when flushing device I/O TLBs 2021-10-11 8:04 ` Ping: " Jan Beulich @ 2021-10-11 9:45 ` Durrant, Paul 0 siblings, 0 replies; 8+ messages in thread From: Durrant, Paul @ 2021-10-11 9:45 UTC (permalink / raw) To: Jan Beulich, Andrew Cooper; +Cc: Paul Durrant, xen-devel On 11/10/2021 09:04, Jan Beulich wrote: > On 17.09.2021 13:00, Jan Beulich wrote: >> 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> > > The VT-d side equivalent having gone in a while ago, I think it would > be good to have the AMD side on par. > Agreed. Reviewed-by: Paul Durrant <paul@xen.org> ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-10-11 9:46 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-17 10:58 [PATCH v2 0/3] IOMMU/PCI: respect device specifics Jan Beulich 2021-09-17 11:00 ` [PATCH v2 1/3] VT-d: consider hidden devices when unmapping Jan Beulich 2021-09-18 0:48 ` Tian, Kevin 2021-09-17 11:00 ` [PATCH v2 2/3] VT-d: PCI segments are up to 16 bits wide Jan Beulich 2021-09-18 0:49 ` Tian, Kevin 2021-09-17 11:00 ` [PATCH v2 3/3] AMD/IOMMU: consider hidden devices when flushing device I/O TLBs Jan Beulich 2021-10-11 8:04 ` Ping: " Jan Beulich 2021-10-11 9:45 ` Durrant, Paul
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.