[PATCH v2 0/4] pcie: Fix ARI next function numbers

[PATCH v2 0/4] pcie: 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")

V1 -> V2:
  Fixed migration. (Michael S. Tsirkin)
  Added a caveat comment. (Michael S. Tsirkin)

Akihiko Odaki (4):
  docs: Fix next function numbers in SR/IOV documentation
  hw/nvme: Fix ARI next function numbers
  igb: Fix ARI next function numbers
  pcie: Note a caveat regarding ARI next function number

 docs/pcie_sriov.txt   | 4 ++--
 hw/nvme/nvme.h        | 1 +
 include/hw/pci/pcie.h | 7 +++++++
 hw/core/machine.c     | 2 ++
 hw/net/igb.c          | 5 ++++-
 hw/nvme/ctrl.c        | 3 ++-
 6 files changed, 18 insertions(+), 4 deletions(-)

-- 
2.41.0

[PATCH v2 1/4] 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

Re: [PATCH v2 1/4] docs: Fix next function numbers in SR/IOV documentation

[Michael S. Tsirkin]

On Sun, Jul 02, 2023 at 06:46:25PM +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.
> 
> 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);


So now code does not match docs.


> -- 
> 2.41.0

Re: [PATCH v2 1/4] docs: Fix next function numbers in SR/IOV documentation

[Akihiko Odaki]

On 2023/07/02 19:40, Michael S. Tsirkin wrote:
> On Sun, Jul 02, 2023 at 06:46:25PM +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.
>>
>> 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);
> 
> 
> So now code does not match docs.

Can you elaborate more?

Re: [PATCH v2 1/4] docs: Fix next function numbers in SR/IOV documentation

[Michael S. Tsirkin]

On Sun, Jul 02, 2023 at 08:19:43PM +0900, Akihiko Odaki wrote:
> On 2023/07/02 19:40, Michael S. Tsirkin wrote:
> > On Sun, Jul 02, 2023 at 06:46:25PM +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.
> > > 
> > > 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);
> > 
> > 
> > So now code does not match docs.
> 
> Can you elaborate more?

your new revision addresses this.

-- 
MST

[PATCH v2 2/4] 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/nvme.h    | 1 +
 hw/core/machine.c | 1 +
 hw/nvme/ctrl.c    | 3 ++-
 3 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h
index 209e8f5b4c..c2ba6755ab 100644
--- a/hw/nvme/nvme.h
+++ b/hw/nvme/nvme.h
@@ -515,6 +515,7 @@ typedef struct NvmeParams {
     uint16_t sriov_vi_flexible;
     uint8_t  sriov_max_vq_per_vf;
     uint8_t  sriov_max_vi_per_vf;
+    bool     ari_nextfn_1;
 } NvmeParams;
 
 typedef struct NvmeCtrl {
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 46f8f9a2b0..f984a767a2 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -41,6 +41,7 @@
 
 GlobalProperty hw_compat_8_0[] = {
     { "migration", "multifd-flush-after-each-section", "on"},
+    { "nvme", "ari-nextfn-1", "on"},
 };
 const size_t hw_compat_8_0_len = G_N_ELEMENTS(hw_compat_8_0);
 
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index fd917fcda1..4bbafc66b5 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, n->params.ari_nextfn_1 ? 1 : 0);
     }
 
     /* add one to max_ioqpairs to account for the admin queue pair */
@@ -8406,6 +8406,7 @@ static Property nvme_props[] = {
                       params.sriov_max_vi_per_vf, 0),
     DEFINE_PROP_UINT8("sriov_max_vq_per_vf", NvmeCtrl,
                       params.sriov_max_vq_per_vf, 0),
