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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 20/03/18 10:51, Jan Beulich wrote:
>>>> 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 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel