| Message ID | 1491438627-10456-2-git-send-email-chao.gao@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Cc: kevin On Fri, Apr 07, 2017 at 04:38:00AM -0600, Jan Beulich wrote: >>>> On 06.04.17 at 02:30, <chao.gao@intel.com> wrote: >> --- 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) ) >> { > >I don't think I've seen you address Kevin's comment on this for v11, >and like Kevin I can't immediately see why the above addition would >be correct. Do you perhaps mean > > if ( (pirq_dpci->flags & HVM_IRQ_DPCI_MACH_MSI) && > /* Needn't migrate pirq if this pirq is delivered to guest directly.*/ > (!pirq_dpci->gmsi.posted || > <whatever is appropriate here, if anything>) && > (pirq_dpci->gmsi.dest_vcpu_id == v->vcpu_id) ) Sorry to Kevin. And thanks to point it out. But I thought we had discussed this in https://lists.xenproject.org/archives/html/xen-devel/2017-03/msg04383.html. I did think you agreed with me. gmsi is invalid when pirq_dpci is not GUEST_MSI, is there something I have ignored? Thanks Chao
On Fri, Apr 07, 2017 at 05:50:36AM -0600, Jan Beulich wrote: >>>> On 07.04.17 at 06:07, <chao.gao@intel.com> wrote: >> Cc: kevin >> >> On Fri, Apr 07, 2017 at 04:38:00AM -0600, Jan Beulich wrote: >>>>>> On 06.04.17 at 02:30, <chao.gao@intel.com> wrote: >>>> --- 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) ) >>>> { >>> >>>I don't think I've seen you address Kevin's comment on this for v11, >>>and like Kevin I can't immediately see why the above addition would >>>be correct. Do you perhaps mean >>> >>> if ( (pirq_dpci->flags & HVM_IRQ_DPCI_MACH_MSI) && >>> /* Needn't migrate pirq if this pirq is delivered to guest directly.*/ >>> (!pirq_dpci->gmsi.posted || >>> <whatever is appropriate here, if anything>) && >>> (pirq_dpci->gmsi.dest_vcpu_id == v->vcpu_id) ) >> >> Sorry to Kevin. And thanks to point it out. >> But I thought we had discussed this in >> https://lists.xenproject.org/archives/html/xen-devel/2017-03/msg04383.html. I >> did think you agreed with me. >> gmsi is invalid when pirq_dpci is not GUEST_MSI, is there something I have >> ignored? > >You've been talking about GUEST_PCI there, which I did (and do) >agree we can't handle here. So for the purposes of your series, >simply adding the gmsi.posted check would be the right thing imo. >I don't think I see anything wrong with the ->gmsi accesses here: >The GUEST_PCI code simply doesn't set them, so dest_vcpu_id >will still be -1 (from pt_pirq_init()). So I don't see any bug being >fixed here with the extra other check you add. If you agree, I >can take that line and the commit message sentence out while >committing. Ok. I admit I said it's bug is wrong. feel free to do what you want. Thanks Chao > >Jan >
>>> On 06.04.17 at 02:30, <chao.gao@intel.com> wrote: > --- 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) ) > { I don't think I've seen you address Kevin's comment on this for v11, and like Kevin I can't immediately see why the above addition would be correct. Do you perhaps mean if ( (pirq_dpci->flags & HVM_IRQ_DPCI_MACH_MSI) && /* Needn't migrate pirq if this pirq is delivered to guest directly.*/ (!pirq_dpci->gmsi.posted || <whatever is appropriate here, if anything>) && (pirq_dpci->gmsi.dest_vcpu_id == v->vcpu_id) ) Jan
>>> On 07.04.17 at 06:07, <chao.gao@intel.com> wrote: > Cc: kevin > > On Fri, Apr 07, 2017 at 04:38:00AM -0600, Jan Beulich wrote: >>>>> On 06.04.17 at 02:30, <chao.gao@intel.com> wrote: >>> --- 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) ) >>> { >> >>I don't think I've seen you address Kevin's comment on this for v11, >>and like Kevin I can't immediately see why the above addition would >>be correct. Do you perhaps mean >> >> if ( (pirq_dpci->flags & HVM_IRQ_DPCI_MACH_MSI) && >> /* Needn't migrate pirq if this pirq is delivered to guest directly.*/ >> (!pirq_dpci->gmsi.posted || >> <whatever is appropriate here, if anything>) && >> (pirq_dpci->gmsi.dest_vcpu_id == v->vcpu_id) ) > > Sorry to Kevin. And thanks to point it out. > But I thought we had discussed this in > https://lists.xenproject.org/archives/html/xen-devel/2017-03/msg04383.html. I > did think you agreed with me. > gmsi is invalid when pirq_dpci is not GUEST_MSI, is there something I have > ignored? You've been talking about GUEST_PCI there, which I did (and do) agree we can't handle here. So for the purposes of your series, simply adding the gmsi.posted check would be the right thing imo. I don't think I see anything wrong with the ->gmsi accesses here: The GUEST_PCI code simply doesn't set them, so dest_vcpu_id will still be -1 (from pt_pirq_init()). So I don't see any bug being fixed here with the extra other check you add. If you agree, I can take that line and the commit message sentence out while committing. Jan
>>> On 07.04.17 at 09:23, <chao.gao@intel.com> wrote: > On Fri, Apr 07, 2017 at 05:50:36AM -0600, Jan Beulich wrote: >>>>> On 07.04.17 at 06:07, <chao.gao@intel.com> wrote: >>> Cc: kevin >>> >>> On Fri, Apr 07, 2017 at 04:38:00AM -0600, Jan Beulich wrote: >>>>>>> On 06.04.17 at 02:30, <chao.gao@intel.com> wrote: >>>>> --- 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) ) >>>>> { >>>> >>>>I don't think I've seen you address Kevin's comment on this for v11, >>>>and like Kevin I can't immediately see why the above addition would >>>>be correct. Do you perhaps mean >>>> >>>> if ( (pirq_dpci->flags & HVM_IRQ_DPCI_MACH_MSI) && >>>> /* Needn't migrate pirq if this pirq is delivered to guest > directly.*/ >>>> (!pirq_dpci->gmsi.posted || >>>> <whatever is appropriate here, if anything>) && >>>> (pirq_dpci->gmsi.dest_vcpu_id == v->vcpu_id) ) >>> >>> Sorry to Kevin. And thanks to point it out. >>> But I thought we had discussed this in >>> https://lists.xenproject.org/archives/html/xen-devel/2017-03/msg04383.html. I >>> did think you agreed with me. >>> gmsi is invalid when pirq_dpci is not GUEST_MSI, is there something I have >>> ignored? >> >>You've been talking about GUEST_PCI there, which I did (and do) >>agree we can't handle here. So for the purposes of your series, >>simply adding the gmsi.posted check would be the right thing imo. >>I don't think I see anything wrong with the ->gmsi accesses here: >>The GUEST_PCI code simply doesn't set them, so dest_vcpu_id >>will still be -1 (from pt_pirq_init()). So I don't see any bug being >>fixed here with the extra other check you add. If you agree, I >>can take that line and the commit message sentence out while >>committing. > > Ok. I admit I said it's bug is wrong. feel free to do what you want. Well, looks like I forgot to adjust the commit message. -ETOOMUCHSTUFFGOINGONTODAY. Jan
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index eba6e9d..d4c8967 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 83e0961..4d19413 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,25 @@ 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 ) + { + if ( 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 f041252..671a6f2 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 was migrated to another pCPU, pt irqs binded to this vCPU might also need migration as a optimization to reduce IPI between pCPUs. 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. Also this patch fixes a bug that hvm_migrate_pirq() accesses pirq_dpci->gmsi.dest_vcpu_id without checking the pirq_dpci's type. Signed-off-by: Chao Gao <chao.gao@intel.com> --- v12: - fix a logic error in fixed delivery case. 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 | 63 +++++++++----------------------------------- xen/include/xen/hvm/irq.h | 1 + 3 files changed, 17 insertions(+), 50 deletions(-)