Re: [Xen-devel] [PATCH v2 1/4] x86/vvmx: fix virtual interrupt injection when Ack on exit control is used

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Jan Beulich
> Sent: Tuesday, February 4, 2020 5:50 PM
> 
> On 04.02.2020 10:23, Roger Pau Monné wrote:
> > On Tue, Feb 04, 2020 at 01:35:05AM +0000, Tian, Kevin wrote:
> >>> From: Roger Pau Monne <roger.pau@xxxxxxxxxx>
> >>> Sent: Monday, February 3, 2020 8:19 PM
> >>>
> >>> When doing a virtual vmexit (ie: a vmexit handled by the L1 VMM)
> >>> interrupts shouldn't be injected using the virtual interrupt delivery
> >>> mechanism unless the Ack on exit vmexit control bit isn't set in the
> >>> nested vmcs.
> >>>
> >>> Gate the call to nvmx_update_apicv helper on whether the nested vmcs
> >>> has the Ack on exit bit set in the vmexit control field.
> >>>
> >>> Note that this fixes the usage of x2APIC by the L1 VMM, at least when
> >>> the L1 VMM is Xen.
> >>>
> >>> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> >>
> >> Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>, with a small comment:
> >>
> >>> ---
> >>> Changes since v1:
> >>>  - Call nvmx_update_apicv if the "Ack on exit" vmexit control bit
> >>>    isn't set.
> >>> ---
> >>>  xen/arch/x86/hvm/vmx/vvmx.c | 7 ++++++-
> >>>  1 file changed, 6 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c
> b/xen/arch/x86/hvm/vmx/vvmx.c
> >>> index d8ab167d62..3d97a293b2 100644
> >>> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> >>> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> >>> @@ -1393,7 +1393,12 @@ 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 )
> >>> +    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) )
> >>
> >> It's a bit odd to put comment between code lines. Can you move it to
> >> the line before 'if'?
> >
> > I usually try to keep the comments as close to the code it references
> > as possible, even if that means adding them in the middle of an if
> > condition.
> >
> > I don't mind moving it, but maybe the committer can do it at commit
> > time?
> 
> This should be easy enough, but - Kevin, I agree with Roger that
> the chose placement is better than what you suggest. You're the
> maintainer of the file, so if you really think the comment should
> be moved - so be it. But please re-confirm one or the other way.
> 

Then leave it as it is. I'm not strong on this request.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel