use tasklet to handle init/sipi?

use tasklet to handle init/sipi?

[Zhang, Yang Z]

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

Re: use tasklet to handle init/sipi?

[Keir Fraser]

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

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

Re: use tasklet to handle init/sipi?

[Zhang, Yang Z]

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

Re: use tasklet to handle init/sipi?

[Keir Fraser]

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.

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

Re: use tasklet to handle init/sipi?

[Zhang, Yang Z]

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);
} 

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.

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

Re: use tasklet to handle init/sipi?

[Jan Beulich]

>>> On 25.03.13 at 13:16, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote:
> Keir Fraser wrote on 2013-03-25:
>> 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, ...

But guests aren't permitted uncontrolled access to vcpu_pause().

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

It's one thing for a guest to loop in guest context, and another
for it to cause an unbounded loop in hypervisor context (which
is not preemptible).

Jan

Re: use tasklet to handle init/sipi?

[Keir Fraser]

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

Re: use tasklet to handle init/sipi?

[Zhang, Yang Z]

Keir Fraser wrote on 2013-03-25:
> 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? ;)
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@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
>> 
>> 
>


Best regards,
Yang

Re: use tasklet to handle init/sipi?

[Keir Fraser]

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

>>> 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();
> }

You'll have t elaborate on the problem *you* are trying to solve, and why
such a change would do the trick. If there's good reason, I'm not against a
change such as this. But the code is subtle and I don't want to mess with it
if there are simpler solutions.

 -- Keir

Re: use tasklet to handle init/sipi?

[Zhang, Yang Z]

Keir Fraser wrote on 2013-03-26:
> On 26/03/2013 03:15, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote:
> 
>>>> 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();
>> }
> 
> You'll have t elaborate on the problem *you* are trying to solve, and why
> such a change would do the trick. If there's good reason, I'm not against a
> change such as this. But the code is subtle and I don't want to mess with it
> if there are simpler solutions.
The problem is:
With apicv support, the apic write is trap like vmexit. We cannot fallback to guest to retry the instruction. So it will break current logic.

Best regards,
Yang

Re: use tasklet to handle init/sipi?

[Keir Fraser]

On 26/03/2013 06:14, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote:

>> You'll have t elaborate on the problem *you* are trying to solve, and why
>> such a change would do the trick. If there's good reason, I'm not against a
>> change such as this. But the code is subtle and I don't want to mess with it
>> if there are simpler solutions.
> The problem is:
> With apicv support, the apic write is trap like vmexit. We cannot fallback to
> guest to retry the instruction. So it will break current logic.

Oh, I see. Well I think it is fine to have
vlapic_schedule_init_sipi_tasklet() return X86EMUL_OKAY rather than
X86EMUL_RETRY. We used to need to return RETRY, but the code got simplified
and now it is actually unnecessary.

That should make your patch a lot simpler eh? ;)

 -- Keir

Re: use tasklet to handle init/sipi?

[Keir Fraser]

On 26/03/2013 07:00, "Keir Fraser" <keir.xen@gmail.com> wrote:

>> The problem is:
>> With apicv support, the apic write is trap like vmexit. We cannot fallback to
>> guest to retry the instruction. So it will break current logic.
> 
> Oh, I see. Well I think it is fine to have
> vlapic_schedule_init_sipi_tasklet() return X86EMUL_OKAY rather than
> X86EMUL_RETRY. We used to need to return RETRY, but the code got simplified
> and now it is actually unnecessary.
> 
> That should make your patch a lot simpler eh? ;)

Given that you ignore the return code on the apicv call path, is there
currently a bug at all for you? Seems what is there already must work for
you?

 -- Keir

Re: use tasklet to handle init/sipi?

[Zhang, Yang Z]

Keir Fraser wrote on 2013-03-26:
> On 26/03/2013 07:00, "Keir Fraser" <keir.xen@gmail.com> wrote:
> 
>>> The problem is:
>>> With apicv support, the apic write is trap like vmexit. We cannot fallback to
>>> guest to retry the instruction. So it will break current logic.
>> 
>> Oh, I see. Well I think it is fine to have
>> vlapic_schedule_init_sipi_tasklet() return X86EMUL_OKAY rather than
>> X86EMUL_RETRY. We used to need to return RETRY, but the code got simplified
>> and now it is actually unnecessary.
>> 
>> That should make your patch a lot simpler eh? ;)
> 
> Given that you ignore the return code on the apicv call path, is there
> currently a bug at all for you? Seems what is there already must work
> for you?
It do cause bug after we change to use seabios. For seabios, it will send INIT/SIPI to all vcpus via broadcasting. And there only one vcpu is waken up via tasklet with current logic. That's the reason why I want to wakeup all vcpus on one callback.
Just change X86EMUL_RETRY to OK cannot solve the problem. still need the logic I mentioned above.

Best regards,
Yang

