| Message ID | 1483641737-26941-1-git-send-email-anshul.makkar@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 05.01.17 at 19:42, <anshul.makkar@citrix.com> wrote: > +static always_inline u64 __vmptrst(void) > +{ > + u64 paddr; > + > + asm volatile ( > +#ifdef HAVE_GAS_VMX > + "vmptrst %0\n" > +#else > + VMPTRST_OPCODE MODRM_EAX_07 > +#endif > + > +#ifdef HAVE_GAS_VMX > + : "=m" (paddr) > + : > +#else > + : > + : "a" (&paddr), > +#endif > + : "memory"); I don't see the point of the memory clobber here in the HAVE_GAS_VMX case (and in the other case it could be easily avoided by making the output common). In fact some time ago I did raise the question already as to whether some of the other inline functions shouldn't also be relaxed. Also there's a missing blank before the closing parenthesis. Beyond that I'll leave it to the VMX maintainers to decide whether having an unused function in the code base is a good idea. Jan
On 06/01/17 14:37, Jan Beulich wrote: >>>> On 05.01.17 at 19:42, <anshul.makkar@citrix.com> wrote: >> +static always_inline u64 __vmptrst(void) >> +{ >> + u64 paddr; >> + >> + asm volatile ( >> +#ifdef HAVE_GAS_VMX >> + "vmptrst %0\n" >> +#else >> + VMPTRST_OPCODE MODRM_EAX_07 >> +#endif >> + >> +#ifdef HAVE_GAS_VMX >> + : "=m" (paddr) >> + : >> +#else >> + : >> + : "a" (&paddr), >> +#endif >> + : "memory"); > I don't see the point of the memory clobber here in the > HAVE_GAS_VMX case (and in the other case it could be easily > avoided by making the output common). Currently it is the only thing covering the fact that paddr actually gets written to. > In fact some time ago I > did raise the question already as to whether some of the other > inline functions shouldn't also be relaxed. Didn't we agree that removing the memory clobbers was a good thing to do? I recall that you asked, but I don't recall what the outcome was. ~Andrew
>>> On 06.01.17 at 15:39, <andrew.cooper3@citrix.com> wrote: > On 06/01/17 14:37, Jan Beulich wrote: >>>>> On 05.01.17 at 19:42, <anshul.makkar@citrix.com> wrote: >>> +static always_inline u64 __vmptrst(void) >>> +{ >>> + u64 paddr; >>> + >>> + asm volatile ( >>> +#ifdef HAVE_GAS_VMX >>> + "vmptrst %0\n" >>> +#else >>> + VMPTRST_OPCODE MODRM_EAX_07 >>> +#endif >>> + >>> +#ifdef HAVE_GAS_VMX >>> + : "=m" (paddr) >>> + : >>> +#else >>> + : >>> + : "a" (&paddr), >>> +#endif >>> + : "memory"); >> I don't see the point of the memory clobber here in the >> HAVE_GAS_VMX case (and in the other case it could be easily >> avoided by making the output common). > > Currently it is the only thing covering the fact that paddr actually > gets written to. Well, see especially my remark in parentheses. >> In fact some time ago I >> did raise the question already as to whether some of the other >> inline functions shouldn't also be relaxed. > > Didn't we agree that removing the memory clobbers was a good thing to > do? I recall that you asked, but I don't recall what the outcome was. Afair the outcome was at best ambiguous, or else I would either have done it already, or I would at least have an item on my todo list. Actually, looking through patch history I did do it for vmread/ vmwrite, so I assume there was a reason to leave the others alone (possibly just to be overcautious). Jan
> From: Anshul Makkar [mailto:anshul.makkar@citrix.com] > Sent: Friday, January 06, 2017 2:42 AM > > Current codebase doesn't implement and use vmptrst. Implementing it as it may > be required in future. > > Signed-off-by: Anshul Makkar <anshul.makkar@citrix.com> Then let's do it when it's really required. :-) Thanks Kevin
On 11/01/17 05:37, Tian, Kevin wrote: >> From: Anshul Makkar [mailto:anshul.makkar@citrix.com] >> Sent: Friday, January 06, 2017 2:42 AM >> >> Current codebase doesn't implement and use vmptrst. Implementing it as it may >> be required in future. >> >> Signed-off-by: Anshul Makkar <anshul.makkar@citrix.com> > Then let's do it when it's really required. :-) I needed it to debug an issue, found it missing and that's why I added. I have a sanity test, that continuously uses it. There may be other users out there who can use it for similar or some other purpose. Having it in standard code base will do no harm. > > Thanks > Kevin Anshul
diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h b/xen/include/asm-x86/hvm/vmx/vmx.h index e5c6499..2db6c1d 100644 --- a/xen/include/asm-x86/hvm/vmx/vmx.h +++ b/xen/include/asm-x86/hvm/vmx/vmx.h @@ -328,6 +328,28 @@ static always_inline void __vmptrld(u64 addr) : "memory"); } +static always_inline u64 __vmptrst(void) +{ + u64 paddr; + + asm volatile ( +#ifdef HAVE_GAS_VMX + "vmptrst %0\n" +#else + VMPTRST_OPCODE MODRM_EAX_07 +#endif + +#ifdef HAVE_GAS_VMX + : "=m" (paddr) + : +#else + : + : "a" (&paddr), +#endif + : "memory"); + return paddr; +} + static always_inline void __vmpclear(u64 addr) { asm volatile (
Current codebase doesn't implement and use vmptrst. Implementing it as it may be required in future. Signed-off-by: Anshul Makkar <anshul.makkar@citrix.com> --- xen/include/asm-x86/hvm/vmx/vmx.h | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)