Re: [PATCH RFC] xen/arm : emulation of arm's psci v0.2 standard

[Julien Grall <julien.grall@linaro.org>]

On 06/23/2014 05:57 PM, Stefano Stabellini wrote:
> On Mon, 23 Jun 2014, Julien Grall wrote:
>> On 06/23/2014 11:40 AM, Stefano Stabellini wrote:
>>> On Mon, 23 Jun 2014, Parth Dixit wrote:
>>>> Next version of my patch is ready except for following things on which i need your suggestion
>>>> 1. Exposing PSCI v0.2 functions in device tree - This was not done because it gives the impression that you can modify the function id's
>>>> and kernel will call the function id's based on function id's exposed in device tree whereas kernel ignores it for PSCI v0.2 while it
>>>> follows it for PSCI v0.1 which can be confusing. Either way is fine with me.
>>>> 2.  Why do you clear the IRQ flag in psci_suspend - I am taking cue from the "vcpu_block_enable_events" in xen/common/schedule.c where
>>>> flag is cleared to enable interrupts before pausing the cpu.
>>>
>>> Keep in mind that vcpu_block_enable_events is common code, while
>>> local_event_delivery_enable is the arm specific implementation.
>>>
>>> In the arm case local_event_delivery_enable is implemented by clearing
>>> PSR_IRQ_MASK because effectively that's what is needed to enable event
>>> delivery. Events are just a Xen specific kind of interrupts.
>>>
>>> vcpu_block_enable_events calls local_event_delivery_enable before
>>> blocking a vcpu, to make sure it can wake the vcpu up if an event needs
>>> to be delivered to it.
>>>
>>> We need to clear PSR_IRQ_MASK because the CPU_SUSPEND call "is intended
>>> for use in idle subsystems where the core is expected to return to
>>> execution through a wake up event". The vcpu is never going to come up
>>> again if we don't clear PSR_IRQ_MASK, because events wouldn't be
>>> delivered to it.
>>
>> With this solution Xen will return into the guest with IRQ enable
>> unconditionally.
>>
>> I don't see anything in the specification that allow a such change. So
>> the guest may assume that the IRQs are still disabled. This would break it.
>>
>> Couldn't we use the same trick as WFI ie:
>>
>> vcpu_block();
>> if ( local_events_delivery_nomask() )
>>   vcpu_unblock(current);
>>
>> It might be better to introduce a new helper for this purpose.
> 
> Actually the spec says:
> 
> "5. The caller must ensure that appropriate wake-up events are enabled
> to allow resumption from that state."
> 
> so maybe we could allow the guest kernel to shut itself in the foot and
> avoiding clearing PSR_IRQ_MASK.

It's annoying that a wake-up events is not described in the spec. I
guess we can get the definition of a wake-up events from the ARM ARM
(see B1.8.13). Which say, among other events, that an interrupt is
considered as a wake-up event if PSR_IRQ_MASK is not set.

-- 
Julien Grall