Re: [Xen-devel] [PATCH v2 07/10] xen/arm: Release maintenance interrupt when CPU is hot-unplugged

[Julien Grall <julien.grall@xxxxxxx>]

(+ Andre and Tim)

On 25/04/18 15:28, Mirela Simonovic wrote:
> Hi Julien,
Hi,

> On Wed, Apr 25, 2018 at 3:23 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
> > On 25/04/18 14:09, Mirela Simonovic wrote:
> > > On Mon, Apr 23, 2018 at 1:33 PM, Julien Grall <julien.grall@xxxxxxx>
> > > wrote:
> > > > On 20/04/18 13:25, Mirela Simonovic wrote:
> > This looks a bit weird. AFAIU, if you disable the CPU interface, then you
> > should never receive interrupt after. So why would you re-enable them?
> >
> > I realize the code in __cpu_disbale do that, but this looks quite wrong to
> > me. There are no way to receive queued timer interrupt afterwards.
> That is what I took as a reference, but I asked myself the same.
> There is (extremely small, but it exists) time window between
> disabling irq locally and disabling CPU interface. An interrupt
> received in that time window would propagate to the CPU but I'm not
> sure would happen after the GIC CPU interface is disabled and
> interrupts are locally enabled. That is the only explanation I can
> come up with, although I believe the answer is nothing. Since you're
> at ARM you could check this internally.
Speaking with Andre (in CC), the GIC CPU interface may have forwarded an 
interrupt to the processor before it gets disabled. So when the 
interrupt will be re-enabled, the processor will jump to the interrupt 
exception entry.
However, looking at the spec (4-78 in ARM IHI 0048B.b), Xen will read a 
spurious interrupt ID from GICC_IAR. So I am not sure what the point of 
that code. It looks like it has been taken from x86, but some bits are 
missing.
AFAIU, x86 will only suspend the timer afterwards (see time_suspend). I 
am not fully sure why this code is there on Arm. Whether we expect a 
timer interrupt to come up. Stefano, Tim, do you have any insight on 
that code?
> Anyway, since we're taking the notifier approach the timer interrupt
> would be disabled before the GIC CPU interface, so I believe the
> mdelay and the surrounding local_irq_enable/disable will not be
> needed.
> Please lets do such a cleanup out of this series.
Depending on the outcome of the discussions, do you plan to fix the code?

> > > then below these lines in the callback I would add
> > >           release_irq(gic_hw_ops->info->maintenance_irq, NULL);
> > >
> > > This would have to be done because CPU_DYING notifiers execute before
> > > __cpu_disable().
> > > How that sounds? If it's ok, should these changes be split into 2
> > > patches (1) notifier based call to gic_disable_cpu + 2) release
> > > maintenance irq, I believe this is better) or should I merge them?
> > I am not sure this is right to do. We want to disable the CPU interface very
> > late (imagine we need to service interrupt).
> This doesn't mean that the gic_disable_cpu can't be done using
> notifiers, we would just first disable maintenance irq and then gic
> cpu interface.
> However, moving gic_disable_cpu in notifier would mean that interrupts
> should be disabled using notifiers (whose priority is higher than gic
> notifier's priority) as well.
I would prefer to keep the gic_disable_cpu where it is at the moment.

> Please lets finalize the discussion and make an agreement on what
> should be done, I would like to get v3 ASAP.
>
> Thanks,
> Mirela
>
> > Cheers,
> >
> > --
> > Julien Grall
--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel