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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.03.18 at 13:07, <andrew.cooper3@xxxxxxxxxx> 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, &exit_qualification);
> -        cr = VMX_CONTROL_REG_ACCESS_NUM(exit_qualification);
> -        write = VMX_CONTROL_REG_ACCESS_TYPE(exit_qualification);
> +        __vmread(EXIT_QUALIFICATION, &qual.raw);
>          /* 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_CLTS        2
> -# define VMX_CONTROL_REG_ACCESS_TYPE_LMSW        3
> - /* 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel