x86/MSI-X: cleanup - __pci_enable_msix() now checks that an MSI-X capability was actually found - pass "pos" to msix_capability_init() as both callers already know it (and hence there's no need to re-obtain it) - call __pci_disable_msi{,x}() directly instead of via pci_disable_msi() from __pci_enable_msi{x,}() state validation paths - use msix_control_reg() instead of open coding it - log message adjustments - coding style corrections Signed-off-by: Jan Beulich Reviewed-by: Andrew Cooper --- a/xen/arch/x86/msi.c +++ b/xen/arch/x86/msi.c @@ -35,6 +35,8 @@ static s8 __read_mostly use_msi = -1; boolean_param("msi", use_msi); +static void __pci_disable_msix(struct msi_desc *); + /* bitmap indicate which fixed map is free */ static DEFINE_SPINLOCK(msix_fixmap_lock); static DECLARE_BITMAP(msix_fixmap_pages, FIX_MSIX_MAX_PAGES); @@ -167,12 +169,14 @@ void msi_compose_msg(unsigned vector, co unsigned dest; memset(msg, 0, sizeof(*msg)); - if ( !cpumask_intersects(cpu_mask, &cpu_online_map) ) { + if ( !cpumask_intersects(cpu_mask, &cpu_online_map) ) + { dprintk(XENLOG_ERR,"%s, compose msi message error!!\n", __func__); return; } - if ( vector ) { + if ( vector ) + { cpumask_t *mask = this_cpu(scratch_mask); cpumask_and(mask, cpu_mask, &cpu_online_map); @@ -233,8 +237,7 @@ static bool_t read_msi_msg(struct msi_de } case PCI_CAP_ID_MSIX: { - void __iomem *base; - base = entry->mask_base; + void __iomem *base = entry->mask_base; if ( unlikely(!msix_memory_decoded(entry->dev, entry->msi_attrib.pos)) ) @@ -300,8 +303,7 @@ static int write_msi_msg(struct msi_desc } case PCI_CAP_ID_MSIX: { - void __iomem *base; - base = entry->mask_base; + void __iomem *base = entry->mask_base; if ( unlikely(!msix_memory_decoded(entry->dev, entry->msi_attrib.pos)) ) @@ -327,7 +329,7 @@ void set_msi_affinity(struct irq_desc *d struct msi_desc *msi_desc = desc->msi_desc; dest = set_desc_affinity(desc, mask); - if (dest == BAD_APICID || !msi_desc) + if ( dest == BAD_APICID || !msi_desc ) return; ASSERT(spin_is_locked(&desc->lock)); @@ -379,11 +381,11 @@ static void msix_set_enable(struct pci_d pos = pci_find_cap_offset(seg, bus, slot, func, PCI_CAP_ID_MSIX); if ( pos ) { - control = pci_conf_read16(seg, bus, slot, func, pos + PCI_MSIX_FLAGS); + control = pci_conf_read16(seg, bus, slot, func, msix_control_reg(pos)); control &= ~PCI_MSIX_FLAGS_ENABLE; if ( enable ) control |= PCI_MSIX_FLAGS_ENABLE; - pci_conf_write16(seg, bus, slot, func, pos + PCI_MSIX_FLAGS, control); + pci_conf_write16(seg, bus, slot, func, msix_control_reg(pos), control); } } @@ -408,9 +410,11 @@ static bool_t msi_set_mask_bit(struct ir bus = pdev->bus; slot = PCI_SLOT(pdev->devfn); func = PCI_FUNC(pdev->devfn); - switch (entry->msi_attrib.type) { + switch ( entry->msi_attrib.type ) + { case PCI_CAP_ID_MSI: - if (entry->msi_attrib.maskbit) { + if ( entry->msi_attrib.maskbit ) + { u32 mask_bits; mask_bits = pci_conf_read32(seg, bus, slot, func, entry->msi.mpos); @@ -778,13 +782,14 @@ static u64 read_pci_mem_bar(u16 seg, u8 * requested MSI-X entries with allocated irqs or non-zero for otherwise. **/ static int msix_capability_init(struct pci_dev *dev, + unsigned int pos, struct msi_info *msi, struct msi_desc **desc, unsigned int nr_entries) { struct arch_msix *msix = dev->msix; struct msi_desc *entry = NULL; - int pos, vf; + int vf; u16 control; u64 table_paddr; u32 table_offset; @@ -796,7 +801,6 @@ static int msix_capability_init(struct p ASSERT(spin_is_locked(&pcidevs_lock)); - pos = pci_find_cap_offset(seg, bus, slot, func, PCI_CAP_ID_MSIX); control = pci_conf_read16(seg, bus, slot, func, msix_control_reg(pos)); /* * Ensure MSI-X interrupts are masked during setup. Some devices require @@ -984,10 +988,9 @@ static int __pci_enable_msi(struct msi_i old_desc = find_msi_entry(pdev, msi->irq, PCI_CAP_ID_MSI); if ( old_desc ) { - dprintk(XENLOG_WARNING, "irq %d has already mapped to MSI on " - "device %04x:%02x:%02x.%01x\n", - msi->irq, msi->seg, msi->bus, - PCI_SLOT(msi->devfn), PCI_FUNC(msi->devfn)); + printk(XENLOG_WARNING "irq %d already mapped to MSI on %04x:%02x:%02x.%u\n", + msi->irq, msi->seg, msi->bus, + PCI_SLOT(msi->devfn), PCI_FUNC(msi->devfn)); *desc = old_desc; return 0; } @@ -995,10 +998,10 @@ static int __pci_enable_msi(struct msi_i old_desc = find_msi_entry(pdev, -1, PCI_CAP_ID_MSIX); if ( old_desc ) { - dprintk(XENLOG_WARNING, "MSI-X is already in use on " - "device %04x:%02x:%02x.%01x\n", msi->seg, msi->bus, - PCI_SLOT(msi->devfn), PCI_FUNC(msi->devfn)); - pci_disable_msi(old_desc); + printk(XENLOG_WARNING "MSI-X already in use on %04x:%02x:%02x.%u\n", + msi->seg, msi->bus, + PCI_SLOT(msi->devfn), PCI_FUNC(msi->devfn)); + __pci_disable_msix(old_desc); } return msi_capability_init(pdev, msi->irq, desc, msi->entry_nr); @@ -1012,7 +1015,6 @@ static void __pci_disable_msi(struct msi msi_set_enable(dev, 0); BUG_ON(list_empty(&dev->msi_list)); - } /** @@ -1032,7 +1034,7 @@ static void __pci_disable_msi(struct msi **/ static int __pci_enable_msix(struct msi_info *msi, struct msi_desc **desc) { - int status, pos, nr_entries; + int pos, nr_entries; struct pci_dev *pdev; u16 control; u8 slot = PCI_SLOT(msi->devfn); @@ -1041,23 +1043,22 @@ static int __pci_enable_msix(struct msi_ ASSERT(spin_is_locked(&pcidevs_lock)); pdev = pci_get_pdev(msi->seg, msi->bus, msi->devfn); - if ( !pdev ) + pos = pci_find_cap_offset(msi->seg, msi->bus, slot, func, PCI_CAP_ID_MSIX); + if ( !pdev || !pos ) return -ENODEV; - pos = pci_find_cap_offset(msi->seg, msi->bus, slot, func, PCI_CAP_ID_MSIX); control = pci_conf_read16(msi->seg, msi->bus, slot, func, msix_control_reg(pos)); nr_entries = multi_msix_capable(control); - if (msi->entry_nr >= nr_entries) + if ( msi->entry_nr >= nr_entries ) return -EINVAL; old_desc = find_msi_entry(pdev, msi->irq, PCI_CAP_ID_MSIX); if ( old_desc ) { - dprintk(XENLOG_WARNING, "irq %d has already mapped to MSIX on " - "device %04x:%02x:%02x.%01x\n", - msi->irq, msi->seg, msi->bus, - PCI_SLOT(msi->devfn), PCI_FUNC(msi->devfn)); + printk(XENLOG_WARNING "irq %d already mapped to MSI-X on %04x:%02x:%02x.%u\n", + msi->irq, msi->seg, msi->bus, + PCI_SLOT(msi->devfn), PCI_FUNC(msi->devfn)); *desc = old_desc; return 0; } @@ -1065,15 +1066,13 @@ static int __pci_enable_msix(struct msi_ old_desc = find_msi_entry(pdev, -1, PCI_CAP_ID_MSI); if ( old_desc ) { - dprintk(XENLOG_WARNING, "MSI is already in use on " - "device %04x:%02x:%02x.%01x\n", msi->seg, msi->bus, - PCI_SLOT(msi->devfn), PCI_FUNC(msi->devfn)); - pci_disable_msi(old_desc); - + printk(XENLOG_WARNING "MSI already in use on %04x:%02x:%02x.%u\n", + msi->seg, msi->bus, + PCI_SLOT(msi->devfn), PCI_FUNC(msi->devfn)); + __pci_disable_msi(old_desc); } - status = msix_capability_init(pdev, msi, desc, nr_entries); - return status; + return msix_capability_init(pdev, pos, msi, desc, nr_entries); } static void _pci_cleanup_msix(struct arch_msix *msix) @@ -1091,19 +1090,16 @@ static void _pci_cleanup_msix(struct arc static void __pci_disable_msix(struct msi_desc *entry) { - struct pci_dev *dev; - int pos; - u16 control, seg; - u8 bus, slot, func; - - dev = entry->dev; - seg = dev->seg; - bus = dev->bus; - slot = PCI_SLOT(dev->devfn); - func = PCI_FUNC(dev->devfn); + struct pci_dev *dev = entry->dev; + u16 seg = dev->seg; + u8 bus = dev->bus; + u8 slot = PCI_SLOT(dev->devfn); + u8 func = PCI_FUNC(dev->devfn); + unsigned int pos = pci_find_cap_offset(seg, bus, slot, func, + PCI_CAP_ID_MSIX); + u16 control = pci_conf_read16(seg, bus, slot, func, + msix_control_reg(entry->msi_attrib.pos)); - pos = pci_find_cap_offset(seg, bus, slot, func, PCI_CAP_ID_MSIX); - control = pci_conf_read16(seg, bus, slot, func, msix_control_reg(pos)); if ( unlikely(!(control & PCI_MSIX_FLAGS_ENABLE)) ) pci_conf_write16(seg, bus, slot, func, msix_control_reg(pos), control | (PCI_MSIX_FLAGS_ENABLE | @@ -1156,7 +1152,7 @@ int pci_prepare_msix(u16 seg, u8 bus, u8 u16 control = pci_conf_read16(seg, bus, slot, func, msix_control_reg(pos)); - rc = msix_capability_init(pdev, NULL, NULL, + rc = msix_capability_init(pdev, pos, NULL, NULL, multi_msix_capable(control)); } spin_unlock(&pcidevs_lock); @@ -1175,8 +1171,8 @@ int pci_enable_msi(struct msi_info *msi, if ( !use_msi ) return -EPERM; - return msi->table_base ? __pci_enable_msix(msi, desc) : - __pci_enable_msi(msi, desc); + return msi->table_base ? __pci_enable_msix(msi, desc) : + __pci_enable_msi(msi, desc); } /* @@ -1229,7 +1225,9 @@ int pci_restore_msi_state(struct pci_dev if ( !pdev ) return -EINVAL; - ret = xsm_resource_setup_pci(XSM_PRIV, (pdev->seg << 16) | (pdev->bus << 8) | pdev->devfn); + ret = xsm_resource_setup_pci(XSM_PRIV, + (pdev->seg << 16) | (pdev->bus << 8) | + pdev->devfn); if ( ret ) return ret;