Re: [Xen-devel] use tasklet to handle init/sipi?

["Zhang, Yang Z" <yang.z.zhang@xxxxxxxxx>]

Keir Fraser wrote on 2013-03-25:
> On 25/03/2013 12:16, "Zhang, Yang Z" <yang.z.zhang@xxxxxxxxx> wrote:
> 
>> Keir Fraser wrote on 2013-03-25:
>>> On 25/03/2013 06:55, "Zhang, Yang Z" <yang.z.zhang@xxxxxxxxx> wrote:
>>> 
>>>> Keir Fraser wrote on 2013-03-25:
>>>>> There are deadlock issues around directly locking and resetting a remote
>>>>> vcpu (e.g., buggy/malicious guest vcpu A sends INIT to vcpu B, and B does
>>>>> same to A).
>>>> 
>>>> Can you elaborate it? Does the lock impact hypervisor or just guest?
>>> 
>>> INIT-handling path takes the domain lock. If two vcpus in same guest try to
>>> INIT each other, one will take the lock and then try to vcpu_pause() the
>>> other. But this will spin forever while that other vcpu itself waits to take
>>> the domain_lock.
>>> 
>>> This seemed to me a fairly fundamental problem of vcpus directly resetting
>>> each other. Hence the deferral to tasklet context.
>> 
>> I see your point. But seems two vcpus call vcpu_pause() simultaneously
>> without hold any lock also will cause the deadlock, see following code:
>> void vcpu_sleep_sync(struct vcpu *v) {
>>     vcpu_sleep_nosync(v);
>>     
>>     while ( !vcpu_runnable(v) && v->is_running )  // two vcpus arrived here
> at
>> same time and waiting each vcpu will cause deadlock?
>>         cpu_relax();
>>     sync_vcpu_execstate(v);
>> }
> 
> Yep, agreed. So we mustn't call vcpu_pause() directly from guest context
> then, you would agree? ;)
Right.

>> Also, should we care about such malicious guest? If the guest really did such
>> thing, it just block himself. It just eat the cpu time which belong to
>> himself. A malicious guest can run a non-stop loop to do same thing.
> 
> No, the spin loop is in the hypervisor. So it is a denial-of-service attack
> on the hypervisor -- i.e., a security concern.

Ok. So we cannot simply removing the tasklet mechanism to fix the issue.
How about we add all target vcpu to a list and iterate the list to wake up all 
VCPUs in then tasklet callback. Then we can wake up all vcpus by call tasklet 
once.

Like this:
static int vlapic_schedule_init_sipi_tasklet(struct vcpu *target, uint32_t icr)
 {
        add target to a list;
        schedule tasklet;
        return X86EMUL_OKAY; //here we return ok instead retry, because we can 
handle all vcpus just once.
}

And in tasklet call back:
for_each_entry_in list
{
        call vlapic_init_sipi_action();
}

>  -- Keir
>>>  -- Keir
>>>>>  -- Keir
>>>>> On 25/03/2013 05:31, "Zhang, Yang Z" <yang.z.zhang@xxxxxxxxx> wrote:
>>>>> 
>>>>>> Hi, Keir,
>>>>>> 
>>>>>> I am looking into a issue and found cs:17457 changes to use tasklet to
>>>>>> handle
>>>>>> init and sipi. And the comments only said "clean up". I wonder is there
>>>>>> any
>>>>>> special reason to use tasklet to handle it? If no, I will send a patch to
>>>>>> call
>>>>>> handler directly instead via tasklet.
>>>>>> The background is that with APICv, it assume all apic write is succeed 
>>>>>> and
>>>>>> don't care the return value of vlapic_reg_write(). But the above logic
>>>>>> need
>>>>>> the caller to check return value. This obviously will break APICv.
>>>>>> 
>>>>>> # HG changeset patch
>>>>>> # User Keir Fraser <keir.fraser@xxxxxxxxxx>
>>>>>> # Date 1208270873 -3600
>>>>>> # Node ID e15be54059e4bde8f5916269dedff5fc3812686a
>>>>>> # Parent  6691ae150d104127c097fd9f3a6acccc5ce43c52
>>>>>> x86, hvm: Clean up handling of APIC INIT and SIPI messages.
>>>>>> Signed-off-by: Keir Fraser <keir.fraser@xxxxxxxxxx>
>>>>>> 
>>>>>> best regards
>>>>>> yang
>>>>>> 
>>>>> 
>>>> 
>>>> 
>>>> Best regards,
>>>> Yang
>>>> 
>>> 
>> 
>> 
>> Best regards,
>> Yang
>> 
>> 
>


Best regards,
Yang


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