[PATCH 0/3] pci: Fix ARI next function numbers

[PATCH 0/3] pci: Fix ARI next function numbers

[Akihiko Odaki]

The ARI next function number field is undefined for VF. The PF should
end the linked list formed with the field by specifying 0.

Supersedes: <20230701070133.24877-1-akihiko.odaki@daynix.com>
("[PATCH 0/4] pci: Compare function number and ARI next function number")

Akihiko Odaki (3):
  docs: Fix next function numbers in SR/IOV documentation
  hw/nvme: Fix ARI next function numbers
  igb: Fix ARI next function numbers

 docs/pcie_sriov.txt | 4 ++--
 hw/net/igb.c        | 2 +-
 hw/nvme/ctrl.c      | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

-- 
2.41.0

[PATCH 1/3] docs: Fix next function numbers in SR/IOV documentation

[Akihiko Odaki]

The ARI next function number field is undefined for VF so the PF should
end the linked list formed with the field by specifying 0.

This also changes the value of the field for VF; it seems to imply the
value has some meaning if it differs from one of the PF, but it doesn't.

Fixes: 2503461691 ("pcie: Add some SR/IOV API documentation in docs/pcie_sriov.txt")
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 docs/pcie_sriov.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/docs/pcie_sriov.txt b/docs/pcie_sriov.txt
index 7eff7f2703..2b7094dc47 100644
--- a/docs/pcie_sriov.txt
+++ b/docs/pcie_sriov.txt
@@ -48,7 +48,7 @@ setting up a BAR for a VF.
       ...
       int ret = pcie_endpoint_cap_init(d, 0x70);
       ...
-      pcie_ari_init(d, 0x100, 1);
+      pcie_ari_init(d, 0x100, 0);
       ...
 
       /* Add and initialize the SR/IOV capability */
@@ -78,7 +78,7 @@ setting up a BAR for a VF.
       ...
       int ret = pcie_endpoint_cap_init(d, 0x60);
       ...
-      pcie_ari_init(d, 0x100, 1);
+      pcie_ari_init(d, 0x100, 0);
       ...
       memory_region_init(mr, ... )
       pcie_sriov_vf_register_bar(d, bar_nr, mr);
-- 
2.41.0

[PATCH 2/3] hw/nvme: Fix ARI next function numbers

[Akihiko Odaki]

The ARI next function number field is undefined for VF. The PF should
end the linked list formed with the field by specifying 0.

Fixes: 44c2c09488 ("hw/nvme: Add support for SR-IOV")
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 hw/nvme/ctrl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index fd917fcda1..6c4809b4f6 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -8088,7 +8088,7 @@ static bool nvme_init_pci(NvmeCtrl *n, PCIDevice *pci_dev, Error **errp)
     pcie_endpoint_cap_init(pci_dev, 0x80);
     pcie_cap_flr_init(pci_dev);
     if (n->params.sriov_max_vfs) {
-        pcie_ari_init(pci_dev, 0x100, 1);
+        pcie_ari_init(pci_dev, 0x100, 0);
     }
 
     /* add one to max_ioqpairs to account for the admin queue pair */
-- 
2.41.0

[PATCH 3/3] igb: Fix ARI next function numbers

[Akihiko Odaki]

The ARI next function number field is undefined for VF so the PF should
end the linked list formed with the field by specifying 0.

Fixes: 3a977deebe ("Intrdocue igb device emulation")
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 hw/net/igb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/igb.c b/hw/net/igb.c
index 1c989d7677..897386fc09 100644
--- a/hw/net/igb.c
+++ b/hw/net/igb.c
@@ -431,7 +431,7 @@ static void igb_pci_realize(PCIDevice *pci_dev, Error **errp)
         hw_error("Failed to initialize AER capability");
     }
 
-    pcie_ari_init(pci_dev, 0x150, 1);
+    pcie_ari_init(pci_dev, 0x150, 0);
 
     pcie_sriov_pf_init(pci_dev, IGB_CAP_SRIOV_OFFSET, TYPE_IGBVF,
         IGB_82576_VF_DEV_ID, IGB_MAX_VF_FUNCTIONS, IGB_MAX_VF_FUNCTIONS,
-- 
2.41.0

Re: [PATCH 3/3] igb: Fix ARI next function numbers

[Michael S. Tsirkin]

On Sun, Jul 02, 2023 at 05:33:56PM +0900, Akihiko Odaki wrote:
> The ARI next function number field is undefined for VF so the PF should
> end the linked list formed with the field by specifying 0.
> 
> Fixes: 3a977deebe ("Intrdocue igb device emulation")
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>


I would also change it for the VF just so people don't wonder
what's the magic value. Do document in commit log though.

Maybe just drop this parameter from pcie_ari_init completely
for now?


> ---
>  hw/net/igb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/net/igb.c b/hw/net/igb.c
> index 1c989d7677..897386fc09 100644
> --- a/hw/net/igb.c
> +++ b/hw/net/igb.c
> @@ -431,7 +431,7 @@ static void igb_pci_realize(PCIDevice *pci_dev, Error **errp)
>          hw_error("Failed to initialize AER capability");
>      }
>  
> -    pcie_ari_init(pci_dev, 0x150, 1);
> +    pcie_ari_init(pci_dev, 0x150, 0);
>  
>      pcie_sriov_pf_init(pci_dev, IGB_CAP_SRIOV_OFFSET, TYPE_IGBVF,
>          IGB_82576_VF_DEV_ID, IGB_MAX_VF_FUNCTIONS, IGB_MAX_VF_FUNCTIONS,
> -- 
> 2.41.0

Re: [PATCH 3/3] igb: Fix ARI next function numbers

[Akihiko Odaki]

On 2023/07/02 18:00, Michael S. Tsirkin wrote:
> On Sun, Jul 02, 2023 at 05:33:56PM +0900, Akihiko Odaki wrote:
>> The ARI next function number field is undefined for VF so the PF should
>> end the linked list formed with the field by specifying 0.
>>
>> Fixes: 3a977deebe ("Intrdocue igb device emulation")
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> 
> 
> I would also change it for the VF just so people don't wonder
> what's the magic value. Do document in commit log though.
> 
> Maybe just drop this parameter from pcie_ari_init completely
> for now?

I sent v2, but it doesn't change the field for VFs either to save code 
for migration. The parameter for pcie_ari_init() also remains to migrate 
from older versions.

> 
> 
>> ---
>>   hw/net/igb.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/net/igb.c b/hw/net/igb.c
>> index 1c989d7677..897386fc09 100644
>> --- a/hw/net/igb.c
>> +++ b/hw/net/igb.c
>> @@ -431,7 +431,7 @@ static void igb_pci_realize(PCIDevice *pci_dev, Error **errp)
>>           hw_error("Failed to initialize AER capability");
>>       }
>>   
>> -    pcie_ari_init(pci_dev, 0x150, 1);
>> +    pcie_ari_init(pci_dev, 0x150, 0);
>>   
>>       pcie_sriov_pf_init(pci_dev, IGB_CAP_SRIOV_OFFSET, TYPE_IGBVF,
>>           IGB_82576_VF_DEV_ID, IGB_MAX_VF_FUNCTIONS, IGB_MAX_VF_FUNCTIONS,
>> -- 
>> 2.41.0
> 

Re: [PATCH 3/3] igb: Fix ARI next function numbers

[Michael S. Tsirkin]

On Sun, Jul 02, 2023 at 06:49:50PM +0900, Akihiko Odaki wrote:
> On 2023/07/02 18:00, Michael S. Tsirkin wrote:
> > On Sun, Jul 02, 2023 at 05:33:56PM +0900, Akihiko Odaki wrote:
> > > The ARI next function number field is undefined for VF so the PF should
> > > end the linked list formed with the field by specifying 0.
> > > 
> > > Fixes: 3a977deebe ("Intrdocue igb device emulation")
> > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > 
> > 
> > I would also change it for the VF just so people don't wonder
> > what's the magic value. Do document in commit log though.
> > 
> > Maybe just drop this parameter from pcie_ari_init completely
> > for now?
> 
> I sent v2, but it doesn't change the field for VFs either to save code for
> migration. The parameter for pcie_ari_init() also remains to migrate from
> older versions.

For migration, stick the boolean in PCIDevice::cap_present. Use QEMU_PCIE_ERR_UNC_MASK_BITNR as
a template.


> > 
> > 
> > > ---
> > >   hw/net/igb.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/net/igb.c b/hw/net/igb.c
> > > index 1c989d7677..897386fc09 100644
> > > --- a/hw/net/igb.c
> > > +++ b/hw/net/igb.c
> > > @@ -431,7 +431,7 @@ static void igb_pci_realize(PCIDevice *pci_dev, Error **errp)
> > >           hw_error("Failed to initialize AER capability");
> > >       }
> > > -    pcie_ari_init(pci_dev, 0x150, 1);
> > > +    pcie_ari_init(pci_dev, 0x150, 0);
> > >       pcie_sriov_pf_init(pci_dev, IGB_CAP_SRIOV_OFFSET, TYPE_IGBVF,
> > >           IGB_82576_VF_DEV_ID, IGB_MAX_VF_FUNCTIONS, IGB_MAX_VF_FUNCTIONS,
> > > -- 
> > > 2.41.0
> > 

Re: [PATCH 0/3] pci: Fix ARI next function numbers

[Michael S. Tsirkin]

On Sun, Jul 02, 2023 at 05:33:53PM +0900, Akihiko Odaki wrote:
> The ARI next function number field is undefined for VF. The PF should
> end the linked list formed with the field by specifying 0.
> 
> Supersedes: <20230701070133.24877-1-akihiko.odaki@daynix.com>
> ("[PATCH 0/4] pci: Compare function number and ARI next function number")

changelog?

motivation?

effect on migration?

testing?


> Akihiko Odaki (3):
>   docs: Fix next function numbers in SR/IOV documentation
>   hw/nvme: Fix ARI next function numbers
>   igb: Fix ARI next function numbers
> 
>  docs/pcie_sriov.txt | 4 ++--
>  hw/net/igb.c        | 2 +-
>  hw/nvme/ctrl.c      | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> -- 
> 2.41.0

Re: [PATCH 0/3] pci: Fix ARI next function numbers

[Ani Sinha]

> On 02-Jul-2023, at 2:03 PM, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> 
> The ARI next function number field is undefined for VF. The PF should
> end the linked list formed with the field by specifying 0.
> 
> Supersedes: <20230701070133.24877-1-akihiko.odaki@daynix.com>
> ("[PATCH 0/4] pci: Compare function number and ARI next function number")

Normally we do this using patch version numbers because otherwise over email, its hard to track the various iterations of the patches trying to solve the same thing.

> 
> Akihiko Odaki (3):
>  docs: Fix next function numbers in SR/IOV documentation
>  hw/nvme: Fix ARI next function numbers
>  igb: Fix ARI next function numbers
> 
> docs/pcie_sriov.txt | 4 ++--
> hw/net/igb.c        | 2 +-
> hw/nvme/ctrl.c      | 2 +-
> 3 files changed, 4 insertions(+), 4 deletions(-)
> 
> -- 
> 2.41.0
>