Re: [Xen-devel] [PATCH v1 2/6] x86/vvmx: correct vmfail() usage for vmptrld and vmclear

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Fri, Oct 12, 2018 at 04:27:55PM +0100, Sergey Dyasli wrote:
> Calling vmfail_valid() is correct only if vvmcx is valid. Modify
> functions to use vmfail() instead which performs the necessary check.
> 
> Signed-off-by: Sergey Dyasli <sergey.dyasli@xxxxxxxxxx>

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

I think the code is a bit convoluted because what fields get access is
hidden behind layers of macros and functions. In order to catch future
issues, may I suggest you add some assertions to vmfail_{in,}valid?

Like

   ASSERT(!cpu_has_vmx_vmcs_shadowing && vvmcx_invalid(v));

in vmfail_invalid. Something similar can be added in vmfail_valid as
well.

> ---
>  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 8eee6d0ea8..26b7d72660 100644
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -1744,7 +1744,7 @@ int nvmx_handle_vmptrld(struct cpu_user_regs *regs)
>               !map_io_bitmap_all(v) ||
>               !_map_msr_bitmap(v) )
>          {
> -            vmfail_valid(regs, VMX_INSN_VMPTRLD_INVALID_PHYADDR);
> +            vmfail(regs, VMX_INSN_VMPTRLD_INVALID_PHYADDR);
>              goto out;
>          }
>      }
> @@ -1828,7 +1828,7 @@ int nvmx_handle_vmclear(struct cpu_user_regs *regs)
>      if ( rc == VMSUCCEED )
>          vmsucceed(regs);
>      else if ( rc == VMFAIL_VALID )
> -        vmfail_valid(regs, VMX_INSN_VMCLEAR_INVALID_PHYADDR);
> +        vmfail(regs, VMX_INSN_VMCLEAR_INVALID_PHYADDR);
>      else
>          vmfail_invalid(regs);
>  
> -- 
> 2.17.1
> 

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