[Xen-devel,v2,02/45] ARM: Implement vcpu_kick()

Message ID 20180315203050.19791-3-andre.przywara@linaro.org
State New
Headers show
Series
  • New VGIC(-v2) implementation
Related show

Commit Message

Andre Przywara March 15, 2018, 8:30 p.m.
If we change something in a vCPU that affects its runnability or
otherwise needs the vCPU's attention, we might need to tell the scheduler
about it.
We are using this in one place (vIRQ injection) at the moment, but will
need this at more places soon.
So let's factor out this functionality, using the already existing
vcpu_kick() prototype (used in x86 only so far), to make this available
to the rest of the Xen code.
Also adjust the perfcounter name to reflect the new usage.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
---
Changelog v1 ... v2:
- use vcpu_kick() name from x86 and existing prototype
- rename performance counter

 xen/arch/arm/domain.c            | 12 ++++++++++++
 xen/arch/arm/vgic.c              | 11 +++--------
 xen/include/asm-arm/perfc_defn.h |  3 ++-
 3 files changed, 17 insertions(+), 9 deletions(-)

Comments

Julien Grall March 16, 2018, 10:59 a.m. | #1
Hi Andre,

On 15/03/18 20:30, Andre Przywara wrote:
> If we change something in a vCPU that affects its runnability or
> otherwise needs the vCPU's attention, we might need to tell the scheduler
> about it.
> We are using this in one place (vIRQ injection) at the moment, but will
> need this at more places soon.
> So let's factor out this functionality, using the already existing
> vcpu_kick() prototype (used in x86 only so far), to make this available
> to the rest of the Xen code.
> Also adjust the perfcounter name to reflect the new usage.
> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>

Reviewed-by: Julien Grall <julien.grall@arm.com>

Cheers,

