Re: [Xen-devel] [PATCH] x86/hvm: implement save/restore for posted interrupts

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

Andrew Cooper wrote on 2014-10-09:
> On 09/10/14 08:24, Zhang, Yang Z wrote:
> 
> 
>       Olaf Hering wrote on 2014-10-08:
> 
>               On Wed, Oct 08, Zhang, Yang Z wrote:
> 
> 
>                       Olaf Hering wrote on 2014-10-08:
> 
>                               On Wed, Oct 08, Zhang, Yang Z wrote:
> 
>                                       Can you reproduce it with APICv 
> disabled? You can add apicv=0
>                                       into your grub to disable it.
> 
>                               Using 'apicv=0' with staging helps, the guest 
> resumes properly.
> 
>                       Does 'resumes properly' mean there are two sysrq in 
> dmesg after
> resume?
> 
> 
>               Yes.
> 
> 
>                               Will now try the patch you mentioned in the 
> other mail.
> 
>                       Thanks. I am set upping the environment for testing now.
> 
> 
>               That patch (Message-ID: <20140804075007.GA11609@xxxxxxxxx>
> <mailto:20140804075007.GA11609@xxxxxxxxx> ) does not
>               help.
> 
> 
>       Sorry. I forget to tell that that patch must combine with one fixing
> from your patch.
>       I rebased it based on latest Xen and the attached patch includes all 
> fixings.
> Could you have a try? It works on my side, hope it helps.
> 
>       diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
>       index 99ae1be..e702ed3 100644
>       --- a/xen/arch/x86/hvm/vlapic.c
>       +++ b/xen/arch/x86/hvm/vlapic.c
>       @@ -1259,6 +1259,9 @@ static int lapic_save_regs(struct domain *d,
> hvm_domain_context_t *h)
> 
>            for_each_vcpu ( d, v )          {  +        if (
> hvm_funcs.sync_pir_to_irr )   +            hvm_funcs.sync_pir_to_irr(v);
>       +                s = vcpu_vlapic(v);             if ( (rc =
> hvm_save_entry(LAPIC_REGS, v->vcpu_id, h, s->regs)) != 0 )                
> break;        diff --git a/xen/arch/x86/hvm/vmx/vmx.c
> b/xen/arch/x86/hvm/vmx/vmx.c  index 304aeea..7c4d796 100644   ---
> 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;    +    int vector;
> 
> 
> unsigned vector; especially as it is used in some pointer arithmetic
> hidden inside the vlapic_test_vector() macro.
> 
> 
> 
> 
>       +    struct vlapic *s = vcpu_vlapic(v);
> 
> 
> 's' seems to be a strange choice of variable name.  Prevailing use in
> this file is "struct vlapic *vlapic = vcpu_vlapic(v);"
> 
> 
> 
> 
> 
>            if ( isr < 0 )              isr = 0;       @@ -1597,6 +1599,14 @@ 
> static
> void vmx_process_isr(int isr, struct vcpu *v)                  status |= isr 
> <<
> VMX_GUEST_INTR_STATUS_SVI_OFFSET;              __vmwrite(GUEST_INTR_STATUS,
> status);           }  +    for ( vector = 0; vector < NR_VECTORS; vector++ )
> 
> 
> Vectors 0 to 0x0f are strictly reserved.  Any reason not to start
> vector at
> 0x10 ?
> 
> 
> 
> 
>       +        if (vlapic_test_vector(vector, &s->regs->data[APIC_TMR]))
>       +            set_bit(vector,  v->arch.hvm_vmx.eoi_exit_bitmap);
> 
> 
> Can't this loop be optimised somewhat to using word-sized |= operations?

This is just a test patch to see whether it is able to fix the problem. I will 
refine it once get the confirm from Olaf. 

> 
> ~Andrew
> 
> 
> 
> 
>       +
>       +    __vmwrite(0x201c, v->arch.hvm_vmx.eoi_exit_bitmap[0]);
>       +    __vmwrite(0x201e, v->arch.hvm_vmx.eoi_exit_bitmap[1]);
>       +    __vmwrite(0x2020, v->arch.hvm_vmx.eoi_exit_bitmap[2]);
>       +    __vmwrite(0x2022, v->arch.hvm_vmx.eoi_exit_bitmap[3]);
>            vmx_vmcs_exit(v);
>        }
> 
>       diff --git a/xen/include/asm-x86/hvm/vlapic.h
> b/xen/include/asm-x86/hvm/vlapic.h    index bf59b95..fc8d131 100644   ---
> 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)))         #define
> vlapic_test_and_set_vector(vec, bitmap) \         
> test_and_set_bit(VEC_POS(vec), (uint32_t *)((bitmap) + REG_POS(vec)))         
> #define vlapic_test_and_clear_vector(vec, bitmap)
> 
> 
> 
>               Olaf
> 
> 
> 
>       Best regards,
>       Yang
> 
> 
> 
> 
> 
>       _______________________________________________
>       Xen-devel mailing list
>       Xen-devel@xxxxxxxxxxxxx
>       http://lists.xen.org/xen-devel
>


Best regards,
Yang


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