pci: Don't cache the count of MSI/MSI-X messages before allocation

A device can in theory change the read-only fields in the MSI/MSI-X
control registers that indicate the maximum number of supported
registers in response to changing other device registers.  For
example, certain Intel networking VFs change the number of messages as
a result of changes in the PCI_IOV_ADD_VF callback.

To support this, always read the current value of the relevant control
register in the *_count and *_alloc methods.  Once messages have been
allocated, the control register value remains cached.

Reported by:	Krzysztof Galazka <krzysztof.galazka@intel.com>
Reviewed by:	Krzysztof Galazka <krzysztof.galazka@intel.com>, erj
Differential Revision:	https://reviews.freebsd.org/D48890

sys/dev/iavf/iavf_lib.c

@@ -1463,31 +1463,6 @@ iavf_mark_del_vlan_filter(struct iavf_sc *sc, u16 vtag)
 	return (i);
 }
 
-/**
- * iavf_update_msix_devinfo - Fix MSIX values for pci_msix_count()
- * @dev: pointer to kernel device
- *
- * Fix cached MSI-X control register information. This is a workaround
- * for an issue where VFs spawned in non-passthrough mode on FreeBSD
- * will have their PCI information cached before the PF driver
- * finishes updating their PCI information.
- *
- * @pre Must be called before pci_msix_count()
- */
-void
-iavf_update_msix_devinfo(device_t dev)
-{
-	struct pci_devinfo *dinfo;
-	u32 msix_ctrl;
-	u8 msix_location;
-
-	dinfo = (struct pci_devinfo *)device_get_ivars(dev);
-	msix_location = dinfo->cfg.msix.msix_location;
-	msix_ctrl = pci_read_config(dev, msix_location + PCIR_MSIX_CTRL, 2);
-	dinfo->cfg.msix.msix_ctrl = msix_ctrl;
-	dinfo->cfg.msix.msix_msgnum = (msix_ctrl & PCIM_MSIXCTRL_TABLE_SIZE) + 1;
-}
-
 /**
  * iavf_disable_queues_with_retries - Send PF multiple DISABLE_QUEUES messages
  * @sc: device softc

sys/dev/iavf/iavf_lib.h

@@ -474,7 +474,6 @@ struct iavf_mac_filter *
 u64 iavf_baudrate_from_link_speed(struct iavf_sc *sc);
 void iavf_add_vlan_filter(struct iavf_sc *sc, u16 vtag);
 int iavf_mark_del_vlan_filter(struct iavf_sc *sc, u16 vtag);
-void iavf_update_msix_devinfo(device_t dev);
 void iavf_disable_queues_with_retries(struct iavf_sc *);
 
 int iavf_sysctl_current_speed(SYSCTL_HANDLER_ARGS);

sys/dev/iavf/if_iavf_iflib.c

@@ -379,9 +379,6 @@ iavf_if_attach_pre(if_ctx_t ctx)
 	scctx->isc_capabilities = scctx->isc_capenable = IAVF_CAPS;
 	scctx->isc_tx_csum_flags = CSUM_OFFLOAD;
 
-	/* Update OS cache of MSIX control register values */
-	iavf_update_msix_devinfo(dev);
-
 	return (0);
 
 err_vc_tq:

sys/dev/pci/pci.c

@@ -952,14 +952,10 @@ pci_read_cap(device_t pcib, pcicfgregs *cfg)
 		case PCIY_MSI:		/* PCI MSI */
 			cfg->msi.msi_location = ptr;
 			cfg->msi.msi_ctrl = REG(ptr + PCIR_MSI_CTRL, 2);
-			cfg->msi.msi_msgnum = 1 << ((cfg->msi.msi_ctrl &
-						     PCIM_MSICTRL_MMC_MASK)>>1);
 			break;
 		case PCIY_MSIX:		/* PCI MSI-X */
 			cfg->msix.msix_location = ptr;
 			cfg->msix.msix_ctrl = REG(ptr + PCIR_MSIX_CTRL, 2);