> ---
> Changelog v1 ... v2:
> - use vcpu_kick() name from x86 and existing prototype
> - rename performance counter
> 
>   xen/arch/arm/domain.c            | 12 ++++++++++++
>   xen/arch/arm/vgic.c              | 11 +++--------
>   xen/include/asm-arm/perfc_defn.h |  3 ++-
>   3 files changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index bc10f412ba..4462e62599 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -967,6 +967,18 @@ void vcpu_block_unless_event_pending(struct vcpu *v)
>           vcpu_unblock(current);
>   }
>   
> +void vcpu_kick(struct vcpu *vcpu)
> +{
> +    bool running = vcpu->is_running;
> +
> +    vcpu_unblock(vcpu);
> +    if ( running && vcpu != current )
> +    {
> +        perfc_incr(vcpu_kick);
> +        smp_send_event_check_mask(cpumask_of(vcpu->processor));
> +    }
> +}
> +
>   /*
>    * Local variables:
>    * mode: C
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index eb09d9ca54..3fafdd0b66 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -26,6 +26,7 @@
>   #include <xen/sched.h>
>   #include <xen/perfc.h>
>   
> +#include <asm/event.h>
>   #include <asm/current.h>
>   
>   #include <asm/mmio.h>
> @@ -530,7 +531,6 @@ void vgic_inject_irq(struct domain *d, struct vcpu *v, unsigned int virq,
>       uint8_t priority;
>       struct pending_irq *iter, *n;
>       unsigned long flags;
> -    bool running;
>   
>       /*
>        * For edge triggered interrupts we always ignore a "falling edge".
> @@ -590,14 +590,9 @@ void vgic_inject_irq(struct domain *d, struct vcpu *v, unsigned int virq,
>       list_add_tail(&n->inflight, &v->arch.vgic.inflight_irqs);
>   out:
>       spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> +
>       /* we have a new higher priority irq, inject it into the guest */
> -    running = v->is_running;
> -    vcpu_unblock(v);
> -    if ( running && v != current )
> -    {
> -        perfc_incr(vgic_cross_cpu_intr_inject);
> -        smp_send_event_check_mask(cpumask_of(v->processor));
> -    }
> +    vcpu_kick(v);
>   
>       return;
>   }
> diff --git a/xen/include/asm-arm/perfc_defn.h b/xen/include/asm-arm/perfc_defn.h
> index 87866264ca..8922e9525a 100644
> --- a/xen/include/asm-arm/perfc_defn.h
> +++ b/xen/include/asm-arm/perfc_defn.h
> @@ -33,6 +33,8 @@ PERFCOUNTER(vpsci_cpu_suspend,         "vpsci: cpu_suspend")
>   PERFCOUNTER(vpsci_cpu_affinity_info,   "vpsci: cpu_affinity_info")
>   PERFCOUNTER(vpsci_features,            "vpsci: features")
>   
> +PERFCOUNTER(vcpu_kick,                 "vcpu: notify other vcpu")
> +
>   PERFCOUNTER(vgicd_reads,                "vgicd: read")
>   PERFCOUNTER(vgicd_writes,               "vgicd: write")
>   PERFCOUNTER(vgicr_reads,                "vgicr: read")
> @@ -44,7 +46,6 @@ PERFCOUNTER(vgic_sysreg_writes,         "vgic: sysreg write")
>   PERFCOUNTER(vgic_sgi_list  ,            "vgic: SGI send to list")
>   PERFCOUNTER(vgic_sgi_others,            "vgic: SGI send to others")
>   PERFCOUNTER(vgic_sgi_self,              "vgic: SGI send to self")
> -PERFCOUNTER(vgic_cross_cpu_intr_inject, "vgic: cross-CPU irq inject")
>   PERFCOUNTER(vgic_irq_migrates,          "vgic: irq migration")
>   
>   PERFCOUNTER(vuart_reads,  "vuart: read")
>
Stefano Stabellini March 16, 2018, 9:23 p.m. | #2
On Thu, 15 Mar 2018, Andre Przywara wrote:
> If we change something in a vCPU that affects its runnability or
> otherwise needs the vCPU's attention, we might need to tell the scheduler
> about it.
> We are using this in one place (vIRQ injection) at the moment, but will
> need this at more places soon.
> So let's factor out this functionality, using the already existing
> vcpu_kick() prototype (used in x86 only so far), to make this available
> to the rest of the Xen code.
> Also adjust the perfcounter name to reflect the new usage.
> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> Changelog v1 ... v2:
> - use vcpu_kick() name from x86 and existing prototype
> - rename performance counter
> 
>  xen/arch/arm/domain.c            | 12 ++++++++++++
>  xen/arch/arm/vgic.c              | 11 +++--------
>  xen/include/asm-arm/perfc_defn.h |  3 ++-
>  3 files changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index bc10f412ba..4462e62599 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -967,6 +967,18 @@ void vcpu_block_unless_event_pending(struct vcpu *v)
>          vcpu_unblock(current);
>  }
>  
> +void vcpu_kick(struct vcpu *vcpu)
> +{
> +    bool running = vcpu->is_running;
> +
> +    vcpu_unblock(vcpu);
> +    if ( running && vcpu != current )
> +    {
> +        perfc_incr(vcpu_kick);
> +        smp_send_event_check_mask(cpumask_of(vcpu->processor));
> +    }
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index eb09d9ca54..3fafdd0b66 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -26,6 +26,7 @@
>  #include <xen/sched.h>
>  #include <xen/perfc.h>
>  
> +#include <asm/event.h>
>  #include <asm/current.h>
>  
>  #include <asm/mmio.h>
> @@ -530,7 +531,6 @@ void vgic_inject_irq(struct domain *d, struct vcpu *v, unsigned int virq,
>      uint8_t priority;
>      struct pending_irq *iter, *n;
>      unsigned long flags;
> -    bool running;
>  
>      /*
>       * For edge triggered interrupts we always ignore a "falling edge".
> @@ -590,14 +590,9 @@ void vgic_inject_irq(struct domain *d, struct vcpu *v, unsigned int virq,
>      list_add_tail(&n->inflight, &v->arch.vgic.inflight_irqs);
>  out:
>      spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> +
>      /* we have a new higher priority irq, inject it into the guest */
> -    running = v->is_running;
> -    vcpu_unblock(v);
> -    if ( running && v != current )
> -    {
> -        perfc_incr(vgic_cross_cpu_intr_inject);
> -        smp_send_event_check_mask(cpumask_of(v->processor));
> -    }
> +    vcpu_kick(v);
>  
>      return;
>  }
> diff --git a/xen/include/asm-arm/perfc_defn.h b/xen/include/asm-arm/perfc_defn.h
> index 87866264ca..8922e9525a 100644
> --- a/xen/include/asm-arm/perfc_defn.h
> +++ b/xen/include/asm-arm/perfc_defn.h
> @@ -33,6 +33,8 @@ PERFCOUNTER(vpsci_cpu_suspend,         "vpsci: cpu_suspend")
>  PERFCOUNTER(vpsci_cpu_affinity_info,   "vpsci: cpu_affinity_info")
>  PERFCOUNTER(vpsci_features,            "vpsci: features")
>  
> +PERFCOUNTER(vcpu_kick,                 "vcpu: notify other vcpu")
> +
>  PERFCOUNTER(vgicd_reads,                "vgicd: read")
>  PERFCOUNTER(vgicd_writes,               "vgicd: write")
>  PERFCOUNTER(vgicr_reads,                "vgicr: read")
> @@ -44,7 +46,6 @@ PERFCOUNTER(vgic_sysreg_writes,         "vgic: sysreg write")
>  PERFCOUNTER(vgic_sgi_list  ,            "vgic: SGI send to list")
>  PERFCOUNTER(vgic_sgi_others,            "vgic: SGI send to others")
>  PERFCOUNTER(vgic_sgi_self,              "vgic: SGI send to self")
> -PERFCOUNTER(vgic_cross_cpu_intr_inject, "vgic: cross-CPU irq inject")
>  PERFCOUNTER(vgic_irq_migrates,          "vgic: irq migration")
>  
>  PERFCOUNTER(vuart_reads,  "vuart: read")
> -- 
> 2.14.1
>
Jan Beulich March 20, 2018, 10:35 a.m. | #3
>>> On 15.03.18 at 21:30, <andre.przywara@linaro.org> wrote:
> If we change something in a vCPU that affects its runnability or
> otherwise needs the vCPU's attention, we might need to tell the scheduler
> about it.
> We are using this in one place (vIRQ injection) at the moment, but will
> need this at more places soon.
> So let's factor out this functionality, using the already existing
> vcpu_kick() prototype (used in x86 only so far), to make this available
> to the rest of the Xen code.

Having just seen this among the commits having gone in recently -
was it considered to make this a common function? The
implementations currently differ, but I'm not sure I see why that
needs to be. With x86's vcpu_kick_softirq() handler doing nothing
I could see the ARM implementation be suitable for x86, just like
I could see the x86 implementation be suitable for ARM.

Jan
Julien Grall March 21, 2018, 4:10 a.m. | #4
Hi Jan,

On 03/20/2018 10:35 AM, Jan Beulich wrote:
>>>> On 15.03.18 at 21:30, <andre.przywara@linaro.org> wrote:
>> If we change something in a vCPU that affects its runnability or
>> otherwise needs the vCPU's attention, we might need to tell the scheduler
>> about it.
>> We are using this in one place (vIRQ injection) at the moment, but will
>> need this at more places soon.
>> So let's factor out this functionality, using the already existing
>> vcpu_kick() prototype (used in x86 only so far), to make this available
>> to the rest of the Xen code.
> 
> Having just seen this among the commits having gone in recently -
> was it considered to make this a common function? The
> implementations currently differ, but I'm not sure I see why that
> needs to be. With x86's vcpu_kick_softirq() handler doing nothing
> I could see the ARM implementation be suitable for x86, just like
> I could see the x86 implementation be suitable for ARM.
I considered it when reviewing the patch but discard it I wasn't 
entirely sure if it was possible to make it common and  I wanted this 
series to move forward (it is 50 patches series)!

I would be happy to consider any patch to make them common. My 
preference would tend to go towards the Arm solution as it has a 
slightly smaller overhead to kick a vCPU. Indeed the x86 version 
requires to raise a softirq and then send an IPI to the other CPU.

Cheers,
Jan Beulich March 21, 2018, 7:40 a.m. | #5
>>> On 21.03.18 at 05:10, <julien.grall@arm.com> wrote:
> On 03/20/2018 10:35 AM, Jan Beulich wrote:
>>>>> On 15.03.18 at 21:30, <andre.przywara@linaro.org> wrote:
>>> If we change something in a vCPU that affects its runnability or
>>> otherwise needs the vCPU's attention, we might need to tell the scheduler
>>> about it.
>>> We are using this in one place (vIRQ injection) at the moment, but will
>>> need this at more places soon.
>>> So let's factor out this functionality, using the already existing
>>> vcpu_kick() prototype (used in x86 only so far), to make this available
>>> to the rest of the Xen code.
>> 
>> Having just seen this among the commits having gone in recently -
>> was it considered to make this a common function? The
>> implementations currently differ, but I'm not sure I see why that
>> needs to be. With x86's vcpu_kick_softirq() handler doing nothing
>> I could see the ARM implementation be suitable for x86, just like
>> I could see the x86 implementation be suitable for ARM.
> I considered it when reviewing the patch but discard it I wasn't 
> entirely sure if it was possible to make it common and  I wanted this 
> series to move forward (it is 50 patches series)!
> 
> I would be happy to consider any patch to make them common. My 
> preference would tend to go towards the Arm solution as it has a 
> slightly smaller overhead to kick a vCPU. Indeed the x86 version 
> requires to raise a softirq and then send an IPI to the other CPU.

Yes, that's my preference too. I'll send something after 4.11 has
settled.

Jan

Patch

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index bc10f412ba..4462e62599 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -967,6 +967,18 @@  void vcpu_block_unless_event_pending(struct vcpu *v)
         vcpu_unblock(current);
 }
 
+void vcpu_kick(struct vcpu *vcpu)
+{
+    bool running = vcpu->is_running;
+
+    vcpu_unblock(vcpu);
+    if ( running && vcpu != current )
+    {
+        perfc_incr(vcpu_kick);
+        smp_send_event_check_mask(cpumask_of(vcpu->processor));
+    }
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index eb09d9ca54..3fafdd0b66 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -26,6 +26,7 @@ 
 #include <xen/sched.h>
 #include <xen/perfc.h>
 
+#include <asm/event.h>
 #include <asm/current.h>
 
 #include <asm/mmio.h>
@@ -530,7 +531,6 @@  void vgic_inject_irq(struct domain *d, struct vcpu *v, unsigned int virq,
     uint8_t priority;
     struct pending_irq *iter, *n;
     unsigned long flags;
-    bool running;
 
     /*
      * For edge triggered interrupts we always ignore a "falling edge".
@@ -590,14 +590,9 @@  void vgic_inject_irq(struct domain *d, struct vcpu *v, unsigned int virq,
     list_add_tail(&n->inflight, &v->arch.vgic.inflight_irqs);
 out:
     spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
+
     /* we have a new higher priority irq, inject it into the guest */
-    running = v->is_running;
-    vcpu_unblock(v);
-    if ( running && v != current )
-    {
-        perfc_incr(vgic_cross_cpu_intr_inject);
-        smp_send_event_check_mask(cpumask_of(v->processor));
-    }
+    vcpu_kick(v);
 
     return;
 }
diff --git a/xen/include/asm-arm/perfc_defn.h b/xen/include/asm-arm/perfc_defn.h
index 87866264ca..8922e9525a 100644
--- a/xen/include/asm-arm/perfc_defn.h
+++ b/xen/include/asm-arm/perfc_defn.h
@@ -33,6 +33,8 @@  PERFCOUNTER(vpsci_cpu_suspend,         "vpsci: cpu_suspend")
 PERFCOUNTER(vpsci_cpu_affinity_info,   "vpsci: cpu_affinity_info")
 PERFCOUNTER(vpsci_features,            "vpsci: features")
 
+PERFCOUNTER(vcpu_kick,                 "vcpu: notify other vcpu")
+
 PERFCOUNTER(vgicd_reads,                "vgicd: read")
 PERFCOUNTER(vgicd_writes,               "vgicd: write")
 PERFCOUNTER(vgicr_reads,                "vgicr: read")
@@ -44,7 +46,6 @@  PERFCOUNTER(vgic_sysreg_writes,         "vgic: sysreg write")
 PERFCOUNTER(vgic_sgi_list  ,            "vgic: SGI send to list")
 PERFCOUNTER(vgic_sgi_others,            "vgic: SGI send to others")
 PERFCOUNTER(vgic_sgi_self,              "vgic: SGI send to self")
-PERFCOUNTER(vgic_cross_cpu_intr_inject, "vgic: cross-CPU irq inject")
 PERFCOUNTER(vgic_irq_migrates,          "vgic: irq migration")
 
 PERFCOUNTER(vuart_reads,  "vuart: read")