| Message ID | E0A769A898ADB6449596C41F51EF62C6AE8C38@SZXEMI506-MBX.china.huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 27.02.17 at 11:53, <xuquan8@huawei.com> wrote: > If guest is already in non-root mode, an posted interrupt will > be directly delivered to guest (leaving softirq being set without > actually incurring a VM-Exit - breaking desired softirq behavior). This is irritating - are you describing a problem you mean to fix with this patch, without making clear what the fix is? Or are you describing state after this patch (which the code below suggests), in which case I have to say no, we certainly don't want this. > Then further posted interrupts will skip the IPI, stay in PIR and > not noted until another VM-Exit happens. > > When target vCPU is in non-root mode and running on remote CPU, > posted way is triggered to inject interrupt without VM-Exit. > Otherwise, set VCPU_KICK_SOFTIRQ bit to inject interrupt until > another VM-Entry as a best effort. Furthermore you talking about non-root mode here doesn't fit with the code change, as you can't find out whether the guest is in non-root mode. > --- a/xen/arch/x86/hvm/vmx/vmx.c > +++ b/xen/arch/x86/hvm/vmx/vmx.c > @@ -1839,17 +1839,14 @@ static void vmx_process_isr(int isr, struct vcpu *v) > > static void __vmx_deliver_posted_interrupt(struct vcpu *v) > { > - bool_t running = v->is_running; > + unsigned int cpu = v->processor; > > vcpu_unblock(v); > - if ( running && (in_irq() || (v != current)) ) > - { > - unsigned int cpu = v->processor; > - > - if ( !test_and_set_bit(VCPU_KICK_SOFTIRQ, &softirq_pending(cpu)) > - && (cpu != smp_processor_id()) ) > - send_IPI_mask(cpumask_of(cpu), posted_intr_vector); > - } > + if ( v->is_running && (in_irq() || (v != current)) && > + (cpu != smp_processor_id()) ) > + send_IPI_mask(cpumask_of(cpu), posted_intr_vector); > + else > + set_bit(VCPU_KICK_SOFTIRQ, &softirq_pending(cpu)); Why don't you simply call raise_softirq() here? Plus, if we already want to fix this and _eliminate_ (rather than shrink) any time windows, is namely looking at v->is_running really useful here? By the time you do the next part of the conditional (or call into send_IPI_mask()) that may already have changed. Similarly, while this isn't something you change, I don't really understand the relevance of using in_irq() here. Hence at the very least a code comment should be added imo, clarifying what all this magic is about. Jan
On February 28, 2017 11:08 PM, Jan Beulich wrote: >>>> On 27.02.17 at 11:53, <xuquan8@huawei.com> wrote: >> If guest is already in non-root mode, an posted interrupt will be >> directly delivered to guest (leaving softirq being set without >> actually incurring a VM-Exit - breaking desired softirq behavior). > >This is irritating - are you describing a problem you mean to fix with this patch, >without making clear what the fix is? Or are you describing state after this >patch (which the code below suggests), in which case I have to say no, we >certainly don't want this. > Sorry. I knew you would not like any assumption in patch description.. But I think this one really help community understand what the problem is( this is also valuable). Also, as you know, I am a clumsy in patch description and always open to your suggestion. :) please don't be irritated.. if you have a better description, I am always open to it. >> Then further posted interrupts will skip the IPI, stay in PIR and not >> noted until another VM-Exit happens. >> >> When target vCPU is in non-root mode and running on remote CPU, posted >> way is triggered to inject interrupt without VM-Exit. >> Otherwise, set VCPU_KICK_SOFTIRQ bit to inject interrupt until another >> VM-Entry as a best effort. > >Furthermore you talking about non-root mode here doesn't fit with the code >change, as you can't find out whether the guest is in non-root mode. > Reply in below.. >> --- a/xen/arch/x86/hvm/vmx/vmx.c >> +++ b/xen/arch/x86/hvm/vmx/vmx.c >> @@ -1839,17 +1839,14 @@ static void vmx_process_isr(int isr, struct >> vcpu *v) >> >> static void __vmx_deliver_posted_interrupt(struct vcpu *v) { >> - bool_t running = v->is_running; >> + unsigned int cpu = v->processor; >> >> vcpu_unblock(v); >> - if ( running && (in_irq() || (v != current)) ) >> - { >> - unsigned int cpu = v->processor; >> - >> - if ( !test_and_set_bit(VCPU_KICK_SOFTIRQ, >&softirq_pending(cpu)) >> - && (cpu != smp_processor_id()) ) >> - send_IPI_mask(cpumask_of(cpu), posted_intr_vector); >> - } >> + if ( v->is_running && (in_irq() || (v != current)) && >> + (cpu != smp_processor_id()) ) >> + send_IPI_mask(cpumask_of(cpu), posted_intr_vector); >> + else >> + set_bit(VCPU_KICK_SOFTIRQ, &softirq_pending(cpu)); > >Why don't you simply call raise_softirq() here? > raise_softirq() is a better wrapper, but I didn't get it until you told me. >Plus, if we already want to fix this and _eliminate_ (rather than >shrink) any time windows, is namely looking at v->is_running really useful >here? By the time you do the next part of the conditional (or call into >send_IPI_mask()) that may already have changed. So far, I can't find a better one to check whether the vcpu is in non-root or not. Any suggestion? I am afraid we could not eliminate any time windows, but try our best. >Similarly, while this isn't >something you change, I don't really understand the relevance of using in_irq() >here. Hence at the very least a code comment should be added imo, clarifying >what all this magic is about. > Gone through the code, in_irq() means that the cpu is dispatching an interrupt.. I am really hesitated whether to drop ' (in_irq() || (v != current)) ' or not in v2, but I found there is a same one in vcpu_kick().. So I leave it as is for further discussion. Now I tend to drop it. -Quan
On Wed, Mar 01, 2017 at 12:38:39AM -0700, Jan Beulich wrote: >>>> On 01.03.17 at 04:23, <xuquan8@huawei.com> wrote: >> On February 28, 2017 11:08 PM, Jan Beulich wrote: >>>>>> On 27.02.17 at 11:53, <xuquan8@huawei.com> wrote: >>>> If guest is already in non-root mode, an posted interrupt will be >>>> directly delivered to guest (leaving softirq being set without >>>> actually incurring a VM-Exit - breaking desired softirq behavior). >>> >>>This is irritating - are you describing a problem you mean to fix with this patch, >>>without making clear what the fix is? Or are you describing state after this >>>patch (which the code below suggests), in which case I have to say no, we >>>certainly don't want this. >>> >> >> Sorry. I knew you would not like any assumption in patch description.. >> But I think this one really help community understand what the problem is( >> this is also valuable). >> Also, as you know, I am a clumsy in patch description and always open to >> your suggestion. >> :) please don't be irritated.. if you have a better description, I am always >> open to it. > >Well, the problem here is that a suitable patch description is vital to >understand what was wrong and why the new behavior is what we >want. Hence you really need to view me as the consumer of it, i.e. >you can't rely on me to describe your change and/or findings. > >>>> Then further posted interrupts will skip the IPI, stay in PIR and not >>>> noted until another VM-Exit happens. >>>> >>>> When target vCPU is in non-root mode and running on remote CPU, posted >>>> way is triggered to inject interrupt without VM-Exit. >>>> Otherwise, set VCPU_KICK_SOFTIRQ bit to inject interrupt until another >>>> VM-Entry as a best effort. >>> >>>Furthermore you talking about non-root mode here doesn't fit with the code >>>change, as you can't find out whether the guest is in non-root mode. >>> >> >> Reply in below.. >> >>>> --- a/xen/arch/x86/hvm/vmx/vmx.c >>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c >>>> @@ -1839,17 +1839,14 @@ static void vmx_process_isr(int isr, struct >>>> vcpu *v) >>>> >>>> static void __vmx_deliver_posted_interrupt(struct vcpu *v) { >>>> - bool_t running = v->is_running; >>>> + unsigned int cpu = v->processor; >>>> >>>> vcpu_unblock(v); >>>> - if ( running && (in_irq() || (v != current)) ) >>>> - { >>>> - unsigned int cpu = v->processor; >>>> - >>>> - if ( !test_and_set_bit(VCPU_KICK_SOFTIRQ, >>>&softirq_pending(cpu)) >>>> - && (cpu != smp_processor_id()) ) >>>> - send_IPI_mask(cpumask_of(cpu), posted_intr_vector); >>>> - } >>>> + if ( v->is_running && (in_irq() || (v != current)) && >>>> + (cpu != smp_processor_id()) ) >>>> + send_IPI_mask(cpumask_of(cpu), posted_intr_vector); >>>> + else >>>> + set_bit(VCPU_KICK_SOFTIRQ, &softirq_pending(cpu)); >>> >>>Why don't you simply call raise_softirq() here? >>> >> >> raise_softirq() is a better wrapper, but I didn't get it until you told me. >> >> >>>Plus, if we already want to fix this and _eliminate_ (rather than >>>shrink) any time windows, is namely looking at v->is_running really useful >>>here? By the time you do the next part of the conditional (or call into >>>send_IPI_mask()) that may already have changed. >> >> So far, I can't find a better one to check whether the vcpu is in non-root or not. >> Any suggestion? > >As said before - you can't find out easily, and if you managed to, >by the time you look at the result that result might be stale. Hence >the problem and/or patch description should be written in different >terms. You may, for example, explain (if that's the case, of course) >... > >> I am afraid we could not eliminate any time windows, but try our best. > >... that v->is_running set covers a superset of the time the vCPU >spends in non-root mode, with it being acceptable to _also_ do the >same actions if the flag is set but the vCPU is in root mode. In such >a scenario there would indeed be no time window left. But as I >continue to not fully understand you intentions, I can't judge >whether this applies here. > >>>Similarly, while this isn't >>>something you change, I don't really understand the relevance of using in_irq() >>>here. Hence at the very least a code comment should be added imo, clarifying >>>what all this magic is about. >>> >> >> Gone through the code, in_irq() means that the cpu is dispatching an >> interrupt.. >> I am really hesitated whether to drop ' (in_irq() || (v != current)) ' or >> not in v2, but I found there is a same one in vcpu_kick().. >> So I leave it as is for further discussion. Now I tend to drop it. > >Well, it being that way in vcpu_kick() rather suggests that you >also want the same thing here - after all this looks to be an >open-coded slight variation of that function. _That's_ likely >what is missing to be said here in a comment (and you wouldn't >even need to repeat any of the commentary there [which sadly >looks to be somewhat stale], but simply refer to it). However, >this also points out that your local variable are likely wrong: >v->is_running wants evaluating before calling vcpu_pause(), >while v->processor wants to be evaluated only after the call. > >The main thing to explain then is why v->processor changing >after having evaluated it is not a problem. While relatively >obvious in vcpu_kick() - the field changing means the vCPU >is running, and getting it to run is all vcpu_kick() wants to >achieve -, the goal here - avoiding to miss delivering an >interrupt - is different, and so is rationalizing the correctness >of the code. Good point. I ignore v->processor maybe change. I have thought over __vmx_deliver_posted_interrupt() again and want to share you my opinion. First of all, __vmx_deliver_posted_interrupt() is to let the target vCPU sync PIR to vIRR ASAP. different strategies we will used to deal with different cases. One is we just unblock the target vCPU when the vCPU is blocked. This can make sure the vCPU will go to vmx_intr_assist() where we achieve the goal. The second one is the vCPU is runnable, we will achieve the goal automatically when the vCPU is chosen to run. The third one is the vCPU is running and running on the same pCPU with the source vCPU. It just wants to notify itself. Just raise a softirq is enough. The fourth one is the vCPU is running on other pCPU. To notify the target vCPU, we send a IPI to the target PCPU which the vCPU is on. Note that when the notification arrives to the target PCPU, the target vCPU maybe is blocked, runnable, running in root mode, or running in non-root mode. If the target vCPU is running in non-root mode, hardware will sync PIR to vIRR. If the target vCPU is in non-root mode, the Interrupt handler starts to run. To make sure, we can go back to vmx_intr_assist(), I have suggested that the interrupt handler should raise a softirq. If the target vCPU is runnable, we will raise a softirq to a wrong vCPU. Maybe it isn't a big issue. If the target vCPU is blocked, since before we block a vCPU we will check events through local_events_need_delivery() , the goal must has been achieved. Also incur a IPI and softirq to a wrong vCPU. Combining with Jan's explaination about why v->processor changing is not a problem, I think we handle all the possible cases. Please let me know if there is something wrong or not clear. Thanks, Chao > >Jan
>>> On 01.03.17 at 04:23, <xuquan8@huawei.com> wrote: > On February 28, 2017 11:08 PM, Jan Beulich wrote: >>>>> On 27.02.17 at 11:53, <xuquan8@huawei.com> wrote: >>> If guest is already in non-root mode, an posted interrupt will be >>> directly delivered to guest (leaving softirq being set without >>> actually incurring a VM-Exit - breaking desired softirq behavior). >> >>This is irritating - are you describing a problem you mean to fix with this patch, >>without making clear what the fix is? Or are you describing state after this >>patch (which the code below suggests), in which case I have to say no, we >>certainly don't want this. >> > > Sorry. I knew you would not like any assumption in patch description.. > But I think this one really help community understand what the problem is( > this is also valuable). > Also, as you know, I am a clumsy in patch description and always open to > your suggestion. > :) please don't be irritated.. if you have a better description, I am always > open to it. Well, the problem here is that a suitable patch description is vital to understand what was wrong and why the new behavior is what we want. Hence you really need to view me as the consumer of it, i.e. you can't rely on me to describe your change and/or findings. >>> Then further posted interrupts will skip the IPI, stay in PIR and not >>> noted until another VM-Exit happens. >>> >>> When target vCPU is in non-root mode and running on remote CPU, posted >>> way is triggered to inject interrupt without VM-Exit. >>> Otherwise, set VCPU_KICK_SOFTIRQ bit to inject interrupt until another >>> VM-Entry as a best effort. >> >>Furthermore you talking about non-root mode here doesn't fit with the code >>change, as you can't find out whether the guest is in non-root mode. >> > > Reply in below.. > >>> --- a/xen/arch/x86/hvm/vmx/vmx.c >>> +++ b/xen/arch/x86/hvm/vmx/vmx.c >>> @@ -1839,17 +1839,14 @@ static void vmx_process_isr(int isr, struct >>> vcpu *v) >>> >>> static void __vmx_deliver_posted_interrupt(struct vcpu *v) { >>> - bool_t running = v->is_running; >>> + unsigned int cpu = v->processor; >>> >>> vcpu_unblock(v); >>> - if ( running && (in_irq() || (v != current)) ) >>> - { >>> - unsigned int cpu = v->processor; >>> - >>> - if ( !test_and_set_bit(VCPU_KICK_SOFTIRQ, >>&softirq_pending(cpu)) >>> - && (cpu != smp_processor_id()) ) >>> - send_IPI_mask(cpumask_of(cpu), posted_intr_vector); >>> - } >>> + if ( v->is_running && (in_irq() || (v != current)) && >>> + (cpu != smp_processor_id()) ) >>> + send_IPI_mask(cpumask_of(cpu), posted_intr_vector); >>> + else >>> + set_bit(VCPU_KICK_SOFTIRQ, &softirq_pending(cpu)); >> >>Why don't you simply call raise_softirq() here? >> > > raise_softirq() is a better wrapper, but I didn't get it until you told me. > > >>Plus, if we already want to fix this and _eliminate_ (rather than >>shrink) any time windows, is namely looking at v->is_running really useful >>here? By the time you do the next part of the conditional (or call into >>send_IPI_mask()) that may already have changed. > > So far, I can't find a better one to check whether the vcpu is in non-root or not. > Any suggestion? As said before - you can't find out easily, and if you managed to, by the time you look at the result that result might be stale. Hence the problem and/or patch description should be written in different terms. You may, for example, explain (if that's the case, of course) ... > I am afraid we could not eliminate any time windows, but try our best. ... that v->is_running set covers a superset of the time the vCPU spends in non-root mode, with it being acceptable to _also_ do the same actions if the flag is set but the vCPU is in root mode. In such a scenario there would indeed be no time window left. But as I continue to not fully understand you intentions, I can't judge whether this applies here. >>Similarly, while this isn't >>something you change, I don't really understand the relevance of using in_irq() >>here. Hence at the very least a code comment should be added imo, clarifying >>what all this magic is about. >> > > Gone through the code, in_irq() means that the cpu is dispatching an > interrupt.. > I am really hesitated whether to drop ' (in_irq() || (v != current)) ' or > not in v2, but I found there is a same one in vcpu_kick().. > So I leave it as is for further discussion. Now I tend to drop it. Well, it being that way in vcpu_kick() rather suggests that you also want the same thing here - after all this looks to be an open-coded slight variation of that function. _That's_ likely what is missing to be said here in a comment (and you wouldn't even need to repeat any of the commentary there [which sadly looks to be somewhat stale], but simply refer to it). However, this also points out that your local variable are likely wrong: v->is_running wants evaluating before calling vcpu_pause(), while v->processor wants to be evaluated only after the call. The main thing to explain then is why v->processor changing after having evaluated it is not a problem. While relatively obvious in vcpu_kick() - the field changing means the vCPU is running, and getting it to run is all vcpu_kick() wants to achieve -, the goal here - avoiding to miss delivering an interrupt - is different, and so is rationalizing the correctness of the code. Jan
>>> On 01.03.17 at 07:23, <chao.gao@intel.com> wrote: > On Wed, Mar 01, 2017 at 12:38:39AM -0700, Jan Beulich wrote: >>>>> On 01.03.17 at 04:23, <xuquan8@huawei.com> wrote: >>> Gone through the code, in_irq() means that the cpu is dispatching an >>> interrupt.. >>> I am really hesitated whether to drop ' (in_irq() || (v != current)) ' or >>> not in v2, but I found there is a same one in vcpu_kick().. >>> So I leave it as is for further discussion. Now I tend to drop it. >> >>Well, it being that way in vcpu_kick() rather suggests that you >>also want the same thing here - after all this looks to be an >>open-coded slight variation of that function. _That's_ likely >>what is missing to be said here in a comment (and you wouldn't >>even need to repeat any of the commentary there [which sadly >>looks to be somewhat stale], but simply refer to it). However, >>this also points out that your local variable are likely wrong: >>v->is_running wants evaluating before calling vcpu_pause(), >>while v->processor wants to be evaluated only after the call. >> >>The main thing to explain then is why v->processor changing >>after having evaluated it is not a problem. While relatively >>obvious in vcpu_kick() - the field changing means the vCPU >>is running, and getting it to run is all vcpu_kick() wants to >>achieve -, the goal here - avoiding to miss delivering an >>interrupt - is different, and so is rationalizing the correctness >>of the code. > > Good point. I ignore v->processor maybe change. I have thought over > __vmx_deliver_posted_interrupt() again and want to share you my > opinion. > First of all, __vmx_deliver_posted_interrupt() is to let the target > vCPU sync PIR to vIRR ASAP. > different strategies we will used to deal with different cases. > One is we just unblock the target vCPU when the vCPU is blocked. This can > make sure the vCPU will go to vmx_intr_assist() where we achieve the goal. > The second one is the vCPU is runnable, we will achieve the goal > automatically > when the vCPU is chosen to run. > The third one is the vCPU is running and running on the same pCPU with the > source vCPU. It just wants to notify itself. Just raise a softirq is enough. > The fourth one is the vCPU is running on other pCPU. To notify the target > vCPU, > we send a IPI to the target PCPU which the vCPU is on. Note that when the > notification > arrives to the target PCPU, the target vCPU maybe is blocked, runnable, > running in root mode, > or running in non-root mode. If the target vCPU is running in non-root mode, > hardware > will sync PIR to vIRR. If the target vCPU is in non-root mode, the Interrupt > handler > starts to run. To make sure, we can go back to vmx_intr_assist(), I have > suggested that > the interrupt handler should raise a softirq. If the target vCPU is > runnable, we will > raise a softirq to a wrong vCPU. Maybe it isn't a big issue. If the target > vCPU is > blocked, since before we block a vCPU we will check events through > local_events_need_delivery() > , the goal must has been achieved. Also incur a IPI and softirq to a wrong > vCPU. Looks to cover all cases, so it just needs converting into suitable pieces of code comments and patch description. Jan
On Thu, Mar 02, 2017 at 07:42:33AM +0000, Xuquan (Quan Xu) wrote: >On March 01, 2017 2:24 PM, wrote: >> >>Good point. I ignore v->processor maybe change. I have thought over >> __vmx_deliver_posted_interrupt() again and want to share you my opinion. >>First of all, __vmx_deliver_posted_interrupt() is to let the target vCPU sync >>PIR to vIRR ASAP. >>different strategies we will used to deal with different cases. >>One is we just unblock the target vCPU when the vCPU is blocked. This can >>make sure the vCPU will go to vmx_intr_assist() where we achieve the goal. >>The second one is the vCPU is runnable, we will achieve the goal automatically >>when the vCPU is chosen to run. >>The third one is the vCPU is running and running on the same pCPU with the >>source vCPU. It just wants to notify itself. Just raise a softirq is enough. >>The fourth one is the vCPU is running on other pCPU. To notify the target >>vCPU, we send a IPI to the target PCPU which the vCPU is on. Note that when >>the notification arrives to the target PCPU, the target vCPU maybe is blocked, >>runnable, running in root mode, or running in non-root mode. If the target >>vCPU is running in non-root mode, hardware will sync PIR to vIRR. If the target >>vCPU is in non-root mode, the Interrupt handler starts to run. To make sure, >>we can go back to vmx_intr_assist(), I have suggested that the interrupt >>handler should raise a softirq. > >Does the interrupt handler refer to pi_notification_interrupt() or event_check_interrupt()? > Yes. Please refer to the v3 patch later. Thanks, Chao
> From: Gao, Chao > Sent: Wednesday, March 01, 2017 2:24 PM > > > Good point. I ignore v->processor maybe change. I have thought over > __vmx_deliver_posted_interrupt() again and want to share you my > opinion. > First of all, __vmx_deliver_posted_interrupt() is to let the target > vCPU sync PIR to vIRR ASAP. > different strategies we will used to deal with different cases. > One is we just unblock the target vCPU when the vCPU is blocked. This can > make sure the vCPU will go to vmx_intr_assist() where we achieve the goal. > The second one is the vCPU is runnable, we will achieve the goal automatically > when the vCPU is chosen to run. > The third one is the vCPU is running and running on the same pCPU with the > source vCPU. It just wants to notify itself. Just raise a softirq is enough. > The fourth one is the vCPU is running on other pCPU. To notify the target vCPU, > we send a IPI to the target PCPU which the vCPU is on. Note that when the notification > arrives to the target PCPU, the target vCPU maybe is blocked, runnable, running in root > mode, > or running in non-root mode. If the target vCPU is running in non-root mode, hardware > will sync PIR to vIRR. If the target vCPU is in non-root mode, the Interrupt handler non-root -> root > starts to run. To make sure, we can go back to vmx_intr_assist(), I have suggested that > the interrupt handler should raise a softirq. If the target vCPU is runnable, we will Doesn't the current handler already raise a softirq? > raise a softirq to a wrong vCPU. Maybe it isn't a big issue. If the target vCPU is > blocked, since before we block a vCPU we will check events through > local_events_need_delivery() > , the goal must has been achieved. Also incur a IPI and softirq to a wrong vCPU. > > Combining with Jan's explaination about why v->processor changing is not a problem, I > think > we handle all the possible cases. Please let me know if there is something wrong or not > clear. > overall above looks good to me. Thanks Kevin
On March 01, 2017 2:24 PM, wrote: >On Wed, Mar 01, 2017 at 12:38:39AM -0700, Jan Beulich wrote: >>>>> On 01.03.17 at 04:23, <xuquan8@huawei.com> wrote: >>> On February 28, 2017 11:08 PM, Jan Beulich wrote: >>>>>>> On 27.02.17 at 11:53, <xuquan8@huawei.com> wrote: >>>>> If guest is already in non-root mode, an posted interrupt will be >>>>> directly delivered to guest (leaving softirq being set without >>>>> actually incurring a VM-Exit - breaking desired softirq behavior). >>>> >>>>This is irritating - are you describing a problem you mean to fix >>>>with this patch, without making clear what the fix is? Or are you >>>>describing state after this patch (which the code below suggests), in >>>>which case I have to say no, we certainly don't want this. >>>> >>> >>> Sorry. I knew you would not like any assumption in patch description.. >>> But I think this one really help community understand what the >>> problem is( this is also valuable). >>> Also, as you know, I am a clumsy in patch description and always open >>> to your suggestion. >>> :) please don't be irritated.. if you have a better description, I am >>> always open to it. >> >>Well, the problem here is that a suitable patch description is vital to >>understand what was wrong and why the new behavior is what we want. >>Hence you really need to view me as the consumer of it, i.e. >>you can't rely on me to describe your change and/or findings. >> >>>>> Then further posted interrupts will skip the IPI, stay in PIR and >>>>> not noted until another VM-Exit happens. >>>>> >>>>> When target vCPU is in non-root mode and running on remote CPU, >>>>> posted way is triggered to inject interrupt without VM-Exit. >>>>> Otherwise, set VCPU_KICK_SOFTIRQ bit to inject interrupt until >>>>> another VM-Entry as a best effort. >>>> >>>>Furthermore you talking about non-root mode here doesn't fit with the >>>>code change, as you can't find out whether the guest is in non-root mode. >>>> >>> >>> Reply in below.. >>> >>>>> --- a/xen/arch/x86/hvm/vmx/vmx.c >>>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c >>>>> @@ -1839,17 +1839,14 @@ static void vmx_process_isr(int isr, struct >>>>> vcpu *v) >>>>> >>>>> static void __vmx_deliver_posted_interrupt(struct vcpu *v) { >>>>> - bool_t running = v->is_running; >>>>> + unsigned int cpu = v->processor; >>>>> >>>>> vcpu_unblock(v); >>>>> - if ( running && (in_irq() || (v != current)) ) >>>>> - { >>>>> - unsigned int cpu = v->processor; >>>>> - >>>>> - if ( !test_and_set_bit(VCPU_KICK_SOFTIRQ, >>>>&softirq_pending(cpu)) >>>>> - && (cpu != smp_processor_id()) ) >>>>> - send_IPI_mask(cpumask_of(cpu), posted_intr_vector); >>>>> - } >>>>> + if ( v->is_running && (in_irq() || (v != current)) && >>>>> + (cpu != smp_processor_id()) ) >>>>> + send_IPI_mask(cpumask_of(cpu), posted_intr_vector); >>>>> + else >>>>> + set_bit(VCPU_KICK_SOFTIRQ, &softirq_pending(cpu)); >>>> >>>>Why don't you simply call raise_softirq() here? >>>> >>> >>> raise_softirq() is a better wrapper, but I didn't get it until you told me. >>> >>> >>>>Plus, if we already want to fix this and _eliminate_ (rather than >>>>shrink) any time windows, is namely looking at v->is_running really >>>>useful here? By the time you do the next part of the conditional (or >>>>call into >>>>send_IPI_mask()) that may already have changed. >>> >>> So far, I can't find a better one to check whether the vcpu is in non-root or >not. >>> Any suggestion? >> >>As said before - you can't find out easily, and if you managed to, by >>the time you look at the result that result might be stale. Hence the >>problem and/or patch description should be written in different terms. >>You may, for example, explain (if that's the case, of course) ... >> >>> I am afraid we could not eliminate any time windows, but try our best. >> >>... that v->is_running set covers a superset of the time the vCPU >>spends in non-root mode, with it being acceptable to _also_ do the same >>actions if the flag is set but the vCPU is in root mode. In such a >>scenario there would indeed be no time window left. But as I continue >>to not fully understand you intentions, I can't judge whether this >>applies here. >> >>>>Similarly, while this isn't >>>>something you change, I don't really understand the relevance of >>>>using in_irq() here. Hence at the very least a code comment should be >>>>added imo, clarifying what all this magic is about. >>>> >>> >>> Gone through the code, in_irq() means that the cpu is dispatching an >>> interrupt.. >>> I am really hesitated whether to drop ' (in_irq() || (v != current)) >>> ' or not in v2, but I found there is a same one in vcpu_kick().. >>> So I leave it as is for further discussion. Now I tend to drop it. >> >>Well, it being that way in vcpu_kick() rather suggests that you also >>want the same thing here - after all this looks to be an open-coded >>slight variation of that function. _That's_ likely what is missing to >>be said here in a comment (and you wouldn't even need to repeat any of >>the commentary there [which sadly looks to be somewhat stale], but >>simply refer to it). However, this also points out that your local >>variable are likely wrong: >>v->is_running wants evaluating before calling vcpu_pause(), >>while v->processor wants to be evaluated only after the call. >> >>The main thing to explain then is why v->processor changing after >>having evaluated it is not a problem. While relatively obvious in >>vcpu_kick() - the field changing means the vCPU is running, and getting >>it to run is all vcpu_kick() wants to achieve -, the goal here - >>avoiding to miss delivering an interrupt - is different, and so is >>rationalizing the correctness of the code. > >Good point. I ignore v->processor maybe change. I have thought over > __vmx_deliver_posted_interrupt() again and want to share you my opinion. >First of all, __vmx_deliver_posted_interrupt() is to let the target vCPU sync >PIR to vIRR ASAP. >different strategies we will used to deal with different cases. >One is we just unblock the target vCPU when the vCPU is blocked. This can >make sure the vCPU will go to vmx_intr_assist() where we achieve the goal. >The second one is the vCPU is runnable, we will achieve the goal automatically >when the vCPU is chosen to run. >The third one is the vCPU is running and running on the same pCPU with the >source vCPU. It just wants to notify itself. Just raise a softirq is enough. >The fourth one is the vCPU is running on other pCPU. To notify the target >vCPU, we send a IPI to the target PCPU which the vCPU is on. Note that when >the notification arrives to the target PCPU, the target vCPU maybe is blocked, >runnable, running in root mode, or running in non-root mode. If the target >vCPU is running in non-root mode, hardware will sync PIR to vIRR. If the target >vCPU is in non-root mode, the Interrupt handler starts to run. To make sure, >we can go back to vmx_intr_assist(), I have suggested that the interrupt >handler should raise a softirq. Does the interrupt handler refer to pi_notification_interrupt() or event_check_interrupt()? Quan > If the target vCPU is runnable, we will raise a >softirq to a wrong vCPU. Maybe it isn't a big issue. If the target vCPU is >blocked, since before we block a vCPU we will check events through >local_events_need_delivery() , the goal must has been achieved. Also incur a >IPI and softirq to a wrong vCPU. > >Combining with Jan's explaination about why v->processor changing is not a >problem, I think we handle all the possible cases. Please let me know if there >is something wrong or not clear. > >Thanks, >Chao > >> >>Jan
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index 61925cf..ecdfbbb 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -1839,17 +1839,14 @@ static void vmx_process_isr(int isr, struct vcpu *v) static void __vmx_deliver_posted_interrupt(struct vcpu *v) { - bool_t running = v->is_running; + unsigned int cpu = v->processor; vcpu_unblock(v); - if ( running && (in_irq() || (v != current)) ) - { - unsigned int cpu = v->processor; - - if ( !test_and_set_bit(VCPU_KICK_SOFTIRQ, &softirq_pending(cpu)) - && (cpu != smp_processor_id()) ) - send_IPI_mask(cpumask_of(cpu), posted_intr_vector); - } + if ( v->is_running && (in_irq() || (v != current)) && + (cpu != smp_processor_id()) ) + send_IPI_mask(cpumask_of(cpu), posted_intr_vector); + else + set_bit(VCPU_KICK_SOFTIRQ, &softirq_pending(cpu)); } static void vmx_deliver_posted_intr(struct vcpu *v, u8 vector)