Re: [PATCH 1/2] VT-d: install sync_cache hook on demand

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.07.2020 12:14, Roger Pau Monné wrote:
> On Wed, Jul 15, 2020 at 12:03:57PM +0200, Jan Beulich wrote:
>> Instead of checking inside the hook whether any non-coherent IOMMUs are
>> present, simply install the hook only when this is the case.
>>
>> To prove that there are no other references to the now dynamically
>> updated ops structure (and hence that its updating happens early
>> enough), make it static and rename it at the same time.
>>
>> Note that this change implies that sync_cache() shouldn't be called
>> directly unless there are unusual circumstances, like is the case in
>> alloc_pgtable_maddr(), which gets invoked too early for iommu_ops to
>> be set already (and therefore we also need to be careful there to
>> avoid accessing vtd_ops later on, as it lives in .init).
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> I think this is slightly better than what we currently have:
> 
> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks.

> I would however prefer if we also added a check to assert that
> alloc_pgtable_maddr is never called before iommu_alloc.

It would be quite odd for this to happen - what point would
there be to allocate a table to hang off of an IOMMU when
no IOMMU was found at all so far? Furthermore, such a
restriction could either be viewed to not suffice afaict (as
a subsequent iommu_alloc() may in principle fine a non-
coherent IOMMU), or to be pointless (until a non-coherent
IOMMU was found and allocated any table for, it doesn't
really matter whether we sync cache). In the end, whether to
sync cache in alloc_pgtable_maddr() doesn't really depend on
any global property, but only on the property / properties
of the IOMMU(s) the table is going to be made visible to.

> We could maybe
> poison the .sync_cache field, and then either set to NULL or to
> sync_cache in iommu_alloc?

Poisoning is at least latently problematic, due to alternative
insn patching we use for indirect calls. There are two passes,
where the 1st pass skips any instances where the target address
is still NULL. Of course that code could be made honor the
poison value as well, but to me this looks like going too far.

Jan