From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] passthrough: streamline _hvm_dirq_assist() Date: Thu, 11 Sep 2014 15:53:19 +0100 Message-ID: <5411B75F.8040002@citrix.com> References: <5411BC630200007800033E1A@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0317016248158574895==" Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1XS5kT-0007f8-Vu for xen-devel@lists.xenproject.org; Thu, 11 Sep 2014 14:53:26 +0000 In-Reply-To: <5411BC630200007800033E1A@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 , xen-devel List-Id: xen-devel@lists.xenproject.org --===============0317016248158574895== Content-Type: multipart/alternative; boundary="------------020706010103010706060201" --------------020706010103010706060201 Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit On 11/09/14 14:14, Jan Beulich wrote: > The loop inside this function was calling two functions with loop- > invariable arguments which clearly don't need calling more than once: > send_guest_pirq() and __msi_pirq_eoi(). After moving these out of the > loop it further became apparent that folding the hvm_pci_msi_assert() > helper into the main function can further help readability. > > In the course of this I noticed that __hvm_dpci_eoi() called > hvm_pci_intx_deassert() unconditionally, whereas hvm_pci_intx_assert() > (correctly) got called only when !hvm_domain_use_pirq(), so the former > is being made conditional now too. > > Signed-off-by: Jan Beulich Reviewed-by: Andrew Cooper > > --- a/xen/drivers/passthrough/io.c > +++ b/xen/drivers/passthrough/io.c > @@ -513,45 +513,39 @@ void hvm_dpci_msi_eoi(struct domain *d, > spin_unlock(&d->event_lock); > } > > -static void hvm_pci_msi_assert( > - struct domain *d, struct hvm_pirq_dpci *pirq_dpci) > -{ > - struct pirq *pirq = dpci_pirq(pirq_dpci); > - > - if ( hvm_domain_use_pirq(d, pirq) ) > - send_guest_pirq(d, pirq); > - else > - vmsi_deliver_pirq(d, pirq_dpci); > -} > - > static int _hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci, > void *arg) > { > if ( test_and_clear_bool(pirq_dpci->masked) ) > { > + struct pirq *pirq = dpci_pirq(pirq_dpci); > const struct dev_intx_gsi_link *digl; > > + if ( hvm_domain_use_pirq(d, pirq) ) > + { > + send_guest_pirq(d, pirq); > + > + if ( pirq_dpci->flags & HVM_IRQ_DPCI_GUEST_MSI ) > + return 0; > + } > + > if ( pirq_dpci->flags & HVM_IRQ_DPCI_GUEST_MSI ) > { > - hvm_pci_msi_assert(d, pirq_dpci); > + vmsi_deliver_pirq(d, pirq_dpci); > return 0; > } > > list_for_each_entry ( digl, &pirq_dpci->digl_list, list ) > { > - struct pirq *info = dpci_pirq(pirq_dpci); > - > - if ( hvm_domain_use_pirq(d, info) ) > - send_guest_pirq(d, info); > - else > - hvm_pci_intx_assert(d, digl->device, digl->intx); > + hvm_pci_intx_assert(d, digl->device, digl->intx); > pirq_dpci->pending++; > + } > > - if ( pirq_dpci->flags & HVM_IRQ_DPCI_TRANSLATE ) > - { > - /* for translated MSI to INTx interrupt, eoi as early as possible */ > - __msi_pirq_eoi(pirq_dpci); > - } > + if ( pirq_dpci->flags & HVM_IRQ_DPCI_TRANSLATE ) > + { > + /* for translated MSI to INTx interrupt, eoi as early as possible */ > + __msi_pirq_eoi(pirq_dpci); > + return 0; > } > > /* > @@ -561,8 +555,8 @@ static int _hvm_dirq_assist(struct domai > * guest will never deal with the irq, then the physical interrupt line > * will never be deasserted. > */ > - if ( pt_irq_need_timer(pirq_dpci->flags) ) > - set_timer(&pirq_dpci->timer, NOW() + PT_IRQ_TIME_OUT); > + ASSERT(pt_irq_need_timer(pirq_dpci->flags)); > + set_timer(&pirq_dpci->timer, NOW() + PT_IRQ_TIME_OUT); > } > > return 0; > @@ -583,12 +577,12 @@ static void __hvm_dpci_eoi(struct domain > const struct hvm_girq_dpci_mapping *girq, > const union vioapic_redir_entry *ent) > { > - struct pirq *pirq; > + struct pirq *pirq = pirq_info(d, girq->machine_gsi); > struct hvm_pirq_dpci *pirq_dpci; > > - hvm_pci_intx_deassert(d, girq->device, girq->intx); > + if ( !hvm_domain_use_pirq(d, pirq) ) > + hvm_pci_intx_deassert(d, girq->device, girq->intx); > > - pirq = pirq_info(d, girq->machine_gsi); > pirq_dpci = pirq_dpci(pirq); > > /* > > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel --------------020706010103010706060201 Content-Type: text/html; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit
On 11/09/14 14:14, Jan Beulich wrote:
The loop inside this function was calling two functions with loop-
invariable arguments which clearly don't need calling more than once:
send_guest_pirq() and __msi_pirq_eoi(). After moving these out of the
loop it further became apparent that folding the hvm_pci_msi_assert()
helper into the main function can further help readability.

In the course of this I noticed that __hvm_dpci_eoi() called
hvm_pci_intx_deassert() unconditionally, whereas hvm_pci_intx_assert()
(correctly) got called only when !hvm_domain_use_pirq(), so the former
is being made conditional now too.

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

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


--- a/xen/drivers/passthrough/io.c
+++ b/xen/drivers/passthrough/io.c
@@ -513,45 +513,39 @@ void hvm_dpci_msi_eoi(struct domain *d, 
     spin_unlock(&d->event_lock);
 }
 
-static void hvm_pci_msi_assert(
-    struct domain *d, struct hvm_pirq_dpci *pirq_dpci)
-{
-    struct pirq *pirq = dpci_pirq(pirq_dpci);
-
-    if ( hvm_domain_use_pirq(d, pirq) )
-        send_guest_pirq(d, pirq);
-    else
-        vmsi_deliver_pirq(d, pirq_dpci);
-}
-
 static int _hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci,
                             void *arg)
 {
     if ( test_and_clear_bool(pirq_dpci->masked) )
     {
+        struct pirq *pirq = dpci_pirq(pirq_dpci);
         const struct dev_intx_gsi_link *digl;
 
+        if ( hvm_domain_use_pirq(d, pirq) )
+        {
+            send_guest_pirq(d, pirq);
+
+            if ( pirq_dpci->flags & HVM_IRQ_DPCI_GUEST_MSI )
+                return 0;
+        }
+
         if ( pirq_dpci->flags & HVM_IRQ_DPCI_GUEST_MSI )
         {
-            hvm_pci_msi_assert(d, pirq_dpci);
+            vmsi_deliver_pirq(d, pirq_dpci);
             return 0;
         }
 
         list_for_each_entry ( digl, &pirq_dpci->digl_list, list )
         {
-            struct pirq *info = dpci_pirq(pirq_dpci);
-
-            if ( hvm_domain_use_pirq(d, info) )
-                send_guest_pirq(d, info);
-            else
-                hvm_pci_intx_assert(d, digl->device, digl->intx);
+            hvm_pci_intx_assert(d, digl->device, digl->intx);
             pirq_dpci->pending++;
+        }
 
-            if ( pirq_dpci->flags & HVM_IRQ_DPCI_TRANSLATE )
-            {
-                /* for translated MSI to INTx interrupt, eoi as early as possible */
-                __msi_pirq_eoi(pirq_dpci);
-            }
+        if ( pirq_dpci->flags & HVM_IRQ_DPCI_TRANSLATE )
+        {
+            /* for translated MSI to INTx interrupt, eoi as early as possible */
+            __msi_pirq_eoi(pirq_dpci);
+            return 0;
         }
 
         /*
@@ -561,8 +555,8 @@ static int _hvm_dirq_assist(struct domai
          * guest will never deal with the irq, then the physical interrupt line
          * will never be deasserted.
          */
-        if ( pt_irq_need_timer(pirq_dpci->flags) )
-            set_timer(&pirq_dpci->timer, NOW() + PT_IRQ_TIME_OUT);
+        ASSERT(pt_irq_need_timer(pirq_dpci->flags));
+        set_timer(&pirq_dpci->timer, NOW() + PT_IRQ_TIME_OUT);
     }
 
     return 0;
@@ -583,12 +577,12 @@ static void __hvm_dpci_eoi(struct domain
                            const struct hvm_girq_dpci_mapping *girq,
                            const union vioapic_redir_entry *ent)
 {
-    struct pirq *pirq;
+    struct pirq *pirq = pirq_info(d, girq->machine_gsi);
     struct hvm_pirq_dpci *pirq_dpci;
 
-    hvm_pci_intx_deassert(d, girq->device, girq->intx);
+    if ( !hvm_domain_use_pirq(d, pirq) )
+        hvm_pci_intx_deassert(d, girq->device, girq->intx);
 
-    pirq = pirq_info(d, girq->machine_gsi);
     pirq_dpci = pirq_dpci(pirq);
 
     /*






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

--------------020706010103010706060201-- --===============0317016248158574895== 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 --===============0317016248158574895==--