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.coop...@citrix.com>
>> ---
>> CC: Jan Beulich <jbeul...@suse.com>
>> CC: Jun Nakajima <jun.nakaj...@intel.com>
>> CC: Kevin Tian <kevin.t...@intel.com>
>> CC: Wei Liu <wei.l...@citrix.com>
>> CC: Roger Pau Monné <roger....@citrix.com>
>> ---
>> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
