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

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

On Thu, Jul 16, 2020 at 12:25:40PM +0200, Jan Beulich wrote:
> 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.

Right, I think I was indeed overly paranoid. I was mostly worried
about iommu_alloc calling alloc_pgtable_maddr before checking whether
the IOOMMU is incoherent, but this is not likely to happen.

Thanks.