From patchwork Tue Feb 7 15:06:46 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Sergey Dyasli X-Patchwork-Id: 9560337 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id A417460434 for ; Tue, 7 Feb 2017 15:13:04 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 968F92816B for ; Tue, 7 Feb 2017 15:13:04 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 8B29128401; Tue, 7 Feb 2017 15:13:04 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.2 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_MED autolearn=ham version=3.3.1 Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 15CAC2816B for ; Tue, 7 Feb 2017 15:13:04 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1cb7Pe-0004kk-5M; Tue, 07 Feb 2017 15:10:34 +0000 Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1cb7Pd-0004ke-8W for xen-devel@lists.xen.org; Tue, 07 Feb 2017 15:10:33 +0000 Received: from [85.158.139.211] by server-10.bemta-5.messagelabs.com id CB/60-01703-863E9985; Tue, 07 Feb 2017 15:10:32 +0000 X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFprCKsWRWlGSWpSXmKPExsWyU9JRQjf98cw Ig7uthhZLPi5mcWD0OLr7N1MAYxRrZl5SfkUCa8blQ7uYCzbrV/zZ/ImtgfGJXhcjJ4eEgJ/E m71LWUBsNgE9iY2zXzGB2CIC2hIb968Esrk4mAWeM0qc2fORESQhLGArceloNyNEkZ3Eg6tX2 bsYOYBsK4melbYgYRYBFYkV8z6BlfAKGEjcufCfHWSOkMAyRolde+8zgyQ4BewlFjZ/YwOxGQ VkJb40rgaLMwuIS9x6Mp8J4jgBiSV7zjND2KISLx//Y4Ww1SQO9r6DqtGROHv9CSOEbSCxdek +FpB7mAU0Jdbv0ocYaSmxp/M0I4StKDGl+yE7xG2CEidnPmGZwCg2C8nmWQjds5B0z0LSPQtJ 9wJG1lWMGsWpRWWpRbqGpnpJRZnpGSW5iZk5uoYGpnq5qcXFiempOYlJxXrJ+bmbGIGxxQAEO xgbtnseYpTkYFIS5T29e2aEEF9SfkplRmJxRnxRaU5q8SFGGQ4OJQne7Q+BcoJFqempFWmZOc Aoh0lLcPAoifAaPgJK8xYXJOYWZ6ZDpE4xKkqJ814A6RMASWSU5sG1wRLLJUZZKWFeRqBDhHg KUotyM0tQ5V8xinMwKgnz8oGM58nMK4Gb/gpoMRPQ4m1XpoEsLklESEk1MGZtcErdqBvXMXHu RZaZ0qkrc0M5p0/atro5JWN/manmUrHryjKvLzTttH45tYFhTat4T8b9CRd+HedIcVSOrFHZN 9nQokuq0pjR97RkXbfuCbknJfZbNq0U/DDPPGGu4rKDRtcdf3vkpu3fEn3lXdxb64/dC2OSOG cXrFz7KKSufsOc6knLhZVYijMSDbWYi4oTAfCPEHMnAwAA X-Env-Sender: prvs=2044c9f81=sergey.dyasli@citrix.com X-Msg-Ref: server-13.tower-206.messagelabs.com!1486480231!68181843!1 X-Originating-IP: [185.25.65.24] X-SpamReason: No, hits=0.0 required=7.0 tests=received_headers: No Received headers X-StarScan-Received: X-StarScan-Version: 9.1.1; banners=-,-,- X-VirusChecked: Checked Received: (qmail 53458 invoked from network); 7 Feb 2017 15:10:31 -0000 Received: from smtp.eu.citrix.com (HELO SMTP.EU.CITRIX.COM) (185.25.65.24) by server-13.tower-206.messagelabs.com with RC4-SHA encrypted SMTP; 7 Feb 2017 15:10:31 -0000 X-IronPort-AV: E=Sophos;i="5.33,346,1477958400"; d="scan'208";a="40311719" From: Sergey Dyasli To: "JBeulich@suse.com" Thread-Topic: [PATCH v2 1/4] x86/vmx: introduce vmwrite_safe() Thread-Index: AQHSgIlphF8Lh5RIGEGECh6hiFhPDaFdU4YAgABCPAA= Date: Tue, 7 Feb 2017 15:06:46 +0000 Message-ID: <1486480006.3301.3.camel@citrix.com> References: <20170206145747.13885-1-sergey.dyasli@citrix.com> <20170206145747.13885-2-sergey.dyasli@citrix.com> <5899B9060200007800137312@prv-mh.provo.novell.com> In-Reply-To: <5899B9060200007800137312@prv-mh.provo.novell.com> Accept-Language: en-GB, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-mailer: Evolution 3.22.3-0ubuntu0.1 x-ms-exchange-messagesentrepresentingtype: 1 x-ms-exchange-transport-fromentityheader: Hosted Content-ID: MIME-Version: 1.0 Cc: Sergey Dyasli , Kevin Tian , Andrew Cooper , "jun.nakajima@intel.com" , "xen-devel@lists.xen.org" Subject: Re: [Xen-devel] [PATCH v2 1/4] x86/vmx: introduce vmwrite_safe() X-BeenThere: xen-devel@lists.xen.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" X-Virus-Scanned: ClamAV using ClamSMTP On Tue, 2017-02-07 at 04:09 -0700, Jan Beulich wrote: > > > > On 06.02.17 at 15:57, wrote: > > > > Any fail during the original __vmwrite() leads to BUG() which can be > > easily exploited from a guest in the nested vmx mode. > > > > The new function returns error code depending on the outcome: > > > > VMsucceed: 0 > > VMfailValid: VM Instruction Error Number > > VMfailInvalid: a new VMX_INSN_FAIL_INVALID > > > > A new macro GAS_VMX_OP is introduced in order to improve the > > readability of asm. Existing ASM_FLAG_OUT macro is reused and copied > > into asm_defns.h > > > > Signed-off-by: Sergey Dyasli > > --- > > Please can you have the revision info for the individual patches > here. I know you've put it in the overview mail, but for reviewers > it's far more useful to (also) be here. > > > --- a/xen/include/asm-x86/hvm/vmx/vmcs.h > > +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h > > @@ -526,6 +526,7 @@ enum vmx_insn_errno > > VMX_INSN_VMPTRLD_INVALID_PHYADDR = 9, > > VMX_INSN_UNSUPPORTED_VMCS_COMPONENT = 12, > > VMX_INSN_VMXON_IN_VMX_ROOT = 15, > > + VMX_INSN_FAIL_INVALID = ~0, > > }; > > The main reason for me to ask for the type change here was to ... > > > @@ -423,6 +429,29 @@ static inline bool_t __vmread_safe(unsigned long field, unsigned long *value) > > return okay; > > } > > > > +static always_inline unsigned long vmwrite_safe(unsigned long field, > > + unsigned long value) > > +{ > > + unsigned long ret = 0; > > + bool fail_invalid, fail_valid; > > + > > + asm volatile ( GAS_VMX_OP("vmwrite %[value], %[field]\n\t", > > + VMWRITE_OPCODE MODRM_EAX_ECX) > > + ASM_FLAG_OUT(, "setc %[invalid]\n\t") > > + ASM_FLAG_OUT(, "setz %[valid]\n\t") > > + : ASM_FLAG_OUT("=@ccc", [invalid] "=rm") (fail_invalid), > > + ASM_FLAG_OUT("=@ccz", [valid] "=rm") (fail_valid) > > + : [field] GAS_VMX_OP("r", "a") (field), > > + [value] GAS_VMX_OP("rm", "c") (value)); > > + > > + if ( unlikely(fail_invalid) ) > > + ret = VMX_INSN_FAIL_INVALID; > > + else if ( unlikely(fail_valid) ) > > + __vmread(VM_INSTRUCTION_ERROR, &ret); > > + > > + return ret; > > +} > > ... allow the function to return enum vmx_insn_errno, and that > to not be a 64-bit quantity. As you're presumably aware, dealing > with 32-bit quantities is on the average slightly more efficient than > dealing with 64-bit ones. The code above should imo still BUG() if > the value read from VM_INSTRUCTION_ERROR doesn't fit in 32 > bits (as it's a 32-bit field only anyway). If I understood correctly, you are suggesting the following change: while vmread_safe() is plain "inline". I believe that plain inline is enough here, what do you think? --- Thanks, Sergey diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h b/xen/include/asm-x86/hvm/vmx/vmx.h index 24fbbd4..f9b3bf1 100644 --- a/xen/include/asm-x86/hvm/vmx/vmx.h +++ b/xen/include/asm-x86/hvm/vmx/vmx.h @@ -424,8 +424,8 @@ static inline unsigned long vmread_safe(unsigned long field,      return ret;  }   -static always_inline unsigned long vmwrite_safe(unsigned long field, -                                                unsigned long value) +static always_inline enum vmx_insn_errno vmwrite_safe(unsigned long field, +                                                      unsigned long value)  {      unsigned long ret = 0;      bool fail_invalid, fail_valid; @@ -440,11 +440,16 @@ static always_inline unsigned long vmwrite_safe(unsigned long field,                       [value] GAS_VMX_OP("rm", "c") (value));        if ( unlikely(fail_invalid) ) +    {          ret = VMX_INSN_FAIL_INVALID; +    }      else if ( unlikely(fail_valid) ) +    {          __vmread(VM_INSTRUCTION_ERROR, &ret); +        BUG_ON(ret >= ~0U); +    }   -    return ret; +    return (enum vmx_insn_errno) ret;  } And I have noticed one inconsistency: vmwrite_safe() is "always_inline"