lkml.org 
[lkml]   [2015]   [May]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [Xen-devel] [PATCH v2 1/4] xen/events: Clear cpu_evtchn_mask before resuming
On 05/01/2015 06:46 AM, David Vrabel wrote:
> On 29/04/15 22:10, Boris Ostrovsky wrote:
>> When a guest is resumed, the hypervisor may change event channel
>> assignments. If this happens and the guest uses 2-level events it
>> is possible for the interrupt to be claimed by wrong VCPU since
>> cpu_evtchn_mask bits may be stale. This can happen even though
>> evtchn_2l_bind_to_cpu() attempts to clear old bits: irq_info that
>> is passed in is not necessarily the original one (from pre-migration
>> times) but instead is freshly allocated during resume and so any
>> information about which CPU the channel was bound to is lost.
>>
>> Thus we should clear the mask during resume.
>>
>> We also need to make sure that bits for xenstore and console channels
>> are set when these two subsystems are resumed. While rebind_evtchn_irq()
>> (which is invoked for both of them on a resume) calls irq_set_affinity(),
>> the latter will in fact postpone setting affinity until handling the
>> interrupt. But because cpu_evtchn_mask will have bits for these two
>> cleared we won't be able to take the interrupt.
>>
>> With that in mind, we need to bind those two channels explicitly in
>> rebind_evtchn_irq(). We will keep irq_set_affinity() so that we have a
>> pass through generic irq affinity code later, in case something needs
>> to be updated there as well.
>>
>> (Also replace cpumask_of(0) with cpumask_of(info->cpu) in
>> rebind_evtchn_irq(): it should be set to zero in preceding
>> xen_irq_info_evtchn_setup().)
> [...]
>> @@ -1279,8 +1280,16 @@ void rebind_evtchn_irq(int evtchn, int irq)
>>
>> mutex_unlock(&irq_mapping_update_lock);
>>
>> - /* new event channels are always bound to cpu 0 */
>> - irq_set_affinity(irq, cpumask_of(0));
>> + bind_vcpu.port = evtchn;
>> + bind_vcpu.vcpu = info->cpu;
>> + if (HYPERVISOR_event_channel_op(EVTCHNOP_bind_vcpu, &bind_vcpu) == 0)
>> + bind_evtchn_to_cpu(evtchn, info->cpu);
> Isn't the hypercall is unnecessary since this is a new event channel
> it's already bound to VCPU 0 and info->cpu == 0?
>
> I think only the bind_evtchn_to_cpu() call is needed here.


True. However, I added the hypercall here to make the routine
independent of what happens in other parts (hypervisor binding new
channels to cpu0, xen_irq_info_evtchn_setup() initializing to zero,
etc.). This way, if either of these two change in the future (unlikely,
but possible) this routine will still work as expected.

That's why I also replaced cpumask_of(0) with cpumask_of(info->cpu) in
irq_set_affinity() call.

-boris


>
> If you agree I can remove the hypercall and apply this series.
>
>> + else
>> + pr_warn("Failed binding port %d to cpu %d\n",
>> + evtchn, info->cpu);
>> +
>> + /* This will be deferred until interrupt is processed */
>> + irq_set_affinity(irq, cpumask_of(info->cpu));
> David
>



\
 
 \ /
  Last update: 2015-05-01 16:21    [W:0.084 / U:0.464 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site