Re: [Xen-devel] [PATCH v4 2/2] Xen: Fix VMCS setting for x2APIC mode guest while enabling APICV

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 29.01.13 at 06:56, "Li, Jiongxi" <jiongxi.li@xxxxxxxxx> wrote:
> +void vmx_enable_intercept_for_msr(struct vcpu *v, u32 msr, int type)
> +{
> +    unsigned long *msr_bitmap = v->arch.hvm_vmx.msr_bitmap;
> +
> +    /* VMX MSR bitmap supported? */
> +    if ( msr_bitmap == NULL )
> +        return;
> +
> +    /*
> +     * See Intel PRM Vol. 3, 20.6.9 (MSR-Bitmap Address). Early manuals
> +     * have the write-low and read-high bitmap offsets the wrong way round.
> +     * We can control MSRs 0x00000000-0x00001fff and 0xc0000000-0xc0001fff.
> +     */
> +    if ( msr <= 0x1fff )
> +    {
> +        if (type & MSR_TYPE_R)

Missing blanks again. I realize that you just cloned
vmx_disable_intercept_for_msr(), but you shouldn't repeat
mistakes made in the original (on the opposite: since you have
to modify the original anyway, you may feel free to adjust
the coding convention violation there too).

> +            set_bit(msr, msr_bitmap + 0x000/BYTES_PER_LONG); /* read-low */
> +        if (type & MSR_TYPE_W)
> +            set_bit(msr, msr_bitmap + 0x800/BYTES_PER_LONG); /* write-low */
> +    }
> +    else if ( (msr >= 0xc0000000) && (msr <= 0xc0001fff) )
> +    {
> +        msr &= 0x1fff;
> +        if (type & MSR_TYPE_R)
> +            set_bit(msr, msr_bitmap + 0x400/BYTES_PER_LONG); /* read-high */
> +        if (type & MSR_TYPE_W)
> +            set_bit(msr, msr_bitmap + 0xc00/BYTES_PER_LONG); /* write-high */

I believe that while the corresponding disable function is fine in
this regard, here you need to do something in a final "else":
A disable not having any effect is fine (we'll still get the
intercept), but an enable not having any effect is a problem. So
I'd suggest adding a one-time warning and/or ASSERT(0) there.

>      if ( !vlapic_hw_disabled(vlapic) &&
>           (vlapic_base_address(vlapic) == APIC_DEFAULT_PHYS_BASE) )
> -        v->arch.hvm_vmx.secondary_exec_control |=
> -            SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES;
> +    {
> +        if ( virtualize_x2apic_mode &&
> +             vlapic_x2apic_mode(vlapic) )

While this easily fits on one line, ...

> +        {
> +            v->arch.hvm_vmx.secondary_exec_control |=
> +                SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE;
> +            if ( cpu_has_vmx_apic_reg_virt )
> +            {
> +                for (int msr = MSR_IA32_APICBASE_MSR; msr <= 
> MSR_IA32_APICBASE_MSR + 0xff; msr++)

... long lines like this ...

> +                    vmx_disable_intercept_for_msr(v, msr, MSR_TYPE_R);
> +
> +                vmx_enable_intercept_for_msr(v, MSR_IA32_APICPPR_MSR, 
> MSR_TYPE_R);
> +                vmx_enable_intercept_for_msr(v, MSR_IA32_APICTMICT_MSR, 
> MSR_TYPE_R);
> +                vmx_enable_intercept_for_msr(v, MSR_IA32_APICTMCCT_MSR, 
> MSR_TYPE_R);

... or these need to be broken up. 

> +            }
> +            if ( cpu_has_vmx_virtual_intr_delivery )
> +            {
> +                vmx_disable_intercept_for_msr(v, MSR_IA32_APICTPR_MSR, 
> MSR_TYPE_W);
> +                vmx_disable_intercept_for_msr(v, MSR_IA32_APICEOI_MSR, 
> MSR_TYPE_W);
> +                vmx_disable_intercept_for_msr(v, MSR_IA32_APICSELF_MSR, 
> MSR_TYPE_W);
> +            }
> +        }
> +        else
> +        {
> +            v->arch.hvm_vmx.secondary_exec_control |=
> +                SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES;
> +            for (int msr = MSR_IA32_APICBASE_MSR; msr <= 
> MSR_IA32_APICBASE_MSR + 0xff; msr++)
> +                vmx_enable_intercept_for_msr(v, msr, MSR_TYPE_R);
> +
> +            vmx_enable_intercept_for_msr(v, MSR_IA32_APICTPR_MSR, 
> MSR_TYPE_W);
> +            vmx_enable_intercept_for_msr(v, MSR_IA32_APICEOI_MSR, 
> MSR_TYPE_W);
> +            vmx_enable_intercept_for_msr(v, MSR_IA32_APICSELF_MSR, 
> MSR_TYPE_W);

Wouldn't it be more safe (especially towards future changes to
the if() portion above) to simply enable all intercepts for read
and write in the loop, rather than special casing the three ones
that _currently_ get their write intercepts disabled above?

Jan

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