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
