Re: use tasklet to handle init/sipi?

[Keir Fraser <keir.xen@gmail.com>]

On 25/03/2013 12:16, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote:

> Keir Fraser wrote on 2013-03-25:
>> On 25/03/2013 06:55, "Zhang, Yang Z" <yang.z.zhang@intel.com> 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? ;)

> 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.

 -- Keir

>>  -- Keir
>>>>  -- Keir
>>>> On 25/03/2013 05:31, "Zhang, Yang Z" <yang.z.zhang@intel.com> 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@citrix.com>
>>>>> # 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@citrix.com>
>>>>> 
>>>>> best regards
>>>>> yang
>>>>> 
>>>> 
>>> 
>>> 
>>> Best regards,
>>> Yang
>>> 
>> 
> 
> 
> Best regards,
> Yang
> 
>