Re: [Qemu-devel] [PATCH v2 13/13] spapr/xive: fix device hotplug when VM is stopped

[David Gibson]

On Fri, Feb 22, 2019 at 02:13:22PM +0100, Cédric Le Goater wrote:
> Instead of switching off the sources, set their state to PENDING to
> possibly catch a hotplug event occuring while the VM is stopped. At
> resume, check the previous state and if an interrupt was queued,
> generate a trigger.

First, I think it would be better to fold this fix into the patch
introducing the state change handlers.

Second, IIUC this would handle any instance of an irq being triggered
while the VM is stopped.  Hotplug interrupts is one obvious case of
that, but I'm not sure its the only one.  VFIO devices could interrupt
while the VM is stopped, I think.  Maybe even emulated devices
depending on how their synchronization with the cpu run state works.
There might be other cases.  Does that sound right to you?

> Signed-off-by: Cédric Le Goater <address@hidden>
> ---
>  hw/intc/spapr_xive_kvm.c | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
> index 99a829fb3f60..64d160babb26 100644
> --- a/hw/intc/spapr_xive_kvm.c
> +++ b/hw/intc/spapr_xive_kvm.c
> @@ -500,8 +500,16 @@ static void kvmppc_xive_change_state_handler(void 
> *opaque, int running,
>      if (running) {
>          for (i = 0; i < xsrc->nr_irqs; i++) {
>              uint8_t pq = xive_source_esb_get(xsrc, i);
> -            if (xive_esb_read(xsrc, i, XIVE_ESB_SET_PQ_00 + (pq << 8)) != 
> 0x1) {
> -                error_report("XIVE: IRQ %d has an invalid state", i);
> +            uint8_t old_pq;
> +
> +            old_pq = xive_esb_read(xsrc, i, XIVE_ESB_SET_PQ_00 + (pq << 8));
> +
> +            /*
> +             * If an interrupt was queued (hotplug event) while VM was
> +             * stopped, generate a trigger.
> +             */
> +            if (pq == XIVE_ESB_RESET && old_pq == XIVE_ESB_QUEUED) {
> +                xive_esb_trigger(xsrc, i);
>              }
>          }
>  
> @@ -515,7 +523,15 @@ static void kvmppc_xive_change_state_handler(void 
> *opaque, int running,
>       * migration is in progress.
>       */
>      for (i = 0; i < xsrc->nr_irqs; i++) {
> -        uint8_t pq = xive_esb_read(xsrc, i, XIVE_ESB_SET_PQ_01);
> +        uint8_t pq = xive_esb_read(xsrc, i, XIVE_ESB_GET);
> +
> +        /*
> +         * PQ is set to PENDING to possibly catch a hotplug event
> +         * occuring while the VM is stopped.
> +         */
> +        if (pq != XIVE_ESB_OFF) {
> +            pq = xive_esb_read(xsrc, i, XIVE_ESB_SET_PQ_10);
> +        }
>          xive_source_esb_set(xsrc, i, pq);
>      }
>  

-- 
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

signature.asc
Description: PGP signature