+    DEFINE_PROP_BOOL("ari-nextfn-1", NvmeCtrl, params.ari_nextfn_1, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.41.0

[PATCH v2 3/4] 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/core/machine.c | 3 ++-
 hw/net/igb.c      | 5 ++++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index f984a767a2..1f5aacd1dc 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -41,7 +41,8 @@
 
 GlobalProperty hw_compat_8_0[] = {
     { "migration", "multifd-flush-after-each-section", "on"},
-    { "nvme", "ari-nextfn-1", "on"},
+    { "igb", "ari-nextfn-1", "on" },
+    { "nvme", "ari-nextfn-1", "on" },
 };
 const size_t hw_compat_8_0_len = G_N_ELEMENTS(hw_compat_8_0);
 
diff --git a/hw/net/igb.c b/hw/net/igb.c
index 1c989d7677..d37d43c155 100644
--- a/hw/net/igb.c
+++ b/hw/net/igb.c
@@ -78,6 +78,8 @@ struct IGBState {
     uint32_t ioaddr;
 
     IGBCore core;
+
+    bool ari_nextfn_1;
 };
 
 #define IGB_CAP_SRIOV_OFFSET    (0x160)
@@ -431,7 +433,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, s->ari_nextfn_1 ? 1 : 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,
@@ -582,6 +584,7 @@ static const VMStateDescription igb_vmstate = {
 
 static Property igb_properties[] = {
     DEFINE_NIC_PROPERTIES(IGBState, conf),
+    DEFINE_PROP_BOOL("ari-nextfn-1", IGBState, ari_nextfn_1, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.41.0

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

[Michael S. Tsirkin]

On Sun, Jul 02, 2023 at 06:46:27PM +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>
> ---
>  hw/core/machine.c | 3 ++-
>  hw/net/igb.c      | 5 ++++-
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index f984a767a2..1f5aacd1dc 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -41,7 +41,8 @@
>  
>  GlobalProperty hw_compat_8_0[] = {
>      { "migration", "multifd-flush-after-each-section", "on"},
> -    { "nvme", "ari-nextfn-1", "on"},
> +    { "igb", "ari-nextfn-1", "on" },
> +    { "nvme", "ari-nextfn-1", "on" },
>  };
>  const size_t hw_compat_8_0_len = G_N_ELEMENTS(hw_compat_8_0);
>  
> diff --git a/hw/net/igb.c b/hw/net/igb.c
> index 1c989d7677..d37d43c155 100644
> --- a/hw/net/igb.c
> +++ b/hw/net/igb.c
> @@ -78,6 +78,8 @@ struct IGBState {
>      uint32_t ioaddr;
>  
>      IGBCore core;
> +
> +    bool ari_nextfn_1;

Document this field please, explaining why it's there.

>  };
>  
>  #define IGB_CAP_SRIOV_OFFSET    (0x160)
> @@ -431,7 +433,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, s->ari_nextfn_1 ? 1 : 0);

Why don't we move the logic to pci core, and drop code duplication
completely?

>  
>      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,
> @@ -582,6 +584,7 @@ static const VMStateDescription igb_vmstate = {
>  
>  static Property igb_properties[] = {
>      DEFINE_NIC_PROPERTIES(IGBState, conf),
> +    DEFINE_PROP_BOOL("ari-nextfn-1", IGBState, ari_nextfn_1, false),
>      DEFINE_PROP_END_OF_LIST(),
>  };

This really needs to be a non stable property (prefix with "x-").



> -- 
> 2.41.0

[PATCH v2 4/4] pcie: Note a caveat regarding ARI next function number

[Akihiko Odaki]

Suggested-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 include/hw/pci/pcie.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
index 3cc2b15957..16860e2216 100644
--- a/include/hw/pci/pcie.h
+++ b/include/hw/pci/pcie.h
@@ -134,7 +134,14 @@ void pcie_sync_bridge_lnk(PCIDevice *dev);
 void pcie_acs_init(PCIDevice *dev, uint16_t offset);
 void pcie_acs_reset(PCIDevice *dev);
 
+/*
+ * Note: for non-VFs, nextfn must be the Function Number of the next higher
+ * numbered Function in the Device, or 00h if there are no higher numbered
+ * Functions.
+ * TODO: validate this.
+ */
 void pcie_ari_init(PCIDevice *dev, uint16_t offset, uint16_t nextfn);
+
 void pcie_dev_ser_num_init(PCIDevice *dev, uint16_t offset, uint64_t ser_num);
 void pcie_ats_init(PCIDevice *dev, uint16_t offset, bool aligned);
 
-- 
2.41.0