Re: PCI PF memory decode disable when sizing VF BARs

[Eric Badger]

On 05/06/15 14:54, Ryan Stone wrote:
On Wed, May 6, 2015 at 2:33 PM, John Baldwin > 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 
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"

Re: PCI PF memory decode disable when sizing VF BARs

[Eric Badger]

On 05/06/15 13:33, John Baldwin wrote:

On Wednesday, May 06, 2015 02:24:24 PM Ryan Stone wrote:

On Wed, May 6, 2015 at 11:45 AM, John Baldwin  wrote:


There are some devices with BARs in non-standard locations. :( If there is
a flag to just disable the VF bar decoding, then ideally we should just be
doing that and leaving the global decoding flag alone while sizing the VF
BAR.


Disabling SR-IOV BAR decoding in this function is currently redundant, as
it's already done in pci_iov.c, but I guess to keep the interface sane it
makes sense to do it here too.  Something like this then?

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?



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
___
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"

Re: PCI PF memory decode disable when sizing VF BARs

[John Baldwin]

On Wednesday, May 06, 2015 02:24:24 PM Ryan Stone wrote:
> On Wed, May 6, 2015 at 11:45 AM, John Baldwin  wrote:
> 
> > There are some devices with BARs in non-standard locations. :( If there is
> > a flag to just disable the VF bar decoding, then ideally we should just be
> > doing that and leaving the global decoding flag alone while sizing the VF
> > BAR.
> >
> 
> Disabling SR-IOV BAR decoding in this function is currently redundant, as
> it's already done in pci_iov.c, but I guess to keep the interface sane it
> makes sense to do it here too.  Something like this then?

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?

-- 
John Baldwin
___
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"

Re: PCI PF memory decode disable when sizing VF BARs

[Ryan Stone]

On Wed, May 6, 2015 at 11:45 AM, John Baldwin  wrote:

> There are some devices with BARs in non-standard locations. :( If there is
> a flag to just disable the VF bar decoding, then ideally we should just be
> doing that and leaving the global decoding flag alone while sizing the VF
> BAR.
>

Disabling SR-IOV BAR decoding in this function is currently redundant, as
it's already done in pci_iov.c, but I guess to keep the interface sane it
makes sense to do it here too.  Something like this then?

diff --git a/sys/dev/pci/pci.c b/sys/dev/pci/pci.c
index b4c6151..c9d7541 100644
--- a/sys/dev/pci/pci.c
+++ b/sys/dev/pci/pci.c
@@ -37,6 +37,7 @@ __FBSDID("$FreeBSD$");
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -62,6 +63,7 @@ __FBSDID("$FreeBSD$");
 #include 
 #include 
 #include 
+#include 

 #include 
 #include 
@@ -75,6 +77,11 @@ __FBSDID("$FreeBSD$");
(((cfg)->hdrtype == PCIM_HDRTYPE_NORMAL && reg == PCIR_BIOS) || \
 ((cfg)->hdrtype == PCIM_HDRTYPE_BRIDGE && reg == PCIR_BIOS_1))

+#definePCIR_IS_IOV(cfg,
reg)  \
+   (((cfg)->iov != NULL) &&\
+ ((reg) >= (cfg)->iov->iov_pos + PCIR_SRIOV_BAR(0)) && \
+ ((reg) <= (cfg)->iov->iov_pos + PCIR_SRIOV_BAR(PCIR_MAX_BAR_0)))
+
 static int pci_has_quirk(uint32_t devid, int quirk);
 static pci_addr_t  pci_mapbase(uint64_t mapreg);
 static const char  *pci_maptype(uint64_t mapreg);
@@ -2647,7 +2654,8 @@ pci_read_bar(device_t dev, int reg, pci_addr_t *mapp,
pci_addr_t *testvalp,
struct pci_devinfo *dinfo;
pci_addr_t map, testval;
int ln2range;
-   uint16_t cmd;
+   uint32_t restore_reg;
+   uint16_t cmd, mask;

/*
 * The device ROM BAR is special.  It is always a 32-bit
@@ -2677,9 +2685,21 @@ 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;
+   cmd = pci_read_config(dev, restore_reg, 2);
+   pci_write_config(dev, restore_reg, cmd &
~PCIM_SRIOV_VF_MSE, 2);
+   } else
+#endif
+   {
+   cmd = pci_read_config(dev, PCIR_COMMAND, 2);
+   mask = PCI_BAR_MEM(map) ? PCIM_CMD_MEMEN : PCIM_CMD_PORTEN;
+   pci_write_config(dev, PCIR_COMMAND, cmd & ~mask, 2);
+   restore_reg = PCIR_COMMAND;
+   }

/*
 * Determine the BAR's length by writing all 1's.  The bottom
@@ -2701,7 +2721,7 @@ pci_read_bar(device_t dev, int reg, pci_addr_t *mapp,
pci_addr_t *testvalp,
pci_write_config(dev, reg, map, 4);
if (ln2range == 64)
pci_write_config(dev, reg + 4, map >> 32, 4);
-   pci_write_config(dev, PCIR_COMMAND, cmd, 2);
+   pci_write_config(dev, restore_reg, cmd, 2);

*mapp = map;
*testvalp = testval;
___
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"

Re: PCI PF memory decode disable when sizing VF BARs

[John Baldwin]

On Tuesday, May 05, 2015 07:15:21 PM Ryan Stone wrote:
> On Tue, May 5, 2015 at 9:17 AM, Eric Badger  wrote:
> 
> > Hi Ryan and -current,
> >
> > During IOV config, when setting up VF bars, several calls are made to
> > 'pci_read_bar' (in sys/dev/pci/pci.c) in order to size VF BARs, which
> > causes memory decoding to be turned off temporarily for the PF associated
> > with those VFs. I'm finding that this can interfere with an already running
> > PF.
> >
> 
> Ouch.  That's a nasty bug.  Thanks for tracking this down.
> 
> 
> > 1. Check the value of the 'reg' arg to 'pci_read_bar' and, if it is
> > outside a standard BAR range, don't disable memory decoding. This is
> > simple, but feels a little hackish and may have consequences I'm missing.
> >
> 2. Pass some flag/context through such that pci_read_bar knows it is
> > configuring VF BARs (we might instead disable VF MSE in this case, if it is
> > enabled). It would be necessary to carry this flag/context through several
> > function calls before reaching pci_read_bar, which might end up being ugly.
> >
> 3. Rearrange the calls so that VF BARs are sized when the PF is not yet
> > running, and that info saved until VFs are created. Probably it would be
> > done when the PF BARs are sized for any device supporting IOV, even if that
> > device never creates VFs.
> >
> 
> I don't think that #3 is possible.  Unfortunately the BAR is sized again
> when the BAR is allocated so it's difficult so it wouldn't be enough to
> just size the BAR during attach.  I would have to reserve the memory space
> during attach, but that might reserve physical address space that other
> devices need to function.
> 
> Actually, that problem will prevent #2 from being easy to implement too.
> We'd have to add an extra flag to pci_alloc_multi_resource.  I think that
> #1 is the best option.  There's already a precedent for something similar
> has it has a special base for the device ROM BAR.
> 
> I haven't had a chance to test this yet, but I believe that this patch will
> solve the problem:
> 
> https://people.freebsd.org/~rstone/patches/iov_bar.diff

There are some devices with BARs in non-standard locations. :(  If there is
a flag to just disable the VF bar decoding, then ideally we should just be
doing that and leaving the global decoding flag alone while sizing the VF
BAR.

-- 
John Baldwin
___
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"

Re: PCI PF memory decode disable when sizing VF BARs

[Ryan Stone]

On Tue, May 5, 2015 at 9:17 AM, Eric Badger  wrote:

> Hi Ryan and -current,
>
> During IOV config, when setting up VF bars, several calls are made to
> 'pci_read_bar' (in sys/dev/pci/pci.c) in order to size VF BARs, which
> causes memory decoding to be turned off temporarily for the PF associated
> with those VFs. I'm finding that this can interfere with an already running
> PF.
>

Ouch.  That's a nasty bug.  Thanks for tracking this down.


> 1. Check the value of the 'reg' arg to 'pci_read_bar' and, if it is
> outside a standard BAR range, don't disable memory decoding. This is
> simple, but feels a little hackish and may have consequences I'm missing.
>
2. Pass some flag/context through such that pci_read_bar knows it is
> configuring VF BARs (we might instead disable VF MSE in this case, if it is
> enabled). It would be necessary to carry this flag/context through several
> function calls before reaching pci_read_bar, which might end up being ugly.
>
3. Rearrange the calls so that VF BARs are sized when the PF is not yet
> running, and that info saved until VFs are created. Probably it would be
> done when the PF BARs are sized for any device supporting IOV, even if that
> device never creates VFs.
>

I don't think that #3 is possible.  Unfortunately the BAR is sized again
when the BAR is allocated so it's difficult so it wouldn't be enough to
just size the BAR during attach.  I would have to reserve the memory space
during attach, but that might reserve physical address space that other
devices need to function.

Actually, that problem will prevent #2 from being easy to implement too.
We'd have to add an extra flag to pci_alloc_multi_resource.  I think that
#1 is the best option.  There's already a precedent for something similar
has it has a special base for the device ROM BAR.

I haven't had a chance to test this yet, but I believe that this patch will
solve the problem:

https://people.freebsd.org/~rstone/patches/iov_bar.diff
___
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"

PCI PF memory decode disable when sizing VF BARs

[Eric Badger]

Hi Ryan and -current,

During IOV config, when setting up VF bars, several calls are made to 
'pci_read_bar' (in sys/dev/pci/pci.c) in order to size VF BARs, which causes 
memory decoding to be turned off temporarily for the PF associated with those 
VFs. I'm finding that this can interfere with an already running PF. I've 
several thoughts about how this might be handled, but I'm not convinced I 
understand all of the consequences each of them entails, so any thoughts from 
others would be appreciated. Here are ideas I've considered:

1. Check the value of the 'reg' arg to 'pci_read_bar' and, if it is outside a 
standard BAR range, don't disable memory decoding. This is simple, but feels a 
little hackish and may have consequences I'm missing.
2. Pass some flag/context through such that pci_read_bar knows it is 
configuring VF BARs (we might instead disable VF MSE in this case, if it is 
enabled). It would be necessary to carry this flag/context through several 
function calls before reaching pci_read_bar, which might end up being ugly.
3. Rearrange the calls so that VF BARs are sized when the PF is not yet 
running, and that info saved until VFs are created. Probably it would be done 
when the PF BARs are sized for any device supporting IOV, even if that device 
never creates VFs.

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"