Re: [Xen-devel] [PATCH] x86/vtx: Introduce a typed union for CR access exit information

[Jan Beulich]

>>> On 20.03.18 at 15:28,  wrote:
> On 20/03/18 10:51, Jan Beulich wrote:
> On 15.03.18 at 13:07,  wrote:
>>> +typedef union cr_access_qual {
>>> +unsigned long raw;
>>> +struct {
>>> +uint16_t cr:4,
>>> + access_type:2,  /* VMX_CR_ACCESS_TYPE_* */
>>> + lmsw_op_type:1, /* 0 => reg, 1 => mem   */
>>> + :1,
>>> + gpr:4,
>>> + :4;
>>> +uint16_t lmsw_data;
>>> +uint32_t :32;
>> Strictly speaking this doesn't belong here, as it doesn't exist for
>> 32-bit VMX implementations.
> 
> It is only here to keep clang happy.  See c/s ac6e7fd7a482

Oh, I didn't recall that.

> As an alternative, we could see about not using transparent unions, and
> explicitly casting in the function calls.

Let's rather not - transparent unions are quite nice an aid to avoid
casts.

Jan


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

Re: [Xen-devel] [PATCH] x86/vtx: Introduce a typed union for CR access exit information

[Andrew Cooper]

On 20/03/18 10:51, Jan Beulich wrote:
 On 15.03.18 at 13:07,  wrote:
>> --- a/xen/arch/x86/hvm/vmx/vvmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
>> @@ -2448,27 +2448,24 @@ int nvmx_n2_vmexit_handler(struct cpu_user_regs 
>> *regs,
>>  break;
>>  case EXIT_REASON_CR_ACCESS:
>>  {
>> -unsigned long exit_qualification;
>> -int cr, write;
>> +cr_access_qual_t qual;
>>  u32 mask = 0;
>>  
>> -__vmread(EXIT_QUALIFICATION, _qualification);
>> -cr = VMX_CONTROL_REG_ACCESS_NUM(exit_qualification);
>> -write = VMX_CONTROL_REG_ACCESS_TYPE(exit_qualification);
>> +__vmread(EXIT_QUALIFICATION, );
>>  /* also according to guest exec_control */
>>  ctrl = __n2_exec_control(v);
>>  
>> -if ( cr == 3 )
>> +if ( qual.cr == 3 )
>>  {
>> -mask = write? CPU_BASED_CR3_STORE_EXITING:
>> -  CPU_BASED_CR3_LOAD_EXITING;
>> +mask = qual.access_type ? CPU_BASED_CR3_STORE_EXITING
>> +: CPU_BASED_CR3_LOAD_EXITING;
> I realize the old code has the same problem, but is this correct?
> Only type 1 is a read from the CR, types 0, 2, and 3 are writes.
> At least have an assertion here that types 2 and 3 can't occur?

If we trust hardware not to give us junk here, then types 2 and 3 can't
occur.  I'll add an assert.

>
>>  if ( ctrl & mask )
>>  nvcpu->nv_vmexit_pending = 1;
>>  }
>> -else if ( cr == 8 )
>> +else if ( qual.cr == 8 )
>>  {
>> -mask = write? CPU_BASED_CR8_STORE_EXITING:
>> -  CPU_BASED_CR8_LOAD_EXITING;
>> +mask = qual.access_type ? CPU_BASED_CR3_STORE_EXITING
>> +: CPU_BASED_CR3_LOAD_EXITING;
> Copy-and-paste mistake (ought to be CR8 here).

Oops.

>
>> +};
>> +typedef union cr_access_qual {
>> +unsigned long raw;
>> +struct {
>> +uint16_t cr:4,
>> + access_type:2,  /* VMX_CR_ACCESS_TYPE_* */
>> + lmsw_op_type:1, /* 0 => reg, 1 => mem   */
>> + :1,
>> + gpr:4,
>> + :4;
>> +uint16_t lmsw_data;
>> +uint32_t :32;
> Strictly speaking this doesn't belong here, as it doesn't exist for
> 32-bit VMX implementations.

It is only here to keep clang happy.  See c/s ac6e7fd7a482

As an alternative, we could see about not using transparent unions, and
explicitly casting in the function calls.

~Andrew

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

Re: [Xen-devel] [PATCH] x86/vtx: Introduce a typed union for CR access exit information

[Jan Beulich]

>>> On 15.03.18 at 13:07,  wrote:
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -2448,27 +2448,24 @@ int nvmx_n2_vmexit_handler(struct cpu_user_regs *regs,
>  break;
>  case EXIT_REASON_CR_ACCESS:
>  {
> -unsigned long exit_qualification;
> -int cr, write;
> +cr_access_qual_t qual;
>  u32 mask = 0;
>  
> -__vmread(EXIT_QUALIFICATION, _qualification);
> -cr = VMX_CONTROL_REG_ACCESS_NUM(exit_qualification);
> -write = VMX_CONTROL_REG_ACCESS_TYPE(exit_qualification);
> +__vmread(EXIT_QUALIFICATION, );
>  /* also according to guest exec_control */
>  ctrl = __n2_exec_control(v);
>  
> -if ( cr == 3 )
> +if ( qual.cr == 3 )
>  {
> -mask = write? CPU_BASED_CR3_STORE_EXITING:
> -  CPU_BASED_CR3_LOAD_EXITING;
> +mask = qual.access_type ? CPU_BASED_CR3_STORE_EXITING
> +: CPU_BASED_CR3_LOAD_EXITING;

I realize the old code has the same problem, but is this correct?
Only type 1 is a read from the CR, types 0, 2, and 3 are writes.
At least have an assertion here that types 2 and 3 can't occur?

>  if ( ctrl & mask )
>  nvcpu->nv_vmexit_pending = 1;
>  }
> -else if ( cr == 8 )
> +else if ( qual.cr == 8 )
>  {
> -mask = write? CPU_BASED_CR8_STORE_EXITING:
> -  CPU_BASED_CR8_LOAD_EXITING;
> +mask = qual.access_type ? CPU_BASED_CR3_STORE_EXITING
> +: CPU_BASED_CR3_LOAD_EXITING;

Copy-and-paste mistake (ought to be CR8 here).

> --- a/xen/include/asm-x86/hvm/vmx/vmx.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmx.h
> @@ -232,18 +232,25 @@ static inline void pi_clear_sn(struct pi_desc *pi_desc)
>  /*
>   * Exit Qualifications for MOV for Control Register Access
>   */
> - /* 3:0 - control register number (CRn) */
> -#define VMX_CONTROL_REG_ACCESS_NUM(eq)  ((eq) & 0xf)
> - /* 5:4 - access type (CR write, CR read, CLTS, LMSW) */
> -#define VMX_CONTROL_REG_ACCESS_TYPE(eq) (((eq) >> 4) & 0x3)
> -# define VMX_CONTROL_REG_ACCESS_TYPE_MOV_TO_CR   0
> -# define VMX_CONTROL_REG_ACCESS_TYPE_MOV_FROM_CR 1
> -# define VMX_CONTROL_REG_ACCESS_TYPE_CLTS2
> -# define VMX_CONTROL_REG_ACCESS_TYPE_LMSW3
> - /* 11:8 - general purpose register operand */
> -#define VMX_CONTROL_REG_ACCESS_GPR(eq)  (((eq) >> 8) & 0xf)
> - /* 31:16 - LMSW source data */
> -#define VMX_CONTROL_REG_ACCESS_DATA(eq)  ((uint32_t)(eq) >> 16)
> +enum {
> +VMX_CR_ACCESS_TYPE_MOV_TO_CR,
> +VMX_CR_ACCESS_TYPE_MOV_FROM_CR,
> +VMX_CR_ACCESS_TYPE_CLTS,
> +VMX_CR_ACCESS_TYPE_LMSW,

The trailing comma is sort of pointless here with the field being
just 2 bits wide.

> +};
> +typedef union cr_access_qual {
> +unsigned long raw;
> +struct {
> +uint16_t cr:4,
> + access_type:2,  /* VMX_CR_ACCESS_TYPE_* */
> + lmsw_op_type:1, /* 0 => reg, 1 => mem   */
> + :1,
> + gpr:4,
> + :4;
> +uint16_t lmsw_data;
> +uint32_t :32;

Strictly speaking this doesn't belong here, as it doesn't exist for
32-bit VMX implementations.

Jan


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

Re: [Xen-devel] [PATCH] x86/vtx: Introduce a typed union for CR access exit information

[Tian, Kevin]

> From: Andrew Cooper [mailto:andrew.coop...@citrix.com]
> Sent: Thursday, March 15, 2018 8:08 PM
> 
> This reduces code volume, and has a minor improvement on compiled size,
> probably due to the removal of several temporary variables.
> 
>   add/remove: 0/0 grow/shrink: 0/2 up/down: 0/-50 (-50)
>   function old new   delta
>   vmx_vmexit_handler  68816878  -3
>   nvmx_n2_vmexit_handler  34733426 -47
> 
> Take the opportunity to make some style corrections, and add some
> ASSERT_UNREACHABLE()s in appropriate places.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper 

Acked-by: Kevin Tian 

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