| Message ID | 1490764315-7162-2-git-send-email-chao.gao@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Mar 31, 2017 at 01:28:02PM +0800, Tian, Kevin wrote: >> From: Chao Gao >> Sent: Wednesday, March 29, 2017 1:12 PM >> >> When a vCPU migrated to another pCPU, pt irqs binded to this vCPU also >> needed migration. When VT-d PI is enabled, interrupt vector will be recorded >> to a main memory resident data-structure and a notification whose >> destination is decided by NDST is generated. NDST is properly adjusted >> during vCPU migration so pirq directly injected to guest needn't be migrated. >> >> This patch adds a indicator, @posted, to show whether the pt irq is delivered >> through VT-d PI. >> >> Signed-off-by: Chao Gao <chao.gao@intel.com> >> >> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index >> 0282986..2d8de16 100644 >> --- a/xen/arch/x86/hvm/hvm.c >> +++ b/xen/arch/x86/hvm/hvm.c >> @@ -438,6 +438,9 @@ static int hvm_migrate_pirq(struct domain *d, struct >> hvm_pirq_dpci *pirq_dpci, >> struct vcpu *v = arg; >> >> if ( (pirq_dpci->flags & HVM_IRQ_DPCI_MACH_MSI) && >> + (pirq_dpci->flags & HVM_IRQ_DPCI_GUEST_MSI) && >> + /* Needn't migrate pirq if this pirq is delivered to guest directly.*/ >> + (!pirq_dpci->gmsi.posted) && >> (pirq_dpci->gmsi.dest_vcpu_id == v->vcpu_id) ) > >simply looking at above change it's more than what you intend to change. >Previously even w/o GUEST_MSI flag will fall into that path, but now >you limit it to only GUEST_MSI and irq remapping (i.e. changed the >behavior for both posted case and w/o GUEST_MSI case). I haven't looked >whether MACH_MASI always set with GUEST_MSI, but my gut-feeling >looks not correct here. Yes. It's a problem. I think the original code may be wrong for it acquires gmsi.dest_vcpu_id without checking 'flags' of pirq_dpci. Do we need to migrate pirq when its type is GUEST_PCI? Thanks Chao
On Fri, Mar 31, 2017 at 03:31:47AM -0600, Jan Beulich wrote: >>>> On 29.03.17 at 07:11, <chao.gao@intel.com> wrote: >> @@ -442,17 +397,24 @@ int pt_irq_create_bind( >> dest_vcpu_id = hvm_girq_dest_2_vcpu_id(d, dest, dest_mode); >> pirq_dpci->gmsi.dest_vcpu_id = dest_vcpu_id; >> spin_unlock(&d->event_lock); >> + >> + pirq_dpci->gmsi.posted = false; >> + vcpu = (dest_vcpu_id >= 0) ? d->vcpu[dest_vcpu_id] : NULL; >> + if ( iommu_intpost && (delivery_mode == dest_LowestPrio) ) > >Why again would dest_Fixed not allow posted delivery? This needs No this restriction. For dest_Fixed case, hvm_girq_dest_2_vcpu_id() gives the same output with pi_find_dest_vcpu(). Thus we don't call it again, just use the return value of hvm_girq_dest_2_vcpu_id(). >recording in a comment, if there really is such a restriction. Or did >you really mean to place ... > >> + { >> + vcpu = vector_hashing_dest(d, dest, dest_mode, >> + pirq_dpci->gmsi.gvec); >> + if ( vcpu ) >> + pirq_dpci->gmsi.posted = true; > >... this ... > >> + } > >... after this brace (which then wouldn't be needed anymore)? If >so, is there any point calling vector_hashing_dest() when vcpu is >already non-NULL prior to the if()? > >This then also raises the question whether the call to >hvm_girq_dest_2_vcpu_id() is actually legitimate for lowest >priority delivery mode. For lowest priority delivery mode, if VT-d PI is enabled, the result (the destination vcpu) is overrided by vector_hashing_dest() to keep the existing behavior. I think the only point we should keep in mind is for cases other than lowest priority delivery mode, pi_find_dest_vcpu() and hvm_girq_dest_2_vcpu_id() give the same destination vcpu. Thanks Chao > >Jan >
On Fri, Mar 31, 2017 at 04:06:27AM -0600, Jan Beulich wrote: >>>> On 31.03.17 at 04:42, <chao.gao@intel.com> wrote: >> On Fri, Mar 31, 2017 at 03:31:47AM -0600, Jan Beulich wrote: >>>>>> On 29.03.17 at 07:11, <chao.gao@intel.com> wrote: >>>> @@ -442,17 +397,24 @@ int pt_irq_create_bind( >>>> dest_vcpu_id = hvm_girq_dest_2_vcpu_id(d, dest, dest_mode); >>>> pirq_dpci->gmsi.dest_vcpu_id = dest_vcpu_id; >>>> spin_unlock(&d->event_lock); >>>> + >>>> + pirq_dpci->gmsi.posted = false; >>>> + vcpu = (dest_vcpu_id >= 0) ? d->vcpu[dest_vcpu_id] : NULL; >>>> + if ( iommu_intpost && (delivery_mode == dest_LowestPrio) ) >>> >>>Why again would dest_Fixed not allow posted delivery? This needs >> >> No this restriction. For dest_Fixed case, hvm_girq_dest_2_vcpu_id() gives >> the same output with pi_find_dest_vcpu(). Thus we don't call it again, just >> use the return value of hvm_girq_dest_2_vcpu_id(). > >But as pointed out you don't set the new posted field in that case. > Indeed. How about this: pirq_dpci->gmsi.posted = false; if ( iommu_intpost ) { vcpu = (dest_vcpu_id >= 0) ? d->vcpu[dest_vcpu_id] : NULL; if ( delivery_mode == dest_LowestPrio ) vcpu = vector_hashing_dest(d, dest, dest_mode, pirq_dpci->gmsi.gvec); if ( vcpu ) pirq_dpci->gmsi.posted = true; } >>>recording in a comment, if there really is such a restriction. Or did >>>you really mean to place ... >>> >>>> + { >>>> + vcpu = vector_hashing_dest(d, dest, dest_mode, >>>> + pirq_dpci->gmsi.gvec); >>>> + if ( vcpu ) >>>> + pirq_dpci->gmsi.posted = true; >>> >>>... this ... >>> >>>> + } >>> >>>... after this brace (which then wouldn't be needed anymore)? If >>>so, is there any point calling vector_hashing_dest() when vcpu is >>>already non-NULL prior to the if()? >>> >>>This then also raises the question whether the call to >>>hvm_girq_dest_2_vcpu_id() is actually legitimate for lowest >>>priority delivery mode. >> >> For lowest priority delivery mode, if VT-d PI is enabled, the result (the >> destination vcpu) is overrided by vector_hashing_dest() to keep the >> existing behavior. I think the only point we should keep in mind is >> for cases other than lowest priority delivery mode, pi_find_dest_vcpu() >> and hvm_girq_dest_2_vcpu_id() give the same destination vcpu. > >Well, the override is done for the iommu_intpost case. The remark >on hvm_girq_dest_2_vcpu_id(), however, was made in general. Ok. You meant the method using in hvm_girq_dest_2_vcpu_id() may not match the method used by real hardware. I will check it. Thanks Chao > >Jan >
> From: Chao Gao > Sent: Wednesday, March 29, 2017 1:12 PM > > When a vCPU migrated to another pCPU, pt irqs binded to this vCPU also > needed migration. When VT-d PI is enabled, interrupt vector will be recorded > to a main memory resident data-structure and a notification whose > destination is decided by NDST is generated. NDST is properly adjusted > during vCPU migration so pirq directly injected to guest needn't be migrated. > > This patch adds a indicator, @posted, to show whether the pt irq is delivered > through VT-d PI. > > Signed-off-by: Chao Gao <chao.gao@intel.com> > --- > v11: > - rename the indicator to 'posted' > - move setting 'posted' field to event lock un-locked region. > > v10: > - Newly added. > > xen/arch/x86/hvm/hvm.c | 3 +++ > xen/drivers/passthrough/io.c | 62 +++++++++----------------------------------- > xen/include/xen/hvm/irq.h | 1 + > 3 files changed, 16 insertions(+), 50 deletions(-) > > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index > 0282986..2d8de16 100644 > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -438,6 +438,9 @@ static int hvm_migrate_pirq(struct domain *d, struct > hvm_pirq_dpci *pirq_dpci, > struct vcpu *v = arg; > > if ( (pirq_dpci->flags & HVM_IRQ_DPCI_MACH_MSI) && > + (pirq_dpci->flags & HVM_IRQ_DPCI_GUEST_MSI) && > + /* Needn't migrate pirq if this pirq is delivered to guest directly.*/ > + (!pirq_dpci->gmsi.posted) && > (pirq_dpci->gmsi.dest_vcpu_id == v->vcpu_id) ) simply looking at above change it's more than what you intend to change. Previously even w/o GUEST_MSI flag will fall into that path, but now you limit it to only GUEST_MSI and irq remapping (i.e. changed the behavior for both posted case and w/o GUEST_MSI case). I haven't looked whether MACH_MASI always set with GUEST_MSI, but my gut-feeling looks not correct here. > { > struct irq_desc *desc = > diff --git a/xen/drivers/passthrough/io.c b/xen/drivers/passthrough/io.c > index 080183e..d53976c 100644 > --- a/xen/drivers/passthrough/io.c > +++ b/xen/drivers/passthrough/io.c > @@ -259,52 +259,6 @@ static struct vcpu *vector_hashing_dest(const struct > domain *d, > return dest; > } > > -/* > - * The purpose of this routine is to find the right destination vCPU for > - * an interrupt which will be delivered by VT-d posted-interrupt. There > - * are several cases as below: > - * > - * - For lowest-priority interrupts, use vector-hashing mechanism to find > - * the destination. > - * - Otherwise, for single destination interrupt, it is straightforward to > - * find the destination vCPU and return true. > - * - For multicast/broadcast vCPU, we cannot handle it via interrupt posting, > - * so return NULL. > - */ > -static struct vcpu *pi_find_dest_vcpu(const struct domain *d, uint32_t > dest_id, > - bool_t dest_mode, uint8_t delivery_mode, > - uint8_t gvec) > -{ > - unsigned int dest_vcpus = 0; > - struct vcpu *v, *dest = NULL; > - > - switch ( delivery_mode ) > - { > - case dest_LowestPrio: > - return vector_hashing_dest(d, dest_id, dest_mode, gvec); > - case dest_Fixed: > - for_each_vcpu ( d, v ) > - { > - if ( !vlapic_match_dest(vcpu_vlapic(v), NULL, APIC_DEST_NOSHORT, > - dest_id, dest_mode) ) > - continue; > - > - dest_vcpus++; > - dest = v; > - } > - > - /* For fixed mode, we only handle single-destination interrupts. */ > - if ( dest_vcpus == 1 ) > - return dest; > - > - break; > - default: > - break; > - } > - > - return NULL; > -} > - > int pt_irq_create_bind( > struct domain *d, xen_domctl_bind_pt_irq_t *pt_irq_bind) { @@ -365,6 > +319,7 @@ int pt_irq_create_bind( > { > uint8_t dest, dest_mode, delivery_mode; > int dest_vcpu_id; > + const struct vcpu *vcpu; > > if ( !(pirq_dpci->flags & HVM_IRQ_DPCI_MAPPED) ) > { > @@ -442,17 +397,24 @@ int pt_irq_create_bind( > dest_vcpu_id = hvm_girq_dest_2_vcpu_id(d, dest, dest_mode); > pirq_dpci->gmsi.dest_vcpu_id = dest_vcpu_id; > spin_unlock(&d->event_lock); > + > + pirq_dpci->gmsi.posted = false; > + vcpu = (dest_vcpu_id >= 0) ? d->vcpu[dest_vcpu_id] : NULL; > + if ( iommu_intpost && (delivery_mode == dest_LowestPrio) ) > + { > + vcpu = vector_hashing_dest(d, dest, dest_mode, > + pirq_dpci->gmsi.gvec); > + if ( vcpu ) > + pirq_dpci->gmsi.posted = true; > + } > if ( dest_vcpu_id >= 0 ) > hvm_migrate_pirqs(d->vcpu[dest_vcpu_id]); > > /* Use interrupt posting if it is supported. */ > if ( iommu_intpost ) > { > - const struct vcpu *vcpu = pi_find_dest_vcpu(d, dest, dest_mode, > - delivery_mode, pirq_dpci->gmsi.gvec); > - > if ( vcpu ) > - pi_update_irte( vcpu, info, pirq_dpci->gmsi.gvec ); > + pi_update_irte(vcpu, info, pirq_dpci->gmsi.gvec); > else > dprintk(XENLOG_G_INFO, > "%pv: deliver interrupt in remapping mode,gvec:%02x\n", diff - > -git a/xen/include/xen/hvm/irq.h b/xen/include/xen/hvm/irq.h index > d3f8623..566854a 100644 > --- a/xen/include/xen/hvm/irq.h > +++ b/xen/include/xen/hvm/irq.h > @@ -63,6 +63,7 @@ struct hvm_gmsi_info { > uint32_t gvec; > uint32_t gflags; > int dest_vcpu_id; /* -1 :multi-dest, non-negative: dest_vcpu_id */ > + bool posted; /* directly deliver to guest via VT-d PI? */ > }; > > struct hvm_girq_dpci_mapping { > -- > 1.8.3.1 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel
>>> On 29.03.17 at 07:11, <chao.gao@intel.com> wrote: > @@ -442,17 +397,24 @@ int pt_irq_create_bind( > dest_vcpu_id = hvm_girq_dest_2_vcpu_id(d, dest, dest_mode); > pirq_dpci->gmsi.dest_vcpu_id = dest_vcpu_id; > spin_unlock(&d->event_lock); > + > + pirq_dpci->gmsi.posted = false; > + vcpu = (dest_vcpu_id >= 0) ? d->vcpu[dest_vcpu_id] : NULL; > + if ( iommu_intpost && (delivery_mode == dest_LowestPrio) ) Why again would dest_Fixed not allow posted delivery? This needs recording in a comment, if there really is such a restriction. Or did you really mean to place ... > + { > + vcpu = vector_hashing_dest(d, dest, dest_mode, > + pirq_dpci->gmsi.gvec); > + if ( vcpu ) > + pirq_dpci->gmsi.posted = true; ... this ... > + } ... after this brace (which then wouldn't be needed anymore)? If so, is there any point calling vector_hashing_dest() when vcpu is already non-NULL prior to the if()? This then also raises the question whether the call to hvm_girq_dest_2_vcpu_id() is actually legitimate for lowest priority delivery mode. Jan
>>> On 31.03.17 at 04:42, <chao.gao@intel.com> wrote: > On Fri, Mar 31, 2017 at 03:31:47AM -0600, Jan Beulich wrote: >>>>> On 29.03.17 at 07:11, <chao.gao@intel.com> wrote: >>> @@ -442,17 +397,24 @@ int pt_irq_create_bind( >>> dest_vcpu_id = hvm_girq_dest_2_vcpu_id(d, dest, dest_mode); >>> pirq_dpci->gmsi.dest_vcpu_id = dest_vcpu_id; >>> spin_unlock(&d->event_lock); >>> + >>> + pirq_dpci->gmsi.posted = false; >>> + vcpu = (dest_vcpu_id >= 0) ? d->vcpu[dest_vcpu_id] : NULL; >>> + if ( iommu_intpost && (delivery_mode == dest_LowestPrio) ) >> >>Why again would dest_Fixed not allow posted delivery? This needs > > No this restriction. For dest_Fixed case, hvm_girq_dest_2_vcpu_id() gives > the same output with pi_find_dest_vcpu(). Thus we don't call it again, just > use the return value of hvm_girq_dest_2_vcpu_id(). But as pointed out you don't set the new posted field in that case. >>recording in a comment, if there really is such a restriction. Or did >>you really mean to place ... >> >>> + { >>> + vcpu = vector_hashing_dest(d, dest, dest_mode, >>> + pirq_dpci->gmsi.gvec); >>> + if ( vcpu ) >>> + pirq_dpci->gmsi.posted = true; >> >>... this ... >> >>> + } >> >>... after this brace (which then wouldn't be needed anymore)? If >>so, is there any point calling vector_hashing_dest() when vcpu is >>already non-NULL prior to the if()? >> >>This then also raises the question whether the call to >>hvm_girq_dest_2_vcpu_id() is actually legitimate for lowest >>priority delivery mode. > > For lowest priority delivery mode, if VT-d PI is enabled, the result (the > destination vcpu) is overrided by vector_hashing_dest() to keep the > existing behavior. I think the only point we should keep in mind is > for cases other than lowest priority delivery mode, pi_find_dest_vcpu() > and hvm_girq_dest_2_vcpu_id() give the same destination vcpu. Well, the override is done for the iommu_intpost case. The remark on hvm_girq_dest_2_vcpu_id(), however, was made in general. Jan
>>> On 31.03.17 at 01:10, <chao.gao@intel.com> wrote: > On Fri, Mar 31, 2017 at 01:28:02PM +0800, Tian, Kevin wrote: >>> From: Chao Gao >>> Sent: Wednesday, March 29, 2017 1:12 PM >>> --- a/xen/arch/x86/hvm/hvm.c >>> +++ b/xen/arch/x86/hvm/hvm.c >>> @@ -438,6 +438,9 @@ static int hvm_migrate_pirq(struct domain *d, struct >>> hvm_pirq_dpci *pirq_dpci, >>> struct vcpu *v = arg; >>> >>> if ( (pirq_dpci->flags & HVM_IRQ_DPCI_MACH_MSI) && >>> + (pirq_dpci->flags & HVM_IRQ_DPCI_GUEST_MSI) && >>> + /* Needn't migrate pirq if this pirq is delivered to guest directly.*/ >>> + (!pirq_dpci->gmsi.posted) && >>> (pirq_dpci->gmsi.dest_vcpu_id == v->vcpu_id) ) >> >>simply looking at above change it's more than what you intend to change. >>Previously even w/o GUEST_MSI flag will fall into that path, but now >>you limit it to only GUEST_MSI and irq remapping (i.e. changed the >>behavior for both posted case and w/o GUEST_MSI case). I haven't looked >>whether MACH_MASI always set with GUEST_MSI, but my gut-feeling >>looks not correct here. > > Yes. It's a problem. I think the original code may be wrong for it acquires > gmsi.dest_vcpu_id without checking 'flags' of pirq_dpci. Do we need to migrate > pirq when its type is GUEST_PCI? It probably would be nice, but we don't know where to migrate it to: The names in "pirq_dpci->gmsi.dest_vcpu_id" should make this pretty clear, I would think. Jan
>>> On 31.03.17 at 05:27, <chao.gao@intel.com> wrote: > On Fri, Mar 31, 2017 at 04:06:27AM -0600, Jan Beulich wrote: >>>>> On 31.03.17 at 04:42, <chao.gao@intel.com> wrote: >>> On Fri, Mar 31, 2017 at 03:31:47AM -0600, Jan Beulich wrote: >>>>>>> On 29.03.17 at 07:11, <chao.gao@intel.com> wrote: >>>>> @@ -442,17 +397,24 @@ int pt_irq_create_bind( >>>>> dest_vcpu_id = hvm_girq_dest_2_vcpu_id(d, dest, dest_mode); >>>>> pirq_dpci->gmsi.dest_vcpu_id = dest_vcpu_id; >>>>> spin_unlock(&d->event_lock); >>>>> + >>>>> + pirq_dpci->gmsi.posted = false; >>>>> + vcpu = (dest_vcpu_id >= 0) ? d->vcpu[dest_vcpu_id] : NULL; >>>>> + if ( iommu_intpost && (delivery_mode == dest_LowestPrio) ) >>>> >>>>Why again would dest_Fixed not allow posted delivery? This needs >>> >>> No this restriction. For dest_Fixed case, hvm_girq_dest_2_vcpu_id() gives >>> the same output with pi_find_dest_vcpu(). Thus we don't call it again, just >>> use the return value of hvm_girq_dest_2_vcpu_id(). >> >>But as pointed out you don't set the new posted field in that case. >> > > Indeed. > > How about this: > pirq_dpci->gmsi.posted = false; > if ( iommu_intpost ) > { > vcpu = (dest_vcpu_id >= 0) ? d->vcpu[dest_vcpu_id] : NULL; > if ( delivery_mode == dest_LowestPrio ) > vcpu = vector_hashing_dest(d, dest, dest_mode, > pirq_dpci->gmsi.gvec); > if ( vcpu ) > pirq_dpci->gmsi.posted = true; > } I think the setting of vcpu needs to stay outside the if(), but other than that the above looks reasonable at a first glance. >>>>This then also raises the question whether the call to >>>>hvm_girq_dest_2_vcpu_id() is actually legitimate for lowest >>>>priority delivery mode. >>> >>> For lowest priority delivery mode, if VT-d PI is enabled, the result (the >>> destination vcpu) is overrided by vector_hashing_dest() to keep the >>> existing behavior. I think the only point we should keep in mind is >>> for cases other than lowest priority delivery mode, pi_find_dest_vcpu() >>> and hvm_girq_dest_2_vcpu_id() give the same destination vcpu. >> >>Well, the override is done for the iommu_intpost case. The remark >>on hvm_girq_dest_2_vcpu_id(), however, was made in general. > > Ok. You meant the method using in hvm_girq_dest_2_vcpu_id() may not match > the method > used by real hardware. I will check it. I mean that the method used there is pretty clearly "fixed" mode handling. Thanks for checking. Jan
On Fri, Mar 31, 2017 at 04:38:02AM -0600, Jan Beulich wrote: >>>> On 31.03.17 at 05:27, <chao.gao@intel.com> wrote: >>>>>This then also raises the question whether the call to >>>>>hvm_girq_dest_2_vcpu_id() is actually legitimate for lowest >>>>>priority delivery mode. >>>> >>>> For lowest priority delivery mode, if VT-d PI is enabled, the result (the >>>> destination vcpu) is overrided by vector_hashing_dest() to keep the >>>> existing behavior. I think the only point we should keep in mind is >>>> for cases other than lowest priority delivery mode, pi_find_dest_vcpu() >>>> and hvm_girq_dest_2_vcpu_id() give the same destination vcpu. >>> >>>Well, the override is done for the iommu_intpost case. The remark >>>on hvm_girq_dest_2_vcpu_id(), however, was made in general. >> >> Ok. You meant the method using in hvm_girq_dest_2_vcpu_id() may not match >> the method >> used by real hardware. I will check it. > >I mean that the method used there is pretty clearly "fixed" mode >handling. Thanks for checking. I think the method is ok here. It is an optimization to reduce the IPI between CPUs. Only the cases that the destination vcpu is a single vcpu can use this optimization. For lowest priority delivery case, we can regard the destination vcpus that match the 'dest_mode' and 'dest' here as possible destination vcpus. To emulate hardware accurately, I think we can use this optimization when a msi has multiple possible destination vcpus. Thank Chao
>>> On 05.04.17 at 02:20, <chao.gao@intel.com> wrote: > On Fri, Mar 31, 2017 at 04:38:02AM -0600, Jan Beulich wrote: >>>>> On 31.03.17 at 05:27, <chao.gao@intel.com> wrote: >>>>>>This then also raises the question whether the call to >>>>>>hvm_girq_dest_2_vcpu_id() is actually legitimate for lowest >>>>>>priority delivery mode. >>>>> >>>>> For lowest priority delivery mode, if VT-d PI is enabled, the result (the >>>>> destination vcpu) is overrided by vector_hashing_dest() to keep the >>>>> existing behavior. I think the only point we should keep in mind is >>>>> for cases other than lowest priority delivery mode, pi_find_dest_vcpu() >>>>> and hvm_girq_dest_2_vcpu_id() give the same destination vcpu. >>>> >>>>Well, the override is done for the iommu_intpost case. The remark >>>>on hvm_girq_dest_2_vcpu_id(), however, was made in general. >>> >>> Ok. You meant the method using in hvm_girq_dest_2_vcpu_id() may not match >>> the method >>> used by real hardware. I will check it. >> >>I mean that the method used there is pretty clearly "fixed" mode >>handling. Thanks for checking. > > I think the method is ok here. It is an optimization to reduce the IPI between > CPUs. Only the cases that the destination vcpu is a single vcpu can use this > optimization. For lowest priority delivery case, we can regard the destination > vcpus that match the 'dest_mode' and 'dest' here as possible destination vcpus. > To emulate hardware accurately, I think we can use this optimization > when a msi has multiple possible destination vcpus. Hmm, looking again I think I was confused when I did look last time. The (non-obvious) use of APIC_DEST_NOSHORT in the call to vlapic_match_dest() looks correct for Fixed and LowestPrio modes (and is in fact independent of delivery mode selection), so there indeed shouldn't be an issue here. I'm sorry for the extra trouble. Jan
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 0282986..2d8de16 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -438,6 +438,9 @@ static int hvm_migrate_pirq(struct domain *d, struct hvm_pirq_dpci *pirq_dpci, struct vcpu *v = arg; if ( (pirq_dpci->flags & HVM_IRQ_DPCI_MACH_MSI) && + (pirq_dpci->flags & HVM_IRQ_DPCI_GUEST_MSI) && + /* Needn't migrate pirq if this pirq is delivered to guest directly.*/ + (!pirq_dpci->gmsi.posted) && (pirq_dpci->gmsi.dest_vcpu_id == v->vcpu_id) ) { struct irq_desc *desc = diff --git a/xen/drivers/passthrough/io.c b/xen/drivers/passthrough/io.c index 080183e..d53976c 100644 --- a/xen/drivers/passthrough/io.c +++ b/xen/drivers/passthrough/io.c @@ -259,52 +259,6 @@ static struct vcpu *vector_hashing_dest(const struct domain *d, return dest; } -/* - * The purpose of this routine is to find the right destination vCPU for - * an interrupt which will be delivered by VT-d posted-interrupt. There - * are several cases as below: - * - * - For lowest-priority interrupts, use vector-hashing mechanism to find - * the destination. - * - Otherwise, for single destination interrupt, it is straightforward to - * find the destination vCPU and return true. - * - For multicast/broadcast vCPU, we cannot handle it via interrupt posting, - * so return NULL. - */ -static struct vcpu *pi_find_dest_vcpu(const struct domain *d, uint32_t dest_id, - bool_t dest_mode, uint8_t delivery_mode, - uint8_t gvec) -{ - unsigned int dest_vcpus = 0; - struct vcpu *v, *dest = NULL; - - switch ( delivery_mode ) - { - case dest_LowestPrio: - return vector_hashing_dest(d, dest_id, dest_mode, gvec); - case dest_Fixed: - for_each_vcpu ( d, v ) - { - if ( !vlapic_match_dest(vcpu_vlapic(v), NULL, APIC_DEST_NOSHORT, - dest_id, dest_mode) ) - continue; - - dest_vcpus++; - dest = v; - } - - /* For fixed mode, we only handle single-destination interrupts. */ - if ( dest_vcpus == 1 ) - return dest; - - break; - default: - break; - } - - return NULL; -} - int pt_irq_create_bind( struct domain *d, xen_domctl_bind_pt_irq_t *pt_irq_bind) { @@ -365,6 +319,7 @@ int pt_irq_create_bind( { uint8_t dest, dest_mode, delivery_mode; int dest_vcpu_id; + const struct vcpu *vcpu; if ( !(pirq_dpci->flags & HVM_IRQ_DPCI_MAPPED) ) { @@ -442,17 +397,24 @@ int pt_irq_create_bind( dest_vcpu_id = hvm_girq_dest_2_vcpu_id(d, dest, dest_mode); pirq_dpci->gmsi.dest_vcpu_id = dest_vcpu_id; spin_unlock(&d->event_lock); + + pirq_dpci->gmsi.posted = false; + vcpu = (dest_vcpu_id >= 0) ? d->vcpu[dest_vcpu_id] : NULL; + if ( iommu_intpost && (delivery_mode == dest_LowestPrio) ) + { + vcpu = vector_hashing_dest(d, dest, dest_mode, + pirq_dpci->gmsi.gvec); + if ( vcpu ) + pirq_dpci->gmsi.posted = true; + } if ( dest_vcpu_id >= 0 ) hvm_migrate_pirqs(d->vcpu[dest_vcpu_id]); /* Use interrupt posting if it is supported. */ if ( iommu_intpost ) { - const struct vcpu *vcpu = pi_find_dest_vcpu(d, dest, dest_mode, - delivery_mode, pirq_dpci->gmsi.gvec); - if ( vcpu ) - pi_update_irte( vcpu, info, pirq_dpci->gmsi.gvec ); + pi_update_irte(vcpu, info, pirq_dpci->gmsi.gvec); else dprintk(XENLOG_G_INFO, "%pv: deliver interrupt in remapping mode,gvec:%02x\n", diff --git a/xen/include/xen/hvm/irq.h b/xen/include/xen/hvm/irq.h index d3f8623..566854a 100644 --- a/xen/include/xen/hvm/irq.h +++ b/xen/include/xen/hvm/irq.h @@ -63,6 +63,7 @@ struct hvm_gmsi_info { uint32_t gvec; uint32_t gflags; int dest_vcpu_id; /* -1 :multi-dest, non-negative: dest_vcpu_id */ + bool posted; /* directly deliver to guest via VT-d PI? */ }; struct hvm_girq_dpci_mapping {
When a vCPU migrated to another pCPU, pt irqs binded to this vCPU also needed migration. When VT-d PI is enabled, interrupt vector will be recorded to a main memory resident data-structure and a notification whose destination is decided by NDST is generated. NDST is properly adjusted during vCPU migration so pirq directly injected to guest needn't be migrated. This patch adds a indicator, @posted, to show whether the pt irq is delivered through VT-d PI. Signed-off-by: Chao Gao <chao.gao@intel.com> --- v11: - rename the indicator to 'posted' - move setting 'posted' field to event lock un-locked region. v10: - Newly added. xen/arch/x86/hvm/hvm.c | 3 +++ xen/drivers/passthrough/io.c | 62 +++++++++----------------------------------- xen/include/xen/hvm/irq.h | 1 + 3 files changed, 16 insertions(+), 50 deletions(-)