Xen devel - Nov 2013 - [PATCH v4] x86: properly handle MSI-X unmask operation from guests

patch revision history
----------------------
v1: Initial patch to handle this issue involving changing the hypercall
interface
v2:Totally handled inside hypervisor.
v3:Change some logics of handling msi-x pending unmask operations.
v4:Some changes related to coding style according to Andrew Cooper''s
comments

From 51c5b7f1f9c8f319da8adf021b39e18fbd3bf314 Mon Sep 17 00:00:00 2001
From: Feng Wu <feng.wu@intel.com>
Date: Wed, 13 Nov 2013 21:43:48 -0500
Subject: [PATCH] x86: properly handle MSI-X unmask operation from guests

For a pass-through device with MSI-x capability, when guest tries
to unmask the MSI-x interrupt for the passed through device, xen
doesn''t clear the mask bit for MSI-x in hardware in the following
scenario, which will cause network disconnection:

1. Guest masks the MSI-x interrupt
2. Guest updates the address and data for it
3. Guest unmasks the MSI-x interrupt (This is the problematic step)

In the step #3 above, Xen doesn''t handle it well. When guest tries
to unmask MSI-X interrupt, it traps to Xen, Xen just returns to Qemu
if it notices that address or data has been modified by guest before,
then Qemu will update Xen with the latest value of address/data by
hypercall. However, in this whole process, the MSI-X interrupt unmask
operation is missing, which means Xen doesn''t clear the mask bit in
hardware for the MSI-X interrupt, so it remains disabled, that is why
it loses the network connection.

This patch fixes this issue.

Signed-off-by: Feng Wu <feng.wu@intel.com>
---
 xen/arch/x86/hvm/io.c        |    3 +++
 xen/arch/x86/hvm/vmsi.c      |   18 ++++++++++++++++++
 xen/include/asm-x86/domain.h |    8 ++++++++
 xen/include/xen/pci.h        |    1 +
 4 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c
index deb7b92..6c83c25 100644
--- a/xen/arch/x86/hvm/io.c
+++ b/xen/arch/x86/hvm/io.c
@@ -297,6 +297,9 @@ void hvm_io_assist(ioreq_t *p)
         break;
     }

+    if ( msix_post_handler(curr) )
+        gdprintk(XENLOG_ERR, "msix_post_handler error\n");
+
     if ( p->state == STATE_IOREQ_NONE )
         vcpu_end_shutdown_deferral(curr);
 }
diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
index 4826b4a..2cdd0dc 100644
--- a/xen/arch/x86/hvm/vmsi.c
+++ b/xen/arch/x86/hvm/vmsi.c
@@ -293,7 +293,11 @@ static int msixtbl_write(struct vcpu *v, unsigned long
address,

     /* exit to device model if address/data has been modified */
     if ( test_and_clear_bit(nr_entry, &entry->table_flags) )
+    {
+        v->arch.pending_msix_unmask.valid = 1;
+        v->arch.pending_msix_unmask.ctrl_address = address;
         goto out;
+    }

     virt = msixtbl_addr_to_virt(entry, address);
     if ( !virt )
@@ -528,3 +532,17 @@ void msixtbl_pt_cleanup(struct domain *d)
     spin_unlock(&d->arch.hvm_domain.msixtbl_list_lock);
     local_irq_restore(flags);
 }
+
+int msix_post_handler(struct vcpu *v)
+{
+    int rc;
+
+    if ( v->arch.pending_msix_unmask.valid == 0 )
+        return 0;
+
+    v->arch.pending_msix_unmask.valid = 0;
+
+    rc = msixtbl_write(v, v->arch.pending_msix_unmask.ctrl_address, 4, 0);
+    return rc != X86EMUL_OKAY ? -1 : 0;
+}
+
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 9d39061..89b1adc 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -375,6 +375,12 @@ struct pv_vcpu
     spinlock_t shadow_ldt_lock;
 };

+struct pending_msix_unmask_info
+{
+    unsigned long ctrl_address;
+    bool_t valid;
+};
+
 struct arch_vcpu
 {
     /*
@@ -439,6 +445,8 @@ struct arch_vcpu

     /* A secondary copy of the vcpu time info. */
     XEN_GUEST_HANDLE(vcpu_time_info_t) time_info_guest;
+
+    struct pending_msix_unmask_info pending_msix_unmask;
 } __cacheline_aligned;

 /* Shorthands to improve code legibility. */
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index cadb525..ce8f6ff 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -147,5 +147,6 @@ struct pirq;
 int msixtbl_pt_register(struct domain *, struct pirq *, uint64_t gtable);
 void msixtbl_pt_unregister(struct domain *, struct pirq *);
 void msixtbl_pt_cleanup(struct domain *d);
+int msix_post_handler(struct vcpu *v);

 #endif /* __XEN_PCI_H__ */
--
1.7.1

Thanks,
Feng

On 22/11/13 01:08, Wu, Feng wrote:
> patch revision history
> ----------------------
> v1: Initial patch to handle this issue involving changing the hypercall
interface
> v2:Totally handled inside hypervisor.
> v3:Change some logics of handling msi-x pending unmask operations.
> v4:Some changes related to coding style according to Andrew
Cooper''s comments
>
> From 51c5b7f1f9c8f319da8adf021b39e18fbd3bf314 Mon Sep 17 00:00:00 2001
> From: Feng Wu <feng.wu@intel.com>
> Date: Wed, 13 Nov 2013 21:43:48 -0500
> Subject: [PATCH] x86: properly handle MSI-X unmask operation from guests
>
> For a pass-through device with MSI-x capability, when guest tries
> to unmask the MSI-x interrupt for the passed through device, xen
> doesn''t clear the mask bit for MSI-x in hardware in the following
> scenario, which will cause network disconnection:
>
> 1. Guest masks the MSI-x interrupt
> 2. Guest updates the address and data for it
> 3. Guest unmasks the MSI-x interrupt (This is the problematic step)
>
> In the step #3 above, Xen doesn''t handle it well. When guest tries
> to unmask MSI-X interrupt, it traps to Xen, Xen just returns to Qemu
> if it notices that address or data has been modified by guest before,
> then Qemu will update Xen with the latest value of address/data by
> hypercall. However, in this whole process, the MSI-X interrupt unmask
> operation is missing, which means Xen doesn''t clear the mask bit
in
> hardware for the MSI-X interrupt, so it remains disabled, that is why
> it loses the network connection.
>
> This patch fixes this issue.
>
> Signed-off-by: Feng Wu <feng.wu@intel.com>
> ---
>  xen/arch/x86/hvm/io.c        |    3 +++
>  xen/arch/x86/hvm/vmsi.c      |   18 ++++++++++++++++++
>  xen/include/asm-x86/domain.h |    8 ++++++++
>  xen/include/xen/pci.h        |    1 +
>  4 files changed, 30 insertions(+), 0 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c
> index deb7b92..6c83c25 100644
> --- a/xen/arch/x86/hvm/io.c
> +++ b/xen/arch/x86/hvm/io.c
> @@ -297,6 +297,9 @@ void hvm_io_assist(ioreq_t *p)
>          break;
>      }
>
> +    if ( msix_post_handler(curr) )
> +        gdprintk(XENLOG_ERR, "msix_post_handler error\n");
> +
>      if ( p->state == STATE_IOREQ_NONE )
>          vcpu_end_shutdown_deferral(curr);
>  }
> diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
> index 4826b4a..2cdd0dc 100644
> --- a/xen/arch/x86/hvm/vmsi.c
> +++ b/xen/arch/x86/hvm/vmsi.c
> @@ -293,7 +293,11 @@ static int msixtbl_write(struct vcpu *v, unsigned long
address,
>
>      /* exit to device model if address/data has been modified */
>      if ( test_and_clear_bit(nr_entry, &entry->table_flags) )
> +    {
> +        v->arch.pending_msix_unmask.valid = 1;
> +        v->arch.pending_msix_unmask.ctrl_address = address;
>          goto out;
> +    }
>
>      virt = msixtbl_addr_to_virt(entry, address);
>      if ( !virt )
> @@ -528,3 +532,17 @@ void msixtbl_pt_cleanup(struct domain *d)
>      spin_unlock(&d->arch.hvm_domain.msixtbl_list_lock);
>      local_irq_restore(flags);
>  }
> +
> +int msix_post_handler(struct vcpu *v)
> +{
> +    int rc;
> +
> +    if ( v->arch.pending_msix_unmask.valid == 0 )
> +        return 0;
> +
> +    v->arch.pending_msix_unmask.valid = 0;
> +
> +    rc = msixtbl_write(v, v->arch.pending_msix_unmask.ctrl_address, 4,
0);
Wont this corrupt other information which might be present in the register ?

~Andrew
> +    return rc != X86EMUL_OKAY ? -1 : 0;
> +}
> +
> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
> index 9d39061..89b1adc 100644
> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -375,6 +375,12 @@ struct pv_vcpu
>      spinlock_t shadow_ldt_lock;
>  };
>
> +struct pending_msix_unmask_info
> +{
> +    unsigned long ctrl_address;
> +    bool_t valid;
> +};
> +
>  struct arch_vcpu
>  {
>      /*
> @@ -439,6 +445,8 @@ struct arch_vcpu
>
>      /* A secondary copy of the vcpu time info. */
>      XEN_GUEST_HANDLE(vcpu_time_info_t) time_info_guest;
> +
> +    struct pending_msix_unmask_info pending_msix_unmask;
>  } __cacheline_aligned;
>
>  /* Shorthands to improve code legibility. */
> diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
> index cadb525..ce8f6ff 100644
> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -147,5 +147,6 @@ struct pirq;
>  int msixtbl_pt_register(struct domain *, struct pirq *, uint64_t gtable);
>  void msixtbl_pt_unregister(struct domain *, struct pirq *);
>  void msixtbl_pt_cleanup(struct domain *d);
> +int msix_post_handler(struct vcpu *v);
>
>  #endif /* __XEN_PCI_H__ */
> --
> 1.7.1
>
> Thanks,
> Feng
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Friday, November 22, 2013 6:49 PM
> To: Wu, Feng
> Cc: Jan Beulich (JBeulich@suse.com); xen-devel@lists.xen.org; Auld, Will;
> Nakajima, Jun; Zhang, Xiantao
> Subject: Re: [Xen-devel] [PATCH v4] x86: properly handle MSI-X unmask
> operation from guests
> 
> On 22/11/13 01:08, Wu, Feng wrote:
> > patch revision history
> > ----------------------
> > v1: Initial patch to handle this issue involving changing the
hypercall interface
> > v2:Totally handled inside hypervisor.
> > v3:Change some logics of handling msi-x pending unmask operations.
> > v4:Some changes related to coding style according to Andrew
Cooper''s
> comments
> >
> > From 51c5b7f1f9c8f319da8adf021b39e18fbd3bf314 Mon Sep 17 00:00:00
> 2001
> > From: Feng Wu <feng.wu@intel.com>
> > Date: Wed, 13 Nov 2013 21:43:48 -0500
> > Subject: [PATCH] x86: properly handle MSI-X unmask operation from
guests
> >
> > For a pass-through device with MSI-x capability, when guest tries
> > to unmask the MSI-x interrupt for the passed through device, xen
> > doesn''t clear the mask bit for MSI-x in hardware in the
following
> > scenario, which will cause network disconnection:
> >
> > 1. Guest masks the MSI-x interrupt
> > 2. Guest updates the address and data for it
> > 3. Guest unmasks the MSI-x interrupt (This is the problematic step)
> >
> > In the step #3 above, Xen doesn''t handle it well. When guest
tries
> > to unmask MSI-X interrupt, it traps to Xen, Xen just returns to Qemu
> > if it notices that address or data has been modified by guest before,
> > then Qemu will update Xen with the latest value of address/data by
> > hypercall. However, in this whole process, the MSI-X interrupt unmask
> > operation is missing, which means Xen doesn''t clear the mask
bit in
> > hardware for the MSI-X interrupt, so it remains disabled, that is why
> > it loses the network connection.
> >
> > This patch fixes this issue.
> >
> > Signed-off-by: Feng Wu <feng.wu@intel.com>
> > ---
> >  xen/arch/x86/hvm/io.c        |    3 +++
> >  xen/arch/x86/hvm/vmsi.c      |   18 ++++++++++++++++++
> >  xen/include/asm-x86/domain.h |    8 ++++++++
> >  xen/include/xen/pci.h        |    1 +
> >  4 files changed, 30 insertions(+), 0 deletions(-)
> >
> > diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c
> > index deb7b92..6c83c25 100644
> > --- a/xen/arch/x86/hvm/io.c
> > +++ b/xen/arch/x86/hvm/io.c
> > @@ -297,6 +297,9 @@ void hvm_io_assist(ioreq_t *p)
> >          break;
> >      }
> >
> > +    if ( msix_post_handler(curr) )
> > +        gdprintk(XENLOG_ERR, "msix_post_handler error\n");
> > +
> >      if ( p->state == STATE_IOREQ_NONE )
> >          vcpu_end_shutdown_deferral(curr);
> >  }
> > diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
> > index 4826b4a..2cdd0dc 100644
> > --- a/xen/arch/x86/hvm/vmsi.c
> > +++ b/xen/arch/x86/hvm/vmsi.c
> > @@ -293,7 +293,11 @@ static int msixtbl_write(struct vcpu *v, unsigned
> long address,
> >
> >      /* exit to device model if address/data has been modified */
> >      if ( test_and_clear_bit(nr_entry, &entry->table_flags) )
> > +    {
> > +        v->arch.pending_msix_unmask.valid = 1;
> > +        v->arch.pending_msix_unmask.ctrl_address = address;
> >          goto out;
> > +    }
> >
> >      virt = msixtbl_addr_to_virt(entry, address);
> >      if ( !virt )
> > @@ -528,3 +532,17 @@ void msixtbl_pt_cleanup(struct domain *d)
> >      spin_unlock(&d->arch.hvm_domain.msixtbl_list_lock);
> >      local_irq_restore(flags);
> >  }
> > +
> > +int msix_post_handler(struct vcpu *v)
> > +{
> > +    int rc;
> > +
> > +    if ( v->arch.pending_msix_unmask.valid == 0 )
> > +        return 0;
> > +
> > +    v->arch.pending_msix_unmask.valid = 0;
> > +
> > +    rc = msixtbl_write(v,
v->arch.pending_msix_unmask.ctrl_address, 4, 0);
> 
> Wont this corrupt other information which might be present in the register
?
It will not corrupt other information in the register. In fact, no matter what
value is passed to msixtbl_write(),
it only modifies the mask bit (bit 0) in hardware, it reads the other bits and
writes them back.
> 
> ~Andrew
> 
> > +    return rc != X86EMUL_OKAY ? -1 : 0;
> > +}
> > +
> > diff --git a/xen/include/asm-x86/domain.h
b/xen/include/asm-x86/domain.h
> > index 9d39061..89b1adc 100644
> > --- a/xen/include/asm-x86/domain.h
> > +++ b/xen/include/asm-x86/domain.h
> > @@ -375,6 +375,12 @@ struct pv_vcpu
> >      spinlock_t shadow_ldt_lock;
> >  };
> >
> > +struct pending_msix_unmask_info
> > +{
> > +    unsigned long ctrl_address;
> > +    bool_t valid;
> > +};
> > +
> >  struct arch_vcpu
> >  {
> >      /*
> > @@ -439,6 +445,8 @@ struct arch_vcpu
> >
> >      /* A secondary copy of the vcpu time info. */
> >      XEN_GUEST_HANDLE(vcpu_time_info_t) time_info_guest;
> > +
> > +    struct pending_msix_unmask_info pending_msix_unmask;
> >  } __cacheline_aligned;
> >
> >  /* Shorthands to improve code legibility. */
> > diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
> > index cadb525..ce8f6ff 100644
> > --- a/xen/include/xen/pci.h
> > +++ b/xen/include/xen/pci.h
> > @@ -147,5 +147,6 @@ struct pirq;
> >  int msixtbl_pt_register(struct domain *, struct pirq *, uint64_t
gtable);
> >  void msixtbl_pt_unregister(struct domain *, struct pirq *);
> >  void msixtbl_pt_cleanup(struct domain *d);
> > +int msix_post_handler(struct vcpu *v);
> >
> >  #endif /* __XEN_PCI_H__ */
> > --
> > 1.7.1
> >
> > Thanks,
> > Feng
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
Thanks,
Feng

On 22/11/13 13:39, Wu, Feng wrote:
>
>> -----Original Message-----
>> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
>> Sent: Friday, November 22, 2013 6:49 PM
>> To: Wu, Feng
>> Cc: Jan Beulich (JBeulich@suse.com); xen-devel@lists.xen.org; Auld,
Will;
>> Nakajima, Jun; Zhang, Xiantao
>> Subject: Re: [Xen-devel] [PATCH v4] x86: properly handle MSI-X unmask
>> operation from guests
>>
>> On 22/11/13 01:08, Wu, Feng wrote:
>>> patch revision history
>>> ----------------------
>>> v1: Initial patch to handle this issue involving changing the
hypercall interface
>>> v2:Totally handled inside hypervisor.
>>> v3:Change some logics of handling msi-x pending unmask operations.
>>> v4:Some changes related to coding style according to Andrew
Cooper''s
>> comments
>>> From 51c5b7f1f9c8f319da8adf021b39e18fbd3bf314 Mon Sep 17 00:00:00
>> 2001
>>> From: Feng Wu <feng.wu@intel.com>
>>> Date: Wed, 13 Nov 2013 21:43:48 -0500
>>> Subject: [PATCH] x86: properly handle MSI-X unmask operation from
guests
>>>
>>> For a pass-through device with MSI-x capability, when guest tries
>>> to unmask the MSI-x interrupt for the passed through device, xen
>>> doesn''t clear the mask bit for MSI-x in hardware in the
following
>>> scenario, which will cause network disconnection:
>>>
>>> 1. Guest masks the MSI-x interrupt
>>> 2. Guest updates the address and data for it
>>> 3. Guest unmasks the MSI-x interrupt (This is the problematic step)
>>>
>>> In the step #3 above, Xen doesn''t handle it well. When
guest tries
>>> to unmask MSI-X interrupt, it traps to Xen, Xen just returns to
Qemu
>>> if it notices that address or data has been modified by guest
before,
>>> then Qemu will update Xen with the latest value of address/data by
>>> hypercall. However, in this whole process, the MSI-X interrupt
unmask
>>> operation is missing, which means Xen doesn''t clear the
mask bit in
>>> hardware for the MSI-X interrupt, so it remains disabled, that is
why
>>> it loses the network connection.
>>>
>>> This patch fixes this issue.
>>>
>>> Signed-off-by: Feng Wu <feng.wu@intel.com>
>>> ---
>>>  xen/arch/x86/hvm/io.c        |    3 +++
>>>  xen/arch/x86/hvm/vmsi.c      |   18 ++++++++++++++++++
>>>  xen/include/asm-x86/domain.h |    8 ++++++++
>>>  xen/include/xen/pci.h        |    1 +
>>>  4 files changed, 30 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c
>>> index deb7b92..6c83c25 100644
>>> --- a/xen/arch/x86/hvm/io.c
>>> +++ b/xen/arch/x86/hvm/io.c
>>> @@ -297,6 +297,9 @@ void hvm_io_assist(ioreq_t *p)
>>>          break;
>>>      }
>>>
>>> +    if ( msix_post_handler(curr) )
>>> +        gdprintk(XENLOG_ERR, "msix_post_handler
error\n");
>>> +
>>>      if ( p->state == STATE_IOREQ_NONE )
>>>          vcpu_end_shutdown_deferral(curr);
>>>  }
>>> diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
>>> index 4826b4a..2cdd0dc 100644
>>> --- a/xen/arch/x86/hvm/vmsi.c
>>> +++ b/xen/arch/x86/hvm/vmsi.c
>>> @@ -293,7 +293,11 @@ static int msixtbl_write(struct vcpu *v,
unsigned
>> long address,
>>>      /* exit to device model if address/data has been modified */
>>>      if ( test_and_clear_bit(nr_entry, &entry->table_flags)
)
>>> +    {
>>> +        v->arch.pending_msix_unmask.valid = 1;
>>> +        v->arch.pending_msix_unmask.ctrl_address = address;
>>>          goto out;
>>> +    }
>>>
>>>      virt = msixtbl_addr_to_virt(entry, address);
>>>      if ( !virt )
>>> @@ -528,3 +532,17 @@ void msixtbl_pt_cleanup(struct domain *d)
>>>      spin_unlock(&d->arch.hvm_domain.msixtbl_list_lock);
>>>      local_irq_restore(flags);
>>>  }
>>> +
>>> +int msix_post_handler(struct vcpu *v)
>>> +{
>>> +    int rc;
>>> +
>>> +    if ( v->arch.pending_msix_unmask.valid == 0 )
>>> +        return 0;
>>> +
>>> +    v->arch.pending_msix_unmask.valid = 0;
>>> +
>>> +    rc = msixtbl_write(v,
v->arch.pending_msix_unmask.ctrl_address, 4, 0);
>> Wont this corrupt other information which might be present in the
register ?
> It will not corrupt other information in the register. In fact, no matter
what value is passed to msixtbl_write(),
> it only modifies the mask bit (bit 0) in hardware, it reads the other bits
and writes them back.
But it will result in corruption iff any other bits in the word become
defined by the spec.

I will defer to others as to whether that should count against this
patch or not.

~Andrew
>
>> ~Andrew
>>
>>> +    return rc != X86EMUL_OKAY ? -1 : 0;
>>> +}
>>> +
>>> diff --git a/xen/include/asm-x86/domain.h
b/xen/include/asm-x86/domain.h
>>> index 9d39061..89b1adc 100644
>>> --- a/xen/include/asm-x86/domain.h
>>> +++ b/xen/include/asm-x86/domain.h
>>> @@ -375,6 +375,12 @@ struct pv_vcpu
>>>      spinlock_t shadow_ldt_lock;
>>>  };
>>>
>>> +struct pending_msix_unmask_info
>>> +{
>>> +    unsigned long ctrl_address;
>>> +    bool_t valid;
>>> +};
>>> +
>>>  struct arch_vcpu
>>>  {
>>>      /*
>>> @@ -439,6 +445,8 @@ struct arch_vcpu
>>>
>>>      /* A secondary copy of the vcpu time info. */
>>>      XEN_GUEST_HANDLE(vcpu_time_info_t) time_info_guest;
>>> +
>>> +    struct pending_msix_unmask_info pending_msix_unmask;
>>>  } __cacheline_aligned;
>>>
>>>  /* Shorthands to improve code legibility. */
>>> diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
>>> index cadb525..ce8f6ff 100644
>>> --- a/xen/include/xen/pci.h
>>> +++ b/xen/include/xen/pci.h
>>> @@ -147,5 +147,6 @@ struct pirq;
>>>  int msixtbl_pt_register(struct domain *, struct pirq *, uint64_t
gtable);
>>>  void msixtbl_pt_unregister(struct domain *, struct pirq *);
>>>  void msixtbl_pt_cleanup(struct domain *d);
>>> +int msix_post_handler(struct vcpu *v);
>>>
>>>  #endif /* __XEN_PCI_H__ */
>>> --
>>> 1.7.1
>>>
>>> Thanks,
>>> Feng
>>>
>>> _______________________________________________
>>> Xen-devel mailing list
>>> Xen-devel@lists.xen.org
>>> http://lists.xen.org/xen-devel
> Thanks,
> Feng

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Friday, November 22, 2013 9:52 PM
> To: Wu, Feng
> Cc: Jan Beulich (JBeulich@suse.com); xen-devel@lists.xen.org; Auld, Will;
> Nakajima, Jun; Zhang, Xiantao
> Subject: Re: [Xen-devel] [PATCH v4] x86: properly handle MSI-X unmask
> operation from guests
> 
> On 22/11/13 13:39, Wu, Feng wrote:
> >
> >> -----Original Message-----
> >> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> >> Sent: Friday, November 22, 2013 6:49 PM
> >> To: Wu, Feng
> >> Cc: Jan Beulich (JBeulich@suse.com); xen-devel@lists.xen.org;
Auld, Will;
> >> Nakajima, Jun; Zhang, Xiantao
> >> Subject: Re: [Xen-devel] [PATCH v4] x86: properly handle MSI-X
unmask
> >> operation from guests
> >>
> >> On 22/11/13 01:08, Wu, Feng wrote:
> >>> patch revision history
> >>> ----------------------
> >>> v1: Initial patch to handle this issue involving changing the
hypercall
> interface
> >>> v2:Totally handled inside hypervisor.
> >>> v3:Change some logics of handling msi-x pending unmask
operations.
> >>> v4:Some changes related to coding style according to Andrew
Cooper''s
> >> comments
> >>> From 51c5b7f1f9c8f319da8adf021b39e18fbd3bf314 Mon Sep 17
00:00:00
> >> 2001
> >>> From: Feng Wu <feng.wu@intel.com>
> >>> Date: Wed, 13 Nov 2013 21:43:48 -0500
> >>> Subject: [PATCH] x86: properly handle MSI-X unmask operation
from guests
> >>>
> >>> For a pass-through device with MSI-x capability, when guest
tries
> >>> to unmask the MSI-x interrupt for the passed through device,
xen
> >>> doesn''t clear the mask bit for MSI-x in hardware in
the following
> >>> scenario, which will cause network disconnection:
> >>>
> >>> 1. Guest masks the MSI-x interrupt
> >>> 2. Guest updates the address and data for it
> >>> 3. Guest unmasks the MSI-x interrupt (This is the problematic
step)
> >>>
> >>> In the step #3 above, Xen doesn''t handle it well.
When guest tries
> >>> to unmask MSI-X interrupt, it traps to Xen, Xen just returns
to Qemu
> >>> if it notices that address or data has been modified by guest
before,
> >>> then Qemu will update Xen with the latest value of
address/data by
> >>> hypercall. However, in this whole process, the MSI-X interrupt
unmask
> >>> operation is missing, which means Xen doesn''t clear
the mask bit in
> >>> hardware for the MSI-X interrupt, so it remains disabled, that
is why
> >>> it loses the network connection.
> >>>
> >>> This patch fixes this issue.
> >>>
> >>> Signed-off-by: Feng Wu <feng.wu@intel.com>
> >>> ---
> >>>  xen/arch/x86/hvm/io.c        |    3 +++
> >>>  xen/arch/x86/hvm/vmsi.c      |   18 ++++++++++++++++++
> >>>  xen/include/asm-x86/domain.h |    8 ++++++++
> >>>  xen/include/xen/pci.h        |    1 +
> >>>  4 files changed, 30 insertions(+), 0 deletions(-)
> >>>
> >>> diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c
> >>> index deb7b92..6c83c25 100644
> >>> --- a/xen/arch/x86/hvm/io.c
> >>> +++ b/xen/arch/x86/hvm/io.c
> >>> @@ -297,6 +297,9 @@ void hvm_io_assist(ioreq_t *p)
> >>>          break;
> >>>      }
> >>>
> >>> +    if ( msix_post_handler(curr) )
> >>> +        gdprintk(XENLOG_ERR, "msix_post_handler
error\n");
> >>> +
> >>>      if ( p->state == STATE_IOREQ_NONE )
> >>>          vcpu_end_shutdown_deferral(curr);
> >>>  }
> >>> diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
> >>> index 4826b4a..2cdd0dc 100644
> >>> --- a/xen/arch/x86/hvm/vmsi.c
> >>> +++ b/xen/arch/x86/hvm/vmsi.c
> >>> @@ -293,7 +293,11 @@ static int msixtbl_write(struct vcpu *v,
unsigned
> >> long address,
> >>>      /* exit to device model if address/data has been modified
*/
> >>>      if ( test_and_clear_bit(nr_entry,
&entry->table_flags) )
> >>> +    {
> >>> +        v->arch.pending_msix_unmask.valid = 1;
> >>> +        v->arch.pending_msix_unmask.ctrl_address =
address;
> >>>          goto out;
> >>> +    }
> >>>
> >>>      virt = msixtbl_addr_to_virt(entry, address);
> >>>      if ( !virt )
> >>> @@ -528,3 +532,17 @@ void msixtbl_pt_cleanup(struct domain *d)
> >>>     
spin_unlock(&d->arch.hvm_domain.msixtbl_list_lock);
> >>>      local_irq_restore(flags);
> >>>  }
> >>> +
> >>> +int msix_post_handler(struct vcpu *v)
> >>> +{
> >>> +    int rc;
> >>> +
> >>> +    if ( v->arch.pending_msix_unmask.valid == 0 )
> >>> +        return 0;
> >>> +
> >>> +    v->arch.pending_msix_unmask.valid = 0;
> >>> +
> >>> +    rc = msixtbl_write(v,
v->arch.pending_msix_unmask.ctrl_address, 4,
> 0);
> >> Wont this corrupt other information which might be present in the
register ?
> > It will not corrupt other information in the register. In fact, no
matter what
> value is passed to msixtbl_write(),
> > it only modifies the mask bit (bit 0) in hardware, it reads the other
bits and
> writes them back.
> 
> But it will result in corruption iff any other bits in the word become
> defined by the spec.
Here is the current logic to handle mask bit write in msixtbl_write():

    /*
     * The mask bit is the only defined bit in the word. But we
     * ought to preserve the reserved bits. Clearing the reserved
     * bits can result in undefined behaviour (see PCI Local Bus
     * Specification revision 2.3).
     */
    val &= PCI_MSIX_VECTOR_BITMASK;
    val |= (orig & ~PCI_MSIX_VECTOR_BITMASK);
    writel(val, virt);

Here it only changes the mask bit, orig is the original value of the control
word, seems it
will not corrupt other bits, just correct me if my understanding is wrong,
Thanks you!
> 
> I will defer to others as to whether that should count against this
> patch or not.
> 
> ~Andrew
> 
> >
> >> ~Andrew
> >>
> >>> +    return rc != X86EMUL_OKAY ? -1 : 0;
> >>> +}
> >>> +
> >>> diff --git a/xen/include/asm-x86/domain.h
b/xen/include/asm-x86/domain.h
> >>> index 9d39061..89b1adc 100644
> >>> --- a/xen/include/asm-x86/domain.h
> >>> +++ b/xen/include/asm-x86/domain.h
> >>> @@ -375,6 +375,12 @@ struct pv_vcpu
> >>>      spinlock_t shadow_ldt_lock;
> >>>  };
> >>>
> >>> +struct pending_msix_unmask_info
> >>> +{
> >>> +    unsigned long ctrl_address;
> >>> +    bool_t valid;
> >>> +};
> >>> +
> >>>  struct arch_vcpu
> >>>  {
> >>>      /*
> >>> @@ -439,6 +445,8 @@ struct arch_vcpu
> >>>
> >>>      /* A secondary copy of the vcpu time info. */
> >>>      XEN_GUEST_HANDLE(vcpu_time_info_t) time_info_guest;
> >>> +
> >>> +    struct pending_msix_unmask_info pending_msix_unmask;
> >>>  } __cacheline_aligned;
> >>>
> >>>  /* Shorthands to improve code legibility. */
> >>> diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
> >>> index cadb525..ce8f6ff 100644
> >>> --- a/xen/include/xen/pci.h
> >>> +++ b/xen/include/xen/pci.h
> >>> @@ -147,5 +147,6 @@ struct pirq;
> >>>  int msixtbl_pt_register(struct domain *, struct pirq *,
uint64_t gtable);
> >>>  void msixtbl_pt_unregister(struct domain *, struct pirq *);
> >>>  void msixtbl_pt_cleanup(struct domain *d);
> >>> +int msix_post_handler(struct vcpu *v);
> >>>
> >>>  #endif /* __XEN_PCI_H__ */
> >>> --
> >>> 1.7.1
> >>>
> >>> Thanks,
> >>> Feng
> >>>
> >>> _______________________________________________
> >>> Xen-devel mailing list
> >>> Xen-devel@lists.xen.org
> >>> http://lists.xen.org/xen-devel
> > Thanks,
> > Feng

