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

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

Hi Ian,

On 05/05/15 12:40, Ian Campbell wrote:
> On Mon, 2015-04-27 at 17:32 +0100, Julien Grall wrote:
>>>> 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. 
> 
> AFAICT that's just a coincidence, since an idle vcpu won't ever have a
> pending up call.
> 
>> 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.
> 
> I'm inclined to just go with this patch for now, unless Stefano is
> nacking it.

This patch seem to turn into a workaround, would it be better to move
check idle_check in apply_p2m_check?

I will prepare a follow-up to avoid properly the call
hypercall_preempt_check with idle_vcpu.

> One question first: What aspect of local_events_need_delivery relies on
> the vcpu not being an idle one? I suppose something is not initialised,
> but what.

Everything related to the vGIC is not initialized. It's used in
local_event_need_delivery_nomask (see irq_to_pending and
gic_events_need_devlivery).

-- 
Julien Grall