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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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?

> +    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.

>          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.

Thanks, Roger.

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