Re: [PATCH] iommu/amd-vi: adjust _amd_iommu_flush_pages() to handle pseudo-domids

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.