| Message ID | 1477622259-3476-2-git-send-email-feng.wu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 28.10.16 at 04:37, <feng.wu@intel.com> wrote: > PI hook vmx_pi_switch_to() is needed even when any previously > assigned device is detached from the domain. Since 'SN' bit is > also used to control the CPU side PI and we change the state of > SN bit in these two functions, There's just one function you've mentioned up to here. > --- a/xen/arch/x86/hvm/vmx/vmx.c > +++ b/xen/arch/x86/hvm/vmx/vmx.c > @@ -221,9 +221,14 @@ void vmx_pi_hooks_deassign(struct domain *d) > ASSERT(d->arch.hvm_domain.vmx.vcpu_block); > > d->arch.hvm_domain.vmx.vcpu_block = NULL; > - d->arch.hvm_domain.vmx.pi_switch_from = NULL; > - d->arch.hvm_domain.vmx.pi_switch_to = NULL; And with the commit message as it is right now, it is completely unclear why the from hook also gets zapped. In fact, with the description pointing out a problem with just the SN=1 case, I don't see what you need the from hook for without devices assigned, as that's where SN gets set (and you want to avoid that). > d->arch.hvm_domain.vmx.pi_do_resume = NULL; > + d->arch.hvm_domain.vmx.pi_switch_from = NULL; > + > + /* > + * In fact, we can remove 'vmx_pi_switch_to' inside itself if no new device s/can/could/ > + * is in the process of getting assigned and "from" hook is NULL. However, > + * it is not straightforward to find a clear solution, so just leave it here. > + */ > } > > static int vmx_domain_initialise(struct domain *d) Jan
>>> On 28.10.16 at 15:04, <JBeulich@suse.com> wrote: >>>> On 28.10.16 at 04:37, <feng.wu@intel.com> wrote: >> --- a/xen/arch/x86/hvm/vmx/vmx.c >> +++ b/xen/arch/x86/hvm/vmx/vmx.c >> @@ -221,9 +221,14 @@ void vmx_pi_hooks_deassign(struct domain *d) >> ASSERT(d->arch.hvm_domain.vmx.vcpu_block); >> >> d->arch.hvm_domain.vmx.vcpu_block = NULL; >> - d->arch.hvm_domain.vmx.pi_switch_from = NULL; >> - d->arch.hvm_domain.vmx.pi_switch_to = NULL; > > And with the commit message as it is right now, it is completely > unclear why the from hook also gets zapped. In fact, with the > description pointing out a problem with just the SN=1 case, I > don't see what you need the from hook for without devices > assigned, as that's where SN gets set (and you want to avoid > that). > >> d->arch.hvm_domain.vmx.pi_do_resume = NULL; >> + d->arch.hvm_domain.vmx.pi_switch_from = NULL; Oh, I'm sorry - I paid attention only to the lines getting removed above, not the one line getting re-inserted here. That addresses my comment, of course; I just can't see why the line needs to be moved. Jan
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: Friday, October 28, 2016 9:18 PM > To: Wu, Feng <feng.wu@intel.com> > Cc: andrew.cooper3@citrix.com; dario.faggioli@citrix.com; > george.dunlap@eu.citrix.com; Tian, Kevin <kevin.tian@intel.com>; xen- > devel@lists.xen.org > Subject: Re: [Xen-devel] [PATCH v6 1/7] VMX: Permanently assign PI hook > vmx_pi_switch_to() > > >>> On 28.10.16 at 15:04, <JBeulich@suse.com> wrote: > >>>> On 28.10.16 at 04:37, <feng.wu@intel.com> wrote: > >> --- a/xen/arch/x86/hvm/vmx/vmx.c > >> +++ b/xen/arch/x86/hvm/vmx/vmx.c > >> @@ -221,9 +221,14 @@ void vmx_pi_hooks_deassign(struct domain *d) > >> ASSERT(d->arch.hvm_domain.vmx.vcpu_block); > >> > >> d->arch.hvm_domain.vmx.vcpu_block = NULL; > >> - d->arch.hvm_domain.vmx.pi_switch_from = NULL; > >> - d->arch.hvm_domain.vmx.pi_switch_to = NULL; > > > > And with the commit message as it is right now, it is completely > > unclear why the from hook also gets zapped. In fact, with the > > description pointing out a problem with just the SN=1 case, I > > don't see what you need the from hook for without devices > > assigned, as that's where SN gets set (and you want to avoid > > that). > > > >> d->arch.hvm_domain.vmx.pi_do_resume = NULL; > >> + d->arch.hvm_domain.vmx.pi_switch_from = NULL; > > Oh, I'm sorry - I paid attention only to the lines getting removed > above, not the one line getting re-inserted here. That addresses > my comment, of course; I just can't see why the line needs to be > moved. Yes, we could just reuse the original line. Thanks! Thanks, Feng > > Jan
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index 3d330b6..faaa987 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -221,9 +221,14 @@ void vmx_pi_hooks_deassign(struct domain *d) ASSERT(d->arch.hvm_domain.vmx.vcpu_block); d->arch.hvm_domain.vmx.vcpu_block = NULL; - d->arch.hvm_domain.vmx.pi_switch_from = NULL; - d->arch.hvm_domain.vmx.pi_switch_to = NULL; d->arch.hvm_domain.vmx.pi_do_resume = NULL; + d->arch.hvm_domain.vmx.pi_switch_from = NULL; + + /* + * In fact, we can remove 'vmx_pi_switch_to' inside itself if no new device + * is in the process of getting assigned and "from" hook is NULL. However, + * it is not straightforward to find a clear solution, so just leave it here. + */ } static int vmx_domain_initialise(struct domain *d)
PI hook vmx_pi_switch_to() is needed even when any previously assigned device is detached from the domain. Since 'SN' bit is also used to control the CPU side PI and we change the state of SN bit in these two functions, then evaluate this bit in vmx_deliver_posted_intr() when trying to deliver the interrupt in posted way via software. The problem is if we deassign the hooks while the vCPU is runnable in the runqueue with 'SN' set, all the furture notificaton event will be suppressed. This patch makes these two hooks statically assigned. Signed-off-by: Feng Wu <feng.wu@intel.com> --- v6: - Adjust the comments and wording. v5: - Zap "pi_switch_from" hook v4: - Don't zap vmx_pi_switch_from() and vmx_pi_switch_to() when any previously assigned device is detached from the domain. - Comments changes. xen/arch/x86/hvm/vmx/vmx.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)