[PATCH 0/4] x86/vvmx: Misc fixes

[PATCH 0/4] x86/vvmx: Misc fixes

[Andrew Cooper]

All from code inspection

Andrew Cooper (4):
  x86/vvmx: Drop unused CASE_{GET,SET}_REG() macros
  x86/vvmx: Correct the INVALID_PADDR checks for VMPTRLD/VMCLEAR
  x86/vvmx: Fixes to VMWRITE emulation
  x86/vvmx: Don't call vmsucceed() at the end of virtual_vmexit()

 xen/arch/x86/hvm/vmx/vvmx.c | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

-- 
2.1.4


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

[PATCH 1/4] x86/vvmx: Drop unused CASE_{GET, SET}_REG() macros

[Andrew Cooper]

These have been obsolete since c/s 053ae230 "x86/vvmx: Remove enum
vmx_regs_enc".

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Sergey Dyasli <sergey.dyasli@citrix.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
---
 xen/arch/x86/hvm/vmx/vvmx.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 88021af..c296660 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -207,11 +207,6 @@ struct vmx_inst_decoded {
     unsigned int reg2;
 };
 
-#define CASE_SET_REG(REG, reg)      \
-    case VMX_REG_ ## REG: regs->reg = value; break
-#define CASE_GET_REG(REG, reg)      \
-    case VMX_REG_ ## REG: value = regs->reg; break
-
 static int vvmcs_offset(u32 width, u32 type, u32 index)
 {
     int offset;
-- 
2.1.4


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

[PATCH 2/4] x86/vvmx: Correct the INVALID_PADDR checks for VMPTRLD/VMCLEAR

[Andrew Cooper]

The referenced addresses also need checking against MAXPHYSADDR.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Sergey Dyasli <sergey.dyasli@citrix.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
---
 xen/arch/x86/hvm/vmx/vvmx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index c296660..5daab82 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1672,7 +1672,7 @@ static int nvmx_handle_vmptrld(struct cpu_user_regs *regs)
     if ( rc != X86EMUL_OKAY )
         return rc;
 
-    if ( gpa & 0xfff )
+    if ( (gpa & ~PAGE_MASK) || !gfn_valid(v->domain, gaddr_to_gfn(gpa)) )
     {
         vmfail(regs, VMX_INSN_VMPTRLD_INVALID_PHYADDR);
         goto out;
@@ -1780,7 +1780,7 @@ static int nvmx_handle_vmclear(struct cpu_user_regs *regs)
         goto out;
     }
 
-    if ( gpa & 0xfff )
+    if ( (gpa & ~PAGE_MASK) || !gfn_valid(v->domain, gaddr_to_gfn(gpa)) )
     {
         vmfail(regs, VMX_INSN_VMCLEAR_INVALID_PHYADDR);
         goto out;
-- 
2.1.4


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

[PATCH 3/4] x86/vvmx: Fixes to VMWRITE emulation

[Andrew Cooper]

 * Don't assume that decode_vmx_inst() always returns X86EMUL_EXCEPTION.
 * The okay boolean is never written, making the else case dead.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Sergey Dyasli <sergey.dyasli@citrix.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
---
 xen/arch/x86/hvm/vmx/vvmx.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 5daab82..41c4e2f 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1872,11 +1872,12 @@ static int nvmx_handle_vmwrite(struct cpu_user_regs *regs)
     struct vmx_inst_decoded decode;
     unsigned long operand; 
     u64 vmcs_encoding;
-    bool_t okay = 1;
     enum vmx_insn_errno err;
+    int rc;
 
-    if ( decode_vmx_inst(regs, &decode, &operand) != X86EMUL_OKAY )
-        return X86EMUL_EXCEPTION;
+    rc = decode_vmx_inst(regs, &decode, &operand);
+    if ( rc != X86EMUL_OKAY )
+        return rc;
 
     if ( !vvmcx_valid(v) )
     {
@@ -1905,10 +1906,7 @@ static int nvmx_handle_vmwrite(struct cpu_user_regs *regs)
         break;
     }
 
-    if ( okay )
-        vmsucceed(regs);
-    else
-        vmfail_valid(regs, VMX_INSN_UNSUPPORTED_VMCS_COMPONENT);
+    vmsucceed(regs);
 
     return X86EMUL_OKAY;
 }
-- 
2.1.4


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

[PATCH 4/4] x86/vvmx: Don't call vmsucceed() at the end of virtual_vmexit()

[Andrew Cooper]

This ends up corrupting L1's view of RFLAGS by setting ZF.  The correct value
is established earlier in the function.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Sergey Dyasli <sergey.dyasli@citrix.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
---
 xen/arch/x86/hvm/vmx/vvmx.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 41c4e2f..a72b519 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1371,7 +1371,6 @@ static void virtual_vmexit(struct cpu_user_regs *regs)
         nvmx_update_apicv(v);
 
     nvcpu->nv_vmswitch_in_progress = 0;
-    vmsucceed(regs);
 }
 
 static void nvmx_eptp_update(void)
