| Message ID | 20170824094753.77795-2-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 24.08.17 at 11:47, <roger.pau@citrix.com> wrote: > @@ -438,6 +439,22 @@ int pt_irq_create_bind( > pi_update_irte(vcpu ? &vcpu->arch.hvm_vmx.pi_desc : NULL, > info, pirq_dpci->gmsi.gvec); > > + if ( pt_irq_bind->u.msi.gflags & VMSI_UNMASKED ) > + { > + struct irq_desc *desc = irq_to_desc(info->arch.irq); > + unsigned long flags; > + > + if ( !desc ) > + { > + pt_irq_destroy_bind(d, pt_irq_bind); > + return -EINVAL; > + } > + > + spin_lock_irqsave(&desc->lock, flags); > + guest_mask_msi_irq(desc, false); > + spin_unlock_irqrestore(&desc->lock, flags); > + } > + > break; > } I think you would better use pirq_spin_lock_irq_desc() here. And wouldn't the addition better be moved up a little (perhaps right after the dropping of the domain's event lock)? > --- a/xen/include/asm-x86/hvm/irq.h > +++ b/xen/include/asm-x86/hvm/irq.h > @@ -136,6 +136,7 @@ struct dev_intx_gsi_link { > #define VMSI_DM_MASK 0x200 > #define VMSI_DELIV_MASK 0x7000 > #define VMSI_TRIG_MODE 0x8000 > +#define VMSI_UNMASKED 0x10000 As pointed out earlier this effectively is a change to domctl.h, which (if it hadn't happened already in this release cycle) would require bumping of the interface version. Please add a note to the commit message clarifying this, as this complicates possible intentions of backporting this change. Jan
On Thu, Aug 24, 2017 at 04:07:40AM -0600, Jan Beulich wrote: > >>> On 24.08.17 at 11:47, <roger.pau@citrix.com> wrote: > > @@ -438,6 +439,22 @@ int pt_irq_create_bind( > > pi_update_irte(vcpu ? &vcpu->arch.hvm_vmx.pi_desc : NULL, > > info, pirq_dpci->gmsi.gvec); > > > > + if ( pt_irq_bind->u.msi.gflags & VMSI_UNMASKED ) > > + { > > + struct irq_desc *desc = irq_to_desc(info->arch.irq); > > + unsigned long flags; > > + > > + if ( !desc ) > > + { > > + pt_irq_destroy_bind(d, pt_irq_bind); > > + return -EINVAL; > > + } > > + > > + spin_lock_irqsave(&desc->lock, flags); > > + guest_mask_msi_irq(desc, false); > > + spin_unlock_irqrestore(&desc->lock, flags); > > + } > > + > > break; > > } > > I think you would better use pirq_spin_lock_irq_desc() here. And > wouldn't the addition better be moved up a little (perhaps right > after the dropping of the domain's event lock)? Shouldn't the unmask happen after the posted interrupt is setup? Or it doesn't really matter? I though it was safer to unmask once the bind process was finished. > > --- a/xen/include/asm-x86/hvm/irq.h > > +++ b/xen/include/asm-x86/hvm/irq.h > > @@ -136,6 +136,7 @@ struct dev_intx_gsi_link { > > #define VMSI_DM_MASK 0x200 > > #define VMSI_DELIV_MASK 0x7000 > > #define VMSI_TRIG_MODE 0x8000 > > +#define VMSI_UNMASKED 0x10000 > > As pointed out earlier this effectively is a change to domctl.h, which > (if it hadn't happened already in this release cycle) would require > bumping of the interface version. Please add a note to the commit > message clarifying this, as this complicates possible intentions of > backporting this change. Ack, thanks.
>>> On 24.08.17 at 12:12, <roger.pau@citrix.com> wrote: > On Thu, Aug 24, 2017 at 04:07:40AM -0600, Jan Beulich wrote: >> >>> On 24.08.17 at 11:47, <roger.pau@citrix.com> wrote: >> > @@ -438,6 +439,22 @@ int pt_irq_create_bind( >> > pi_update_irte(vcpu ? &vcpu->arch.hvm_vmx.pi_desc : NULL, >> > info, pirq_dpci->gmsi.gvec); >> > >> > + if ( pt_irq_bind->u.msi.gflags & VMSI_UNMASKED ) >> > + { >> > + struct irq_desc *desc = irq_to_desc(info->arch.irq); >> > + unsigned long flags; >> > + >> > + if ( !desc ) >> > + { >> > + pt_irq_destroy_bind(d, pt_irq_bind); >> > + return -EINVAL; >> > + } >> > + >> > + spin_lock_irqsave(&desc->lock, flags); >> > + guest_mask_msi_irq(desc, false); >> > + spin_unlock_irqrestore(&desc->lock, flags); >> > + } >> > + >> > break; >> > } >> >> I think you would better use pirq_spin_lock_irq_desc() here. And >> wouldn't the addition better be moved up a little (perhaps right >> after the dropping of the domain's event lock)? > > Shouldn't the unmask happen after the posted interrupt is setup? Or it > doesn't really matter? > > I though it was safer to unmask once the bind process was finished. Yeah, I'm not entirely certain either, hence I've put it as a question. Kevin, Chao? Jan
On Thu, Aug 24, 2017 at 04:17:32AM -0600, Jan Beulich wrote: >>>> On 24.08.17 at 12:12, <roger.pau@citrix.com> wrote: >> On Thu, Aug 24, 2017 at 04:07:40AM -0600, Jan Beulich wrote: >>> >>> On 24.08.17 at 11:47, <roger.pau@citrix.com> wrote: >>> > @@ -438,6 +439,22 @@ int pt_irq_create_bind( >>> > pi_update_irte(vcpu ? &vcpu->arch.hvm_vmx.pi_desc : NULL, >>> > info, pirq_dpci->gmsi.gvec); >>> > >>> > + if ( pt_irq_bind->u.msi.gflags & VMSI_UNMASKED ) >>> > + { >>> > + struct irq_desc *desc = irq_to_desc(info->arch.irq); >>> > + unsigned long flags; >>> > + >>> > + if ( !desc ) >>> > + { >>> > + pt_irq_destroy_bind(d, pt_irq_bind); >>> > + return -EINVAL; >>> > + } >>> > + >>> > + spin_lock_irqsave(&desc->lock, flags); >>> > + guest_mask_msi_irq(desc, false); >>> > + spin_unlock_irqrestore(&desc->lock, flags); >>> > + } >>> > + >>> > break; >>> > } >>> >>> I think you would better use pirq_spin_lock_irq_desc() here. And >>> wouldn't the addition better be moved up a little (perhaps right >>> after the dropping of the domain's event lock)? >> >> Shouldn't the unmask happen after the posted interrupt is setup? Or it >> doesn't really matter? >> >> I though it was safer to unmask once the bind process was finished. > >Yeah, I'm not entirely certain either, hence I've put it as a question. >Kevin, Chao? > Hi, Jan and Roger. pi_update_irte() right above the piece of code is to set IRTE properly according to the request. Unmasking the msi without updating IRTE, I think may leads to inject an interrupt whose vector or destination is out of date. Thanks Chao
diff --git a/xen/drivers/passthrough/io.c b/xen/drivers/passthrough/io.c index 19a21bf85a..f68b31809f 100644 --- a/xen/drivers/passthrough/io.c +++ b/xen/drivers/passthrough/io.c @@ -342,13 +342,14 @@ int pt_irq_create_bind( uint8_t dest, dest_mode, delivery_mode; int dest_vcpu_id; const struct vcpu *vcpu; + uint32_t gflags = pt_irq_bind->u.msi.gflags & ~VMSI_UNMASKED; if ( !(pirq_dpci->flags & HVM_IRQ_DPCI_MAPPED) ) { pirq_dpci->flags = HVM_IRQ_DPCI_MAPPED | HVM_IRQ_DPCI_MACH_MSI | HVM_IRQ_DPCI_GUEST_MSI; pirq_dpci->gmsi.gvec = pt_irq_bind->u.msi.gvec; - pirq_dpci->gmsi.gflags = pt_irq_bind->u.msi.gflags; + pirq_dpci->gmsi.gflags = gflags; /* * 'pt_irq_create_bind' can be called after 'pt_irq_destroy_bind'. * The 'pirq_cleanup_check' which would free the structure is only @@ -401,13 +402,13 @@ int pt_irq_create_bind( /* If pirq is already mapped as vmsi, update guest data/addr. */ if ( pirq_dpci->gmsi.gvec != pt_irq_bind->u.msi.gvec || - pirq_dpci->gmsi.gflags != pt_irq_bind->u.msi.gflags ) + pirq_dpci->gmsi.gflags != gflags ) { /* Directly clear pending EOIs before enabling new MSI info. */ pirq_guest_eoi(info); pirq_dpci->gmsi.gvec = pt_irq_bind->u.msi.gvec; - pirq_dpci->gmsi.gflags = pt_irq_bind->u.msi.gflags; + pirq_dpci->gmsi.gflags = gflags; } } /* Calculate dest_vcpu_id for MSI-type pirq migration. */ @@ -438,6 +439,22 @@ int pt_irq_create_bind( pi_update_irte(vcpu ? &vcpu->arch.hvm_vmx.pi_desc : NULL, info, pirq_dpci->gmsi.gvec); + if ( pt_irq_bind->u.msi.gflags & VMSI_UNMASKED ) + { + struct irq_desc *desc = irq_to_desc(info->arch.irq); + unsigned long flags; + + if ( !desc ) + { + pt_irq_destroy_bind(d, pt_irq_bind); + return -EINVAL; + } + + spin_lock_irqsave(&desc->lock, flags); + guest_mask_msi_irq(desc, false); + spin_unlock_irqrestore(&desc->lock, flags); + } + break; } diff --git a/xen/include/asm-x86/hvm/irq.h b/xen/include/asm-x86/hvm/irq.h index 106dc19613..9546c24879 100644 --- a/xen/include/asm-x86/hvm/irq.h +++ b/xen/include/asm-x86/hvm/irq.h @@ -136,6 +136,7 @@ struct dev_intx_gsi_link { #define VMSI_DM_MASK 0x200 #define VMSI_DELIV_MASK 0x7000 #define VMSI_TRIG_MODE 0x8000 +#define VMSI_UNMASKED 0x10000 #define GFLAGS_SHIFT_RH 8 #define GFLAGS_SHIFT_DELIV_MODE 12
The flag is part of the gflags, and should be used to request the unmask of a MSI interrupt once it's bound. This is required for the device model in order to be capable of binding MSIX interrupts that have the entry mask bit already unset at bind time. Without this fix the interrupts would be left masked. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Reported by: Andreas Kinzler <hfp@posteo.de> --- Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> --- xen/drivers/passthrough/io.c | 23 ++++++++++++++++++++--- xen/include/asm-x86/hvm/irq.h | 1 + 2 files changed, 21 insertions(+), 3 deletions(-)