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

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

> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Wednesday, March 01, 2017 3:42 PM
> 
> >>> 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

Good to me too.

Acked-by: Kevin Tian <kevin.tian@xxxxxxxxx>

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