-			cfg->msix.msix_msgnum = (cfg->msix.msix_ctrl &
-			    PCIM_MSIXCTRL_TABLE_SIZE) + 1;
 			val = REG(ptr + PCIR_MSIX_TABLE, 4);
 			cfg->msix.msix_table_bar = PCIR_BAR(val &
 			    PCIM_MSIX_BIR_MASK);
@@ -1734,7 +1730,7 @@ pci_mask_msix(device_t dev, u_int index)
 	struct pcicfg_msix *msix = &dinfo->cfg.msix;
 	uint32_t offset, val;
 
-	KASSERT(msix->msix_msgnum > index, ("bogus index"));
+	KASSERT(PCI_MSIX_MSGNUM(msix->msix_ctrl) > index, ("bogus index"));
 	offset = msix->msix_table_offset + index * 16 + 12;
 	val = bus_read_4(msix->msix_table_res, offset);
 	val |= PCIM_MSIX_VCTRL_MASK;
@@ -1753,7 +1749,7 @@ pci_unmask_msix(device_t dev, u_int index)
 	struct pcicfg_msix *msix = &dinfo->cfg.msix;
 	uint32_t offset, val;
 
-	KASSERT(msix->msix_table_len > index, ("bogus index"));
+	KASSERT(PCI_MSIX_MSGNUM(msix->msix_ctrl) > index, ("bogus index"));
 	offset = msix->msix_table_offset + index * 16 + 12;
 	val = bus_read_4(msix->msix_table_res, offset);
 	val &= ~PCIM_MSIX_VCTRL_MASK;
@@ -1790,11 +1786,13 @@ pci_resume_msix(device_t dev)
 	struct pcicfg_msix *msix = &dinfo->cfg.msix;
 	struct msix_table_entry *mte;
 	struct msix_vector *mv;
-	u_int i;
+	u_int i, msgnum;
 
 	if (msix->msix_alloc > 0) {
+		msgnum = PCI_MSIX_MSGNUM(msix->msix_ctrl);
+
 		/* First, mask all vectors. */
-		for (i = 0; i < msix->msix_msgnum; i++)
+		for (i = 0; i < msgnum; i++)
 			pci_mask_msix(dev, i);
 
 		/* Second, program any messages with at least one handler. */
@@ -1825,6 +1823,7 @@ pci_alloc_msix_method(device_t dev, device_t child, int *count)
 	struct resource_list_entry *rle;
 	u_int actual, i, max;
 	int error, irq;
+	uint16_t ctrl, msgnum;
 
 	/* Don't let count == 0 get us into trouble. */
 	if (*count < 1)
@@ -1863,11 +1862,14 @@ pci_alloc_msix_method(device_t dev, device_t child, int *count)
 	}
 	cfg->msix.msix_pba_res = rle->res;
 
+	ctrl = pci_read_config(child, cfg->msix.msix_location + PCIR_MSIX_CTRL,
+	    2);
+	msgnum = PCI_MSIX_MSGNUM(ctrl);
 	if (bootverbose)
 		device_printf(child,
 		    "attempting to allocate %d MSI-X vectors (%d supported)\n",
-		    *count, cfg->msix.msix_msgnum);
+		    *count, msgnum);
-	max = min(*count, cfg->msix.msix_msgnum);
+	max = min(*count, msgnum);
 	for (i = 0; i < max; i++) {
 		/* Allocate a message. */
 		error = PCIB_ALLOC_MSIX(device_get_parent(dev), child, &irq);
@@ -1927,7 +1929,7 @@ pci_alloc_msix_method(device_t dev, device_t child, int *count)
 	}
 
 	/* Mask all vectors. */
-	for (i = 0; i < cfg->msix.msix_msgnum; i++)
+	for (i = 0; i < msgnum; i++)
 		pci_mask_msix(child, i);
 
 	/* Allocate and initialize vector data and virtual table. */
@@ -1942,9 +1944,10 @@ pci_alloc_msix_method(device_t dev, device_t child, int *count)
 	}
 
 	/* Update control register to enable MSI-X. */
-	cfg->msix.msix_ctrl |= PCIM_MSIXCTRL_MSIX_ENABLE;
+	ctrl |= PCIM_MSIXCTRL_MSIX_ENABLE;
 	pci_write_config(child, cfg->msix.msix_location + PCIR_MSIX_CTRL,
-	    cfg->msix.msix_ctrl, 2);
+	    ctrl, 2);
+	cfg->msix.msix_ctrl = ctrl;
 
 	/* Update counts of alloc'd messages. */
 	cfg->msix.msix_alloc = actual;
@@ -2007,7 +2010,7 @@ pci_remap_msix_method(device_t dev, device_t child, int count,
 	 * table can't be bigger than the actual MSI-X table in the
 	 * device.
 	 */
-	if (count < 1 || count > msix->msix_msgnum)
+	if (count < 1 || count > PCI_MSIX_MSGNUM(msix->msix_ctrl))
 		return (EINVAL);
 
 	/* Sanity check the vectors. */
@@ -2173,9 +2176,13 @@ pci_msix_count_method(device_t dev, device_t child)
 {
 	struct pci_devinfo *dinfo = device_get_ivars(child);
 	struct pcicfg_msix *msix = &dinfo->cfg.msix;
+	uint16_t ctrl;
 
-	if (pci_do_msix && msix->msix_location != 0)
+	if (pci_do_msix && msix->msix_location != 0) {
-		return (msix->msix_msgnum);
+		ctrl = pci_read_config(child, msix->msix_location +
+		    PCIR_MSI_CTRL, 2);
+		return (PCI_MSIX_MSGNUM(ctrl));
+	}
 	return (0);
 }
 
@@ -2610,7 +2617,7 @@ pci_alloc_msi_method(device_t dev, device_t child, int *count)
 	struct resource_list_entry *rle;
 	u_int actual, i;
 	int error, irqs[32];
-	uint16_t ctrl;
+	uint16_t ctrl, msgnum;
 
 	/* Don't let count == 0 get us into trouble. */
 	if (*count < 1)
@@ -2633,13 +2640,15 @@ pci_alloc_msi_method(device_t dev, device_t child, int *count)
 	if (cfg->msi.msi_location == 0 || !pci_do_msi)
 		return (ENODEV);
 
+	ctrl = pci_read_config(child, cfg->msi.msi_location + PCIR_MSI_CTRL, 2);
+	msgnum = PCI_MSI_MSGNUM(ctrl);
 	if (bootverbose)
 		device_printf(child,
-		    "attempting to allocate %d MSI vectors (%d supported)\n",
+		    "attempting to allocate %d MSI vectors (%u supported)\n",
-		    *count, cfg->msi.msi_msgnum);
+		    *count, msgnum);
 
 	/* Don't ask for more than the device supports. */
-	actual = min(*count, cfg->msi.msi_msgnum);
+	actual = min(*count, msgnum);
 
 	/* Don't ask for more than 32 messages. */
 	actual = min(actual, 32);
@@ -2708,7 +2717,6 @@ pci_alloc_msi_method(device_t dev, device_t child, int *count)
 	}
 
 	/* Update control register with actual count. */
-	ctrl = cfg->msi.msi_ctrl;
 	ctrl &= ~PCIM_MSICTRL_MME_MASK;
 	ctrl |= (ffs(actual) - 1) << 4;
 	cfg->msi.msi_ctrl = ctrl;
@@ -2782,9 +2790,13 @@ pci_msi_count_method(device_t dev, device_t child)
 {
 	struct pci_devinfo *dinfo = device_get_ivars(child);
 	struct pcicfg_msi *msi = &dinfo->cfg.msi;
+	uint16_t ctrl;
 
-	if (pci_do_msi && msi->msi_location != 0)
+	if (pci_do_msi && msi->msi_location != 0) {
-		return (msi->msi_msgnum);
+		ctrl = pci_read_config(child, msi->msi_location + PCIR_MSI_CTRL,
+		    2);
+		return (PCI_MSI_MSGNUM(ctrl));
+	}
 	return (0);
 }
 
