[PATCH for-next 0/3] xen/x86: Misc improvements

[PATCH for-next 0/3] xen/x86: Misc improvements

[Andrew Cooper]

Andrew Cooper (3):
  x86/smp: Rework cpu_smpboot_alloc() to cope with more than just
    -ENOMEM
  xen/x86: Introduce static inline wrappers for l{idt,gdt,ldt,tr}()
  x86/ldt: Alter how invalidate_shadow_ldt() deals with TLB flushes

 xen/arch/x86/cpu/common.c  |  8 ++++----
 xen/arch/x86/domain.c      |  6 ++++--
 xen/arch/x86/mm.c          | 39 +++++++++++++++++++++++++--------------
 xen/arch/x86/smpboot.c     | 31 ++++++++++++++++++-------------
 xen/common/efi/runtime.c   |  4 ++--
 xen/include/asm-x86/desc.h | 20 ++++++++++++++++++++
 xen/include/asm-x86/ldt.h  |  6 ++----
 7 files changed, 75 insertions(+), 39 deletions(-)

-- 
2.1.4


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

[PATCH for-next 1/3] x86/smp: Rework cpu_smpboot_alloc() to cope with more than just -ENOMEM

[Andrew Cooper]

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>
---
 xen/arch/x86/smpboot.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index 3ca716c..a3c42ea 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -696,33 +696,34 @@ static int cpu_smpboot_alloc(unsigned int cpu)
     nodeid_t node = cpu_to_node(cpu);
     struct desc_struct *gdt;
     unsigned long stub_page;
+    int rc = -ENOMEM;
 
     if ( node != NUMA_NO_NODE )
         memflags = MEMF_node(node);
 
     stack_base[cpu] = alloc_xenheap_pages(STACK_ORDER, memflags);
     if ( stack_base[cpu] == NULL )
-        goto oom;
+        goto out;
     memguard_guard_stack(stack_base[cpu]);
 
     order = get_order_from_pages(NR_RESERVED_GDT_PAGES);
     per_cpu(gdt_table, cpu) = gdt = alloc_xenheap_pages(order, memflags);
     if ( gdt == NULL )
-        goto oom;
+        goto out;
     memcpy(gdt, boot_cpu_gdt_table, NR_RESERVED_GDT_PAGES * PAGE_SIZE);
     BUILD_BUG_ON(NR_CPUS > 0x10000);
     gdt[PER_CPU_GDT_ENTRY - FIRST_RESERVED_GDT_ENTRY].a = cpu;
 
     per_cpu(compat_gdt_table, cpu) = gdt = alloc_xenheap_pages(order, memflags);
     if ( gdt == NULL )
-        goto oom;
+        goto out;
     memcpy(gdt, boot_cpu_compat_gdt_table, NR_RESERVED_GDT_PAGES * PAGE_SIZE);
     gdt[PER_CPU_GDT_ENTRY - FIRST_RESERVED_GDT_ENTRY].a = cpu;
 
     order = get_order_from_bytes(IDT_ENTRIES * sizeof(idt_entry_t));
     idt_tables[cpu] = alloc_xenheap_pages(order, memflags);
     if ( idt_tables[cpu] == NULL )
-        goto oom;
+        goto out;
     memcpy(idt_tables[cpu], idt_table, IDT_ENTRIES * sizeof(idt_entry_t));
 
     for ( stub_page = 0, i = cpu & ~(STUBS_PER_PAGE - 1);
@@ -735,21 +736,25 @@ static int cpu_smpboot_alloc(unsigned int cpu)
     BUG_ON(i == cpu);
     stub_page = alloc_stub_page(cpu, &per_cpu(stubs.mfn, cpu));
     if ( !stub_page )
-        goto oom;
+        goto out;
     per_cpu(stubs.addr, cpu) = stub_page + STUB_BUF_CPU_OFFS(cpu);
 
     if ( secondary_socket_cpumask == NULL &&
          (secondary_socket_cpumask = xzalloc(cpumask_t)) == NULL )
-        goto oom;
+        goto out;
 
-    if ( zalloc_cpumask_var(&per_cpu(cpu_sibling_mask, cpu)) &&
-         zalloc_cpumask_var(&per_cpu(cpu_core_mask, cpu)) &&
-         alloc_cpumask_var(&per_cpu(scratch_cpumask, cpu)) )
-        return 0;
+    if ( !(zalloc_cpumask_var(&per_cpu(cpu_sibling_mask, cpu)) &&
+           zalloc_cpumask_var(&per_cpu(cpu_core_mask, cpu)) &&
+           alloc_cpumask_var(&per_cpu(scratch_cpumask, cpu))) )
+        goto out;
+
+    rc = 0;
+
+ out:
+    if ( rc )
+        cpu_smpboot_free(cpu);
 
- oom:
-    cpu_smpboot_free(cpu);
-    return -ENOMEM;
+    return rc;
 }
 
 static int cpu_smpboot_callback(
-- 
2.1.4


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

[PATCH for-next 2/3] xen/x86: Introduce static inline wrappers for l{idt, gdt, ldt, tr}()

[Andrew Cooper]

This avoids indirection and parameter constraint issues.  Doing so relaxes the
load_LDT() constraints from %ax to any general purpose register.

The triple-fault reboot method stays as is, to avoid the int3 possibly getting
moved relative to the lidt.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>
---
 xen/arch/x86/cpu/common.c  |  8 ++++----
 xen/arch/x86/domain.c      |  6 ++++--
 xen/common/efi/runtime.c   |  4 ++--
 xen/include/asm-x86/desc.h | 20 ++++++++++++++++++++
 xen/include/asm-x86/ldt.h  |  6 ++----
 5 files changed, 32 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index 78f5667..9c50ec6 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -697,10 +697,10 @@ void load_system_tables(void)
 		offsetof(struct tss_struct, __cacheline_filler) - 1,
 		SYS_DESC_tss_busy);
 
-	asm volatile ("lgdt %0"  : : "m"  (gdtr) );
-	asm volatile ("lidt %0"  : : "m"  (idtr) );
-	asm volatile ("ltr  %w0" : : "rm" (TSS_ENTRY << 3) );
-	asm volatile ("lldt %w0" : : "rm" (0) );
+	lgdt(&gdtr);
+	lidt(&idtr);
+	ltr(TSS_ENTRY << 3);
+	lldt(0);
 
 	/*
 	 * Bottom-of-stack must be 16-byte aligned!
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 466a1a2..dfe905f 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1625,7 +1625,8 @@ static void __context_switch(void)
     {
         gdt_desc.limit = LAST_RESERVED_GDT_BYTE;
         gdt_desc.base  = (unsigned long)(gdt - FIRST_RESERVED_GDT_ENTRY);
-        asm volatile ( "lgdt %0" : : "m" (gdt_desc) );
+
+        lgdt(&gdt_desc);
     }
 
     write_ptbase(n);
@@ -1635,7 +1636,8 @@ static void __context_switch(void)
     {
         gdt_desc.limit = LAST_RESERVED_GDT_BYTE;
         gdt_desc.base = GDT_VIRT_START(n);
-        asm volatile ( "lgdt %0" : : "m" (gdt_desc) );
+
+        lgdt(&gdt_desc);
     }
 
     if ( pd != nd )
diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c
index c38f00a..3dbc2e8 100644
--- a/xen/common/efi/runtime.c
+++ b/xen/common/efi/runtime.c
@@ -108,7 +108,7 @@ struct efi_rs_state efi_rs_enter(void)
                                      FIRST_RESERVED_GDT_ENTRY)
         };
 
-        asm volatile ( "lgdt %0" : : "m" (gdt_desc) );
+        lgdt(&gdt_desc);
     }
 
     write_cr3(virt_to_maddr(efi_l4_pgtable));
@@ -128,7 +128,7 @@ void efi_rs_leave(struct efi_rs_state *state)
             .base  = GDT_VIRT_START(current)
         };
 
-        asm volatile ( "lgdt %0" : : "m" (gdt_desc) );
+        lgdt(&gdt_desc);
     }
     irq_exit();
     efi_rs_on_cpu = NR_CPUS;
diff --git a/xen/include/asm-x86/desc.h b/xen/include/asm-x86/desc.h
index da924bf..1ec320e 100644
--- a/xen/include/asm-x86/desc.h
+++ b/xen/include/asm-x86/desc.h
@@ -197,6 +197,26 @@ DECLARE_PER_CPU(struct desc_struct *, compat_gdt_table);
 
 extern void load_TR(void);
 
+static inline void lgdt(const struct desc_ptr *gdtr)
+{
+    asm volatile ("lgdt %0" :: "m" (*gdtr));
+}
+
+static inline void lidt(const struct desc_ptr *idtr)
+{
+    asm volatile ("lidt %0" :: "m" (*idtr));
+}
+
+static inline void lldt(unsigned int sel)
+{
+    asm volatile ("lldt %w0" :: "rm" (sel));
+}
+
+static inline void ltr(unsigned int sel)
+{
+    asm volatile ("ltr %w0" :: "rm" (sel));
+}
+
 #endif /* !__ASSEMBLY__ */
 
 #endif /* __ARCH_DESC_H */
