[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Thu, 16 Jul 2020 12:34:37 +0200
  • Authentication-results: esa3.hc3370-68.iphmx.com; dkim=none (message not signed) header.i=none
  • Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Kevin Tian <kevin.tian@xxxxxxxxx>
  • Delivery-date: Thu, 16 Jul 2020 10:34:47 +0000
  • Ironport-sdr: hAOVjV6VmwPMPFTql9GX7W71qPlw21i+K1BM3+mOVGPxx1RwIO8BCPIX7yBDY1HLd9yCpXHKX8 Lw4/gqnY3qCVgBmObx4XRJq0YHjnTA6prbckmG+D23Rr0v8qpJFK0XJJ1mwtKUUzv096WvvGtv AlDrmGjVZ2I55iYmMgl4AGu7MK2H2pHCgpYbkDPJfxoQH3vUXf6DQoCJs86/1hcbYogVPpP2q1 5zuRTFARMtlTf7STAGPa2zSE7ANyMfmOJQ+TokzWNFQaOonhx3O/uRWC9cHdVduV+QjQulDcYU z4w=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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.




Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.