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

[Julien Grall <julien.grall@arm.com>]

Hi,

On 25/04/18 14:09, Mirela Simonovic wrote:
> On Mon, Apr 23, 2018 at 1:33 PM, Julien Grall <julien.grall@arm.com> 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@aggios.com>
>>>
>>> ---
>>> CC: Stefano Stabellini <sstabellini@kernel.org>
>>> CC: Julien Grall <julien.grall@arm.com>
>>> ---
>>>    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.

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

Cheers,

-- 
Julien Grall

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