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

[Jan Beulich <jbeulich@xxxxxxxx>]

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