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"
