[PATCH 0/4] (mainly) IOMMU hook adjustments

[PATCH 0/4] (mainly) IOMMU hook adjustments

[Jan Beulich]

Besides the altcall conversion, some further adjustments appeared desirable
to do while touching that area.

1: IOMMU/x86: switch to alternatives-call patching in further instances
2: VT-d / x86: re-arrange cache syncing
3: VT-d: replace flush_all_cache()
4: libxc: correct bounce direction in xc_get_device_group()

Jan

[PATCH 1/4] IOMMU/x86: switch to alternatives-call patching in further instances

[Jan Beulich]

This is, once again, to limit the number of indirect calls as much as
possible. The only hook invocation which isn't sensible to convert is
setup(). And of course Arm-only use sites are left alone as well.

Note regarding the introduction / use of local variables in pci.c:
struct pci_dev's involved fields are const. This const propagates, via
typeof(), to the local helper variables in the altcall macros. These
helper variables are, however, used as outputs (and hence can't be
const). In iommu_get_device_group() make use of the new local variables
to also simplify some adjacent code.

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

--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -198,7 +198,7 @@ int iommu_domain_init(struct domain *d,
         return ret;
 
     hd->platform_ops = iommu_get_ops();
-    ret = hd->platform_ops->init(d);
+    ret = iommu_call(hd->platform_ops, init, d);
     if ( ret || is_system_domain(d) )
         return ret;
 
@@ -233,7 +233,7 @@ void __hwdom_init iommu_hwdom_init(struc
 
     register_keyhandler('o', &iommu_dump_page_tables, "dump iommu page tables", 0);
 
-    hd->platform_ops->hwdom_init(d);
+    iommu_vcall(hd->platform_ops, hwdom_init, d);
 }
 
 static void iommu_teardown(struct domain *d)
@@ -576,7 +576,7 @@ int iommu_get_reserved_device_memory(iom
     if ( !ops->get_reserved_device_memory )
         return 0;
 
-    return ops->get_reserved_device_memory(func, ctxt);
+    return iommu_call(ops, get_reserved_device_memory, func, ctxt);
 }
 
 bool_t iommu_has_feature(struct domain *d, enum iommu_feature feature)
@@ -603,7 +603,7 @@ static void iommu_dump_page_tables(unsig
             continue;
         }
 
-        dom_iommu(d)->platform_ops->dump_page_tables(d);
+        iommu_vcall(dom_iommu(d)->platform_ops, dump_page_tables, d);
     }
 
     rcu_read_unlock(&domlist_read_lock);
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -861,15 +861,15 @@ static int deassign_device(struct domain
         devfn += pdev->phantom_stride;
         if ( PCI_SLOT(devfn) != PCI_SLOT(pdev->devfn) )
             break;
-        ret = hd->platform_ops->reassign_device(d, target, devfn,
-                                                pci_to_dev(pdev));
+        ret = iommu_call(hd->platform_ops, reassign_device, d, target, devfn,
+                         pci_to_dev(pdev));
         if ( ret )
             goto out;
     }
 
     devfn = pdev->devfn;
-    ret = hd->platform_ops->reassign_device(d, target, devfn,
-                                            pci_to_dev(pdev));
+    ret = iommu_call(hd->platform_ops, reassign_device, d, target, devfn,
+                     pci_to_dev(pdev));
     if ( ret )
         goto out;
 
@@ -1300,7 +1300,7 @@ static int iommu_add_device(struct pci_d
 {
     const struct domain_iommu *hd;
     int rc;
-    u8 devfn;
+    unsigned int devfn = pdev->devfn;
 
     if ( !pdev->domain )
         return -EINVAL;
@@ -1311,16 +1311,16 @@ static int iommu_add_device(struct pci_d
     if ( !is_iommu_enabled(pdev->domain) )
         return 0;
 
-    rc = hd->platform_ops->add_device(pdev->devfn, pci_to_dev(pdev));
+    rc = iommu_call(hd->platform_ops, add_device, devfn, pci_to_dev(pdev));
     if ( rc || !pdev->phantom_stride )
         return rc;
 
-    for ( devfn = pdev->devfn ; ; )
+    for ( ; ; )
     {
         devfn += pdev->phantom_stride;
         if ( PCI_SLOT(devfn) != PCI_SLOT(pdev->devfn) )
             return 0;
-        rc = hd->platform_ops->add_device(devfn, pci_to_dev(pdev));
+        rc = iommu_call(hd->platform_ops, add_device, devfn, pci_to_dev(pdev));
         if ( rc )
             printk(XENLOG_WARNING "IOMMU: add %pp failed (%d)\n",
                    &pdev->sbdf, rc);
@@ -1341,7 +1341,7 @@ static int iommu_enable_device(struct pc
          !hd->platform_ops->enable_device )
         return 0;
 
-    return hd->platform_ops->enable_device(pci_to_dev(pdev));
+    return iommu_call(hd->platform_ops, enable_device, pci_to_dev(pdev));
 }
 
 static int iommu_remove_device(struct pci_dev *pdev)
@@ -1363,7 +1363,8 @@ static int iommu_remove_device(struct pc
         devfn += pdev->phantom_stride;
         if ( PCI_SLOT(devfn) != PCI_SLOT(pdev->devfn) )
             break;
-        rc = hd->platform_ops->remove_device(devfn, pci_to_dev(pdev));
+        rc = iommu_call(hd->platform_ops, remove_device, devfn,
+                        pci_to_dev(pdev));
         if ( !rc )
             continue;
 
@@ -1371,7 +1372,9 @@ static int iommu_remove_device(struct pc
         return rc;
     }
 
-    return hd->platform_ops->remove_device(pdev->devfn, pci_to_dev(pdev));
+    devfn = pdev->devfn;
+
+    return iommu_call(hd->platform_ops, remove_device, devfn, pci_to_dev(pdev));
 }
 
 static int device_assigned(u16 seg, u8 bus, u8 devfn)
@@ -1421,7 +1424,8 @@ static int assign_device(struct domain *
 
     pdev->fault.count = 0;
 
-    if ( (rc = hd->platform_ops->assign_device(d, devfn, pci_to_dev(pdev), flag)) )
+    if ( (rc = iommu_call(hd->platform_ops, assign_device, d, devfn,
+                          pci_to_dev(pdev), flag)) )
         goto done;
 
     for ( ; pdev->phantom_stride; rc = 0 )
@@ -1429,7 +1433,8 @@ static int assign_device(struct domain *
         devfn += pdev->phantom_stride;
         if ( PCI_SLOT(devfn) != PCI_SLOT(pdev->devfn) )
             break;
-        rc = hd->platform_ops->assign_device(d, devfn, pci_to_dev(pdev), flag);
+        rc = iommu_call(hd->platform_ops, assign_device, d, devfn,
+                        pci_to_dev(pdev), flag);
     }
 
  done:
@@ -1457,24 +1462,24 @@ static int iommu_get_device_group(
     if ( !is_iommu_enabled(d) || !ops->get_device_group_id )
         return 0;
 
-    group_id = ops->get_device_group_id(seg, bus, devfn);
+    group_id = iommu_call(ops, get_device_group_id, seg, bus, devfn);
 
     pcidevs_lock();
     for_each_pdev( d, pdev )
     {
-        if ( (pdev->seg != seg) ||
-             ((pdev->bus == bus) && (pdev->devfn == devfn)) )
+        unsigned int b = pdev->bus;
+        unsigned int df = pdev->devfn;
+
+        if ( (pdev->seg != seg) || ((b == bus) && (df == devfn)) )
             continue;
 
-        if ( xsm_get_device_group(XSM_HOOK, (seg << 16) | (pdev->bus << 8) | pdev->devfn) )
+        if ( xsm_get_device_group(XSM_HOOK, (seg << 16) | (b << 8) | df) )
             continue;
 
-        sdev_id = ops->get_device_group_id(seg, pdev->bus, pdev->devfn);
+        sdev_id = iommu_call(ops, get_device_group_id, seg, b, df);
         if ( (sdev_id == group_id) && (i < max_sdevs) )
         {
-            bdf = 0;
-            bdf |= (pdev->bus & 0xff) << 16;
-            bdf |= (pdev->devfn & 0xff) << 8;
+            bdf = (b << 16) | (df << 8);
 
             if ( unlikely(copy_to_guest_offset(buf, i, &bdf, 1)) )
             {
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -142,7 +142,7 @@ unsigned int iommu_read_apic_from_ire(un
 int __init iommu_setup_hpet_msi(struct msi_desc *msi)
 {
     const struct iommu_ops *ops = iommu_get_ops();
-    return ops->setup_hpet_msi ? ops->setup_hpet_msi(msi) : -ENODEV;
+    return ops->setup_hpet_msi ? iommu_call(ops, setup_hpet_msi, msi) : -ENODEV;
 }
 
 void __hwdom_init arch_iommu_check_autotranslated_hwdom(struct domain *d)
@@ -403,7 +403,7 @@ int iommu_free_pgtables(struct domain *d
      * Pages will be moved to the free list below. So we want to
      * clear the root page-table to avoid any potential use after-free.
      */
-    hd->platform_ops->clear_root_pgtable(d);
+    iommu_vcall(hd->platform_ops, clear_root_pgtable, d);
 
     while ( (pg = page_list_remove_head(&hd->arch.pgtables.list)) )
     {

[PATCH 2/4] VT-d / x86: re-arrange cache syncing

[Jan Beulich]

The actual function should always have lived in core x86 code; move it
there, replacing get_cache_line_size() by readily available (except very
early during boot; see the code comment) data.

Drop the respective IOMMU hook, (re)introducing a respective boolean
instead. Replace a true and an almost open-coding instance of
iommu_sync_cache().

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Placing the function next to flush_area_local() exposes a curious
asymmetry between the SFENCE placements: sync_cache() has it after the
flush, while flush_area_local() has it before it. I think the latter one
is misplaced.

--- a/xen/arch/x86/flushtlb.c
+++ b/xen/arch/x86/flushtlb.c
@@ -11,6 +11,7 @@
 #include <xen/sched.h>
 #include <xen/smp.h>
 #include <xen/softirq.h>
+#include <asm/cache.h>
 #include <asm/flushtlb.h>
 #include <asm/invpcid.h>
 #include <asm/nops.h>
@@ -265,6 +266,57 @@ unsigned int flush_area_local(const void
     return flags;
 }
 
+void sync_cache(const void *addr, unsigned int size)
+{
+    /*
+     * This function may be called before current_cpu_data is established.
+     * Hence a fallback is needed to prevent the loop below becoming infinite.
+     */
+    unsigned int clflush_size = current_cpu_data.x86_clflush_size ?: 16;
+    const void *end = addr + size;
+
+    addr -= (unsigned long)addr & (clflush_size - 1);
+    for ( ; addr < end; addr += clflush_size )
+    {
+/*
+ * The arguments to a macro must not include preprocessor directives. Doing so
+ * results in undefined behavior, so we have to create some defines here in
+ * order to avoid it.
+ */
+#if defined(HAVE_AS_CLWB)
+# define CLWB_ENCODING "clwb %[p]"
+#elif defined(HAVE_AS_XSAVEOPT)
+# define CLWB_ENCODING "data16 xsaveopt %[p]" /* clwb */
+#else
+# define CLWB_ENCODING ".byte 0x66, 0x0f, 0xae, 0x30" /* clwb (%%rax) */
+#endif
+
+#define BASE_INPUT(addr) [p] "m" (*(const char *)(addr))
+#if defined(HAVE_AS_CLWB) || defined(HAVE_AS_XSAVEOPT)
+# define INPUT BASE_INPUT
+#else
+# define INPUT(addr) "a" (addr), BASE_INPUT(addr)
+#endif
+        /*
+         * Note regarding the use of NOP_DS_PREFIX: it's faster to do a clflush
+         * + prefix than a clflush + nop, and hence the prefix is added instead
+         * of letting the alternative framework fill the gap by appending nops.
+         */
+        alternative_io_2(".byte " __stringify(NOP_DS_PREFIX) "; clflush %[p]",
+                         "data16 clflush %[p]", /* clflushopt */
+                         X86_FEATURE_CLFLUSHOPT,
+                         CLWB_ENCODING,
+                         X86_FEATURE_CLWB, /* no outputs */,
+                         INPUT(addr));
+#undef INPUT
+#undef BASE_INPUT
+#undef CLWB_ENCODING
+    }
+
+    alternative_2("", "sfence", X86_FEATURE_CLFLUSHOPT,
+                      "sfence", X86_FEATURE_CLWB);
+}
+
 unsigned int guest_flush_tlb_flags(const struct domain *d)
 {
     bool shadow = paging_mode_shadow(d);
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -202,54 +202,6 @@ static void check_cleanup_domid_map(stru
     }
 }
 
-static void sync_cache(const void *addr, unsigned int size)
-{
-    static unsigned long clflush_size = 0;
-    const void *end = addr + size;
-
-    if ( clflush_size == 0 )
-        clflush_size = get_cache_line_size();
-
-    addr -= (unsigned long)addr & (clflush_size - 1);
-    for ( ; addr < end; addr += clflush_size )
-/*
- * The arguments to a macro must not include preprocessor directives. Doing so
- * results in undefined behavior, so we have to create some defines here in
- * order to avoid it.
- */
-#if defined(HAVE_AS_CLWB)
-# define CLWB_ENCODING "clwb %[p]"
-#elif defined(HAVE_AS_XSAVEOPT)
-# define CLWB_ENCODING "data16 xsaveopt %[p]" /* clwb */
-#else
-# define CLWB_ENCODING ".byte 0x66, 0x0f, 0xae, 0x30" /* clwb (%%rax) */
-#endif
-
-#define BASE_INPUT(addr) [p] "m" (*(const char *)(addr))
-#if defined(HAVE_AS_CLWB) || defined(HAVE_AS_XSAVEOPT)
-# define INPUT BASE_INPUT
-#else
-# define INPUT(addr) "a" (addr), BASE_INPUT(addr)
-#endif
-        /*
-         * Note regarding the use of NOP_DS_PREFIX: it's faster to do a clflush
-         * + prefix than a clflush + nop, and hence the prefix is added instead
-         * of letting the alternative framework fill the gap by appending nops.
-         */
-        alternative_io_2(".byte " __stringify(NOP_DS_PREFIX) "; clflush %[p]",
-                         "data16 clflush %[p]", /* clflushopt */
-                         X86_FEATURE_CLFLUSHOPT,
-                         CLWB_ENCODING,
-                         X86_FEATURE_CLWB, /* no outputs */,
-                         INPUT(addr));
-#undef INPUT
-#undef BASE_INPUT
-#undef CLWB_ENCODING
-
-    alternative_2("", "sfence", X86_FEATURE_CLFLUSHOPT,
-                      "sfence", X86_FEATURE_CLWB);
-}
-
 /* Allocate page table, return its machine address */
 uint64_t alloc_pgtable_maddr(unsigned long npages, nodeid_t node)
 {
@@ -268,8 +220,7 @@ uint64_t alloc_pgtable_maddr(unsigned lo
 
         clear_page(vaddr);
 
-        if ( (iommu_ops.init ? &iommu_ops : &vtd_ops)->sync_cache )
-            sync_cache(vaddr, PAGE_SIZE);
+        iommu_sync_cache(vaddr, PAGE_SIZE);
         unmap_domain_page(vaddr);
         cur_pg++;
     }
@@ -1295,7 +1246,7 @@ int __init iommu_alloc(struct acpi_drhd_
     iommu->nr_pt_levels = agaw_to_level(agaw);
 
     if ( !ecap_coherent(iommu->ecap) )
-        vtd_ops.sync_cache = sync_cache;
+        iommu_non_coherent = true;
 
     /* allocate domain id bitmap */
     nr_dom = cap_ndoms(iommu->cap);
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -28,6 +28,7 @@
 
 const struct iommu_init_ops *__initdata iommu_init_ops;
 struct iommu_ops __read_mostly iommu_ops;
+bool __read_mostly iommu_non_coherent;
 
 enum iommu_intremap __read_mostly iommu_intremap = iommu_intremap_full;
 
@@ -435,8 +436,7 @@ struct page_info *iommu_alloc_pgtable(st
     p = __map_domain_page(pg);
     clear_page(p);
 
-    if ( hd->platform_ops->sync_cache )
-        iommu_vcall(hd->platform_ops, sync_cache, p, PAGE_SIZE);
+    iommu_sync_cache(p, PAGE_SIZE);
 
     unmap_domain_page(p);
 
--- a/xen/include/asm-x86/cache.h
+++ b/xen/include/asm-x86/cache.h
@@ -11,4 +11,10 @@
 
 #define __read_mostly __section(".data.read_mostly")
 
+#ifndef __ASSEMBLY__
+
+void sync_cache(const void *addr, unsigned int size);
+
+#endif
+
 #endif
--- a/xen/include/asm-x86/iommu.h
+++ b/xen/include/asm-x86/iommu.h
@@ -19,6 +19,7 @@
 #include <xen/mem_access.h>
 #include <xen/spinlock.h>
 #include <asm/apicdef.h>
+#include <asm/cache.h>
 #include <asm/processor.h>
 #include <asm/hvm/vmx/vmcs.h>
 
@@ -134,12 +135,13 @@ extern bool untrusted_msi;
 int pi_update_irte(const struct pi_desc *pi_desc, const struct pirq *pirq,
                    const uint8_t gvec);
 
-#define iommu_sync_cache(addr, size) ({                 \
-    const struct iommu_ops *ops = iommu_get_ops();      \
-                                                        \
-    if ( ops->sync_cache )                              \
-        iommu_vcall(ops, sync_cache, addr, size);       \
-})
+extern bool iommu_non_coherent;
+
+static inline void iommu_sync_cache(const void *addr, unsigned int size)
+{
+    if ( iommu_non_coherent )
+        sync_cache(addr, size);
+}
 
 int __must_check iommu_free_pgtables(struct domain *d);
 struct page_info *__must_check iommu_alloc_pgtable(struct domain *d);
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -268,7 +268,6 @@ struct iommu_ops {
     int (*setup_hpet_msi)(struct msi_desc *);
 
     int (*adjust_irq_affinities)(void);
-    void (*sync_cache)(const void *addr, unsigned int size);
     void (*clear_root_pgtable)(struct domain *d);
     int (*update_ire_from_msi)(struct msi_desc *msi_desc, struct msi_msg *msg);
 #endif /* CONFIG_X86 */
--- a/xen/drivers/passthrough/vtd/extern.h
+++ b/xen/drivers/passthrough/vtd/extern.h
@@ -76,7 +76,6 @@ int __must_check qinval_device_iotlb_syn
                                           struct pci_dev *pdev,
                                           u16 did, u16 size, u64 addr);
 
-unsigned int get_cache_line_size(void);
 void flush_all_cache(void);
 
 uint64_t alloc_pgtable_maddr(unsigned long npages, nodeid_t node);
--- a/xen/drivers/passthrough/vtd/x86/vtd.c
+++ b/xen/drivers/passthrough/vtd/x86/vtd.c
@@ -47,11 +47,6 @@ void unmap_vtd_domain_page(const void *v
     unmap_domain_page(va);
 }
 
-unsigned int get_cache_line_size(void)
-{
-    return ((cpuid_ebx(1) >> 8) & 0xff) * 8;
-}
-
 void flush_all_cache()
 {
     wbinvd();

[PATCH 3/4] VT-d: replace flush_all_cache()

[Jan Beulich]

Let's use infrastructure we have available instead of an open-coded
wbinvd() invocation.

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

--- a/xen/drivers/passthrough/vtd/extern.h
+++ b/xen/drivers/passthrough/vtd/extern.h
@@ -76,8 +76,6 @@ int __must_check qinval_device_iotlb_syn
                                           struct pci_dev *pdev,
                                           u16 did, u16 size, u64 addr);
 
-void flush_all_cache(void);
-
 uint64_t alloc_pgtable_maddr(unsigned long npages, nodeid_t node);
 void free_pgtable_maddr(u64 maddr);
 void *map_vtd_domain_page(u64 maddr);
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -591,7 +591,8 @@ static int __must_check iommu_flush_all(
     bool_t flush_dev_iotlb;
     int rc = 0;
 
-    flush_all_cache();
+    flush_local(FLUSH_CACHE);
+
     for_each_drhd_unit ( drhd )
     {
         int context_rc, iotlb_rc;
--- a/xen/drivers/passthrough/vtd/x86/vtd.c
+++ b/xen/drivers/passthrough/vtd/x86/vtd.c
@@ -46,8 +46,3 @@ void unmap_vtd_domain_page(const void *v
 {
     unmap_domain_page(va);
 }
-
-void flush_all_cache()
-{
-    wbinvd();
-}

[PATCH 4/4] libxc: correct bounce direction in xc_get_device_group()

[Jan Beulich]

The array of IDs is an output.

Fixes: 79647c5bc9c6 ("libxc: convert domctl interfaces over to hypercall buffers")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Clearly the function, including its Python wrapper, cannot have been
used by anything for many years. I wonder whether that isn't good enough
a reason to sanitize the layout of the array elements: Right now they
have BDF in bits 8...23, when conventionally this would be bits 0...15.

--- a/tools/libs/ctrl/xc_domain.c
+++ b/tools/libs/ctrl/xc_domain.c
@@ -1546,7 +1546,8 @@ int xc_get_device_group(
 {
     int rc;
     DECLARE_DOMCTL;
-    DECLARE_HYPERCALL_BOUNCE(sdev_array, max_sdevs * sizeof(*sdev_array), XC_HYPERCALL_BUFFER_BOUNCE_IN);
+    DECLARE_HYPERCALL_BOUNCE(sdev_array, max_sdevs * sizeof(*sdev_array),
+                             XC_HYPERCALL_BUFFER_BOUNCE_OUT);
 
     if ( xc_hypercall_bounce_pre(xch, sdev_array) )
     {

Re: [PATCH 4/4] libxc: correct bounce direction in xc_get_device_group()

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 274 bytes --]

On 01.12.21 10:42, Jan Beulich wrote:
> The array of IDs is an output.
> 
> Fixes: 79647c5bc9c6 ("libxc: convert domctl interfaces over to hypercall buffers")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 3/4] VT-d: replace flush_all_cache()

[Andrew Cooper]

On 01/12/2021 09:41, Jan Beulich wrote:
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -591,7 +591,8 @@ static int __must_check iommu_flush_all(
>      bool_t flush_dev_iotlb;
>      int rc = 0;
>  
> -    flush_all_cache();
> +    flush_local(FLUSH_CACHE);

While I agree that the conversion is an improvement, the logic still
looks totally bogus.

I can believe that it might have been a stopgap to fix problems before
we identified the need for sync_cache() for non-coherent IOMMUs, but
there's no need I can spot for any WBINVDs on any of these paths.

I'm fairly sure this should just be dropped, and Xen will get faster as
a result.

~Andrew

Re: [PATCH 2/4] VT-d / x86: re-arrange cache syncing

[Andrew Cooper]

On 01/12/2021 09:40, Jan Beulich wrote:
> The actual function should always have lived in core x86 code; move it
> there, replacing get_cache_line_size() by readily available (except very
> early during boot; see the code comment) data.
>
> Drop the respective IOMMU hook, (re)introducing a respective boolean
> instead. Replace a true and an almost open-coding instance of
> iommu_sync_cache().

Coherency (or not) is a per-IOMMU property, not a global platform
property.  iGPU IOMMUs are very different to those the uncore, and
there's no reason to presume that IOMMUs in the PCH would have the same
properties as those in the uncore.

Given how expensive sync_cache() is, we cannot afford to be using it for
coherent IOMMUs in a mixed system.

This wants to be a boolean in arch_iommu.


> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Placing the function next to flush_area_local() exposes a curious
> asymmetry between the SFENCE placements: sync_cache() has it after the
> flush, while flush_area_local() has it before it. I think the latter one
> is misplaced.

Wow this is a mess.

First and foremost, AMD state that on pre-CLFLUSHOPT parts, CLFLUSH is
unordered with ~anything (including SFENCE), and need bounding with
MFENCE on both sides.  We definitely aren't doing this correctly right now.


AMD explicitly states that SFENCE drains the store and WC buffers (i.e.
make stuff instantaneously globally visible).  Intel does not, and
merely guarantees ordering.

A leading SFENCE would only make sense if there were WC concerns, but
both manuals say that the memory type doesn't matter, so I can't see a
justification for it.

What does matter, from the IOMMU's point of view, is that the line has
been written back (or evicted on pre-CLWB parts) before the IOTLB
invalidation occurs.  The invalidation will be a write to a different
address, which is why the trailing SFENCE is necessary, as CLFLUSHOPT
isn't ordered with respect to unaliasing writes.


On a more minor note, both Intel and AMD say that CLFLUSH* are permitted
to target an execute-only code segment, so we need a fix in
hvmemul_cache_op()'s use of hvmemul_virtual_to_linear(...,
hvm_access_read, ...) which will currently #GP in this case.

>
> --- a/xen/arch/x86/flushtlb.c
> +++ b/xen/arch/x86/flushtlb.c
> @@ -11,6 +11,7 @@
>  #include <xen/sched.h>
>  #include <xen/smp.h>
>  #include <xen/softirq.h>
> +#include <asm/cache.h>
>  #include <asm/flushtlb.h>
>  #include <asm/invpcid.h>
>  #include <asm/nops.h>
> @@ -265,6 +266,57 @@ unsigned int flush_area_local(const void
>      return flags;
>  }
>  
> +void sync_cache(const void *addr, unsigned int size)

Can we name this cache_writeback()?  sync is very generic, and it really
doesn't want confusing cache_flush().

~Andrew

Re: [PATCH 4/4] libxc: correct bounce direction in xc_get_device_group()

[Andrew Cooper]

On 01/12/2021 09:42, Jan Beulich wrote:
> The array of IDs is an output.
>
> Fixes: 79647c5bc9c6 ("libxc: convert domctl interfaces over to hypercall buffers")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Clearly the function, including its Python wrapper, cannot have been
> used by anything for many years. I wonder whether that isn't good enough
> a reason to sanitize the layout of the array elements: Right now they
> have BDF in bits 8...23, when conventionally this would be bits 0...15.

There is a lot of WTF with this hypercall.  It's obviously an attempt to
do the thing that Linux calls IOMMU groups now, except that the correct
way to do this would be for the group ID to be the unit of
assignment/deassignment.  (We need to do this anyway for other reasons.)

The last user was deleted with Xend (2013), which suggests that it was
broken for 3 years due to the incorrect bounce direction (2010).

Furthermore, it will arbitrarily fail if targetting domains without an
IOMMU configured, but everything else seems to be invariant of the
passed domain.  This should clearly be sysctl, not a domctl.


I suggest ripping all of this infrastructure out.  It's clearly unused
(and broken in Xen too - see patch 1), and not something which should be
used in this form in the future.

~Andrew

Re: [PATCH 1/4] IOMMU/x86: switch to alternatives-call patching in further instances

[Andrew Cooper]

On 01/12/2021 09:39, Jan Beulich wrote:
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -1457,24 +1462,24 @@ static int iommu_get_device_group(
>      if ( !is_iommu_enabled(d) || !ops->get_device_group_id )
>          return 0;
>  
> -    group_id = ops->get_device_group_id(seg, bus, devfn);
> +    group_id = iommu_call(ops, get_device_group_id, seg, bus, devfn);

So I was going to suggest adjusting this to use more pci_sbdf_t, but the
Intel implementation can fail and return -1.

However, given the observations in patch 4, I'd instead recommend
dropping all of this.

Everything unrelated to get_device_group() looks fine.

~Andrew

Re: [PATCH 1/4] IOMMU/x86: switch to alternatives-call patching in further instances

[Jan Beulich]

On 01.12.2021 16:13, Andrew Cooper wrote:
> On 01/12/2021 09:39, Jan Beulich wrote:
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -1457,24 +1462,24 @@ static int iommu_get_device_group(
>>      if ( !is_iommu_enabled(d) || !ops->get_device_group_id )
>>          return 0;
>>  
>> -    group_id = ops->get_device_group_id(seg, bus, devfn);
>> +    group_id = iommu_call(ops, get_device_group_id, seg, bus, devfn);
> 
> So I was going to suggest adjusting this to use more pci_sbdf_t, but the
> Intel implementation can fail and return -1.

How are the two aspects related? Wouldn't you mean the parameter of the
function to become pci_sbdf_t? If so, I'd view this as an orthogonal
change. I'll reply to the question of removal on the subthread of patch 4.

Jan

Re: [PATCH 4/4] libxc: correct bounce direction in xc_get_device_group()

[Jan Beulich]

On 01.12.2021 16:11, Andrew Cooper wrote:
> On 01/12/2021 09:42, Jan Beulich wrote:
>> The array of IDs is an output.
>>
>> Fixes: 79647c5bc9c6 ("libxc: convert domctl interfaces over to hypercall buffers")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> Clearly the function, including its Python wrapper, cannot have been
>> used by anything for many years. I wonder whether that isn't good enough
>> a reason to sanitize the layout of the array elements: Right now they
>> have BDF in bits 8...23, when conventionally this would be bits 0...15.
> 
> There is a lot of WTF with this hypercall.  It's obviously an attempt to
> do the thing that Linux calls IOMMU groups now, except that the correct
> way to do this would be for the group ID to be the unit of
> assignment/deassignment.  (We need to do this anyway for other reasons.)
> 
> The last user was deleted with Xend (2013), which suggests that it was
> broken for 3 years due to the incorrect bounce direction (2010).
> 
> Furthermore, it will arbitrarily fail if targetting domains without an
> IOMMU configured, but everything else seems to be invariant of the
> passed domain.  This should clearly be sysctl, not a domctl.
> 
> 
> I suggest ripping all of this infrastructure out.  It's clearly unused
> (and broken in Xen too - see patch 1),

I've not seen you point out any breakage in reply to patch 1, unless you
mean VT-d's returning of -1 (which you didn't point out as broken, but
which I can see would lead to misbehavior).

> and not something which should be used in this form in the future.

I didn't think the concept here was wrong. What's missing is the tool
stack actually making use of this plus a way to do assignment in groups.
Iirc the latter was something Paul had started work on before leaving
Citrix? (That's leaving aside the bug you mention plus potential further
ones.)

Paul, Ian, Wei - what are your thoughts towards Andrew's proposal?

Jan

Re: [PATCH 3/4] VT-d: replace flush_all_cache()

[Jan Beulich]

On 01.12.2021 14:02, Andrew Cooper wrote:
> On 01/12/2021 09:41, Jan Beulich wrote:
>> --- a/xen/drivers/passthrough/vtd/iommu.c
>> +++ b/xen/drivers/passthrough/vtd/iommu.c
>> @@ -591,7 +591,8 @@ static int __must_check iommu_flush_all(
>>      bool_t flush_dev_iotlb;
>>      int rc = 0;
>>  
>> -    flush_all_cache();
>> +    flush_local(FLUSH_CACHE);
> 
> While I agree that the conversion is an improvement, the logic still
> looks totally bogus.
> 
> I can believe that it might have been a stopgap to fix problems before
> we identified the need for sync_cache() for non-coherent IOMMUs, but
> there's no need I can spot for any WBINVDs on any of these paths.
> 
> I'm fairly sure this should just be dropped, and Xen will get faster as
> a result.

Kevin, thoughts? I have to admit I'm hesitant to remove such code, when
there's no clear indication why it's there. I'm also not sure how much
of a win the dropping would be, considering the places where this
function gets called from.

Jan

Re: [PATCH 2/4] VT-d / x86: re-arrange cache syncing

[Jan Beulich]

On 01.12.2021 15:39, Andrew Cooper wrote:
> On 01/12/2021 09:40, Jan Beulich wrote:
>> The actual function should always have lived in core x86 code; move it
>> there, replacing get_cache_line_size() by readily available (except very
>> early during boot; see the code comment) data.
>>
>> Drop the respective IOMMU hook, (re)introducing a respective boolean
>> instead. Replace a true and an almost open-coding instance of
>> iommu_sync_cache().
> 
> Coherency (or not) is a per-IOMMU property, not a global platform
> property.  iGPU IOMMUs are very different to those the uncore, and
> there's no reason to presume that IOMMUs in the PCH would have the same
> properties as those in the uncore.
> 
> Given how expensive sync_cache() is, we cannot afford to be using it for
> coherent IOMMUs in a mixed system.
> 
> This wants to be a boolean in arch_iommu.

That's a valid consideration, but may not be as easy as it may seem on
the surface. Certainly not something I could promise to find time for
soon. And definitely separate from the specific change here.

>> ---
>> Placing the function next to flush_area_local() exposes a curious
>> asymmetry between the SFENCE placements: sync_cache() has it after the
>> flush, while flush_area_local() has it before it. I think the latter one
>> is misplaced.
> 
> Wow this is a mess.
> 
> First and foremost, AMD state that on pre-CLFLUSHOPT parts, CLFLUSH is
> unordered with ~anything (including SFENCE), and need bounding with
> MFENCE on both sides.  We definitely aren't doing this correctly right now.
> 
> 
> AMD explicitly states that SFENCE drains the store and WC buffers (i.e.
> make stuff instantaneously globally visible).  Intel does not, and
> merely guarantees ordering.
> 
> A leading SFENCE would only make sense if there were WC concerns, but
> both manuals say that the memory type doesn't matter, so I can't see a
> justification for it.
> 
> What does matter, from the IOMMU's point of view, is that the line has
> been written back (or evicted on pre-CLWB parts) before the IOTLB
> invalidation occurs.  The invalidation will be a write to a different
> address, which is why the trailing SFENCE is necessary, as CLFLUSHOPT
> isn't ordered with respect to unaliasing writes.

IOW for the purposes of this change all is correct, and everything else
will require taking care of separately.

> On a more minor note, both Intel and AMD say that CLFLUSH* are permitted
> to target an execute-only code segment, so we need a fix in
> hvmemul_cache_op()'s use of hvmemul_virtual_to_linear(...,
> hvm_access_read, ...) which will currently #GP in this case.

Hmm, this specific case would seem to require to simply use hvm_access_none
(like hvmemul_tlb_op() already does), except for CLWB.

But then hvm_vcpu_virtual_to_linear() also doesn't look to handle
hvm_access_insn_fetch correctly for data segments. Changing of which would
in turn require to first audit all use sites, to make sure we don't break
anything.

I'll see about doing both.

>> --- a/xen/arch/x86/flushtlb.c
>> +++ b/xen/arch/x86/flushtlb.c
>> @@ -11,6 +11,7 @@
>>  #include <xen/sched.h>
>>  #include <xen/smp.h>
>>  #include <xen/softirq.h>
>> +#include <asm/cache.h>
>>  #include <asm/flushtlb.h>
>>  #include <asm/invpcid.h>
>>  #include <asm/nops.h>
>> @@ -265,6 +266,57 @@ unsigned int flush_area_local(const void
>>      return flags;
>>  }
>>  
>> +void sync_cache(const void *addr, unsigned int size)
> 
> Can we name this cache_writeback()?  sync is very generic, and it really
> doesn't want confusing cache_flush().

Oh, sure, can do. As long as you don't insist on also changing
iommu_sync_cache(): While purely mechanical, this would bloat the
patch by quite a bit.

Bottom line: This last item is the only change to the actual patch;
everything else will require further work yielding separate patches.

Jan

RE: [PATCH 2/4] VT-d / x86: re-arrange cache syncing

[Tian, Kevin]

> From: Jan Beulich <jbeulich@suse.com>
> Sent: Thursday, December 2, 2021 5:19 PM
> 
> On 01.12.2021 15:39, Andrew Cooper wrote:
> > On 01/12/2021 09:40, Jan Beulich wrote:
> >> The actual function should always have lived in core x86 code; move it
> >> there, replacing get_cache_line_size() by readily available (except very
> >> early during boot; see the code comment) data.
> >>
> >> Drop the respective IOMMU hook, (re)introducing a respective boolean
> >> instead. Replace a true and an almost open-coding instance of
> >> iommu_sync_cache().
> >
> > Coherency (or not) is a per-IOMMU property, not a global platform
> > property.  iGPU IOMMUs are very different to those the uncore, and
> > there's no reason to presume that IOMMUs in the PCH would have the
> same
> > properties as those in the uncore.
> >
> > Given how expensive sync_cache() is, we cannot afford to be using it for
> > coherent IOMMUs in a mixed system.
> >
> > This wants to be a boolean in arch_iommu.
> 
> That's a valid consideration, but may not be as easy as it may seem on
> the surface. Certainly not something I could promise to find time for
> soon. And definitely separate from the specific change here.

I'm fine with this patch if you prefer to a staging approach to improve it.
By any means this patch doesn't make things worse.

> 
> >> ---
> >> Placing the function next to flush_area_local() exposes a curious
> >> asymmetry between the SFENCE placements: sync_cache() has it after the
> >> flush, while flush_area_local() has it before it. I think the latter one
> >> is misplaced.
> >
> > Wow this is a mess.
> >
> > First and foremost, AMD state that on pre-CLFLUSHOPT parts, CLFLUSH is
> > unordered with ~anything (including SFENCE), and need bounding with
> > MFENCE on both sides.  We definitely aren't doing this correctly right now.
> >
> >
> > AMD explicitly states that SFENCE drains the store and WC buffers (i.e.
> > make stuff instantaneously globally visible).  Intel does not, and
> > merely guarantees ordering.
> >
> > A leading SFENCE would only make sense if there were WC concerns, but
> > both manuals say that the memory type doesn't matter, so I can't see a
> > justification for it.
> >
> > What does matter, from the IOMMU's point of view, is that the line has
> > been written back (or evicted on pre-CLWB parts) before the IOTLB
> > invalidation occurs.  The invalidation will be a write to a different
> > address, which is why the trailing SFENCE is necessary, as CLFLUSHOPT
> > isn't ordered with respect to unaliasing writes.
> 
> IOW for the purposes of this change all is correct, and everything else
> will require taking care of separately.
> 

Same for this part. btw Linux does mfence both before and after clflush.

Thanks
Kevin

RE: [PATCH 3/4] VT-d: replace flush_all_cache()

[Tian, Kevin]

> From: Jan Beulich <jbeulich@suse.com>
> Sent: Thursday, December 2, 2021 4:48 PM
> 
> On 01.12.2021 14:02, Andrew Cooper wrote:
> > On 01/12/2021 09:41, Jan Beulich wrote:
> >> --- a/xen/drivers/passthrough/vtd/iommu.c
> >> +++ b/xen/drivers/passthrough/vtd/iommu.c
> >> @@ -591,7 +591,8 @@ static int __must_check iommu_flush_all(
> >>      bool_t flush_dev_iotlb;
> >>      int rc = 0;
> >>
> >> -    flush_all_cache();
> >> +    flush_local(FLUSH_CACHE);
> >
> > While I agree that the conversion is an improvement, the logic still
> > looks totally bogus.
> >
> > I can believe that it might have been a stopgap to fix problems before
> > we identified the need for sync_cache() for non-coherent IOMMUs, but
> > there's no need I can spot for any WBINVDs on any of these paths.
> >
> > I'm fairly sure this should just be dropped, and Xen will get faster as
> > a result.
> 
> Kevin, thoughts? I have to admit I'm hesitant to remove such code, when
> there's no clear indication why it's there. I'm also not sure how much
> of a win the dropping would be, considering the places where this
> function gets called from.
> 

me too. Could Andrew elaborate further on "fairly sure" part?

Thanks
Kevin