* [PATCH 0/3] XSA-96 follow-ups
@ 2014-06-03 13:52 Jan Beulich
2014-06-03 13:58 ` [PATCH 1/3] x86/HVM: properly propagate errors from HVMOP_inject_msi Jan Beulich
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Jan Beulich @ 2014-06-03 13:52 UTC (permalink / raw)
To: xen-devel; +Cc: Keir Fraser
1: x86/HVM: properly propagate errors from HVMOP_inject_msi
2: x86/HVM: make vmsi_deliver() return proper error values
3: pt-irq fixes and improvements
Signed-off-by: Jan Beulich <jbeulich@suse.com>
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 1/3] x86/HVM: properly propagate errors from HVMOP_inject_msi 2014-06-03 13:52 [PATCH 0/3] XSA-96 follow-ups Jan Beulich @ 2014-06-03 13:58 ` Jan Beulich 2014-06-03 14:05 ` Andrew Cooper 2014-06-03 13:59 ` [PATCH 2/3] x86/HVM: make vmsi_deliver() return proper error values Jan Beulich 2014-06-03 14:00 ` [PATCH 3/3] pt-irq fixes and improvements Jan Beulich 2 siblings, 1 reply; 12+ messages in thread From: Jan Beulich @ 2014-06-03 13:58 UTC (permalink / raw) To: xen-devel; +Cc: Keir Fraser, Stefano Stabellini [-- Attachment #1: Type: text/plain, Size: 2677 bytes --] There are a number of ways this operation can go wrong, all of which got ignored so far. In the context of this I wonder whether map_domain_emuirq_pirq() returning 0 in the "already mapped" case is really intended to be that way (this is why the subsequent NULL check here can't be an ASSERT()). Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -5096,7 +5096,7 @@ static int hvmop_inject_msi( if ( rc ) goto out; - hvm_inject_msi(d, op.addr, op.data); + rc = hvm_inject_msi(d, op.addr, op.data); out: rcu_unlock_domain(d); --- a/xen/arch/x86/hvm/irq.c +++ b/xen/arch/x86/hvm/irq.c @@ -270,7 +270,7 @@ void hvm_set_pci_link_route(struct domai d->domain_id, link, old_isa_irq, isa_irq); } -void hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data) +int hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data) { uint32_t tmp = (uint32_t) addr; uint8_t dest = (tmp & MSI_ADDR_DEST_ID_MASK) >> MSI_ADDR_DEST_ID_SHIFT; @@ -292,20 +292,28 @@ void hvm_inject_msi(struct domain *d, ui /* if it is the first time, allocate the pirq */ if ( !info || info->arch.hvm.emuirq == IRQ_UNBOUND ) { + int rc; + spin_lock(&d->event_lock); - map_domain_emuirq_pirq(d, pirq, IRQ_MSI_EMU); + rc = map_domain_emuirq_pirq(d, pirq, IRQ_MSI_EMU); spin_unlock(&d->event_lock); + if ( rc ) + return rc; info = pirq_info(d, pirq); if ( !info ) - return; - } else if (info->arch.hvm.emuirq != IRQ_MSI_EMU) - return; + return -EBUSY; + } + else if ( info->arch.hvm.emuirq != IRQ_MSI_EMU ) + return -EINVAL; send_guest_pirq(d, info); - return; + return 0; } + return -ERANGE; } vmsi_deliver(d, vector, dest, dest_mode, delivery_mode, trig_mode); + + return 0; } void hvm_set_callback_via(struct domain *d, uint64_t via) --- a/xen/include/xen/hvm/irq.h +++ b/xen/include/xen/hvm/irq.h @@ -123,7 +123,7 @@ void hvm_isa_irq_deassert( void hvm_set_pci_link_route(struct domain *d, u8 link, u8 isa_irq); -void hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data); +int hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data); void hvm_maybe_deassert_evtchn_irq(void); void hvm_assert_evtchn_irq(struct vcpu *v); [-- Attachment #2: x86-HVM-inject-MSI-retval.patch --] [-- Type: text/plain, Size: 2731 bytes --] x86/HVM: properly propagate errors from HVMOP_inject_msi There are a number of ways this operation can go wrong, all of which got ignored so far. In the context of this I wonder whether map_domain_emuirq_pirq() returning 0 in the "already mapped" case is really intended to be that way (this is why the subsequent NULL check here can't be an ASSERT()). Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -5096,7 +5096,7 @@ static int hvmop_inject_msi( if ( rc ) goto out; - hvm_inject_msi(d, op.addr, op.data); + rc = hvm_inject_msi(d, op.addr, op.data); out: rcu_unlock_domain(d); --- a/xen/arch/x86/hvm/irq.c +++ b/xen/arch/x86/hvm/irq.c @@ -270,7 +270,7 @@ void hvm_set_pci_link_route(struct domai d->domain_id, link, old_isa_irq, isa_irq); } -void hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data) +int hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data) { uint32_t tmp = (uint32_t) addr; uint8_t dest = (tmp & MSI_ADDR_DEST_ID_MASK) >> MSI_ADDR_DEST_ID_SHIFT; @@ -292,20 +292,28 @@ void hvm_inject_msi(struct domain *d, ui /* if it is the first time, allocate the pirq */ if ( !info || info->arch.hvm.emuirq == IRQ_UNBOUND ) { + int rc; + spin_lock(&d->event_lock); - map_domain_emuirq_pirq(d, pirq, IRQ_MSI_EMU); + rc = map_domain_emuirq_pirq(d, pirq, IRQ_MSI_EMU); spin_unlock(&d->event_lock); + if ( rc ) + return rc; info = pirq_info(d, pirq); if ( !info ) - return; - } else if (info->arch.hvm.emuirq != IRQ_MSI_EMU) - return; + return -EBUSY; + } + else if ( info->arch.hvm.emuirq != IRQ_MSI_EMU ) + return -EINVAL; send_guest_pirq(d, info); - return; + return 0; } + return -ERANGE; } vmsi_deliver(d, vector, dest, dest_mode, delivery_mode, trig_mode); + + return 0; } void hvm_set_callback_via(struct domain *d, uint64_t via) --- a/xen/include/xen/hvm/irq.h +++ b/xen/include/xen/hvm/irq.h @@ -123,7 +123,7 @@ void hvm_isa_irq_deassert( void hvm_set_pci_link_route(struct domain *d, u8 link, u8 isa_irq); -void hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data); +int hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data); void hvm_maybe_deassert_evtchn_irq(void); void hvm_assert_evtchn_irq(struct vcpu *v); [-- Attachment #3: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] x86/HVM: properly propagate errors from HVMOP_inject_msi 2014-06-03 13:58 ` [PATCH 1/3] x86/HVM: properly propagate errors from HVMOP_inject_msi Jan Beulich @ 2014-06-03 14:05 ` Andrew Cooper 0 siblings, 0 replies; 12+ messages in thread From: Andrew Cooper @ 2014-06-03 14:05 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Keir Fraser, Stefano Stabellini [-- Attachment #1.1: Type: text/plain, Size: 2984 bytes --] On 03/06/14 14:58, Jan Beulich wrote: > There are a number of ways this operation can go wrong, all of which > got ignored so far. > > In the context of this I wonder whether map_domain_emuirq_pirq() > returning 0 in the "already mapped" case is really intended to be that > way (this is why the subsequent NULL check here can't be an ASSERT()). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> > > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -5096,7 +5096,7 @@ static int hvmop_inject_msi( > if ( rc ) > goto out; > > - hvm_inject_msi(d, op.addr, op.data); > + rc = hvm_inject_msi(d, op.addr, op.data); > > out: > rcu_unlock_domain(d); > --- a/xen/arch/x86/hvm/irq.c > +++ b/xen/arch/x86/hvm/irq.c > @@ -270,7 +270,7 @@ void hvm_set_pci_link_route(struct domai > d->domain_id, link, old_isa_irq, isa_irq); > } > > -void hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data) > +int hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data) > { > uint32_t tmp = (uint32_t) addr; > uint8_t dest = (tmp & MSI_ADDR_DEST_ID_MASK) >> MSI_ADDR_DEST_ID_SHIFT; > @@ -292,20 +292,28 @@ void hvm_inject_msi(struct domain *d, ui > /* if it is the first time, allocate the pirq */ > if ( !info || info->arch.hvm.emuirq == IRQ_UNBOUND ) > { > + int rc; > + > spin_lock(&d->event_lock); > - map_domain_emuirq_pirq(d, pirq, IRQ_MSI_EMU); > + rc = map_domain_emuirq_pirq(d, pirq, IRQ_MSI_EMU); > spin_unlock(&d->event_lock); > + if ( rc ) > + return rc; > info = pirq_info(d, pirq); > if ( !info ) > - return; > - } else if (info->arch.hvm.emuirq != IRQ_MSI_EMU) > - return; > + return -EBUSY; > + } > + else if ( info->arch.hvm.emuirq != IRQ_MSI_EMU ) > + return -EINVAL; > send_guest_pirq(d, info); > - return; > + return 0; > } > + return -ERANGE; > } > > vmsi_deliver(d, vector, dest, dest_mode, delivery_mode, trig_mode); > + > + return 0; > } > > void hvm_set_callback_via(struct domain *d, uint64_t via) > --- a/xen/include/xen/hvm/irq.h > +++ b/xen/include/xen/hvm/irq.h > @@ -123,7 +123,7 @@ void hvm_isa_irq_deassert( > > void hvm_set_pci_link_route(struct domain *d, u8 link, u8 isa_irq); > > -void hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data); > +int hvm_inject_msi(struct domain *d, uint64_t addr, uint32_t data); > > void hvm_maybe_deassert_evtchn_irq(void); > void hvm_assert_evtchn_irq(struct vcpu *v); > > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel [-- Attachment #1.2: Type: text/html, Size: 3793 bytes --] [-- Attachment #2: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/3] x86/HVM: make vmsi_deliver() return proper error values 2014-06-03 13:52 [PATCH 0/3] XSA-96 follow-ups Jan Beulich 2014-06-03 13:58 ` [PATCH 1/3] x86/HVM: properly propagate errors from HVMOP_inject_msi Jan Beulich @ 2014-06-03 13:59 ` Jan Beulich 2014-06-03 14:13 ` Andrew Cooper 2014-06-03 14:00 ` [PATCH 3/3] pt-irq fixes and improvements Jan Beulich 2 siblings, 1 reply; 12+ messages in thread From: Jan Beulich @ 2014-06-03 13:59 UTC (permalink / raw) To: xen-devel; +Cc: Keir Fraser [-- Attachment #1: Type: text/plain, Size: 3477 bytes --] ... and propagate this from hvm_inject_msi(). In the course of this I spotted further room for cleanup: - vmsi_inj_irq()'s struct domain * parameter was unused - vmsi_deliver() pointlessly passed on dest_ExtINT to vmsi_inj_irq() (which that one validly refused to handle) - vmsi_inj_irq()'s sole caller guarantees a proper delivery mode (i.e. rather than printing an obscure message we can just BUG()) - some formatting and log message quirks Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/hvm/irq.c +++ b/xen/arch/x86/hvm/irq.c @@ -311,9 +311,7 @@ int hvm_inject_msi(struct domain *d, uin return -ERANGE; } - vmsi_deliver(d, vector, dest, dest_mode, delivery_mode, trig_mode); - - return 0; + return vmsi_deliver(d, vector, dest, dest_mode, delivery_mode, trig_mode); } void hvm_set_callback_via(struct domain *d, uint64_t via) --- a/xen/arch/x86/hvm/vmsi.c +++ b/xen/arch/x86/hvm/vmsi.c @@ -43,14 +43,12 @@ #include <asm/io_apic.h> static void vmsi_inj_irq( - struct domain *d, struct vlapic *target, uint8_t vector, uint8_t trig_mode, uint8_t delivery_mode) { - HVM_DBG_LOG(DBG_LEVEL_IOAPIC, "vmsi_inj_irq " - "irq %d trig %d delive mode %d\n", + HVM_DBG_LOG(DBG_LEVEL_VLAPIC, "vmsi_inj_irq: vec %02x trig %d dm %d\n", vector, trig_mode, delivery_mode); switch ( delivery_mode ) @@ -60,8 +58,7 @@ static void vmsi_inj_irq( vlapic_set_irq(target, vector, trig_mode); break; default: - gdprintk(XENLOG_WARNING, "error delivery mode %d\n", delivery_mode); - break; + BUG(); } } @@ -76,38 +73,31 @@ int vmsi_deliver( switch ( delivery_mode ) { case dest_LowestPrio: - { target = vlapic_lowest_prio(d, NULL, 0, dest, dest_mode); if ( target != NULL ) - vmsi_inj_irq(d, target, vector, trig_mode, delivery_mode); - else - HVM_DBG_LOG(DBG_LEVEL_IOAPIC, "null round robin: " - "vector=%x delivery_mode=%x\n", - vector, dest_LowestPrio); - break; - } + { + vmsi_inj_irq(target, vector, trig_mode, delivery_mode); + break; + } + HVM_DBG_LOG(DBG_LEVEL_VLAPIC, "null MSI round robin: vector=%02x\n", + vector); + return -ESRCH; case dest_Fixed: - case dest_ExtINT: - { for_each_vcpu ( d, v ) if ( vlapic_match_dest(vcpu_vlapic(v), NULL, 0, dest, dest_mode) ) - vmsi_inj_irq(d, vcpu_vlapic(v), - vector, trig_mode, delivery_mode); + vmsi_inj_irq(vcpu_vlapic(v), vector, + trig_mode, delivery_mode); break; - } - case dest_SMI: - case dest_NMI: - case dest_INIT: - case dest__reserved_2: default: - gdprintk(XENLOG_WARNING, "Unsupported delivery mode %d\n", - delivery_mode); - break; + printk(XENLOG_G_WARNING + "%pv: Unsupported MSI delivery mode %d for Dom%d\n", + current, delivery_mode, d->domain_id); + return -EINVAL; } - return 1; + return 0; } void vmsi_deliver_pirq(struct domain *d, const struct hvm_pirq_dpci *pirq_dpci) [-- Attachment #2: x86-HVM-vmsi_deliver-retval.patch --] [-- Type: text/plain, Size: 3530 bytes --] x86/HVM: make vmsi_deliver() return proper error values ... and propagate this from hvm_inject_msi(). In the course of this I spotted further room for cleanup: - vmsi_inj_irq()'s struct domain * parameter was unused - vmsi_deliver() pointlessly passed on dest_ExtINT to vmsi_inj_irq() (which that one validly refused to handle) - vmsi_inj_irq()'s sole caller guarantees a proper delivery mode (i.e. rather than printing an obscure message we can just BUG()) - some formatting and log message quirks Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/hvm/irq.c +++ b/xen/arch/x86/hvm/irq.c @@ -311,9 +311,7 @@ int hvm_inject_msi(struct domain *d, uin return -ERANGE; } - vmsi_deliver(d, vector, dest, dest_mode, delivery_mode, trig_mode); - - return 0; + return vmsi_deliver(d, vector, dest, dest_mode, delivery_mode, trig_mode); } void hvm_set_callback_via(struct domain *d, uint64_t via) --- a/xen/arch/x86/hvm/vmsi.c +++ b/xen/arch/x86/hvm/vmsi.c @@ -43,14 +43,12 @@ #include <asm/io_apic.h> static void vmsi_inj_irq( - struct domain *d, struct vlapic *target, uint8_t vector, uint8_t trig_mode, uint8_t delivery_mode) { - HVM_DBG_LOG(DBG_LEVEL_IOAPIC, "vmsi_inj_irq " - "irq %d trig %d delive mode %d\n", + HVM_DBG_LOG(DBG_LEVEL_VLAPIC, "vmsi_inj_irq: vec %02x trig %d dm %d\n", vector, trig_mode, delivery_mode); switch ( delivery_mode ) @@ -60,8 +58,7 @@ static void vmsi_inj_irq( vlapic_set_irq(target, vector, trig_mode); break; default: - gdprintk(XENLOG_WARNING, "error delivery mode %d\n", delivery_mode); - break; + BUG(); } } @@ -76,38 +73,31 @@ int vmsi_deliver( switch ( delivery_mode ) { case dest_LowestPrio: - { target = vlapic_lowest_prio(d, NULL, 0, dest, dest_mode); if ( target != NULL ) - vmsi_inj_irq(d, target, vector, trig_mode, delivery_mode); - else - HVM_DBG_LOG(DBG_LEVEL_IOAPIC, "null round robin: " - "vector=%x delivery_mode=%x\n", - vector, dest_LowestPrio); - break; - } + { + vmsi_inj_irq(target, vector, trig_mode, delivery_mode); + break; + } + HVM_DBG_LOG(DBG_LEVEL_VLAPIC, "null MSI round robin: vector=%02x\n", + vector); + return -ESRCH; case dest_Fixed: - case dest_ExtINT: - { for_each_vcpu ( d, v ) if ( vlapic_match_dest(vcpu_vlapic(v), NULL, 0, dest, dest_mode) ) - vmsi_inj_irq(d, vcpu_vlapic(v), - vector, trig_mode, delivery_mode); + vmsi_inj_irq(vcpu_vlapic(v), vector, + trig_mode, delivery_mode); break; - } - case dest_SMI: - case dest_NMI: - case dest_INIT: - case dest__reserved_2: default: - gdprintk(XENLOG_WARNING, "Unsupported delivery mode %d\n", - delivery_mode); - break; + printk(XENLOG_G_WARNING + "%pv: Unsupported MSI delivery mode %d for Dom%d\n", + current, delivery_mode, d->domain_id); + return -EINVAL; } - return 1; + return 0; } void vmsi_deliver_pirq(struct domain *d, const struct hvm_pirq_dpci *pirq_dpci) [-- Attachment #3: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] x86/HVM: make vmsi_deliver() return proper error values 2014-06-03 13:59 ` [PATCH 2/3] x86/HVM: make vmsi_deliver() return proper error values Jan Beulich @ 2014-06-03 14:13 ` Andrew Cooper 0 siblings, 0 replies; 12+ messages in thread From: Andrew Cooper @ 2014-06-03 14:13 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Keir Fraser [-- Attachment #1.1: Type: text/plain, Size: 3937 bytes --] On 03/06/14 14:59, Jan Beulich wrote: > ... and propagate this from hvm_inject_msi(). In the course of this I > spotted further room for cleanup: > - vmsi_inj_irq()'s struct domain * parameter was unused > - vmsi_deliver() pointlessly passed on dest_ExtINT to vmsi_inj_irq() > (which that one validly refused to handle) > - vmsi_inj_irq()'s sole caller guarantees a proper delivery mode (i.e. > rather than printing an obscure message we can just BUG()) > - some formatting and log message quirks > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> Arguably under Xen style, the final addition in the final hunk should have a newline before return 0, but that's trivial. > > --- a/xen/arch/x86/hvm/irq.c > +++ b/xen/arch/x86/hvm/irq.c > @@ -311,9 +311,7 @@ int hvm_inject_msi(struct domain *d, uin > return -ERANGE; > } > > - vmsi_deliver(d, vector, dest, dest_mode, delivery_mode, trig_mode); > - > - return 0; > + return vmsi_deliver(d, vector, dest, dest_mode, delivery_mode, trig_mode); > } > > void hvm_set_callback_via(struct domain *d, uint64_t via) > --- a/xen/arch/x86/hvm/vmsi.c > +++ b/xen/arch/x86/hvm/vmsi.c > @@ -43,14 +43,12 @@ > #include <asm/io_apic.h> > > static void vmsi_inj_irq( > - struct domain *d, > struct vlapic *target, > uint8_t vector, > uint8_t trig_mode, > uint8_t delivery_mode) > { > - HVM_DBG_LOG(DBG_LEVEL_IOAPIC, "vmsi_inj_irq " > - "irq %d trig %d delive mode %d\n", > + HVM_DBG_LOG(DBG_LEVEL_VLAPIC, "vmsi_inj_irq: vec %02x trig %d dm %d\n", > vector, trig_mode, delivery_mode); > > switch ( delivery_mode ) > @@ -60,8 +58,7 @@ static void vmsi_inj_irq( > vlapic_set_irq(target, vector, trig_mode); > break; > default: > - gdprintk(XENLOG_WARNING, "error delivery mode %d\n", delivery_mode); > - break; > + BUG(); > } > } > > @@ -76,38 +73,31 @@ int vmsi_deliver( > switch ( delivery_mode ) > { > case dest_LowestPrio: > - { > target = vlapic_lowest_prio(d, NULL, 0, dest, dest_mode); > if ( target != NULL ) > - vmsi_inj_irq(d, target, vector, trig_mode, delivery_mode); > - else > - HVM_DBG_LOG(DBG_LEVEL_IOAPIC, "null round robin: " > - "vector=%x delivery_mode=%x\n", > - vector, dest_LowestPrio); > - break; > - } > + { > + vmsi_inj_irq(target, vector, trig_mode, delivery_mode); > + break; > + } > + HVM_DBG_LOG(DBG_LEVEL_VLAPIC, "null MSI round robin: vector=%02x\n", > + vector); > + return -ESRCH; > > case dest_Fixed: > - case dest_ExtINT: > - { > for_each_vcpu ( d, v ) > if ( vlapic_match_dest(vcpu_vlapic(v), NULL, > 0, dest, dest_mode) ) > - vmsi_inj_irq(d, vcpu_vlapic(v), > - vector, trig_mode, delivery_mode); > + vmsi_inj_irq(vcpu_vlapic(v), vector, > + trig_mode, delivery_mode); > break; > - } > > - case dest_SMI: > - case dest_NMI: > - case dest_INIT: > - case dest__reserved_2: > default: > - gdprintk(XENLOG_WARNING, "Unsupported delivery mode %d\n", > - delivery_mode); > - break; > + printk(XENLOG_G_WARNING > + "%pv: Unsupported MSI delivery mode %d for Dom%d\n", > + current, delivery_mode, d->domain_id); > + return -EINVAL; > } > - return 1; > + return 0; > } > > void vmsi_deliver_pirq(struct domain *d, const struct hvm_pirq_dpci *pirq_dpci) > > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel [-- Attachment #1.2: Type: text/html, Size: 4680 bytes --] [-- Attachment #2: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/3] pt-irq fixes and improvements 2014-06-03 13:52 [PATCH 0/3] XSA-96 follow-ups Jan Beulich 2014-06-03 13:58 ` [PATCH 1/3] x86/HVM: properly propagate errors from HVMOP_inject_msi Jan Beulich 2014-06-03 13:59 ` [PATCH 2/3] x86/HVM: make vmsi_deliver() return proper error values Jan Beulich @ 2014-06-03 14:00 ` Jan Beulich 2014-06-03 14:50 ` Andrew Cooper 2 siblings, 1 reply; 12+ messages in thread From: Jan Beulich @ 2014-06-03 14:00 UTC (permalink / raw) To: xen-devel Cc: Kevin Tian, Keir Fraser, Ian Jackson, Ian Campbell, Stefano Stabellini, Yang Z Zhang [-- Attachment #1: Type: text/plain, Size: 17597 bytes --] Tools side: - don't silently ignore unrecognized PT_IRQ_TYPE_* values - respect that the interface type contains a union, making the code at once no longer depend on the hypervisor ignoring the bus field of the PCI portion of the interface structure) Hypervisor side: - don't ignore the PCI bus number passed in - don't store values (gsi, link) calculated from other stored values - avoid calling xfree() with a spin lock held where easily possible - have pt_irq_destroy_bind() respect the passed in type - scope reduction and constification of various variables - use switch instead of if/else-if chains - formatting Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/tools/libxc/xc_domain.c +++ b/tools/libxc/xc_domain.c @@ -1702,15 +1702,20 @@ int xc_domain_bind_pt_irq( bind->hvm_domid = domid; bind->irq_type = irq_type; bind->machine_irq = machine_irq; - if ( irq_type == PT_IRQ_TYPE_PCI || - irq_type == PT_IRQ_TYPE_MSI_TRANSLATE ) + switch ( irq_type ) { + case PT_IRQ_TYPE_PCI: + case PT_IRQ_TYPE_MSI_TRANSLATE: bind->u.pci.bus = bus; bind->u.pci.device = device; bind->u.pci.intx = intx; - } - else if ( irq_type == PT_IRQ_TYPE_ISA ) + case PT_IRQ_TYPE_ISA: bind->u.isa.isa_irq = isa_irq; + break; + default: + errno = EINVAL; + return -1; + } rc = do_domctl(xch, &domctl); return rc; @@ -1737,10 +1742,21 @@ int xc_domain_unbind_pt_irq( bind->hvm_domid = domid; bind->irq_type = irq_type; bind->machine_irq = machine_irq; - bind->u.pci.bus = bus; - bind->u.pci.device = device; - bind->u.pci.intx = intx; - bind->u.isa.isa_irq = isa_irq; + switch ( irq_type ) + { + case PT_IRQ_TYPE_PCI: + case PT_IRQ_TYPE_MSI_TRANSLATE: + bind->u.pci.bus = bus; + bind->u.pci.device = device; + bind->u.pci.intx = intx; + break; + case PT_IRQ_TYPE_ISA: + bind->u.isa.isa_irq = isa_irq; + break; + default: + errno = EINVAL; + return -1; + } rc = do_domctl(xch, &domctl); return rc; --- a/xen/arch/x86/physdev.c +++ b/xen/arch/x86/physdev.c @@ -37,9 +37,8 @@ static int physdev_hvm_map_pirq( switch ( type ) { case MAP_PIRQ_TYPE_GSI: { - struct hvm_irq_dpci *hvm_irq_dpci; - struct hvm_girq_dpci_mapping *girq; - uint32_t machine_gsi = 0; + const struct hvm_irq_dpci *hvm_irq_dpci; + unsigned int machine_gsi = 0; if ( *index < 0 || *index >= NR_HVM_IRQS ) { @@ -52,6 +51,8 @@ static int physdev_hvm_map_pirq( hvm_irq_dpci = domain_get_irq_dpci(d); if ( hvm_irq_dpci ) { + const struct hvm_girq_dpci_mapping *girq; + BUILD_BUG_ON(ARRAY_SIZE(hvm_irq_dpci->girq) < NR_HVM_IRQS); list_for_each_entry ( girq, &hvm_irq_dpci->girq[*index], --- a/xen/drivers/passthrough/io.c +++ b/xen/drivers/passthrough/io.c @@ -51,11 +51,8 @@ static int pt_irq_guest_eoi(struct domai static void pt_irq_time_out(void *data) { struct hvm_pirq_dpci *irq_map = data; - unsigned int guest_gsi; - struct hvm_irq_dpci *dpci = NULL; - struct dev_intx_gsi_link *digl; - struct hvm_girq_dpci_mapping *girq; - uint32_t device, intx; + const struct hvm_irq_dpci *dpci; + const struct dev_intx_gsi_link *digl; spin_lock(&irq_map->dom->event_lock); @@ -63,16 +60,16 @@ static void pt_irq_time_out(void *data) ASSERT(dpci); list_for_each_entry ( digl, &irq_map->digl_list, list ) { - guest_gsi = digl->gsi; + unsigned int guest_gsi = hvm_pci_intx_gsi(digl->device, digl->intx); + const struct hvm_girq_dpci_mapping *girq; + list_for_each_entry ( girq, &dpci->girq[guest_gsi], list ) { struct pirq *pirq = pirq_info(irq_map->dom, girq->machine_gsi); pirq_dpci(pirq)->flags |= HVM_IRQ_DPCI_EOI_LATCH; } - device = digl->device; - intx = digl->intx; - hvm_pci_intx_deassert(irq_map->dom, device, intx); + hvm_pci_intx_deassert(irq_map->dom, digl->device, digl->intx); } pt_pirq_iterate(irq_map->dom, pt_irq_guest_eoi, NULL); @@ -96,13 +93,9 @@ void free_hvm_irq_dpci(struct hvm_irq_dp int pt_irq_create_bind( struct domain *d, xen_domctl_bind_pt_irq_t *pt_irq_bind) { - struct hvm_irq_dpci *hvm_irq_dpci = NULL; + struct hvm_irq_dpci *hvm_irq_dpci; struct hvm_pirq_dpci *pirq_dpci; struct pirq *info; - uint32_t guest_gsi; - uint32_t device, intx, link; - struct dev_intx_gsi_link *digl; - struct hvm_girq_dpci_mapping *girq; int rc, pirq = pt_irq_bind->machine_irq; if ( pirq < 0 || pirq >= d->nr_pirqs ) @@ -136,7 +129,9 @@ int pt_irq_create_bind( } pirq_dpci = pirq_dpci(info); - if ( pt_irq_bind->irq_type == PT_IRQ_TYPE_MSI ) + switch ( pt_irq_bind->irq_type ) + { + case PT_IRQ_TYPE_MSI: { uint8_t dest, dest_mode; int dest_vcpu_id; @@ -169,15 +164,16 @@ int pt_irq_create_bind( { uint32_t mask = HVM_IRQ_DPCI_MACH_MSI | HVM_IRQ_DPCI_GUEST_MSI; - if ( (pirq_dpci->flags & mask) != mask) + if ( (pirq_dpci->flags & mask) != mask ) { - spin_unlock(&d->event_lock); - return -EBUSY; + spin_unlock(&d->event_lock); + return -EBUSY; } - /* if pirq is already mapped as vmsi, update the guest data/addr */ + /* 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 != pt_irq_bind->u.msi.gflags ) + { /* Directly clear pending EOIs before enabling new MSI info. */ pirq_guest_eoi(info); @@ -185,7 +181,7 @@ int pt_irq_create_bind( pirq_dpci->gmsi.gflags = pt_irq_bind->u.msi.gflags; } } - /* Caculate dest_vcpu_id for MSI-type pirq migration */ + /* Calculate dest_vcpu_id for MSI-type pirq migration. */ dest = pirq_dpci->gmsi.gflags & VMSI_DEST_ID_MASK; dest_mode = !!(pirq_dpci->gmsi.gflags & VMSI_DM_MASK); dest_vcpu_id = hvm_girq_dest_2_vcpu_id(d, dest, dest_mode); @@ -193,36 +189,37 @@ int pt_irq_create_bind( spin_unlock(&d->event_lock); if ( dest_vcpu_id >= 0 ) hvm_migrate_pirqs(d->vcpu[dest_vcpu_id]); + break; } - else + + case PT_IRQ_TYPE_PCI: + case PT_IRQ_TYPE_MSI_TRANSLATE: { - device = pt_irq_bind->u.pci.device; - intx = pt_irq_bind->u.pci.intx; - guest_gsi = hvm_pci_intx_gsi(device, intx); - link = hvm_pci_intx_link(device, intx); - hvm_irq_dpci->link_cnt[link]++; + unsigned int bus = pt_irq_bind->u.pci.bus; + unsigned int device = pt_irq_bind->u.pci.device; + unsigned int intx = pt_irq_bind->u.pci.intx; + unsigned int guest_gsi = hvm_pci_intx_gsi(device, intx); + unsigned int link = hvm_pci_intx_link(device, intx); + struct dev_intx_gsi_link *digl = xmalloc(struct dev_intx_gsi_link); + struct hvm_girq_dpci_mapping *girq = + xmalloc(struct hvm_girq_dpci_mapping); - digl = xmalloc(struct dev_intx_gsi_link); - if ( !digl ) + if ( !digl || !girq ) { spin_unlock(&d->event_lock); - return -ENOMEM; - } - - girq = xmalloc(struct hvm_girq_dpci_mapping); - if ( !girq ) - { + xfree(girq); xfree(digl); - spin_unlock(&d->event_lock); return -ENOMEM; } + hvm_irq_dpci->link_cnt[link]++; + + digl->bus = bus; digl->device = device; digl->intx = intx; - digl->gsi = guest_gsi; - digl->link = link; list_add_tail(&digl->list, &pirq_dpci->digl_list); + girq->bus = bus; girq->device = device; girq->intx = intx; girq->machine_gsi = pirq; @@ -261,12 +258,12 @@ int pt_irq_create_bind( kill_timer(&pirq_dpci->timer); pirq_dpci->dom = NULL; list_del(&girq->list); - xfree(girq); list_del(&digl->list); hvm_irq_dpci->link_cnt[link]--; pirq_dpci->flags = 0; pirq_cleanup_check(info, d); spin_unlock(&d->event_lock); + xfree(girq); xfree(digl); return rc; } @@ -276,33 +273,51 @@ int pt_irq_create_bind( if ( iommu_verbose ) dprintk(XENLOG_G_INFO, - "d%d: bind: m_gsi=%u g_gsi=%u device=%u intx=%u\n", - d->domain_id, pirq, guest_gsi, device, intx); + "d%d: bind: m_gsi=%u g_gsi=%u dev=%02x.%02x.%u intx=%u\n", + d->domain_id, pirq, guest_gsi, bus, + PCI_SLOT(device), PCI_FUNC(device), intx); + break; + } + + default: + spin_unlock(&d->event_lock); + return -EOPNOTSUPP; } + return 0; } int pt_irq_destroy_bind( struct domain *d, xen_domctl_bind_pt_irq_t *pt_irq_bind) { - struct hvm_irq_dpci *hvm_irq_dpci = NULL; + struct hvm_irq_dpci *hvm_irq_dpci; struct hvm_pirq_dpci *pirq_dpci; - uint32_t machine_gsi, guest_gsi; - uint32_t device, intx, link; + unsigned int machine_gsi = pt_irq_bind->machine_irq; + unsigned int bus = pt_irq_bind->u.pci.bus; + unsigned int device = pt_irq_bind->u.pci.device; + unsigned int intx = pt_irq_bind->u.pci.intx; + unsigned int guest_gsi = hvm_pci_intx_gsi(device, intx); + unsigned int link = hvm_pci_intx_link(device, intx); struct dev_intx_gsi_link *digl, *tmp; struct hvm_girq_dpci_mapping *girq; struct pirq *pirq; - machine_gsi = pt_irq_bind->machine_irq; - device = pt_irq_bind->u.pci.device; - intx = pt_irq_bind->u.pci.intx; - guest_gsi = hvm_pci_intx_gsi(device, intx); - link = hvm_pci_intx_link(device, intx); + switch ( pt_irq_bind->irq_type ) + { + case PT_IRQ_TYPE_PCI: + case PT_IRQ_TYPE_MSI_TRANSLATE: + break; + case PT_IRQ_TYPE_MSI: + return 0; + default: + return -EOPNOTSUPP; + } if ( iommu_verbose ) dprintk(XENLOG_G_INFO, - "d%d: unbind: m_gsi=%u g_gsi=%u device=%u intx=%u\n", - d->domain_id, machine_gsi, guest_gsi, device, intx); + "d%d: unbind: m_gsi=%u g_gsi=%u dev=%02x:%02x.%u intx=%u\n", + d->domain_id, machine_gsi, guest_gsi, bus, + PCI_SLOT(device), PCI_FUNC(device), intx); spin_lock(&d->event_lock); @@ -314,18 +329,28 @@ int pt_irq_destroy_bind( return -EINVAL; } - hvm_irq_dpci->link_cnt[link]--; - list_for_each_entry ( girq, &hvm_irq_dpci->girq[guest_gsi], list ) { - if ( girq->machine_gsi == machine_gsi ) + if ( girq->bus == bus && + girq->device == device && + girq->intx == intx && + girq->machine_gsi == machine_gsi ) { - list_del(&girq->list); - xfree(girq); - break; + list_del(&girq->list); + xfree(girq); + girq = NULL; + break; } } + if ( girq ) + { + spin_unlock(&d->event_lock); + return -EINVAL; + } + + hvm_irq_dpci->link_cnt[link]--; + pirq = pirq_info(d, machine_gsi); pirq_dpci = pirq_dpci(pirq); @@ -334,10 +359,9 @@ int pt_irq_destroy_bind( { list_for_each_entry_safe ( digl, tmp, &pirq_dpci->digl_list, list ) { - if ( digl->device == device && - digl->intx == intx && - digl->link == link && - digl->gsi == guest_gsi ) + if ( digl->bus == bus && + digl->device == device && + digl->intx == intx ) { list_del(&digl->list); xfree(digl); @@ -359,8 +383,9 @@ int pt_irq_destroy_bind( if ( iommu_verbose ) dprintk(XENLOG_G_INFO, - "d%d unmap: m_irq=%u device=%u intx=%u\n", - d->domain_id, machine_gsi, device, intx); + "d%d unmap: m_irq=%u dev=%02x:%02x.%u intx=%u\n", + d->domain_id, machine_gsi, bus, + PCI_SLOT(device), PCI_FUNC(device), intx); return 0; } @@ -481,11 +506,10 @@ static void hvm_pci_msi_assert( static int _hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci, void *arg) { - uint32_t device, intx; - struct dev_intx_gsi_link *digl; - if ( test_and_clear_bool(pirq_dpci->masked) ) { + const struct dev_intx_gsi_link *digl; + if ( pirq_dpci->flags & HVM_IRQ_DPCI_GUEST_MSI ) { hvm_pci_msi_assert(d, pirq_dpci); @@ -496,12 +520,10 @@ static int _hvm_dirq_assist(struct domai { struct pirq *info = dpci_pirq(pirq_dpci); - device = digl->device; - intx = digl->intx; if ( hvm_domain_use_pirq(d, info) ) send_guest_pirq(d, info); else - hvm_pci_intx_assert(d, device, intx); + hvm_pci_intx_assert(d, digl->device, digl->intx); pirq_dpci->pending++; if ( pirq_dpci->flags & HVM_IRQ_DPCI_TRANSLATE ) @@ -537,16 +559,13 @@ static void hvm_dirq_assist(unsigned lon } static void __hvm_dpci_eoi(struct domain *d, - struct hvm_girq_dpci_mapping *girq, - union vioapic_redir_entry *ent) + const struct hvm_girq_dpci_mapping *girq, + const union vioapic_redir_entry *ent) { - uint32_t device, intx; struct pirq *pirq; struct hvm_pirq_dpci *pirq_dpci; - device = girq->device; - intx = girq->intx; - hvm_pci_intx_deassert(d, device, intx); + hvm_pci_intx_deassert(d, girq->device, girq->intx); pirq = pirq_info(d, girq->machine_gsi); pirq_dpci = pirq_dpci(pirq); @@ -556,8 +575,8 @@ static void __hvm_dpci_eoi(struct domain * since interrupt is still not EOIed */ if ( --pirq_dpci->pending || - ( ent && ent->fields.mask ) || - ! pt_irq_need_timer(pirq_dpci->flags) ) + (ent && ent->fields.mask) || + !pt_irq_need_timer(pirq_dpci->flags) ) return; stop_timer(&pirq_dpci->timer); @@ -565,10 +584,10 @@ static void __hvm_dpci_eoi(struct domain } void hvm_dpci_eoi(struct domain *d, unsigned int guest_gsi, - union vioapic_redir_entry *ent) + const union vioapic_redir_entry *ent) { - struct hvm_irq_dpci *hvm_irq_dpci; - struct hvm_girq_dpci_mapping *girq; + const struct hvm_irq_dpci *hvm_irq_dpci; + const struct hvm_girq_dpci_mapping *girq; if ( !iommu_enabled ) return; --- a/xen/drivers/passthrough/vtd/x86/vtd.c +++ b/xen/drivers/passthrough/vtd/x86/vtd.c @@ -69,11 +69,13 @@ static int _hvm_dpci_isairq_eoi(struct d { struct hvm_irq *hvm_irq = &d->arch.hvm_domain.irq; unsigned int isairq = (long)arg; - struct dev_intx_gsi_link *digl, *tmp; + const struct dev_intx_gsi_link *digl; - list_for_each_entry_safe ( digl, tmp, &pirq_dpci->digl_list, list ) + list_for_each_entry ( digl, &pirq_dpci->digl_list, list ) { - if ( hvm_irq->pci_link.route[digl->link] == isairq ) + unsigned int link = hvm_pci_intx_link(digl->device, digl->intx); + + if ( hvm_irq->pci_link.route[link] == isairq ) { hvm_pci_intx_deassert(d, digl->device, digl->intx); if ( --pirq_dpci->pending == 0 ) --- a/xen/include/asm-x86/hvm/io.h +++ b/xen/include/asm-x86/hvm/io.h @@ -123,7 +123,7 @@ int handle_pio(uint16_t port, unsigned i void hvm_interrupt_post(struct vcpu *v, int vector, int type); void hvm_io_assist(ioreq_t *p); void hvm_dpci_eoi(struct domain *d, unsigned int guest_irq, - union vioapic_redir_entry *ent); + const union vioapic_redir_entry *ent); void msix_write_completion(struct vcpu *); struct hvm_hw_stdvga { --- a/xen/include/xen/hvm/irq.h +++ b/xen/include/xen/hvm/irq.h @@ -30,10 +30,9 @@ struct dev_intx_gsi_link { struct list_head list; + uint8_t bus; uint8_t device; uint8_t intx; - uint8_t gsi; - uint8_t link; }; #define _HVM_IRQ_DPCI_MACH_PCI_SHIFT 0 @@ -69,6 +68,7 @@ struct hvm_gmsi_info { struct hvm_girq_dpci_mapping { struct list_head list; + uint8_t bus; uint8_t device; uint8_t intx; uint8_t machine_gsi; [-- Attachment #2: pt-irq-binding.patch --] [-- Type: text/plain, Size: 17626 bytes --] pt-irq fixes and improvements Tools side: - don't silently ignore unrecognized PT_IRQ_TYPE_* values - respect that the interface type contains a union, making the code at once no longer depend on the hypervisor ignoring the bus field of the PCI portion of the interface structure) Hypervisor side: - don't ignore the PCI bus number passed in - don't store values (gsi, link) calculated from other stored values - avoid calling xfree() with a spin lock held where easily possible - have pt_irq_destroy_bind() respect the passed in type - scope reduction and constification of various variables - use switch instead of if/else-if chains - formatting Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/tools/libxc/xc_domain.c +++ b/tools/libxc/xc_domain.c @@ -1702,15 +1702,20 @@ int xc_domain_bind_pt_irq( bind->hvm_domid = domid; bind->irq_type = irq_type; bind->machine_irq = machine_irq; - if ( irq_type == PT_IRQ_TYPE_PCI || - irq_type == PT_IRQ_TYPE_MSI_TRANSLATE ) + switch ( irq_type ) { + case PT_IRQ_TYPE_PCI: + case PT_IRQ_TYPE_MSI_TRANSLATE: bind->u.pci.bus = bus; bind->u.pci.device = device; bind->u.pci.intx = intx; - } - else if ( irq_type == PT_IRQ_TYPE_ISA ) + case PT_IRQ_TYPE_ISA: bind->u.isa.isa_irq = isa_irq; + break; + default: + errno = EINVAL; + return -1; + } rc = do_domctl(xch, &domctl); return rc; @@ -1737,10 +1742,21 @@ int xc_domain_unbind_pt_irq( bind->hvm_domid = domid; bind->irq_type = irq_type; bind->machine_irq = machine_irq; - bind->u.pci.bus = bus; - bind->u.pci.device = device; - bind->u.pci.intx = intx; - bind->u.isa.isa_irq = isa_irq; + switch ( irq_type ) + { + case PT_IRQ_TYPE_PCI: + case PT_IRQ_TYPE_MSI_TRANSLATE: + bind->u.pci.bus = bus; + bind->u.pci.device = device; + bind->u.pci.intx = intx; + break; + case PT_IRQ_TYPE_ISA: + bind->u.isa.isa_irq = isa_irq; + break; + default: + errno = EINVAL; + return -1; + } rc = do_domctl(xch, &domctl); return rc; --- a/xen/arch/x86/physdev.c +++ b/xen/arch/x86/physdev.c @@ -37,9 +37,8 @@ static int physdev_hvm_map_pirq( switch ( type ) { case MAP_PIRQ_TYPE_GSI: { - struct hvm_irq_dpci *hvm_irq_dpci; - struct hvm_girq_dpci_mapping *girq; - uint32_t machine_gsi = 0; + const struct hvm_irq_dpci *hvm_irq_dpci; + unsigned int machine_gsi = 0; if ( *index < 0 || *index >= NR_HVM_IRQS ) { @@ -52,6 +51,8 @@ static int physdev_hvm_map_pirq( hvm_irq_dpci = domain_get_irq_dpci(d); if ( hvm_irq_dpci ) { + const struct hvm_girq_dpci_mapping *girq; + BUILD_BUG_ON(ARRAY_SIZE(hvm_irq_dpci->girq) < NR_HVM_IRQS); list_for_each_entry ( girq, &hvm_irq_dpci->girq[*index], --- a/xen/drivers/passthrough/io.c +++ b/xen/drivers/passthrough/io.c @@ -51,11 +51,8 @@ static int pt_irq_guest_eoi(struct domai static void pt_irq_time_out(void *data) { struct hvm_pirq_dpci *irq_map = data; - unsigned int guest_gsi; - struct hvm_irq_dpci *dpci = NULL; - struct dev_intx_gsi_link *digl; - struct hvm_girq_dpci_mapping *girq; - uint32_t device, intx; + const struct hvm_irq_dpci *dpci; + const struct dev_intx_gsi_link *digl; spin_lock(&irq_map->dom->event_lock); @@ -63,16 +60,16 @@ static void pt_irq_time_out(void *data) ASSERT(dpci); list_for_each_entry ( digl, &irq_map->digl_list, list ) { - guest_gsi = digl->gsi; + unsigned int guest_gsi = hvm_pci_intx_gsi(digl->device, digl->intx); + const struct hvm_girq_dpci_mapping *girq; + list_for_each_entry ( girq, &dpci->girq[guest_gsi], list ) { struct pirq *pirq = pirq_info(irq_map->dom, girq->machine_gsi); pirq_dpci(pirq)->flags |= HVM_IRQ_DPCI_EOI_LATCH; } - device = digl->device; - intx = digl->intx; - hvm_pci_intx_deassert(irq_map->dom, device, intx); + hvm_pci_intx_deassert(irq_map->dom, digl->device, digl->intx); } pt_pirq_iterate(irq_map->dom, pt_irq_guest_eoi, NULL); @@ -96,13 +93,9 @@ void free_hvm_irq_dpci(struct hvm_irq_dp int pt_irq_create_bind( struct domain *d, xen_domctl_bind_pt_irq_t *pt_irq_bind) { - struct hvm_irq_dpci *hvm_irq_dpci = NULL; + struct hvm_irq_dpci *hvm_irq_dpci; struct hvm_pirq_dpci *pirq_dpci; struct pirq *info; - uint32_t guest_gsi; - uint32_t device, intx, link; - struct dev_intx_gsi_link *digl; - struct hvm_girq_dpci_mapping *girq; int rc, pirq = pt_irq_bind->machine_irq; if ( pirq < 0 || pirq >= d->nr_pirqs ) @@ -136,7 +129,9 @@ int pt_irq_create_bind( } pirq_dpci = pirq_dpci(info); - if ( pt_irq_bind->irq_type == PT_IRQ_TYPE_MSI ) + switch ( pt_irq_bind->irq_type ) + { + case PT_IRQ_TYPE_MSI: { uint8_t dest, dest_mode; int dest_vcpu_id; @@ -169,15 +164,16 @@ int pt_irq_create_bind( { uint32_t mask = HVM_IRQ_DPCI_MACH_MSI | HVM_IRQ_DPCI_GUEST_MSI; - if ( (pirq_dpci->flags & mask) != mask) + if ( (pirq_dpci->flags & mask) != mask ) { - spin_unlock(&d->event_lock); - return -EBUSY; + spin_unlock(&d->event_lock); + return -EBUSY; } - /* if pirq is already mapped as vmsi, update the guest data/addr */ + /* 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 != pt_irq_bind->u.msi.gflags ) + { /* Directly clear pending EOIs before enabling new MSI info. */ pirq_guest_eoi(info); @@ -185,7 +181,7 @@ int pt_irq_create_bind( pirq_dpci->gmsi.gflags = pt_irq_bind->u.msi.gflags; } } - /* Caculate dest_vcpu_id for MSI-type pirq migration */ + /* Calculate dest_vcpu_id for MSI-type pirq migration. */ dest = pirq_dpci->gmsi.gflags & VMSI_DEST_ID_MASK; dest_mode = !!(pirq_dpci->gmsi.gflags & VMSI_DM_MASK); dest_vcpu_id = hvm_girq_dest_2_vcpu_id(d, dest, dest_mode); @@ -193,36 +189,37 @@ int pt_irq_create_bind( spin_unlock(&d->event_lock); if ( dest_vcpu_id >= 0 ) hvm_migrate_pirqs(d->vcpu[dest_vcpu_id]); + break; } - else + + case PT_IRQ_TYPE_PCI: + case PT_IRQ_TYPE_MSI_TRANSLATE: { - device = pt_irq_bind->u.pci.device; - intx = pt_irq_bind->u.pci.intx; - guest_gsi = hvm_pci_intx_gsi(device, intx); - link = hvm_pci_intx_link(device, intx); - hvm_irq_dpci->link_cnt[link]++; + unsigned int bus = pt_irq_bind->u.pci.bus; + unsigned int device = pt_irq_bind->u.pci.device; + unsigned int intx = pt_irq_bind->u.pci.intx; + unsigned int guest_gsi = hvm_pci_intx_gsi(device, intx); + unsigned int link = hvm_pci_intx_link(device, intx); + struct dev_intx_gsi_link *digl = xmalloc(struct dev_intx_gsi_link); + struct hvm_girq_dpci_mapping *girq = + xmalloc(struct hvm_girq_dpci_mapping); - digl = xmalloc(struct dev_intx_gsi_link); - if ( !digl ) + if ( !digl || !girq ) { spin_unlock(&d->event_lock); - return -ENOMEM; - } - - girq = xmalloc(struct hvm_girq_dpci_mapping); - if ( !girq ) - { + xfree(girq); xfree(digl); - spin_unlock(&d->event_lock); return -ENOMEM; } + hvm_irq_dpci->link_cnt[link]++; + + digl->bus = bus; digl->device = device; digl->intx = intx; - digl->gsi = guest_gsi; - digl->link = link; list_add_tail(&digl->list, &pirq_dpci->digl_list); + girq->bus = bus; girq->device = device; girq->intx = intx; girq->machine_gsi = pirq; @@ -261,12 +258,12 @@ int pt_irq_create_bind( kill_timer(&pirq_dpci->timer); pirq_dpci->dom = NULL; list_del(&girq->list); - xfree(girq); list_del(&digl->list); hvm_irq_dpci->link_cnt[link]--; pirq_dpci->flags = 0; pirq_cleanup_check(info, d); spin_unlock(&d->event_lock); + xfree(girq); xfree(digl); return rc; } @@ -276,33 +273,51 @@ int pt_irq_create_bind( if ( iommu_verbose ) dprintk(XENLOG_G_INFO, - "d%d: bind: m_gsi=%u g_gsi=%u device=%u intx=%u\n", - d->domain_id, pirq, guest_gsi, device, intx); + "d%d: bind: m_gsi=%u g_gsi=%u dev=%02x.%02x.%u intx=%u\n", + d->domain_id, pirq, guest_gsi, bus, + PCI_SLOT(device), PCI_FUNC(device), intx); + break; + } + + default: + spin_unlock(&d->event_lock); + return -EOPNOTSUPP; } + return 0; } int pt_irq_destroy_bind( struct domain *d, xen_domctl_bind_pt_irq_t *pt_irq_bind) { - struct hvm_irq_dpci *hvm_irq_dpci = NULL; + struct hvm_irq_dpci *hvm_irq_dpci; struct hvm_pirq_dpci *pirq_dpci; - uint32_t machine_gsi, guest_gsi; - uint32_t device, intx, link; + unsigned int machine_gsi = pt_irq_bind->machine_irq; + unsigned int bus = pt_irq_bind->u.pci.bus; + unsigned int device = pt_irq_bind->u.pci.device; + unsigned int intx = pt_irq_bind->u.pci.intx; + unsigned int guest_gsi = hvm_pci_intx_gsi(device, intx); + unsigned int link = hvm_pci_intx_link(device, intx); struct dev_intx_gsi_link *digl, *tmp; struct hvm_girq_dpci_mapping *girq; struct pirq *pirq; - machine_gsi = pt_irq_bind->machine_irq; - device = pt_irq_bind->u.pci.device; - intx = pt_irq_bind->u.pci.intx; - guest_gsi = hvm_pci_intx_gsi(device, intx); - link = hvm_pci_intx_link(device, intx); + switch ( pt_irq_bind->irq_type ) + { + case PT_IRQ_TYPE_PCI: + case PT_IRQ_TYPE_MSI_TRANSLATE: + break; + case PT_IRQ_TYPE_MSI: + return 0; + default: + return -EOPNOTSUPP; + } if ( iommu_verbose ) dprintk(XENLOG_G_INFO, - "d%d: unbind: m_gsi=%u g_gsi=%u device=%u intx=%u\n", - d->domain_id, machine_gsi, guest_gsi, device, intx); + "d%d: unbind: m_gsi=%u g_gsi=%u dev=%02x:%02x.%u intx=%u\n", + d->domain_id, machine_gsi, guest_gsi, bus, + PCI_SLOT(device), PCI_FUNC(device), intx); spin_lock(&d->event_lock); @@ -314,18 +329,28 @@ int pt_irq_destroy_bind( return -EINVAL; } - hvm_irq_dpci->link_cnt[link]--; - list_for_each_entry ( girq, &hvm_irq_dpci->girq[guest_gsi], list ) { - if ( girq->machine_gsi == machine_gsi ) + if ( girq->bus == bus && + girq->device == device && + girq->intx == intx && + girq->machine_gsi == machine_gsi ) { - list_del(&girq->list); - xfree(girq); - break; + list_del(&girq->list); + xfree(girq); + girq = NULL; + break; } } + if ( girq ) + { + spin_unlock(&d->event_lock); + return -EINVAL; + } + + hvm_irq_dpci->link_cnt[link]--; + pirq = pirq_info(d, machine_gsi); pirq_dpci = pirq_dpci(pirq); @@ -334,10 +359,9 @@ int pt_irq_destroy_bind( { list_for_each_entry_safe ( digl, tmp, &pirq_dpci->digl_list, list ) { - if ( digl->device == device && - digl->intx == intx && - digl->link == link && - digl->gsi == guest_gsi ) + if ( digl->bus == bus && + digl->device == device && + digl->intx == intx ) { list_del(&digl->list); xfree(digl); @@ -359,8 +383,9 @@ int pt_irq_destroy_bind( if ( iommu_verbose ) dprintk(XENLOG_G_INFO, - "d%d unmap: m_irq=%u device=%u intx=%u\n", - d->domain_id, machine_gsi, device, intx); + "d%d unmap: m_irq=%u dev=%02x:%02x.%u intx=%u\n", + d->domain_id, machine_gsi, bus, + PCI_SLOT(device), PCI_FUNC(device), intx); return 0; } @@ -481,11 +506,10 @@ static void hvm_pci_msi_assert( static int _hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci, void *arg) { - uint32_t device, intx; - struct dev_intx_gsi_link *digl; - if ( test_and_clear_bool(pirq_dpci->masked) ) { + const struct dev_intx_gsi_link *digl; + if ( pirq_dpci->flags & HVM_IRQ_DPCI_GUEST_MSI ) { hvm_pci_msi_assert(d, pirq_dpci); @@ -496,12 +520,10 @@ static int _hvm_dirq_assist(struct domai { struct pirq *info = dpci_pirq(pirq_dpci); - device = digl->device; - intx = digl->intx; if ( hvm_domain_use_pirq(d, info) ) send_guest_pirq(d, info); else - hvm_pci_intx_assert(d, device, intx); + hvm_pci_intx_assert(d, digl->device, digl->intx); pirq_dpci->pending++; if ( pirq_dpci->flags & HVM_IRQ_DPCI_TRANSLATE ) @@ -537,16 +559,13 @@ static void hvm_dirq_assist(unsigned lon } static void __hvm_dpci_eoi(struct domain *d, - struct hvm_girq_dpci_mapping *girq, - union vioapic_redir_entry *ent) + const struct hvm_girq_dpci_mapping *girq, + const union vioapic_redir_entry *ent) { - uint32_t device, intx; struct pirq *pirq; struct hvm_pirq_dpci *pirq_dpci; - device = girq->device; - intx = girq->intx; - hvm_pci_intx_deassert(d, device, intx); + hvm_pci_intx_deassert(d, girq->device, girq->intx); pirq = pirq_info(d, girq->machine_gsi); pirq_dpci = pirq_dpci(pirq); @@ -556,8 +575,8 @@ static void __hvm_dpci_eoi(struct domain * since interrupt is still not EOIed */ if ( --pirq_dpci->pending || - ( ent && ent->fields.mask ) || - ! pt_irq_need_timer(pirq_dpci->flags) ) + (ent && ent->fields.mask) || + !pt_irq_need_timer(pirq_dpci->flags) ) return; stop_timer(&pirq_dpci->timer); @@ -565,10 +584,10 @@ static void __hvm_dpci_eoi(struct domain } void hvm_dpci_eoi(struct domain *d, unsigned int guest_gsi, - union vioapic_redir_entry *ent) + const union vioapic_redir_entry *ent) { - struct hvm_irq_dpci *hvm_irq_dpci; - struct hvm_girq_dpci_mapping *girq; + const struct hvm_irq_dpci *hvm_irq_dpci; + const struct hvm_girq_dpci_mapping *girq; if ( !iommu_enabled ) return; --- a/xen/drivers/passthrough/vtd/x86/vtd.c +++ b/xen/drivers/passthrough/vtd/x86/vtd.c @@ -69,11 +69,13 @@ static int _hvm_dpci_isairq_eoi(struct d { struct hvm_irq *hvm_irq = &d->arch.hvm_domain.irq; unsigned int isairq = (long)arg; - struct dev_intx_gsi_link *digl, *tmp; + const struct dev_intx_gsi_link *digl; - list_for_each_entry_safe ( digl, tmp, &pirq_dpci->digl_list, list ) + list_for_each_entry ( digl, &pirq_dpci->digl_list, list ) { - if ( hvm_irq->pci_link.route[digl->link] == isairq ) + unsigned int link = hvm_pci_intx_link(digl->device, digl->intx); + + if ( hvm_irq->pci_link.route[link] == isairq ) { hvm_pci_intx_deassert(d, digl->device, digl->intx); if ( --pirq_dpci->pending == 0 ) --- a/xen/include/asm-x86/hvm/io.h +++ b/xen/include/asm-x86/hvm/io.h @@ -123,7 +123,7 @@ int handle_pio(uint16_t port, unsigned i void hvm_interrupt_post(struct vcpu *v, int vector, int type); void hvm_io_assist(ioreq_t *p); void hvm_dpci_eoi(struct domain *d, unsigned int guest_irq, - union vioapic_redir_entry *ent); + const union vioapic_redir_entry *ent); void msix_write_completion(struct vcpu *); struct hvm_hw_stdvga { --- a/xen/include/xen/hvm/irq.h +++ b/xen/include/xen/hvm/irq.h @@ -30,10 +30,9 @@ struct dev_intx_gsi_link { struct list_head list; + uint8_t bus; uint8_t device; uint8_t intx; - uint8_t gsi; - uint8_t link; }; #define _HVM_IRQ_DPCI_MACH_PCI_SHIFT 0 @@ -69,6 +68,7 @@ struct hvm_gmsi_info { struct hvm_girq_dpci_mapping { struct list_head list; + uint8_t bus; uint8_t device; uint8_t intx; uint8_t machine_gsi; [-- Attachment #3: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] pt-irq fixes and improvements 2014-06-03 14:00 ` [PATCH 3/3] pt-irq fixes and improvements Jan Beulich @ 2014-06-03 14:50 ` Andrew Cooper 2014-06-04 8:24 ` [PATCH v2 " Jan Beulich 0 siblings, 1 reply; 12+ messages in thread From: Andrew Cooper @ 2014-06-03 14:50 UTC (permalink / raw) To: Jan Beulich Cc: Kevin Tian, Keir Fraser, Ian Jackson, Ian Campbell, Stefano Stabellini, Yang Z Zhang, xen-devel On 03/06/14 15:00, Jan Beulich wrote: > Tools side: > - don't silently ignore unrecognized PT_IRQ_TYPE_* values > - respect that the interface type contains a union, making the code at > once no longer depend on the hypervisor ignoring the bus field of the > PCI portion of the interface structure) > > Hypervisor side: > - don't ignore the PCI bus number passed in > - don't store values (gsi, link) calculated from other stored values > - avoid calling xfree() with a spin lock held where easily possible > - have pt_irq_destroy_bind() respect the passed in type > - scope reduction and constification of various variables > - use switch instead of if/else-if chains > - formatting > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> with a few suggestions for further cleanup... > --- a/tools/libxc/xc_domain.c > +++ b/tools/libxc/xc_domain.c > @@ -1702,15 +1702,20 @@ int xc_domain_bind_pt_irq( > bind->hvm_domid = domid; > bind->irq_type = irq_type; > bind->machine_irq = machine_irq; > - if ( irq_type == PT_IRQ_TYPE_PCI || > - irq_type == PT_IRQ_TYPE_MSI_TRANSLATE ) > + switch ( irq_type ) > { > + case PT_IRQ_TYPE_PCI: > + case PT_IRQ_TYPE_MSI_TRANSLATE: > bind->u.pci.bus = bus; > bind->u.pci.device = device; Given all this other cleanup, can you nuke the trailing whitespace here... > bind->u.pci.intx = intx; > - } > - else if ( irq_type == PT_IRQ_TYPE_ISA ) > + case PT_IRQ_TYPE_ISA: > bind->u.isa.isa_irq = isa_irq; > + break; > + default: > + errno = EINVAL; > + return -1; > + } > ... and here. > rc = do_domctl(xch, &domctl); > return rc; > @@ -1737,10 +1742,21 @@ int xc_domain_unbind_pt_irq( > bind->hvm_domid = domid; > bind->irq_type = irq_type; > bind->machine_irq = machine_irq; > - bind->u.pci.bus = bus; > - bind->u.pci.device = device; > - bind->u.pci.intx = intx; > - bind->u.isa.isa_irq = isa_irq; > + switch ( irq_type ) > + { > + case PT_IRQ_TYPE_PCI: > + case PT_IRQ_TYPE_MSI_TRANSLATE: > + bind->u.pci.bus = bus; > + bind->u.pci.device = device; > + bind->u.pci.intx = intx; > + break; > + case PT_IRQ_TYPE_ISA: > + bind->u.isa.isa_irq = isa_irq; > + break; > + default: > + errno = EINVAL; > + return -1; > + } > And here in this function. > @@ -96,13 +93,9 @@ void free_hvm_irq_dpci(struct hvm_irq_dp > int pt_irq_create_bind( > struct domain *d, xen_domctl_bind_pt_irq_t *pt_irq_bind) > { > - struct hvm_irq_dpci *hvm_irq_dpci = NULL; > + struct hvm_irq_dpci *hvm_irq_dpci; > struct hvm_pirq_dpci *pirq_dpci; > struct pirq *info; > - uint32_t guest_gsi; > - uint32_t device, intx, link; > - struct dev_intx_gsi_link *digl; > - struct hvm_girq_dpci_mapping *girq; > int rc, pirq = pt_irq_bind->machine_irq; > > if ( pirq < 0 || pirq >= d->nr_pirqs ) Given all the cleanup, there is a further piece which could be done between these two hunks. There is an int i (declared in a for loop initialiser, despite our fairly consistent C89 style), which should be unsigned. > @@ -136,7 +129,9 @@ int pt_irq_create_bind( > } > pirq_dpci = pirq_dpci(info); > > - if ( pt_irq_bind->irq_type == PT_IRQ_TYPE_MSI ) > + switch ( pt_irq_bind->irq_type ) > + { > + case PT_IRQ_TYPE_MSI: > { > uint8_t dest, dest_mode; > int dest_vcpu_id; > ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 3/3] pt-irq fixes and improvements 2014-06-03 14:50 ` Andrew Cooper @ 2014-06-04 8:24 ` Jan Beulich 2014-06-10 0:32 ` Zhang, Yang Z ` (2 more replies) 0 siblings, 3 replies; 12+ messages in thread From: Jan Beulich @ 2014-06-04 8:24 UTC (permalink / raw) To: xen-devel Cc: Kevin Tian, Keir Fraser, Andrew Cooper, Ian Jackson, Ian Campbell, Stefano Stabellini, Yang Z Zhang [-- Attachment #1: Type: text/plain, Size: 18436 bytes --] Tools side: - don't silently ignore unrecognized PT_IRQ_TYPE_* values - respect that the interface type contains a union, making the code at once no longer depend on the hypervisor ignoring the bus field of the PCI portion of the interface structure) Hypervisor side: - don't ignore the PCI bus number passed in - don't store values (gsi, link) calculated from other stored values - avoid calling xfree() with a spin lock held where easily possible - have pt_irq_destroy_bind() respect the passed in type - scope reduction and constification of various variables - use switch instead of if/else-if chains - formatting Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> --- v2: A few extra coding style corrections according to Andrew's suggestions. --- a/tools/libxc/xc_domain.c +++ b/tools/libxc/xc_domain.c @@ -1702,16 +1702,21 @@ int xc_domain_bind_pt_irq( bind->hvm_domid = domid; bind->irq_type = irq_type; bind->machine_irq = machine_irq; - if ( irq_type == PT_IRQ_TYPE_PCI || - irq_type == PT_IRQ_TYPE_MSI_TRANSLATE ) + switch ( irq_type ) { + case PT_IRQ_TYPE_PCI: + case PT_IRQ_TYPE_MSI_TRANSLATE: bind->u.pci.bus = bus; - bind->u.pci.device = device; + bind->u.pci.device = device; bind->u.pci.intx = intx; - } - else if ( irq_type == PT_IRQ_TYPE_ISA ) + case PT_IRQ_TYPE_ISA: bind->u.isa.isa_irq = isa_irq; - + break; + default: + errno = EINVAL; + return -1; + } + rc = do_domctl(xch, &domctl); return rc; } @@ -1737,11 +1742,22 @@ int xc_domain_unbind_pt_irq( bind->hvm_domid = domid; bind->irq_type = irq_type; bind->machine_irq = machine_irq; - bind->u.pci.bus = bus; - bind->u.pci.device = device; - bind->u.pci.intx = intx; - bind->u.isa.isa_irq = isa_irq; - + switch ( irq_type ) + { + case PT_IRQ_TYPE_PCI: + case PT_IRQ_TYPE_MSI_TRANSLATE: + bind->u.pci.bus = bus; + bind->u.pci.device = device; + bind->u.pci.intx = intx; + break; + case PT_IRQ_TYPE_ISA: + bind->u.isa.isa_irq = isa_irq; + break; + default: + errno = EINVAL; + return -1; + } + rc = do_domctl(xch, &domctl); return rc; } --- a/xen/arch/x86/physdev.c +++ b/xen/arch/x86/physdev.c @@ -37,9 +37,8 @@ static int physdev_hvm_map_pirq( switch ( type ) { case MAP_PIRQ_TYPE_GSI: { - struct hvm_irq_dpci *hvm_irq_dpci; - struct hvm_girq_dpci_mapping *girq; - uint32_t machine_gsi = 0; + const struct hvm_irq_dpci *hvm_irq_dpci; + unsigned int machine_gsi = 0; if ( *index < 0 || *index >= NR_HVM_IRQS ) { @@ -52,6 +51,8 @@ static int physdev_hvm_map_pirq( hvm_irq_dpci = domain_get_irq_dpci(d); if ( hvm_irq_dpci ) { + const struct hvm_girq_dpci_mapping *girq; + BUILD_BUG_ON(ARRAY_SIZE(hvm_irq_dpci->girq) < NR_HVM_IRQS); list_for_each_entry ( girq, &hvm_irq_dpci->girq[*index], --- a/xen/drivers/passthrough/io.c +++ b/xen/drivers/passthrough/io.c @@ -51,11 +51,8 @@ static int pt_irq_guest_eoi(struct domai static void pt_irq_time_out(void *data) { struct hvm_pirq_dpci *irq_map = data; - unsigned int guest_gsi; - struct hvm_irq_dpci *dpci = NULL; - struct dev_intx_gsi_link *digl; - struct hvm_girq_dpci_mapping *girq; - uint32_t device, intx; + const struct hvm_irq_dpci *dpci; + const struct dev_intx_gsi_link *digl; spin_lock(&irq_map->dom->event_lock); @@ -63,16 +60,16 @@ static void pt_irq_time_out(void *data) ASSERT(dpci); list_for_each_entry ( digl, &irq_map->digl_list, list ) { - guest_gsi = digl->gsi; + unsigned int guest_gsi = hvm_pci_intx_gsi(digl->device, digl->intx); + const struct hvm_girq_dpci_mapping *girq; + list_for_each_entry ( girq, &dpci->girq[guest_gsi], list ) { struct pirq *pirq = pirq_info(irq_map->dom, girq->machine_gsi); pirq_dpci(pirq)->flags |= HVM_IRQ_DPCI_EOI_LATCH; } - device = digl->device; - intx = digl->intx; - hvm_pci_intx_deassert(irq_map->dom, device, intx); + hvm_pci_intx_deassert(irq_map->dom, digl->device, digl->intx); } pt_pirq_iterate(irq_map->dom, pt_irq_guest_eoi, NULL); @@ -96,13 +93,9 @@ void free_hvm_irq_dpci(struct hvm_irq_dp int pt_irq_create_bind( struct domain *d, xen_domctl_bind_pt_irq_t *pt_irq_bind) { - struct hvm_irq_dpci *hvm_irq_dpci = NULL; + struct hvm_irq_dpci *hvm_irq_dpci; struct hvm_pirq_dpci *pirq_dpci; struct pirq *info; - uint32_t guest_gsi; - uint32_t device, intx, link; - struct dev_intx_gsi_link *digl; - struct hvm_girq_dpci_mapping *girq; int rc, pirq = pt_irq_bind->machine_irq; if ( pirq < 0 || pirq >= d->nr_pirqs ) @@ -113,6 +106,8 @@ int pt_irq_create_bind( hvm_irq_dpci = domain_get_irq_dpci(d); if ( hvm_irq_dpci == NULL ) { + unsigned int i; + hvm_irq_dpci = xzalloc(struct hvm_irq_dpci); if ( hvm_irq_dpci == NULL ) { @@ -122,7 +117,7 @@ int pt_irq_create_bind( softirq_tasklet_init( &hvm_irq_dpci->dirq_tasklet, hvm_dirq_assist, (unsigned long)d); - for ( int i = 0; i < NR_HVM_IRQS; i++ ) + for ( i = 0; i < NR_HVM_IRQS; i++ ) INIT_LIST_HEAD(&hvm_irq_dpci->girq[i]); d->arch.hvm_domain.irq.dpci = hvm_irq_dpci; @@ -136,7 +131,9 @@ int pt_irq_create_bind( } pirq_dpci = pirq_dpci(info); - if ( pt_irq_bind->irq_type == PT_IRQ_TYPE_MSI ) + switch ( pt_irq_bind->irq_type ) + { + case PT_IRQ_TYPE_MSI: { uint8_t dest, dest_mode; int dest_vcpu_id; @@ -169,15 +166,16 @@ int pt_irq_create_bind( { uint32_t mask = HVM_IRQ_DPCI_MACH_MSI | HVM_IRQ_DPCI_GUEST_MSI; - if ( (pirq_dpci->flags & mask) != mask) + if ( (pirq_dpci->flags & mask) != mask ) { - spin_unlock(&d->event_lock); - return -EBUSY; + spin_unlock(&d->event_lock); + return -EBUSY; } - /* if pirq is already mapped as vmsi, update the guest data/addr */ + /* 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 != pt_irq_bind->u.msi.gflags ) + { /* Directly clear pending EOIs before enabling new MSI info. */ pirq_guest_eoi(info); @@ -185,7 +183,7 @@ int pt_irq_create_bind( pirq_dpci->gmsi.gflags = pt_irq_bind->u.msi.gflags; } } - /* Caculate dest_vcpu_id for MSI-type pirq migration */ + /* Calculate dest_vcpu_id for MSI-type pirq migration. */ dest = pirq_dpci->gmsi.gflags & VMSI_DEST_ID_MASK; dest_mode = !!(pirq_dpci->gmsi.gflags & VMSI_DM_MASK); dest_vcpu_id = hvm_girq_dest_2_vcpu_id(d, dest, dest_mode); @@ -193,36 +191,37 @@ int pt_irq_create_bind( spin_unlock(&d->event_lock); if ( dest_vcpu_id >= 0 ) hvm_migrate_pirqs(d->vcpu[dest_vcpu_id]); + break; } - else + + case PT_IRQ_TYPE_PCI: + case PT_IRQ_TYPE_MSI_TRANSLATE: { - device = pt_irq_bind->u.pci.device; - intx = pt_irq_bind->u.pci.intx; - guest_gsi = hvm_pci_intx_gsi(device, intx); - link = hvm_pci_intx_link(device, intx); - hvm_irq_dpci->link_cnt[link]++; + unsigned int bus = pt_irq_bind->u.pci.bus; + unsigned int device = pt_irq_bind->u.pci.device; + unsigned int intx = pt_irq_bind->u.pci.intx; + unsigned int guest_gsi = hvm_pci_intx_gsi(device, intx); + unsigned int link = hvm_pci_intx_link(device, intx); + struct dev_intx_gsi_link *digl = xmalloc(struct dev_intx_gsi_link); + struct hvm_girq_dpci_mapping *girq = + xmalloc(struct hvm_girq_dpci_mapping); - digl = xmalloc(struct dev_intx_gsi_link); - if ( !digl ) + if ( !digl || !girq ) { spin_unlock(&d->event_lock); - return -ENOMEM; - } - - girq = xmalloc(struct hvm_girq_dpci_mapping); - if ( !girq ) - { + xfree(girq); xfree(digl); - spin_unlock(&d->event_lock); return -ENOMEM; } + hvm_irq_dpci->link_cnt[link]++; + + digl->bus = bus; digl->device = device; digl->intx = intx; - digl->gsi = guest_gsi; - digl->link = link; list_add_tail(&digl->list, &pirq_dpci->digl_list); + girq->bus = bus; girq->device = device; girq->intx = intx; girq->machine_gsi = pirq; @@ -261,12 +260,12 @@ int pt_irq_create_bind( kill_timer(&pirq_dpci->timer); pirq_dpci->dom = NULL; list_del(&girq->list); - xfree(girq); list_del(&digl->list); hvm_irq_dpci->link_cnt[link]--; pirq_dpci->flags = 0; pirq_cleanup_check(info, d); spin_unlock(&d->event_lock); + xfree(girq); xfree(digl); return rc; } @@ -276,33 +275,51 @@ int pt_irq_create_bind( if ( iommu_verbose ) dprintk(XENLOG_G_INFO, - "d%d: bind: m_gsi=%u g_gsi=%u device=%u intx=%u\n", - d->domain_id, pirq, guest_gsi, device, intx); + "d%d: bind: m_gsi=%u g_gsi=%u dev=%02x.%02x.%u intx=%u\n", + d->domain_id, pirq, guest_gsi, bus, + PCI_SLOT(device), PCI_FUNC(device), intx); + break; } + + default: + spin_unlock(&d->event_lock); + return -EOPNOTSUPP; + } + return 0; } int pt_irq_destroy_bind( struct domain *d, xen_domctl_bind_pt_irq_t *pt_irq_bind) { - struct hvm_irq_dpci *hvm_irq_dpci = NULL; + struct hvm_irq_dpci *hvm_irq_dpci; struct hvm_pirq_dpci *pirq_dpci; - uint32_t machine_gsi, guest_gsi; - uint32_t device, intx, link; + unsigned int machine_gsi = pt_irq_bind->machine_irq; + unsigned int bus = pt_irq_bind->u.pci.bus; + unsigned int device = pt_irq_bind->u.pci.device; + unsigned int intx = pt_irq_bind->u.pci.intx; + unsigned int guest_gsi = hvm_pci_intx_gsi(device, intx); + unsigned int link = hvm_pci_intx_link(device, intx); struct dev_intx_gsi_link *digl, *tmp; struct hvm_girq_dpci_mapping *girq; struct pirq *pirq; - machine_gsi = pt_irq_bind->machine_irq; - device = pt_irq_bind->u.pci.device; - intx = pt_irq_bind->u.pci.intx; - guest_gsi = hvm_pci_intx_gsi(device, intx); - link = hvm_pci_intx_link(device, intx); + switch ( pt_irq_bind->irq_type ) + { + case PT_IRQ_TYPE_PCI: + case PT_IRQ_TYPE_MSI_TRANSLATE: + break; + case PT_IRQ_TYPE_MSI: + return 0; + default: + return -EOPNOTSUPP; + } if ( iommu_verbose ) dprintk(XENLOG_G_INFO, - "d%d: unbind: m_gsi=%u g_gsi=%u device=%u intx=%u\n", - d->domain_id, machine_gsi, guest_gsi, device, intx); + "d%d: unbind: m_gsi=%u g_gsi=%u dev=%02x:%02x.%u intx=%u\n", + d->domain_id, machine_gsi, guest_gsi, bus, + PCI_SLOT(device), PCI_FUNC(device), intx); spin_lock(&d->event_lock); @@ -314,18 +331,28 @@ int pt_irq_destroy_bind( return -EINVAL; } - hvm_irq_dpci->link_cnt[link]--; - list_for_each_entry ( girq, &hvm_irq_dpci->girq[guest_gsi], list ) { - if ( girq->machine_gsi == machine_gsi ) + if ( girq->bus == bus && + girq->device == device && + girq->intx == intx && + girq->machine_gsi == machine_gsi ) { - list_del(&girq->list); - xfree(girq); - break; + list_del(&girq->list); + xfree(girq); + girq = NULL; + break; } } + if ( girq ) + { + spin_unlock(&d->event_lock); + return -EINVAL; + } + + hvm_irq_dpci->link_cnt[link]--; + pirq = pirq_info(d, machine_gsi); pirq_dpci = pirq_dpci(pirq); @@ -334,10 +361,9 @@ int pt_irq_destroy_bind( { list_for_each_entry_safe ( digl, tmp, &pirq_dpci->digl_list, list ) { - if ( digl->device == device && - digl->intx == intx && - digl->link == link && - digl->gsi == guest_gsi ) + if ( digl->bus == bus && + digl->device == device && + digl->intx == intx ) { list_del(&digl->list); xfree(digl); @@ -359,8 +385,9 @@ int pt_irq_destroy_bind( if ( iommu_verbose ) dprintk(XENLOG_G_INFO, - "d%d unmap: m_irq=%u device=%u intx=%u\n", - d->domain_id, machine_gsi, device, intx); + "d%d unmap: m_irq=%u dev=%02x:%02x.%u intx=%u\n", + d->domain_id, machine_gsi, bus, + PCI_SLOT(device), PCI_FUNC(device), intx); return 0; } @@ -481,11 +508,10 @@ static void hvm_pci_msi_assert( static int _hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci, void *arg) { - uint32_t device, intx; - struct dev_intx_gsi_link *digl; - if ( test_and_clear_bool(pirq_dpci->masked) ) { + const struct dev_intx_gsi_link *digl; + if ( pirq_dpci->flags & HVM_IRQ_DPCI_GUEST_MSI ) { hvm_pci_msi_assert(d, pirq_dpci); @@ -496,12 +522,10 @@ static int _hvm_dirq_assist(struct domai { struct pirq *info = dpci_pirq(pirq_dpci); - device = digl->device; - intx = digl->intx; if ( hvm_domain_use_pirq(d, info) ) send_guest_pirq(d, info); else - hvm_pci_intx_assert(d, device, intx); + hvm_pci_intx_assert(d, digl->device, digl->intx); pirq_dpci->pending++; if ( pirq_dpci->flags & HVM_IRQ_DPCI_TRANSLATE ) @@ -537,16 +561,13 @@ static void hvm_dirq_assist(unsigned lon } static void __hvm_dpci_eoi(struct domain *d, - struct hvm_girq_dpci_mapping *girq, - union vioapic_redir_entry *ent) + const struct hvm_girq_dpci_mapping *girq, + const union vioapic_redir_entry *ent) { - uint32_t device, intx; struct pirq *pirq; struct hvm_pirq_dpci *pirq_dpci; - device = girq->device; - intx = girq->intx; - hvm_pci_intx_deassert(d, device, intx); + hvm_pci_intx_deassert(d, girq->device, girq->intx); pirq = pirq_info(d, girq->machine_gsi); pirq_dpci = pirq_dpci(pirq); @@ -556,8 +577,8 @@ static void __hvm_dpci_eoi(struct domain * since interrupt is still not EOIed */ if ( --pirq_dpci->pending || - ( ent && ent->fields.mask ) || - ! pt_irq_need_timer(pirq_dpci->flags) ) + (ent && ent->fields.mask) || + !pt_irq_need_timer(pirq_dpci->flags) ) return; stop_timer(&pirq_dpci->timer); @@ -565,10 +586,10 @@ static void __hvm_dpci_eoi(struct domain } void hvm_dpci_eoi(struct domain *d, unsigned int guest_gsi, - union vioapic_redir_entry *ent) + const union vioapic_redir_entry *ent) { - struct hvm_irq_dpci *hvm_irq_dpci; - struct hvm_girq_dpci_mapping *girq; + const struct hvm_irq_dpci *hvm_irq_dpci; + const struct hvm_girq_dpci_mapping *girq; if ( !iommu_enabled ) return; --- a/xen/drivers/passthrough/vtd/x86/vtd.c +++ b/xen/drivers/passthrough/vtd/x86/vtd.c @@ -69,11 +69,13 @@ static int _hvm_dpci_isairq_eoi(struct d { struct hvm_irq *hvm_irq = &d->arch.hvm_domain.irq; unsigned int isairq = (long)arg; - struct dev_intx_gsi_link *digl, *tmp; + const struct dev_intx_gsi_link *digl; - list_for_each_entry_safe ( digl, tmp, &pirq_dpci->digl_list, list ) + list_for_each_entry ( digl, &pirq_dpci->digl_list, list ) { - if ( hvm_irq->pci_link.route[digl->link] == isairq ) + unsigned int link = hvm_pci_intx_link(digl->device, digl->intx); + + if ( hvm_irq->pci_link.route[link] == isairq ) { hvm_pci_intx_deassert(d, digl->device, digl->intx); if ( --pirq_dpci->pending == 0 ) --- a/xen/include/asm-x86/hvm/io.h +++ b/xen/include/asm-x86/hvm/io.h @@ -123,7 +123,7 @@ int handle_pio(uint16_t port, unsigned i void hvm_interrupt_post(struct vcpu *v, int vector, int type); void hvm_io_assist(ioreq_t *p); void hvm_dpci_eoi(struct domain *d, unsigned int guest_irq, - union vioapic_redir_entry *ent); + const union vioapic_redir_entry *ent); void msix_write_completion(struct vcpu *); struct hvm_hw_stdvga { --- a/xen/include/xen/hvm/irq.h +++ b/xen/include/xen/hvm/irq.h @@ -30,10 +30,9 @@ struct dev_intx_gsi_link { struct list_head list; + uint8_t bus; uint8_t device; uint8_t intx; - uint8_t gsi; - uint8_t link; }; #define _HVM_IRQ_DPCI_MACH_PCI_SHIFT 0 @@ -69,6 +68,7 @@ struct hvm_gmsi_info { struct hvm_girq_dpci_mapping { struct list_head list; + uint8_t bus; uint8_t device; uint8_t intx; uint8_t machine_gsi; [-- Attachment #2: pt-irq-binding.patch --] [-- Type: text/plain, Size: 18465 bytes --] pt-irq fixes and improvements Tools side: - don't silently ignore unrecognized PT_IRQ_TYPE_* values - respect that the interface type contains a union, making the code at once no longer depend on the hypervisor ignoring the bus field of the PCI portion of the interface structure) Hypervisor side: - don't ignore the PCI bus number passed in - don't store values (gsi, link) calculated from other stored values - avoid calling xfree() with a spin lock held where easily possible - have pt_irq_destroy_bind() respect the passed in type - scope reduction and constification of various variables - use switch instead of if/else-if chains - formatting Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> --- v2: A few extra coding style corrections according to Andrew's suggestions. --- a/tools/libxc/xc_domain.c +++ b/tools/libxc/xc_domain.c @@ -1702,16 +1702,21 @@ int xc_domain_bind_pt_irq( bind->hvm_domid = domid; bind->irq_type = irq_type; bind->machine_irq = machine_irq; - if ( irq_type == PT_IRQ_TYPE_PCI || - irq_type == PT_IRQ_TYPE_MSI_TRANSLATE ) + switch ( irq_type ) { + case PT_IRQ_TYPE_PCI: + case PT_IRQ_TYPE_MSI_TRANSLATE: bind->u.pci.bus = bus; - bind->u.pci.device = device; + bind->u.pci.device = device; bind->u.pci.intx = intx; - } - else if ( irq_type == PT_IRQ_TYPE_ISA ) + case PT_IRQ_TYPE_ISA: bind->u.isa.isa_irq = isa_irq; - + break; + default: + errno = EINVAL; + return -1; + } + rc = do_domctl(xch, &domctl); return rc; } @@ -1737,11 +1742,22 @@ int xc_domain_unbind_pt_irq( bind->hvm_domid = domid; bind->irq_type = irq_type; bind->machine_irq = machine_irq; - bind->u.pci.bus = bus; - bind->u.pci.device = device; - bind->u.pci.intx = intx; - bind->u.isa.isa_irq = isa_irq; - + switch ( irq_type ) + { + case PT_IRQ_TYPE_PCI: + case PT_IRQ_TYPE_MSI_TRANSLATE: + bind->u.pci.bus = bus; + bind->u.pci.device = device; + bind->u.pci.intx = intx; + break; + case PT_IRQ_TYPE_ISA: + bind->u.isa.isa_irq = isa_irq; + break; + default: + errno = EINVAL; + return -1; + } + rc = do_domctl(xch, &domctl); return rc; } --- a/xen/arch/x86/physdev.c +++ b/xen/arch/x86/physdev.c @@ -37,9 +37,8 @@ static int physdev_hvm_map_pirq( switch ( type ) { case MAP_PIRQ_TYPE_GSI: { - struct hvm_irq_dpci *hvm_irq_dpci; - struct hvm_girq_dpci_mapping *girq; - uint32_t machine_gsi = 0; + const struct hvm_irq_dpci *hvm_irq_dpci; + unsigned int machine_gsi = 0; if ( *index < 0 || *index >= NR_HVM_IRQS ) { @@ -52,6 +51,8 @@ static int physdev_hvm_map_pirq( hvm_irq_dpci = domain_get_irq_dpci(d); if ( hvm_irq_dpci ) { + const struct hvm_girq_dpci_mapping *girq; + BUILD_BUG_ON(ARRAY_SIZE(hvm_irq_dpci->girq) < NR_HVM_IRQS); list_for_each_entry ( girq, &hvm_irq_dpci->girq[*index], --- a/xen/drivers/passthrough/io.c +++ b/xen/drivers/passthrough/io.c @@ -51,11 +51,8 @@ static int pt_irq_guest_eoi(struct domai static void pt_irq_time_out(void *data) { struct hvm_pirq_dpci *irq_map = data; - unsigned int guest_gsi; - struct hvm_irq_dpci *dpci = NULL; - struct dev_intx_gsi_link *digl; - struct hvm_girq_dpci_mapping *girq; - uint32_t device, intx; + const struct hvm_irq_dpci *dpci; + const struct dev_intx_gsi_link *digl; spin_lock(&irq_map->dom->event_lock); @@ -63,16 +60,16 @@ static void pt_irq_time_out(void *data) ASSERT(dpci); list_for_each_entry ( digl, &irq_map->digl_list, list ) { - guest_gsi = digl->gsi; + unsigned int guest_gsi = hvm_pci_intx_gsi(digl->device, digl->intx); + const struct hvm_girq_dpci_mapping *girq; + list_for_each_entry ( girq, &dpci->girq[guest_gsi], list ) { struct pirq *pirq = pirq_info(irq_map->dom, girq->machine_gsi); pirq_dpci(pirq)->flags |= HVM_IRQ_DPCI_EOI_LATCH; } - device = digl->device; - intx = digl->intx; - hvm_pci_intx_deassert(irq_map->dom, device, intx); + hvm_pci_intx_deassert(irq_map->dom, digl->device, digl->intx); } pt_pirq_iterate(irq_map->dom, pt_irq_guest_eoi, NULL); @@ -96,13 +93,9 @@ void free_hvm_irq_dpci(struct hvm_irq_dp int pt_irq_create_bind( struct domain *d, xen_domctl_bind_pt_irq_t *pt_irq_bind) { - struct hvm_irq_dpci *hvm_irq_dpci = NULL; + struct hvm_irq_dpci *hvm_irq_dpci; struct hvm_pirq_dpci *pirq_dpci; struct pirq *info; - uint32_t guest_gsi; - uint32_t device, intx, link; - struct dev_intx_gsi_link *digl; - struct hvm_girq_dpci_mapping *girq; int rc, pirq = pt_irq_bind->machine_irq; if ( pirq < 0 || pirq >= d->nr_pirqs ) @@ -113,6 +106,8 @@ int pt_irq_create_bind( hvm_irq_dpci = domain_get_irq_dpci(d); if ( hvm_irq_dpci == NULL ) { + unsigned int i; + hvm_irq_dpci = xzalloc(struct hvm_irq_dpci); if ( hvm_irq_dpci == NULL ) { @@ -122,7 +117,7 @@ int pt_irq_create_bind( softirq_tasklet_init( &hvm_irq_dpci->dirq_tasklet, hvm_dirq_assist, (unsigned long)d); - for ( int i = 0; i < NR_HVM_IRQS; i++ ) + for ( i = 0; i < NR_HVM_IRQS; i++ ) INIT_LIST_HEAD(&hvm_irq_dpci->girq[i]); d->arch.hvm_domain.irq.dpci = hvm_irq_dpci; @@ -136,7 +131,9 @@ int pt_irq_create_bind( } pirq_dpci = pirq_dpci(info); - if ( pt_irq_bind->irq_type == PT_IRQ_TYPE_MSI ) + switch ( pt_irq_bind->irq_type ) + { + case PT_IRQ_TYPE_MSI: { uint8_t dest, dest_mode; int dest_vcpu_id; @@ -169,15 +166,16 @@ int pt_irq_create_bind( { uint32_t mask = HVM_IRQ_DPCI_MACH_MSI | HVM_IRQ_DPCI_GUEST_MSI; - if ( (pirq_dpci->flags & mask) != mask) + if ( (pirq_dpci->flags & mask) != mask ) { - spin_unlock(&d->event_lock); - return -EBUSY; + spin_unlock(&d->event_lock); + return -EBUSY; } - /* if pirq is already mapped as vmsi, update the guest data/addr */ + /* 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 != pt_irq_bind->u.msi.gflags ) + { /* Directly clear pending EOIs before enabling new MSI info. */ pirq_guest_eoi(info); @@ -185,7 +183,7 @@ int pt_irq_create_bind( pirq_dpci->gmsi.gflags = pt_irq_bind->u.msi.gflags; } } - /* Caculate dest_vcpu_id for MSI-type pirq migration */ + /* Calculate dest_vcpu_id for MSI-type pirq migration. */ dest = pirq_dpci->gmsi.gflags & VMSI_DEST_ID_MASK; dest_mode = !!(pirq_dpci->gmsi.gflags & VMSI_DM_MASK); dest_vcpu_id = hvm_girq_dest_2_vcpu_id(d, dest, dest_mode); @@ -193,36 +191,37 @@ int pt_irq_create_bind( spin_unlock(&d->event_lock); if ( dest_vcpu_id >= 0 ) hvm_migrate_pirqs(d->vcpu[dest_vcpu_id]); + break; } - else + + case PT_IRQ_TYPE_PCI: + case PT_IRQ_TYPE_MSI_TRANSLATE: { - device = pt_irq_bind->u.pci.device; - intx = pt_irq_bind->u.pci.intx; - guest_gsi = hvm_pci_intx_gsi(device, intx); - link = hvm_pci_intx_link(device, intx); - hvm_irq_dpci->link_cnt[link]++; + unsigned int bus = pt_irq_bind->u.pci.bus; + unsigned int device = pt_irq_bind->u.pci.device; + unsigned int intx = pt_irq_bind->u.pci.intx; + unsigned int guest_gsi = hvm_pci_intx_gsi(device, intx); + unsigned int link = hvm_pci_intx_link(device, intx); + struct dev_intx_gsi_link *digl = xmalloc(struct dev_intx_gsi_link); + struct hvm_girq_dpci_mapping *girq = + xmalloc(struct hvm_girq_dpci_mapping); - digl = xmalloc(struct dev_intx_gsi_link); - if ( !digl ) + if ( !digl || !girq ) { spin_unlock(&d->event_lock); - return -ENOMEM; - } - - girq = xmalloc(struct hvm_girq_dpci_mapping); - if ( !girq ) - { + xfree(girq); xfree(digl); - spin_unlock(&d->event_lock); return -ENOMEM; } + hvm_irq_dpci->link_cnt[link]++; + + digl->bus = bus; digl->device = device; digl->intx = intx; - digl->gsi = guest_gsi; - digl->link = link; list_add_tail(&digl->list, &pirq_dpci->digl_list); + girq->bus = bus; girq->device = device; girq->intx = intx; girq->machine_gsi = pirq; @@ -261,12 +260,12 @@ int pt_irq_create_bind( kill_timer(&pirq_dpci->timer); pirq_dpci->dom = NULL; list_del(&girq->list); - xfree(girq); list_del(&digl->list); hvm_irq_dpci->link_cnt[link]--; pirq_dpci->flags = 0; pirq_cleanup_check(info, d); spin_unlock(&d->event_lock); + xfree(girq); xfree(digl); return rc; } @@ -276,33 +275,51 @@ int pt_irq_create_bind( if ( iommu_verbose ) dprintk(XENLOG_G_INFO, - "d%d: bind: m_gsi=%u g_gsi=%u device=%u intx=%u\n", - d->domain_id, pirq, guest_gsi, device, intx); + "d%d: bind: m_gsi=%u g_gsi=%u dev=%02x.%02x.%u intx=%u\n", + d->domain_id, pirq, guest_gsi, bus, + PCI_SLOT(device), PCI_FUNC(device), intx); + break; } + + default: + spin_unlock(&d->event_lock); + return -EOPNOTSUPP; + } + return 0; } int pt_irq_destroy_bind( struct domain *d, xen_domctl_bind_pt_irq_t *pt_irq_bind) { - struct hvm_irq_dpci *hvm_irq_dpci = NULL; + struct hvm_irq_dpci *hvm_irq_dpci; struct hvm_pirq_dpci *pirq_dpci; - uint32_t machine_gsi, guest_gsi; - uint32_t device, intx, link; + unsigned int machine_gsi = pt_irq_bind->machine_irq; + unsigned int bus = pt_irq_bind->u.pci.bus; + unsigned int device = pt_irq_bind->u.pci.device; + unsigned int intx = pt_irq_bind->u.pci.intx; + unsigned int guest_gsi = hvm_pci_intx_gsi(device, intx); + unsigned int link = hvm_pci_intx_link(device, intx); struct dev_intx_gsi_link *digl, *tmp; struct hvm_girq_dpci_mapping *girq; struct pirq *pirq; - machine_gsi = pt_irq_bind->machine_irq; - device = pt_irq_bind->u.pci.device; - intx = pt_irq_bind->u.pci.intx; - guest_gsi = hvm_pci_intx_gsi(device, intx); - link = hvm_pci_intx_link(device, intx); + switch ( pt_irq_bind->irq_type ) + { + case PT_IRQ_TYPE_PCI: + case PT_IRQ_TYPE_MSI_TRANSLATE: + break; + case PT_IRQ_TYPE_MSI: + return 0; + default: + return -EOPNOTSUPP; + } if ( iommu_verbose ) dprintk(XENLOG_G_INFO, - "d%d: unbind: m_gsi=%u g_gsi=%u device=%u intx=%u\n", - d->domain_id, machine_gsi, guest_gsi, device, intx); + "d%d: unbind: m_gsi=%u g_gsi=%u dev=%02x:%02x.%u intx=%u\n", + d->domain_id, machine_gsi, guest_gsi, bus, + PCI_SLOT(device), PCI_FUNC(device), intx); spin_lock(&d->event_lock); @@ -314,18 +331,28 @@ int pt_irq_destroy_bind( return -EINVAL; } - hvm_irq_dpci->link_cnt[link]--; - list_for_each_entry ( girq, &hvm_irq_dpci->girq[guest_gsi], list ) { - if ( girq->machine_gsi == machine_gsi ) + if ( girq->bus == bus && + girq->device == device && + girq->intx == intx && + girq->machine_gsi == machine_gsi ) { - list_del(&girq->list); - xfree(girq); - break; + list_del(&girq->list); + xfree(girq); + girq = NULL; + break; } } + if ( girq ) + { + spin_unlock(&d->event_lock); + return -EINVAL; + } + + hvm_irq_dpci->link_cnt[link]--; + pirq = pirq_info(d, machine_gsi); pirq_dpci = pirq_dpci(pirq); @@ -334,10 +361,9 @@ int pt_irq_destroy_bind( { list_for_each_entry_safe ( digl, tmp, &pirq_dpci->digl_list, list ) { - if ( digl->device == device && - digl->intx == intx && - digl->link == link && - digl->gsi == guest_gsi ) + if ( digl->bus == bus && + digl->device == device && + digl->intx == intx ) { list_del(&digl->list); xfree(digl); @@ -359,8 +385,9 @@ int pt_irq_destroy_bind( if ( iommu_verbose ) dprintk(XENLOG_G_INFO, - "d%d unmap: m_irq=%u device=%u intx=%u\n", - d->domain_id, machine_gsi, device, intx); + "d%d unmap: m_irq=%u dev=%02x:%02x.%u intx=%u\n", + d->domain_id, machine_gsi, bus, + PCI_SLOT(device), PCI_FUNC(device), intx); return 0; } @@ -481,11 +508,10 @@ static void hvm_pci_msi_assert( static int _hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci, void *arg) { - uint32_t device, intx; - struct dev_intx_gsi_link *digl; - if ( test_and_clear_bool(pirq_dpci->masked) ) { + const struct dev_intx_gsi_link *digl; + if ( pirq_dpci->flags & HVM_IRQ_DPCI_GUEST_MSI ) { hvm_pci_msi_assert(d, pirq_dpci); @@ -496,12 +522,10 @@ static int _hvm_dirq_assist(struct domai { struct pirq *info = dpci_pirq(pirq_dpci); - device = digl->device; - intx = digl->intx; if ( hvm_domain_use_pirq(d, info) ) send_guest_pirq(d, info); else - hvm_pci_intx_assert(d, device, intx); + hvm_pci_intx_assert(d, digl->device, digl->intx); pirq_dpci->pending++; if ( pirq_dpci->flags & HVM_IRQ_DPCI_TRANSLATE ) @@ -537,16 +561,13 @@ static void hvm_dirq_assist(unsigned lon } static void __hvm_dpci_eoi(struct domain *d, - struct hvm_girq_dpci_mapping *girq, - union vioapic_redir_entry *ent) + const struct hvm_girq_dpci_mapping *girq, + const union vioapic_redir_entry *ent) { - uint32_t device, intx; struct pirq *pirq; struct hvm_pirq_dpci *pirq_dpci; - device = girq->device; - intx = girq->intx; - hvm_pci_intx_deassert(d, device, intx); + hvm_pci_intx_deassert(d, girq->device, girq->intx); pirq = pirq_info(d, girq->machine_gsi); pirq_dpci = pirq_dpci(pirq); @@ -556,8 +577,8 @@ static void __hvm_dpci_eoi(struct domain * since interrupt is still not EOIed */ if ( --pirq_dpci->pending || - ( ent && ent->fields.mask ) || - ! pt_irq_need_timer(pirq_dpci->flags) ) + (ent && ent->fields.mask) || + !pt_irq_need_timer(pirq_dpci->flags) ) return; stop_timer(&pirq_dpci->timer); @@ -565,10 +586,10 @@ static void __hvm_dpci_eoi(struct domain } void hvm_dpci_eoi(struct domain *d, unsigned int guest_gsi, - union vioapic_redir_entry *ent) + const union vioapic_redir_entry *ent) { - struct hvm_irq_dpci *hvm_irq_dpci; - struct hvm_girq_dpci_mapping *girq; + const struct hvm_irq_dpci *hvm_irq_dpci; + const struct hvm_girq_dpci_mapping *girq; if ( !iommu_enabled ) return; --- a/xen/drivers/passthrough/vtd/x86/vtd.c +++ b/xen/drivers/passthrough/vtd/x86/vtd.c @@ -69,11 +69,13 @@ static int _hvm_dpci_isairq_eoi(struct d { struct hvm_irq *hvm_irq = &d->arch.hvm_domain.irq; unsigned int isairq = (long)arg; - struct dev_intx_gsi_link *digl, *tmp; + const struct dev_intx_gsi_link *digl; - list_for_each_entry_safe ( digl, tmp, &pirq_dpci->digl_list, list ) + list_for_each_entry ( digl, &pirq_dpci->digl_list, list ) { - if ( hvm_irq->pci_link.route[digl->link] == isairq ) + unsigned int link = hvm_pci_intx_link(digl->device, digl->intx); + + if ( hvm_irq->pci_link.route[link] == isairq ) { hvm_pci_intx_deassert(d, digl->device, digl->intx); if ( --pirq_dpci->pending == 0 ) --- a/xen/include/asm-x86/hvm/io.h +++ b/xen/include/asm-x86/hvm/io.h @@ -123,7 +123,7 @@ int handle_pio(uint16_t port, unsigned i void hvm_interrupt_post(struct vcpu *v, int vector, int type); void hvm_io_assist(ioreq_t *p); void hvm_dpci_eoi(struct domain *d, unsigned int guest_irq, - union vioapic_redir_entry *ent); + const union vioapic_redir_entry *ent); void msix_write_completion(struct vcpu *); struct hvm_hw_stdvga { --- a/xen/include/xen/hvm/irq.h +++ b/xen/include/xen/hvm/irq.h @@ -30,10 +30,9 @@ struct dev_intx_gsi_link { struct list_head list; + uint8_t bus; uint8_t device; uint8_t intx; - uint8_t gsi; - uint8_t link; }; #define _HVM_IRQ_DPCI_MACH_PCI_SHIFT 0 @@ -69,6 +68,7 @@ struct hvm_gmsi_info { struct hvm_girq_dpci_mapping { struct list_head list; + uint8_t bus; uint8_t device; uint8_t intx; uint8_t machine_gsi; [-- Attachment #3: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 3/3] pt-irq fixes and improvements 2014-06-04 8:24 ` [PATCH v2 " Jan Beulich @ 2014-06-10 0:32 ` Zhang, Yang Z 2014-06-11 12:32 ` Ping [tools]: " Jan Beulich 2014-06-12 9:02 ` Ian Campbell 2 siblings, 0 replies; 12+ messages in thread From: Zhang, Yang Z @ 2014-06-10 0:32 UTC (permalink / raw) To: Jan Beulich, xen-devel Cc: Tian, Kevin, Keir Fraser, Andrew Cooper, Ian Jackson, Ian Campbell, Stefano Stabellini Jan Beulich wrote on 2014-06-04: > Tools side: > - don't silently ignore unrecognized PT_IRQ_TYPE_* values > - respect that the interface type contains a union, making the code at > once no longer depend on the hypervisor ignoring the bus field of the > PCI portion of the interface structure) Hypervisor side: > - don't ignore the PCI bus number passed in > - don't store values (gsi, link) calculated from other stored values > - avoid calling xfree() with a spin lock held where easily possible > - have pt_irq_destroy_bind() respect the passed in type > - scope reduction and constification of various variables > - use switch instead of if/else-if chains > - formatting > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> Acked-by: Yang Zhang <yang.z.zhang@intel.com> > --- > v2: A few extra coding style corrections according to Andrew's > suggestions. > --- a/tools/libxc/xc_domain.c > +++ b/tools/libxc/xc_domain.c > @@ -1702,16 +1702,21 @@ int xc_domain_bind_pt_irq( > bind->hvm_domid = domid; > bind->irq_type = irq_type; > bind->machine_irq = machine_irq; > - if ( irq_type == PT_IRQ_TYPE_PCI || > - irq_type == PT_IRQ_TYPE_MSI_TRANSLATE ) > + switch ( irq_type ) > { > + case PT_IRQ_TYPE_PCI: > + case PT_IRQ_TYPE_MSI_TRANSLATE: > bind->u.pci.bus = bus; > - bind->u.pci.device = device; > + bind->u.pci.device = device; > bind->u.pci.intx = intx; > - } > - else if ( irq_type == PT_IRQ_TYPE_ISA ) > + case PT_IRQ_TYPE_ISA: > bind->u.isa.isa_irq = isa_irq; > - > + break; > + default: > + errno = EINVAL; > + return -1; > + } > + > rc = do_domctl(xch, &domctl); > return rc; > } > @@ -1737,11 +1742,22 @@ int xc_domain_unbind_pt_irq( > bind->hvm_domid = domid; > bind->irq_type = irq_type; > bind->machine_irq = machine_irq; > - bind->u.pci.bus = bus; > - bind->u.pci.device = device; > - bind->u.pci.intx = intx; > - bind->u.isa.isa_irq = isa_irq; > - > + switch ( irq_type ) > + { > + case PT_IRQ_TYPE_PCI: > + case PT_IRQ_TYPE_MSI_TRANSLATE: > + bind->u.pci.bus = bus; > + bind->u.pci.device = device; > + bind->u.pci.intx = intx; > + break; > + case PT_IRQ_TYPE_ISA: > + bind->u.isa.isa_irq = isa_irq; > + break; > + default: > + errno = EINVAL; > + return -1; > + } > + > rc = do_domctl(xch, &domctl); > return rc; > } > --- a/xen/arch/x86/physdev.c > +++ b/xen/arch/x86/physdev.c > @@ -37,9 +37,8 @@ static int physdev_hvm_map_pirq( > switch ( type ) > { > case MAP_PIRQ_TYPE_GSI: { > - struct hvm_irq_dpci *hvm_irq_dpci; > - struct hvm_girq_dpci_mapping *girq; > - uint32_t machine_gsi = 0; > + const struct hvm_irq_dpci *hvm_irq_dpci; > + unsigned int machine_gsi = 0; > > if ( *index < 0 || *index >= NR_HVM_IRQS ) { @@ -52,6 +51,8 @@ > static int physdev_hvm_map_pirq( hvm_irq_dpci = > domain_get_irq_dpci(d); if ( hvm_irq_dpci ) { > + const struct hvm_girq_dpci_mapping *girq; > + > BUILD_BUG_ON(ARRAY_SIZE(hvm_irq_dpci->girq) < NR_HVM_IRQS); > list_for_each_entry ( girq, > &hvm_irq_dpci->girq[*index], > --- a/xen/drivers/passthrough/io.c > +++ b/xen/drivers/passthrough/io.c > @@ -51,11 +51,8 @@ static int pt_irq_guest_eoi(struct domai static > void pt_irq_time_out(void *data) { > struct hvm_pirq_dpci *irq_map = data; > - unsigned int guest_gsi; > - struct hvm_irq_dpci *dpci = NULL; > - struct dev_intx_gsi_link *digl; > - struct hvm_girq_dpci_mapping *girq; > - uint32_t device, intx; > + const struct hvm_irq_dpci *dpci; > + const struct dev_intx_gsi_link *digl; > > spin_lock(&irq_map->dom->event_lock); > @@ -63,16 +60,16 @@ static void pt_irq_time_out(void *data) > ASSERT(dpci); > list_for_each_entry ( digl, &irq_map->digl_list, list ) > { > - guest_gsi = digl->gsi; > + unsigned int guest_gsi = hvm_pci_intx_gsi(digl->device, digl->intx); > + const struct hvm_girq_dpci_mapping *girq; > + > list_for_each_entry ( girq, &dpci->girq[guest_gsi], list ) > { > struct pirq *pirq = pirq_info(irq_map->dom, > girq->machine_gsi); > > pirq_dpci(pirq)->flags |= HVM_IRQ_DPCI_EOI_LATCH; > } > - device = digl->device; > - intx = digl->intx; > - hvm_pci_intx_deassert(irq_map->dom, device, intx); > + hvm_pci_intx_deassert(irq_map->dom, digl->device, > + digl->intx); > } > > pt_pirq_iterate(irq_map->dom, pt_irq_guest_eoi, NULL); @@ -96,13 > +93,9 @@ void free_hvm_irq_dpci(struct hvm_irq_dp int > +pt_irq_create_bind( > struct domain *d, xen_domctl_bind_pt_irq_t *pt_irq_bind) { > - struct hvm_irq_dpci *hvm_irq_dpci = NULL; > + struct hvm_irq_dpci *hvm_irq_dpci; > struct hvm_pirq_dpci *pirq_dpci; > struct pirq *info; > - uint32_t guest_gsi; > - uint32_t device, intx, link; > - struct dev_intx_gsi_link *digl; > - struct hvm_girq_dpci_mapping *girq; > int rc, pirq = pt_irq_bind->machine_irq; > > if ( pirq < 0 || pirq >= d->nr_pirqs ) @@ -113,6 +106,8 @@ int > pt_irq_create_bind( hvm_irq_dpci = domain_get_irq_dpci(d); if ( > hvm_irq_dpci == NULL ) { > + unsigned int i; > + > hvm_irq_dpci = xzalloc(struct hvm_irq_dpci); if ( hvm_irq_dpci > == NULL ) { @@ -122,7 +117,7 @@ int pt_irq_create_bind( > softirq_tasklet_init( > &hvm_irq_dpci->dirq_tasklet, > hvm_dirq_assist, (unsigned long)d); > - for ( int i = 0; i < NR_HVM_IRQS; i++ ) > + for ( i = 0; i < NR_HVM_IRQS; i++ ) > INIT_LIST_HEAD(&hvm_irq_dpci->girq[i]); > d->arch.hvm_domain.irq.dpci = hvm_irq_dpci; @@ -136,7 +131,9 > @@ int pt_irq_create_bind( > } > pirq_dpci = pirq_dpci(info); > - if ( pt_irq_bind->irq_type == PT_IRQ_TYPE_MSI ) > + switch ( pt_irq_bind->irq_type ) > + { > + case PT_IRQ_TYPE_MSI: > { > uint8_t dest, dest_mode; int dest_vcpu_id; @@ -169,15 +166,16 > @@ int pt_irq_create_bind( { > uint32_t mask = HVM_IRQ_DPCI_MACH_MSI | > HVM_IRQ_DPCI_GUEST_MSI; > > - if ( (pirq_dpci->flags & mask) != mask) > + if ( (pirq_dpci->flags & mask) != mask ) > { > - spin_unlock(&d->event_lock); > - return -EBUSY; > + spin_unlock(&d->event_lock); > + return -EBUSY; > } > - /* if pirq is already mapped as vmsi, update the guest > data/addr */ > + /* 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 != pt_irq_bind->u.msi.gflags ) > + { > /* Directly clear pending EOIs before enabling new MSI > info. */ pirq_guest_eoi(info); @@ -185,7 +183,7 @@ > int pt_irq_create_bind( > pirq_dpci->gmsi.gflags = pt_irq_bind->u.msi.gflags; > } > } > - /* Caculate dest_vcpu_id for MSI-type pirq migration */ > + /* Calculate dest_vcpu_id for MSI-type pirq migration. */ > dest = pirq_dpci->gmsi.gflags & VMSI_DEST_ID_MASK; dest_mode = > !!(pirq_dpci->gmsi.gflags & VMSI_DM_MASK); dest_vcpu_id = > hvm_girq_dest_2_vcpu_id(d, dest, dest_mode); @@ -193,36 +191,37 > @@ int pt_irq_create_bind( spin_unlock(&d->event_lock); if ( > dest_vcpu_id >= 0 ) > hvm_migrate_pirqs(d->vcpu[dest_vcpu_id]); > + break; > } > - else > + > + case PT_IRQ_TYPE_PCI: > + case PT_IRQ_TYPE_MSI_TRANSLATE: > { > - device = pt_irq_bind->u.pci.device; > - intx = pt_irq_bind->u.pci.intx; > - guest_gsi = hvm_pci_intx_gsi(device, intx); > - link = hvm_pci_intx_link(device, intx); > - hvm_irq_dpci->link_cnt[link]++; > + unsigned int bus = pt_irq_bind->u.pci.bus; > + unsigned int device = pt_irq_bind->u.pci.device; > + unsigned int intx = pt_irq_bind->u.pci.intx; > + unsigned int guest_gsi = hvm_pci_intx_gsi(device, intx); > + unsigned int link = hvm_pci_intx_link(device, intx); > + struct dev_intx_gsi_link *digl = xmalloc(struct dev_intx_gsi_link); > + struct hvm_girq_dpci_mapping *girq = > + xmalloc(struct hvm_girq_dpci_mapping); > > - digl = xmalloc(struct dev_intx_gsi_link); > - if ( !digl ) > + if ( !digl || !girq ) > { > spin_unlock(&d->event_lock); > - return -ENOMEM; > - } > - > - girq = xmalloc(struct hvm_girq_dpci_mapping); > - if ( !girq ) > - { > + xfree(girq); > xfree(digl); - spin_unlock(&d->event_lock); > return -ENOMEM; > } > + hvm_irq_dpci->link_cnt[link]++; > + > + digl->bus = bus; > digl->device = device; > digl->intx = intx; > - digl->gsi = guest_gsi; > - digl->link = link; > list_add_tail(&digl->list, &pirq_dpci->digl_list); > + girq->bus = bus; > girq->device = device; > girq->intx = intx; > girq->machine_gsi = pirq; > @@ -261,12 +260,12 @@ int pt_irq_create_bind( > kill_timer(&pirq_dpci->timer); > pirq_dpci->dom = NULL; list_del(&girq->list); - > xfree(girq); list_del(&digl->list); > hvm_irq_dpci->link_cnt[link]--; pirq_dpci->flags = 0; > pirq_cleanup_check(info, d); > spin_unlock(&d->event_lock); + > xfree(girq); xfree(digl); return rc; > } > @@ -276,33 +275,51 @@ int pt_irq_create_bind( > > if ( iommu_verbose ) > dprintk(XENLOG_G_INFO, > - "d%d: bind: m_gsi=%u g_gsi=%u device=%u intx=%u\n", > - d->domain_id, pirq, guest_gsi, device, intx); + > "d%d: bind: m_gsi=%u g_gsi=%u dev=%02x.%02x.%u > intx=%u\n", + d->domain_id, pirq, guest_gsi, bus, + > PCI_SLOT(device), PCI_FUNC(device), intx); + > break; > } > + > + default: > + spin_unlock(&d->event_lock); > + return -EOPNOTSUPP; > + } > + > return 0; > } > > int pt_irq_destroy_bind( > struct domain *d, xen_domctl_bind_pt_irq_t *pt_irq_bind) { > - struct hvm_irq_dpci *hvm_irq_dpci = NULL; > + struct hvm_irq_dpci *hvm_irq_dpci; > struct hvm_pirq_dpci *pirq_dpci; > - uint32_t machine_gsi, guest_gsi; > - uint32_t device, intx, link; > + unsigned int machine_gsi = pt_irq_bind->machine_irq; > + unsigned int bus = pt_irq_bind->u.pci.bus; > + unsigned int device = pt_irq_bind->u.pci.device; > + unsigned int intx = pt_irq_bind->u.pci.intx; > + unsigned int guest_gsi = hvm_pci_intx_gsi(device, intx); > + unsigned int link = hvm_pci_intx_link(device, intx); > struct dev_intx_gsi_link *digl, *tmp; > struct hvm_girq_dpci_mapping *girq; > struct pirq *pirq; > - machine_gsi = pt_irq_bind->machine_irq; > - device = pt_irq_bind->u.pci.device; > - intx = pt_irq_bind->u.pci.intx; > - guest_gsi = hvm_pci_intx_gsi(device, intx); > - link = hvm_pci_intx_link(device, intx); > + switch ( pt_irq_bind->irq_type ) > + { > + case PT_IRQ_TYPE_PCI: > + case PT_IRQ_TYPE_MSI_TRANSLATE: > + break; > + case PT_IRQ_TYPE_MSI: > + return 0; > + default: > + return -EOPNOTSUPP; > + } > > if ( iommu_verbose ) > dprintk(XENLOG_G_INFO, > - "d%d: unbind: m_gsi=%u g_gsi=%u device=%u intx=%u\n", - > d->domain_id, machine_gsi, guest_gsi, device, intx); + > "d%d: unbind: m_gsi=%u g_gsi=%u dev=%02x:%02x.%u > intx=%u\n", > + d->domain_id, machine_gsi, guest_gsi, bus, + > PCI_SLOT(device), PCI_FUNC(device), intx); > > spin_lock(&d->event_lock); > @@ -314,18 +331,28 @@ int pt_irq_destroy_bind( > return -EINVAL; > } > - hvm_irq_dpci->link_cnt[link]--; > - > list_for_each_entry ( girq, &hvm_irq_dpci->girq[guest_gsi], list ) > { > - if ( girq->machine_gsi == machine_gsi ) > + if ( girq->bus == bus && > + girq->device == device && > + girq->intx == intx && > + girq->machine_gsi == machine_gsi ) > { > - list_del(&girq->list); > - xfree(girq); > - break; > + list_del(&girq->list); > + xfree(girq); > + girq = NULL; > + break; > } > } > + if ( girq ) > + { > + spin_unlock(&d->event_lock); > + return -EINVAL; > + } > + > + hvm_irq_dpci->link_cnt[link]--; > + > pirq = pirq_info(d, machine_gsi); > pirq_dpci = pirq_dpci(pirq); > @@ -334,10 +361,9 @@ int pt_irq_destroy_bind( > { > list_for_each_entry_safe ( digl, tmp, &pirq_dpci->digl_list, list ) > { > - if ( digl->device == device && > - digl->intx == intx && > - digl->link == link && > - digl->gsi == guest_gsi ) > + if ( digl->bus == bus && > + digl->device == device && > + digl->intx == intx ) > { > list_del(&digl->list); > xfree(digl); > @@ -359,8 +385,9 @@ int pt_irq_destroy_bind( > > if ( iommu_verbose ) > dprintk(XENLOG_G_INFO, > - "d%d unmap: m_irq=%u device=%u intx=%u\n", > - d->domain_id, machine_gsi, device, intx); > + "d%d unmap: m_irq=%u dev=%02x:%02x.%u intx=%u\n", > + d->domain_id, machine_gsi, bus, > + PCI_SLOT(device), PCI_FUNC(device), intx); > > return 0; > } > @@ -481,11 +508,10 @@ static void hvm_pci_msi_assert( static int > _hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci, > void *arg) { > - uint32_t device, intx; > - struct dev_intx_gsi_link *digl; > - > if ( test_and_clear_bool(pirq_dpci->masked) ) > { > + const struct dev_intx_gsi_link *digl; > + > if ( pirq_dpci->flags & HVM_IRQ_DPCI_GUEST_MSI ) > { > hvm_pci_msi_assert(d, pirq_dpci); @@ -496,12 +522,10 @@ > static int _hvm_dirq_assist(struct domai > { > struct pirq *info = dpci_pirq(pirq_dpci); > - device = digl->device; > - intx = digl->intx; > if ( hvm_domain_use_pirq(d, info) ) > send_guest_pirq(d, info); > else > - hvm_pci_intx_assert(d, device, intx); > + hvm_pci_intx_assert(d, digl->device, digl->intx); > pirq_dpci->pending++; > > if ( pirq_dpci->flags & HVM_IRQ_DPCI_TRANSLATE ) @@ > -537,16 +561,13 @@ static void hvm_dirq_assist(unsigned lon } > > static void __hvm_dpci_eoi(struct domain *d, > - struct hvm_girq_dpci_mapping *girq, > - union vioapic_redir_entry *ent) > + const struct hvm_girq_dpci_mapping *girq, > + const union vioapic_redir_entry *ent) > { > - uint32_t device, intx; > struct pirq *pirq; > struct hvm_pirq_dpci *pirq_dpci; > - device = girq->device; > - intx = girq->intx; > - hvm_pci_intx_deassert(d, device, intx); > + hvm_pci_intx_deassert(d, girq->device, girq->intx); > > pirq = pirq_info(d, girq->machine_gsi); > pirq_dpci = pirq_dpci(pirq); > @@ -556,8 +577,8 @@ static void __hvm_dpci_eoi(struct domain > * since interrupt is still not EOIed > */ > if ( --pirq_dpci->pending || > - ( ent && ent->fields.mask ) || > - ! pt_irq_need_timer(pirq_dpci->flags) ) > + (ent && ent->fields.mask) || > + !pt_irq_need_timer(pirq_dpci->flags) ) > return; > stop_timer(&pirq_dpci->timer); > @@ -565,10 +586,10 @@ static void __hvm_dpci_eoi(struct domain } > > void hvm_dpci_eoi(struct domain *d, unsigned int guest_gsi, > - union vioapic_redir_entry *ent) > + const union vioapic_redir_entry *ent) > { > - struct hvm_irq_dpci *hvm_irq_dpci; > - struct hvm_girq_dpci_mapping *girq; > + const struct hvm_irq_dpci *hvm_irq_dpci; > + const struct hvm_girq_dpci_mapping *girq; > > if ( !iommu_enabled ) > return; > --- a/xen/drivers/passthrough/vtd/x86/vtd.c > +++ b/xen/drivers/passthrough/vtd/x86/vtd.c > @@ -69,11 +69,13 @@ static int _hvm_dpci_isairq_eoi(struct d { > struct hvm_irq *hvm_irq = &d->arch.hvm_domain.irq; > unsigned int isairq = (long)arg; > - struct dev_intx_gsi_link *digl, *tmp; > + const struct dev_intx_gsi_link *digl; > > - list_for_each_entry_safe ( digl, tmp, &pirq_dpci->digl_list, list ) > + list_for_each_entry ( digl, &pirq_dpci->digl_list, list ) > { > - if ( hvm_irq->pci_link.route[digl->link] == isairq ) > + unsigned int link = hvm_pci_intx_link(digl->device, > + digl->intx); > + > + if ( hvm_irq->pci_link.route[link] == isairq ) > { > hvm_pci_intx_deassert(d, digl->device, digl->intx); > if ( --pirq_dpci->pending == 0 ) > --- a/xen/include/asm-x86/hvm/io.h > +++ b/xen/include/asm-x86/hvm/io.h > @@ -123,7 +123,7 @@ int handle_pio(uint16_t port, unsigned i void > hvm_interrupt_post(struct vcpu *v, int vector, int type); void > hvm_io_assist(ioreq_t *p); void hvm_dpci_eoi(struct domain *d, > unsigned int guest_irq, > - union vioapic_redir_entry *ent); > + const union vioapic_redir_entry *ent); > void msix_write_completion(struct vcpu *); > > struct hvm_hw_stdvga { > --- a/xen/include/xen/hvm/irq.h > +++ b/xen/include/xen/hvm/irq.h > @@ -30,10 +30,9 @@ > > struct dev_intx_gsi_link { > struct list_head list; + uint8_t bus; uint8_t device; uint8_t > intx; > - uint8_t gsi; > - uint8_t link; > }; > > #define _HVM_IRQ_DPCI_MACH_PCI_SHIFT 0 > @@ -69,6 +68,7 @@ struct hvm_gmsi_info { > > struct hvm_girq_dpci_mapping { > struct list_head list; + uint8_t bus; uint8_t device; uint8_t > intx; uint8_t machine_gsi; Best regards, Yang ^ permalink raw reply [flat|nested] 12+ messages in thread
* Ping [tools]: [PATCH v2 3/3] pt-irq fixes and improvements 2014-06-04 8:24 ` [PATCH v2 " Jan Beulich 2014-06-10 0:32 ` Zhang, Yang Z @ 2014-06-11 12:32 ` Jan Beulich 2014-06-11 16:54 ` Ian Campbell 2014-06-12 9:02 ` Ian Campbell 2 siblings, 1 reply; 12+ messages in thread From: Jan Beulich @ 2014-06-11 12:32 UTC (permalink / raw) To: Stefano Stabellini, Ian Campbell, Ian Jackson Cc: Yang Z Zhang, Andrew Cooper, Kevin Tian, Keir Fraser, xen-devel >>> On 04.06.14 at 10:24, <JBeulich@suse.com> wrote: > Tools side: > - don't silently ignore unrecognized PT_IRQ_TYPE_* values > - respect that the interface type contains a union, making the code at > once no longer depend on the hypervisor ignoring the bus field of the > PCI portion of the interface structure) This part is still awaiting a tools maintainer's opinion. Thanks, Jan > Hypervisor side: > - don't ignore the PCI bus number passed in > - don't store values (gsi, link) calculated from other stored values > - avoid calling xfree() with a spin lock held where easily possible > - have pt_irq_destroy_bind() respect the passed in type > - scope reduction and constification of various variables > - use switch instead of if/else-if chains > - formatting > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > v2: A few extra coding style corrections according to Andrew's > suggestions. > > --- a/tools/libxc/xc_domain.c > +++ b/tools/libxc/xc_domain.c > @@ -1702,16 +1702,21 @@ int xc_domain_bind_pt_irq( > bind->hvm_domid = domid; > bind->irq_type = irq_type; > bind->machine_irq = machine_irq; > - if ( irq_type == PT_IRQ_TYPE_PCI || > - irq_type == PT_IRQ_TYPE_MSI_TRANSLATE ) > + switch ( irq_type ) > { > + case PT_IRQ_TYPE_PCI: > + case PT_IRQ_TYPE_MSI_TRANSLATE: > bind->u.pci.bus = bus; > - bind->u.pci.device = device; > + bind->u.pci.device = device; > bind->u.pci.intx = intx; > - } > - else if ( irq_type == PT_IRQ_TYPE_ISA ) > + case PT_IRQ_TYPE_ISA: > bind->u.isa.isa_irq = isa_irq; > - > + break; > + default: > + errno = EINVAL; > + return -1; > + } > + > rc = do_domctl(xch, &domctl); > return rc; > } > @@ -1737,11 +1742,22 @@ int xc_domain_unbind_pt_irq( > bind->hvm_domid = domid; > bind->irq_type = irq_type; > bind->machine_irq = machine_irq; > - bind->u.pci.bus = bus; > - bind->u.pci.device = device; > - bind->u.pci.intx = intx; > - bind->u.isa.isa_irq = isa_irq; > - > + switch ( irq_type ) > + { > + case PT_IRQ_TYPE_PCI: > + case PT_IRQ_TYPE_MSI_TRANSLATE: > + bind->u.pci.bus = bus; > + bind->u.pci.device = device; > + bind->u.pci.intx = intx; > + break; > + case PT_IRQ_TYPE_ISA: > + bind->u.isa.isa_irq = isa_irq; > + break; > + default: > + errno = EINVAL; > + return -1; > + } > + > rc = do_domctl(xch, &domctl); > return rc; > } > --- a/xen/arch/x86/physdev.c > +++ b/xen/arch/x86/physdev.c > @@ -37,9 +37,8 @@ static int physdev_hvm_map_pirq( > switch ( type ) > { > case MAP_PIRQ_TYPE_GSI: { > - struct hvm_irq_dpci *hvm_irq_dpci; > - struct hvm_girq_dpci_mapping *girq; > - uint32_t machine_gsi = 0; > + const struct hvm_irq_dpci *hvm_irq_dpci; > + unsigned int machine_gsi = 0; > > if ( *index < 0 || *index >= NR_HVM_IRQS ) > { > @@ -52,6 +51,8 @@ static int physdev_hvm_map_pirq( > hvm_irq_dpci = domain_get_irq_dpci(d); > if ( hvm_irq_dpci ) > { > + const struct hvm_girq_dpci_mapping *girq; > + > BUILD_BUG_ON(ARRAY_SIZE(hvm_irq_dpci->girq) < NR_HVM_IRQS); > list_for_each_entry ( girq, > &hvm_irq_dpci->girq[*index], > --- a/xen/drivers/passthrough/io.c > +++ b/xen/drivers/passthrough/io.c > @@ -51,11 +51,8 @@ static int pt_irq_guest_eoi(struct domai > static void pt_irq_time_out(void *data) > { > struct hvm_pirq_dpci *irq_map = data; > - unsigned int guest_gsi; > - struct hvm_irq_dpci *dpci = NULL; > - struct dev_intx_gsi_link *digl; > - struct hvm_girq_dpci_mapping *girq; > - uint32_t device, intx; > + const struct hvm_irq_dpci *dpci; > + const struct dev_intx_gsi_link *digl; > > spin_lock(&irq_map->dom->event_lock); > > @@ -63,16 +60,16 @@ static void pt_irq_time_out(void *data) > ASSERT(dpci); > list_for_each_entry ( digl, &irq_map->digl_list, list ) > { > - guest_gsi = digl->gsi; > + unsigned int guest_gsi = hvm_pci_intx_gsi(digl->device, digl->intx); > + const struct hvm_girq_dpci_mapping *girq; > + > list_for_each_entry ( girq, &dpci->girq[guest_gsi], list ) > { > struct pirq *pirq = pirq_info(irq_map->dom, girq->machine_gsi); > > pirq_dpci(pirq)->flags |= HVM_IRQ_DPCI_EOI_LATCH; > } > - device = digl->device; > - intx = digl->intx; > - hvm_pci_intx_deassert(irq_map->dom, device, intx); > + hvm_pci_intx_deassert(irq_map->dom, digl->device, digl->intx); > } > > pt_pirq_iterate(irq_map->dom, pt_irq_guest_eoi, NULL); > @@ -96,13 +93,9 @@ void free_hvm_irq_dpci(struct hvm_irq_dp > int pt_irq_create_bind( > struct domain *d, xen_domctl_bind_pt_irq_t *pt_irq_bind) > { > - struct hvm_irq_dpci *hvm_irq_dpci = NULL; > + struct hvm_irq_dpci *hvm_irq_dpci; > struct hvm_pirq_dpci *pirq_dpci; > struct pirq *info; > - uint32_t guest_gsi; > - uint32_t device, intx, link; > - struct dev_intx_gsi_link *digl; > - struct hvm_girq_dpci_mapping *girq; > int rc, pirq = pt_irq_bind->machine_irq; > > if ( pirq < 0 || pirq >= d->nr_pirqs ) > @@ -113,6 +106,8 @@ int pt_irq_create_bind( > hvm_irq_dpci = domain_get_irq_dpci(d); > if ( hvm_irq_dpci == NULL ) > { > + unsigned int i; > + > hvm_irq_dpci = xzalloc(struct hvm_irq_dpci); > if ( hvm_irq_dpci == NULL ) > { > @@ -122,7 +117,7 @@ int pt_irq_create_bind( > softirq_tasklet_init( > &hvm_irq_dpci->dirq_tasklet, > hvm_dirq_assist, (unsigned long)d); > - for ( int i = 0; i < NR_HVM_IRQS; i++ ) > + for ( i = 0; i < NR_HVM_IRQS; i++ ) > INIT_LIST_HEAD(&hvm_irq_dpci->girq[i]); > > d->arch.hvm_domain.irq.dpci = hvm_irq_dpci; > @@ -136,7 +131,9 @@ int pt_irq_create_bind( > } > pirq_dpci = pirq_dpci(info); > > - if ( pt_irq_bind->irq_type == PT_IRQ_TYPE_MSI ) > + switch ( pt_irq_bind->irq_type ) > + { > + case PT_IRQ_TYPE_MSI: > { > uint8_t dest, dest_mode; > int dest_vcpu_id; > @@ -169,15 +166,16 @@ int pt_irq_create_bind( > { > uint32_t mask = HVM_IRQ_DPCI_MACH_MSI | HVM_IRQ_DPCI_GUEST_MSI; > > - if ( (pirq_dpci->flags & mask) != mask) > + if ( (pirq_dpci->flags & mask) != mask ) > { > - spin_unlock(&d->event_lock); > - return -EBUSY; > + spin_unlock(&d->event_lock); > + return -EBUSY; > } > > - /* if pirq is already mapped as vmsi, update the guest data/addr > */ > + /* 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 != pt_irq_bind->u.msi.gflags ) > + { > /* Directly clear pending EOIs before enabling new MSI > info. */ > pirq_guest_eoi(info); > > @@ -185,7 +183,7 @@ int pt_irq_create_bind( > pirq_dpci->gmsi.gflags = pt_irq_bind->u.msi.gflags; > } > } > - /* Caculate dest_vcpu_id for MSI-type pirq migration */ > + /* Calculate dest_vcpu_id for MSI-type pirq migration. */ > dest = pirq_dpci->gmsi.gflags & VMSI_DEST_ID_MASK; > dest_mode = !!(pirq_dpci->gmsi.gflags & VMSI_DM_MASK); > dest_vcpu_id = hvm_girq_dest_2_vcpu_id(d, dest, dest_mode); > @@ -193,36 +191,37 @@ int pt_irq_create_bind( > spin_unlock(&d->event_lock); > if ( dest_vcpu_id >= 0 ) > hvm_migrate_pirqs(d->vcpu[dest_vcpu_id]); > + break; > } > - else > + > + case PT_IRQ_TYPE_PCI: > + case PT_IRQ_TYPE_MSI_TRANSLATE: > { > - device = pt_irq_bind->u.pci.device; > - intx = pt_irq_bind->u.pci.intx; > - guest_gsi = hvm_pci_intx_gsi(device, intx); > - link = hvm_pci_intx_link(device, intx); > - hvm_irq_dpci->link_cnt[link]++; > + unsigned int bus = pt_irq_bind->u.pci.bus; > + unsigned int device = pt_irq_bind->u.pci.device; > + unsigned int intx = pt_irq_bind->u.pci.intx; > + unsigned int guest_gsi = hvm_pci_intx_gsi(device, intx); > + unsigned int link = hvm_pci_intx_link(device, intx); > + struct dev_intx_gsi_link *digl = xmalloc(struct dev_intx_gsi_link); > + struct hvm_girq_dpci_mapping *girq = > + xmalloc(struct hvm_girq_dpci_mapping); > > - digl = xmalloc(struct dev_intx_gsi_link); > - if ( !digl ) > + if ( !digl || !girq ) > { > spin_unlock(&d->event_lock); > - return -ENOMEM; > - } > - > - girq = xmalloc(struct hvm_girq_dpci_mapping); > - if ( !girq ) > - { > + xfree(girq); > xfree(digl); > - spin_unlock(&d->event_lock); > return -ENOMEM; > } > > + hvm_irq_dpci->link_cnt[link]++; > + > + digl->bus = bus; > digl->device = device; > digl->intx = intx; > - digl->gsi = guest_gsi; > - digl->link = link; > list_add_tail(&digl->list, &pirq_dpci->digl_list); > > + girq->bus = bus; > girq->device = device; > girq->intx = intx; > girq->machine_gsi = pirq; > @@ -261,12 +260,12 @@ int pt_irq_create_bind( > kill_timer(&pirq_dpci->timer); > pirq_dpci->dom = NULL; > list_del(&girq->list); > - xfree(girq); > list_del(&digl->list); > hvm_irq_dpci->link_cnt[link]--; > pirq_dpci->flags = 0; > pirq_cleanup_check(info, d); > spin_unlock(&d->event_lock); > + xfree(girq); > xfree(digl); > return rc; > } > @@ -276,33 +275,51 @@ int pt_irq_create_bind( > > if ( iommu_verbose ) > dprintk(XENLOG_G_INFO, > - "d%d: bind: m_gsi=%u g_gsi=%u device=%u intx=%u\n", > - d->domain_id, pirq, guest_gsi, device, intx); > + "d%d: bind: m_gsi=%u g_gsi=%u dev=%02x.%02x.%u > intx=%u\n", > + d->domain_id, pirq, guest_gsi, bus, > + PCI_SLOT(device), PCI_FUNC(device), intx); > + break; > } > + > + default: > + spin_unlock(&d->event_lock); > + return -EOPNOTSUPP; > + } > + > return 0; > } > > int pt_irq_destroy_bind( > struct domain *d, xen_domctl_bind_pt_irq_t *pt_irq_bind) > { > - struct hvm_irq_dpci *hvm_irq_dpci = NULL; > + struct hvm_irq_dpci *hvm_irq_dpci; > struct hvm_pirq_dpci *pirq_dpci; > - uint32_t machine_gsi, guest_gsi; > - uint32_t device, intx, link; > + unsigned int machine_gsi = pt_irq_bind->machine_irq; > + unsigned int bus = pt_irq_bind->u.pci.bus; > + unsigned int device = pt_irq_bind->u.pci.device; > + unsigned int intx = pt_irq_bind->u.pci.intx; > + unsigned int guest_gsi = hvm_pci_intx_gsi(device, intx); > + unsigned int link = hvm_pci_intx_link(device, intx); > struct dev_intx_gsi_link *digl, *tmp; > struct hvm_girq_dpci_mapping *girq; > struct pirq *pirq; > > - machine_gsi = pt_irq_bind->machine_irq; > - device = pt_irq_bind->u.pci.device; > - intx = pt_irq_bind->u.pci.intx; > - guest_gsi = hvm_pci_intx_gsi(device, intx); > - link = hvm_pci_intx_link(device, intx); > + switch ( pt_irq_bind->irq_type ) > + { > + case PT_IRQ_TYPE_PCI: > + case PT_IRQ_TYPE_MSI_TRANSLATE: > + break; > + case PT_IRQ_TYPE_MSI: > + return 0; > + default: > + return -EOPNOTSUPP; > + } > > if ( iommu_verbose ) > dprintk(XENLOG_G_INFO, > - "d%d: unbind: m_gsi=%u g_gsi=%u device=%u intx=%u\n", > - d->domain_id, machine_gsi, guest_gsi, device, intx); > + "d%d: unbind: m_gsi=%u g_gsi=%u dev=%02x:%02x.%u > intx=%u\n", > + d->domain_id, machine_gsi, guest_gsi, bus, > + PCI_SLOT(device), PCI_FUNC(device), intx); > > spin_lock(&d->event_lock); > > @@ -314,18 +331,28 @@ int pt_irq_destroy_bind( > return -EINVAL; > } > > - hvm_irq_dpci->link_cnt[link]--; > - > list_for_each_entry ( girq, &hvm_irq_dpci->girq[guest_gsi], list ) > { > - if ( girq->machine_gsi == machine_gsi ) > + if ( girq->bus == bus && > + girq->device == device && > + girq->intx == intx && > + girq->machine_gsi == machine_gsi ) > { > - list_del(&girq->list); > - xfree(girq); > - break; > + list_del(&girq->list); > + xfree(girq); > + girq = NULL; > + break; > } > } > > + if ( girq ) > + { > + spin_unlock(&d->event_lock); > + return -EINVAL; > + } > + > + hvm_irq_dpci->link_cnt[link]--; > + > pirq = pirq_info(d, machine_gsi); > pirq_dpci = pirq_dpci(pirq); > > @@ -334,10 +361,9 @@ int pt_irq_destroy_bind( > { > list_for_each_entry_safe ( digl, tmp, &pirq_dpci->digl_list, list ) > { > - if ( digl->device == device && > - digl->intx == intx && > - digl->link == link && > - digl->gsi == guest_gsi ) > + if ( digl->bus == bus && > + digl->device == device && > + digl->intx == intx ) > { > list_del(&digl->list); > xfree(digl); > @@ -359,8 +385,9 @@ int pt_irq_destroy_bind( > > if ( iommu_verbose ) > dprintk(XENLOG_G_INFO, > - "d%d unmap: m_irq=%u device=%u intx=%u\n", > - d->domain_id, machine_gsi, device, intx); > + "d%d unmap: m_irq=%u dev=%02x:%02x.%u intx=%u\n", > + d->domain_id, machine_gsi, bus, > + PCI_SLOT(device), PCI_FUNC(device), intx); > > return 0; > } > @@ -481,11 +508,10 @@ static void hvm_pci_msi_assert( > static int _hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci > *pirq_dpci, > void *arg) > { > - uint32_t device, intx; > - struct dev_intx_gsi_link *digl; > - > if ( test_and_clear_bool(pirq_dpci->masked) ) > { > + const struct dev_intx_gsi_link *digl; > + > if ( pirq_dpci->flags & HVM_IRQ_DPCI_GUEST_MSI ) > { > hvm_pci_msi_assert(d, pirq_dpci); > @@ -496,12 +522,10 @@ static int _hvm_dirq_assist(struct domai > { > struct pirq *info = dpci_pirq(pirq_dpci); > > - device = digl->device; > - intx = digl->intx; > if ( hvm_domain_use_pirq(d, info) ) > send_guest_pirq(d, info); > else > - hvm_pci_intx_assert(d, device, intx); > + hvm_pci_intx_assert(d, digl->device, digl->intx); > pirq_dpci->pending++; > > if ( pirq_dpci->flags & HVM_IRQ_DPCI_TRANSLATE ) > @@ -537,16 +561,13 @@ static void hvm_dirq_assist(unsigned lon > } > > static void __hvm_dpci_eoi(struct domain *d, > - struct hvm_girq_dpci_mapping *girq, > - union vioapic_redir_entry *ent) > + const struct hvm_girq_dpci_mapping *girq, > + const union vioapic_redir_entry *ent) > { > - uint32_t device, intx; > struct pirq *pirq; > struct hvm_pirq_dpci *pirq_dpci; > > - device = girq->device; > - intx = girq->intx; > - hvm_pci_intx_deassert(d, device, intx); > + hvm_pci_intx_deassert(d, girq->device, girq->intx); > > pirq = pirq_info(d, girq->machine_gsi); > pirq_dpci = pirq_dpci(pirq); > @@ -556,8 +577,8 @@ static void __hvm_dpci_eoi(struct domain > * since interrupt is still not EOIed > */ > if ( --pirq_dpci->pending || > - ( ent && ent->fields.mask ) || > - ! pt_irq_need_timer(pirq_dpci->flags) ) > + (ent && ent->fields.mask) || > + !pt_irq_need_timer(pirq_dpci->flags) ) > return; > > stop_timer(&pirq_dpci->timer); > @@ -565,10 +586,10 @@ static void __hvm_dpci_eoi(struct domain > } > > void hvm_dpci_eoi(struct domain *d, unsigned int guest_gsi, > - union vioapic_redir_entry *ent) > + const union vioapic_redir_entry *ent) > { > - struct hvm_irq_dpci *hvm_irq_dpci; > - struct hvm_girq_dpci_mapping *girq; > + const struct hvm_irq_dpci *hvm_irq_dpci; > + const struct hvm_girq_dpci_mapping *girq; > > if ( !iommu_enabled ) > return; > --- a/xen/drivers/passthrough/vtd/x86/vtd.c > +++ b/xen/drivers/passthrough/vtd/x86/vtd.c > @@ -69,11 +69,13 @@ static int _hvm_dpci_isairq_eoi(struct d > { > struct hvm_irq *hvm_irq = &d->arch.hvm_domain.irq; > unsigned int isairq = (long)arg; > - struct dev_intx_gsi_link *digl, *tmp; > + const struct dev_intx_gsi_link *digl; > > - list_for_each_entry_safe ( digl, tmp, &pirq_dpci->digl_list, list ) > + list_for_each_entry ( digl, &pirq_dpci->digl_list, list ) > { > - if ( hvm_irq->pci_link.route[digl->link] == isairq ) > + unsigned int link = hvm_pci_intx_link(digl->device, digl->intx); > + > + if ( hvm_irq->pci_link.route[link] == isairq ) > { > hvm_pci_intx_deassert(d, digl->device, digl->intx); > if ( --pirq_dpci->pending == 0 ) > --- a/xen/include/asm-x86/hvm/io.h > +++ b/xen/include/asm-x86/hvm/io.h > @@ -123,7 +123,7 @@ int handle_pio(uint16_t port, unsigned i > void hvm_interrupt_post(struct vcpu *v, int vector, int type); > void hvm_io_assist(ioreq_t *p); > void hvm_dpci_eoi(struct domain *d, unsigned int guest_irq, > - union vioapic_redir_entry *ent); > + const union vioapic_redir_entry *ent); > void msix_write_completion(struct vcpu *); > > struct hvm_hw_stdvga { > --- a/xen/include/xen/hvm/irq.h > +++ b/xen/include/xen/hvm/irq.h > @@ -30,10 +30,9 @@ > > struct dev_intx_gsi_link { > struct list_head list; > + uint8_t bus; > uint8_t device; > uint8_t intx; > - uint8_t gsi; > - uint8_t link; > }; > > #define _HVM_IRQ_DPCI_MACH_PCI_SHIFT 0 > @@ -69,6 +68,7 @@ struct hvm_gmsi_info { > > struct hvm_girq_dpci_mapping { > struct list_head list; > + uint8_t bus; > uint8_t device; > uint8_t intx; > uint8_t machine_gsi; ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Ping [tools]: [PATCH v2 3/3] pt-irq fixes and improvements 2014-06-11 12:32 ` Ping [tools]: " Jan Beulich @ 2014-06-11 16:54 ` Ian Campbell 0 siblings, 0 replies; 12+ messages in thread From: Ian Campbell @ 2014-06-11 16:54 UTC (permalink / raw) To: Jan Beulich Cc: Kevin Tian, Keir Fraser, Andrew Cooper, Ian Jackson, Stefano Stabellini, Yang Z Zhang, xen-devel On Wed, 2014-06-11 at 13:32 +0100, Jan Beulich wrote: > >>> On 04.06.14 at 10:24, <JBeulich@suse.com> wrote: > > Tools side: > > - don't silently ignore unrecognized PT_IRQ_TYPE_* values > > - respect that the interface type contains a union, making the code at > > once no longer depend on the hypervisor ignoring the bus field of the > > PCI portion of the interface structure) > > This part is still awaiting a tools maintainer's opinion. Sorry, I missed that there was a tools bit here. I've moved it into my queue folder and will look at it ASAP. Ian. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 3/3] pt-irq fixes and improvements 2014-06-04 8:24 ` [PATCH v2 " Jan Beulich 2014-06-10 0:32 ` Zhang, Yang Z 2014-06-11 12:32 ` Ping [tools]: " Jan Beulich @ 2014-06-12 9:02 ` Ian Campbell 2 siblings, 0 replies; 12+ messages in thread From: Ian Campbell @ 2014-06-12 9:02 UTC (permalink / raw) To: Jan Beulich Cc: Kevin Tian, Keir Fraser, Andrew Cooper, Ian Jackson, Stefano Stabellini, Yang Z Zhang, xen-devel On Wed, 2014-06-04 at 09:24 +0100, Jan Beulich wrote: > Tools side: > - don't silently ignore unrecognized PT_IRQ_TYPE_* values > - respect that the interface type contains a union, making the code at > once no longer depend on the hypervisor ignoring the bus field of the > PCI portion of the interface structure) > > Hypervisor side: > - don't ignore the PCI bus number passed in > - don't store values (gsi, link) calculated from other stored values > - avoid calling xfree() with a spin lock held where easily possible > - have pt_irq_destroy_bind() respect the passed in type > - scope reduction and constification of various variables > - use switch instead of if/else-if chains > - formatting > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> Toosl: Acked-by: Ian Campbell <ian.campbell@citrix.com> ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2014-06-12 9:02 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-06-03 13:52 [PATCH 0/3] XSA-96 follow-ups Jan Beulich 2014-06-03 13:58 ` [PATCH 1/3] x86/HVM: properly propagate errors from HVMOP_inject_msi Jan Beulich 2014-06-03 14:05 ` Andrew Cooper 2014-06-03 13:59 ` [PATCH 2/3] x86/HVM: make vmsi_deliver() return proper error values Jan Beulich 2014-06-03 14:13 ` Andrew Cooper 2014-06-03 14:00 ` [PATCH 3/3] pt-irq fixes and improvements Jan Beulich 2014-06-03 14:50 ` Andrew Cooper 2014-06-04 8:24 ` [PATCH v2 " Jan Beulich 2014-06-10 0:32 ` Zhang, Yang Z 2014-06-11 12:32 ` Ping [tools]: " Jan Beulich 2014-06-11 16:54 ` Ian Campbell 2014-06-12 9:02 ` Ian Campbell
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.