[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

(+ Andre and Tim)

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


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



Julien Grall

Julien Grall

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.