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

[Cédric Le Goater]

On 2/26/19 5:17 AM, David Gibson wrote:
> 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. 

yes.

> Hotplug interrupts is one obvious case of that, but I'm not sure its 
> the only one.  

Do we really need to support device hotplug when the VM is stopped ? 
Is that a libvirt requirement ?  

> VFIO devices could interrupt while the VM is stopped, I think. 

If the guest has configured and mapped the IRQs, I would say yes. 

> Maybe even emulated devices
> depending on how their synchronization with the cpu run state works.

The console is one example.

> There might be other cases.  Does that sound right to you?

yes.

Supporting interrupts while a VM is stopped seems like a weird 
test scenario to me. Should we or should we not ? 

Thanks,

C.

>> 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);
>>      }
>>  
>