Re: [Xen-devel] [PATCH 2/9] x86/vmx: Internal cleanup for MSR load/save infrastructure

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 23/05/18 17:28, Roger Pau Monné wrote:
> On Tue, May 22, 2018 at 12:20:39PM +0100, Andrew Cooper wrote:
>>  * Use an arch_vmx_struct local variable to reduce later code volume.
>>  * Use start/total instead of msr_area/msr_count.  This is in preparation for
>>    more finegrained handling with later changes.
>>  * Use ent/end pointers (again for preparation), and to make the 
>> vmx_add_msr()
>>    logic easier to follow.
>>  * Make the memory allocation block of vmx_add_msr() unlikely, and calculate
>>    virt_to_maddr() just once.
>>
>> No practical change to functionality.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> ---
>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>> CC: Jun Nakajima <jun.nakajima@xxxxxxxxx>
>> CC: Kevin Tian <kevin.tian@xxxxxxxxx>
>> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
>> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>> ---
>>  xen/arch/x86/hvm/vmx/vmcs.c | 74 
>> ++++++++++++++++++++++++---------------------
>>  1 file changed, 40 insertions(+), 34 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
>> index a5dcf5c..f557857 100644
>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
>> @@ -1292,48 +1292,50 @@ static int vmx_msr_entry_key_cmp(const void *key, 
>> const void *elt)
>>  struct vmx_msr_entry *vmx_find_msr(uint32_t msr, enum vmx_msr_list_type 
>> type)
>>  {
>>      struct vcpu *curr = current;
>> -    unsigned int msr_count;
>> -    struct vmx_msr_entry *msr_area = NULL;
>> +    struct arch_vmx_struct *arch_vmx = &curr->arch.hvm_vmx;
> curr is used here only, so you can use current and get rid of the curr
> local variable?

curr disappears in patch 4 "x86/vmx: Support remote access to the MSR
lists", when v starts getting passed in from the outside.

>
>> +    struct vmx_msr_entry *start = NULL;
>> +    unsigned int total;
>>  
>>      switch ( type )
>>      {
>>      case VMX_MSR_HOST:
>> -        msr_count = curr->arch.hvm_vmx.host_msr_count;
>> -        msr_area = curr->arch.hvm_vmx.host_msr_area;
>> +        start    = arch_vmx->host_msr_area;
>> +        total    = arch_vmx->host_msr_count;
>>          break;
>>  
>>      case VMX_MSR_GUEST:
>> -        msr_count = curr->arch.hvm_vmx.msr_count;
>> -        msr_area = curr->arch.hvm_vmx.msr_area;
>> +        start    = arch_vmx->msr_area;
>> +        total    = arch_vmx->msr_count;
> Not that I think is wrong, but why are you adding the extra spaces
> after the variable name? Those assignments will already be aligned
> because start and total names have the same length.

There are changes in later patches, for which this is the correct
indentation.

>
>>          break;
>>  
>>      default:
>>          ASSERT_UNREACHABLE();
>>      }
>>  
>> -    if ( msr_area == NULL )
>> +    if ( !start )
>>          return NULL;
>>  
>> -    return bsearch(&msr, msr_area, msr_count, sizeof(struct vmx_msr_entry),
>> +    return bsearch(&msr, start, total, sizeof(struct vmx_msr_entry),
>>                     vmx_msr_entry_key_cmp);
>>  }
>>  
>>  int vmx_add_msr(uint32_t msr, enum vmx_msr_list_type type)
>>  {
>>      struct vcpu *curr = current;
> curr seems to be used only once below in order to get hvm_vmx? In
> which case it could be removed.
>
>> -    unsigned int idx, *msr_count;
>> -    struct vmx_msr_entry **msr_area, *msr_area_elem;
>> +    struct arch_vmx_struct *arch_vmx = &curr->arch.hvm_vmx;
>> +    struct vmx_msr_entry **ptr, *start = NULL, *ent, *end;
> Do you really need to initialize start here? It seems like it's
> unconditionally set to *ptr before any usage.

Yes, for safety through the default case in release builds.

~Andrew

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