Re: [Xen-devel] [PATCH v9 1/8] VMX: Permanently assign PI hook vmx_pi_switch_to()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.03.17 at 01:01, <chao.gao@xxxxxxxxx> wrote:
> On Tue, Feb 28, 2017 at 09:43:09AM -0700, Jan Beulich wrote:
>>>>> On 27.02.17 at 02:45, <chao.gao@xxxxxxxxx> wrote:
>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>> @@ -260,9 +260,15 @@ void vmx_pi_hooks_deassign(struct domain *d)
>>>  
>>>      ASSERT(d->arch.hvm_domain.pi_ops.vcpu_block);
>>>  
>>> +    /*
>>> +     * Note that we don't set 'd->arch.hvm_domain.pi_ops.switch_to' to NULL
>>> +     * here. If we deassign the hooks while the vCPU is runnable in the
>>> +     * runqueue with 'SN' set, all the future notification event will be
>>> +     * suppressed. Preserving the 'switch_to' hook function can make sure
>>> +     * event time the vCPU is running the 'SN' field is cleared.
>>> +     */
>>
>>Did we now lose the remark indicating that at least in theory
>>we could remove the hook after it had run one more time? It's
> 
> I also think it only need to run one more time, because the hook
> function that set 'SN' bit is already removed.
> 
>>also not really becoming clear why SN continues to matter
>>after device removal, but perhaps that's just because of my
>>so far limited understanding of PI.
>>
>>Also I think you mean "every time" on the last line, albeit that
>>(as per my remark above) is irrelevant - we need it to run just
>>once more.
> 
> I would like to make the comment as clear as possible.
> How about the underlying comment,
> 
> Note that we don't set 'd->arch.hvm_domain.pi_ops.switch_to' to NULL
> here. If we deassign the hooks while the vCPU is runnable in the
> runqueue with 'SN' set, all the future notification event will be
> suppressed since vmx_deliver_posted_intr() also use 'SN' bit
> as the suppression flag. Preserving the 'switch_to' hook function can
> clear the 'SN' bit when the vCPU becomes running next time. After
> that, No matter which status(runnable, running or block) the vCPU is in,
> the 'SN' bit will keep clear for the 'switch_from' hook function that set
> the 'SN' bit has been removed. At that time, the 'switch_to' hook function
> is also useless. Considering the function doesn't do harm to the whole
> system, leave it here until we find a clean solution to deassign the 
> 'switch_to' hook function.

Sounds good to me (read: Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>).
If Kevin would give his ack, I could replace the comment while committing,
so you wouldn't need to re-send.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel