| Message ID | 20230614083236.64574-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iommu/amd-vi: adjust _amd_iommu_flush_pages() to handle pseudo-domids | expand |
On 14.06.2023 10:32, Roger Pau Monne wrote: > When the passed domain is DomIO iterate over the list of DomIO > assigned devices and flush each pseudo-domid found. > > invalidate_all_domain_pages() does call amd_iommu_flush_all_pages() > with DomIO as a parameter, Does it? Since the full function is visible in the patch (because of the "While there ..." change), it seems pretty clear that it doesn't: for_each_domain() iterates over ordinary domains only. > and hence the underlying > _amd_iommu_flush_pages() implementation must be capable of flushing > all pseudo-domids used by the quarantine domain logic. While it didn't occur to me right away when we discussed this, it may well be that I left alone all flushing when introducing the pseudo domain IDs simply because no flushing would ever happen for the quarantine domain. > While there fix invalidate_all_domain_pages() to only attempt to flush > the domains that have IOMMU enabled, otherwise the flush is pointless. For the moment at least it looks to me as if this change alone wants to go in. Jan
On Wed, Jun 14, 2023 at 02:58:14PM +0200, Jan Beulich wrote: > On 14.06.2023 10:32, Roger Pau Monne wrote: > > When the passed domain is DomIO iterate over the list of DomIO > > assigned devices and flush each pseudo-domid found. > > > > invalidate_all_domain_pages() does call amd_iommu_flush_all_pages() > > with DomIO as a parameter, > > Does it? Since the full function is visible in the patch (because of > the "While there ..." change), it seems pretty clear that it doesn't: > for_each_domain() iterates over ordinary domains only. Oh, I got confused by domain_create() returning early for system domains. > > and hence the underlying > > _amd_iommu_flush_pages() implementation must be capable of flushing > > all pseudo-domids used by the quarantine domain logic. > > While it didn't occur to me right away when we discussed this, it > may well be that I left alone all flushing when introducing the pseudo > domain IDs simply because no flushing would ever happen for the > quarantine domain. But the purpose of the calls to invalidate_all_devices() and invalidate_all_domain_pages() in amd_iommu_resume() is to cover up for the lack of Invalidate All support in the IOMMU, so flushing pseudo-domids is also required in order to flush all possible IOMMU state. Note that as part of invalidate_all_devices() we do invalidate DTEs for devices assigned to pseudo-domids, hence it seems natural that we also flush such pseudo-domids. > > While there fix invalidate_all_domain_pages() to only attempt to flush > > the domains that have IOMMU enabled, otherwise the flush is pointless. > > For the moment at least it looks to me as if this change alone wants > to go in. I would rather get the current patch with an added call to flush dom_io in invalidate_all_domain_pages(). Thanks, Roger.
On 14.06.2023 15:23, Roger Pau Monné wrote: > On Wed, Jun 14, 2023 at 02:58:14PM +0200, Jan Beulich wrote: >> On 14.06.2023 10:32, Roger Pau Monne wrote: >>> When the passed domain is DomIO iterate over the list of DomIO >>> assigned devices and flush each pseudo-domid found. >>> >>> invalidate_all_domain_pages() does call amd_iommu_flush_all_pages() >>> with DomIO as a parameter, >> >> Does it? Since the full function is visible in the patch (because of >> the "While there ..." change), it seems pretty clear that it doesn't: >> for_each_domain() iterates over ordinary domains only. > > Oh, I got confused by domain_create() returning early for system > domains. > >>> and hence the underlying >>> _amd_iommu_flush_pages() implementation must be capable of flushing >>> all pseudo-domids used by the quarantine domain logic. >> >> While it didn't occur to me right away when we discussed this, it >> may well be that I left alone all flushing when introducing the pseudo >> domain IDs simply because no flushing would ever happen for the >> quarantine domain. > > But the purpose of the calls to invalidate_all_devices() and > invalidate_all_domain_pages() in amd_iommu_resume() is to cover up for > the lack of Invalidate All support in the IOMMU, so flushing > pseudo-domids is also required in order to flush all possible IOMMU > state. > > Note that as part of invalidate_all_devices() we do invalidate DTEs > for devices assigned to pseudo-domids, hence it seems natural that we > also flush such pseudo-domids. > >>> While there fix invalidate_all_domain_pages() to only attempt to flush >>> the domains that have IOMMU enabled, otherwise the flush is pointless. >> >> For the moment at least it looks to me as if this change alone wants >> to go in. > > I would rather get the current patch with an added call to flush > dom_io in invalidate_all_domain_pages(). The question is: Is there anything that needs flushing for the quarantine domain. Right now I'm thinking that there isn't. Jan
diff --git a/xen/drivers/passthrough/amd/iommu_cmd.c b/xen/drivers/passthrough/amd/iommu_cmd.c index 40ddf366bb4d..ff55e3b88ae6 100644 --- a/xen/drivers/passthrough/amd/iommu_cmd.c +++ b/xen/drivers/passthrough/amd/iommu_cmd.c @@ -330,13 +330,34 @@ static void _amd_iommu_flush_pages(struct domain *d, daddr_t daddr, unsigned int order) { struct amd_iommu *iommu; - unsigned int dom_id = d->domain_id; /* send INVALIDATE_IOMMU_PAGES command */ - for_each_amd_iommu ( iommu ) + if ( d != dom_io ) { - invalidate_iommu_pages(iommu, daddr, dom_id, order); - flush_command_buffer(iommu, 0); + for_each_amd_iommu ( iommu ) + { + invalidate_iommu_pages(iommu, daddr, d->domain_id, order); + flush_command_buffer(iommu, 0); + } + } + else + { + const struct pci_dev *pdev; + + for_each_pdev(dom_io, pdev) + if ( pdev->arch.pseudo_domid != DOMID_INVALID ) + { + iommu = find_iommu_for_device(pdev->sbdf.seg, pdev->sbdf.bdf); + if ( !iommu ) + { + ASSERT_UNREACHABLE(); + continue; + } + + invalidate_iommu_pages(iommu, daddr, pdev->arch.pseudo_domid, + order); + flush_command_buffer(iommu, 0); + } } if ( ats_enabled ) diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c index 7dbd7e7d094a..af6713d2fc02 100644 --- a/xen/drivers/passthrough/amd/iommu_init.c +++ b/xen/drivers/passthrough/amd/iommu_init.c @@ -1532,8 +1532,10 @@ int __init amd_iommu_init_late(void) static void invalidate_all_domain_pages(void) { struct domain *d; + for_each_domain( d ) - amd_iommu_flush_all_pages(d); + if ( is_iommu_enabled(d) ) + amd_iommu_flush_all_pages(d); } static int cf_check _invalidate_all_devices(
When the passed domain is DomIO iterate over the list of DomIO assigned devices and flush each pseudo-domid found. invalidate_all_domain_pages() does call amd_iommu_flush_all_pages() with DomIO as a parameter, and hence the underlying _amd_iommu_flush_pages() implementation must be capable of flushing all pseudo-domids used by the quarantine domain logic. While there fix invalidate_all_domain_pages() to only attempt to flush the domains that have IOMMU enabled, otherwise the flush is pointless. Fixes: 14dd241aad8a ('IOMMU/x86: use per-device page tables for quarantining') Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Only build tested, as I don't have an AMD system that I can do suspend/resume on. --- xen/drivers/passthrough/amd/iommu_cmd.c | 29 ++++++++++++++++++++---- xen/drivers/passthrough/amd/iommu_init.c | 4 +++- 2 files changed, 28 insertions(+), 5 deletions(-)