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

[David Gibson <david@gibson.dropbear.id.au>]

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]