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
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 >
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
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 >
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
>>> 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
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 > >
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
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
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
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
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
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
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
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
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 > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
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 >> >> >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
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 > >> > >> > >
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 >> >> >
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 >>> >>> >> >
>>> 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
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 >