-- 
2.1.4


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

Re: [PATCH 1/4] x86/vvmx: Drop unused CASE_{GET, SET}_REG() macros

[Roger Pau Monné]

On Thu, Nov 15, 2018 at 01:52:47PM +0000, Andrew Cooper wrote:
> These have been obsolete since c/s 053ae230 "x86/vvmx: Remove enum
> vmx_regs_enc".
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

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

Re: [PATCH 2/4] x86/vvmx: Correct the INVALID_PADDR checks for VMPTRLD/VMCLEAR

[Roger Pau Monné]

On Thu, Nov 15, 2018 at 01:52:48PM +0000, Andrew Cooper wrote:
> The referenced addresses also need checking against MAXPHYSADDR.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Sergey Dyasli <sergey.dyasli@citrix.com>
> CC: Jun Nakajima <jun.nakajima@intel.com>
> CC: Kevin Tian <kevin.tian@intel.com>
> ---
>  xen/arch/x86/hvm/vmx/vvmx.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
> index c296660..5daab82 100644
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -1672,7 +1672,7 @@ static int nvmx_handle_vmptrld(struct cpu_user_regs *regs)
>      if ( rc != X86EMUL_OKAY )
>          return rc;
>  
> -    if ( gpa & 0xfff )
> +    if ( (gpa & ~PAGE_MASK) || !gfn_valid(v->domain, gaddr_to_gfn(gpa)) )

Seeing this is repeated in several places, it might be helpful to
introduce a helper?

Thanks, Roger.

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

Re: [PATCH 3/4] x86/vvmx: Fixes to VMWRITE emulation

[Roger Pau Monné]

On Thu, Nov 15, 2018 at 01:52:49PM +0000, Andrew Cooper wrote:
>  * Don't assume that decode_vmx_inst() always returns X86EMUL_EXCEPTION.
>  * The okay boolean is never written, making the else case dead.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

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

Re: [PATCH 4/4] x86/vvmx: Don't call vmsucceed() at the end of virtual_vmexit()

[Roger Pau Monné]

On Thu, Nov 15, 2018 at 01:52:50PM +0000, Andrew Cooper wrote:
> This ends up corrupting L1's view of RFLAGS by setting ZF.  The correct value
> is established earlier in the function.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

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

Re: [PATCH 4/4] x86/vvmx: Don't call vmsucceed() at the end of virtual_vmexit()

[Sergey Dyasli]

On 15/11/2018 13:52, Andrew Cooper wrote:
> This ends up corrupting L1's view of RFLAGS by setting ZF.  The correct value
> is established earlier in the function.

vmsucceed() doesn't set any flags, only clears some. And in this function it's
just redundant. ZF is set by VMfailValid. So I think the description must be
changed.

--
Sergey

> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Sergey Dyasli <sergey.dyasli@citrix.com>
> CC: Jun Nakajima <jun.nakajima@intel.com>
> CC: Kevin Tian <kevin.tian@intel.com>
> ---
>  xen/arch/x86/hvm/vmx/vvmx.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
> index 41c4e2f..a72b519 100644
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -1371,7 +1371,6 @@ static void virtual_vmexit(struct cpu_user_regs *regs)
>          nvmx_update_apicv(v);
>  
>      nvcpu->nv_vmswitch_in_progress = 0;
> -    vmsucceed(regs);
>  }
>  
>  static void nvmx_eptp_update(void)
> 

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

