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-14:
>>>> On 14.10.14 at 15:10, <yang.z.zhang@xxxxxxxxx> wrote:
>> Jan Beulich wrote on 2014-10-14:
>>>>>> On 14.10.14 at 07:43, <yang.z.zhang@xxxxxxxxx> wrote:
>>>> Jan Beulich wrote on 2014-10-13:
>>>>>>>> On 11.10.14 at 09:54, <yang.z.zhang@xxxxxxxxx> wrote:
>>>>>> @@ -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
>>> 
>>> Compared to everything else involved in migration, a few extra
>>> cycles here
>> 
>> Xen is far behind KVM on live migration. If we want to remain the
>> competitive, I'd suggest doing a cleanup on current migration logic
>> to make it more effective.
> 
> But that surely needs to be done elsewhere, where more than a couple
> of nanoseconds are to be gained.

If you think it is ok, I will use vector based operations:

+    for ( vector = 0x10; vector < NR_VECTORS; vector++ )
+        if (vlapic_test_vector(vector, &s->regs->data[APIC_IRR]) ||
+                vlapic_test_vector(vector, &s->regs->data[APIC_ISR]))
+            set_bit(vector,  v->arch.hvm_vmx.eoi_exit_bitmap);

Andrew, is it ok for you? 

> 
>>>>>> +    {
>>>>>> +        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.
>>> 
>>> But then why not look at TMR right away? It's being kept up to date
>>> as long as the guest is running, and gets migrated over afterwards.
>>> Considering what
>>> vmx_update_eoi_exit_bitmap()) does, you really want to or TMR into
>>> EOI_EXIT_BITMAP (which post-migration could just be an assignment
>>> rather than an or).
>> 
>> No, we cannot use TMR directly. As I said in the comments, the
>> periodic timer interrupt is tricky which is edge-trigger but need an
>> EOI vmexit to let the pending interrupts have the chance to be
>> injected for
> compensation.
> 
> The respective "normal" (non-migration) handling of this being in
> vmx_intr_assist(), involving pt_vector? If so, I think I understand now.

Yes.

> 
> Thanks, Jan


Best regards,
Yang



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