[Xen-devel] [PATCH for-4.13 v3 0/7] Unbreak evaluate_nospec() and livepatching

[Xen-devel] [PATCH for-4.13 v3 0/7] Unbreak evaluate_nospec() and livepatching

[Andrew Cooper]

This resolves an oustanding blocker for 4.13, fixing both the code generation
for evaulate_nospec(), and making the resulting hypervisor able to be
livepatched.

Andrew Cooper (6):
  x86/nospec: Two trivial fixes
  xen/nospec: Use always_inline to fix code gen for evaluate_nospec
  xen/nospec: Introduce CONFIG_SPECULATIVE_HARDEN_BRANCH
  x86/nospec: Rename and rework l1tf-barrier as branch-harden
  x86/nospec: Move array_index_mask_nospec() into nospec.h
  x86/nospec: Optimise array_index_mask_nospec() for power-of-2 arrays

Ross Lagerwall (1):
  x86/livepatch: Fail the build if duplicate symbols exist

 docs/misc/xen-command-line.pandoc   | 11 +++++-----
 xen/arch/x86/Makefile               |  1 +
 xen/arch/x86/domain.c               |  2 +-
 xen/arch/x86/pv/mm.h                | 12 +++++------
 xen/arch/x86/spec_ctrl.c            | 18 +++++++---------
 xen/common/Kconfig                  | 41 +++++++++++++++++++++++++++++++++++--
 xen/include/asm-x86/cpufeatures.h   |  2 +-
 xen/include/asm-x86/event.h         |  2 +-
 xen/include/asm-x86/guest_pt.h      | 28 ++++++++++++++-----------
 xen/include/asm-x86/hvm/nestedhvm.h |  2 +-
 xen/include/asm-x86/nospec.h        | 41 ++++++++++++++++++++++++++++++++++---
 xen/include/asm-x86/paging.h        |  2 +-
 xen/include/asm-x86/spec_ctrl.h     |  2 +-
 xen/include/asm-x86/system.h        | 24 ----------------------
 xen/include/xen/config.h            |  1 +
 xen/include/xen/nospec.h            |  6 ++++--
 xen/include/xen/sched.h             | 20 +++++++++---------
 xen/tools/kconfig/allrandom.config  |  1 +
 xen/tools/symbols.c                 | 11 ++++++++--
 19 files changed, 143 insertions(+), 84 deletions(-)

-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v3 1/7] x86/nospec: Two trivial fixes

[Andrew Cooper]

The include of asm/cpuid.h in spec_ctrl.c was an artefact of an older version
of c/s 3860d5534df, and is not used in its current incarnation.

Fix a typo in a comment.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Juergen Gross <jgross@suse.com>

v3:
 * New
---
 xen/arch/x86/spec_ctrl.c     | 1 -
 xen/include/asm-x86/nospec.h | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
index 731d5a767b..ee5439a371 100644
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -21,7 +21,6 @@
 #include <xen/lib.h>
 #include <xen/warning.h>
 
-#include <asm/cpuid.h>
 #include <asm/microcode.h>
 #include <asm/msr.h>
 #include <asm/processor.h>
diff --git a/xen/include/asm-x86/nospec.h b/xen/include/asm-x86/nospec.h
index 2aa47b3455..427b5ff9df 100644
--- a/xen/include/asm-x86/nospec.h
+++ b/xen/include/asm-x86/nospec.h
@@ -15,7 +15,7 @@ static always_inline bool barrier_nospec_true(void)
     return true;
 }
 
-/* Allow to protect evaluation of conditionasl with respect to speculation */
+/* Allow to protect evaluation of conditionals with respect to speculation */
 static always_inline bool evaluate_nospec(bool condition)
 {
     return condition ? barrier_nospec_true() : !barrier_nospec_true();
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/7] x86/nospec: Two trivial fixes

[Jan Beulich]

On 23.10.2019 15:58, Andrew Cooper wrote:
> The include of asm/cpuid.h in spec_ctrl.c was an artefact of an older version
> of c/s 3860d5534df, and is not used in its current incarnation.
> 
> Fix a typo in a comment.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/7] x86/nospec: Two trivial fixes

[Jürgen Groß]

On 23.10.19 15:58, Andrew Cooper wrote:
> The include of asm/cpuid.h in spec_ctrl.c was an artefact of an older version
> of c/s 3860d5534df, and is not used in its current incarnation.
> 
> Fix a typo in a comment.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v3 2/7] xen/nospec: Use always_inline to fix code gen for evaluate_nospec

[Andrew Cooper]

evaluate_nospec() is incredibly fragile, and this is one giant bodge.

To correctly protect jumps, the generated code needs to be of the form:

    cmp/test <cond>
    jcc 1f
    lfence
    ...
 1: lfence
    ...

Critically, the lfence must be at the head of both basic blocks, later in the
instruction stream than the conditional jump in need of protection.

When a static inline is involved, the optimiser decides to be clever and
rearranges the code as:

 pred:
    lfence
    <calculate cond>
    ret

    call pred
    cmp $0, %eax
    jcc 1f
    ...
 1: ...

which breaks the speculative safety.

Any use of evaluate_nospec() needs all static inline predicates which use it
to be declared always_inline to prevent the optimiser having the flexibility
to generate unsafe code.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Wei Liu <wl@xen.org>
CC: Julien Grall <julien@xen.org>
CC: Juergen Gross <jgross@suse.com>

This is the transitive set of predicates which I can spot which need
protecting.  There are probably ones I've missed.  Personally, I'm -1 for this
approach, but the only other option for 4.13 is to revert it all to unbreak
livepatching.

v3:
 * New
---
 xen/arch/x86/domain.c               |  2 +-
 xen/arch/x86/pv/mm.h                | 12 ++++++------
 xen/include/asm-x86/event.h         |  2 +-
 xen/include/asm-x86/guest_pt.h      | 28 ++++++++++++++++------------
 xen/include/asm-x86/hvm/nestedhvm.h |  2 +-
 xen/include/asm-x86/paging.h        |  2 +-
 xen/include/xen/sched.h             | 20 ++++++++++----------
 7 files changed, 36 insertions(+), 32 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index c8d7f491ea..1b88cc2d68 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1699,7 +1699,7 @@ static void _update_runstate_area(struct vcpu *v)
  * regular per-CPU GDT frame to appear with selectors at the appropriate
  * offset.
  */
-static inline bool need_full_gdt(const struct domain *d)
+static always_inline bool need_full_gdt(const struct domain *d)
 {
     return is_pv_domain(d) && !is_idle_domain(d);
 }
diff --git a/xen/arch/x86/pv/mm.h b/xen/arch/x86/pv/mm.h
index 2d427b418d..a1bd473b29 100644
--- a/xen/arch/x86/pv/mm.h
+++ b/xen/arch/x86/pv/mm.h
@@ -88,8 +88,8 @@ static inline bool update_intpte(intpte_t *p, intpte_t old, intpte_t new,
                   _t ## e_get_intpte(_o), _t ## e_get_intpte(_n),   \
                   (_m), (_v), (_ad))
 
-static inline l1_pgentry_t adjust_guest_l1e(l1_pgentry_t l1e,
-                                            const struct domain *d)
+static always_inline l1_pgentry_t adjust_guest_l1e(l1_pgentry_t l1e,
+                                                   const struct domain *d)
 {
     if ( likely(l1e_get_flags(l1e) & _PAGE_PRESENT) &&
          likely(!is_pv_32bit_domain(d)) )
@@ -120,8 +120,8 @@ static inline l2_pgentry_t adjust_guest_l2e(l2_pgentry_t l2e,
     return l2e;
 }
 
-static inline l3_pgentry_t adjust_guest_l3e(l3_pgentry_t l3e,
-                                            const struct domain *d)
+static always_inline l3_pgentry_t adjust_guest_l3e(l3_pgentry_t l3e,
+                                                   const struct domain *d)
 {
     if ( likely(l3e_get_flags(l3e) & _PAGE_PRESENT) )
         l3e_add_flags(l3e, (likely(!is_pv_32bit_domain(d))
@@ -140,8 +140,8 @@ static inline l3_pgentry_t unadjust_guest_l3e(l3_pgentry_t l3e,
     return l3e;
 }
 
-static inline l4_pgentry_t adjust_guest_l4e(l4_pgentry_t l4e,
-                                            const struct domain *d)
+static always_inline l4_pgentry_t adjust_guest_l4e(l4_pgentry_t l4e,
+                                                   const struct domain *d)
 {
     /*
      * When shadowing an L4 behind the guests back (e.g. for per-pcpu
diff --git a/xen/include/asm-x86/event.h b/xen/include/asm-x86/event.h
index 2f6ea54bcb..98a85233cb 100644
--- a/xen/include/asm-x86/event.h
+++ b/xen/include/asm-x86/event.h
@@ -20,7 +20,7 @@ static inline int vcpu_event_delivery_is_enabled(struct vcpu *v)
 }
 
 int hvm_local_events_need_delivery(struct vcpu *v);
-static inline int local_events_need_delivery(void)
+static always_inline bool local_events_need_delivery(void)
 {
     struct vcpu *v = current;
 
diff --git a/xen/include/asm-x86/guest_pt.h b/xen/include/asm-x86/guest_pt.h
index 8684b83fd6..6ab2041e48 100644
--- a/xen/include/asm-x86/guest_pt.h
+++ b/xen/include/asm-x86/guest_pt.h
@@ -202,7 +202,7 @@ static inline guest_l4e_t guest_l4e_from_gfn(gfn_t gfn, u32 flags)
 
 /* Which pagetable features are supported on this vcpu? */
 
-static inline bool guest_can_use_l2_superpages(const struct vcpu *v)
+static always_inline bool guest_can_use_l2_superpages(const struct vcpu *v)
 {
     /*
      * PV guests use Xen's paging settings.  Being 4-level, 2M
@@ -218,7 +218,7 @@ static inline bool guest_can_use_l2_superpages(const struct vcpu *v)
             (v->arch.hvm.guest_cr[4] & X86_CR4_PSE));
 }
 
-static inline bool guest_can_use_l3_superpages(const struct domain *d)
+static always_inline bool guest_can_use_l3_superpages(const struct domain *d)
 {
     /*
      * There are no control register settings for the hardware pagewalk on the
@@ -252,7 +252,7 @@ static inline bool guest_can_use_pse36(const struct domain *d)
     return paging_mode_hap(d) && cpu_has_pse36;
 }
 
-static inline bool guest_nx_enabled(const struct vcpu *v)
+static always_inline bool guest_nx_enabled(const struct vcpu *v)
 {
     if ( GUEST_PAGING_LEVELS == 2 ) /* NX has no effect witout CR4.PAE. */
         return false;
@@ -261,23 +261,23 @@ static inline bool guest_nx_enabled(const struct vcpu *v)
     return is_pv_vcpu(v) ? cpu_has_nx : hvm_nx_enabled(v);
 }
 
-static inline bool guest_wp_enabled(const struct vcpu *v)
+static always_inline bool guest_wp_enabled(const struct vcpu *v)
 {
     /* PV guests can't control CR0.WP, and it is unconditionally set by Xen. */
     return is_pv_vcpu(v) || hvm_wp_enabled(v);
 }
 
-static inline bool guest_smep_enabled(const struct vcpu *v)
+static always_inline bool guest_smep_enabled(const struct vcpu *v)
 {
     return !is_pv_vcpu(v) && hvm_smep_enabled(v);
 }
 
-static inline bool guest_smap_enabled(const struct vcpu *v)
+static always_inline bool guest_smap_enabled(const struct vcpu *v)
 {
     return !is_pv_vcpu(v) && hvm_smap_enabled(v);
 }
 
-static inline bool guest_pku_enabled(const struct vcpu *v)
+static always_inline bool guest_pku_enabled(const struct vcpu *v)
 {
     return !is_pv_vcpu(v) && hvm_pku_enabled(v);
 }
@@ -285,19 +285,21 @@ static inline bool guest_pku_enabled(const struct vcpu *v)
 /* Helpers for identifying whether guest entries have reserved bits set. */
 
 /* Bits reserved because of maxphysaddr, and (lack of) EFER.NX */
-static inline uint64_t guest_rsvd_bits(const struct vcpu *v)
+static always_inline uint64_t guest_rsvd_bits(const struct vcpu *v)
 {
     return ((PADDR_MASK &
              ~((1ul << v->domain->arch.cpuid->extd.maxphysaddr) - 1)) |
             (guest_nx_enabled(v) ? 0 : put_pte_flags(_PAGE_NX_BIT)));
 }
 
-static inline bool guest_l1e_rsvd_bits(const struct vcpu *v, guest_l1e_t l1e)
+static always_inline bool guest_l1e_rsvd_bits(const struct vcpu *v,
+                                              guest_l1e_t l1e)
 {
     return l1e.l1 & (guest_rsvd_bits(v) | GUEST_L1_PAGETABLE_RSVD);
 }
 
-static inline bool guest_l2e_rsvd_bits(const struct vcpu *v, guest_l2e_t l2e)
+static always_inline bool guest_l2e_rsvd_bits(const struct vcpu *v,
+                                              guest_l2e_t l2e)
 {
     uint64_t rsvd_bits = guest_rsvd_bits(v);
 
@@ -311,7 +313,8 @@ static inline bool guest_l2e_rsvd_bits(const struct vcpu *v, guest_l2e_t l2e)
 }
 
 #if GUEST_PAGING_LEVELS >= 3
-static inline bool guest_l3e_rsvd_bits(const struct vcpu *v, guest_l3e_t l3e)
+static always_inline bool guest_l3e_rsvd_bits(const struct vcpu *v,
+                                              guest_l3e_t l3e)
 {
     return ((l3e.l3 & (guest_rsvd_bits(v) | GUEST_L3_PAGETABLE_RSVD |
                        (guest_can_use_l3_superpages(v->domain) ? 0 : _PAGE_PSE))) ||
@@ -320,7 +323,8 @@ static inline bool guest_l3e_rsvd_bits(const struct vcpu *v, guest_l3e_t l3e)
 }
 
 #if GUEST_PAGING_LEVELS >= 4
-static inline bool guest_l4e_rsvd_bits(const struct vcpu *v, guest_l4e_t l4e)
+static always_inline bool guest_l4e_rsvd_bits(const struct vcpu *v,
+                                              guest_l4e_t l4e)
 {
     return l4e.l4 & (guest_rsvd_bits(v) | GUEST_L4_PAGETABLE_RSVD |
                      ((v->domain->arch.cpuid->x86_vendor == X86_VENDOR_AMD)
diff --git a/xen/include/asm-x86/hvm/nestedhvm.h b/xen/include/asm-x86/hvm/nestedhvm.h
index e09fa9d47d..256fed733a 100644
--- a/xen/include/asm-x86/hvm/nestedhvm.h
+++ b/xen/include/asm-x86/hvm/nestedhvm.h
@@ -33,7 +33,7 @@ enum nestedhvm_vmexits {
 };
 
 /* Nested HVM on/off per domain */
-static inline bool nestedhvm_enabled(const struct domain *d)
+static always_inline bool nestedhvm_enabled(const struct domain *d)
 {
     return is_hvm_domain(d) && d->arch.hvm.params &&
         d->arch.hvm.params[HVM_PARAM_NESTEDHVM];
diff --git a/xen/include/asm-x86/paging.h b/xen/include/asm-x86/paging.h
index 8c2027c791..7544f73121 100644
--- a/xen/include/asm-x86/paging.h
+++ b/xen/include/asm-x86/paging.h
@@ -383,7 +383,7 @@ static inline bool gfn_valid(const struct domain *d, gfn_t gfn)
 }
 
 /* Maxphysaddr supportable by the paging infrastructure. */
-static inline unsigned int paging_max_paddr_bits(const struct domain *d)
+static always_inline unsigned int paging_max_paddr_bits(const struct domain *d)
 {
     unsigned int bits = paging_mode_hap(d) ? hap_paddr_bits : paddr_bits;
 
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 629a4c52e0..9f7bc69293 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -968,50 +968,50 @@ void watchdog_domain_destroy(struct domain *d);
 
 #define VM_ASSIST(d, t) (test_bit(VMASST_TYPE_ ## t, &(d)->vm_assist))
 
-static inline bool is_pv_domain(const struct domain *d)
+static always_inline bool is_pv_domain(const struct domain *d)
 {
     return IS_ENABLED(CONFIG_PV) &&
         evaluate_nospec(!(d->options & XEN_DOMCTL_CDF_hvm));
 }
 
-static inline bool is_pv_vcpu(const struct vcpu *v)
+static always_inline bool is_pv_vcpu(const struct vcpu *v)
 {
     return is_pv_domain(v->domain);
 }
 
 #ifdef CONFIG_COMPAT
-static inline bool is_pv_32bit_domain(const struct domain *d)
+static always_inline bool is_pv_32bit_domain(const struct domain *d)
 {
     return is_pv_domain(d) && d->arch.is_32bit_pv;
 }
 
-static inline bool is_pv_32bit_vcpu(const struct vcpu *v)
+static always_inline bool is_pv_32bit_vcpu(const struct vcpu *v)
 {
     return is_pv_32bit_domain(v->domain);
 }
 
-static inline bool is_pv_64bit_domain(const struct domain *d)
+static always_inline bool is_pv_64bit_domain(const struct domain *d)
 {
     return is_pv_domain(d) && !d->arch.is_32bit_pv;
 }
 
-static inline bool is_pv_64bit_vcpu(const struct vcpu *v)
+static always_inline bool is_pv_64bit_vcpu(const struct vcpu *v)
 {
     return is_pv_64bit_domain(v->domain);
 }
 #endif
-static inline bool is_hvm_domain(const struct domain *d)
+static always_inline bool is_hvm_domain(const struct domain *d)
 {
     return IS_ENABLED(CONFIG_HVM) &&
         evaluate_nospec(d->options & XEN_DOMCTL_CDF_hvm);
 }
 
-static inline bool is_hvm_vcpu(const struct vcpu *v)
+static always_inline bool is_hvm_vcpu(const struct vcpu *v)
 {
     return is_hvm_domain(v->domain);
 }
 
-static inline bool hap_enabled(const struct domain *d)
+static always_inline bool hap_enabled(const struct domain *d)
 {
     /* sanitise_domain_config() rejects HAP && !HVM */
     return IS_ENABLED(CONFIG_HVM) &&
@@ -1034,7 +1034,7 @@ static inline bool is_xenstore_domain(const struct domain *d)
     return d->options & XEN_DOMCTL_CDF_xs_domain;
 }
 
-static inline bool is_iommu_enabled(const struct domain *d)
+static always_inline bool is_iommu_enabled(const struct domain *d)
 {
     return evaluate_nospec(d->options & XEN_DOMCTL_CDF_iommu);
 }
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/7] xen/nospec: Use always_inline to fix code gen for evaluate_nospec

[Jürgen Groß]

On 23.10.19 15:58, Andrew Cooper wrote:
> evaluate_nospec() is incredibly fragile, and this is one giant bodge.
> 
> To correctly protect jumps, the generated code needs to be of the form:
> 
>      cmp/test <cond>
>      jcc 1f
>      lfence
>      ...
>   1: lfence
>      ...
> 
> Critically, the lfence must be at the head of both basic blocks, later in the
> instruction stream than the conditional jump in need of protection.
> 
> When a static inline is involved, the optimiser decides to be clever and
> rearranges the code as:
> 
>   pred:
>      lfence
>      <calculate cond>
>      ret
> 
>      call pred
>      cmp $0, %eax
>      jcc 1f
>      ...
>   1: ...
> 
> which breaks the speculative safety.
> 
> Any use of evaluate_nospec() needs all static inline predicates which use it
> to be declared always_inline to prevent the optimiser having the flexibility
> to generate unsafe code.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/7] xen/nospec: Use always_inline to fix code gen for evaluate_nospec

[Jan Beulich]

On 23.10.2019 15:58, Andrew Cooper wrote:
> evaluate_nospec() is incredibly fragile, and this is one giant bodge.
> 
> To correctly protect jumps, the generated code needs to be of the form:
> 
>     cmp/test <cond>
>     jcc 1f
>     lfence
>     ...
>  1: lfence
>     ...
> 
> Critically, the lfence must be at the head of both basic blocks, later in the
> instruction stream than the conditional jump in need of protection.
> 
> When a static inline is involved, the optimiser decides to be clever and
> rearranges the code as:
> 
>  pred:
>     lfence
>     <calculate cond>
>     ret
> 
>     call pred
>     cmp $0, %eax
>     jcc 1f
>     ...
>  1: ...
> 
> which breaks the speculative safety.

Aiui "pred" is a non-inlined static inline here. There's no "optimiser decides
to be clever" in this case imo - it all is a result of not inlining, when the
construct in evaluate_nospec() is specifically assuming this wouldn't happen.
Therefore I continue to think that the description is misleading.

> Any use of evaluate_nospec() needs all static inline predicates which use it
> to be declared always_inline to prevent the optimiser having the flexibility
> to generate unsafe code.

I agree with this part.

> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: George Dunlap <George.Dunlap@eu.citrix.com>
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Wei Liu <wl@xen.org>
> CC: Julien Grall <julien@xen.org>
> CC: Juergen Gross <jgross@suse.com>
> 
> This is the transitive set of predicates which I can spot which need
> protecting.  There are probably ones I've missed.  Personally, I'm -1 for this
> approach, but the only other option for 4.13 is to revert it all to unbreak
> livepatching.

To unbreak livepatching, aiui what you need is symbol disambiguation,
a patch for which has been sent. With this I think we should focus on
code generation aspects here. I'm fine ack-ing the code changes with
a modified description. But since you're -1 for this, I'm not sure in
the first place that we want to go this route.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/7] xen/nospec: Use always_inline to fix code gen for evaluate_nospec

[Andrew Cooper]

On 25/10/2019 13:03, Jan Beulich wrote:
> On 23.10.2019 15:58, Andrew Cooper wrote:
>> evaluate_nospec() is incredibly fragile, and this is one giant bodge.
>>
>> To correctly protect jumps, the generated code needs to be of the form:
>>
>>     cmp/test <cond>
>>     jcc 1f
>>     lfence
>>     ...
>>  1: lfence
>>     ...
>>
>> Critically, the lfence must be at the head of both basic blocks, later in the
>> instruction stream than the conditional jump in need of protection.
>>
>> When a static inline is involved, the optimiser decides to be clever and
>> rearranges the code as:
>>
>>  pred:
>>     lfence
>>     <calculate cond>
>>     ret
>>
>>     call pred
>>     cmp $0, %eax
>>     jcc 1f
>>     ...
>>  1: ...
>>
>> which breaks the speculative safety.
> Aiui "pred" is a non-inlined static inline here.

Correct, although it actually applies to anything which the compiler
chose to out of line, perhaps even as a side effect of CSE pass.

>  There's no "optimiser decides
> to be clever" in this case imo - it all is a result of not inlining, when the
> construct in evaluate_nospec() is specifically assuming this wouldn't happen.
> Therefore I continue to think that the description is misleading.
>
>> Any use of evaluate_nospec() needs all static inline predicates which use it
>> to be declared always_inline to prevent the optimiser having the flexibility
>> to generate unsafe code.
> I agree with this part.
>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: George Dunlap <George.Dunlap@eu.citrix.com>
>> CC: Ian Jackson <ian.jackson@citrix.com>
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Wei Liu <wl@xen.org>
>> CC: Julien Grall <julien@xen.org>
>> CC: Juergen Gross <jgross@suse.com>
>>
>> This is the transitive set of predicates which I can spot which need
>> protecting.  There are probably ones I've missed.  Personally, I'm -1 for this
>> approach, but the only other option for 4.13 is to revert it all to unbreak
>> livepatching.
> To unbreak livepatching, aiui what you need is symbol disambiguation,
> a patch for which has been sent.

Correct, but..

> With this I think we should focus on
> code generation aspects here. I'm fine ack-ing the code changes with
> a modified description. But since you're -1 for this, I'm not sure in
> the first place that we want to go this route.

... without this change, l1tf-barrier/branch-hardening is still broken,
and a performance overhead.

The two choices to unblock 4.13 are this patch, or the previous version
which made CONFIG_HARDEN_BRANCH depend on BROKEN, which was even more
disliked.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/7] xen/nospec: Use always_inline to fix code gen for evaluate_nospec

[Jan Beulich]

On 25.10.2019 14:10, Andrew Cooper wrote:
> On 25/10/2019 13:03, Jan Beulich wrote:
>> On 23.10.2019 15:58, Andrew Cooper wrote:
>>> evaluate_nospec() is incredibly fragile, and this is one giant bodge.
>>>
>>> To correctly protect jumps, the generated code needs to be of the form:
>>>
>>>     cmp/test <cond>
>>>     jcc 1f
>>>     lfence
>>>     ...
>>>  1: lfence
>>>     ...
>>>
>>> Critically, the lfence must be at the head of both basic blocks, later in the
>>> instruction stream than the conditional jump in need of protection.
>>>
>>> When a static inline is involved, the optimiser decides to be clever and
>>> rearranges the code as:
>>>
>>>  pred:
>>>     lfence
>>>     <calculate cond>
>>>     ret
>>>
>>>     call pred
>>>     cmp $0, %eax
>>>     jcc 1f
>>>     ...
>>>  1: ...
>>>
>>> which breaks the speculative safety.
>> Aiui "pred" is a non-inlined static inline here.
> 
> Correct, although it actually applies to anything which the compiler
> chose to out of line, perhaps even as a side effect of CSE pass.

Not sure if you're alluding to such, but I've never seen the compiler
out-of-line something that wasn't a function (or perhaps a specialization
of one) at the source level.

>>> This is the transitive set of predicates which I can spot which need
>>> protecting.  There are probably ones I've missed.  Personally, I'm -1 for this
>>> approach, but the only other option for 4.13 is to revert it all to unbreak
>>> livepatching.
>> To unbreak livepatching, aiui what you need is symbol disambiguation,
>> a patch for which has been sent.
> 
> Correct, but..
> 
>> With this I think we should focus on
>> code generation aspects here. I'm fine ack-ing the code changes with
>> a modified description. But since you're -1 for this, I'm not sure in
>> the first place that we want to go this route.
> 
> ... without this change, l1tf-barrier/branch-hardening is still broken,
> and a performance overhead.

Well, it has less of an effect, but it's still better than without any
of this altogether. In some cases code generation is correct, and in
some other cases code generation is at least such that the window size
gets shrunk.

> The two choices to unblock 4.13 are this patch, or the previous version
> which made CONFIG_HARDEN_BRANCH depend on BROKEN, which was even more
> disliked.

Option 3 is to have just the config option, for people to turn this
off if they feel like doing so.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/7] xen/nospec: Use always_inline to fix code gen for evaluate_nospec

[Andrew Cooper]

On 25/10/2019 13:34, Jan Beulich wrote:
> On 25.10.2019 14:10, Andrew Cooper wrote:
>> On 25/10/2019 13:03, Jan Beulich wrote:
>>> On 23.10.2019 15:58, Andrew Cooper wrote:
>>>> evaluate_nospec() is incredibly fragile, and this is one giant bodge.
>>>>
>>>> To correctly protect jumps, the generated code needs to be of the form:
>>>>
>>>>     cmp/test <cond>
>>>>     jcc 1f
>>>>     lfence
>>>>     ...
>>>>  1: lfence
>>>>     ...
>>>>
>>>> Critically, the lfence must be at the head of both basic blocks, later in the
>>>> instruction stream than the conditional jump in need of protection.
>>>>
>>>> When a static inline is involved, the optimiser decides to be clever and
>>>> rearranges the code as:
>>>>
>>>>  pred:
>>>>     lfence
>>>>     <calculate cond>
>>>>     ret
>>>>
>>>>     call pred
>>>>     cmp $0, %eax
>>>>     jcc 1f
>>>>     ...
>>>>  1: ...
>>>>
>>>> which breaks the speculative safety.
>>> Aiui "pred" is a non-inlined static inline here.
>> Correct, although it actually applies to anything which the compiler
>> chose to out of line, perhaps even as a side effect of CSE pass.
> Not sure if you're alluding to such, but I've never seen the compiler
> out-of-line something that wasn't a function (or perhaps a specialization
> of one) at the source level.

I've seen it with LTO in both Clang and GCC, where the compilers can
safely reason about the lack of side effects in function calls.

>
>>>> This is the transitive set of predicates which I can spot which need
>>>> protecting.  There are probably ones I've missed.  Personally, I'm -1 for this
>>>> approach, but the only other option for 4.13 is to revert it all to unbreak
>>>> livepatching.
>>> To unbreak livepatching, aiui what you need is symbol disambiguation,
>>> a patch for which has been sent.
>> Correct, but..
>>
>>> With this I think we should focus on
>>> code generation aspects here. I'm fine ack-ing the code changes with
>>> a modified description. But since you're -1 for this, I'm not sure in
>>> the first place that we want to go this route.
>> ... without this change, l1tf-barrier/branch-hardening is still broken,
>> and a performance overhead.
> Well, it has less of an effect, but it's still better than without any
> of this altogether.

I certainly don't agree with this conclusion.

> In some cases code generation is correct,

I agree with this, but ...

> and in some other cases code generation is at least such that the window size
> gets shrunk.

... this isn't accurate.  In the case that out-of-lining happens, you
get an lfence earlier in the instruction stream, which serialises an
unrelated bit of logic (hence the perf hit), and does nothing for the
speculation window which the evaluate_nospec() was intending to protect.

>
>> The two choices to unblock 4.13 are this patch, or the previous version
>> which made CONFIG_HARDEN_BRANCH depend on BROKEN, which was even more
>> disliked.
> Option 3 is to have just the config option, for people to turn this
> off if they feel like doing so.

Yes, but no.  A facade of security is worse than no security, and I
don't consider doing that an acceptable solution in this case.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/7] xen/nospec: Use always_inline to fix code gen for evaluate_nospec

[Jan Beulich]

On 25.10.2019 17:27, Andrew Cooper wrote:
> On 25/10/2019 13:34, Jan Beulich wrote:
>> On 25.10.2019 14:10, Andrew Cooper wrote:
>>> The two choices to unblock 4.13 are this patch, or the previous version
>>> which made CONFIG_HARDEN_BRANCH depend on BROKEN, which was even more
>>> disliked.
>> Option 3 is to have just the config option, for people to turn this
>> off if they feel like doing so.
> 
> Yes, but no.  A facade of security is worse than no security, and I
> don't consider doing that an acceptable solution in this case.

But I thought we all agree that this is something that's presumably
going to remain incomplete (as in not provably complete) altogether
anyway. It's just that without the change here it's far more
incomplete then with it.

In any event I think we should (also) have an opinion from the people
who had originally contributed this logic. You didn't Cc anyone of
them; I've added at least Norbert now.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/7] xen/nospec: Use always_inline to fix code gen for evaluate_nospec

[Norbert Manthey]

On 10/25/19 17:40, Jan Beulich wrote:
> On 25.10.2019 17:27, Andrew Cooper wrote:
>> On 25/10/2019 13:34, Jan Beulich wrote:
>>> On 25.10.2019 14:10, Andrew Cooper wrote:
>>>> The two choices to unblock 4.13 are this patch, or the previous version
>>>> which made CONFIG_HARDEN_BRANCH depend on BROKEN, which was even more
>>>> disliked.
>>> Option 3 is to have just the config option, for people to turn this
>>> off if they feel like doing so.
>> Yes, but no.  A facade of security is worse than no security, and I
>> don't consider doing that an acceptable solution in this case.
> But I thought we all agree that this is something that's presumably
> going to remain incomplete (as in not provably complete) altogether
> anyway. It's just that without the change here it's far more
> incomplete then with it.
>
> In any event I think we should (also) have an opinion from the people
> who had originally contributed this logic. You didn't Cc anyone of
> them; I've added at least Norbert now.

Thanks for adding me. I had a quick look into the discussion. Only
making adding lfence statements around conditionals depending on config
BROKEN does not help, as it would still need to be always_inline to work
as expected, correct? Hence, in my opinion, this patch does the right
thing to benefit from the lfences that are placed after evaluation
conditionals.

From a "is this lfence required" point of view, we have been able to
trigger loads where the lfence has not been present, and could not
reproduce any more once we added the lfence statements on both branches
after the conditional jump.

Best,
Norbert

>
> Jan



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/7] xen/nospec: Use always_inline to fix code gen for evaluate_nospec

[Andrew Cooper]

On 25/10/2019 22:56, Norbert Manthey wrote:
> On 10/25/19 17:40, Jan Beulich wrote:
>> On 25.10.2019 17:27, Andrew Cooper wrote:
>>> On 25/10/2019 13:34, Jan Beulich wrote:
>>>> On 25.10.2019 14:10, Andrew Cooper wrote:
>>>>> The two choices to unblock 4.13 are this patch, or the previous version
>>>>> which made CONFIG_HARDEN_BRANCH depend on BROKEN, which was even more
>>>>> disliked.
>>>> Option 3 is to have just the config option, for people to turn this
>>>> off if they feel like doing so.
>>> Yes, but no.  A facade of security is worse than no security, and I
>>> don't consider doing that an acceptable solution in this case.
>> But I thought we all agree that this is something that's presumably
>> going to remain incomplete (as in not provably complete) altogether
>> anyway. It's just that without the change here it's far more
>> incomplete then with it.

This is inherently a best-effort approach, but without the
always_inline, it is tantamount to useless.

Only the grant table operations, and __mfn_valid() are correctly
protected with the code in its current form, where as the large changes
(in terms of absolute number of protected paths) comes from the predicates.

>>
>> In any event I think we should (also) have an opinion from the people
>> who had originally contributed this logic. You didn't Cc anyone of
>> them; I've added at least Norbert now.
> Thanks for adding me. I had a quick look into the discussion. Only
> making adding lfence statements around conditionals depending on config
> BROKEN does not help, as it would still need to be always_inline to work
> as expected, correct?

"depends on BROKEN" is a way to unconditionally compile out
functionality, which was one option considered to this problem.

> Hence, in my opinion, this patch does the right
> thing to benefit from the lfences that are placed after evaluation
> conditionals.

This patch is the alternative to compiling everything out.

I'm still holding out hope that we'll find a better compiler based
mitigation in the longer term, because I don't see either of these
options being viable strategies longterm.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/7] xen/nospec: Use always_inline to fix code gen for evaluate_nospec

[Norbert Manthey]

On 10/28/19 18:05, Andrew Cooper wrote:
> On 25/10/2019 22:56, Norbert Manthey wrote:
>> On 10/25/19 17:40, Jan Beulich wrote:
>>> On 25.10.2019 17:27, Andrew Cooper wrote:
>>>> On 25/10/2019 13:34, Jan Beulich wrote:
>>>>> On 25.10.2019 14:10, Andrew Cooper wrote:
>>>>>> The two choices to unblock 4.13 are this patch, or the previous version
>>>>>> which made CONFIG_HARDEN_BRANCH depend on BROKEN, which was even more
>>>>>> disliked.
>>>>> Option 3 is to have just the config option, for people to turn this
>>>>> off if they feel like doing so.
>>>> Yes, but no.  A facade of security is worse than no security, and I
>>>> don't consider doing that an acceptable solution in this case.
>>> But I thought we all agree that this is something that's presumably
>>> going to remain incomplete (as in not provably complete) altogether
>>> anyway. It's just that without the change here it's far more
>>> incomplete then with it.
> This is inherently a best-effort approach, but without the
> always_inline, it is tantamount to useless.
>
> Only the grant table operations, and __mfn_valid() are correctly
> protected with the code in its current form, where as the large changes
> (in terms of absolute number of protected paths) comes from the predicates.
>
>>> In any event I think we should (also) have an opinion from the people
>>> who had originally contributed this logic. You didn't Cc anyone of
>>> them; I've added at least Norbert now.
>> Thanks for adding me. I had a quick look into the discussion. Only
>> making adding lfence statements around conditionals depending on config
>> BROKEN does not help, as it would still need to be always_inline to work
>> as expected, correct?
> "depends on BROKEN" is a way to unconditionally compile out
> functionality, which was one option considered to this problem.
>
>> Hence, in my opinion, this patch does the right
>> thing to benefit from the lfences that are placed after evaluation
>> conditionals.
> This patch is the alternative to compiling everything out.
>
> I'm still holding out hope that we'll find a better compiler based
> mitigation in the longer term, because I don't see either of these
> options being viable strategies longterm.

I fully agree that in the long run, we should have compiler support, to
e.g. not move lfence statements around.

However, until we have that, we should allow users of Xen to get the
lfence statements at the correct positions as a best effort practice.
Hence, the always_inline modification should be there. In case you still
want to compile out this functionality, you could even add a dependency
on BROKEN on top, and then users can chose to compile it in, but at
least get a version where the lfences are placed at the right position.

Best,
Norbert

>
> ~Andrew




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/7] xen/nospec: Use always_inline to fix code gen for evaluate_nospec

[Andrew Cooper]

On 29/10/2019 08:25, Norbert Manthey wrote:
> On 10/28/19 18:05, Andrew Cooper wrote:
>> On 25/10/2019 22:56, Norbert Manthey wrote:
>>> On 10/25/19 17:40, Jan Beulich wrote:
>>>> On 25.10.2019 17:27, Andrew Cooper wrote:
>>>>> On 25/10/2019 13:34, Jan Beulich wrote:
>>>>>> On 25.10.2019 14:10, Andrew Cooper wrote:
>>>>>>> The two choices to unblock 4.13 are this patch, or the previous version
>>>>>>> which made CONFIG_HARDEN_BRANCH depend on BROKEN, which was even more
>>>>>>> disliked.
>>>>>> Option 3 is to have just the config option, for people to turn this
>>>>>> off if they feel like doing so.
>>>>> Yes, but no.  A facade of security is worse than no security, and I
>>>>> don't consider doing that an acceptable solution in this case.
>>>> But I thought we all agree that this is something that's presumably
>>>> going to remain incomplete (as in not provably complete) altogether
>>>> anyway. It's just that without the change here it's far more
>>>> incomplete then with it.
>> This is inherently a best-effort approach, but without the
>> always_inline, it is tantamount to useless.
>>
>> Only the grant table operations, and __mfn_valid() are correctly
>> protected with the code in its current form, where as the large changes
>> (in terms of absolute number of protected paths) comes from the predicates.
>>
>>>> In any event I think we should (also) have an opinion from the people
>>>> who had originally contributed this logic. You didn't Cc anyone of
>>>> them; I've added at least Norbert now.
>>> Thanks for adding me. I had a quick look into the discussion. Only
>>> making adding lfence statements around conditionals depending on config
>>> BROKEN does not help, as it would still need to be always_inline to work
>>> as expected, correct?
>> "depends on BROKEN" is a way to unconditionally compile out
>> functionality, which was one option considered to this problem.
>>
>>> Hence, in my opinion, this patch does the right
>>> thing to benefit from the lfences that are placed after evaluation
>>> conditionals.
>> This patch is the alternative to compiling everything out.
>>
>> I'm still holding out hope that we'll find a better compiler based
>> mitigation in the longer term, because I don't see either of these
>> options being viable strategies longterm.
> I fully agree that in the long run, we should have compiler support, to
> e.g. not move lfence statements around.
>
> However, until we have that, we should allow users of Xen to get the
> lfence statements at the correct positions as a best effort practice.
> Hence, the always_inline modification should be there. In case you still
> want to compile out this functionality, you could even add a dependency
> on BROKEN on top, and then users can chose to compile it in, but at
> least get a version where the lfences are placed at the right position.

This is going around in circles.

The following patch in this series is a fully user-selectable Kconfig
option for whether they want to use branch hardening, and is in place
once there is a plausible expectation that the lfences are in suitable
positions.

If this patch series does not agreement, I will unblock livepatching on
4.13 by committing the v2 patch which causes BRANCH_HARDEN to depend on
BROKEN and force it to be compiled out with no user choice at all.

Unbreaking livepatching is strictly more important than keeping a brand
new feature in 4.13 in a broken form.  I've provided two alternative
strategies to fix the 4.13 blockers, but if noone can agree on which
approach to use, I will commit the simpler fix.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/7] xen/nospec: Use always_inline to fix code gen for evaluate_nospec

[Jan Beulich]

On 29.10.2019 14:46, Andrew Cooper wrote:
> If this patch series does not agreement, I will unblock livepatching on
> 4.13 by committing the v2 patch which causes BRANCH_HARDEN to depend on
> BROKEN and force it to be compiled out with no user choice at all.
> 
> Unbreaking livepatching is strictly more important than keeping a brand
> new feature in 4.13 in a broken form.  I've provided two alternative
> strategies to fix the 4.13 blockers, but if noone can agree on which
> approach to use, I will commit the simpler fix.

As to unblocking live patching - may I ask you look at the symbol
disambiguation patch I did submit? The thread here, as suggested
before, should now be solely about code generation. And just in
case you've missed this: I did indicate I'm willing to give my
ack on the v3 patch here, provided you adjust the description as
asked for in my initial(?) reply.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/7] xen/nospec: Use always_inline to fix code gen for evaluate_nospec

[Andrew Cooper]

On 29/10/2019 14:03, Jan Beulich wrote:
> On 29.10.2019 14:46, Andrew Cooper wrote:
>> If this patch series does not agreement, I will unblock livepatching on
>> 4.13 by committing the v2 patch which causes BRANCH_HARDEN to depend on
>> BROKEN and force it to be compiled out with no user choice at all.
>>
>> Unbreaking livepatching is strictly more important than keeping a brand
>> new feature in 4.13 in a broken form.  I've provided two alternative
>> strategies to fix the 4.13 blockers, but if noone can agree on which
>> approach to use, I will commit the simpler fix.
> As to unblocking live patching - may I ask you look at the symbol
> disambiguation patch I did submit? The thread here, as suggested
> before, should now be solely about code generation.

Right, but the current state of l1tf-barrier is also a 4.13 blocker too
- it is not in a shippable state.

It either wants compiling out totally (the v2 patch), or having the code
generation fixing (this v3 series), or some concrete 3rd suggestion.

> And just in
> case you've missed this: I did indicate I'm willing to give my
> ack on the v3 patch here, provided you adjust the description as
> asked for in my initial(?) reply.

I had missed that, yes.  I'll see about tweaking the commit message.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/7] xen/nospec: Use always_inline to fix code gen for evaluate_nospec

[Norbert Manthey]

On 10/29/19 15:16, Andrew Cooper wrote:
> On 29/10/2019 14:03, Jan Beulich wrote:
>> On 29.10.2019 14:46, Andrew Cooper wrote:
>>> If this patch series does not agreement, I will unblock livepatching on
>>> 4.13 by committing the v2 patch which causes BRANCH_HARDEN to depend on
>>> BROKEN and force it to be compiled out with no user choice at all.
>>>
>>> Unbreaking livepatching is strictly more important than keeping a brand
>>> new feature in 4.13 in a broken form.  I've provided two alternative
>>> strategies to fix the 4.13 blockers, but if noone can agree on which
>>> approach to use, I will commit the simpler fix.
>> As to unblocking live patching - may I ask you look at the symbol
>> disambiguation patch I did submit? The thread here, as suggested
>> before, should now be solely about code generation.
> Right, but the current state of l1tf-barrier is also a 4.13 blocker too
> - it is not in a shippable state.
>
> It either wants compiling out totally (the v2 patch), or having the code
> generation fixing (this v3 series), or some concrete 3rd suggestion.

I vote for having the code generation fixed, i.e. v3 of this series.

Best,
Norbert




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/7] xen/nospec: Use always_inline to fix code gen for evaluate_nospec

[Andrew Cooper]

On 25/10/2019 13:03, Jan Beulich wrote:
> On 23.10.2019 15:58, Andrew Cooper wrote:
>> evaluate_nospec() is incredibly fragile, and this is one giant bodge.
>>
>> To correctly protect jumps, the generated code needs to be of the form:
>>
>>     cmp/test <cond>
>>     jcc 1f
>>     lfence
>>     ...
>>  1: lfence
>>     ...
>>
>> Critically, the lfence must be at the head of both basic blocks, later in the
>> instruction stream than the conditional jump in need of protection.
>>
>> When a static inline is involved, the optimiser decides to be clever and
>> rearranges the code as:
>>
>>  pred:
>>     lfence
>>     <calculate cond>
>>     ret
>>
>>     call pred
>>     cmp $0, %eax
>>     jcc 1f
>>     ...
>>  1: ...
>>
>> which breaks the speculative safety.
> Aiui "pred" is a non-inlined static inline here. There's no "optimiser decides
> to be clever" in this case imo - it all is a result of not inlining, when the
> construct in evaluate_nospec() is specifically assuming this wouldn't happen.
> Therefore I continue to think that the description is misleading.
>
>> Any use of evaluate_nospec() needs all static inline predicates which use it
>> to be declared always_inline to prevent the optimiser having the flexibility
>> to generate unsafe code.
> I agree with this part.

How about:

When the compiler chooses to out-of-line the condition calculation (e.g. by
not inlining a predicate), the code layout can end up as:
   
 pred:
    lfence
    <calculate cond>
    ret
   
    call pred
    cmp $0, %eax
    jcc 1f
    ...
 1: ...
   
which breaks the speculative safety, as the lfences are earlier in the
instruction stream than the jump in need of protection.

?

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/7] xen/nospec: Use always_inline to fix code gen for evaluate_nospec

[Jan Beulich]

On 29.10.2019 17:53, Andrew Cooper wrote:
> On 25/10/2019 13:03, Jan Beulich wrote:
>> On 23.10.2019 15:58, Andrew Cooper wrote:
>>> evaluate_nospec() is incredibly fragile, and this is one giant bodge.
>>>
>>> To correctly protect jumps, the generated code needs to be of the form:
>>>
>>>     cmp/test <cond>
>>>     jcc 1f
>>>     lfence
>>>     ...
>>>  1: lfence
>>>     ...
>>>
>>> Critically, the lfence must be at the head of both basic blocks, later in the
>>> instruction stream than the conditional jump in need of protection.
>>>
>>> When a static inline is involved, the optimiser decides to be clever and
>>> rearranges the code as:
>>>
>>>  pred:
>>>     lfence
>>>     <calculate cond>
>>>     ret
>>>
>>>     call pred
>>>     cmp $0, %eax
>>>     jcc 1f
>>>     ...
>>>  1: ...
>>>
>>> which breaks the speculative safety.
>> Aiui "pred" is a non-inlined static inline here. There's no "optimiser decides
>> to be clever" in this case imo - it all is a result of not inlining, when the
>> construct in evaluate_nospec() is specifically assuming this wouldn't happen.
>> Therefore I continue to think that the description is misleading.
>>
>>> Any use of evaluate_nospec() needs all static inline predicates which use it
>>> to be declared always_inline to prevent the optimiser having the flexibility
>>> to generate unsafe code.
>> I agree with this part.
> 
> How about:
> 
> When the compiler chooses to out-of-line the condition calculation (e.g. by
> not inlining a predicate), the code layout can end up as:
>    
>  pred:
>     lfence
>     <calculate cond>
>     ret
>    
>     call pred
>     cmp $0, %eax
>     jcc 1f
>     ...
>  1: ...
>    
> which breaks the speculative safety, as the lfences are earlier in the
> instruction stream than the jump in need of protection.
> 
> ?

Sounds good, thanks. With this
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v3 3/7] xen/nospec: Introduce CONFIG_SPECULATIVE_HARDEN_BRANCH

[Andrew Cooper]

Just as with CONFIG_SPECULATIVE_HARDEN_ARRAY, branch hardening should be
configurable at compile time.

The previous CONFIG_HVM was a consequence of what could be discussed publicly
at the time the patches were submitted, and wasn't actually correct.  Later
patches will make further corrections.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Juergen Gross <jgross@suse.com>

v3:
 * Reduce to just the Kconfig option.  Split other changes out into separate
   patches.

v2:
 * Expand the commit message to describe how the generated code is broken.
 * Rename to CONFIG_SPECULATIVE_HARDEN_BRANCH
 * Switch alternative() to asm()
 * Fix a comment typo
---
 xen/common/Kconfig           | 23 +++++++++++++++++++++++
 xen/include/asm-x86/nospec.h |  2 +-
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 7b5dd9d495..c9e671869e 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -102,6 +102,29 @@ config SPECULATIVE_HARDEN_ARRAY
 
 	  If unsure, say Y.
 
+config SPECULATIVE_HARDEN_BRANCH
+	bool "Speculative Branch Hardening"
+	default y
+	depends on X86
+        ---help---
+	  Contemporary processors may use speculative execution as a
+	  performance optimisation, but this can potentially be abused by an
+	  attacker to leak data via speculative sidechannels.
+
+	  One source of misbehaviour is by executing the wrong basic block
+	  following a conditional jump.
+
+	  When enabled, specific conditions which have been deemed liable to
+	  be speculatively abused will be hardened to avoid entering the wrong
+	  basic block.
+
+	  This is a best-effort mitigation.  There are no guarantees that all
+	  areas of code open to abuse have been hardened, nor that
+	  optimisations in the compiler haven't subverted the attempts to
+	  harden.
+
+	  If unsure, say Y.
+
 endmenu
 
 config KEXEC
diff --git a/xen/include/asm-x86/nospec.h b/xen/include/asm-x86/nospec.h
index 427b5ff9df..154e92aed8 100644
--- a/xen/include/asm-x86/nospec.h
+++ b/xen/include/asm-x86/nospec.h
@@ -9,7 +9,7 @@
 /* Allow to insert a read memory barrier into conditionals */
 static always_inline bool barrier_nospec_true(void)
 {
-#ifdef CONFIG_HVM
+#ifdef CONFIG_SPECULATIVE_HARDEN_BRANCH
     alternative("", "lfence", X86_FEATURE_SC_L1TF_VULN);
 #endif
     return true;
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 3/7] xen/nospec: Introduce CONFIG_SPECULATIVE_HARDEN_BRANCH

[Jürgen Groß]

On 23.10.19 15:58, Andrew Cooper wrote:
> Just as with CONFIG_SPECULATIVE_HARDEN_ARRAY, branch hardening should be
> configurable at compile time.
> 
> The previous CONFIG_HVM was a consequence of what could be discussed publicly
> at the time the patches were submitted, and wasn't actually correct.  Later
> patches will make further corrections.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 3/7] xen/nospec: Introduce CONFIG_SPECULATIVE_HARDEN_BRANCH

[Jan Beulich]

On 23.10.2019 15:58, Andrew Cooper wrote:
> Just as with CONFIG_SPECULATIVE_HARDEN_ARRAY, branch hardening should be
> configurable at compile time.
> 
> The previous CONFIG_HVM was a consequence of what could be discussed publicly
> at the time the patches were submitted, and wasn't actually correct.  Later
> patches will make further corrections.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v3 4/7] x86/nospec: Rename and rework l1tf-barrier as branch-harden

[Andrew Cooper]

l1tf-barrier is an inappropriate name, and came about because of restrictions
on could be discussed publicly when the patches were proposed.

In practice, it is for general Spectre v1 mitigations, and is necessary in all
cases.  An adversary which can control speculation in Xen can leak data in
cross-core (BCBS, etc) or remote (NetSpectre) scenarios - the problem is not
limited to just L1TF with HT active.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Juergen Gross <jgross@suse.com>

v3:
 * New

In principle it should be tristate and being disabled by default on parts
which don't speculate, but it is too late in 4.13 to organise this.
---
 docs/misc/xen-command-line.pandoc | 11 +++++------
 xen/arch/x86/spec_ctrl.c          | 17 +++++++----------
 xen/include/asm-x86/cpufeatures.h |  2 +-
 xen/include/asm-x86/nospec.h      |  2 +-
 xen/include/asm-x86/spec_ctrl.h   |  2 +-
 5 files changed, 15 insertions(+), 19 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 67df80c50d..e37a13ed11 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -1960,7 +1960,7 @@ By default SSBD will be mitigated at runtime (i.e `ssbd=runtime`).
 ### spec-ctrl (x86)
 > `= List of [ <bool>, xen=<bool>, {pv,hvm,msr-sc,rsb,md-clear}=<bool>,
 >              bti-thunk=retpoline|lfence|jmp, {ibrs,ibpb,ssbd,eager-fpu,
->              l1d-flush,l1tf-barrier}=<bool> ]`
+>              l1d-flush,branch-harden}=<bool> ]`
 
 Controls for speculative execution sidechannel mitigations.  By default, Xen
 will pick the most appropriate mitigations based on compiled in support,
@@ -2032,11 +2032,10 @@ Irrespective of Xen's setting, the feature is virtualised for HVM guests to
 use.  By default, Xen will enable this mitigation on hardware believed to be
 vulnerable to L1TF.
 
-On hardware vulnerable to L1TF, the `l1tf-barrier=` option can be used to force
-or prevent Xen from protecting evaluations inside the hypervisor with a barrier
-instruction to not load potentially secret information into L1 cache.  By
-default, Xen will enable this mitigation on hardware believed to be vulnerable
-to L1TF.
+If Xen is compiled with `CONFIG_SPECULATIVE_HARDEN_BRANCH`, the
+`branch-harden=` boolean can be used to force or prevent Xen from using
+speculation barriers to protect selected conditional branches.  By default,
+Xen will enabled this mitigation.
 
 ### sync_console
 > `= <boolean>`
diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
index ee5439a371..e74e0cc619 100644
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -52,7 +52,7 @@ bool __read_mostly opt_ibpb = true;
 bool __read_mostly opt_ssbd = false;
 int8_t __read_mostly opt_eager_fpu = -1;
 int8_t __read_mostly opt_l1d_flush = -1;
-int8_t __read_mostly opt_l1tf_barrier = -1;
+bool __read_mostly opt_branch_harden = true;
 
 bool __initdata bsp_delay_spec_ctrl;
 uint8_t __read_mostly default_xen_spec_ctrl;
@@ -97,7 +97,7 @@ static int __init parse_spec_ctrl(const char *s)
             if ( opt_pv_l1tf_domu < 0 )
                 opt_pv_l1tf_domu = 0;
 
-            opt_l1tf_barrier = 0;
+            opt_branch_harden = false;
 
         disable_common:
             opt_rsb_pv = false;
@@ -174,8 +174,8 @@ static int __init parse_spec_ctrl(const char *s)
             opt_eager_fpu = val;
         else if ( (val = parse_boolean("l1d-flush", s, ss)) >= 0 )
             opt_l1d_flush = val;
-        else if ( (val = parse_boolean("l1tf-barrier", s, ss)) >= 0 )
-            opt_l1tf_barrier = val;
+        else if ( (val = parse_boolean("branch-harden", s, ss)) >= 0 )
+            opt_branch_harden = val;
         else
             rc = -EINVAL;
 
@@ -348,7 +348,7 @@ static void __init print_details(enum ind_thunk thunk, uint64_t caps)
            opt_ibpb                                  ? " IBPB"  : "",
            opt_l1d_flush                             ? " L1D_FLUSH" : "",
            opt_md_clear_pv || opt_md_clear_hvm       ? " VERW"  : "",
-           opt_l1tf_barrier                          ? " L1TF_BARRIER" : "");
+           opt_branch_harden                         ? " BRANCH_HARDEN" : "");
 
     /* L1TF diagnostics, printed if vulnerable or PV shadowing is in use. */
     if ( cpu_has_bug_l1tf || opt_pv_l1tf_hwdom || opt_pv_l1tf_domu )
@@ -1033,11 +1033,8 @@ void __init init_speculation_mitigations(void)
     else if ( opt_l1d_flush == -1 )
         opt_l1d_flush = cpu_has_bug_l1tf && !(caps & ARCH_CAPS_SKIP_L1DFL);
 
-    /* By default, enable L1TF_VULN on L1TF-vulnerable hardware */
-    if ( opt_l1tf_barrier == -1 )
-        opt_l1tf_barrier = cpu_has_bug_l1tf && (opt_smt || !opt_l1d_flush);
-    if ( opt_l1tf_barrier > 0 )
-        setup_force_cpu_cap(X86_FEATURE_SC_L1TF_VULN);
+    if ( opt_branch_harden )
+        setup_force_cpu_cap(X86_FEATURE_SC_BRANCH_HARDEN);
 
     /*
      * We do not disable HT by default on affected hardware.
diff --git a/xen/include/asm-x86/cpufeatures.h b/xen/include/asm-x86/cpufeatures.h
index 91eccf5161..b9d3cac975 100644
--- a/xen/include/asm-x86/cpufeatures.h
+++ b/xen/include/asm-x86/cpufeatures.h
@@ -27,7 +27,7 @@ XEN_CPUFEATURE(XEN_SMAP,          X86_SYNTH(11)) /* SMAP gets used by Xen itself
 XEN_CPUFEATURE(LFENCE_DISPATCH,   X86_SYNTH(12)) /* lfence set as Dispatch Serialising */
 XEN_CPUFEATURE(IND_THUNK_LFENCE,  X86_SYNTH(13)) /* Use IND_THUNK_LFENCE */
 XEN_CPUFEATURE(IND_THUNK_JMP,     X86_SYNTH(14)) /* Use IND_THUNK_JMP */
-XEN_CPUFEATURE(SC_L1TF_VULN,      X86_SYNTH(15)) /* L1TF protection required */
+XEN_CPUFEATURE(SC_BRANCH_HARDEN,  X86_SYNTH(15)) /* Conditional Branch Hardening */
 XEN_CPUFEATURE(SC_MSR_PV,         X86_SYNTH(16)) /* MSR_SPEC_CTRL used by Xen for PV */
 XEN_CPUFEATURE(SC_MSR_HVM,        X86_SYNTH(17)) /* MSR_SPEC_CTRL used by Xen for HVM */
 XEN_CPUFEATURE(SC_RSB_PV,         X86_SYNTH(18)) /* RSB overwrite needed for PV */
diff --git a/xen/include/asm-x86/nospec.h b/xen/include/asm-x86/nospec.h
index 154e92aed8..f6eb84eee5 100644
--- a/xen/include/asm-x86/nospec.h
+++ b/xen/include/asm-x86/nospec.h
@@ -10,7 +10,7 @@
 static always_inline bool barrier_nospec_true(void)
 {
 #ifdef CONFIG_SPECULATIVE_HARDEN_BRANCH
-    alternative("", "lfence", X86_FEATURE_SC_L1TF_VULN);
+    alternative("", "lfence", X86_FEATURE_SC_BRANCH_HARDEN);
 #endif
     return true;
 }
diff --git a/xen/include/asm-x86/spec_ctrl.h b/xen/include/asm-x86/spec_ctrl.h
index 1339ddd7ef..9caecddfec 100644
--- a/xen/include/asm-x86/spec_ctrl.h
+++ b/xen/include/asm-x86/spec_ctrl.h
@@ -37,7 +37,7 @@ extern bool opt_ibpb;
 extern bool opt_ssbd;
 extern int8_t opt_eager_fpu;
 extern int8_t opt_l1d_flush;
-extern int8_t opt_l1tf_barrier;
+extern bool opt_branch_harden;
 
 extern bool bsp_delay_spec_ctrl;
 extern uint8_t default_xen_spec_ctrl;
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 4/7] x86/nospec: Rename and rework l1tf-barrier as branch-harden

[Jürgen Groß]

On 23.10.19 15:58, Andrew Cooper wrote:
> l1tf-barrier is an inappropriate name, and came about because of restrictions
> on could be discussed publicly when the patches were proposed.
> 
> In practice, it is for general Spectre v1 mitigations, and is necessary in all
> cases.  An adversary which can control speculation in Xen can leak data in
> cross-core (BCBS, etc) or remote (NetSpectre) scenarios - the problem is not
> limited to just L1TF with HT active.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

... one nit below.

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wl@xen.org>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Juergen Gross <jgross@suse.com>
> 
> v3:
>   * New
> 
> In principle it should be tristate and being disabled by default on parts
> which don't speculate, but it is too late in 4.13 to organise this.
> ---
>   docs/misc/xen-command-line.pandoc | 11 +++++------
>   xen/arch/x86/spec_ctrl.c          | 17 +++++++----------
>   xen/include/asm-x86/cpufeatures.h |  2 +-
>   xen/include/asm-x86/nospec.h      |  2 +-
>   xen/include/asm-x86/spec_ctrl.h   |  2 +-
>   5 files changed, 15 insertions(+), 19 deletions(-)
> 
> diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
> index 67df80c50d..e37a13ed11 100644
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -1960,7 +1960,7 @@ By default SSBD will be mitigated at runtime (i.e `ssbd=runtime`).
>   ### spec-ctrl (x86)
>   > `= List of [ <bool>, xen=<bool>, {pv,hvm,msr-sc,rsb,md-clear}=<bool>,
>   >              bti-thunk=retpoline|lfence|jmp, {ibrs,ibpb,ssbd,eager-fpu,
> ->              l1d-flush,l1tf-barrier}=<bool> ]`
> +>              l1d-flush,branch-harden}=<bool> ]`
>   
>   Controls for speculative execution sidechannel mitigations.  By default, Xen
>   will pick the most appropriate mitigations based on compiled in support,
> @@ -2032,11 +2032,10 @@ Irrespective of Xen's setting, the feature is virtualised for HVM guests to
>   use.  By default, Xen will enable this mitigation on hardware believed to be
>   vulnerable to L1TF.
>   
> -On hardware vulnerable to L1TF, the `l1tf-barrier=` option can be used to force
> -or prevent Xen from protecting evaluations inside the hypervisor with a barrier
> -instruction to not load potentially secret information into L1 cache.  By
> -default, Xen will enable this mitigation on hardware believed to be vulnerable
> -to L1TF.
> +If Xen is compiled with `CONFIG_SPECULATIVE_HARDEN_BRANCH`, the
> +`branch-harden=` boolean can be used to force or prevent Xen from using
> +speculation barriers to protect selected conditional branches.  By default,
> +Xen will enabled this mitigation.

s/enabled/enable/


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 4/7] x86/nospec: Rename and rework l1tf-barrier as branch-harden

[Jan Beulich]

On 23.10.2019 15:58, Andrew Cooper wrote:
> l1tf-barrier is an inappropriate name, and came about because of restrictions
> on could be discussed publicly when the patches were proposed.
> 
> In practice, it is for general Spectre v1 mitigations, and is necessary in all
> cases.  An adversary which can control speculation in Xen can leak data in
> cross-core (BCBS, etc) or remote (NetSpectre) scenarios - the problem is not
> limited to just L1TF with HT active.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

> In principle it should be tristate and being disabled by default on parts
> which don't speculate, but it is too late in 4.13 to organise this.

And the fundamental issue is correctly compiling the conditions under which
to default to true and false respectively? I ask because if it was not this
then I wouldn't see what hindering factor there is to make this tristate
right away.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 4/7] x86/nospec: Rename and rework l1tf-barrier as branch-harden

[Andrew Cooper]

On 25/10/2019 13:09, Jan Beulich wrote:
> On 23.10.2019 15:58, Andrew Cooper wrote:
>> l1tf-barrier is an inappropriate name, and came about because of restrictions
>> on could be discussed publicly when the patches were proposed.
>>
>> In practice, it is for general Spectre v1 mitigations, and is necessary in all
>> cases.  An adversary which can control speculation in Xen can leak data in
>> cross-core (BCBS, etc) or remote (NetSpectre) scenarios - the problem is not
>> limited to just L1TF with HT active.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
>> In principle it should be tristate and being disabled by default on parts
>> which don't speculate, but it is too late in 4.13 to organise this.
> And the fundamental issue is correctly compiling the conditions under which
> to default to true and false respectively?

Yes.

> I ask because if it was not this
> then I wouldn't see what hindering factor there is to make this tristate
> right away.

Linux's NO_SPECULATION list is a start (if there is actually an
intersection with 64bit capable CPUs), but there are mitigating factors
on some later CPUs as well.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v3 5/7] x86/livepatch: Fail the build if duplicate symbols exist

[Andrew Cooper]

From: Ross Lagerwall <ross.lagerwall@citrix.com>

The binary diffing algorithm used by xen-livepatch depends on having unique
symbols.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>

The livepatch loading algorithm used by Xen resolves relocations by symbol
name, and thus also depends on having unique symbols.

Introduce CONFIG_ENFORCE_UNIQUE_SYMBOLS to control failing the build if
duplicate symbols are found, and disable it in the RANDCONFIG build.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Ross Lagerwall <ross.lagerwall@citrix.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Norbert Manthey <nmanthey@amazon.de>
CC: Juergen Gross <jgross@suse.com>

v3:
 * Use a new config option
---
 xen/arch/x86/Makefile              |  1 +
 xen/common/Kconfig                 | 18 ++++++++++++++++--
 xen/tools/kconfig/allrandom.config |  1 +
 xen/tools/symbols.c                | 11 +++++++++--
 4 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 2443fd2cc5..6b369f21cb 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -99,6 +99,7 @@ endif
 
 syms-warn-dup-y := --warn-dup
 syms-warn-dup-$(CONFIG_SUPPRESS_DUPLICATE_SYMBOL_WARNINGS) :=
+syms-warn-dup-$(CONFIG_ENFORCE_UNIQUE_SYMBOLS) := --error-dup
 
 $(TARGET): TMP = $(@D)/.$(@F).elf32
 $(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index c9e671869e..4c837d6892 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -361,9 +361,23 @@ config FAST_SYMBOL_LOOKUP
 
 	  If unsure, say Y.
 
+config ENFORCE_UNIQUE_SYMBOLS
+	bool "Enforce unique symbols" if LIVEPATCH
+	default y if LIVEPATCH
+	---help---
+	  Multiple symbols with the same name aren't generally a problem
+	  unless Live patching is to be used.
+
+	  Livepatch loading involves resolving relocations against symbol
+	  names, and attempting to a duplicate symbol in a livepatch will
+	  result in incorrect livepatch application.
+
+	  This option should be used to ensure that a build of Xen can have a
+	  livepatch build and apply correctly.
+
 config SUPPRESS_DUPLICATE_SYMBOL_WARNINGS
-	bool "Suppress duplicate symbol warnings" if !LIVEPATCH
-	default y if !LIVEPATCH
+	bool "Suppress duplicate symbol warnings" if !ENFORCE_UNIQUE_SYMBOLS
+	default y if !ENFORCE_UNIQUE_SYMBOLS
 	---help---
 	  Multiple symbols with the same name aren't generally a problem
 	  unless Live patching is to be used, so these warnings can be
diff --git a/xen/tools/kconfig/allrandom.config b/xen/tools/kconfig/allrandom.config
index 76f74320b5..c480896b96 100644
--- a/xen/tools/kconfig/allrandom.config
+++ b/xen/tools/kconfig/allrandom.config
@@ -2,3 +2,4 @@
 
 CONFIG_GCOV_FORMAT_AUTODETECT=y
 CONFIG_UBSAN=n
+CONFIG_ENFORCE_UNIQUE_SYMBOLS=n
diff --git a/xen/tools/symbols.c b/xen/tools/symbols.c
index 05139d1600..9f9e2c9900 100644
--- a/xen/tools/symbols.c
+++ b/xen/tools/symbols.c
@@ -599,7 +599,7 @@ static int compare_name(const void *p1, const void *p2)
 int main(int argc, char **argv)
 {
 	unsigned int i;
-	bool unsorted = false, warn_dup = false;
+	bool unsorted = false, warn_dup = false, error_dup = false, found_dup = false;
 
 	if (argc >= 2) {
 		for (i = 1; i < argc; i++) {
@@ -619,6 +619,8 @@ int main(int argc, char **argv)
 				sort_by_name = 1;
 			else if (strcmp(argv[i], "--warn-dup") == 0)
 				warn_dup = true;
+			else if (strcmp(argv[i], "--error-dup") == 0)
+				warn_dup = error_dup = true;
 			else if (strcmp(argv[i], "--xensyms") == 0)
 				map_only = true;
 			else
@@ -634,14 +636,19 @@ int main(int argc, char **argv)
 		for (i = 1; i < table_cnt; ++i)
 			if (strcmp(SYMBOL_NAME(table + i - 1),
 				   SYMBOL_NAME(table + i)) == 0 &&
-			    table[i - 1].addr != table[i].addr)
+			    table[i - 1].addr != table[i].addr) {
 				fprintf(stderr,
 					"Duplicate symbol '%s' (%llx != %llx)\n",
 					SYMBOL_NAME(table + i),
 					table[i].addr, table[i - 1].addr);
+				found_dup = true;
+			}
 		unsorted = true;
 	}
 
+	if (error_dup && found_dup)
+		exit(1);
+
 	if (unsorted)
 		qsort(table, table_cnt, sizeof(*table), compare_value);
 
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 5/7] x86/livepatch: Fail the build if duplicate symbols exist

[Jürgen Groß]

On 23.10.19 15:58, Andrew Cooper wrote:
> From: Ross Lagerwall <ross.lagerwall@citrix.com>
> 
> The binary diffing algorithm used by xen-livepatch depends on having unique
> symbols.
> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> 
> The livepatch loading algorithm used by Xen resolves relocations by symbol
> name, and thus also depends on having unique symbols.
> 
> Introduce CONFIG_ENFORCE_UNIQUE_SYMBOLS to control failing the build if
> duplicate symbols are found, and disable it in the RANDCONFIG build.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 5/7] x86/livepatch: Fail the build if duplicate symbols exist

[Konrad Rzeszutek Wilk]

On October 23, 2019 10:46:37 AM EDT, "Jürgen Groß" <jgross@suse.com> wrote:
>On 23.10.19 15:58, Andrew Cooper wrote:
>> From: Ross Lagerwall <ross.lagerwall@citrix.com>
>> 
>> The binary diffing algorithm used by xen-livepatch depends on having
>unique
>> symbols.
>> 
>> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
>> 
>> The livepatch loading algorithm used by Xen resolves relocations by
>symbol
>> name, and thus also depends on having unique symbols.
>> 
>> Introduce CONFIG_ENFORCE_UNIQUE_SYMBOLS to control failing the build
>if
>> duplicate symbols are found, and disable it in the RANDCONFIG build.
>> 
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

What is up with that SoBs not being together?

Could you squash them please?

Patch wise, feel free to add my Reviewed-by.

Thx
>
>Release-acked-by: Juergen Gross <jgross@suse.com>
>
>
>Juergen


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 5/7] x86/livepatch: Fail the build if duplicate symbols exist

[Ross Lagerwall]

On 10/23/19 2:58 PM, Andrew Cooper wrote:
> From: Ross Lagerwall <ross.lagerwall@citrix.com>
> 
> The binary diffing algorithm used by xen-livepatch depends on having unique
> symbols.
> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> 
Presumably this should be "livepatch-build" or "livepatch-build-tools" 
rather than "xen-livepatch".

-- 
Ross Lagerwall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 5/7] x86/livepatch: Fail the build if duplicate symbols exist

[Jan Beulich]

On 23.10.2019 15:58, Andrew Cooper wrote:
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -361,9 +361,23 @@ config FAST_SYMBOL_LOOKUP
>  
>  	  If unsure, say Y.
>  
> +config ENFORCE_UNIQUE_SYMBOLS
> +	bool "Enforce unique symbols" if LIVEPATCH
> +	default y if LIVEPATCH

Instead of two identical "if", why not "depends on LIVEPATCH"?

> +	---help---
> +	  Multiple symbols with the same name aren't generally a problem
> +	  unless Live patching is to be used.
> +
> +	  Livepatch loading involves resolving relocations against symbol
> +	  names, and attempting to a duplicate symbol in a livepatch will
> +	  result in incorrect livepatch application.
> +
> +	  This option should be used to ensure that a build of Xen can have a
> +	  livepatch build and apply correctly.
> +
>  config SUPPRESS_DUPLICATE_SYMBOL_WARNINGS
> -	bool "Suppress duplicate symbol warnings" if !LIVEPATCH
> -	default y if !LIVEPATCH
> +	bool "Suppress duplicate symbol warnings" if !ENFORCE_UNIQUE_SYMBOLS
> +	default y if !ENFORCE_UNIQUE_SYMBOLS

Similarly here then. With this changed, or with a proper reason
supplied
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 5/7] x86/livepatch: Fail the build if duplicate symbols exist

[Andrew Cooper]

On 24/10/2019 13:03, Jan Beulich wrote:
> On 23.10.2019 15:58, Andrew Cooper wrote:
>> --- a/xen/common/Kconfig
>> +++ b/xen/common/Kconfig
>> @@ -361,9 +361,23 @@ config FAST_SYMBOL_LOOKUP
>>  
>>  	  If unsure, say Y.
>>  
>> +config ENFORCE_UNIQUE_SYMBOLS
>> +	bool "Enforce unique symbols" if LIVEPATCH
>> +	default y if LIVEPATCH
> Instead of two identical "if", why not "depends on LIVEPATCH"?
>
>> +	---help---
>> +	  Multiple symbols with the same name aren't generally a problem
>> +	  unless Live patching is to be used.
>> +
>> +	  Livepatch loading involves resolving relocations against symbol
>> +	  names, and attempting to a duplicate symbol in a livepatch will
>> +	  result in incorrect livepatch application.
>> +
>> +	  This option should be used to ensure that a build of Xen can have a
>> +	  livepatch build and apply correctly.
>> +
>>  config SUPPRESS_DUPLICATE_SYMBOL_WARNINGS
>> -	bool "Suppress duplicate symbol warnings" if !LIVEPATCH
>> -	default y if !LIVEPATCH
>> +	bool "Suppress duplicate symbol warnings" if !ENFORCE_UNIQUE_SYMBOLS
>> +	default y if !ENFORCE_UNIQUE_SYMBOLS
> Similarly here then. With this changed, or with a proper reason
> supplied
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

