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

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

[-- Attachment #1.1: Type: text/plain, Size: 3850 bytes --]

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@aepfle.de>) 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?

~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@lists.xen.org
> http://lists.xen.org/xen-devel


[-- Attachment #1.2: Type: text/html, Size: 5886 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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