On 05/06/15 14:54, Ryan Stone wrote:
On Wed, May 6, 2015 at 2:33 PM, John Baldwin <j...@freebsd.org <mailto:j...@freebsd.org>> wrote:


    Ah, sorry, I didn't know you did it in the caller already.  Perhaps
    then something more like your previous patch, but using the test you
    added here (PCIR_IS_IOV) instead of your previous check against BAR
    values to decide when to frob the command register?

I think that I prefer the current version, as it keeps the interface consistent. It's redundant now, but caller could evolve in the future. Given that this is just being run during initialization a couple of extra register accesses are irrelevant anyway.

On Wed, May 6, 2015 at 2:58 PM, Eric Badger <eric.bad...@compellent.com <mailto:eric.bad...@compellent.com>> wrote:

    Does the disabling of VF MSE in pci_iov_config actually protect
    anything else beyond what happens in pci_read_bar? I gave a read
    through which suggests "no", but I might have missed something.
    Just thinking that the code would be a bit more hardy if it were
    done the same way for both VFs and other devices.

    Eric


I think that it inherently has to be done differently. For real PCI devices the device might be important during the boot process (e.g. the video card) so we need to stay working. For VFs the devices don't even exist until I enable the VF Enable bit is set, so setting MSE before that point is irrelevant (it's allowed by the spec, but any access to a VF memory space with MSE set and VF Enable clear just gets an Unsupported Request response).

Sure; what I meant was to leave the disabling of VF MSE when sizing VF BARs in pci_read_bar (as in your second patch) for consistency and, if possible, not bother disabling VF MSE in pci_iov_config. But if it's not worth nixing the latter (or not possible), it's no big deal.

I've been testing out the second patch in my environment and it looks good. I might suggest something like the below (which I find more readable) as a cosmetic change:

@@ -2627,9 +2635,18 @@ pci_read_bar(device_t dev, int reg, pci_addr_t *mapp, pci_addr_t *testvalp,
         * determining the BAR's length since we will be placing it in
         * a weird state.
         */
-   cmd = pci_read_config(dev, PCIR_COMMAND, 2);
-   pci_write_config(dev, PCIR_COMMAND,
-       cmd & ~(PCI_BAR_MEM(map) ? PCIM_CMD_MEMEN : PCIM_CMD_PORTEN), 2);
+#ifdef PCI_IOV
+    if (PCIR_IS_IOV(&dinfo->cfg, reg)) {
+        restore_reg = dinfo->cfg.iov->iov_pos + PCIR_SRIOV_CTL;
+        mask = PCIM_SRIOV_VF_MSE;
+    } else
+#endif
+    {
+        restore_reg = PCIR_COMMAND;
+        mask = PCI_BAR_MEM(map) ? PCIM_CMD_MEMEN : PCIM_CMD_PORTEN;
+    }
+    cmd = pci_read_config(dev, restore_reg, 2);
+    pci_write_config(dev, restore_reg, cmd & ~mask, 2);


Thanks,
Eric
_______________________________________________
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"

Reply via email to