Re: [Xen-devel] [PATCH v2 1/5] x86/vmx: add struct vmx_msr_policy

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Sergey Dyasli [mailto:sergey.dyasli@xxxxxxxxxx]
> Sent: Monday, July 24, 2017 9:48 PM
> 
> This structure provides a convenient way of accessing contents of
> VMX MSRs: every bit value is accessible by its name. Bit names match
> existing Xen's definitions as close as possible. The structure also
> contains the bitmap of available MSRs since not all of them may be
> available on a particular H/W.
> 
> A set of helper functions is introduced to provide a simple way of
> interacting with the new structure.
> 
> Signed-off-by: Sergey Dyasli <sergey.dyasli@xxxxxxxxxx>
> ---
> v1 --> v2:
> - Replaced MSR indices with MSR names in struct vmx_msr_policy's
> comments
> - Named "always zero bit" 31 of basic msr as mbz
> - Added placeholder bits into union vmfunc
> - Added structures cr0_bits and cr4_bits
> - Added MSR_IA32_VMX_LAST define to use instead of
> MSR_IA32_VMX_VMFUNC
> - vmx_msr_available() now uses pointer to const struct vmx_msr_policy
> - build_assertions() now uses local struct vmx_msr_policy
> - Added BUILD_BUG_ON to check that width of vmx_msr_policy::available
>   bitmap is enough for all existing VMX MSRs
> - Helpers get_vmx_msr_val(), get_vmx_msr_ptr() and gen_vmx_msr_mask()
>   are added
> 
>  xen/arch/x86/hvm/vmx/vmcs.c        |  78 ++++++++
>  xen/include/asm-x86/hvm/vmx/vmcs.h | 380
> +++++++++++++++++++++++++++++++++++++
>  xen/include/asm-x86/msr-index.h    |   1 +
>  3 files changed, 459 insertions(+)
> 
> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
> index 8103b20d29..33715748f0 100644
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -144,6 +144,40 @@ static void __init vmx_display_features(void)
>          printk(" - none\n");
>  }
> 
> +bool vmx_msr_available(const struct vmx_msr_policy *p, uint32_t msr)

regarding to naming, many functions use vmx_ as prefix 
with later strings represent the actual purpose e.g. 
vmx_msr_read_intercept which is about general msr read
interception. while here your intention is whether "vmx_msr"
is available, which is for specific VMX MSRs. similar for
vmx_msr_policy which reads more general than the real
intention here. Can we find a way to differentiate? SDM
calls this category as "VMX CAPABILITY REPORTING FACILITY",
maybe using vmxcap_msr?

> +{
> +    if ( msr < MSR_IA32_VMX_BASIC || msr > MSR_IA32_VMX_LAST )
> +        return 0;

then why not also introducing MSR_IA32_VMX_FIRST for
better readability?

> +
> +    return p->available & (1u << (msr - MSR_IA32_VMX_BASIC));
> +}
> +
> +uint64_t get_vmx_msr_val(const struct vmx_msr_policy *p, uint32_t msr)
> +{
> +    if ( !vmx_msr_available(p, msr))
> +        return 0;

0 is a valid MSR value. better return value in a pointer parameter and
then return error number here.

> +
> +    return p->msr[msr - MSR_IA32_VMX_BASIC];
> +}

Thanks
Kevin

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel