[PATCH] PCI: Init NumVFs register to zero in sriov_init()

[PATCH] PCI: Init NumVFs register to zero in sriov_init()

[ethan.zhao]

Though no specification about NumVFs register initial value after POST, to void the confusion
lspci output as following before VF was enabled, we should clear the NumVFs value left by BIOS
to zero:

$lspci -vvv -s 03:00.0
Ethernet controller: Intel Corporation 82599EB 10-Gigabit SFI/SFP+ Network Connection (rev 01)
~
Capabilities: [160 v1] Single Root I/O Virtualization (SR-IOV)
		IOVCap:	Migration-, Interrupt Message Number: 000
		IOVCtl:	Enable+ Migration- Interrupt- MSE+ ARIHierarchy+
		IOVSta:	Migration-
		Initial VFs: 64, Total VFs: 64, Number of VFs: 64, Function Dependency Link: 00
                                                              ^dazed !
~
Signed-off-by: ethan.zhao <ethan.kernel@gmail.com>
---
 drivers/pci/iov.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index de8ffac..a4941ad 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -439,6 +439,8 @@ static int sriov_init(struct pci_dev *dev, int pos)
 
 found:
 	pci_write_config_word(dev, pos + PCI_SRIOV_CTRL, ctrl);
+	/* VF Enable is cleared, so we could init the NumVFs register to 0 */
+	pci_write_config_word(dev, pos + PCI_SRIOV_NUM_VF, 0);
 	pci_read_config_word(dev, pos + PCI_SRIOV_VF_OFFSET, &offset);
 	pci_read_config_word(dev, pos + PCI_SRIOV_VF_STRIDE, &stride);
 	if (!offset || (total > 1 && !stride))
-- 
1.8.3.4 (Apple Git-47)

Re: [PATCH] PCI: Init NumVFs register to zero in sriov_init()

[Ethan Zhao]

Yinghai,
     Could you help to take a look, we need it in our downstream kernel.


Thanks,
Ethan

On Wed, Nov 6, 2013 at 10:49 PM, ethan.zhao <ethan.kernel@gmail.com> wrote:
> Though no specification about NumVFs register initial value after POST, to void the confusion
> lspci output as following before VF was enabled, we should clear the NumVFs value left by BIOS
> to zero:
>
> $lspci -vvv -s 03:00.0
> Ethernet controller: Intel Corporation 82599EB 10-Gigabit SFI/SFP+ Network Connection (rev 01)
> ~
> Capabilities: [160 v1] Single Root I/O Virtualization (SR-IOV)
>                 IOVCap: Migration-, Interrupt Message Number: 000
>                 IOVCtl: Enable+ Migration- Interrupt- MSE+ ARIHierarchy+
>                 IOVSta: Migration-
>                 Initial VFs: 64, Total VFs: 64, Number of VFs: 64, Function Dependency Link: 00
>                                                               ^dazed !
> ~
> Signed-off-by: ethan.zhao <ethan.kernel@gmail.com>
> ---
>  drivers/pci/iov.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index de8ffac..a4941ad 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -439,6 +439,8 @@ static int sriov_init(struct pci_dev *dev, int pos)
>
>  found:
>         pci_write_config_word(dev, pos + PCI_SRIOV_CTRL, ctrl);
> +       /* VF Enable is cleared, so we could init the NumVFs register to 0 */
> +       pci_write_config_word(dev, pos + PCI_SRIOV_NUM_VF, 0);
>         pci_read_config_word(dev, pos + PCI_SRIOV_VF_OFFSET, &offset);
>         pci_read_config_word(dev, pos + PCI_SRIOV_VF_STRIDE, &stride);
>         if (!offset || (total > 1 && !stride))
> --
> 1.8.3.4 (Apple Git-47)
>

Re: [PATCH] PCI: Init NumVFs register to zero in sriov_init()

[Yinghai Lu]

On Wed, Nov 13, 2013 at 5:57 PM, Ethan Zhao <ethan.kernel@gmail.com> wrote:
> On Wed, Nov 6, 2013 at 10:49 PM, ethan.zhao <ethan.kernel@gmail.com> wrote:
>> Though no specification about NumVFs register initial value after POST, to void the confusion
>> lspci output as following before VF was enabled, we should clear the NumVFs value left by BIOS
>> to zero:

Does BIOS need to clear it?


>>
>> $lspci -vvv -s 03:00.0
>> Ethernet controller: Intel Corporation 82599EB 10-Gigabit SFI/SFP+ Network Connection (rev 01)
>> ~
>> Capabilities: [160 v1] Single Root I/O Virtualization (SR-IOV)
>>                 IOVCap: Migration-, Interrupt Message Number: 000
>>                 IOVCtl: Enable+ Migration- Interrupt- MSE+ ARIHierarchy+
>>                 IOVSta: Migration-
>>                 Initial VFs: 64, Total VFs: 64, Number of VFs: 64, Function Dependency Link: 00
>>                                                               ^dazed !
>> ~

just display problem?

>> Signed-off-by: ethan.zhao <ethan.kernel@gmail.com>
>> ---
>>  drivers/pci/iov.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
>> index de8ffac..a4941ad 100644
>> --- a/drivers/pci/iov.c
>> +++ b/drivers/pci/iov.c
>> @@ -439,6 +439,8 @@ static int sriov_init(struct pci_dev *dev, int pos)
>>
>>  found:
>>         pci_write_config_word(dev, pos + PCI_SRIOV_CTRL, ctrl);
>> +       /* VF Enable is cleared, so we could init the NumVFs register to 0 */
>> +       pci_write_config_word(dev, pos + PCI_SRIOV_NUM_VF, 0);
>>         pci_read_config_word(dev, pos + PCI_SRIOV_VF_OFFSET, &offset);
>>         pci_read_config_word(dev, pos + PCI_SRIOV_VF_STRIDE, &stride);
>>         if (!offset || (total > 1 && !stride))

Thanks

Yinghai

Re: [PATCH] PCI: Init NumVFs register to zero in sriov_init()

[Ethan Zhao]

On Fri, Nov 15, 2013 at 2:59 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Wed, Nov 13, 2013 at 5:57 PM, Ethan Zhao <ethan.kernel@gmail.com> wrote:
>> On Wed, Nov 6, 2013 at 10:49 PM, ethan.zhao <ethan.kernel@gmail.com> wrote:
>>> Though no specification about NumVFs register initial value after POST, to void the confusion
>>> lspci output as following before VF was enabled, we should clear the NumVFs value left by BIOS
>>> to zero:
>
> Does BIOS need to clear it?

No, don't think BIOS need to clear it, according to the SR-IOV and PCI
specification, BIOS hasn't the responsibility to clear NumVFs register
 or not.  "The initial value of NumVFs is undefined." (SINGLE ROOT I/O
VIRTUALIZATION AND SHARING SPECIFICATION, REV. 1.1 chapter 3.3.7 ).
In fact, Linux doesn't trust the PCI_SRIOV_XX registers initial value
left by BIOS, exactly, it did in iov.c , Linux will re-initialize the
PCI_SRIOV_XX registers' value whatever their original value left by
BIOS.

static int sriov_init(struct pci_dev *dev, int pos)
{
  ... ...
  pci_read_config_word(dev, pos + PCI_SRIOV_CTRL, &ctrl);
  if (ctrl & PCI_SRIOV_CTRL_VFE) {
  pci_write_config_word(dev, pos + PCI_SRIOV_CTRL, 0);
  ssleep(1);
  }

... ...
ctrl = 0;
... ...
found:
pci_write_config_word(dev, pos + PCI_SRIOV_CTRL, ctrl);
... ...
}

If so, why leave  value of NumVFs register un-initialized, till be
exposed to user via lspci etc.

>
>
>>>
>>> $lspci -vvv -s 03:00.0
>>> Ethernet controller: Intel Corporation 82599EB 10-Gigabit SFI/SFP+ Network Connection (rev 01)
>>> ~
>>> Capabilities: [160 v1] Single Root I/O Virtualization (SR-IOV)
>>>                 IOVCap: Migration-, Interrupt Message Number: 000
>>>                 IOVCtl: Enable+ Migration- Interrupt- MSE+ ARIHierarchy+
>>>                 IOVSta: Migration-
>>>                 Initial VFs: 64, Total VFs: 64, Number of VFs: 64, Function Dependency Link: 00
>>>                                                                            ^dazed !
>>> ~
>
> just display problem?

So far, only catch the issue via lspci, "seems' only confuses user,
the VFs are enabled to 64. in fact, they are disabled. but it is not
bug of lspci, binary dump of lspci could prove it.


Thanks,
Ethan
>
>>> Signed-off-by: ethan.zhao <ethan.kernel@gmail.com>
>>> ---
>>>  drivers/pci/iov.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
>>> index de8ffac..a4941ad 100644
>>> --- a/drivers/pci/iov.c
>>> +++ b/drivers/pci/iov.c
>>> @@ -439,6 +439,8 @@ static int sriov_init(struct pci_dev *dev, int pos)
>>>
>>>  found:
>>>         pci_write_config_word(dev, pos + PCI_SRIOV_CTRL, ctrl);
>>> +       /* VF Enable is cleared, so we could init the NumVFs register to 0 */
>>> +       pci_write_config_word(dev, pos + PCI_SRIOV_NUM_VF, 0);
>>>         pci_read_config_word(dev, pos + PCI_SRIOV_VF_OFFSET, &offset);
>>>         pci_read_config_word(dev, pos + PCI_SRIOV_VF_STRIDE, &stride);
>>>         if (!offset || (total > 1 && !stride))
>
> Thanks
>
> Yinghai

Re: [PATCH] PCI: Init NumVFs register to zero in sriov_init()

[Bjorn Helgaas]

[+cc linux-pci]

On Wed, Nov 6, 2013 at 7:49 AM, ethan.zhao <ethan.kernel@gmail.com> wrote:
> Though no specification about NumVFs register initial value after POST, to void the confusion
> lspci output as following before VF was enabled, we should clear the NumVFs value left by BIOS
> to zero:
>
> $lspci -vvv -s 03:00.0
> Ethernet controller: Intel Corporation 82599EB 10-Gigabit SFI/SFP+ Network Connection (rev 01)
> ~
> Capabilities: [160 v1] Single Root I/O Virtualization (SR-IOV)
>                 IOVCap: Migration-, Interrupt Message Number: 000
>                 IOVCtl: Enable+ Migration- Interrupt- MSE+ ARIHierarchy+
>                 IOVSta: Migration-
>                 Initial VFs: 64, Total VFs: 64, Number of VFs: 64, Function Dependency Link: 00
>                                                               ^dazed !
> ~
> Signed-off-by: ethan.zhao <ethan.kernel@gmail.com>
> ---
>  drivers/pci/iov.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index de8ffac..a4941ad 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -439,6 +439,8 @@ static int sriov_init(struct pci_dev *dev, int pos)
>
>  found:
>         pci_write_config_word(dev, pos + PCI_SRIOV_CTRL, ctrl);
> +       /* VF Enable is cleared, so we could init the NumVFs register to 0 */
> +       pci_write_config_word(dev, pos + PCI_SRIOV_NUM_VF, 0);
>         pci_read_config_word(dev, pos + PCI_SRIOV_VF_OFFSET, &offset);
>         pci_read_config_word(dev, pos + PCI_SRIOV_VF_STRIDE, &stride);
>         if (!offset || (total > 1 && !stride))

I'm not 100% sure we should always disable SR-IOV when we enumerate a
PF, but as long as we do, it seems OK to me to clear NumVFs at the
same time.

We already clear NumVFs at other places where we disable SR-IOV
(sriov_enable() failure path and sriov_disable()).

Bjorn

Re: [PATCH] PCI: Init NumVFs register to zero in sriov_init()

[Bjorn Helgaas]

On Tue, Nov 19, 2013 at 3:24 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> [+cc linux-pci]
>
> On Wed, Nov 6, 2013 at 7:49 AM, ethan.zhao <ethan.kernel@gmail.com> wrote:
>> Though no specification about NumVFs register initial value after POST, to void the confusion
>> lspci output as following before VF was enabled, we should clear the NumVFs value left by BIOS
>> to zero:
>>
>> $lspci -vvv -s 03:00.0
>> Ethernet controller: Intel Corporation 82599EB 10-Gigabit SFI/SFP+ Network Connection (rev 01)
>> ~
>> Capabilities: [160 v1] Single Root I/O Virtualization (SR-IOV)
>>                 IOVCap: Migration-, Interrupt Message Number: 000
>>                 IOVCtl: Enable+ Migration- Interrupt- MSE+ ARIHierarchy+
>>                 IOVSta: Migration-
>>                 Initial VFs: 64, Total VFs: 64, Number of VFs: 64, Function Dependency Link: 00
>>                                                               ^dazed !

Did you mean to show lspci output from before SR-IOV was enabled?  It
looks like SR-IOV is enabled here, so I don't think your patch would
change this output at all.

Bjorn

Re: [PATCH] PCI: Init NumVFs register to zero in sriov_init()

[Ethan Zhao]

Bjorn,
    To be honest, the output above is picked up from other guy's mail,
because I composed the patch mail at home and left the real bug
material at company, they 'really' hit an issue confused by lspci
output while SR-IOV is initialized to disabled .
   There is an error in above output as description that not made on
purpose, sorry again, the
    IOVCtl: Enable+
   should be
    IOVCtl: Enable-

Current iov.c code, in fact, initializing the SR-IOV to disabled whatever BIOS
setup the PCI_SRIOV_XX registers.

Thanks,
Ethan


On Wed, Nov 20, 2013 at 8:07 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Tue, Nov 19, 2013 at 3:24 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> [+cc linux-pci]
>>
>> On Wed, Nov 6, 2013 at 7:49 AM, ethan.zhao <ethan.kernel@gmail.com> wrote:
>>> Though no specification about NumVFs register initial value after POST, to void the confusion
>>> lspci output as following before VF was enabled, we should clear the NumVFs value left by BIOS
>>> to zero:
>>>
>>> $lspci -vvv -s 03:00.0
>>> Ethernet controller: Intel Corporation 82599EB 10-Gigabit SFI/SFP+ Network Connection (rev 01)
>>> ~
>>> Capabilities: [160 v1] Single Root I/O Virtualization (SR-IOV)
>>>                 IOVCap: Migration-, Interrupt Message Number: 000
>>>                 IOVCtl: Enable+ Migration- Interrupt- MSE+ ARIHierarchy+
>>>                 IOVSta: Migration-
>>>                 Initial VFs: 64, Total VFs: 64, Number of VFs: 64, Function Dependency Link: 00
>>>                                                               ^dazed !
>
> Did you mean to show lspci output from before SR-IOV was enabled?  It
> looks like SR-IOV is enabled here, so I don't think your patch would
> change this output at all.
>
> Bjorn

Re: [PATCH] PCI: Init NumVFs register to zero in sriov_init()

[Ethan Zhao]

Bjorn,
    I revised the description part with the original bug material as
below, help to take a look, will send V2 back to home, SMTP blocked by
company network.

    PCI: Init NumVFs register to zero in sriov_init()

    Though no specification about NumVFs register initial value after
POST, to void the confusion
    lspci output as following before VF was enabled, we should clear
the NumVFs value left by BIOS
    to zero:

    #lspci -vvvxxx -s 13:00.0
   13:00.0 Ethernet controller: Intel Corporation 82599EB 10-Gigabit SFI/SFP+
    Network Connection (rev 01)  Subsystem: Oracle/SUN Ethernet Server
Adapter X520-2
    ~
    Capabilities: [160 v1] Single Root I/O Virtualization (SR-IOV)
            IOVCap: Migration-, Interrupt Message Number: 000
            IOVCtl: Enable- Migration- Interrupt- MSE- ARIHierarchy+
            IOVSta: Migration-
            Initial VFs: 64, Total VFs: 64, Number of VFs: 64, Function
            Dependency Link: 00
                                                                     ^dazed !
    140: 03 00 01 15 c4 24 c8 ff ff 21 1b 00 00 00 00 00
    150: 0e 00 01 16 00 01 00 00 00 00 00 00 00 00 00 00
    160: 10 00 01 00 00 00 00 00 10 00 00 00 40 00 40 00

          ^        ^Total VFs

          Initial VFs
    170: 40 00 00 00 80 00 02 00 00 00 ed 10 53 05 00 00
             ^Number of VFs


Thanks,
Ethan


On Wed, Nov 20, 2013 at 8:07 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Tue, Nov 19, 2013 at 3:24 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> [+cc linux-pci]
>>
>> On Wed, Nov 6, 2013 at 7:49 AM, ethan.zhao <ethan.kernel@gmail.com> wrote:
>>> Though no specification about NumVFs register initial value after POST, to void the confusion
>>> lspci output as following before VF was enabled, we should clear the NumVFs value left by BIOS
>>> to zero:
>>>
>>> $lspci -vvv -s 03:00.0
>>> Ethernet controller: Intel Corporation 82599EB 10-Gigabit SFI/SFP+ Network Connection (rev 01)
>>> ~
>>> Capabilities: [160 v1] Single Root I/O Virtualization (SR-IOV)
>>>                 IOVCap: Migration-, Interrupt Message Number: 000
>>>                 IOVCtl: Enable+ Migration- Interrupt- MSE+ ARIHierarchy+
>>>                 IOVSta: Migration-
>>>                 Initial VFs: 64, Total VFs: 64, Number of VFs: 64, Function Dependency Link: 00
>>>                                                               ^dazed !
>
> Did you mean to show lspci output from before SR-IOV was enabled?  It
> looks like SR-IOV is enabled here, so I don't think your patch would
> change this output at all.
>
> Bjorn

Re: [PATCH] PCI: Init NumVFs register to zero in sriov_init()

[Bjorn Helgaas]

On Wed, Nov 6, 2013 at 7:49 AM, ethan.zhao <ethan.kernel@gmail.com> wrote:
> Though no specification about NumVFs register initial value after POST, to void the confusion
> lspci output as following before VF was enabled, we should clear the NumVFs value left by BIOS
> to zero:
>
> $lspci -vvv -s 03:00.0
> Ethernet controller: Intel Corporation 82599EB 10-Gigabit SFI/SFP+ Network Connection (rev 01)
> ~
> Capabilities: [160 v1] Single Root I/O Virtualization (SR-IOV)
>                 IOVCap: Migration-, Interrupt Message Number: 000
>                 IOVCtl: Enable+ Migration- Interrupt- MSE+ ARIHierarchy+
>                 IOVSta: Migration-
>                 Initial VFs: 64, Total VFs: 64, Number of VFs: 64, Function Dependency Link: 00
>                                                               ^dazed !
> ~
> Signed-off-by: ethan.zhao <ethan.kernel@gmail.com>

Applied to my pci/misc branch for v3.14, thanks!

Bjorn

> ---
>  drivers/pci/iov.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index de8ffac..a4941ad 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -439,6 +439,8 @@ static int sriov_init(struct pci_dev *dev, int pos)
>
>  found:
>         pci_write_config_word(dev, pos + PCI_SRIOV_CTRL, ctrl);
> +       /* VF Enable is cleared, so we could init the NumVFs register to 0 */
> +       pci_write_config_word(dev, pos + PCI_SRIOV_NUM_VF, 0);
>         pci_read_config_word(dev, pos + PCI_SRIOV_VF_OFFSET, &offset);
>         pci_read_config_word(dev, pos + PCI_SRIOV_VF_STRIDE, &stride);
>         if (!offset || (total > 1 && !stride))
> --
> 1.8.3.4 (Apple Git-47)
>