Re: PCI PF memory decode disable when sizing VF BARs

On 05/06/15 14:54, Ryan Stone wrote:
> On Wed, May 6, 2015 at 2:33 PM, John Baldwin <jhb_at_freebsd.org 
> <mailto:jhb_at_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.badger_at_compellent.com <mailto:eric.badger_at_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:

_at__at_ -2627,9 +2635,18 _at__at_ 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