Linux Virtualization - Sep 2017 - [PATCH v1 1/4] KVM/vmx: re-write the msr auto switch feature

On 25/09/2017 06:44, Wei Wang wrote:
>  
> +static void update_msr_autoload_count_max(void)
> +{
> +	u64 vmx_msr;
> +	int n;
> +
> +	/*
> +	 * According to the Intel SDM, if Bits 27:25 of MSR_IA32_VMX_MISC is
> +	 * n, then (n + 1) * 512 is the recommended max number of MSRs to be
> +	 * included in the VMExit and VMEntry MSR auto switch list.
> +	 */
> +	rdmsrl(MSR_IA32_VMX_MISC, vmx_msr);
> +	n = ((vmx_msr & 0xe000000) >> 25) + 1;
> +	msr_autoload_count_max = n * KVM_VMX_DEFAULT_MSR_AUTO_LOAD_COUNT;
> +}
> +

Any reasons to do this if it's unlikely that we'll ever update more than
512 MSRs?

Paolo

On 09/25/2017 07:54 PM, Paolo Bonzini wrote:
> On 25/09/2017 06:44, Wei Wang wrote:
>>   
>> +static void update_msr_autoload_count_max(void)
>> +{
>> +	u64 vmx_msr;
>> +	int n;
>> +
>> +	/*
>> +	 * According to the Intel SDM, if Bits 27:25 of MSR_IA32_VMX_MISC is
>> +	 * n, then (n + 1) * 512 is the recommended max number of MSRs to be
>> +	 * included in the VMExit and VMEntry MSR auto switch list.
>> +	 */
>> +	rdmsrl(MSR_IA32_VMX_MISC, vmx_msr);
>> +	n = ((vmx_msr & 0xe000000) >> 25) + 1;
>> +	msr_autoload_count_max = n * KVM_VMX_DEFAULT_MSR_AUTO_LOAD_COUNT;
>> +}
>> +
>
> Any reasons to do this if it's unlikely that we'll ever update more
than
> 512 MSRs?
>
> Paolo
It isn't a must to allocate memory for 512 MSRs, but I think it would be 
good to
allocate memory at least for 128 MSRs, because on skylake we have 32 
triplets
of MSRs (FROM/TO/INFO), which are 96 in total already.

The disadvantage is that developers would need to manually calculate and 
change
the number carefully when they need to add new MSRs for auto switching 
in the
future. So, if consuming a little bit more memory isn't a concern, I 
think we can
directly allocate memory based on what's recommended by the hardware.


Best,
Wei

On 25/09/2017 15:02, Wei Wang wrote:
> On 09/25/2017 07:54 PM, Paolo Bonzini wrote:
>> On 25/09/2017 06:44, Wei Wang wrote:
>>> ? +static void update_msr_autoload_count_max(void)
>>> +{
>>> +??? u64 vmx_msr;
>>> +??? int n;
>>> +
>>> +??? /*
>>> +???? * According to the Intel SDM, if Bits 27:25 of
>>> MSR_IA32_VMX_MISC is
>>> +???? * n, then (n + 1) * 512 is the recommended max number of MSRs
>>> to be
>>> +???? * included in the VMExit and VMEntry MSR auto switch list.
>>> +???? */
>>> +??? rdmsrl(MSR_IA32_VMX_MISC, vmx_msr);
>>> +??? n = ((vmx_msr & 0xe000000) >> 25) + 1;
>>> +??? msr_autoload_count_max = n *
KVM_VMX_DEFAULT_MSR_AUTO_LOAD_COUNT;
>>> +}
>>> +
>>
>> Any reasons to do this if it's unlikely that we'll ever update
more than
>> 512 MSRs?
>>
>> Paolo
> 
> It isn't a must to allocate memory for 512 MSRs, but I think it would
> be good to allocate memory at least for 128 MSRs, because on skylake
> we have 32 triplets of MSRs (FROM/TO/INFO), which are 96 in total
> already.
> 
> The disadvantage is that developers would need to manually calculate
> and change the number carefully when they need to add new MSRs for
> auto switching in the future. So, if consuming a little bit more
> memory isn't a concern, I think we can directly allocate memory based
> on what's recommended by the hardware.
Sure.  I was just thinking that it's unnecessary to actually use
VMX_MISC; sticking to one-page allocations is nicer.

Paolo