Re: use tasklet to handle init/sipi?

[Keir Fraser]

On 26/03/2013 07:17, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote:

>>> Oh, I see. Well I think it is fine to have
>>> vlapic_schedule_init_sipi_tasklet() return X86EMUL_OKAY rather than
>>> X86EMUL_RETRY. We used to need to return RETRY, but the code got simplified
>>> and now it is actually unnecessary.
>>> 
>>> That should make your patch a lot simpler eh? ;)
>> 
>> Given that you ignore the return code on the apicv call path, is there
>> currently a bug at all for you? Seems what is there already must work
>> for you?
> It do cause bug after we change to use seabios. For seabios, it will send
> INIT/SIPI to all vcpus via broadcasting. And there only one vcpu is waken up
> via tasklet with current logic. That's the reason why I want to wakeup all
> vcpus on one callback.
> Just change X86EMUL_RETRY to OK cannot solve the problem. still need the logic
> I mentioned above.

Ok, wait a sec, I will sort out a patch for you to try...

 -- Keir

Re: use tasklet to handle init/sipi?

[Zhang, Yang Z]

Keir Fraser wrote on 2013-03-26:
> On 26/03/2013 07:17, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote:
> 
>>>> Oh, I see. Well I think it is fine to have
>>>> vlapic_schedule_init_sipi_tasklet() return X86EMUL_OKAY rather than
>>>> X86EMUL_RETRY. We used to need to return RETRY, but the code got
>>>> simplified and now it is actually unnecessary.
>>>> 
>>>> That should make your patch a lot simpler eh? ;)
>>> 
>>> Given that you ignore the return code on the apicv call path, is there
>>> currently a bug at all for you? Seems what is there already must work
>>> for you?
>> It do cause bug after we change to use seabios. For seabios, it will
>> send INIT/SIPI to all vcpus via broadcasting. And there only one vcpu
>> is waken up via tasklet with current logic. That's the reason why I
>> want to wakeup all vcpus on one callback. Just change X86EMUL_RETRY to
>> OK cannot solve the problem. still need the logic I mentioned above.
> 
> Ok, wait a sec, I will sort out a patch for you to try...
Thanks. Actually, I have patch on hand and testing it now. But it's ok if you can provide a more better solution.

Best regards,
Yang

Re: use tasklet to handle init/sipi?

[Keir Fraser]

[-- Attachment #1: Type: text/plain, Size: 1419 bytes --]

On 26/03/2013 07:41, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote:

> Keir Fraser wrote on 2013-03-26:
>> On 26/03/2013 07:17, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote:
>> 
>>>>> Oh, I see. Well I think it is fine to have
>>>>> vlapic_schedule_init_sipi_tasklet() return X86EMUL_OKAY rather than
>>>>> X86EMUL_RETRY. We used to need to return RETRY, but the code got
>>>>> simplified and now it is actually unnecessary.
>>>>> 
>>>>> That should make your patch a lot simpler eh? ;)
>>>> 
>>>> Given that you ignore the return code on the apicv call path, is there
>>>> currently a bug at all for you? Seems what is there already must work
>>>> for you?
>>> It do cause bug after we change to use seabios. For seabios, it will
>>> send INIT/SIPI to all vcpus via broadcasting. And there only one vcpu
>>> is waken up via tasklet with current logic. That's the reason why I
>>> want to wakeup all vcpus on one callback. Just change X86EMUL_RETRY to
>>> OK cannot solve the problem. still need the logic I mentioned above.
>> 
>> Ok, wait a sec, I will sort out a patch for you to try...
> Thanks. Actually, I have patch on hand and testing it now. But it's ok if you
> can provide a more better solution.

See how you like it compared with the attached patch. Attached doesn't
really make the code any more complicated, which is nice. However it is not
tested, at all. ;)

 -- Keir

> Best regards,
> Yang
> 
> 


[-- Attachment #2: 00-vlapic-init --]
[-- Type: application/octet-stream, Size: 6450 bytes --]

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index ea7adf6..38e87ce 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3461,8 +3461,6 @@ void hvm_vcpu_reset_state(struct vcpu *v, uint16_t cs, uint16_t ip)
     struct domain *d = v->domain;
     struct segment_register reg;
 
-    BUG_ON(vcpu_runnable(v));
-
     domain_lock(d);
 
     if ( v->is_initialised )
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 38ff216..bb897bb 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -243,18 +243,22 @@ bool_t vlapic_match_dest(
     return 0;
 }
 
-static void vlapic_init_sipi_action(unsigned long _vcpu)
+static void vlapic_init_sipi_one(struct vcpu *target, uint32_t icr)
 {
-    struct vcpu *origin = (struct vcpu *)_vcpu;
-    struct vcpu *target = vcpu_vlapic(origin)->init_sipi.target;
-    uint32_t icr = vcpu_vlapic(origin)->init_sipi.icr;
-
     vcpu_pause(target);
 
     switch ( icr & APIC_MODE_MASK )
     {
     case APIC_DM_INIT: {
         bool_t fpu_initialised;
+        /* No work on INIT de-assert for P4-type APIC. */
+        if ( (icr & (APIC_INT_LEVELTRIG | APIC_INT_ASSERT)) ==
+             APIC_INT_LEVELTRIG )
+            break;
+        /* Nothing to do if the VCPU is already reset. */
+        if ( !target->is_initialised )
+            break;
+        hvm_vcpu_down(target);
         domain_lock(target->domain);
         /* Reset necessary VCPU state. This does not include FPU state. */
         fpu_initialised = target->fpu_initialised;
@@ -276,28 +280,29 @@ static void vlapic_init_sipi_action(unsigned long _vcpu)
     }
 
     vcpu_unpause(target);
-
-    vcpu_vlapic(origin)->init_sipi.target = NULL;
-    vcpu_unpause(origin);
 }
 
-static int vlapic_schedule_init_sipi_tasklet(struct vcpu *target, uint32_t icr)
+static void vlapic_init_sipi_action(unsigned long _vcpu)
 {
-    struct vcpu *origin = current;
+    struct vcpu *origin = (struct vcpu *)_vcpu;
+    uint32_t icr = vcpu_vlapic(origin)->init_sipi.icr;
+    uint32_t dest = vcpu_vlapic(origin)->init_sipi.dest;
+    uint32_t short_hand = icr & APIC_SHORT_MASK;
+    uint32_t dest_mode  = !!(icr & APIC_DEST_MASK);
+    struct vcpu *v;
 
-    if ( vcpu_vlapic(origin)->init_sipi.target != NULL )
+    if ( icr == 0 )
+        return;
+
+    for_each_vcpu ( origin->domain, v )
     {
-        WARN(); /* should be impossible but don't BUG, just in case */
-        return X86EMUL_UNHANDLEABLE;
+        if ( vlapic_match_dest(vcpu_vlapic(v), vcpu_vlapic(origin),
+                               short_hand, dest, dest_mode) )
+            vlapic_init_sipi_one(v, icr);
     }
 
-    vcpu_pause_nosync(origin);
-
-    vcpu_vlapic(origin)->init_sipi.target = target;
-    vcpu_vlapic(origin)->init_sipi.icr = icr;
-    tasklet_schedule(&vcpu_vlapic(origin)->init_sipi.tasklet);
-
-    return X86EMUL_RETRY;
+    vcpu_vlapic(origin)->init_sipi.icr = 0;
+    vcpu_unpause(origin);
 }
 
 /* Add a pending IRQ into lapic. */
@@ -339,23 +344,9 @@ static int vlapic_accept_irq(struct vcpu *v, uint32_t icr_low)
         break;
 
     case APIC_DM_INIT:
-        /* No work on INIT de-assert for P4-type APIC. */
-        if ( (icr_low & (APIC_INT_LEVELTRIG | APIC_INT_ASSERT)) ==
-             APIC_INT_LEVELTRIG )
-            break;
-        /* Nothing to do if the VCPU is already reset. */
-        if ( !v->is_initialised )
-            break;
-        hvm_vcpu_down(v);
-        rc = vlapic_schedule_init_sipi_tasklet(v, icr_low);
-        break;
-
     case APIC_DM_STARTUP:
-        /* Nothing to do if the VCPU is already initialised. */
-        if ( v->is_initialised )
-            break;
-        rc = vlapic_schedule_init_sipi_tasklet(v, icr_low);
-        break;
+        /* Handled in vlapic_ipi(). */
+        BUG();
 
     default:
         gdprintk(XENLOG_ERR, "TODO: unsupported delivery mode in ICR %x\n",
@@ -427,8 +418,6 @@ int vlapic_ipi(
     unsigned int dest;
     unsigned int short_hand = icr_low & APIC_SHORT_MASK;
     unsigned int dest_mode  = !!(icr_low & APIC_DEST_MASK);
-    struct vlapic *target;
-    struct vcpu *v;
     int rc = X86EMUL_OKAY;
 
     HVM_DBG_LOG(DBG_LEVEL_VLAPIC, "icr = 0x%08x:%08x", icr_high, icr_low);
@@ -437,22 +426,41 @@ int vlapic_ipi(
             ? icr_high
             : GET_xAPIC_DEST_FIELD(icr_high));
 
-    if ( (icr_low & APIC_MODE_MASK) == APIC_DM_LOWEST )
+    switch ( icr_low & APIC_MODE_MASK )
     {
-        target = vlapic_lowest_prio(vlapic_domain(vlapic), vlapic,
-                                    short_hand, dest, dest_mode);
+    case APIC_DM_INIT:
+    case APIC_DM_STARTUP:
+        if ( vlapic->init_sipi.icr != 0 )
+        {
+            WARN(); /* should be impossible but don't BUG, just in case */
+            break;
+        }
+        vcpu_pause_nosync(vlapic_vcpu(vlapic));
+        vlapic->init_sipi.icr = icr_low;
+        vlapic->init_sipi.dest = dest;
+        tasklet_schedule(&vlapic->init_sipi.tasklet);
+        break;
+
+    case APIC_DM_LOWEST: {
+        struct vlapic *target = vlapic_lowest_prio(
+            vlapic_domain(vlapic), vlapic, short_hand, dest, dest_mode);
         if ( target != NULL )
             rc = vlapic_accept_irq(vlapic_vcpu(target), icr_low);
-        return rc;
+        break;
     }
 
-    for_each_vcpu ( vlapic_domain(vlapic), v )
-    {
-        if ( vlapic_match_dest(vcpu_vlapic(v), vlapic,
-                               short_hand, dest, dest_mode) )
+    default: {
+        struct vcpu *v;
+        for_each_vcpu ( vlapic_domain(vlapic), v )
+        {
+            if ( vlapic_match_dest(vcpu_vlapic(v), vlapic,
+                                   short_hand, dest, dest_mode) )
                 rc = vlapic_accept_irq(v, icr_low);
-        if ( rc != X86EMUL_OKAY )
-            break;
+            if ( rc != X86EMUL_OKAY )
+                break;
+        }
+        break;
+    }
     }
 
     return rc;
diff --git a/xen/include/asm-x86/hvm/vlapic.h b/xen/include/asm-x86/hvm/vlapic.h
index 09cb63c..8537a35 100644
--- a/xen/include/asm-x86/hvm/vlapic.h
+++ b/xen/include/asm-x86/hvm/vlapic.h
@@ -62,8 +62,7 @@ struct vlapic {
     struct page_info         *regs_page;
     /* INIT-SIPI-SIPI work gets deferred to a tasklet. */
     struct {
-        struct vcpu          *target;
-        uint32_t             icr;
+        uint32_t             icr, dest;
         struct tasklet       tasklet;
     } init_sipi;
 };

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

Re: use tasklet to handle init/sipi?

[Keir Fraser]

[-- Attachment #1: Type: text/plain, Size: 1604 bytes --]

On 26/03/2013 07:55, "Keir Fraser" <keir.xen@gmail.com> wrote:

> On 26/03/2013 07:41, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote:
> 
>> Keir Fraser wrote on 2013-03-26:
>>> On 26/03/2013 07:17, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote:
>>> 
>>>>>> Oh, I see. Well I think it is fine to have
>>>>>> vlapic_schedule_init_sipi_tasklet() return X86EMUL_OKAY rather than
>>>>>> X86EMUL_RETRY. We used to need to return RETRY, but the code got
>>>>>> simplified and now it is actually unnecessary.
>>>>>> 
>>>>>> That should make your patch a lot simpler eh? ;)
>>>>> 
>>>>> Given that you ignore the return code on the apicv call path, is there
>>>>> currently a bug at all for you? Seems what is there already must work
>>>>> for you?
>>>> It do cause bug after we change to use seabios. For seabios, it will
>>>> send INIT/SIPI to all vcpus via broadcasting. And there only one vcpu
>>>> is waken up via tasklet with current logic. That's the reason why I
>>>> want to wakeup all vcpus on one callback. Just change X86EMUL_RETRY to
>>>> OK cannot solve the problem. still need the logic I mentioned above.
>>> 
>>> Ok, wait a sec, I will sort out a patch for you to try...
>> Thanks. Actually, I have patch on hand and testing it now. But it's ok if you
>> can provide a more better solution.
> 
> See how you like it compared with the attached patch. Attached doesn't
> really make the code any more complicated, which is nice. However it is not
> tested, at all. ;)

And here's a version which actually net *reduces* the code size.

 -- Keir

>  -- Keir
> 
>> Best regards,
>> Yang
>> 
>> 
> 


[-- Attachment #2: 01-vlapic-init --]
[-- Type: application/octet-stream, Size: 8647 bytes --]

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index ea7adf6..38e87ce 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3461,8 +3461,6 @@ void hvm_vcpu_reset_state(struct vcpu *v, uint16_t cs, uint16_t ip)
     struct domain *d = v->domain;
     struct segment_register reg;
 
-    BUG_ON(vcpu_runnable(v));
-
     domain_lock(d);
 
     if ( v->is_initialised )
diff --git a/xen/arch/x86/hvm/viridian.c b/xen/arch/x86/hvm/viridian.c
index 6c7f2dc..1ee0f7f 100644
--- a/xen/arch/x86/hvm/viridian.c
+++ b/xen/arch/x86/hvm/viridian.c
@@ -240,8 +240,8 @@ int wrmsr_viridian_regs(uint32_t idx, uint64_t val)
         eax &= ~(1 << 12);
         edx &= 0xff000000;
         vlapic_set_reg(vlapic, APIC_ICR2, edx);
-        if ( vlapic_ipi(vlapic, eax, edx) == X86EMUL_OKAY )
-            vlapic_set_reg(vlapic, APIC_ICR, eax);
+        vlapic_ipi(vlapic, eax, edx);
+        vlapic_set_reg(vlapic, APIC_ICR, eax);
         break;
     }
 
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 38ff216..d69e8af 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -243,18 +243,22 @@ bool_t vlapic_match_dest(
     return 0;
 }
 
-static void vlapic_init_sipi_action(unsigned long _vcpu)
+static void vlapic_init_sipi_one(struct vcpu *target, uint32_t icr)
 {
-    struct vcpu *origin = (struct vcpu *)_vcpu;
-    struct vcpu *target = vcpu_vlapic(origin)->init_sipi.target;
-    uint32_t icr = vcpu_vlapic(origin)->init_sipi.icr;
-
     vcpu_pause(target);
 
     switch ( icr & APIC_MODE_MASK )
     {
     case APIC_DM_INIT: {
         bool_t fpu_initialised;
+        /* No work on INIT de-assert for P4-type APIC. */
+        if ( (icr & (APIC_INT_LEVELTRIG | APIC_INT_ASSERT)) ==
+             APIC_INT_LEVELTRIG )
+            break;
+        /* Nothing to do if the VCPU is already reset. */
+        if ( !target->is_initialised )
+            break;
+        hvm_vcpu_down(target);
         domain_lock(target->domain);
         /* Reset necessary VCPU state. This does not include FPU state. */
         fpu_initialised = target->fpu_initialised;
@@ -276,36 +280,36 @@ static void vlapic_init_sipi_action(unsigned long _vcpu)
     }
 
     vcpu_unpause(target);
-
-    vcpu_vlapic(origin)->init_sipi.target = NULL;
-    vcpu_unpause(origin);
 }
 
-static int vlapic_schedule_init_sipi_tasklet(struct vcpu *target, uint32_t icr)
+static void vlapic_init_sipi_action(unsigned long _vcpu)
 {
-    struct vcpu *origin = current;
+    struct vcpu *origin = (struct vcpu *)_vcpu;
+    uint32_t icr = vcpu_vlapic(origin)->init_sipi.icr;
+    uint32_t dest = vcpu_vlapic(origin)->init_sipi.dest;
+    uint32_t short_hand = icr & APIC_SHORT_MASK;
+    uint32_t dest_mode  = !!(icr & APIC_DEST_MASK);
+    struct vcpu *v;
+
+    if ( icr == 0 )
+        return;
 
-    if ( vcpu_vlapic(origin)->init_sipi.target != NULL )
+    for_each_vcpu ( origin->domain, v )
     {
-        WARN(); /* should be impossible but don't BUG, just in case */
-        return X86EMUL_UNHANDLEABLE;
+        if ( vlapic_match_dest(vcpu_vlapic(v), vcpu_vlapic(origin),
+                               short_hand, dest, dest_mode) )
+            vlapic_init_sipi_one(v, icr);
     }
 
-    vcpu_pause_nosync(origin);
-
-    vcpu_vlapic(origin)->init_sipi.target = target;
-    vcpu_vlapic(origin)->init_sipi.icr = icr;
-    tasklet_schedule(&vcpu_vlapic(origin)->init_sipi.tasklet);
-
-    return X86EMUL_RETRY;
+    vcpu_vlapic(origin)->init_sipi.icr = 0;
+    vcpu_unpause(origin);
 }
 
 /* Add a pending IRQ into lapic. */
-static int vlapic_accept_irq(struct vcpu *v, uint32_t icr_low)
+static void vlapic_accept_irq(struct vcpu *v, uint32_t icr_low)
 {
     struct vlapic *vlapic = vcpu_vlapic(v);
     uint8_t vector = (uint8_t)icr_low;
-    int rc = X86EMUL_OKAY;
 
     switch ( icr_low & APIC_MODE_MASK )
     {
@@ -339,31 +343,15 @@ static int vlapic_accept_irq(struct vcpu *v, uint32_t icr_low)
         break;
 
     case APIC_DM_INIT:
-        /* No work on INIT de-assert for P4-type APIC. */
-        if ( (icr_low & (APIC_INT_LEVELTRIG | APIC_INT_ASSERT)) ==
-             APIC_INT_LEVELTRIG )
-            break;
-        /* Nothing to do if the VCPU is already reset. */
-        if ( !v->is_initialised )
-            break;
-        hvm_vcpu_down(v);
-        rc = vlapic_schedule_init_sipi_tasklet(v, icr_low);
-        break;
-
     case APIC_DM_STARTUP:
-        /* Nothing to do if the VCPU is already initialised. */
-        if ( v->is_initialised )
-            break;
-        rc = vlapic_schedule_init_sipi_tasklet(v, icr_low);
-        break;
+        /* Handled in vlapic_ipi(). */
+        BUG();
 
     default:
         gdprintk(XENLOG_ERR, "TODO: unsupported delivery mode in ICR %x\n",
                  icr_low);
         domain_crash(v->domain);
     }
-
-    return rc;
 }
 
 struct vlapic *vlapic_lowest_prio(
@@ -421,15 +409,12 @@ void vlapic_handle_EOI_induced_exit(struct vlapic *vlapic, int vector)
     hvm_dpci_msi_eoi(current->domain, vector);
 }
 
-int vlapic_ipi(
+void vlapic_ipi(
     struct vlapic *vlapic, uint32_t icr_low, uint32_t icr_high)
 {
     unsigned int dest;
     unsigned int short_hand = icr_low & APIC_SHORT_MASK;
     unsigned int dest_mode  = !!(icr_low & APIC_DEST_MASK);
-    struct vlapic *target;
-    struct vcpu *v;
-    int rc = X86EMUL_OKAY;
 
     HVM_DBG_LOG(DBG_LEVEL_VLAPIC, "icr = 0x%08x:%08x", icr_high, icr_low);
 
@@ -437,25 +422,40 @@ int vlapic_ipi(
             ? icr_high
             : GET_xAPIC_DEST_FIELD(icr_high));
 
-    if ( (icr_low & APIC_MODE_MASK) == APIC_DM_LOWEST )
+    switch ( icr_low & APIC_MODE_MASK )
     {
-        target = vlapic_lowest_prio(vlapic_domain(vlapic), vlapic,
-                                    short_hand, dest, dest_mode);
+    case APIC_DM_INIT:
+    case APIC_DM_STARTUP:
+        if ( vlapic->init_sipi.icr != 0 )
+        {
+            WARN(); /* should be impossible but don't BUG, just in case */
+            break;
+        }
+        vcpu_pause_nosync(vlapic_vcpu(vlapic));
+        vlapic->init_sipi.icr = icr_low;
+        vlapic->init_sipi.dest = dest;
+        tasklet_schedule(&vlapic->init_sipi.tasklet);
+        break;
+
+    case APIC_DM_LOWEST: {
+        struct vlapic *target = vlapic_lowest_prio(
+            vlapic_domain(vlapic), vlapic, short_hand, dest, dest_mode);
         if ( target != NULL )
-            rc = vlapic_accept_irq(vlapic_vcpu(target), icr_low);
-        return rc;
+            vlapic_accept_irq(vlapic_vcpu(target), icr_low);
+        break;
     }
 
-    for_each_vcpu ( vlapic_domain(vlapic), v )
-    {
-        if ( vlapic_match_dest(vcpu_vlapic(v), vlapic,
-                               short_hand, dest, dest_mode) )
-                rc = vlapic_accept_irq(v, icr_low);
-        if ( rc != X86EMUL_OKAY )
-            break;
+    default: {
+        struct vcpu *v;
+        for_each_vcpu ( vlapic_domain(vlapic), v )
+        {
+            if ( vlapic_match_dest(vcpu_vlapic(v), vlapic,
+                                   short_hand, dest, dest_mode) )
+                vlapic_accept_irq(v, icr_low);
+        }
+        break;
+    }
     }
-
-    return rc;
 }
 
 static uint32_t vlapic_get_tmcct(struct vlapic *vlapic)
@@ -688,9 +688,8 @@ static int vlapic_reg_write(struct vcpu *v,
 
     case APIC_ICR:
         val &= ~(1 << 12); /* always clear the pending bit */
-        rc = vlapic_ipi(vlapic, val, vlapic_get_reg(vlapic, APIC_ICR2));
-        if ( rc == X86EMUL_OKAY )
-            vlapic_set_reg(vlapic, APIC_ICR, val);
+        vlapic_ipi(vlapic, val, vlapic_get_reg(vlapic, APIC_ICR2));
+        vlapic_set_reg(vlapic, APIC_ICR, val);
         break;
 
     case APIC_ICR2:
diff --git a/xen/include/asm-x86/hvm/vlapic.h b/xen/include/asm-x86/hvm/vlapic.h
index 09cb63c..101ef57 100644
--- a/xen/include/asm-x86/hvm/vlapic.h
+++ b/xen/include/asm-x86/hvm/vlapic.h
@@ -62,8 +62,7 @@ struct vlapic {
     struct page_info         *regs_page;
     /* INIT-SIPI-SIPI work gets deferred to a tasklet. */
     struct {
-        struct vcpu          *target;
-        uint32_t             icr;
+        uint32_t             icr, dest;
         struct tasklet       tasklet;
     } init_sipi;
 };
@@ -102,7 +101,7 @@ void vlapic_adjust_i8259_target(struct domain *d);
 void vlapic_EOI_set(struct vlapic *vlapic);
 void vlapic_handle_EOI_induced_exit(struct vlapic *vlapic, int vector);
 
-int vlapic_ipi(struct vlapic *vlapic, uint32_t icr_low, uint32_t icr_high);
+void vlapic_ipi(struct vlapic *vlapic, uint32_t icr_low, uint32_t icr_high);
 
 int vlapic_apicv_write(struct vcpu *v, unsigned int offset);
 

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

Re: use tasklet to handle init/sipi?

[Zhang, Yang Z]

Hi, songtao,

Can you help to test the attached this patch? Thanks.

Best regards,
Yang


> -----Original Message-----
> From: Keir Fraser [mailto:keir.xen@gmail.com]
> Sent: Tuesday, March 26, 2013 4:02 PM
> To: Zhang, Yang Z; xen-devel@lists.xen.org
> Cc: Jan Beulich; Zhang, Xiantao; Qiu, Shuang
> Subject: Re: use tasklet to handle init/sipi?
> 
> On 26/03/2013 07:55, "Keir Fraser" <keir.xen@gmail.com> wrote:
> 
> > On 26/03/2013 07:41, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote:
> >
> >> Keir Fraser wrote on 2013-03-26:
> >>> On 26/03/2013 07:17, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote:
> >>>
> >>>>>> Oh, I see. Well I think it is fine to have
> >>>>>> vlapic_schedule_init_sipi_tasklet() return X86EMUL_OKAY rather than
> >>>>>> X86EMUL_RETRY. We used to need to return RETRY, but the code got
> >>>>>> simplified and now it is actually unnecessary.
> >>>>>>
> >>>>>> That should make your patch a lot simpler eh? ;)
> >>>>>
> >>>>> Given that you ignore the return code on the apicv call path, is there
> >>>>> currently a bug at all for you? Seems what is there already must work
> >>>>> for you?
> >>>> It do cause bug after we change to use seabios. For seabios, it will
> >>>> send INIT/SIPI to all vcpus via broadcasting. And there only one vcpu
> >>>> is waken up via tasklet with current logic. That's the reason why I
> >>>> want to wakeup all vcpus on one callback. Just change X86EMUL_RETRY to
> >>>> OK cannot solve the problem. still need the logic I mentioned above.
> >>>
> >>> Ok, wait a sec, I will sort out a patch for you to try...
> >> Thanks. Actually, I have patch on hand and testing it now. But it's ok if you
> >> can provide a more better solution.
> >
> > See how you like it compared with the attached patch. Attached doesn't
> > really make the code any more complicated, which is nice. However it is not
> > tested, at all. ;)
> 
> And here's a version which actually net *reduces* the code size.
> 
>  -- Keir
> 
> >  -- Keir
> >
> >> Best regards,
> >> Yang
> >>
> >>
> >

Re: use tasklet to handle init/sipi?

[Qiu, Shuang]

Hi Keir,

Thank you for the patch.

We've patched the enclosed file on xen-unstable and tested on qemu.git and qemu-upstream-unstable.git as backends.
In both cases, your patch works.

Best regards!
Shuang


-----Original Message-----
From: Keir Fraser [mailto:keir.xen@gmail.com] 
Sent: Tuesday, March 26, 2013 4:02 PM
To: Zhang, Yang Z; xen-devel@lists.xen.org
Cc: Jan Beulich; Zhang, Xiantao; Qiu, Shuang
Subject: Re: use tasklet to handle init/sipi?

On 26/03/2013 07:55, "Keir Fraser" <keir.xen@gmail.com> wrote:

> On 26/03/2013 07:41, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote:
> 
>> Keir Fraser wrote on 2013-03-26:
>>> On 26/03/2013 07:17, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote:
>>> 
>>>>>> Oh, I see. Well I think it is fine to have
>>>>>> vlapic_schedule_init_sipi_tasklet() return X86EMUL_OKAY rather 
>>>>>> than X86EMUL_RETRY. We used to need to return RETRY, but the code 
>>>>>> got simplified and now it is actually unnecessary.
>>>>>> 
>>>>>> That should make your patch a lot simpler eh? ;)
>>>>> 
>>>>> Given that you ignore the return code on the apicv call path, is 
>>>>> there currently a bug at all for you? Seems what is there already 
>>>>> must work for you?
>>>> It do cause bug after we change to use seabios. For seabios, it 
>>>> will send INIT/SIPI to all vcpus via broadcasting. And there only 
>>>> one vcpu is waken up via tasklet with current logic. That's the 
>>>> reason why I want to wakeup all vcpus on one callback. Just change 
>>>> X86EMUL_RETRY to OK cannot solve the problem. still need the logic I mentioned above.
>>> 
>>> Ok, wait a sec, I will sort out a patch for you to try...
>> Thanks. Actually, I have patch on hand and testing it now. But it's 
>> ok if you can provide a more better solution.
> 
> See how you like it compared with the attached patch. Attached doesn't 
> really make the code any more complicated, which is nice. However it 
> is not tested, at all. ;)

And here's a version which actually net *reduces* the code size.

 -- Keir

>  -- Keir
> 
>> Best regards,
>> Yang
>> 
>> 
> 

Re: use tasklet to handle init/sipi?

[Keir Fraser]

On 28/03/2013 06:39, "Qiu, Shuang" <shuang.qiu@intel.com> wrote:

> Hi Keir,
> 
> Thank you for the patch.
> 
> We've patched the enclosed file on xen-unstable and tested on qemu.git and
> qemu-upstream-unstable.git as backends.
> In both cases, your patch works.

Thanks for the testing! I have now committed my patch.

 -- Keir

> Best regards!
> Shuang
> 
> 
> -----Original Message-----
> From: Keir Fraser [mailto:keir.xen@gmail.com]
> Sent: Tuesday, March 26, 2013 4:02 PM
> To: Zhang, Yang Z; xen-devel@lists.xen.org
> Cc: Jan Beulich; Zhang, Xiantao; Qiu, Shuang
> Subject: Re: use tasklet to handle init/sipi?
> 
> On 26/03/2013 07:55, "Keir Fraser" <keir.xen@gmail.com> wrote:
> 
>> On 26/03/2013 07:41, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote:
>> 
>>> Keir Fraser wrote on 2013-03-26:
>>>> On 26/03/2013 07:17, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote:
>>>> 
>>>>>>> Oh, I see. Well I think it is fine to have
>>>>>>> vlapic_schedule_init_sipi_tasklet() return X86EMUL_OKAY rather
>>>>>>> than X86EMUL_RETRY. We used to need to return RETRY, but the code
>>>>>>> got simplified and now it is actually unnecessary.
>>>>>>> 
>>>>>>> That should make your patch a lot simpler eh? ;)
>>>>>> 
>>>>>> Given that you ignore the return code on the apicv call path, is
>>>>>> there currently a bug at all for you? Seems what is there already
>>>>>> must work for you?
>>>>> It do cause bug after we change to use seabios. For seabios, it
>>>>> will send INIT/SIPI to all vcpus via broadcasting. And there only
>>>>> one vcpu is waken up via tasklet with current logic. That's the
>>>>> reason why I want to wakeup all vcpus on one callback. Just change
>>>>> X86EMUL_RETRY to OK cannot solve the problem. still need the logic I
>>>>> mentioned above.
>>>> 
>>>> Ok, wait a sec, I will sort out a patch for you to try...
>>> Thanks. Actually, I have patch on hand and testing it now. But it's
>>> ok if you can provide a more better solution.
>> 
>> See how you like it compared with the attached patch. Attached doesn't
>> really make the code any more complicated, which is nice. However it
>> is not tested, at all. ;)
> 
> And here's a version which actually net *reduces* the code size.
> 
>  -- Keir
> 
>>  -- Keir
>> 
>>> Best regards,
>>> Yang
>>> 
>>> 
>> 
> 

Re: use tasklet to handle init/sipi?

[Jan Beulich]

>>> On 28.03.13 at 12:48, Keir Fraser <keir.xen@gmail.com> wrote:
> On 28/03/2013 06:39, "Qiu, Shuang" <shuang.qiu@intel.com> wrote:
>> We've patched the enclosed file on xen-unstable and tested on qemu.git and
>> qemu-upstream-unstable.git as backends.
>> In both cases, your patch works.
> 
> Thanks for the testing! I have now committed my patch.

Question now is - do we want this on the stable trees (once it
passed the push gate of course), despite APIC virtualization
(which triggered this) isn't in anything pre-4.3?

Jan

Re: use tasklet to handle init/sipi?

[Keir Fraser]

On 28/03/2013 15:29, "Jan Beulich" <JBeulich@suse.com> wrote:

>>>> On 28.03.13 at 12:48, Keir Fraser <keir.xen@gmail.com> wrote:
>> On 28/03/2013 06:39, "Qiu, Shuang" <shuang.qiu@intel.com> wrote:
>>> We've patched the enclosed file on xen-unstable and tested on qemu.git and
>>> qemu-upstream-unstable.git as backends.
>>> In both cases, your patch works.
>> 
>> Thanks for the testing! I have now committed my patch.
> 
> Question now is - do we want this on the stable trees (once it
> passed the push gate of course), despite APIC virtualization
> (which triggered this) isn't in anything pre-4.3?

There are possibly other ways in which an INIT or SIPI could be
broadcast/multicast and not be properly handled -- hvm_x2apic_msr_write()?
vlapic_ipi() via the Viridian MSRs?

I think it's fairly theoretical risk though!

Overall I would be inclined not to backport.

 -- Keir

> Jan
>