| Message ID | 1472615791-8664-2-git-send-email-feng.wu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 31.08.16 at 05:56, <feng.wu@intel.com> wrote: > --- a/xen/arch/x86/hvm/vmx/vmx.c > +++ b/xen/arch/x86/hvm/vmx/vmx.c > @@ -207,8 +207,6 @@ void vmx_pi_hooks_assign(struct domain *d) > ASSERT(!d->arch.hvm_domain.vmx.vcpu_block); > > d->arch.hvm_domain.vmx.vcpu_block = vmx_vcpu_block; > - d->arch.hvm_domain.vmx.pi_switch_from = vmx_pi_switch_from; > - d->arch.hvm_domain.vmx.pi_switch_to = vmx_pi_switch_to; > d->arch.hvm_domain.vmx.pi_do_resume = vmx_pi_do_resume; > } I'm sure I've said so before: While I can see why you want the adjustment to vmx_pi_hooks_deassign(), I don't see why leaving out this and ... > @@ -236,6 +232,12 @@ static int vmx_domain_initialise(struct domain *d) > if ( (rc = vmx_alloc_vlapic_mapping(d)) != 0 ) > return rc; > > + if ( iommu_intpost ) > + { > + d->arch.hvm_domain.vmx.pi_switch_from = vmx_pi_switch_from; > + d->arch.hvm_domain.vmx.pi_switch_to = vmx_pi_switch_to; > + } > + > return 0; > } ... this would conflict with the goal of the patch. Jan
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: Thursday, September 1, 2016 4:17 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: [PATCH v3 1/6] VMX: Statically assign two PI hooks > > >>> On 31.08.16 at 05:56, <feng.wu@intel.com> wrote: > > --- a/xen/arch/x86/hvm/vmx/vmx.c > > +++ b/xen/arch/x86/hvm/vmx/vmx.c > > @@ -207,8 +207,6 @@ void vmx_pi_hooks_assign(struct domain *d) > > ASSERT(!d->arch.hvm_domain.vmx.vcpu_block); > > > > d->arch.hvm_domain.vmx.vcpu_block = vmx_vcpu_block; > > - d->arch.hvm_domain.vmx.pi_switch_from = vmx_pi_switch_from; > > - d->arch.hvm_domain.vmx.pi_switch_to = vmx_pi_switch_to; > > d->arch.hvm_domain.vmx.pi_do_resume = vmx_pi_do_resume; > > } > > I'm sure I've said so before: While I can see why you want the > adjustment to vmx_pi_hooks_deassign(), I don't see why leaving > out this and ... As mentioned in the comments of this patch, these two hooks are also needed for CPU side PI, so we cannot install here and ... > > > @@ -236,6 +232,12 @@ static int vmx_domain_initialise(struct domain *d) > > if ( (rc = vmx_alloc_vlapic_mapping(d)) != 0 ) > > return rc; > > > > + if ( iommu_intpost ) > > + { > > + d->arch.hvm_domain.vmx.pi_switch_from = vmx_pi_switch_from; > > + d->arch.hvm_domain.vmx.pi_switch_to = vmx_pi_switch_to; > > + } > > + > > return 0; > > } > > ... this would conflict with the goal of the patch. ... Yes this is incorrect, I should install these two hooks when CPU side PI is supported. Thanks, Feng > > Jan
>>> On 01.09.16 at 11:13, <feng.wu@intel.com> wrote: >> From: Jan Beulich [mailto:JBeulich@suse.com] >> Sent: Thursday, September 1, 2016 4:17 PM >> >>> On 31.08.16 at 05:56, <feng.wu@intel.com> wrote: >> > --- a/xen/arch/x86/hvm/vmx/vmx.c >> > +++ b/xen/arch/x86/hvm/vmx/vmx.c >> > @@ -207,8 +207,6 @@ void vmx_pi_hooks_assign(struct domain *d) >> > ASSERT(!d->arch.hvm_domain.vmx.vcpu_block); >> > >> > d->arch.hvm_domain.vmx.vcpu_block = vmx_vcpu_block; >> > - d->arch.hvm_domain.vmx.pi_switch_from = vmx_pi_switch_from; >> > - d->arch.hvm_domain.vmx.pi_switch_to = vmx_pi_switch_to; >> > d->arch.hvm_domain.vmx.pi_do_resume = vmx_pi_do_resume; >> > } >> >> I'm sure I've said so before: While I can see why you want the >> adjustment to vmx_pi_hooks_deassign(), I don't see why leaving >> out this and ... > > As mentioned in the comments of this patch, these two hooks are also > needed for CPU side PI, so we cannot install here and ... I've just read it again, and to me it doesn't say so. I.e. ... >> > @@ -236,6 +232,12 @@ static int vmx_domain_initialise(struct domain *d) >> > if ( (rc = vmx_alloc_vlapic_mapping(d)) != 0 ) >> > return rc; >> > >> > + if ( iommu_intpost ) >> > + { >> > + d->arch.hvm_domain.vmx.pi_switch_from = vmx_pi_switch_from; >> > + d->arch.hvm_domain.vmx.pi_switch_to = vmx_pi_switch_to; >> > + } >> > + >> > return 0; >> > } >> >> ... this would conflict with the goal of the patch. > > ... Yes this is incorrect, I should install these two hooks when CPU side PI is > supported. ... you'd then additionally need to explain what bug there is without these hooks and with only CPU side PI present (iirc that's functionality which has been there before the VT-d part). Jan
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: Thursday, September 1, 2016 5:23 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: [PATCH v3 1/6] VMX: Statically assign two PI hooks > > >>> On 01.09.16 at 11:13, <feng.wu@intel.com> wrote: > >> From: Jan Beulich [mailto:JBeulich@suse.com] > >> Sent: Thursday, September 1, 2016 4:17 PM > >> >>> On 31.08.16 at 05:56, <feng.wu@intel.com> wrote: > >> > --- a/xen/arch/x86/hvm/vmx/vmx.c > >> > +++ b/xen/arch/x86/hvm/vmx/vmx.c > >> > @@ -207,8 +207,6 @@ void vmx_pi_hooks_assign(struct domain *d) > >> > ASSERT(!d->arch.hvm_domain.vmx.vcpu_block); > >> > > >> > d->arch.hvm_domain.vmx.vcpu_block = vmx_vcpu_block; > >> > - d->arch.hvm_domain.vmx.pi_switch_from = vmx_pi_switch_from; > >> > - d->arch.hvm_domain.vmx.pi_switch_to = vmx_pi_switch_to; > >> > d->arch.hvm_domain.vmx.pi_do_resume = vmx_pi_do_resume; > >> > } > >> > >> I'm sure I've said so before: While I can see why you want the > >> adjustment to vmx_pi_hooks_deassign(), I don't see why leaving > >> out this and ... > > > > As mentioned in the comments of this patch, these two hooks are also > > needed for CPU side PI, so we cannot install here and ... > > I've just read it again, and to me it doesn't say so. I.e. ... > > >> > @@ -236,6 +232,12 @@ static int vmx_domain_initialise(struct domain *d) > >> > if ( (rc = vmx_alloc_vlapic_mapping(d)) != 0 ) > >> > return rc; > >> > > >> > + if ( iommu_intpost ) > >> > + { > >> > + d->arch.hvm_domain.vmx.pi_switch_from = vmx_pi_switch_from; > >> > + d->arch.hvm_domain.vmx.pi_switch_to = vmx_pi_switch_to; > >> > + } > >> > + > >> > return 0; > >> > } > >> > >> ... this would conflict with the goal of the patch. > > > > ... Yes this is incorrect, I should install these two hooks when CPU side PI is > > supported. > > ... you'd then additionally need to explain what bug there is without > these hooks and with only CPU side PI present (iirc that's functionality > which has been there before the VT-d part). Oh, my bad, I mixed things up. Then I should install these two hooks when the first device assignment happens and don't zap it after that. Thanks for the comments! Thanks, Feng > > Jan
On Wed, 2016-08-31 at 11:56 +0800, Feng Wu wrote: > PI hooks: vmx_pi_switch_from() and vmx_pi_switch_to() are > needed even all the assigned devices were dettached from > the domain. > maybe "are needed even when any previously passed through device is detached from the domain" (or something like that)? > We change the state of SN bit in these two > functions, and 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. > Which, if SN is used only for controlling VT-d PI from passed thru devices does not sound like an issue to me. What I sort of get from the discussion you had with Jan, however, is that this is an issue, because SN is also used for other things, i.e., it is indeed useful even when there are no passed thru device, is that the case? If yes, I think this deserves at least a quick mention in the sentence above. Regards, Dario
> -----Original Message----- > From: Dario Faggioli [mailto:dario.faggioli@citrix.com] > Sent: Tuesday, September 6, 2016 4:43 PM > To: Wu, Feng <feng.wu@intel.com>; xen-devel@lists.xen.org > Cc: Tian, Kevin <kevin.tian@intel.com>; george.dunlap@eu.citrix.com; > andrew.cooper3@citrix.com; jbeulich@suse.com > Subject: Re: [Xen-devel] [PATCH v3 1/6] VMX: Statically assign two PI hooks > > On Wed, 2016-08-31 at 11:56 +0800, Feng Wu wrote: > > PI hooks: vmx_pi_switch_from() and vmx_pi_switch_to() are > > needed even all the assigned devices were dettached from > > the domain. > > > maybe "are needed even when any previously passed through device is > detached from the domain" (or something like that)? Looks good, thanks for improve the wording. > > > We change the state of SN bit in these two > > functions, and 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. > > > Which, if SN is used only for controlling VT-d PI from passed thru > devices does not sound like an issue to me. > > What I sort of get from the discussion you had with Jan, however, is > that this is an issue, because SN is also used for other things, i.e., > it is indeed useful even when there are no passed thru device, is that > the case? > > If yes, I think this deserves at least a quick mention in the sentence > above. Yes, SN controls all the PI including CPU side PI activity, sure, I will explicitly add those information. Thanks, Feng > > Regards, > Dario > -- > <<This happens because I choose it to happen!>> (Raistlin Majere) > ----------------------------------------------------------------- > Dario Faggioli, Ph.D, http://about.me/dario.faggioli > Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index 3d330b6..f5d2d3c 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -207,8 +207,6 @@ void vmx_pi_hooks_assign(struct domain *d) ASSERT(!d->arch.hvm_domain.vmx.vcpu_block); d->arch.hvm_domain.vmx.vcpu_block = vmx_vcpu_block; - d->arch.hvm_domain.vmx.pi_switch_from = vmx_pi_switch_from; - d->arch.hvm_domain.vmx.pi_switch_to = vmx_pi_switch_to; d->arch.hvm_domain.vmx.pi_do_resume = vmx_pi_do_resume; } @@ -221,8 +219,6 @@ 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; } @@ -236,6 +232,12 @@ static int vmx_domain_initialise(struct domain *d) if ( (rc = vmx_alloc_vlapic_mapping(d)) != 0 ) return rc; + if ( iommu_intpost ) + { + d->arch.hvm_domain.vmx.pi_switch_from = vmx_pi_switch_from; + d->arch.hvm_domain.vmx.pi_switch_to = vmx_pi_switch_to; + } + return 0; }
PI hooks: vmx_pi_switch_from() and vmx_pi_switch_to() are needed even all the assigned devices were dettached from the domain. We change the state of SN bit in these two functions, and 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> --- xen/arch/x86/hvm/vmx/vmx.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)