[Qemu-devel] [PATCH v4 13/14] ppc/xics: fix irq priority in ics_set_irq_type()

Cédric Le Goater posted 14 patches 6 years, 9 months ago
[Qemu-devel] [PATCH v4 13/14] ppc/xics: fix irq priority in ics_set_irq_type()
Posted by Cédric Le Goater 6 years, 9 months ago
Recent commits changed the behavior of ics_set_irq_type() to
initialize correctly LSIs at the KVM level. ics_set_irq_type() is also
called by the realize routine of the different devices of the machine
when initial interrupts are claimed, before the ICSState device is
reseted.

In the case, the ICSIRQState priority is 0x0 and the call to
ics_set_irq_type() results in configuring the target of the
interrupt. On P9, when using the KVM XICS-on-XIVE device, the target
is configured to be server 0, priority 0 and the event queue 0 is
created automatically by KVM.

With the dual interrupt mode creating the KVM device at reset, it
leads to unexpected effects on the guest, mostly blocking IPIs. This
is wrong, fix it by reseting the ICSIRQState structure when
ics_set_irq_type() is called.

Fixes: commit 6cead90c5c9c ("xics: Write source state to KVM at claim time")
Signed-off-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/intc/xics.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index af7dc709abab..79f5a8a91665 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -610,6 +610,12 @@ static const TypeInfo ics_simple_info = {
     .class_size = sizeof(ICSStateClass),
 };
 
+static void ics_reset_irq(ICSIRQState *irq)
+{
+    irq->priority = 0xff;
+    irq->saved_priority = 0xff;
+}
+
 static void ics_base_reset(DeviceState *dev)
 {
     ICSState *ics = ICS_BASE(dev);
@@ -623,8 +629,7 @@ static void ics_base_reset(DeviceState *dev)
     memset(ics->irqs, 0, sizeof(ICSIRQState) * ics->nr_irqs);
 
     for (i = 0; i < ics->nr_irqs; i++) {
-        ics->irqs[i].priority = 0xff;
-        ics->irqs[i].saved_priority = 0xff;
+        ics_reset_irq(ics->irqs + i);
         ics->irqs[i].flags = flags[i];
     }
 }
@@ -760,6 +765,7 @@ void ics_set_irq_type(ICSState *ics, int srcno, bool lsi)
         lsi ? XICS_FLAGS_IRQ_LSI : XICS_FLAGS_IRQ_MSI;
 
     if (kvm_irqchip_in_kernel()) {
+        ics_reset_irq(ics->irqs + srcno);
         ics_set_kvm_state_one(ics, srcno);
     }
 }
-- 
2.20.1


Re: [Qemu-devel] [PATCH v4 13/14] ppc/xics: fix irq priority in ics_set_irq_type()
Posted by David Gibson 6 years, 9 months ago
On Mon, May 13, 2019 at 10:42:44AM +0200, Cédric Le Goater wrote:
> Recent commits changed the behavior of ics_set_irq_type() to
> initialize correctly LSIs at the KVM level. ics_set_irq_type() is also
> called by the realize routine of the different devices of the machine
> when initial interrupts are claimed, before the ICSState device is
> reseted.
> 
> In the case, the ICSIRQState priority is 0x0 and the call to
> ics_set_irq_type() results in configuring the target of the
> interrupt. On P9, when using the KVM XICS-on-XIVE device, the target
> is configured to be server 0, priority 0 and the event queue 0 is
> created automatically by KVM.
> 
> With the dual interrupt mode creating the KVM device at reset, it
> leads to unexpected effects on the guest, mostly blocking IPIs. This
> is wrong, fix it by reseting the ICSIRQState structure when
> ics_set_irq_type() is called.
> 
> Fixes: commit 6cead90c5c9c ("xics: Write source state to KVM at claim time")
> Signed-off-by: Greg Kurz <groug@kaod.org>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/intc/xics.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index af7dc709abab..79f5a8a91665 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -610,6 +610,12 @@ static const TypeInfo ics_simple_info = {
>      .class_size = sizeof(ICSStateClass),
>  };
>  
> +static void ics_reset_irq(ICSIRQState *irq)
> +{
> +    irq->priority = 0xff;
> +    irq->saved_priority = 0xff;
> +}
> +
>  static void ics_base_reset(DeviceState *dev)
>  {
>      ICSState *ics = ICS_BASE(dev);
> @@ -623,8 +629,7 @@ static void ics_base_reset(DeviceState *dev)
>      memset(ics->irqs, 0, sizeof(ICSIRQState) * ics->nr_irqs);
>  
>      for (i = 0; i < ics->nr_irqs; i++) {
> -        ics->irqs[i].priority = 0xff;
> -        ics->irqs[i].saved_priority = 0xff;
> +        ics_reset_irq(ics->irqs + i);
>          ics->irqs[i].flags = flags[i];
>      }
>  }
> @@ -760,6 +765,7 @@ void ics_set_irq_type(ICSState *ics, int srcno, bool lsi)
>          lsi ? XICS_FLAGS_IRQ_LSI : XICS_FLAGS_IRQ_MSI;
>  
>      if (kvm_irqchip_in_kernel()) {
> +        ics_reset_irq(ics->irqs + srcno);
>          ics_set_kvm_state_one(ics, srcno);
>      }
>  }

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson