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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 14/10/14 06:43, Zhang, Yang Z wrote:
> Jan Beulich wrote on 2014-10-13:
>>>>> On 11.10.14 at 09:54, <yang.z.zhang@intel.com> 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

It is run once per vLAPIC on  restore.  I would not qualify it as
performance critical, but I would also advocate the efficient method of
word accesses over the inefficient bit accesses where it makes sense.

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

And what do you expect will happen to DoS guests which continue to use
the legacy PICs?

This range cannot possibly be skipped.

~Andrew