From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH for-next 2/3] xen/x86: Introduce static inline wrappers for l{idt, gdt, ldt, tr}() Date: Thu, 12 Oct 2017 17:06:32 +0100 Message-ID: References: <1506960829-18991-1-git-send-email-andrew.cooper3@citrix.com> <1506960829-18991-3-git-send-email-andrew.cooper3@citrix.com> <59DFAC370200007800185BE4@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0665811777337183879==" Return-path: In-Reply-To: <59DFAC370200007800185BE4@prv-mh.provo.novell.com> Content-Language: en-GB List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Jan Beulich Cc: George Dunlap , Wei Liu , Xen-devel List-Id: xen-devel@lists.xenproject.org --===============0665811777337183879== Content-Type: multipart/alternative; boundary="------------511B09AAA8708BC80EB555D7" Content-Language: en-GB --------------511B09AAA8708BC80EB555D7 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit On 12/10/17 16:53, Jan Beulich wrote: >>>> On 02.10.17 at 18:13, 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 Does this still stand in light of the barrier change above? ~Andrew --------------511B09AAA8708BC80EB555D7 Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: 8bit
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
--------------511B09AAA8708BC80EB555D7-- --===============0665811777337183879== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwczovL2xpc3RzLnhlbi5v cmcveGVuLWRldmVsCg== --===============0665811777337183879==--