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

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

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.

Regards,

-- 
Julien Grall