From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH 1/3] x86/HVM: properly propagate errors from HVMOP_inject_msi Date: Tue, 3 Jun 2014 15:05:34 +0100 Message-ID: <538DD62E.3050306@citrix.com> References: <538DEF42020000780001767A@mail.emea.novell.com> <538DF0BF0200007800017689@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============8718701962221501096==" Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1WrpLl-0002AQ-2a for xen-devel@lists.xenproject.org; Tue, 03 Jun 2014 14:06:01 +0000 In-Reply-To: <538DF0BF0200007800017689@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: xen-devel , Keir Fraser , Stefano Stabellini List-Id: xen-devel@lists.xenproject.org --===============8718701962221501096== Content-Type: multipart/alternative; boundary="------------080109090409040800040402" --------------080109090409040800040402 Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit On 03/06/14 14:58, Jan Beulich wrote: > There are a number of ways this operation can go wrong, all of which > got ignored so far. > > In the context of this I wonder whether map_domain_emuirq_pirq() > returning 0 in the "already mapped" case is really intended to be that > way (this is why the subsequent NULL check here can't be an ASSERT()). > > Signed-off-by: Jan Beulich Reviewed-by: Andrew Cooper > > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -5096,7 +5096,7 @@ static int hvmop_inject_msi( > if ( rc ) > goto out; > > - hvm_inject_msi(d, op.addr, op.data); > + rc = hvm_inject_msi(d, op.addr, op.data); > > out: > rcu_unlock_domain(d); > --- a/xen/arch/x86/hvm/irq.c > +++ b/xen/arch/x86/hvm/irq.c > @@ -270,7 +270,7 @@ void hvm_set_pci_link_route(struct domai > d->domain_id, link, old_isa_irq, isa_irq); > } > > -void hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data) > +int hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data) > { > uint32_t tmp = (uint32_t) addr; > uint8_t dest = (tmp & MSI_ADDR_DEST_ID_MASK) >> MSI_ADDR_DEST_ID_SHIFT; > @@ -292,20 +292,28 @@ void hvm_inject_msi(struct domain *d, ui > /* if it is the first time, allocate the pirq */ > if ( !info || info->arch.hvm.emuirq == IRQ_UNBOUND ) > { > + int rc; > + > spin_lock(&d->event_lock); > - map_domain_emuirq_pirq(d, pirq, IRQ_MSI_EMU); > + rc = map_domain_emuirq_pirq(d, pirq, IRQ_MSI_EMU); > spin_unlock(&d->event_lock); > + if ( rc ) > + return rc; > info = pirq_info(d, pirq); > if ( !info ) > - return; > - } else if (info->arch.hvm.emuirq != IRQ_MSI_EMU) > - return; > + return -EBUSY; > + } > + else if ( info->arch.hvm.emuirq != IRQ_MSI_EMU ) > + return -EINVAL; > send_guest_pirq(d, info); > - return; > + return 0; > } > + return -ERANGE; > } > > vmsi_deliver(d, vector, dest, dest_mode, delivery_mode, trig_mode); > + > + return 0; > } > > void hvm_set_callback_via(struct domain *d, uint64_t via) > --- a/xen/include/xen/hvm/irq.h > +++ b/xen/include/xen/hvm/irq.h > @@ -123,7 +123,7 @@ void hvm_isa_irq_deassert( > > void hvm_set_pci_link_route(struct domain *d, u8 link, u8 isa_irq); > > -void hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data); > +int hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data); > > void hvm_maybe_deassert_evtchn_irq(void); > void hvm_assert_evtchn_irq(struct vcpu *v); > > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel --------------080109090409040800040402 Content-Type: text/html; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit
On 03/06/14 14:58, Jan Beulich wrote:
There are a number of ways this operation can go wrong, all of which
got ignored so far.

In the context of this I wonder whether map_domain_emuirq_pirq()
returning 0 in the "already mapped" case is really intended to be that
way (this is why the subsequent NULL check here can't be an ASSERT()).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>


--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -5096,7 +5096,7 @@ static int hvmop_inject_msi(
     if ( rc )
         goto out;
 
-    hvm_inject_msi(d, op.addr, op.data);
+    rc = hvm_inject_msi(d, op.addr, op.data);
 
  out:
     rcu_unlock_domain(d);
--- a/xen/arch/x86/hvm/irq.c
+++ b/xen/arch/x86/hvm/irq.c
@@ -270,7 +270,7 @@ void hvm_set_pci_link_route(struct domai
             d->domain_id, link, old_isa_irq, isa_irq);
 }
 
-void hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data)
+int hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data)
 {
     uint32_t tmp = (uint32_t) addr;
     uint8_t  dest = (tmp & MSI_ADDR_DEST_ID_MASK) >> MSI_ADDR_DEST_ID_SHIFT;
@@ -292,20 +292,28 @@ void hvm_inject_msi(struct domain *d, ui
             /* if it is the first time, allocate the pirq */
             if ( !info || info->arch.hvm.emuirq == IRQ_UNBOUND )
             {
+                int rc;
+
                 spin_lock(&d->event_lock);
-                map_domain_emuirq_pirq(d, pirq, IRQ_MSI_EMU);
+                rc = map_domain_emuirq_pirq(d, pirq, IRQ_MSI_EMU);
                 spin_unlock(&d->event_lock);
+                if ( rc )
+                    return rc;
                 info = pirq_info(d, pirq);
                 if ( !info )
-                    return;
-            } else if (info->arch.hvm.emuirq != IRQ_MSI_EMU)
-                return;
+                    return -EBUSY;
+            }
+            else if ( info->arch.hvm.emuirq != IRQ_MSI_EMU )
+                return -EINVAL;
             send_guest_pirq(d, info);
-            return;
+            return 0;
         }
+        return -ERANGE;
     }
 
     vmsi_deliver(d, vector, dest, dest_mode, delivery_mode, trig_mode);
+
+    return 0;
 }
 
 void hvm_set_callback_via(struct domain *d, uint64_t via)
--- a/xen/include/xen/hvm/irq.h
+++ b/xen/include/xen/hvm/irq.h
@@ -123,7 +123,7 @@ void hvm_isa_irq_deassert(
 
 void hvm_set_pci_link_route(struct domain *d, u8 link, u8 isa_irq);
 
-void hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data);
+int hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data);
 
 void hvm_maybe_deassert_evtchn_irq(void);
 void hvm_assert_evtchn_irq(struct vcpu *v);






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

--------------080109090409040800040402-- --===============8718701962221501096== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============8718701962221501096==--