[PATCH 0/2] VT-d: XSA-321 follow-up

[PATCH 0/2] VT-d: XSA-321 follow-up

[Jan Beulich]

1: install sync_cache hook on demand
2: use clear_page() in alloc_pgtable_maddr()

Jan

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

[Jan Beulich]

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@suse.com>

--- a/xen/drivers/passthrough/vtd/extern.h
+++ b/xen/drivers/passthrough/vtd/extern.h
@@ -28,7 +28,6 @@
 struct pci_ats_dev;
 extern bool_t rwbf_quirk;
 extern const struct iommu_init_ops intel_iommu_init_ops;
-extern const struct iommu_ops intel_iommu_ops;
 
 void print_iommu_regs(struct acpi_drhd_unit *drhd);
 void print_vtd_entries(struct vtd_iommu *iommu, int bus, int devfn, u64 gmfn);
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -59,6 +59,7 @@ bool __read_mostly iommu_snoop = true;
 
 int nr_iommus;
 
+static struct iommu_ops vtd_ops;
 static struct tasklet vtd_fault_tasklet;
 
 static int setup_hwdom_device(u8 devfn, struct pci_dev *);
@@ -146,16 +147,11 @@ static int context_get_domain_id(struct
     return domid;
 }
 
-static int iommus_incoherent;
-
 static void sync_cache(const void *addr, unsigned int size)
 {
     static unsigned long clflush_size = 0;
     const void *end = addr + size;
 
-    if ( !iommus_incoherent )
-        return;
-
     if ( clflush_size == 0 )
         clflush_size = get_cache_line_size();
 
@@ -217,7 +213,8 @@ uint64_t alloc_pgtable_maddr(unsigned lo
         vaddr = __map_domain_page(cur_pg);
         memset(vaddr, 0, PAGE_SIZE);
 
-        sync_cache(vaddr, PAGE_SIZE);
+        if ( (iommu_ops.init ? &iommu_ops : &vtd_ops)->sync_cache )
+            sync_cache(vaddr, PAGE_SIZE);
         unmap_domain_page(vaddr);
         cur_pg++;
     }
@@ -1227,7 +1224,7 @@ int __init iommu_alloc(struct acpi_drhd_
     iommu->nr_pt_levels = agaw_to_level(agaw);
 
     if ( !ecap_coherent(iommu->ecap) )
-        iommus_incoherent = 1;
+        vtd_ops.sync_cache = sync_cache;
 
     /* allocate domain id bitmap */
     nr_dom = cap_ndoms(iommu->cap);
@@ -2737,7 +2734,7 @@ static int __init intel_iommu_quarantine
     return level ? -ENOMEM : rc;
 }
 
-const struct iommu_ops __initconstrel intel_iommu_ops = {
+static struct iommu_ops __initdata vtd_ops = {
     .init = intel_iommu_domain_init,
     .hwdom_init = intel_iommu_hwdom_init,
     .quarantine_init = intel_iommu_quarantine_init,
@@ -2768,11 +2765,10 @@ const struct iommu_ops __initconstrel in
     .iotlb_flush_all = iommu_flush_iotlb_all,
     .get_reserved_device_memory = intel_iommu_get_reserved_device_memory,
     .dump_p2m_table = vtd_dump_p2m_table,
-    .sync_cache = sync_cache,
 };
 
 const struct iommu_init_ops __initconstrel intel_iommu_init_ops = {
-    .ops = &intel_iommu_ops,
+    .ops = &vtd_ops,
     .setup = vtd_setup,
     .supports_x2apic = intel_iommu_supports_eim,
 };

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

[Roger Pau Monné]

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@suse.com>

I think this is slightly better than what we currently have:

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

I would however prefer if we also added a check to assert that
alloc_pgtable_maddr is never called before iommu_alloc. We could maybe
poison the .sync_cache field, and then either set to NULL or to
sync_cache in iommu_alloc?

Maybe I'm just overly paranoid with this.

Thanks.

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

[Jan Beulich]

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@suse.com>
> 
> I think this is slightly better than what we currently have:
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

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

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

[Roger Pau Monné]

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@suse.com>
> > 
> > I think this is slightly better than what we currently have:
> > 
> > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> 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.

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

[Tian, Kevin]

> From: Jan Beulich <jbeulich@suse.com>
> Sent: Wednesday, July 15, 2020 6:04 PM
> 
> 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@suse.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

> 
> --- a/xen/drivers/passthrough/vtd/extern.h
> +++ b/xen/drivers/passthrough/vtd/extern.h
> @@ -28,7 +28,6 @@
>  struct pci_ats_dev;
>  extern bool_t rwbf_quirk;
>  extern const struct iommu_init_ops intel_iommu_init_ops;
> -extern const struct iommu_ops intel_iommu_ops;
> 
>  void print_iommu_regs(struct acpi_drhd_unit *drhd);
>  void print_vtd_entries(struct vtd_iommu *iommu, int bus, int devfn, u64
> gmfn);
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -59,6 +59,7 @@ bool __read_mostly iommu_snoop = true;
> 
>  int nr_iommus;
> 
> +static struct iommu_ops vtd_ops;
>  static struct tasklet vtd_fault_tasklet;
> 
>  static int setup_hwdom_device(u8 devfn, struct pci_dev *);
> @@ -146,16 +147,11 @@ static int context_get_domain_id(struct
>      return domid;
>  }
> 
> -static int iommus_incoherent;
> -
>  static void sync_cache(const void *addr, unsigned int size)
>  {
>      static unsigned long clflush_size = 0;
>      const void *end = addr + size;
> 
> -    if ( !iommus_incoherent )
> -        return;
> -
>      if ( clflush_size == 0 )
>          clflush_size = get_cache_line_size();
> 
> @@ -217,7 +213,8 @@ uint64_t alloc_pgtable_maddr(unsigned lo
>          vaddr = __map_domain_page(cur_pg);
>          memset(vaddr, 0, PAGE_SIZE);
> 
> -        sync_cache(vaddr, PAGE_SIZE);
> +        if ( (iommu_ops.init ? &iommu_ops : &vtd_ops)->sync_cache )
> +            sync_cache(vaddr, PAGE_SIZE);
>          unmap_domain_page(vaddr);
>          cur_pg++;
>      }
> @@ -1227,7 +1224,7 @@ int __init iommu_alloc(struct acpi_drhd_
>      iommu->nr_pt_levels = agaw_to_level(agaw);
> 
>      if ( !ecap_coherent(iommu->ecap) )
> -        iommus_incoherent = 1;
> +        vtd_ops.sync_cache = sync_cache;
> 
>      /* allocate domain id bitmap */
>      nr_dom = cap_ndoms(iommu->cap);
> @@ -2737,7 +2734,7 @@ static int __init intel_iommu_quarantine
>      return level ? -ENOMEM : rc;
>  }
> 
> -const struct iommu_ops __initconstrel intel_iommu_ops = {
> +static struct iommu_ops __initdata vtd_ops = {
>      .init = intel_iommu_domain_init,
>      .hwdom_init = intel_iommu_hwdom_init,
>      .quarantine_init = intel_iommu_quarantine_init,
> @@ -2768,11 +2765,10 @@ const struct iommu_ops __initconstrel in
>      .iotlb_flush_all = iommu_flush_iotlb_all,
>      .get_reserved_device_memory =
> intel_iommu_get_reserved_device_memory,
>      .dump_p2m_table = vtd_dump_p2m_table,
> -    .sync_cache = sync_cache,
>  };
> 
>  const struct iommu_init_ops __initconstrel intel_iommu_init_ops = {
> -    .ops = &intel_iommu_ops,
> +    .ops = &vtd_ops,
>      .setup = vtd_setup,
>      .supports_x2apic = intel_iommu_supports_eim,
>  };

[PATCH 2/2] VT-d: use clear_page() in alloc_pgtable_maddr()

[Jan Beulich]

For full pages this is (meant to be) more efficient. Also change the
type and reduce the scope of the involved local variable.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -199,7 +199,6 @@ static void sync_cache(const void *addr,
 uint64_t alloc_pgtable_maddr(unsigned long npages, nodeid_t node)
 {
     struct page_info *pg, *cur_pg;
-    u64 *vaddr;
     unsigned int i;
 
     pg = alloc_domheap_pages(NULL, get_order_from_pages(npages),
@@ -210,8 +209,9 @@ uint64_t alloc_pgtable_maddr(unsigned lo
     cur_pg = pg;
     for ( i = 0; i < npages; i++ )
     {
-        vaddr = __map_domain_page(cur_pg);
-        memset(vaddr, 0, PAGE_SIZE);
+        void *vaddr = __map_domain_page(cur_pg);
+
+        clear_page(vaddr);
 
         if ( (iommu_ops.init ? &iommu_ops : &vtd_ops)->sync_cache )
             sync_cache(vaddr, PAGE_SIZE);

Re: [PATCH 2/2] VT-d: use clear_page() in alloc_pgtable_maddr()

[Roger Pau Monné]

On Wed, Jul 15, 2020 at 12:04:15PM +0200, Jan Beulich wrote:
> For full pages this is (meant to be) more efficient. Also change the
> type and reduce the scope of the involved local variable.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

RE: [PATCH 2/2] VT-d: use clear_page() in alloc_pgtable_maddr()

[Tian, Kevin]

> From: Jan Beulich <jbeulich@suse.com>
> Sent: Wednesday, July 15, 2020 6:04 PM
> 
> For full pages this is (meant to be) more efficient. Also change the
> type and reduce the scope of the involved local variable.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

> 
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -199,7 +199,6 @@ static void sync_cache(const void *addr,
>  uint64_t alloc_pgtable_maddr(unsigned long npages, nodeid_t node)
>  {
>      struct page_info *pg, *cur_pg;
> -    u64 *vaddr;
>      unsigned int i;
> 
>      pg = alloc_domheap_pages(NULL, get_order_from_pages(npages),
> @@ -210,8 +209,9 @@ uint64_t alloc_pgtable_maddr(unsigned lo
>      cur_pg = pg;
>      for ( i = 0; i < npages; i++ )
>      {
> -        vaddr = __map_domain_page(cur_pg);
> -        memset(vaddr, 0, PAGE_SIZE);
> +        void *vaddr = __map_domain_page(cur_pg);
> +
> +        clear_page(vaddr);
> 
>          if ( (iommu_ops.init ? &iommu_ops : &vtd_ops)->sync_cache )
>              sync_cache(vaddr, PAGE_SIZE);