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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4246 bytes --]

On 03/07/2017 06:16 PM, Julian Kirsch wrote:
> Provide read/write access to x86 model specific registers (MSRs) by means of
> two new HMP commands "msr-list" and "msr-set". The rationale behind this
> is to improve introspection capabilities for system virtualization mode.
> For instance, many modern x86-64 operating systems maintain access to internal
> data structures via the MSR_GSBASE/MSR_KERNELGSBASE MSRs. Giving
> introspection utilities (such as a remotely attached gdb) a way of
> accessing these registers improves analysis results drastically.
> 
> Signed-off-by: Julian Kirsch <git@kirschju.re>
> ---

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?


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

> +##
> +{ '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'.

> +
> +##
> +# @msr-list:
> +#
> +# Retrieve model specific registers (MSRs) on x86
> +#
> +# @msr_idx: MSR index to read
> +#
> +# Returns: A list of one MSR value per CPU, or nothing
> +#
> +# Since: 2.8.1

2.10

> +##
> +{ 'command': 'msr-list', 'returns': ['MsrInfo'],
> +  'data': {'msr_idx': 'uint32'} }

'msr-idx' (or even 'msr-index')

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

> +#
> +# Since: 2.8.1

2.10

> +##
> +{ 'command': 'msr-set',
> +  'data': {'cpu_idx': 'uint32', 'msr_idx': 'uint32', 'value': 'uint64'} }

again, dash instead of underscore


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]