[PATCH] x86/vMSI-X: avoid missing first unmask of vectors

[PATCH] x86/vMSI-X: avoid missing first unmask of vectors

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 3512 bytes --]

Recent changes to Linux result in there just being a single unmask
operation prior to expecting the first interrupts to arrive. However,
we've had a chicken-and-egg problem here: Qemu invokes
xc_domain_update_msi_irq(), ultimately leading to
msixtbl_pt_register(), upon seeing that first unmask operation. Yet
for msixtbl_range() to return true (in order to msixtbl_write() to get
invoked at all) msixtbl_pt_register() must have completed.

Deal with this by snooping suitable writes in msixtbl_range() and
triggering the invocation of msix_write_completion() from
msixtbl_pt_register() when that happens in the context of a still in
progress vector control field write.

Note that the seemingly unrelated deletion of the redundant
irq_desc->msi_desc checks in msixtbl_pt_register() is to make clear to
any compiler version used that the "msi_desc" local variable isn't
being used uninitialized. (Doing the same in msixtbl_pt_unregister() is
just for consistency reasons.)

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
TODO: How to deal with REP MOVS to the MSI-X table (in msixtbl_range())?

--- a/xen/arch/x86/hvm/vmsi.c
+++ b/xen/arch/x86/hvm/vmsi.c
@@ -341,7 +352,21 @@ static int msixtbl_range(struct vcpu *v,
     desc = msixtbl_addr_to_desc(msixtbl_find_entry(v, addr), addr);
     rcu_read_unlock(&msixtbl_rcu_lock);
 
-    return !!desc;
+    if ( desc )
+        return 1;
+
+    if ( (addr & (PCI_MSIX_ENTRY_SIZE - 1)) ==
+         PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET )
+    {
+        const ioreq_t *r = &v->arch.hvm_vcpu.hvm_io.io_req;
+
+        ASSERT(r->type == IOREQ_TYPE_COPY);
+        if ( r->dir == IOREQ_WRITE && r->size == 4 && !r->data_is_ptr
+             && !(r->data & PCI_MSIX_VECTOR_BITMASK) )
+            v->arch.hvm_vcpu.hvm_io.msix_snoop_address = addr;
+    }
+
+    return 0;
 }
 
 static const struct hvm_mmio_ops msixtbl_mmio_ops = {
@@ -410,9 +434,6 @@ int msixtbl_pt_register(struct domain *d
         return r;
     }
 
-    if ( !irq_desc->msi_desc )
-        goto out;
-
     msi_desc = irq_desc->msi_desc;
     if ( !msi_desc )
         goto out;
@@ -437,6 +458,21 @@ found:
 out:
     spin_unlock_irq(&irq_desc->lock);
     xfree(new_entry);
+
+    if ( !r )
+    {
+        struct vcpu *v;
+
+        for_each_vcpu ( d, v )
+        {
+            if ( v->arch.hvm_vcpu.hvm_io.msix_snoop_address ==
+                 (gtable + msi_desc->msi_attrib.entry_nr * PCI_MSIX_ENTRY_SIZE +
+                  PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET) )
+                v->arch.hvm_vcpu.hvm_io.msix_unmask_address =
+                    v->arch.hvm_vcpu.hvm_io.msix_snoop_address;
+        }
+    }
+
     return r;
 }
 
@@ -457,9 +496,6 @@ void msixtbl_pt_unregister(struct domain
     if ( !irq_desc )
         return;
 
-    if ( !irq_desc->msi_desc )
-        goto out;
-
     msi_desc = irq_desc->msi_desc;
     if ( !msi_desc )
         goto out;
@@ -522,6 +558,8 @@ void msix_write_completion(struct vcpu *
 {
     unsigned long ctrl_address = v->arch.hvm_vcpu.hvm_io.msix_unmask_address;
 
+    v->arch.hvm_vcpu.hvm_io.msix_snoop_address = 0;
+
     if ( !ctrl_address )
         return;
 
--- a/xen/include/asm-x86/hvm/vcpu.h
+++ b/xen/include/asm-x86/hvm/vcpu.h
@@ -85,6 +85,7 @@ struct hvm_vcpu_io {
     bool_t mmio_retry;
 
     unsigned long msix_unmask_address;
+    unsigned long msix_snoop_address;
 
     const struct g2m_ioport *g2m_ioport;
 };




[-- Attachment #2: x86-vMSI-X-first-unmask.patch --]
[-- Type: text/plain, Size: 3559 bytes --]

x86/vMSI-X: avoid missing first unmask of vectors

Recent changes to Linux result in there just being a single unmask
operation prior to expecting the first interrupts to arrive. However,
we've had a chicken-and-egg problem here: Qemu invokes
xc_domain_update_msi_irq(), ultimately leading to
msixtbl_pt_register(), upon seeing that first unmask operation. Yet
for msixtbl_range() to return true (in order to msixtbl_write() to get
invoked at all) msixtbl_pt_register() must have completed.

Deal with this by snooping suitable writes in msixtbl_range() and
triggering the invocation of msix_write_completion() from
msixtbl_pt_register() when that happens in the context of a still in
progress vector control field write.

Note that the seemingly unrelated deletion of the redundant
irq_desc->msi_desc checks in msixtbl_pt_register() is to make clear to
any compiler version used that the "msi_desc" local variable isn't
being used uninitialized. (Doing the same in msixtbl_pt_unregister() is
just for consistency reasons.)

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
TODO: How to deal with REP MOVS to the MSI-X table (in msixtbl_range())?

--- a/xen/arch/x86/hvm/vmsi.c
+++ b/xen/arch/x86/hvm/vmsi.c
@@ -341,7 +352,21 @@ static int msixtbl_range(struct vcpu *v,
     desc = msixtbl_addr_to_desc(msixtbl_find_entry(v, addr), addr);
     rcu_read_unlock(&msixtbl_rcu_lock);
 
-    return !!desc;
+    if ( desc )
+        return 1;
+
+    if ( (addr & (PCI_MSIX_ENTRY_SIZE - 1)) ==
+         PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET )
+    {
+        const ioreq_t *r = &v->arch.hvm_vcpu.hvm_io.io_req;
+
+        ASSERT(r->type == IOREQ_TYPE_COPY);
+        if ( r->dir == IOREQ_WRITE && r->size == 4 && !r->data_is_ptr
+             && !(r->data & PCI_MSIX_VECTOR_BITMASK) )
+            v->arch.hvm_vcpu.hvm_io.msix_snoop_address = addr;
+    }
+
+    return 0;
 }
 
 static const struct hvm_mmio_ops msixtbl_mmio_ops = {
@@ -410,9 +434,6 @@ int msixtbl_pt_register(struct domain *d
         return r;
     }
 
-    if ( !irq_desc->msi_desc )
-        goto out;
-
     msi_desc = irq_desc->msi_desc;
     if ( !msi_desc )
         goto out;
@@ -437,6 +458,21 @@ found:
 out:
     spin_unlock_irq(&irq_desc->lock);
     xfree(new_entry);
+
+    if ( !r )
+    {
+        struct vcpu *v;
+
+        for_each_vcpu ( d, v )
+        {
+            if ( v->arch.hvm_vcpu.hvm_io.msix_snoop_address ==
+                 (gtable + msi_desc->msi_attrib.entry_nr * PCI_MSIX_ENTRY_SIZE +
+                  PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET) )
+                v->arch.hvm_vcpu.hvm_io.msix_unmask_address =
+                    v->arch.hvm_vcpu.hvm_io.msix_snoop_address;
+        }
+    }
+
     return r;
 }
 
@@ -457,9 +496,6 @@ void msixtbl_pt_unregister(struct domain
     if ( !irq_desc )
         return;
 
-    if ( !irq_desc->msi_desc )
-        goto out;
-
     msi_desc = irq_desc->msi_desc;
     if ( !msi_desc )
         goto out;
@@ -522,6 +558,8 @@ void msix_write_completion(struct vcpu *
 {
     unsigned long ctrl_address = v->arch.hvm_vcpu.hvm_io.msix_unmask_address;
 
+    v->arch.hvm_vcpu.hvm_io.msix_snoop_address = 0;
+
     if ( !ctrl_address )
         return;
 
--- a/xen/include/asm-x86/hvm/vcpu.h
+++ b/xen/include/asm-x86/hvm/vcpu.h
@@ -85,6 +85,7 @@ struct hvm_vcpu_io {
     bool_t mmio_retry;
 
     unsigned long msix_unmask_address;
+    unsigned long msix_snoop_address;
 
     const struct g2m_ioport *g2m_ioport;
 };

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

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

Re: [PATCH] x86/vMSI-X: avoid missing first unmask of vectors

[Jan Beulich]

>>> On 21.04.16 at 11:38, <JBeulich@suse.com> wrote:
> Recent changes to Linux result in there just being a single unmask
> operation prior to expecting the first interrupts to arrive. However,
> we've had a chicken-and-egg problem here: Qemu invokes
> xc_domain_update_msi_irq(), ultimately leading to
> msixtbl_pt_register(), upon seeing that first unmask operation. Yet
> for msixtbl_range() to return true (in order to msixtbl_write() to get
> invoked at all) msixtbl_pt_register() must have completed.
> 
> Deal with this by snooping suitable writes in msixtbl_range() and
> triggering the invocation of msix_write_completion() from
> msixtbl_pt_register() when that happens in the context of a still in
> progress vector control field write.
> 
> Note that the seemingly unrelated deletion of the redundant
> irq_desc->msi_desc checks in msixtbl_pt_register() is to make clear to
> any compiler version used that the "msi_desc" local variable isn't
> being used uninitialized. (Doing the same in msixtbl_pt_unregister() is
> just for consistency reasons.)
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> TODO: How to deal with REP MOVS to the MSI-X table (in msixtbl_range())?

One option I just now thought of might be to also look at batches
of up to 4 dword or 2 qword writes there, tracking the address of
the one that writes to the vector control field. One might even
limit this to taking into consideration only the last iteration on such
batches, on the assumption that if at all it should be that last one
which writes the field of interest. (The fundamental assumption
then would be that no OS uses REP MOVS to fill data for more than
one table entry at a time.) This would then also cover various
Windows versions.

Jan


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

Re: [PATCH] x86/vMSI-X: avoid missing first unmask of vectors

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 21 April 2016 10:38
> To: xen-devel
> Cc: Andrew Cooper; Paul Durrant; Wei Liu; Keir (Xen.org)
> Subject: [PATCH] x86/vMSI-X: avoid missing first unmask of vectors
> 
> Recent changes to Linux result in there just being a single unmask
> operation prior to expecting the first interrupts to arrive. However,
> we've had a chicken-and-egg problem here: Qemu invokes
> xc_domain_update_msi_irq(), ultimately leading to
> msixtbl_pt_register(), upon seeing that first unmask operation. Yet
> for msixtbl_range() to return true (in order to msixtbl_write() to get
> invoked at all) msixtbl_pt_register() must have completed.
> 
> Deal with this by snooping suitable writes in msixtbl_range() and
> triggering the invocation of msix_write_completion() from
> msixtbl_pt_register() when that happens in the context of a still in
> progress vector control field write.
> 
> Note that the seemingly unrelated deletion of the redundant
> irq_desc->msi_desc checks in msixtbl_pt_register() is to make clear to
> any compiler version used that the "msi_desc" local variable isn't
> being used uninitialized. (Doing the same in msixtbl_pt_unregister() is
> just for consistency reasons.)
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> TODO: How to deal with REP MOVS to the MSI-X table (in msixtbl_range())?
> 
> --- a/xen/arch/x86/hvm/vmsi.c
> +++ b/xen/arch/x86/hvm/vmsi.c
> @@ -341,7 +352,21 @@ static int msixtbl_range(struct vcpu *v,
>      desc = msixtbl_addr_to_desc(msixtbl_find_entry(v, addr), addr);
>      rcu_read_unlock(&msixtbl_rcu_lock);
> 
> -    return !!desc;
> +    if ( desc )
> +        return 1;
> +
> +    if ( (addr & (PCI_MSIX_ENTRY_SIZE - 1)) ==
> +         PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET )
> +    {
> +        const ioreq_t *r = &v->arch.hvm_vcpu.hvm_io.io_req;
> +

If you need to start digging into the ioreq here then I think it would be better to have vmsi register a full hvm_io_ops rather than hvm_mmio_ops. That way it gets the ioreq passed the accept method without any need to dig.

  Paul

> +        ASSERT(r->type == IOREQ_TYPE_COPY);
> +        if ( r->dir == IOREQ_WRITE && r->size == 4 && !r->data_is_ptr
> +             && !(r->data & PCI_MSIX_VECTOR_BITMASK) )
> +            v->arch.hvm_vcpu.hvm_io.msix_snoop_address = addr;
> +    }
> +
> +    return 0;
>  }
> 
>  static const struct hvm_mmio_ops msixtbl_mmio_ops = {
> @@ -410,9 +434,6 @@ int msixtbl_pt_register(struct domain *d
>          return r;
>      }
> 
> -    if ( !irq_desc->msi_desc )
> -        goto out;
> -
>      msi_desc = irq_desc->msi_desc;
>      if ( !msi_desc )
>          goto out;
> @@ -437,6 +458,21 @@ found:
>  out:
>      spin_unlock_irq(&irq_desc->lock);
>      xfree(new_entry);
> +
> +    if ( !r )
> +    {
> +        struct vcpu *v;
> +
> +        for_each_vcpu ( d, v )
> +        {
> +            if ( v->arch.hvm_vcpu.hvm_io.msix_snoop_address ==
> +                 (gtable + msi_desc->msi_attrib.entry_nr * PCI_MSIX_ENTRY_SIZE
> +
> +                  PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET) )
> +                v->arch.hvm_vcpu.hvm_io.msix_unmask_address =
> +                    v->arch.hvm_vcpu.hvm_io.msix_snoop_address;
> +        }
> +    }
> +
>      return r;
>  }
> 
> @@ -457,9 +496,6 @@ void msixtbl_pt_unregister(struct domain
>      if ( !irq_desc )
>          return;
> 
> -    if ( !irq_desc->msi_desc )
> -        goto out;
> -
>      msi_desc = irq_desc->msi_desc;
>      if ( !msi_desc )
>          goto out;
> @@ -522,6 +558,8 @@ void msix_write_completion(struct vcpu *
>  {
>      unsigned long ctrl_address = v-
> >arch.hvm_vcpu.hvm_io.msix_unmask_address;
> 
> +    v->arch.hvm_vcpu.hvm_io.msix_snoop_address = 0;
> +
>      if ( !ctrl_address )
>          return;
> 
> --- a/xen/include/asm-x86/hvm/vcpu.h
> +++ b/xen/include/asm-x86/hvm/vcpu.h
> @@ -85,6 +85,7 @@ struct hvm_vcpu_io {
>      bool_t mmio_retry;
> 
>      unsigned long msix_unmask_address;
> +    unsigned long msix_snoop_address;
> 
>      const struct g2m_ioport *g2m_ioport;
>  };
> 
> 


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

Re: [PATCH] x86/vMSI-X: avoid missing first unmask of vectors

[Jan Beulich]

>>> On 21.04.16 at 13:33, <Paul.Durrant@citrix.com> wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 21 April 2016 10:38
>> --- a/xen/arch/x86/hvm/vmsi.c
>> +++ b/xen/arch/x86/hvm/vmsi.c
>> @@ -341,7 +352,21 @@ static int msixtbl_range(struct vcpu *v,
>>      desc = msixtbl_addr_to_desc(msixtbl_find_entry(v, addr), addr);
>>      rcu_read_unlock(&msixtbl_rcu_lock);
>> 
>> -    return !!desc;
>> +    if ( desc )
>> +        return 1;
>> +
>> +    if ( (addr & (PCI_MSIX_ENTRY_SIZE - 1)) ==
>> +         PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET )
>> +    {
>> +        const ioreq_t *r = &v->arch.hvm_vcpu.hvm_io.io_req;
>> +
> 
> If you need to start digging into the ioreq here then I think it would be 
> better to have vmsi register a full hvm_io_ops rather than hvm_mmio_ops. That 
> way it gets the ioreq passed the accept method without any need to dig.

I can certainly try and see how this works out, but it's relatively
clear that it'll make the patch bigger and backporting more difficult.
So maybe this would better be left as a subsequent cleanup
exercise?

Jan


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

Re: [PATCH] x86/vMSI-X: avoid missing first unmask of vectors

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 21 April 2016 12:44
> To: Paul Durrant
> Cc: Andrew Cooper; Wei Liu; xen-devel; Keir (Xen.org)
> Subject: RE: [PATCH] x86/vMSI-X: avoid missing first unmask of vectors
> 
> >>> On 21.04.16 at 13:33, <Paul.Durrant@citrix.com> wrote:
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: 21 April 2016 10:38
> >> --- a/xen/arch/x86/hvm/vmsi.c
> >> +++ b/xen/arch/x86/hvm/vmsi.c
> >> @@ -341,7 +352,21 @@ static int msixtbl_range(struct vcpu *v,
> >>      desc = msixtbl_addr_to_desc(msixtbl_find_entry(v, addr), addr);
> >>      rcu_read_unlock(&msixtbl_rcu_lock);
> >>
> >> -    return !!desc;
> >> +    if ( desc )
> >> +        return 1;
> >> +
> >> +    if ( (addr & (PCI_MSIX_ENTRY_SIZE - 1)) ==
> >> +         PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET )
> >> +    {
> >> +        const ioreq_t *r = &v->arch.hvm_vcpu.hvm_io.io_req;
> >> +
> >
> > If you need to start digging into the ioreq here then I think it would be
> > better to have vmsi register a full hvm_io_ops rather than hvm_mmio_ops.
> That
> > way it gets the ioreq passed the accept method without any need to dig.
> 
> I can certainly try and see how this works out, but it's relatively
> clear that it'll make the patch bigger and backporting more difficult.
> So maybe this would better be left as a subsequent cleanup
> exercise?
>

True, it would make backporting more tricky so maybe a follow-up patch would be the better approach. In which case...

Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

> Jan


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

Re: [PATCH] x86/vMSI-X: avoid missing first unmask of vectors

[Wei Liu]

On Thu, Apr 21, 2016 at 03:38:26AM -0600, Jan Beulich wrote:
> Recent changes to Linux result in there just being a single unmask
> operation prior to expecting the first interrupts to arrive. However,
> we've had a chicken-and-egg problem here: Qemu invokes
> xc_domain_update_msi_irq(), ultimately leading to
> msixtbl_pt_register(), upon seeing that first unmask operation. Yet
> for msixtbl_range() to return true (in order to msixtbl_write() to get
> invoked at all) msixtbl_pt_register() must have completed.
> 
> Deal with this by snooping suitable writes in msixtbl_range() and
> triggering the invocation of msix_write_completion() from
> msixtbl_pt_register() when that happens in the context of a still in
> progress vector control field write.
> 
> Note that the seemingly unrelated deletion of the redundant
> irq_desc->msi_desc checks in msixtbl_pt_register() is to make clear to
> any compiler version used that the "msi_desc" local variable isn't
> being used uninitialized. (Doing the same in msixtbl_pt_unregister() is
> just for consistency reasons.)
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> TODO: How to deal with REP MOVS to the MSI-X table (in msixtbl_range())?
> 

As I understand it, this should be future improvement. The patch itself
is an improvement in its own right so it can go in:

Release-acked-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH] x86/vMSI-X: avoid missing first unmask of vectors

[Jan Beulich]

>>> On 25.04.16 at 15:25, <wei.liu2@citrix.com> wrote:
> On Thu, Apr 21, 2016 at 03:38:26AM -0600, Jan Beulich wrote:
>> Recent changes to Linux result in there just being a single unmask
>> operation prior to expecting the first interrupts to arrive. However,
>> we've had a chicken-and-egg problem here: Qemu invokes
>> xc_domain_update_msi_irq(), ultimately leading to
>> msixtbl_pt_register(), upon seeing that first unmask operation. Yet
>> for msixtbl_range() to return true (in order to msixtbl_write() to get
>> invoked at all) msixtbl_pt_register() must have completed.
>> 
>> Deal with this by snooping suitable writes in msixtbl_range() and
>> triggering the invocation of msix_write_completion() from
>> msixtbl_pt_register() when that happens in the context of a still in
>> progress vector control field write.
>> 
>> Note that the seemingly unrelated deletion of the redundant
>> irq_desc->msi_desc checks in msixtbl_pt_register() is to make clear to
>> any compiler version used that the "msi_desc" local variable isn't
>> being used uninitialized. (Doing the same in msixtbl_pt_unregister() is
>> just for consistency reasons.)
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> TODO: How to deal with REP MOVS to the MSI-X table (in msixtbl_range())?
>> 
> 
> As I understand it, this should be future improvement.

Yes. I've actually been working on this the last couple of hours.

> The patch itself
> is an improvement in its own right so it can go in:
> 
> Release-acked-by: Wei Liu <wei.liu2@citrix.com>

Thanks, Jan


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