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

[Mirela Simonovic <mirela.simonovic@xxxxxxxxxx>]

Hi Julien,


On Wed, Apr 25, 2018 at 3:23 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
> Hi,
>
>
> On 25/04/18 14:09, Mirela Simonovic wrote:
>>
>> On Mon, Apr 23, 2018 at 1:33 PM, Julien Grall <julien.grall@xxxxxxx>
>> wrote:
>>>
>>> Hi Mirela,
>>>
>>>
>>> On 20/04/18 13:25, Mirela Simonovic wrote:
>>>>
>>>>
>>>> When a CPU is hot-unplugged the maintenance interrupt has to be
>>>> released in order to free the memory that was allocated when the CPU
>>>> was hotplugged and interrupt requested. The interrupt was requested
>>>> using request_irq() which is called from start_secondary->
>>>> init_maintenance_interrupt.
>>>>
>>>> Signed-off-by: Mirela Simonovic <mirela.simonovic@xxxxxxxxxx>
>>>>
>>>> ---
>>>> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>>>> CC: Julien Grall <julien.grall@xxxxxxx>
>>>> ---
>>>>    xen/arch/arm/gic.c        | 5 +++++
>>>>    xen/arch/arm/smpboot.c    | 7 +++++++
>>>>    xen/include/asm-arm/gic.h | 1 +
>>>>    3 files changed, 13 insertions(+)
>>>>
>>>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>>>> index 653a815127..e536b99e84 100644
>>>> --- a/xen/arch/arm/gic.c
>>>> +++ b/xen/arch/arm/gic.c
>>>> @@ -431,6 +431,11 @@ void init_maintenance_interrupt(void)
>>>>                    "irq-maintenance", NULL);
>>>>    }
>>>>    +void deinit_maintenance_interrupt(void)
>>>> +{
>>>> +    release_irq(gic_hw_ops->info->maintenance_irq, NULL);
>>>> +}
>>>> +
>>>>    int gic_make_hwdom_dt_node(const struct domain *d,
>>>>                               const struct dt_device_node *gic,
>>>>                               void *fdt)
>>>> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
>>>> index abc642804f..449fefc77d 100644
>>>> --- a/xen/arch/arm/smpboot.c
>>>> +++ b/xen/arch/arm/smpboot.c
>>>> @@ -375,11 +375,18 @@ void __cpu_disable(void)
>>>>          local_irq_disable();
>>>>        gic_disable_cpu();
>>>> +
>>>
>>>
>>>
>>> Spurious change.
>>>
>>>>        /* Allow any queued timer interrupts to get serviced */
>>>>        local_irq_enable();
>>>>        mdelay(1);
>>>>        local_irq_disable();
>>>>    +    /*
>>>> +     * Deinitialize interrupts (this will free the memory that was
>>>> allocated
>>>> +     * in respective init interrupt functions called from
>>>> start_secondary)
>>>> +     */
>>>> +    deinit_maintenance_interrupt();
>>>
>>>
>>>
>>> Can you have a look at using a notifier (see CPU_DIYING)? This would
>>> avoid
>>> exporting too much new function.
>>
>>
>> I believe releasing of maintenance irq should happen after the dying
>> CPU's GIC interface is disabled.
>
>
> Why? The maintenance interrupt will only be fired when running in guest
> context. Furthermore, it is initialized after the GIC has been initialized,
> so it makes sense to disable before hand.
>
>> To make such ordering using notifiers I would need to move these lines
>> from __cpu_disable into the notifier callback under the CPU_DYING
>> case:
>>          local_irq_disable();
>>          gic_disable_cpu();
>>          local_irq_enable();
>
>
> 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.

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.

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

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