Re: [PATCH] xen/arm: Make local_events_need_delivery working with idle VPCU

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

Hi,

On 04/05/2015 12:44, Riku Voipio wrote:
> On 27 April 2015 at 19:32, Julien Grall <julien.grall@citrix.com> wrote:
>> Hi Stefano,
>>
>> On 27/04/15 16:36, Stefano Stabellini wrote:
>>> On Mon, 27 Apr 2015, Julien Grall wrote:
>>>> The commit 569fb6c "xen/arm: Data abort exception (R/W) mem_access
>>>> events" makes apply_p2m_changes to call hypercall_preempt_check for any
>>>> operation rather than for relinquish.
>>>>
>>>> The function hypercall_preempt_check call local_events_need_delivery
>>>> which rely on the current VCPU is not an idle VCPU.
>>>> Although, during DOM0 building the current VCPU is an idle one. This
>>>> would make Xen crash with the following stack trace:
>>>>
>>>> (XEN) CPU0: Unexpected Trap: Data Abort
>>>> [...]
>>>> (XEN) Xen call trace:
>>>> (XEN)    [<00256ef4>] apply_p2m_changes+0x210/0x1190 (PC)
>>>> (XEN)    [<002506b4>] gic_events_need_delivery+0x5c/0x13c (LR)
>>>> (XEN)    [<002580ec>] map_mmio_regions+0x64/0x74
>>>> (XEN)    [<00251958>] gicv2v_setup+0xf8/0x150
>>>> (XEN)    [<00250964>] gicv_setup+0x20/0x30
>>>> (XEN)    [<0024cb3c>] arch_domain_create+0x170/0x244
>>>> (XEN)    [<00207df0>] domain_create+0x2ac/0x4d8
>>>> (XEN)    [<0028e3d0>] start_xen+0xcbc/0xee4
>>>> (XEN)    [<00200540>] paging+0x94/0xd8
>>>> (XEN)
>>>> (XEN)
>>>> (XEN) ****************************************
>>>> (XEN) Panic on CPU 0:
>>>> (XEN) CPU0: Unexpected Trap: Data Abort
>>>> (XEN)
>>>> (XEN) ****************************************
>>>>
>>>> As an idle VCPU can never receive an event, return 0 when the current
>>>> VCPU is an idle VCPU in local_events_need_delivery.
>>>>
>>>> Reported-by: Riku Voipio <riku.voipio@linaro.org>
>>>> Signed-off-by: Julien Grall <julien.grall@citrix.com>
>>>> CC: Tamas K Lengyel <tklengyel@sec.in.tum.de>
>>>>
>>>> ---
>>>>
>>>> This bug has been catched during boot on Mustang. This is because we
>>>> have to map large chunk of PCI memory region.
>>>>
>>>> I was able to reproduce the bug on midway by lowering down
>>>> preempt_count_limit to 16 in apply_p2m_changes.
>>>>
>>>> Although, I'm not sure this is the right fix for the bug.
>>>> ---
>>>>   xen/include/asm-arm/event.h | 7 ++++++-
>>>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/xen/include/asm-arm/event.h b/xen/include/asm-arm/event.h
>>>> index 5330dfe..0149d06 100644
>>>> --- a/xen/include/asm-arm/event.h
>>>> +++ b/xen/include/asm-arm/event.h
>>>> @@ -39,7 +39,12 @@ static inline int local_events_need_delivery_nomask(void)
>>>>
>>>>   static inline int local_events_need_delivery(void)
>>>>   {
>>>> -    if ( !vcpu_event_delivery_is_enabled(current) )
>>>> +    struct vcpu *v = current;
>>>> +
>>>> +    if ( unlikely(is_idle_vcpu(v)) )
>>>> +        return 0;
>>>> +
>>>> +    if ( !vcpu_event_delivery_is_enabled(v) )
>>>>           return 0;
>>>>       return local_events_need_delivery_nomask();
>>>>   }
>>>
>>> Is it actually considered correct in Xen to call hypercall_preempt_check
>>> and/or local_events_need_delivery on the idle vcpu?
>>
>> It seems that the x86 version of hypercall_preempt_check is able to cope
>> with idle VCPU. Although, I'm not sure if there is path where
>> hypercall_preempt_check can be called on an idle VCPU (cc x86
>> maintainers for this purpose).
>>
>>> Shouldn't it be avoided and maybe a BUG_ON added here instead?
>>
>> This patch was the simple way to fix the bug. I have other ideas in mind
>> which require some rework in apply_p2m_changes.
>
>> I'll wait to see what x86 maintainers think.
>
> Any news on this front? The head Xen is still failing in LAVA. We are
> risking that
> new regressions get introduced undetected while we wait for the fix for this...

Most of the maintainers were at the Xen hackathon last week and it's a 
bank holiday in England. I expect to have feedback during this week.

Although, no big changes has been pushed to Xen upstream as the 
committers were away too. Therefore, we should not see any major regression.

Regards,

-- 
Julien Grall