That's a question for the author of c/s 064a2652233 to answer...  I'm
merely following the prevailing style.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 5/7] x86/livepatch: Fail the build if duplicate symbols exist

[Jan Beulich]

On 29.10.2019 18:06, Andrew Cooper wrote:
> On 24/10/2019 13:03, Jan Beulich wrote:
>> On 23.10.2019 15:58, Andrew Cooper wrote:
>>> --- a/xen/common/Kconfig
>>> +++ b/xen/common/Kconfig
>>> @@ -361,9 +361,23 @@ config FAST_SYMBOL_LOOKUP
>>>  
>>>  	  If unsure, say Y.
>>>  
>>> +config ENFORCE_UNIQUE_SYMBOLS
>>> +	bool "Enforce unique symbols" if LIVEPATCH
>>> +	default y if LIVEPATCH
>> Instead of two identical "if", why not "depends on LIVEPATCH"?
>>
>>> +	---help---
>>> +	  Multiple symbols with the same name aren't generally a problem
>>> +	  unless Live patching is to be used.
>>> +
>>> +	  Livepatch loading involves resolving relocations against symbol
>>> +	  names, and attempting to a duplicate symbol in a livepatch will
>>> +	  result in incorrect livepatch application.
>>> +
>>> +	  This option should be used to ensure that a build of Xen can have a
>>> +	  livepatch build and apply correctly.
>>> +
>>>  config SUPPRESS_DUPLICATE_SYMBOL_WARNINGS
>>> -	bool "Suppress duplicate symbol warnings" if !LIVEPATCH
>>> -	default y if !LIVEPATCH
>>> +	bool "Suppress duplicate symbol warnings" if !ENFORCE_UNIQUE_SYMBOLS
>>> +	default y if !ENFORCE_UNIQUE_SYMBOLS
>> Similarly here then. With this changed, or with a proper reason
>> supplied
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> That's a question for the author of c/s 064a2652233 to answer...  I'm
> merely following the prevailing style.

"Prevailing style" seems a little bold considering that according to
my grep-ing there's exactly on other such instance (VGA). I.e. you'd
grow the "badness" by 50% when you could equally well shrink it by
this same percentage.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 5/7] x86/livepatch: Fail the build if duplicate symbols exist

[Andrew Cooper]

On 30/10/2019 08:41, Jan Beulich wrote:
> On 29.10.2019 18:06, Andrew Cooper wrote:
>> On 24/10/2019 13:03, Jan Beulich wrote:
>>> On 23.10.2019 15:58, Andrew Cooper wrote:
>>>> --- a/xen/common/Kconfig
>>>> +++ b/xen/common/Kconfig
>>>> @@ -361,9 +361,23 @@ config FAST_SYMBOL_LOOKUP
>>>>  
>>>>  	  If unsure, say Y.
>>>>  
>>>> +config ENFORCE_UNIQUE_SYMBOLS
>>>> +	bool "Enforce unique symbols" if LIVEPATCH
>>>> +	default y if LIVEPATCH
>>> Instead of two identical "if", why not "depends on LIVEPATCH"?
>>>
>>>> +	---help---
>>>> +	  Multiple symbols with the same name aren't generally a problem
>>>> +	  unless Live patching is to be used.
>>>> +
>>>> +	  Livepatch loading involves resolving relocations against symbol
>>>> +	  names, and attempting to a duplicate symbol in a livepatch will
>>>> +	  result in incorrect livepatch application.
>>>> +
>>>> +	  This option should be used to ensure that a build of Xen can have a
>>>> +	  livepatch build and apply correctly.
>>>> +
>>>>  config SUPPRESS_DUPLICATE_SYMBOL_WARNINGS
>>>> -	bool "Suppress duplicate symbol warnings" if !LIVEPATCH
>>>> -	default y if !LIVEPATCH
>>>> +	bool "Suppress duplicate symbol warnings" if !ENFORCE_UNIQUE_SYMBOLS
>>>> +	default y if !ENFORCE_UNIQUE_SYMBOLS
>>> Similarly here then. With this changed, or with a proper reason
>>> supplied
>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>> That's a question for the author of c/s 064a2652233 to answer...  I'm
>> merely following the prevailing style.
> "Prevailing style" seems a little bold considering that according to
> my grep-ing there's exactly on other such instance (VGA). I.e. you'd
> grow the "badness" by 50% when you could equally well shrink it by
> this same percentage.

Allow me to be less subtle.

*You* are the author of the code, in this form.

As a consequence, I expect there is a deliberate reason.

And seeing as I've had to reverse engineer the reason myself, it looks
to be related to the fact that other options in the build select these,
so they need not to be dependent on livepatching in the first place.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 5/7] x86/livepatch: Fail the build if duplicate symbols exist

[Jan Beulich]

On 30.10.2019 11:37, Andrew Cooper wrote:
> On 30/10/2019 08:41, Jan Beulich wrote:
>> On 29.10.2019 18:06, Andrew Cooper wrote:
>>> On 24/10/2019 13:03, Jan Beulich wrote:
>>>> On 23.10.2019 15:58, Andrew Cooper wrote:
>>>>> --- a/xen/common/Kconfig
>>>>> +++ b/xen/common/Kconfig
>>>>> @@ -361,9 +361,23 @@ config FAST_SYMBOL_LOOKUP
>>>>>  
>>>>>  	  If unsure, say Y.
>>>>>  
>>>>> +config ENFORCE_UNIQUE_SYMBOLS
>>>>> +	bool "Enforce unique symbols" if LIVEPATCH
>>>>> +	default y if LIVEPATCH
>>>> Instead of two identical "if", why not "depends on LIVEPATCH"?
>>>>
>>>>> +	---help---
>>>>> +	  Multiple symbols with the same name aren't generally a problem
>>>>> +	  unless Live patching is to be used.
>>>>> +
>>>>> +	  Livepatch loading involves resolving relocations against symbol
>>>>> +	  names, and attempting to a duplicate symbol in a livepatch will
>>>>> +	  result in incorrect livepatch application.
>>>>> +
>>>>> +	  This option should be used to ensure that a build of Xen can have a
>>>>> +	  livepatch build and apply correctly.
>>>>> +
>>>>>  config SUPPRESS_DUPLICATE_SYMBOL_WARNINGS
>>>>> -	bool "Suppress duplicate symbol warnings" if !LIVEPATCH
>>>>> -	default y if !LIVEPATCH
>>>>> +	bool "Suppress duplicate symbol warnings" if !ENFORCE_UNIQUE_SYMBOLS
>>>>> +	default y if !ENFORCE_UNIQUE_SYMBOLS
>>>> Similarly here then. With this changed, or with a proper reason
>>>> supplied
>>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>> That's a question for the author of c/s 064a2652233 to answer...  I'm
>>> merely following the prevailing style.
>> "Prevailing style" seems a little bold considering that according to
>> my grep-ing there's exactly on other such instance (VGA). I.e. you'd
>> grow the "badness" by 50% when you could equally well shrink it by
>> this same percentage.
> 
> Allow me to be less subtle.
> 
> *You* are the author of the code, in this form.

I'm sorry for not recalling.

> As a consequence, I expect there is a deliberate reason.
> 
> And seeing as I've had to reverse engineer the reason myself, it looks
> to be related to the fact that other options in the build select these,
> so they need not to be dependent on livepatching in the first place.

It wasn't without reason that I did say "or with a proper reason
supplied" - the select in xen/Kconfig.debug is a proper reason for
SUPPRESS_DUPLICATE_SYMBOL_WARNINGS staying as it is, indeed. But
it's then still not a reason for ENFORCE_UNIQUE_SYMBOLS to be
this same way, as there's no similar select for it anywhere.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH for-next v3 6/7] x86/nospec: Move array_index_mask_nospec() into nospec.h

[Andrew Cooper]

system.h isn't an appropriate place to live, now that asm/nospec.h exists.
This should arguably have been part of c/s db591d6e76e

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Juergen Gross <jgross@suse.com>

This is probably post-4.13 content
---
 xen/include/asm-x86/nospec.h | 22 ++++++++++++++++++++++
 xen/include/asm-x86/system.h | 24 ------------------------
 xen/include/xen/nospec.h     |  3 ++-
 3 files changed, 24 insertions(+), 25 deletions(-)

diff --git a/xen/include/asm-x86/nospec.h b/xen/include/asm-x86/nospec.h
index f6eb84eee5..0039cd2713 100644
--- a/xen/include/asm-x86/nospec.h
+++ b/xen/include/asm-x86/nospec.h
@@ -6,6 +6,28 @@
 
 #include <asm/alternative.h>
 
+/**
+ * array_index_mask_nospec() - generate a mask that is ~0UL when the
+ *      bounds check succeeds and 0 otherwise
+ * @index: array element index
+ * @size: number of elements in array
+ *
+ * Returns:
+ *     0 - (index < size)
+ */
+#define array_index_mask_nospec array_index_mask_nospec
+static inline unsigned long array_index_mask_nospec(unsigned long index,
+                                                    unsigned long size)
+{
+    unsigned long mask;
+
+    asm volatile ( "cmp %[size], %[index]; sbb %[mask], %[mask];"
+                   : [mask] "=r" (mask)
+                   : [size] "g" (size), [index] "r" (index) );
+
+    return mask;
+}
+
 /* Allow to insert a read memory barrier into conditionals */
 static always_inline bool barrier_nospec_true(void)
 {
diff --git a/xen/include/asm-x86/system.h b/xen/include/asm-x86/system.h
index 069f422f0d..9f1b296855 100644
--- a/xen/include/asm-x86/system.h
+++ b/xen/include/asm-x86/system.h
@@ -233,30 +233,6 @@ static always_inline unsigned long __xadd(
 #define set_mb(var, value) do { xchg(&var, value); } while (0)
 #define set_wmb(var, value) do { var = value; smp_wmb(); } while (0)
 
-/**
- * array_index_mask_nospec() - generate a mask that is ~0UL when the
- *      bounds check succeeds and 0 otherwise
- * @index: array element index
- * @size: number of elements in array
- *
- * Returns:
- *     0 - (index < size)
- */
-static inline unsigned long array_index_mask_nospec(unsigned long index,
-                                                    unsigned long size)
-{
-    unsigned long mask;
-
-    asm volatile ( "cmp %[size], %[index]; sbb %[mask], %[mask];"
-                   : [mask] "=r" (mask)
-                   : [size] "g" (size), [index] "r" (index) );
-
-    return mask;
-}
-
-/* Override default implementation in nospec.h. */
-#define array_index_mask_nospec array_index_mask_nospec
-
 #define local_irq_disable()     asm volatile ( "cli" : : : "memory" )
 #define local_irq_enable()      asm volatile ( "sti" : : : "memory" )
 
diff --git a/xen/include/xen/nospec.h b/xen/include/xen/nospec.h
index 76255bc46e..7578210f16 100644
--- a/xen/include/xen/nospec.h
+++ b/xen/include/xen/nospec.h
@@ -7,7 +7,8 @@
 #ifndef XEN_NOSPEC_H
 #define XEN_NOSPEC_H
 
-#include <asm/system.h>
+#include <xen/compiler.h>
+
 #include <asm/nospec.h>
 
 /**
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for-next v3 6/7] x86/nospec: Move array_index_mask_nospec() into nospec.h

[Jan Beulich]

On 23.10.2019 15:58, Andrew Cooper wrote:
> system.h isn't an appropriate place to live, now that asm/nospec.h exists.
> This should arguably have been part of c/s db591d6e76e
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v3 7/7] x86/nospec: Optimise array_index_mask_nospec() for power-of-2 arrays

[Andrew Cooper]

When the compiler can determine that an array bound is a power of two, the
array index can be bounded even under speculation with a single and
instruction.

Respecify array_index_mask_nospec() to allow for masks other than ~0 and 0,
and introduce an IS_POWER_OF_2() helper.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Juergen Gross <jgross@suse.com>

This optimisation is not safe on ARM, because some CPUs do data value
speculation, which is why the CSDB barrer was introduced.
---
 xen/include/asm-x86/nospec.h | 25 +++++++++++++++++++------
 xen/include/xen/config.h     |  1 +
 xen/include/xen/nospec.h     |  3 ++-
 3 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/xen/include/asm-x86/nospec.h b/xen/include/asm-x86/nospec.h
index 0039cd2713..4f36069eac 100644
--- a/xen/include/asm-x86/nospec.h
+++ b/xen/include/asm-x86/nospec.h
@@ -7,13 +7,20 @@
 #include <asm/alternative.h>
 
 /**
- * array_index_mask_nospec() - generate a mask that is ~0UL when the
- *      bounds check succeeds and 0 otherwise
+ * array_index_mask_nospec() - generate a mask to bound an array index
+ * which is safe even under adverse speculation.
  * @index: array element index
  * @size: number of elements in array
  *
- * Returns:
+ * In general, returns:
  *     0 - (index < size)
+ *
+ * This yeild ~0UL in within-bounds case, and 0 in the out-of-bounds
+ * case.
+ *
+ * When the compiler can determine that the array is a power of two, a
+ * lower overhead option is to mask the index with a single and
+ * instruction.
  */
 #define array_index_mask_nospec array_index_mask_nospec
 static inline unsigned long array_index_mask_nospec(unsigned long index,
@@ -21,9 +28,15 @@ static inline unsigned long array_index_mask_nospec(unsigned long index,
 {
     unsigned long mask;
 
-    asm volatile ( "cmp %[size], %[index]; sbb %[mask], %[mask];"
-                   : [mask] "=r" (mask)
-                   : [size] "g" (size), [index] "r" (index) );
+    if ( __builtin_constant_p(size) && IS_POWER_OF_2(size) )
+    {
+        mask = size - 1;
+        OPTIMIZER_HIDE_VAR(mask);
+    }
+    else
+        asm volatile ( "cmp %[size], %[index]; sbb %[mask], %[mask];"
+                       : [mask] "=r" (mask)
+                       : [size] "g" (size), [index] "r" (index) );
 
     return mask;
 }
diff --git a/xen/include/xen/config.h b/xen/include/xen/config.h
index a106380a23..21c763617c 100644
--- a/xen/include/xen/config.h
+++ b/xen/include/xen/config.h
@@ -75,6 +75,7 @@
 #define GB(_gb)     (_AC(_gb, ULL) << 30)
 
 #define IS_ALIGNED(val, align) (((val) & ((align) - 1)) == 0)
+#define IS_POWER_OF_2(val) ((val) && IS_ALIGNED(val, val))
 
 #define __STR(...) #__VA_ARGS__
 #define STR(...) __STR(__VA_ARGS__)
diff --git a/xen/include/xen/nospec.h b/xen/include/xen/nospec.h
index 7578210f16..cfc31f11b7 100644
--- a/xen/include/xen/nospec.h
+++ b/xen/include/xen/nospec.h
@@ -12,7 +12,8 @@
 #include <asm/nospec.h>
 
 /**
- * array_index_mask_nospec() - generate a ~0 mask when index < size, 0 otherwise
+ * array_index_mask_nospec() - generate a mask to bound an array index
+ * which is safe even under adverse speculation.
  * @index: array element index
  * @size: number of elements in array
  *
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 7/7] x86/nospec: Optimise array_index_mask_nospec() for power-of-2 arrays

[Jan Beulich]

On 23.10.2019 15:58, Andrew Cooper wrote:
> This optimisation is not safe on ARM, because some CPUs do data value
> speculation, which is why the CSDB barrer was introduced.

So if an x86 CPU appeared which did so too, it would immediately be
unsafe as well aiui. I.e. we'd have to again go and fix the logic.
Not to be taken as an outright objection, but to perhaps prompt
further consideration.

> --- a/xen/include/asm-x86/nospec.h
> +++ b/xen/include/asm-x86/nospec.h
> @@ -7,13 +7,20 @@
>  #include <asm/alternative.h>
>  
>  /**
> - * array_index_mask_nospec() - generate a mask that is ~0UL when the
> - *      bounds check succeeds and 0 otherwise
> + * array_index_mask_nospec() - generate a mask to bound an array index
> + * which is safe even under adverse speculation.
>   * @index: array element index
>   * @size: number of elements in array
>   *
> - * Returns:
> + * In general, returns:
>   *     0 - (index < size)
> + *
> + * This yeild ~0UL in within-bounds case, and 0 in the out-of-bounds

Nit: "yields"?

> @@ -21,9 +28,15 @@ static inline unsigned long array_index_mask_nospec(unsigned long index,
>  {
>      unsigned long mask;
>  
> -    asm volatile ( "cmp %[size], %[index]; sbb %[mask], %[mask];"
> -                   : [mask] "=r" (mask)
> -                   : [size] "g" (size), [index] "r" (index) );
> +    if ( __builtin_constant_p(size) && IS_POWER_OF_2(size) )
> +    {
> +        mask = size - 1;
> +        OPTIMIZER_HIDE_VAR(mask);

I can't seem to be able to figure why you need this.

> --- a/xen/include/xen/config.h
> +++ b/xen/include/xen/config.h
> @@ -75,6 +75,7 @@
>  #define GB(_gb)     (_AC(_gb, ULL) << 30)
>  
>  #define IS_ALIGNED(val, align) (((val) & ((align) - 1)) == 0)
> +#define IS_POWER_OF_2(val) ((val) && IS_ALIGNED(val, val))

While the risk may seem low for someone to pass an expression with
side effect here, evaluating "val" up to three times here doesn't
look very desirable.

As a minor remark, without considering representation I'd expect
an expression IS_ALIGNED(val, val) to consistently produce "true"
for all non-zero values. E.g. 3 is certainly "aligned" on a
boundary of 3.

Finally this may want guarding against use on signed types - at
the very least it looks to produce the wrong answer for e.g.
INT_MIN or LONG_MIN. I.e. perhaps the expression to the left of
&& wants to be (val) > 0.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 7/7] x86/nospec: Optimise array_index_mask_nospec() for power-of-2 arrays

[Andrew Cooper]

On 25/10/2019 13:24, Jan Beulich wrote:
> On 23.10.2019 15:58, Andrew Cooper wrote:
>> This optimisation is not safe on ARM, because some CPUs do data value
>> speculation, which is why the CSDB barrer was introduced.
> So if an x86 CPU appeared which did so too, it would immediately be
> unsafe as well aiui. I.e. we'd have to again go and fix the logic.
> Not to be taken as an outright objection, but to perhaps prompt
> further consideration.

Actually any masking approach, even cmp/sbb, would be unsafe so I
suppose this note isn't accurate.

I'm aware of one x86 plan for data value speculation, which was delayed
indefinitely following the fallout from Spectre/Meltdown, especially as
L1TF at its core is a speculative address prediction bug.  Suffice it to
say that the vendors are aware that any plans along these lines will
need to be done with great care.

>
>> --- a/xen/include/asm-x86/nospec.h
>> +++ b/xen/include/asm-x86/nospec.h
>> @@ -7,13 +7,20 @@
>>  #include <asm/alternative.h>
>>  
>>  /**
>> - * array_index_mask_nospec() - generate a mask that is ~0UL when the
>> - *      bounds check succeeds and 0 otherwise
>> + * array_index_mask_nospec() - generate a mask to bound an array index
>> + * which is safe even under adverse speculation.
>>   * @index: array element index
>>   * @size: number of elements in array
>>   *
>> - * Returns:
>> + * In general, returns:
>>   *     0 - (index < size)
>> + *
>> + * This yeild ~0UL in within-bounds case, and 0 in the out-of-bounds
> Nit: "yields"?

Oops yes.

>
>> @@ -21,9 +28,15 @@ static inline unsigned long array_index_mask_nospec(unsigned long index,
>>  {
>>      unsigned long mask;
>>  
>> -    asm volatile ( "cmp %[size], %[index]; sbb %[mask], %[mask];"
>> -                   : [mask] "=r" (mask)
>> -                   : [size] "g" (size), [index] "r" (index) );
>> +    if ( __builtin_constant_p(size) && IS_POWER_OF_2(size) )
>> +    {
>> +        mask = size - 1;
>> +        OPTIMIZER_HIDE_VAR(mask);
> I can't seem to be able to figure why you need this.

Because I found cases where the AND was elided by the compiler entirely
without it.

>
>> --- a/xen/include/xen/config.h
>> +++ b/xen/include/xen/config.h
>> @@ -75,6 +75,7 @@
>>  #define GB(_gb)     (_AC(_gb, ULL) << 30)
>>  
>>  #define IS_ALIGNED(val, align) (((val) & ((align) - 1)) == 0)
>> +#define IS_POWER_OF_2(val) ((val) && IS_ALIGNED(val, val))
> While the risk may seem low for someone to pass an expression with
> side effect here, evaluating "val" up to three times here doesn't
> look very desirable.

That is easy to fix.

> As a minor remark, without considering representation I'd expect
> an expression IS_ALIGNED(val, val) to consistently produce "true"
> for all non-zero values. E.g. 3 is certainly "aligned" on a
> boundary of 3.

IS_ALIGNED() comes with an expectation of being against a power of 2,
because otherwise you'd need a DIV instruction for the general case.

Some users can't cope with a compile time check.

> Finally this may want guarding against use on signed types - at
> the very least it looks to produce the wrong answer for e.g.
> INT_MIN or LONG_MIN. I.e. perhaps the expression to the left of
> && wants to be (val) > 0.

How about the above expansion fix becoming:

({
    unsigned typeof(val) _val = val;
    _val && (_val & (_val - 1)) == 0;
})

This check makes no sense on negative numbers.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 7/7] x86/nospec: Optimise array_index_mask_nospec() for power-of-2 arrays

[Jan Beulich]

On 25.10.2019 14:58, Andrew Cooper wrote:
> On 25/10/2019 13:24, Jan Beulich wrote:
>> On 23.10.2019 15:58, Andrew Cooper wrote:
>>> @@ -21,9 +28,15 @@ static inline unsigned long array_index_mask_nospec(unsigned long index,
>>>  {
>>>      unsigned long mask;
>>>  
>>> -    asm volatile ( "cmp %[size], %[index]; sbb %[mask], %[mask];"
>>> -                   : [mask] "=r" (mask)
>>> -                   : [size] "g" (size), [index] "r" (index) );
>>> +    if ( __builtin_constant_p(size) && IS_POWER_OF_2(size) )
>>> +    {
>>> +        mask = size - 1;
>>> +        OPTIMIZER_HIDE_VAR(mask);
>> I can't seem to be able to figure why you need this.
> 
> Because I found cases where the AND was elided by the compiler entirely
> without it.

Would you mind mentioning this in the description, or in a comment?

>>> --- a/xen/include/xen/config.h
>>> +++ b/xen/include/xen/config.h
>>> @@ -75,6 +75,7 @@
>>>  #define GB(_gb)     (_AC(_gb, ULL) << 30)
>>>  
>>>  #define IS_ALIGNED(val, align) (((val) & ((align) - 1)) == 0)
>>> +#define IS_POWER_OF_2(val) ((val) && IS_ALIGNED(val, val))
>> While the risk may seem low for someone to pass an expression with
>> side effect here, evaluating "val" up to three times here doesn't
>> look very desirable.
> 
> That is easy to fix.
> 
>> As a minor remark, without considering representation I'd expect
>> an expression IS_ALIGNED(val, val) to consistently produce "true"
>> for all non-zero values. E.g. 3 is certainly "aligned" on a
>> boundary of 3.
> 
> IS_ALIGNED() comes with an expectation of being against a power of 2,
> because otherwise you'd need a DIV instruction for the general case.
> 
> Some users can't cope with a compile time check.
> 
>> Finally this may want guarding against use on signed types - at
>> the very least it looks to produce the wrong answer for e.g.
>> INT_MIN or LONG_MIN. I.e. perhaps the expression to the left of
>> && wants to be (val) > 0.
> 
> How about the above expansion fix becoming:
> 
> ({
>     unsigned typeof(val) _val = val;
>     _val && (_val & (_val - 1)) == 0;
> })

Well, if the "unsigned typeof()" construct works - why not (with
"val" properly parenthesized, and preferable the leading underscore
changed to a trailing one).

> This check makes no sense on negative numbers.

Of course not, but someone might use it on a signed type and get
back true when it was supposed to be false, just because the value
used ended up being a negative number.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel