From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH for 4.5 v2] vmx, apicv: fix save/restore issue with apicv Date: Mon, 27 Oct 2014 10:47:28 +0000 Message-ID: <544E22C0.8030704@citrix.com> References: <1413869566-23391-1-git-send-email-yang.z.zhang@intel.com> <54463DF1020000780004091C@mail.emea.novell.com> <544A33F10200007800041CAD@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: "Zhang, Yang Z" , Jan Beulich Cc: "Tian, Kevin" , "olaf@aepfle.de" , "malcolm.crossley@citrix.com" , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org On 27/10/14 01:00, Zhang, Yang Z wrote: > Jan Beulich wrote on 2014-10-24: >>>>> On 24.10.14 at 04:00, wrote: >>> Jan Beulich wrote on 2014-10-21: >>>>>>> On 21.10.14 at 07:32, wrote: >>>>> --- a/xen/arch/x86/hvm/vmx/vmx.c >>>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c >>>>> @@ -1584,6 +1584,8 @@ static void vmx_process_isr(int isr, struct >>>>> vcpu >>>>> *v) { >>>>> unsigned long status; >>>>> u8 old; >>>>> + unsigned int i, vector; >>>> I don't really see the need for two variables here - just "i" would >>>> seem to suffice. >>>> >>>>> + struct vlapic *vlapic = vcpu_vlapic(v); >>>> const? >>>> >>>>> @@ -1597,6 +1599,28 @@ static void vmx_process_isr(int isr, struct >>>>> vcpu >>>> *v) >>>>> status |= isr << VMX_GUEST_INTR_STATUS_SVI_OFFSET; >>>>> __vmwrite(GUEST_INTR_STATUS, status); >>>>> } >>>>> + + /* + * Theoretically, only level triggered interrupts can >>>>> have their + * corresponding bits set in the eoi exit bitmap. >>>>> That is, the bits + * set in the eoi exit bitmap should also be >>>>> set in TMR. But a periodic + * timer interrupt does not follow >>>>> the rule: it is edge triggered, but + * requires its >>>>> corresponding bit be set in the eoi exit bitmap. So we + * should >>>>> not construct the eoi exit bitmap based on TMR. + * Here we will >>>>> construct the eoi exit bitmap via (IRR | ISR). This + * means >>>>> that EOIs to the interrupts that are set in the IRR + or ISR will + >>>>> * cause VM exits after restoring, regardless of the trigger modes. >>>>> It + * is acceptable because the subsequent interrupts will set + >>>>> up the eoi + * bitmap correctly. + */ + for ( vector = >>>>> 0x10; vector < NR_VECTORS; vector++ ) + if ( >>>>> vlapic_test_vector(vector, + &vlapic->regs->data[APIC_IRR]) || + >>>>> vlapic_test_vector(vector, &vlapic->regs->data[APIC_ISR]) ) + >>>>> set_bit(vector, v->arch.hvm_vmx.eoi_exit_bitmap); + + >>>>> for ( i = 0; i < 4; i++ ) >>>> ARRAY_SIZE(v->arch.hvm_vmx.eoi_exit_bitmap) instead of 4 please, >>>> like already done in construct_vmcs(). >>>> >>>>> + __vmwrite(EOI_EXIT_BITMAP(i), >>>>> + v->arch.hvm_vmx.eoi_exit_bitmap[i]); >>>>> + >>>>> vmx_vmcs_exit(v); >>>>> } >>>>> --- a/xen/include/asm-x86/hvm/vlapic.h >>>>> +++ b/xen/include/asm-x86/hvm/vlapic.h >>>>> @@ -61,6 +61,8 @@ >>>>> >>>>> #define VEC_POS(v) ((v) % 32) >>>>> #define REG_POS(v) (((v) / 32) * 0x10) >>>>> +#define vlapic_test_vector(vec, bitmap) \ + test_bit(VEC_POS(vec), >>>>> (uint32_t *)((bitmap) + REG_POS(vec))) >>>> Don't cast away possible const-ness here. >>>> >>>> But of course if there are no other needs to change the patch I >>>> could take care of all of these while committing. >>> It appears no other comments. Should I send out the v3 or you will >>> help to commit it with modifications? >> As said above - I'm fine doing those minor adjustments, but I >> certainly would like to wait for Citrix folks' confirmation. This has >> been taking so long that I don't think waiting a few more days really matters. > I want to catch the first RC release since we will perform a full testing with first RC. But it is ok now since the first RC already released. With the changes Jan wants, Reviewed-by: Andrew Cooper We don't have much time for out-of-band testing at the moment, but I will test it specifically as part of our 4.5-rc$N testing. The patch does look functionally very similar to the previous version which we positively confirmed as fixing our observed issues. ~Andrew