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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

> +    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.

> +    /*
> +     * 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).

> +    {
> +        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.

Also please check the spelling in that comment.

Jan


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