Re: [Qemu-devel] [PATCH] X86/HMP: Expose x86 model specific registers via human monitor

[Julian Kirsch]

Hi Eric,

thank you for your comments. My answers are inlined.

On 08.03.2017 03:36, Eric Blake wrote:

> 
> I'm just focusing on the QMP interface portion of this.
> 
> Is any of this information...
> 
>> This patch moves the logic of the rdmsr and wrmsr functions to helper.c and
>> replaces their current versions in misc_helper.c with stubs calling the new
>> functions. The ordering of MSRs now loosely follows the ordering used in the 
>> KVM
>> module. As qemu operates on cached values in the CPUX86State struct, the 
>> msr-set
>> command is implemented in a hackish way: In order to force qemu to flush the 
>> new
>> values to KVM a call to cpu_synchronize_post_init is triggered, eventually
>> ending up in calling kvm_put_msrs at KVM_PUT_FULL_STATE level. As MSR writes
>> could *still* be caught by the code logic in this function, the msr-set 
>> function
>> reads back the values written to determine whether the write was successful.
>> This way, we don't have to duplicate the logic used in kvm_put_msrs 
>> (has_msr_XXX)
>> to x86_cpu_wrmsr.
>> There are several things I would like to pooint out about this patch:
>>   - The "msr-list" command currently displays MSR values for all virtual 
>> cpus.
>>     This is somewhat inconsistent with "info registers" just displaying the
>>     value of the current default cpu. One might think about just displaying 
>> the
>>     current value and offer access to other CPU's MSRs by means of switching
>>     between CPUs using the "cpu" monitor command.
>>   - The new version of x86_cpu_rdmsr exposes MSRs that are arguably of
>>     questionable help for any human / tool using the monitor. However I 
>> merely
>>     felt a deep urge to reflect the full MSR list from kvm.c when writing the
>>     code.
>>   - While the need for msr-list is evident (see commit msg), msr-set could be
>>     used in more obscure cases. For instance, one might offer a way to access
>>     and configure performance counter MSRs of the guest via the hmp. If this
>>     feels too much like an inacceptable hack, I'll happily drop the msr-set
>>     part.
> 
> ...useful above the --- as part of the commit message?
> 

I tried to explain the issue of qemu not flushing back the msrs to kvm in cpu.c
. The remainder is merely design questions that are imho not important for the
commit message.

> 
>> +++ b/qapi-schema.json
>> @@ -2365,6 +2365,55 @@
>>    'data': {'val': 'int', 'size': 'int', 'filename': 'str'} }
>>  
>>  ##
>> +# @MsrInfo:
>> +#
>> +# Information about a MSR
>> +#
>> +# @cpu_idx: CPU index
>> +#
>> +# @msr_idx: MSR index
>> +#
>> +# @value: MSR value
>> +#
>> +# Since: 2.8.1
> 
> You've missed 2.8 by a long shot; you've even missed soft freeze for
> 2.9.  This should be 2.10.
> 

Ops. thanks for pointing this out, will update it. I sticked to the latest
version returned by "git tag".

>> +##
>> +{ 'struct': 'MsrInfo',
>> +  'data': {'cpu_idx': 'int', 'msr_idx': 'uint32', 'value': 'uint64'} }
> 
> Please spell new members with '-' rather than '_', as in 'cpu-idx' (or
> even spell it out as 'cpu-index') and 'msr-idx'.
> 

Again, thank you, will take care of it.

>> +
>> +##
>> +# @msr-set:
>> +#
>> +# Set model specific registers (MSRs) on x86
>> +#
>> +# @cpu_idx: CPU holding the MSR that should be written
>> +#
>> +# @msr_idx: MSR index to write
>> +#
>> +# @value: Value to write
>> +#
>> +# Returns: Nothing on success
> 
> Useless Returns: line.
> 

Removed.

Best,
Julian