Re: [RFC PATCH V2 09/11] xen: Clear IRQD_IRQ_STARTED flag during shutdown PIRQs

[Anchal Agarwal <anchalag@amazon.com>]

On Fri, Jan 10, 2020 at 08:13:16PM +0100, Thomas Gleixner wrote:
> Anchal,
> 
> Anchal Agarwal <anchalag@amazon.com> writes:
> > On Thu, Jan 09, 2020 at 01:07:27PM +0100, Thomas Gleixner wrote:
> >> Anchal Agarwal <anchalag@amazon.com> writes:
> >> So either you can handle it purely on the XEN side without touching any
> >> core state or you need to come up with some way of letting the core code
> >> know that it should invoke shutdown instead of disable.
> >> 
> >> Something like the completely untested patch below.
> >
> > Understandable. Really appreciate the patch suggestion below and i will test it
> > for sure and see if things can be fixed properly in irq core if thats the only
> > option. In the meanwhile, I tried to fix it on xen side unless it gives you the 
> > same feeling as above? MSI-x are just fine, just ioapic ones don't get any event
> > channel asssigned hence enable_dynirq does nothing. Those needs to be restarted.
> >
> > diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
> > index 1bb0b522d004..2ed152f35816 100644
> > --- a/drivers/xen/events/events_base.c
> >     +++ b/drivers/xen/events/events_base.c
> > @@ -575,6 +575,11 @@ static void shutdown_pirq(struct irq_data *data)
> >
> > static void enable_pirq(struct irq_data *data)
> > {
> >     +/*ioapic interrupts don't get event channel assigned
> >        + * after being explicitly shutdown during guest
> >        + * hibernation. They need to be restarted*/
> >         +       if(!evtchn_from_irq(data->irq))
> >         +               startup_pirq(data);
> >     enable_dynirq(data);
> >  }
> 
> Interesting patch format :)
Apparently vim and me rushing through the email [did not format the patch]
were the culprit and I only caught it after sending an email
> 
> Doing the shutdown from syscore_ops and the startup conditionally in a
> totaly unrelated function is not really intuitive.
> 
I agree to the point that still the startup is not as synchronous 
to shutdown however, enable_pirq is still invoked during irq_startup
for xen specific code and I was trying to reuse the code path to fix 
within xen. Basically borrowing from what this commit [commit 020db9d3]
changed. Not sure if this could have broken under any other environment
though :(

But anyways I think the patch you suggested is much more clean and 
intuitive.

> So either you do it symmetrically in XEN via syscore_ops callbacks or
> you let the irq core code help you out with the patch I provided
> 
In my understanding, it may not be the right thing as syscore stuff runs
with one cpu online and disabled interrupts. Also I did try it in the past 
and failed horribly unless there is any smarter way of doing it.
It should correctly be done in suspend/resume devices as are other device 
interrupts.

I did test the patch you suggested and it works.
I haven't done large scale testing but it looks like it may just work fine.
I will send out an updated patch for shutdown/startup of pirq after I do some
more testing and will drop patches related to shutdown/startup of pirqs from 
the original series.

Thanks,

Anchal

> Thanks,
> 
>         tglx