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

[Christoffer Dall <christoffer.dall@xxxxxxxxxx>]

On 23 June 2014 19:19, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> 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.
>
AFAIU, wake-up events are implementation-defined.  So KVM takes the
approach to directly define what wake-up events are on that system,
which is simply interrupts:
https://github.com/torvalds/linux/blob/master/arch/arm/kvm/psci.c

But yes, agreed, it's annoying that the wake-up business is a bit vague.

-Christoffer

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel