Re: [Xen-devel] [PATCH v1 1/6] vmx: add struct vmx_msr_policy

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 26.06.17 at 12:44, <sergey.dyasli@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -144,6 +144,14 @@ static void __init vmx_display_features(void)
>          printk(" - none\n");
>  }
>  
> +bool vmx_msr_available(struct vmx_msr_policy *p, uint32_t msr)

const

> --- a/xen/include/asm-x86/hvm/vmx/vmcs.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
> @@ -562,6 +562,350 @@ void vmx_domain_flush_pml_buffers(struct domain *d);
>  
>  void vmx_domain_update_eptp(struct domain *d);
>  
> +union vmx_pin_based_exec_control_bits {
> +    uint32_t raw;
> +    struct {
> +        bool ext_intr_exiting:1;
> +        uint32_t             :2;  /* 1:2 reserved */
> +        bool      nmi_exiting:1;
> +        uint32_t             :1;  /* 4 reserved */
> +        bool     virtual_nmis:1;
> +        bool    preempt_timer:1;
> +        bool posted_interrupt:1;
> +        uint32_t             :24; /* 8:31 reserved */

This mixture of bool and uint32_t worries me - I don't think the
resulting layout is well defined. Yes, you put suitable
BUILD_BUG_ON()s in place to catch possible issues, but anyway.

> +struct vmx_msr_policy
> +{
> +    /*
> +     * Bitmap of readable MSRs, starting from MSR_IA32_VMX_BASIC,
> +     * derived from contents of MSRs in this structure.
> +     */
> +    uint32_t available;
> +
> +    union {
> +        uint64_t msr[MSR_IA32_VMX_VMFUNC - MSR_IA32_VMX_BASIC + 1];

Considering the recurring use of MSR_IA32_VMX_VMFUNC,
wouldn't it be worthwhile to have a "last" #define? You'd then
clearly want to add a BUILD_BUG_ON() to vmx_msr_available()
making sure the delta doesn't grow beyond 32.

> +        struct {
> +            /* MSR 0x480 */

Please also give the msr-index.h name in the comment, for grep-s
to match here. In fact I'm unconvinced the hex index is of much use.

> +            union {
> +                uint64_t raw;
> +                struct {
> +                    uint32_t vmcs_revision_id:31;
> +                    bool                     :1;  /* 31 always zero */

Name it mbz then?

> +            /* MSR 0x486 */
> +            union {
> +                uint64_t raw;
> +                struct {
> +                    uint32_t allowed_0;
> +                    uint32_t :32;
> +                };
> +            } cr0_fixed_0;

I can't find any indication that this and the following MSRs have an
undefined upper half. The VMCS fields they correspond to are native
width, so I think the type here should be unsigned long.

Yet then the question arises whether breaking these up into bit
fields wouldn't be useful too. Of if that's no useful, is there really
a point in having both a "raw" and a properly named field?

> +            /* MSR 0x48A */
> +            union {
> +                uint64_t raw;
> +                struct {
> +                    uint32_t                      :1;  /* 0 reserved */
> +                    uint32_t vmcs_encoding_max_idx:9;
> +                    uint64_t                      :54; /* 10:63 reserved */

This pairing of uint32_t and uint64_t looks even more worrying to
me than the bool/uint32_t one further up. I'm actually surprised
this doesn't cause the respective BUILD_BUG_ON() to trigger.

> +            /* MSR 0x491 */
> +            union {
> +                uint64_t raw;
> +                struct {
> +                    bool eptp_switching:1;
> +                };

Any reason the other 63 bits don't have a placeholder here, just like
you do everywhere else?

Jan


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