diff --git a/xen/include/asm-x86/ldt.h b/xen/include/asm-x86/ldt.h
index 289ae19..589daf8 100644
--- a/xen/include/asm-x86/ldt.h
+++ b/xen/include/asm-x86/ldt.h
@@ -10,16 +10,14 @@ static inline void load_LDT(struct vcpu *v)
     unsigned long ents;
 
     if ( (ents = v->arch.pv_vcpu.ldt_ents) == 0 )
-    {
-        __asm__ __volatile__ ( "lldt %%ax" : : "a" (0) );
-    }
+        lldt(0);
     else
     {
         desc = (!is_pv_32bit_vcpu(v)
                 ? this_cpu(gdt_table) : this_cpu(compat_gdt_table))
                + LDT_ENTRY - FIRST_RESERVED_GDT_ENTRY;
         _set_tssldt_desc(desc, LDT_VIRT_START(v), ents*8-1, SYS_DESC_ldt);
-        __asm__ __volatile__ ( "lldt %%ax" : : "a" (LDT_ENTRY << 3) );
+        lldt(LDT_ENTRY << 3);
     }
 }
 
-- 
2.1.4


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

[PATCH for-next 3/3] x86/ldt: Alter how invalidate_shadow_ldt() deals with TLB flushes

[Andrew Cooper]

Modify invalidate_shadow_ldt() to return a boolean indicating whether mappings
have been dropped, rather than taking a flush parameter.  Tweak the internal
logic to be able to ASSERT() that v->arch.pv_vcpu.shadow_ldt_mapcnt matches
the number of PTEs removed.

This allows MMUEXTOP_SET_LDT to avoid a local TLB flush if no LDT entries had
been faulted in to begin with.

Finally, correct a comment in __get_page_type().  Under no circumstance is it
safe to forgo the TLB shootdown for GDT/LDT pages, as that would allow one
vcpu to gain a writeable mapping to a frame still mapped as a GDT/LDT by
another vcpu.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/mm.c | 39 +++++++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 14 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index d9df5ca..37de4ff 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -505,10 +505,14 @@ static inline void page_set_tlbflush_timestamp(struct page_info *page)
 const char __section(".bss.page_aligned.const") __aligned(PAGE_SIZE)
     zero_page[PAGE_SIZE];
 
-static void invalidate_shadow_ldt(struct vcpu *v, int flush)
+/*
+ * Flush the LDT, dropping any typerefs.  Returns a boolean indicating whether
+ * mappings have been removed (i.e. a TLB flush is needed).
+ */
+static bool invalidate_shadow_ldt(struct vcpu *v)
 {
     l1_pgentry_t *pl1e;
-    unsigned int i;
+    unsigned int i, mappings_dropped = 0;
     struct page_info *page;
 
     BUG_ON(unlikely(in_irq()));
@@ -518,26 +522,29 @@ static void invalidate_shadow_ldt(struct vcpu *v, int flush)
     if ( v->arch.pv_vcpu.shadow_ldt_mapcnt == 0 )
         goto out;
 
-    v->arch.pv_vcpu.shadow_ldt_mapcnt = 0;
     pl1e = pv_ldt_ptes(v);
 
     for ( i = 0; i < 16; i++ )
     {
         if ( !(l1e_get_flags(pl1e[i]) & _PAGE_PRESENT) )
             continue;
+
         page = l1e_get_page(pl1e[i]);
         l1e_write(&pl1e[i], l1e_empty());
+        mappings_dropped++;
+
         ASSERT_PAGE_IS_TYPE(page, PGT_seg_desc_page);
         ASSERT_PAGE_IS_DOMAIN(page, v->domain);
         put_page_and_type(page);
     }
 
-    /* Rid TLBs of stale mappings (guest mappings and shadow mappings). */
-    if ( flush )
-        flush_tlb_mask(v->vcpu_dirty_cpumask);
+    ASSERT(v->arch.pv_vcpu.shadow_ldt_mapcnt == mappings_dropped);
+    v->arch.pv_vcpu.shadow_ldt_mapcnt = 0;
 
  out:
     spin_unlock(&v->arch.pv_vcpu.shadow_ldt_lock);
+
+    return !!mappings_dropped;
 }
 
 
@@ -1087,7 +1094,10 @@ void put_page_from_l1e(l1_pgentry_t l1e, struct domain *l1e_owner)
              (l1e_owner == pg_owner) )
         {
             for_each_vcpu ( pg_owner, v )
-                invalidate_shadow_ldt(v, 1);
+            {
+                if ( invalidate_shadow_ldt(v) )
+                    flush_tlb_mask(v->vcpu_dirty_cpumask);
+            }
         }
         put_page(page);
     }
@@ -2237,9 +2247,9 @@ static int __get_page_type(struct page_info *page, unsigned long type,
             if ( (x & PGT_type_mask) != type )
             {
                 /*
-                 * On type change we check to flush stale TLB entries. This
-                 * may be unnecessary (e.g., page was GDT/LDT) but those
-                 * circumstances should be very rare.
+                 * On type change we check to flush stale TLB entries. It is
+                 * vital that no other CPUs are left with mappings of a frame
+                 * which is about to become writeable to the guest.
                  */
                 cpumask_t *mask = this_cpu(scratch_cpumask);
 
@@ -2486,7 +2496,7 @@ int new_guest_cr3(mfn_t mfn)
             return rc;
         }
 
-        invalidate_shadow_ldt(curr, 0);
+        invalidate_shadow_ldt(curr); /* Unconditional TLB flush later. */
         write_ptbase(curr);
 
         return 0;
@@ -2524,7 +2534,7 @@ int new_guest_cr3(mfn_t mfn)
         return rc;
     }
 
-    invalidate_shadow_ldt(curr, 0);
+    invalidate_shadow_ldt(curr); /* Unconditional TLB flush later. */
 
     if ( !VM_ASSIST(d, m2p_strict) && !paging_mode_refcounts(d) )
         fill_ro_mpt(mfn);
@@ -3025,8 +3035,9 @@ long do_mmuext_op(
             else if ( (curr->arch.pv_vcpu.ldt_ents != ents) ||
                       (curr->arch.pv_vcpu.ldt_base != ptr) )
             {
-                invalidate_shadow_ldt(curr, 0);
-                flush_tlb_local();
+                if ( invalidate_shadow_ldt(curr) )
+                    flush_tlb_local();
+
                 curr->arch.pv_vcpu.ldt_base = ptr;
                 curr->arch.pv_vcpu.ldt_ents = ents;
                 load_LDT(curr);
-- 
2.1.4


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

Re: [PATCH for-next 1/3] x86/smp: Rework cpu_smpboot_alloc() to cope with more than just -ENOMEM

[Jan Beulich]

>>> On 02.10.17 at 18:13, <andrew.cooper3@citrix.com> wrote:
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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



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

Re: [PATCH for-next 2/3] xen/x86: Introduce static inline wrappers for l{idt, gdt, ldt, tr}()

[Jan Beulich]

>>> On 02.10.17 at 18:13, <andrew.cooper3@citrix.com> wrote:
> The triple-fault reboot method stays as is, to avoid the int3 possibly getting
> moved relative to the lidt.

Aren't asm volatile()s ordered wrt to one another?

> --- a/xen/include/asm-x86/desc.h
> +++ b/xen/include/asm-x86/desc.h
> @@ -197,6 +197,26 @@ DECLARE_PER_CPU(struct desc_struct *, compat_gdt_table);
>  
>  extern void load_TR(void);
>  
> +static inline void lgdt(const struct desc_ptr *gdtr)
> +{
> +    asm volatile ("lgdt %0" :: "m" (*gdtr));
> +}
> +
> +static inline void lidt(const struct desc_ptr *idtr)
> +{
> +    asm volatile ("lidt %0" :: "m" (*idtr));
> +}
> +
> +static inline void lldt(unsigned int sel)
> +{
> +    asm volatile ("lldt %w0" :: "rm" (sel));
> +}
> +
> +static inline void ltr(unsigned int sel)
> +{
> +    asm volatile ("ltr %w0" :: "rm" (sel));
> +}

As can be seen from the code you replace in ldt.h, in headers we
generally prefer to use __asm__ (and __volatile__ where needed).
I'm sure this isn't consistent, so I won't insist. However, style-wise
please add blanks immediately inside the parentheses. With at least
this last point taken care of
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan


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

Re: [PATCH for-next 3/3] x86/ldt: Alter how invalidate_shadow_ldt() deals with TLB flushes

[Jan Beulich]

>>> On 02.10.17 at 18:13, <andrew.cooper3@citrix.com> wrote:
> @@ -518,26 +522,29 @@ static void invalidate_shadow_ldt(struct vcpu *v, int flush)
>      if ( v->arch.pv_vcpu.shadow_ldt_mapcnt == 0 )
>          goto out;
>  
> -    v->arch.pv_vcpu.shadow_ldt_mapcnt = 0;
>      pl1e = pv_ldt_ptes(v);
>  
>      for ( i = 0; i < 16; i++ )
>      {
>          if ( !(l1e_get_flags(pl1e[i]) & _PAGE_PRESENT) )
>              continue;
> +
>          page = l1e_get_page(pl1e[i]);
>          l1e_write(&pl1e[i], l1e_empty());
> +        mappings_dropped++;
> +
>          ASSERT_PAGE_IS_TYPE(page, PGT_seg_desc_page);
>          ASSERT_PAGE_IS_DOMAIN(page, v->domain);
>          put_page_and_type(page);
>      }
>  
> -    /* Rid TLBs of stale mappings (guest mappings and shadow mappings). */
> -    if ( flush )
> -        flush_tlb_mask(v->vcpu_dirty_cpumask);
> +    ASSERT(v->arch.pv_vcpu.shadow_ldt_mapcnt == mappings_dropped);
> +    v->arch.pv_vcpu.shadow_ldt_mapcnt = 0;
>  
>   out:
>      spin_unlock(&v->arch.pv_vcpu.shadow_ldt_lock);
> +
> +    return !!mappings_dropped;

You don't need the !! here. With that
Reviewed-by: Jan Beulich <jbeulich@suse.com>


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

Re: [PATCH for-next 2/3] xen/x86: Introduce static inline wrappers for l{idt, gdt, ldt, tr}()

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 1894 bytes --]

On 12/10/17 16:53, Jan Beulich wrote:
>>>> On 02.10.17 at 18:13, <andrew.cooper3@citrix.com> wrote:
>> The triple-fault reboot method stays as is, to avoid the int3 possibly getting
>> moved relative to the lidt.
> Aren't asm volatile()s ordered wrt to one another?

>From the docs

"Note that the compiler can move even volatile |asm| instructions
relative to other code, including across jump instructions."

Also, I seem to recall Tim finding an example where GCC 6 did reorder
two asm volatiles relative to each other, due to their output operands
not being causally linked.

On that note however, these should gain memory clobbers to make them
full barriers.  l{i,g}dt() are serialising, while nothing good will come
of having a segment register access reordered with respect to l{g,l}dt().

>
>> --- a/xen/include/asm-x86/desc.h
>> +++ b/xen/include/asm-x86/desc.h
>> @@ -197,6 +197,26 @@ DECLARE_PER_CPU(struct desc_struct *, compat_gdt_table);
>>  
>>  extern void load_TR(void);
>>  
>> +static inline void lgdt(const struct desc_ptr *gdtr)
>> +{
>> +    asm volatile ("lgdt %0" :: "m" (*gdtr));
>> +}
>> +
>> +static inline void lidt(const struct desc_ptr *idtr)
>> +{
>> +    asm volatile ("lidt %0" :: "m" (*idtr));
>> +}
>> +
>> +static inline void lldt(unsigned int sel)
>> +{
>> +    asm volatile ("lldt %w0" :: "rm" (sel));
>> +}
>> +
>> +static inline void ltr(unsigned int sel)
>> +{
>> +    asm volatile ("ltr %w0" :: "rm" (sel));
>> +}
> As can be seen from the code you replace in ldt.h, in headers we
> generally prefer to use __asm__ (and __volatile__ where needed).
> I'm sure this isn't consistent, so I won't insist. However, style-wise
> please add blanks immediately inside the parentheses. With at least
> this last point taken care of

Will do.

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

Does this still stand in light of the barrier change above?

~Andrew

[-- Attachment #1.2: Type: text/html, Size: 3071 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH for-next 2/3] xen/x86: Introduce static inline wrappers for l{idt, gdt, ldt, tr}()

[Jan Beulich]

>>> On 12.10.17 at 18:06, <andrew.cooper3@citrix.com> wrote:
> On 12/10/17 16:53, Jan Beulich wrote:
>>>>> On 02.10.17 at 18:13, <andrew.cooper3@citrix.com> wrote:
>>> The triple-fault reboot method stays as is, to avoid the int3 possibly getting
>>> moved relative to the lidt.
>> Aren't asm volatile()s ordered wrt to one another?
> 
> From the docs
> 
> "Note that the compiler can move even volatile |asm| instructions
> relative to other code, including across jump instructions."

I had looked at the doc (and this sentence) before replying: It
does not make explicit whether two volatile asm()s can be
legitimately moved. After all the purpose of the volatile is to
deal with resource (register) uses which can't otherwise be
expressed to the compiler. The example they give there with
the floating point control/status register isn't sufficient, as it
doesn't show how an asm() producing and one consuming the
resource would interact.

But yes, to be on the safe side keeping insn here both in one
asm() is certainly the better option.

> Also, I seem to recall Tim finding an example where GCC 6 did reorder
> two asm volatiles relative to each other, due to their output operands
> not being causally linked.

Oh, good to know. In which case using a fake operand would
help to establish a dependency (like suggested in that PPC
example in the doc, albeit partially wrong afaict in that the
asm() writing a variable the subsequent statement also write
does not make this a valid dependency - the asm() would need
to write x or y imo), but of course that wouldn't make things
any better for the specific triple fault case here.

> On that note however, these should gain memory clobbers to make them
> full barriers.  l{i,g}dt() are serialising, while nothing good will come
> of having a segment register access reordered with respect to l{g,l}dt().
>[...]
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> Does this still stand in light of the barrier change above?

Sure.

Jan


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

Re: [PATCH for-next 1/3] x86/smp: Rework cpu_smpboot_alloc() to cope with more than just -ENOMEM

[Wei Liu]

On Mon, Oct 02, 2017 at 05:13:47PM +0100, Andrew Cooper wrote:
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

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