netdev - Re: [RFC PATCH V2 09/11] xen: Clear IRQD_IRQ

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20200110225718.GA13573@dev-dsk-anchalag-2a-9c2d1d96.us-west-2.amazon.com>
Date:   Fri, 10 Jan 2020 22:57:18 +0000
From:   Anchal Agarwal <anchalag@...zon.com>
To:     Thomas Gleixner <tglx@...utronix.de>
CC:     <mingo@...hat.com>, <bp@...en8.de>, <hpa@...or.com>,
        <x86@...nel.org>, <boris.ostrovsky@...cle.com>, <jgross@...e.com>,
        <linux-pm@...r.kernel.org>, <linux-mm@...ck.org>,
        <kamatam@...zon.com>, <sstabellini@...nel.org>,
        <konrad.wilk@...cle.co>, <roger.pau@...rix.com>, <axboe@...nel.dk>,
        <davem@...emloft.net>, <rjw@...ysocki.net>, <len.brown@...el.com>,
        <pavel@....cz>, <peterz@...radead.org>, <eduval@...zon.com>,
        <sblbir@...zon.com>, <xen-devel@...ts.xenproject.org>,
        <vkuznets@...hat.com>, <netdev@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <dwmw@...zon.co.uk>,
        <anchalag@...zon.com>
Subject: Re: [RFC PATCH V2 09/11] xen: Clear IRQD_IRQ_STARTED flag during
 shutdown PIRQs

On Fri, Jan 10, 2020 at 08:13:16PM +0100, Thomas Gleixner wrote:
> Anchal,
> 
> Anchal Agarwal <anchalag@...zon.com> writes:
> > On Thu, Jan 09, 2020 at 01:07:27PM +0100, Thomas Gleixner wrote:
> >> Anchal Agarwal <anchalag@...zon.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