Re: [PATCH 4/4] x86/vvmx: Don't call vmsucceed() at the end of virtual_vmexit()

[Andrew Cooper]

On 15/11/2018 15:28, Sergey Dyasli wrote:
> On 15/11/2018 13:52, Andrew Cooper wrote:
>> This ends up corrupting L1's view of RFLAGS by setting ZF.  The correct value
>> is established earlier in the function.
> vmsucceed() doesn't set any flags, only clears some. And in this function it's
> just redundant. ZF is set by VMfailValid. So I think the description must be
> changed.

Oh - so it does.  Yes - I was confusing it with the failvalid case. 
I'll fix the wording to "The correct RFLAGS value is established earlier
in the function, and this call has no net effect."

~Andrew

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

Re: [PATCH 0/4] x86/vvmx: Misc fixes

[Sergey Dyasli]

On 15/11/2018 13:52, Andrew Cooper wrote:
> All from code inspection
> 
> Andrew Cooper (4):
>   x86/vvmx: Drop unused CASE_{GET,SET}_REG() macros
>   x86/vvmx: Correct the INVALID_PADDR checks for VMPTRLD/VMCLEAR
>   x86/vvmx: Fixes to VMWRITE emulation
>   x86/vvmx: Don't call vmsucceed() at the end of virtual_vmexit()

Reviewed-by: Sergey Dyasli <sergey.dyasli@citrix.com>

--
Thanks,
Sergey

>  xen/arch/x86/hvm/vmx/vvmx.c | 22 +++++++---------------
>  1 file changed, 7 insertions(+), 15 deletions(-)
> 

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

Re: [PATCH 0/4] x86/vvmx: Misc fixes

[Andrew Cooper]

Ping VT-x.

On 15/11/2018 13:52, Andrew Cooper wrote:
> All from code inspection
>
> Andrew Cooper (4):
>   x86/vvmx: Drop unused CASE_{GET,SET}_REG() macros
>   x86/vvmx: Correct the INVALID_PADDR checks for VMPTRLD/VMCLEAR
>   x86/vvmx: Fixes to VMWRITE emulation
>   x86/vvmx: Don't call vmsucceed() at the end of virtual_vmexit()
>
>  xen/arch/x86/hvm/vmx/vvmx.c | 22 +++++++---------------
>  1 file changed, 7 insertions(+), 15 deletions(-)
>


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

Re: [PATCH 1/4] x86/vvmx: Drop unused CASE_{GET, SET}_REG() macros

[Tian, Kevin]

> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Thursday, November 15, 2018 9:53 PM
> 
> These have been obsolete since c/s 053ae230 "x86/vvmx: Remove enum
> vmx_regs_enc".
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/4] x86/vvmx: Correct the INVALID_PADDR checks for VMPTRLD/VMCLEAR

[Tian, Kevin]

> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Thursday, November 15, 2018 9:53 PM
> 
> The referenced addresses also need checking against MAXPHYSADDR.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/4] x86/vvmx: Fixes to VMWRITE emulation

[Tian, Kevin]

> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Thursday, November 15, 2018 9:53 PM
> 
>  * Don't assume that decode_vmx_inst() always returns
> X86EMUL_EXCEPTION.
>  * The okay boolean is never written, making the else case dead.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Kevin Tian <kevin.tian@intel.com>

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

Re: [PATCH 4/4] x86/vvmx: Don't call vmsucceed() at the end of virtual_vmexit()

[Tian, Kevin]

> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Thursday, November 15, 2018 11:31 PM
> 
> On 15/11/2018 15:28, Sergey Dyasli wrote:
> > On 15/11/2018 13:52, Andrew Cooper wrote:
> >> This ends up corrupting L1's view of RFLAGS by setting ZF.  The correct
> value
> >> is established earlier in the function.
> > vmsucceed() doesn't set any flags, only clears some. And in this function
> it's
> > just redundant. ZF is set by VMfailValid. So I think the description must be
> > changed.
> 
> Oh - so it does.  Yes - I was confusing it with the failvalid case.
> I'll fix the wording to "The correct RFLAGS value is established earlier
> in the function, and this call has no net effect."
> 

with wording fixed, Acked-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel