| Message ID | 20200320190737.42110-2-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/nvmx: attempt to fix interrupt injection | expand |
On 20.03.2020 20:07, Roger Pau Monne wrote: > This reverts commit f96e1469ad06b61796c60193daaeb9f8a96d7458. > > The commit is wrong, as the whole point of nvmx_update_apicv is to > update the guest interrupt status field when the Ack on exit VMEXIT > control feature is enabled. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Before anyone gets to look at the other two patches, should this be thrown in right away? Jan
On Mon, Mar 23, 2020 at 09:09:59AM +0100, Jan Beulich wrote: > On 20.03.2020 20:07, Roger Pau Monne wrote: > > This reverts commit f96e1469ad06b61796c60193daaeb9f8a96d7458. > > > > The commit is wrong, as the whole point of nvmx_update_apicv is to > > update the guest interrupt status field when the Ack on exit VMEXIT > > control feature is enabled. > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > Before anyone gets to look at the other two patches, should this > be thrown in right away? I would like if possible get a confirmation from Kevin (or anyone else) that my understanding is correct. I find the nested code very confusing, and I've already made a mistake while trying to fix it. That being said, this was spotted by osstest as introducing a regression, so I guess it's safe to just toss it in now. FWIW patch 2/3 attempts to provide a description of my understanding of how nvmx_update_apicv works. Thanks, Roger.
> From: Roger Pau Monné <roger.pau@citrix.com> > Sent: Monday, March 23, 2020 10:49 PM > > On Mon, Mar 23, 2020 at 09:09:59AM +0100, Jan Beulich wrote: > > On 20.03.2020 20:07, Roger Pau Monne wrote: > > > This reverts commit f96e1469ad06b61796c60193daaeb9f8a96d7458. > > > > > > The commit is wrong, as the whole point of nvmx_update_apicv is to > > > update the guest interrupt status field when the Ack on exit VMEXIT > > > control feature is enabled. > > > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > > > Before anyone gets to look at the other two patches, should this > > be thrown in right away? > > I would like if possible get a confirmation from Kevin (or anyone > else) that my understanding is correct. I find the nested code very > confusing, and I've already made a mistake while trying to fix it. > That being said, this was spotted by osstest as introducing a > regression, so I guess it's safe to just toss it in now. > > FWIW patch 2/3 attempts to provide a description of my understanding > of how nvmx_update_apicv works. > I feel it is not good to take this patch alone, as it was introduced to fix another problem. W/o understanding whether the whole series can fix both old and new problems, we may risk putting nested interrupt logic in an even worse state... Thanks Kevin
On 24.03.2020 06:41, Tian, Kevin wrote: >> From: Roger Pau Monné <roger.pau@citrix.com> >> Sent: Monday, March 23, 2020 10:49 PM >> >> On Mon, Mar 23, 2020 at 09:09:59AM +0100, Jan Beulich wrote: >>> On 20.03.2020 20:07, Roger Pau Monne wrote: >>>> This reverts commit f96e1469ad06b61796c60193daaeb9f8a96d7458. >>>> >>>> The commit is wrong, as the whole point of nvmx_update_apicv is to >>>> update the guest interrupt status field when the Ack on exit VMEXIT >>>> control feature is enabled. >>>> >>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >>> >>> Before anyone gets to look at the other two patches, should this >>> be thrown in right away? >> >> I would like if possible get a confirmation from Kevin (or anyone >> else) that my understanding is correct. I find the nested code very >> confusing, and I've already made a mistake while trying to fix it. >> That being said, this was spotted by osstest as introducing a >> regression, so I guess it's safe to just toss it in now. >> >> FWIW patch 2/3 attempts to provide a description of my understanding >> of how nvmx_update_apicv works. >> > > I feel it is not good to take this patch alone, as it was introduced to fix > another problem. W/o understanding whether the whole series can > fix both old and new problems, we may risk putting nested interrupt > logic in an even worse state... Well, okay, I'll wait then, but it would seem to me that reverting wouldn't put us in a worse state than we were in before that change was put in. Jan
> From: Jan Beulich <jbeulich@suse.com> > Sent: Tuesday, March 24, 2020 4:10 PM > > On 24.03.2020 06:41, Tian, Kevin wrote: > >> From: Roger Pau Monné <roger.pau@citrix.com> > >> Sent: Monday, March 23, 2020 10:49 PM > >> > >> On Mon, Mar 23, 2020 at 09:09:59AM +0100, Jan Beulich wrote: > >>> On 20.03.2020 20:07, Roger Pau Monne wrote: > >>>> This reverts commit f96e1469ad06b61796c60193daaeb9f8a96d7458. > >>>> > >>>> The commit is wrong, as the whole point of nvmx_update_apicv is to > >>>> update the guest interrupt status field when the Ack on exit VMEXIT > >>>> control feature is enabled. > >>>> > >>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > >>> > >>> Before anyone gets to look at the other two patches, should this > >>> be thrown in right away? > >> > >> I would like if possible get a confirmation from Kevin (or anyone > >> else) that my understanding is correct. I find the nested code very > >> confusing, and I've already made a mistake while trying to fix it. > >> That being said, this was spotted by osstest as introducing a > >> regression, so I guess it's safe to just toss it in now. > >> > >> FWIW patch 2/3 attempts to provide a description of my understanding > >> of how nvmx_update_apicv works. > >> > > > > I feel it is not good to take this patch alone, as it was introduced to fix > > another problem. W/o understanding whether the whole series can > > fix both old and new problems, we may risk putting nested interrupt > > logic in an even worse state... > > Well, okay, I'll wait then, but it would seem to me that reverting > wouldn't put us in a worse state than we were in before that change > was put in. Roger needs to make the call, i.e. which problem is more severe, old or new one. Thanks Kevin
On Tue, Mar 24, 2020 at 08:49:27AM +0000, Tian, Kevin wrote: > > From: Jan Beulich <jbeulich@suse.com> > > Sent: Tuesday, March 24, 2020 4:10 PM > > > > On 24.03.2020 06:41, Tian, Kevin wrote: > > >> From: Roger Pau Monné <roger.pau@citrix.com> > > >> Sent: Monday, March 23, 2020 10:49 PM > > >> > > >> On Mon, Mar 23, 2020 at 09:09:59AM +0100, Jan Beulich wrote: > > >>> On 20.03.2020 20:07, Roger Pau Monne wrote: > > >>>> This reverts commit f96e1469ad06b61796c60193daaeb9f8a96d7458. > > >>>> > > >>>> The commit is wrong, as the whole point of nvmx_update_apicv is to > > >>>> update the guest interrupt status field when the Ack on exit VMEXIT > > >>>> control feature is enabled. > > >>>> > > >>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > >>> > > >>> Before anyone gets to look at the other two patches, should this > > >>> be thrown in right away? > > >> > > >> I would like if possible get a confirmation from Kevin (or anyone > > >> else) that my understanding is correct. I find the nested code very > > >> confusing, and I've already made a mistake while trying to fix it. > > >> That being said, this was spotted by osstest as introducing a > > >> regression, so I guess it's safe to just toss it in now. > > >> > > >> FWIW patch 2/3 attempts to provide a description of my understanding > > >> of how nvmx_update_apicv works. > > >> > > > > > > I feel it is not good to take this patch alone, as it was introduced to fix > > > another problem. W/o understanding whether the whole series can > > > fix both old and new problems, we may risk putting nested interrupt > > > logic in an even worse state... > > > > Well, okay, I'll wait then, but it would seem to me that reverting > > wouldn't put us in a worse state than we were in before that change > > was put in. > > Roger needs to make the call, i.e. which problem is more severe, old or > new one. AFAICT those problems affect different kind of hardware, so it doesn't make much difference. IMO f96e1469ad06b6 should be reverted because it's plain wrong, and albeit it seemed to fix the issue in one of my boxes it was just luck. I don't thinks it's going to make matters worse, as osstest is already blocked, but reverting it alone without the rest of the series is not going to unblock osstest either. If you agree it's wrong I think it could go in now, even if it's just to avoid having to repost it with newer versions of the series. Thanks, Roger.
> From: Roger Pau Monné <roger.pau@citrix.com> > Sent: Tuesday, March 24, 2020 6:47 PM > > On Tue, Mar 24, 2020 at 08:49:27AM +0000, Tian, Kevin wrote: > > > From: Jan Beulich <jbeulich@suse.com> > > > Sent: Tuesday, March 24, 2020 4:10 PM > > > > > > On 24.03.2020 06:41, Tian, Kevin wrote: > > > >> From: Roger Pau Monné <roger.pau@citrix.com> > > > >> Sent: Monday, March 23, 2020 10:49 PM > > > >> > > > >> On Mon, Mar 23, 2020 at 09:09:59AM +0100, Jan Beulich wrote: > > > >>> On 20.03.2020 20:07, Roger Pau Monne wrote: > > > >>>> This reverts commit f96e1469ad06b61796c60193daaeb9f8a96d7458. > > > >>>> > > > >>>> The commit is wrong, as the whole point of nvmx_update_apicv is > to > > > >>>> update the guest interrupt status field when the Ack on exit VMEXIT > > > >>>> control feature is enabled. > > > >>>> > > > >>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > > >>> > > > >>> Before anyone gets to look at the other two patches, should this > > > >>> be thrown in right away? > > > >> > > > >> I would like if possible get a confirmation from Kevin (or anyone > > > >> else) that my understanding is correct. I find the nested code very > > > >> confusing, and I've already made a mistake while trying to fix it. > > > >> That being said, this was spotted by osstest as introducing a > > > >> regression, so I guess it's safe to just toss it in now. > > > >> > > > >> FWIW patch 2/3 attempts to provide a description of my > understanding > > > >> of how nvmx_update_apicv works. > > > >> > > > > > > > > I feel it is not good to take this patch alone, as it was introduced to fix > > > > another problem. W/o understanding whether the whole series can > > > > fix both old and new problems, we may risk putting nested interrupt > > > > logic in an even worse state... > > > > > > Well, okay, I'll wait then, but it would seem to me that reverting > > > wouldn't put us in a worse state than we were in before that change > > > was put in. > > > > Roger needs to make the call, i.e. which problem is more severe, old or > > new one. > > AFAICT those problems affect different kind of hardware, so it doesn't > make much difference. IMO f96e1469ad06b6 should be reverted because > it's plain wrong, and albeit it seemed to fix the issue in one of my > boxes it was just luck. > > I don't thinks it's going to make matters worse, as osstest is already > blocked, but reverting it alone without the rest of the series is not > going to unblock osstest either. If you agree it's wrong I think it > could go in now, even if it's just to avoid having to repost it with > newer versions of the series. > yes, I agree it's wrong. Reviewed-by: Kevin Tian <kevin.tian@intel.com>
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c index f049920196..1b8461ba30 100644 --- a/xen/arch/x86/hvm/vmx/vvmx.c +++ b/xen/arch/x86/hvm/vmx/vvmx.c @@ -1456,12 +1456,7 @@ static void virtual_vmexit(struct cpu_user_regs *regs) /* updating host cr0 to sync TS bit */ __vmwrite(HOST_CR0, v->arch.hvm.vmx.host_cr0); - if ( cpu_has_vmx_virtual_intr_delivery && - /* - * Only inject the vector if the Ack on exit bit is not set, else the - * interrupt will be signaled in the vmcs VM_EXIT_INTR_INFO field. - */ - !(get_vvmcs(v, VM_EXIT_CONTROLS) & VM_EXIT_ACK_INTR_ON_EXIT) ) + if ( cpu_has_vmx_virtual_intr_delivery ) nvmx_update_apicv(v); nvcpu->nv_vmswitch_in_progress = 0;
This reverts commit f96e1469ad06b61796c60193daaeb9f8a96d7458. The commit is wrong, as the whole point of nvmx_update_apicv is to update the guest interrupt status field when the Ack on exit VMEXIT control feature is enabled. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/x86/hvm/vmx/vvmx.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)