[PATCH] x86/vmx: fix vmx_is_singlestep_supported return value

[PATCH] x86/vmx: fix vmx_is_singlestep_supported return value

[Tamas K Lengyel]

The function supposed to return a boolean but instead it returned
the value 0x8000000 which is the Intel internal flag for MTF. This has
caused various checks using this function to falsely report no MTF
capability.

Signed-off-by: Tamas K Lengyel <tlengyel@novetta.com>
---
 xen/arch/x86/hvm/vmx/vmx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 999defe..35bcd79 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1768,7 +1768,7 @@ static void vmx_enable_msr_exit_interception(struct domain *d)
 
 static bool_t vmx_is_singlestep_supported(void)
 {
-    return cpu_has_monitor_trap_flag;
+    return cpu_has_monitor_trap_flag ? 1 : 0;
 }
 
 static void vmx_vcpu_update_eptp(struct vcpu *v)
-- 
2.1.4

Re: [PATCH] x86/vmx: fix vmx_is_singlestep_supported return value

[Andrew Cooper]

On 24/08/2015 20:55, Tamas K Lengyel wrote:
> The function supposed to return a boolean but instead it returned
> the value 0x8000000 which is the Intel internal flag for MTF. This has
> caused various checks using this function to falsely report no MTF
> capability.

Ouch.  Given than bool_t is current signed char, that won't be of much use.

>
> Signed-off-by: Tamas K Lengyel <tlengyel@novetta.com>
> ---
>  xen/arch/x86/hvm/vmx/vmx.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 999defe..35bcd79 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -1768,7 +1768,7 @@ static void vmx_enable_msr_exit_interception(struct domain *d)
>  
>  static bool_t vmx_is_singlestep_supported(void)
>  {
> -    return cpu_has_monitor_trap_flag;
> +    return cpu_has_monitor_trap_flag ? 1 : 0;

Prevailing style would tend towards !!cpu_has_monitor_trap_flag

Either way, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

>  }
>  
>  static void vmx_vcpu_update_eptp(struct vcpu *v)

Re: [PATCH] x86/vmx: fix vmx_is_singlestep_supported return value

[Tamas K Lengyel]

>> @@ -1768,7 +1768,7 @@ static void vmx_enable_msr_exit_interception(struct domain *d)
>>
>>  static bool_t vmx_is_singlestep_supported(void)
>>  {
>> -    return cpu_has_monitor_trap_flag;
>> +    return cpu_has_monitor_trap_flag ? 1 : 0;
>
> Prevailing style would tend towards !!cpu_has_monitor_trap_flag

Yeap, you are right. If the maintainers prefer I can resend with that style.

>
> Either way, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks!
Tamas

Re: [PATCH] x86/vmx: fix vmx_is_singlestep_supported return value

[Jan Beulich]

>>> Tamas K Lengyel <tamas.lengyel@zentific.com> 08/25/15 1:51 AM >>>
>>> @@ -1768,7 +1768,7 @@ static void vmx_enable_msr_exit_interception(struct domain *d)
>>>
>>>  static bool_t vmx_is_singlestep_supported(void)
>>>  {
>>> -    return cpu_has_monitor_trap_flag;
>>> +    return cpu_has_monitor_trap_flag ? 1 : 0;
>>
>> Prevailing style would tend towards !!cpu_has_monitor_trap_flag
>
>Yeap, you are right. If the maintainers prefer I can resend with that style.

This could easily be adjusted upon commit. What I'm wondering is whether this
is the right place to fix it: Wouldn't it be better for the cpu_has_* macros
themselves to be adjusted so other (future) users won't fall into the same trap
(vmx_virtual_intr_delivery_enabled() is a good second example bogusly using
int as its return type, and once adjusted to bool_t it would break)?

Jan

Re: [PATCH] x86/vmx: fix vmx_is_singlestep_supported return value

[Wei Liu]

On Tue, Aug 25, 2015 at 01:58:42AM -0600, Jan Beulich wrote:
[...]
> (vmx_virtual_intr_delivery_enabled() is a good second example bogusly using
> int as its return type, and once adjusted to bool_t it would break)?
> 

VMX maintainers, I think Jan is waiting for reply from you for that
particular question. We need reply / confirmation from you to get a fix
for this issue.

Wei.

> Jan

Re: [PATCH] x86/vmx: fix vmx_is_singlestep_supported return value

[Tian, Kevin]

> From: Jan Beulich [mailto:jbeulich@suse.com]
> Sent: Tuesday, August 25, 2015 3:59 PM
> 
> >>> Tamas K Lengyel <tamas.lengyel@zentific.com> 08/25/15 1:51 AM >>>
> >>> @@ -1768,7 +1768,7 @@ static void vmx_enable_msr_exit_interception(struct
> domain *d)
> >>>
> >>>  static bool_t vmx_is_singlestep_supported(void)
> >>>  {
> >>> -    return cpu_has_monitor_trap_flag;
> >>> +    return cpu_has_monitor_trap_flag ? 1 : 0;
> >>
> >> Prevailing style would tend towards !!cpu_has_monitor_trap_flag
> >
> >Yeap, you are right. If the maintainers prefer I can resend with that style.
> 
> This could easily be adjusted upon commit. What I'm wondering is whether this
> is the right place to fix it: Wouldn't it be better for the cpu_has_* macros
> themselves to be adjusted so other (future) users won't fall into the same trap
> (vmx_virtual_intr_delivery_enabled() is a good second example bogusly using
> int as its return type, and once adjusted to bool_t it would break)?
> 

I'm OK with original patch. Above example can be taken care when changing
return type.

Thanks
Kevin

Re: [PATCH] x86/vmx: fix vmx_is_singlestep_supported return value

[Jan Beulich]

>>> On 28.08.15 at 04:01, <kevin.tian@intel.com> wrote:
>>  From: Jan Beulich [mailto:jbeulich@suse.com]
>> Sent: Tuesday, August 25, 2015 3:59 PM
>> 
>> >>> Tamas K Lengyel <tamas.lengyel@zentific.com> 08/25/15 1:51 AM >>>
>> >>> @@ -1768,7 +1768,7 @@ static void vmx_enable_msr_exit_interception(struct
>> domain *d)
>> >>>
>> >>>  static bool_t vmx_is_singlestep_supported(void)
>> >>>  {
>> >>> -    return cpu_has_monitor_trap_flag;
>> >>> +    return cpu_has_monitor_trap_flag ? 1 : 0;
>> >>
>> >> Prevailing style would tend towards !!cpu_has_monitor_trap_flag
>> >
>> >Yeap, you are right. If the maintainers prefer I can resend with that style.
>> 
>> This could easily be adjusted upon commit. What I'm wondering is whether this
>> is the right place to fix it: Wouldn't it be better for the cpu_has_* macros
>> themselves to be adjusted so other (future) users won't fall into the same trap
>> (vmx_virtual_intr_delivery_enabled() is a good second example bogusly using
>> int as its return type, and once adjusted to bool_t it would break)?
> 
> I'm OK with original patch. Above example can be taken care when changing
> return type.

Okay, I'll commit it the way it is, despite not being convinced of the
approach of waiting for the next one to fall into the same trap.

Jan

Re: [PATCH] x86/vmx: fix vmx_is_singlestep_supported return value

[Wei Liu]

On Mon, Aug 24, 2015 at 03:55:33PM -0400, Tamas K Lengyel wrote:
> The function supposed to return a boolean but instead it returned
> the value 0x8000000 which is the Intel internal flag for MTF. This has
> caused various checks using this function to falsely report no MTF
> capability.
> 
> Signed-off-by: Tamas K Lengyel <tlengyel@novetta.com>

Release-acked-by: Wei Liu <wei.liu2@citrix.com>

> ---
>  xen/arch/x86/hvm/vmx/vmx.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 999defe..35bcd79 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -1768,7 +1768,7 @@ static void vmx_enable_msr_exit_interception(struct domain *d)
>  
>  static bool_t vmx_is_singlestep_supported(void)
>  {
> -    return cpu_has_monitor_trap_flag;
> +    return cpu_has_monitor_trap_flag ? 1 : 0;
>  }
>  
>  static void vmx_vcpu_update_eptp(struct vcpu *v)
> -- 
> 2.1.4

Re: [PATCH] x86/vmx: fix vmx_is_singlestep_supported return value

[Tian, Kevin]

> From: Tamas K Lengyel [mailto:tamas@tklengyel.com]
> Sent: Tuesday, August 25, 2015 3:56 AM
> 
> The function supposed to return a boolean but instead it returned
> the value 0x8000000 which is the Intel internal flag for MTF. This has
> caused various checks using this function to falsely report no MTF
> capability.
> 
> Signed-off-by: Tamas K Lengyel <tlengyel@novetta.com>

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

Thanks,
Kevin