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

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

Andrew Cooper wrote on 2014-10-14:
> 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@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
> 
> It is run once per vLAPIC on  restore.  I would not qualify it as
> performance critical, but I would also advocate the efficient method

Since the live migration is largely used in cloud, and the downtime of live 
migration is very important to cloud user. So it is worth to put the restore 
code path as performance critical area in future. BTW, I am working on KVM to 
reduce the downtime now.

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

I didn't get you point. Can you explain more?

> 
> This range cannot possibly be skipped.
> 
> ~Andrew


Best regards,
Yang



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