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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel