Re: [Xen-devel] [PATCH] vmx, apicv: fix save/restore issue with apicv

["Zhang, Yang Z" <yang.z.zhang@xxxxxxxxx>]

Jan Beulich wrote on 2014-10-13:
> >>> On 11.10.14 at 09:54, <yang.z.zhang@xxxxxxxxx> wrote:
> > --- a/xen/arch/x86/hvm/vmx/vmx.c
> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
> > @@ -1583,7 +1583,9 @@ static int
> > vmx_virtual_intr_delivery_enabled(void)
> >  static void vmx_process_isr(int isr, struct vcpu *v)  {
> >      unsigned long status;
> > -    u8 old;
> > +    u8 old, i;
> 
> There is absolutely no point in i being u8 - this can be plain unsigned int.

It's ok to change it to unsigned int.

> 
> > +    unsigned int *eoi_exit_bitmap, val;
> > +    struct vlapic *vlapic = vcpu_vlapic(v);
> >
> >      if ( isr < 0 )
> >          isr = 0;
> > @@ -1597,6 +1599,29 @@ static void vmx_process_isr(int isr, struct vcpu
> *v)
> >          status |= isr << VMX_GUEST_INTR_STATUS_SVI_OFFSET;
> >          __vmwrite(GUEST_INTR_STATUS, status);
> >      }
> > +
> > +    eoi_exit_bitmap = (unsigned int
> > + *)v->arch.hvm_vmx.eoi_exit_bitmap;
> 
> No casts like this please. This is a bitmap and would hence preferably be
> treated that way consistently. The code here isn't performance critical.

Yes, it's performance critical from the live migration's point and that's why I 
use the cast here and

> 
> > +    /*
> > +     * We cannot simple copy the TMR as eoi exit bitmap due to the special
> > +     * periodic timer interrupt which is edge trigger but need a mandatory
> > +     * EOI-induced VM exit to let pending periodic timer interrupts have
> the
> > +     * chance to be injected for compensation.
> > +     * Here we will construct the eoi exit bimap via (IRR | ISR). Though it
> > +     * may cause unnecessary eoi exit of the interrupts that pending in IRR
> or
> > +     * ISR during save/resotre, each pending interrupt only causes one
> vmexit.
> > +     * The subsequent interrupts will adjust the eoi exit bitmap correctly.
> So
> > +     * the performace hurt can be ingored.
> > +     */
> > +    for ( i = 0x10; i < 0x80; i += 0x10 )
> 
> So you skip the first 32 vectors instead of just the first 16. That's not in 
> line with


> the APIC definitions. Also basing the loop variable on APIC register numbers 
> is
> kind of odd. I'd really suggest using something more natural, like vector 
> space
> (which would also allow you to use existing helper functions/macros).
> 

...skip the first 32 vectors and operates based on group instead vector

Also, Spec says "21-31 - Intel reserved. Do not use." So it's ok to skip them.

> > +    {
> > +        val = vlapic_get_reg(vlapic, APIC_IRR + i);
> > +        val |= vlapic_get_reg(vlapic, APIC_ISR + i);
> 
> The comment above doesn't really make clear why not just IRR.

Upon acceptance of an interrupt into the IRR, the corresponding TMR will be 
changed. This means an interrupt either in IRR or in ISR has no chance to 
update the TMR and eoi exit bitmap. So we need to check IRR + ISR while 
rebuilding the eoi exit bitmap. It is not enough to check only IRR.

> 
> Also please check the spelling in that comment.

Sorry for my poor English.

> 
> Jan


Best regards,
Yang



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