@@ -3038,19 +3050,21 @@ pci_print_verbose(struct pci_devinfo *dinfo)
 			    status & PCIM_PSTAT_DMASK);
 		}
 		if (cfg->msi.msi_location) {
-			int ctrl;
+			uint16_t ctrl, msgnum;
 
 			ctrl = cfg->msi.msi_ctrl;
+			msgnum = PCI_MSI_MSGNUM(ctrl);
 			printf("\tMSI supports %d message%s%s%s\n",
-			    cfg->msi.msi_msgnum,
+			    msgnum, (msgnum == 1) ? "" : "s",
-			    (cfg->msi.msi_msgnum == 1) ? "" : "s",
 			    (ctrl & PCIM_MSICTRL_64BIT) ? ", 64 bit" : "",
 			    (ctrl & PCIM_MSICTRL_VECTOR) ? ", vector masks":"");
 		}
 		if (cfg->msix.msix_location) {
+			uint16_t msgnum;
+
+			msgnum = PCI_MSIX_MSGNUM(cfg->msix.msix_ctrl);
 			printf("\tMSI-X supports %d message%s ",
-			    cfg->msix.msix_msgnum,
+			    msgnum, (msgnum == 1) ? "" : "s");
-			    (cfg->msix.msix_msgnum == 1) ? "" : "s");
 			if (cfg->msix.msix_table_bar == cfg->msix.msix_pba_bar)
 				printf("in map 0x%x\n",
 				    cfg->msix.msix_table_bar);

sys/dev/pci/pcireg.h

@@ -616,6 +616,8 @@
 #define	PCIM_MSICTRL_MMC_16		0x0008
 #define	PCIM_MSICTRL_MMC_32		0x000A
 #define	PCIM_MSICTRL_MSI_ENABLE		0x0001
+#define	PCI_MSI_MSGNUM(ctrl)						\
+	(1 << (((ctrl) & PCIM_MSICTRL_MMC_MASK) >> 1))
 #define	PCIR_MSI_ADDR		0x4
 #define	PCIR_MSI_ADDR_HIGH	0x8
 #define	PCIR_MSI_DATA		0x8
@@ -965,6 +967,7 @@
 #define	PCIM_MSIXCTRL_MSIX_ENABLE	0x8000
 #define	PCIM_MSIXCTRL_FUNCTION_MASK	0x4000
 #define	PCIM_MSIXCTRL_TABLE_SIZE	0x07FF
+#define	PCI_MSIX_MSGNUM(ctrl)	(((ctrl) & PCIM_MSIXCTRL_TABLE_SIZE) + 1)
 #define	PCIR_MSIX_TABLE		0x4
 #define	PCIR_MSIX_PBA		0x8
 #define	PCIM_MSIX_BIR_MASK		0x7

sys/dev/pci/pcivar.h

@@ -90,7 +90,6 @@ struct pcicfg_vpd {
 struct pcicfg_msi {
     uint16_t	msi_ctrl;	/* Message Control */
     uint8_t	msi_location;	/* Offset of MSI capability registers. */
-    uint8_t	msi_msgnum;	/* Number of messages */
     int		msi_alloc;	/* Number of allocated messages. */
     uint64_t	msi_addr;	/* Contents of address register. */
     uint16_t	msi_data;	/* Contents of data register. */
@@ -111,7 +110,6 @@ struct msix_table_entry {
 
 struct pcicfg_msix {
     uint16_t	msix_ctrl;	/* Message Control */
-    uint16_t	msix_msgnum;	/* Number of messages */
     uint8_t	msix_location;	/* Offset of MSI-X capability registers. */
     uint8_t	msix_table_bar;	/* BAR containing vector table. */
     uint8_t	msix_pba_bar;	/* BAR containing PBA. */