Date: Wed, 6 May 2015 18:11:57 -0500 From: Eric Badger <eric.badger@compellent.com> To: Ryan Stone <rysto32@gmail.com>, FreeBSD Current <freebsd-current@freebsd.org>, John Baldwin <jhb@freebsd.org> Subject: Re: PCI PF memory decode disable when sizing VF BARs Message-ID: <554A9FBD.4010908@compellent.com> In-Reply-To: <CAFMmRNwHTpQ6Q8-=GhTgt64SgOZkB9QWsmgMsavW2-cVcp_p7Q@mail.gmail.com> References: <cb2ce4e0ff294bcd982c6db87ae64dfb@mspexmb1.Beer.Town> <11092809.7nmbPfKl0V@ralph.baldwin.cx> <CAFMmRNyUiet0O2WXBv1K%2BTF=qtQT7Tdwf4npcCNKZiF4SZVh8w@mail.gmail.com> <2106775.8KnH0oLhUZ@ralph.baldwin.cx> <554A6457.4060702@compellent.com> <CAFMmRNwHTpQ6Q8-=GhTgt64SgOZkB9QWsmgMsavW2-cVcp_p7Q@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On 05/06/15 14:54, Ryan Stone wrote:
> On Wed, May 6, 2015 at 2:33 PM, John Baldwin <jhb@freebsd.org
> <mailto:jhb@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@compellent.com <mailto:eric.badger@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?554A9FBD.4010908>