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

["Li, Jiongxi" <jiongxi.li@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Tuesday, January 29, 2013 5:08 PM
> To: Li, Jiongxi
> Cc: Keir Fraser; xen-devel
> Subject: Re: [PATCH v4 2/2] Xen: Fix VMCS setting for x2APIC mode guest while
> enabling APICV
> 
> >>> 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.

A final "else" means out of the ranges 00000000H - 00001FFFH and
C0000000H - C0001FFFH. According to SDM, RDMSR and WRMSR
out of the ranges will cause a VM exit, it is just what we want for
"enable intercept" function, right?. So is it necessary to handle 
"else" case here?
> 
> >      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