>>> On 22.11.13 at 02:08, "Wu, Feng"
<feng.wu@intel.com> wrote:
> v1: Initial patch to handle this issue involving changing the hypercall
interface
> v2:Totally handled inside hypervisor.
> v3:Change some logics of handling msi-x pending unmask operations.
> v4:Some changes related to coding style according to Andrew
Cooper''s comments
So this is _much_ less intrusive than what you did before - good!
> --- a/xen/arch/x86/hvm/io.c
> +++ b/xen/arch/x86/hvm/io.c
> @@ -297,6 +297,9 @@ void hvm_io_assist(ioreq_t *p)
>          break;
>      }
> 
> +    if ( msix_post_handler(curr) )
> +        gdprintk(XENLOG_ERR, "msix_post_handler error\n");
> +
>      if ( p->state == STATE_IOREQ_NONE )
>          vcpu_end_shutdown_deferral(curr);
I think the addition should be moved into the body of this if(),
so that it gets executed only upon completion of I/O, not when
it e.g. need retrying.

Also, XENLOG_ERR seems to heavy a message. XENLOG_WARN
would be the highest I''d accept.
> +int msix_post_handler(struct vcpu *v)
> +{
> +    int rc;
> +
> +    if ( v->arch.pending_msix_unmask.valid == 0 )
Iff you keep this (see below), then boolean checks are
conventionally done with ! rather than == 0.
> +        return 0;
> +
> +    v->arch.pending_msix_unmask.valid = 0;
> +
> +    rc = msixtbl_write(v, v->arch.pending_msix_unmask.ctrl_address, 4,
0);
> +    return rc != X86EMUL_OKAY ? -1 : 0;
Make the function return bool_t, and then simply 

   return msixtbl_write(v, v->arch.pending_msix_unmask.ctrl_address, 4, 0) ==
X86EMUL_OKAY;
> +struct pending_msix_unmask_info
> +{
> +    unsigned long ctrl_address;
> +    bool_t valid;
> +};
> +
>  struct arch_vcpu
>  {
>      /*
> @@ -439,6 +445,8 @@ struct arch_vcpu
> 
>      /* A secondary copy of the vcpu time info. */
>      XEN_GUEST_HANDLE(vcpu_time_info_t) time_info_guest;
> +
> +    struct pending_msix_unmask_info pending_msix_unmask;
I don''t think you need a separate boolean here - for one I
don''t
think the address can reasonably be zero, and even if you have
the bottom two bits available (as it being 4-byte aligned gets
checked before you consume it).

Jan

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Saturday, November 23, 2013 12:19 AM
> To: Wu, Feng
> Cc: Nakajima, Jun; Auld, Will; Zhang, Xiantao; xen-devel
> Subject: Re: [PATCH v4] x86: properly handle MSI-X unmask operation from
> guests
> 
> >>> On 22.11.13 at 02:08, "Wu, Feng"
<feng.wu@intel.com> wrote:
> > v1: Initial patch to handle this issue involving changing the
hypercall interface
> > v2:Totally handled inside hypervisor.
> > v3:Change some logics of handling msi-x pending unmask operations.
> > v4:Some changes related to coding style according to Andrew
Cooper''s
> comments
> 
> So this is _much_ less intrusive than what you did before - good!
> 
> > --- a/xen/arch/x86/hvm/io.c
> > +++ b/xen/arch/x86/hvm/io.c
> > @@ -297,6 +297,9 @@ void hvm_io_assist(ioreq_t *p)
> >          break;
> >      }
> >
> > +    if ( msix_post_handler(curr) )
> > +        gdprintk(XENLOG_ERR, "msix_post_handler error\n");
> > +
> >      if ( p->state == STATE_IOREQ_NONE )
> >          vcpu_end_shutdown_deferral(curr);
> 
> I think the addition should be moved into the body of this if(),
> so that it gets executed only upon completion of I/O, not when
> it e.g. need retrying.
> 
> Also, XENLOG_ERR seems to heavy a message. XENLOG_WARN
> would be the highest I''d accept.
> 
> > +int msix_post_handler(struct vcpu *v)
> > +{
> > +    int rc;
> > +
> > +    if ( v->arch.pending_msix_unmask.valid == 0 )
> 
> Iff you keep this (see below), then boolean checks are
> conventionally done with ! rather than == 0.
> 
> > +        return 0;
> > +
> > +    v->arch.pending_msix_unmask.valid = 0;
> > +
> > +    rc = msixtbl_write(v,
v->arch.pending_msix_unmask.ctrl_address, 4, 0);
> > +    return rc != X86EMUL_OKAY ? -1 : 0;
> 
> Make the function return bool_t, and then simply
> 
>    return msixtbl_write(v, v->arch.pending_msix_unmask.ctrl_address, 4,
0) => X86EMUL_OKAY;
> 
> > +struct pending_msix_unmask_info
> > +{
> > +    unsigned long ctrl_address;
> > +    bool_t valid;
> > +};
> > +
> >  struct arch_vcpu
> >  {
> >      /*
> > @@ -439,6 +445,8 @@ struct arch_vcpu
> >
> >      /* A secondary copy of the vcpu time info. */
> >      XEN_GUEST_HANDLE(vcpu_time_info_t) time_info_guest;
> > +
> > +    struct pending_msix_unmask_info pending_msix_unmask;
> 
> I don''t think you need a separate boolean here - for one I
don''t
> think the address can reasonably be zero, and even if you have
> the bottom two bits available (as it being 4-byte aligned gets
> checked before you consume it).

The boolean variant "valid", which is set in msixtbl_write(), means
whether there is a
msix pending unmask, if there is, just clean this flag and unmask the msix in
hardware,
otherwise we just do nothing. If removing "valid", how can I know
whether there is a
msix pending unmask operation ? Thanks you!
> 
> Jan

On 23/11/13 01:40, Wu, Feng wrote:
>
>> -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Saturday, November 23, 2013 12:19 AM
>> To: Wu, Feng
>> Cc: Nakajima, Jun; Auld, Will; Zhang, Xiantao; xen-devel
>> Subject: Re: [PATCH v4] x86: properly handle MSI-X unmask operation
from
>> guests
>>
>>>>> On 22.11.13 at 02:08, "Wu, Feng"
<feng.wu@intel.com> wrote:
>>> v1: Initial patch to handle this issue involving changing the
hypercall interface
>>> v2:Totally handled inside hypervisor.
>>> v3:Change some logics of handling msi-x pending unmask operations.
>>> v4:Some changes related to coding style according to Andrew
Cooper''s
>> comments
>>
>> So this is _much_ less intrusive than what you did before - good!
>>
>>> --- a/xen/arch/x86/hvm/io.c
>>> +++ b/xen/arch/x86/hvm/io.c
>>> @@ -297,6 +297,9 @@ void hvm_io_assist(ioreq_t *p)
>>>          break;
>>>      }
>>>
>>> +    if ( msix_post_handler(curr) )
>>> +        gdprintk(XENLOG_ERR, "msix_post_handler
error\n");
>>> +
>>>      if ( p->state == STATE_IOREQ_NONE )
>>>          vcpu_end_shutdown_deferral(curr);
>> I think the addition should be moved into the body of this if(),
>> so that it gets executed only upon completion of I/O, not when
>> it e.g. need retrying.
>>
>> Also, XENLOG_ERR seems to heavy a message. XENLOG_WARN
>> would be the highest I''d accept.
>>
>>> +int msix_post_handler(struct vcpu *v)
>>> +{
>>> +    int rc;
>>> +
>>> +    if ( v->arch.pending_msix_unmask.valid == 0 )
>> Iff you keep this (see below), then boolean checks are
>> conventionally done with ! rather than == 0.
>>
>>> +        return 0;
>>> +
>>> +    v->arch.pending_msix_unmask.valid = 0;
>>> +
>>> +    rc = msixtbl_write(v,
v->arch.pending_msix_unmask.ctrl_address, 4, 0);
>>> +    return rc != X86EMUL_OKAY ? -1 : 0;
>> Make the function return bool_t, and then simply
>>
>>    return msixtbl_write(v, v->arch.pending_msix_unmask.ctrl_address,
4, 0) =>> X86EMUL_OKAY;
>>
>>> +struct pending_msix_unmask_info
>>> +{
>>> +    unsigned long ctrl_address;
>>> +    bool_t valid;
>>> +};
>>> +
>>>  struct arch_vcpu
>>>  {
>>>      /*
>>> @@ -439,6 +445,8 @@ struct arch_vcpu
>>>
>>>      /* A secondary copy of the vcpu time info. */
>>>      XEN_GUEST_HANDLE(vcpu_time_info_t) time_info_guest;
>>> +
>>> +    struct pending_msix_unmask_info pending_msix_unmask;
>> I don''t think you need a separate boolean here - for one I
don''t
>> think the address can reasonably be zero, and even if you have
>> the bottom two bits available (as it being 4-byte aligned gets
>> checked before you consume it).
>
> The boolean variant "valid", which is set in msixtbl_write(),
means whether there is a
> msix pending unmask, if there is, just clean this flag and unmask the msix
in hardware,
> otherwise we just do nothing. If removing "valid", how can I know
whether there is a
> msix pending unmask operation ? Thanks you!
Address can''t reasonably be 0, meaning that an address of 0 indicates
that an unmask is not pending.

~Andrew

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Sunday, November 24, 2013 12:20 AM
> To: Wu, Feng
> Cc: Jan Beulich; xen-devel; Auld, Will; Zhang, Xiantao; Nakajima, Jun
> Subject: Re: [Xen-devel] [PATCH v4] x86: properly handle MSI-X unmask
> operation from guests
> 
> On 23/11/13 01:40, Wu, Feng wrote:
> >
> >> -----Original Message-----
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: Saturday, November 23, 2013 12:19 AM
> >> To: Wu, Feng
> >> Cc: Nakajima, Jun; Auld, Will; Zhang, Xiantao; xen-devel
> >> Subject: Re: [PATCH v4] x86: properly handle MSI-X unmask
operation from
> >> guests
> >>
> >>>>> On 22.11.13 at 02:08, "Wu, Feng"
<feng.wu@intel.com> wrote:
> >>> v1: Initial patch to handle this issue involving changing the
hypercall
> interface
> >>> v2:Totally handled inside hypervisor.
> >>> v3:Change some logics of handling msi-x pending unmask
operations.
> >>> v4:Some changes related to coding style according to Andrew
Cooper''s
> >> comments
> >>
> >> So this is _much_ less intrusive than what you did before - good!
> >>
> >>> --- a/xen/arch/x86/hvm/io.c
> >>> +++ b/xen/arch/x86/hvm/io.c
> >>> @@ -297,6 +297,9 @@ void hvm_io_assist(ioreq_t *p)
> >>>          break;
> >>>      }
> >>>
> >>> +    if ( msix_post_handler(curr) )
> >>> +        gdprintk(XENLOG_ERR, "msix_post_handler
error\n");
> >>> +
> >>>      if ( p->state == STATE_IOREQ_NONE )
> >>>          vcpu_end_shutdown_deferral(curr);
> >> I think the addition should be moved into the body of this if(),
> >> so that it gets executed only upon completion of I/O, not when
> >> it e.g. need retrying.
> >>
> >> Also, XENLOG_ERR seems to heavy a message. XENLOG_WARN
> >> would be the highest I''d accept.
> >>
> >>> +int msix_post_handler(struct vcpu *v)
> >>> +{
> >>> +    int rc;
> >>> +
> >>> +    if ( v->arch.pending_msix_unmask.valid == 0 )
> >> Iff you keep this (see below), then boolean checks are
> >> conventionally done with ! rather than == 0.
> >>
> >>> +        return 0;
> >>> +
> >>> +    v->arch.pending_msix_unmask.valid = 0;
> >>> +
> >>> +    rc = msixtbl_write(v,
v->arch.pending_msix_unmask.ctrl_address, 4,
> 0);
> >>> +    return rc != X86EMUL_OKAY ? -1 : 0;
> >> Make the function return bool_t, and then simply
> >>
> >>    return msixtbl_write(v,
v->arch.pending_msix_unmask.ctrl_address, 4,
> 0) => >> X86EMUL_OKAY;
> >>
> >>> +struct pending_msix_unmask_info
> >>> +{
> >>> +    unsigned long ctrl_address;
> >>> +    bool_t valid;
> >>> +};
> >>> +
> >>>  struct arch_vcpu
> >>>  {
> >>>      /*
> >>> @@ -439,6 +445,8 @@ struct arch_vcpu
> >>>
> >>>      /* A secondary copy of the vcpu time info. */
> >>>      XEN_GUEST_HANDLE(vcpu_time_info_t) time_info_guest;
> >>> +
> >>> +    struct pending_msix_unmask_info pending_msix_unmask;
> >> I don''t think you need a separate boolean here - for one
I don''t
> >> think the address can reasonably be zero, and even if you have
> >> the bottom two bits available (as it being 4-byte aligned gets
> >> checked before you consume it).
> >
> > The boolean variant "valid", which is set in
msixtbl_write(), means whether
> there is a
> > msix pending unmask, if there is, just clean this flag and unmask the
msix in
> hardware,
> > otherwise we just do nothing. If removing "valid", how can I
know whether
> there is a
> > msix pending unmask operation ? Thanks you!
> 
> Address can''t reasonably be 0, meaning that an address of 0
indicates
> that an unmask is not pending.
In that case, we should make v->arch.pending_msix_unmask.ctrl_address 0 in
msix_post_handler() after unmasking the msix with this address, right?
> 
> ~Andrew

>>> On 23.11.13 at 02:40, "Wu, Feng"
<feng.wu@intel.com> wrote:
>> > +struct pending_msix_unmask_info
>> > +{
>> > +    unsigned long ctrl_address;
>> > +    bool_t valid;
>> > +};
>> > +
>> >  struct arch_vcpu
>> >  {
>> >      /*
>> > @@ -439,6 +445,8 @@ struct arch_vcpu
>> >
>> >      /* A secondary copy of the vcpu time info. */
>> >      XEN_GUEST_HANDLE(vcpu_time_info_t) time_info_guest;
>> > +
>> > +    struct pending_msix_unmask_info pending_msix_unmask;
>> 
>> I don''t think you need a separate boolean here - for one I
don''t
>> think the address can reasonably be zero, and even if you have
>> the bottom two bits available (as it being 4-byte aligned gets
>> checked before you consume it).
> 
> 
> The boolean variant "valid", which is set in msixtbl_write(),
means whether
> there is a
> msix pending unmask, if there is, just clean this flag and unmask the msix 
> in hardware,
> otherwise we just do nothing. If removing "valid", how can I know
whether
> there is a
> msix pending unmask operation ? Thanks you!
Quoting your patch:

@@ -293,7 +293,11 @@ static int msixtbl_write(struct vcpu *v, unsigned long
address,

     /* exit to device model if address/data has been modified */
     if ( test_and_clear_bit(nr_entry, &entry->table_flags) )
+    {
+        v->arch.pending_msix_unmask.valid = 1;
+        v->arch.pending_msix_unmask.ctrl_address = address;
         goto out;
+    }

     virt = msixtbl_addr_to_virt(entry, address);
     if ( !virt )

You always set "ctrl_address" together with "valid". Hence,
as long
as "ctrl_address" is guaranteed to be non-zero (either a apriori
knowledge, or by setting one of the low to bits, which aren''t used
for other purposes), you can then check "ctrl_address" to be non-
zero instead of checking "valid".

Jan

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Monday, November 25, 2013 5:30 PM
> To: Wu, Feng
> Cc: Nakajima, Jun; Auld, Will; Zhang, Xiantao; xen-devel
> Subject: RE: [PATCH v4] x86: properly handle MSI-X unmask operation from
> guests
> 
> >>> On 23.11.13 at 02:40, "Wu, Feng"
<feng.wu@intel.com> wrote:
> >> > +struct pending_msix_unmask_info
> >> > +{
> >> > +    unsigned long ctrl_address;
> >> > +    bool_t valid;
> >> > +};
> >> > +
> >> >  struct arch_vcpu
> >> >  {
> >> >      /*
> >> > @@ -439,6 +445,8 @@ struct arch_vcpu
> >> >
> >> >      /* A secondary copy of the vcpu time info. */
> >> >      XEN_GUEST_HANDLE(vcpu_time_info_t) time_info_guest;
> >> > +
> >> > +    struct pending_msix_unmask_info pending_msix_unmask;
> >>
> >> I don''t think you need a separate boolean here - for one
I don''t
> >> think the address can reasonably be zero, and even if you have
> >> the bottom two bits available (as it being 4-byte aligned gets
> >> checked before you consume it).
> >
> >
> > The boolean variant "valid", which is set in
msixtbl_write(), means whether
> > there is a
> > msix pending unmask, if there is, just clean this flag and unmask the
msix
> > in hardware,
> > otherwise we just do nothing. If removing "valid", how can I
know whether
> > there is a
> > msix pending unmask operation ? Thanks you!
> 
> Quoting your patch:
> 
> @@ -293,7 +293,11 @@ static int msixtbl_write(struct vcpu *v, unsigned long
> address,
> 
>      /* exit to device model if address/data has been modified */
>      if ( test_and_clear_bit(nr_entry, &entry->table_flags) )
> +    {
> +        v->arch.pending_msix_unmask.valid = 1;
> +        v->arch.pending_msix_unmask.ctrl_address = address;
>          goto out;
> +    }
> 
>      virt = msixtbl_addr_to_virt(entry, address);
>      if ( !virt )
> 
> You always set "ctrl_address" together with "valid".
Hence, as long
> as "ctrl_address" is guaranteed to be non-zero (either a apriori
> knowledge, or by setting one of the low to bits, which aren''t used
> for other purposes), you can then check "ctrl_address" to be non-
> zero instead of checking "valid".
Yes, this is a good idea. Thanks for the clear explanation!
> 
> Jan