1. Patchew
  2. QEMU
  3. [PATCH 0/4] pci: Compare function number and ARI next function number
  4. View patch

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

Akihiko Odaki posted 4 patches 2 years, 5 months ago
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Akihiko Odaki <akihiko.odaki@daynix.com>, Sriram Yagnaraman <sriram.yagnaraman@est.tech>, Jason Wang <jasowang@redhat.com>, Keith Busch <kbusch@kernel.org>, Klaus Jensen <its@irrelevant.dk>
There is a newer version of this series
[PATCH 3/4] igb: Fix ARI next function numbers
Posted by Akihiko Odaki 2 years, 5 months ago 
The next function numbers are expected to form a linked list ending with
0.

Fixes: 3a977deebe ("Intrdocue igb device emulation")
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 hw/net/igb_core.h | 3 +++
 hw/net/igb.c      | 4 +---
 hw/net/igbvf.c    | 5 ++++-
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/hw/net/igb_core.h b/hw/net/igb_core.h
index 9cbbfd516b..e1dab76995 100644
--- a/hw/net/igb_core.h
+++ b/hw/net/igb_core.h
@@ -49,6 +49,9 @@
 #define IGB_NUM_QUEUES          (16)
 #define IGB_NUM_VM_POOLS        (8)
 
+#define IGB_VF_OFFSET           (0x80)
+#define IGB_VF_STRIDE           (2)
+
 typedef struct IGBCore IGBCore;
 
 enum { PHY_R = BIT(0),
diff --git a/hw/net/igb.c b/hw/net/igb.c
index 1c989d7677..543ca0114a 100644
--- a/hw/net/igb.c
+++ b/hw/net/igb.c
@@ -81,8 +81,6 @@ struct IGBState {
 };
 
 #define IGB_CAP_SRIOV_OFFSET    (0x160)
-#define IGB_VF_OFFSET           (0x80)
-#define IGB_VF_STRIDE           (2)
 
 #define E1000E_MMIO_IDX     0
 #define E1000E_FLASH_IDX    1
@@ -431,7 +429,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, IGB_VF_OFFSET);
 
     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,
diff --git a/hw/net/igbvf.c b/hw/net/igbvf.c
index 284ea61184..bf2f237ab5 100644
--- a/hw/net/igbvf.c
+++ b/hw/net/igbvf.c
@@ -240,6 +240,9 @@ static const MemoryRegionOps mmio_ops = {
 static void igbvf_pci_realize(PCIDevice *dev, Error **errp)
 {
     IgbVfState *s = IGBVF(dev);
+    uint16_t nextvfn = pcie_sriov_vf_number(dev) + 1;
+    uint16_t nextfn = nextvfn < IGB_MAX_VF_FUNCTIONS ?
+                      IGB_VF_OFFSET + nextvfn * IGB_VF_STRIDE : 0;
     int ret;
     int i;
 
@@ -270,7 +273,7 @@ static void igbvf_pci_realize(PCIDevice *dev, Error **errp)
         hw_error("Failed to initialize AER capability");
     }
 
-    pcie_ari_init(dev, 0x150, 1);
+    pcie_ari_init(dev, 0x150, nextfn);
 }
 
 static void igbvf_pci_uninit(PCIDevice *dev)
-- 
2.41.0
Re: [PATCH 3/4] igb: Fix ARI next function numbers
Posted by Michael S. Tsirkin 2 years, 5 months ago 
On Sat, Jul 01, 2023 at 04:01:21PM +0900, Akihiko Odaki wrote:
> The next function numbers are expected to form a linked list ending with
> 0.
> 
> Fixes: 3a977deebe ("Intrdocue igb device emulation")
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  hw/net/igb_core.h | 3 +++
>  hw/net/igb.c      | 4 +---
>  hw/net/igbvf.c    | 5 ++++-
>  3 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/net/igb_core.h b/hw/net/igb_core.h
> index 9cbbfd516b..e1dab76995 100644
> --- a/hw/net/igb_core.h
> +++ b/hw/net/igb_core.h
> @@ -49,6 +49,9 @@
>  #define IGB_NUM_QUEUES          (16)
>  #define IGB_NUM_VM_POOLS        (8)
>  
> +#define IGB_VF_OFFSET           (0x80)
> +#define IGB_VF_STRIDE           (2)
> +
>  typedef struct IGBCore IGBCore;
>  
>  enum { PHY_R = BIT(0),
> diff --git a/hw/net/igb.c b/hw/net/igb.c
> index 1c989d7677..543ca0114a 100644
> --- a/hw/net/igb.c
> +++ b/hw/net/igb.c
> @@ -81,8 +81,6 @@ struct IGBState {
>  };
>  
>  #define IGB_CAP_SRIOV_OFFSET    (0x160)
> -#define IGB_VF_OFFSET           (0x80)
> -#define IGB_VF_STRIDE           (2)
>  
>  #define E1000E_MMIO_IDX     0
>  #define E1000E_FLASH_IDX    1
> @@ -431,7 +429,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, IGB_VF_OFFSET);
>  
>      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,


I think this change would break migrations from 8.0. No?


More importantly your commit log says linked list should end
with 0, but you make it point at a VF instead.


> diff --git a/hw/net/igbvf.c b/hw/net/igbvf.c
> index 284ea61184..bf2f237ab5 100644
> --- a/hw/net/igbvf.c
> +++ b/hw/net/igbvf.c
> @@ -240,6 +240,9 @@ static const MemoryRegionOps mmio_ops = {
>  static void igbvf_pci_realize(PCIDevice *dev, Error **errp)
>  {
>      IgbVfState *s = IGBVF(dev);
> +    uint16_t nextvfn = pcie_sriov_vf_number(dev) + 1;
> +    uint16_t nextfn = nextvfn < IGB_MAX_VF_FUNCTIONS ?
> +                      IGB_VF_OFFSET + nextvfn * IGB_VF_STRIDE : 0;
>      int ret;
>      int i;
>  
> @@ -270,7 +273,7 @@ static void igbvf_pci_realize(PCIDevice *dev, Error **errp)
>          hw_error("Failed to initialize AER capability");
>      }
>  
> -    pcie_ari_init(dev, 0x150, 1);
> +    pcie_ari_init(dev, 0x150, nextfn);



For this one I don't see why it matters at all:

The presence of Shadow Functions does not affect this field.
For VFs, this field is undefined since VFs are located using First VF Offset (see § Section 9.3.3.9 ) and VF
Stride (see § Section 9.3.3.10 ).




>  }
>  
>  static void igbvf_pci_uninit(PCIDevice *dev)
> -- 
> 2.41.0
Re: [PATCH 3/4] igb: Fix ARI next function numbers
Posted by Akihiko Odaki 2 years, 5 months ago 
On 2023/07/02 14:05, Michael S. Tsirkin wrote:
> On Sat, Jul 01, 2023 at 04:01:21PM +0900, Akihiko Odaki wrote:
>> The next function numbers are expected to form a linked list ending with
>> 0.
>>
>> Fixes: 3a977deebe ("Intrdocue igb device emulation")
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>> ---
>>   hw/net/igb_core.h | 3 +++
>>   hw/net/igb.c      | 4 +---
>>   hw/net/igbvf.c    | 5 ++++-
>>   3 files changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/net/igb_core.h b/hw/net/igb_core.h
>> index 9cbbfd516b..e1dab76995 100644
>> --- a/hw/net/igb_core.h
>> +++ b/hw/net/igb_core.h
>> @@ -49,6 +49,9 @@
>>   #define IGB_NUM_QUEUES          (16)
>>   #define IGB_NUM_VM_POOLS        (8)
>>   
>> +#define IGB_VF_OFFSET           (0x80)
>> +#define IGB_VF_STRIDE           (2)
>> +
>>   typedef struct IGBCore IGBCore;
>>   
>>   enum { PHY_R = BIT(0),
>> diff --git a/hw/net/igb.c b/hw/net/igb.c
>> index 1c989d7677..543ca0114a 100644
>> --- a/hw/net/igb.c
>> +++ b/hw/net/igb.c
>> @@ -81,8 +81,6 @@ struct IGBState {
>>   };
>>   
>>   #define IGB_CAP_SRIOV_OFFSET    (0x160)
>> -#define IGB_VF_OFFSET           (0x80)
>> -#define IGB_VF_STRIDE           (2)
>>   
>>   #define E1000E_MMIO_IDX     0
>>   #define E1000E_FLASH_IDX    1
>> @@ -431,7 +429,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, IGB_VF_OFFSET);
>>   
>>       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,
> 
> 
> I think this change would break migrations from 8.0. No?

Well, I don't have a reason to concern more for this change than other 
bug fixes with behavioral changes observable from the guest.

> 
> 
> More importantly your commit log says linked list should end
> with 0, but you make it point at a VF instead.
> 
> 
>> diff --git a/hw/net/igbvf.c b/hw/net/igbvf.c
>> index 284ea61184..bf2f237ab5 100644
>> --- a/hw/net/igbvf.c
>> +++ b/hw/net/igbvf.c
>> @@ -240,6 +240,9 @@ static const MemoryRegionOps mmio_ops = {
>>   static void igbvf_pci_realize(PCIDevice *dev, Error **errp)
>>   {
>>       IgbVfState *s = IGBVF(dev);
>> +    uint16_t nextvfn = pcie_sriov_vf_number(dev) + 1;
>> +    uint16_t nextfn = nextvfn < IGB_MAX_VF_FUNCTIONS ?
>> +                      IGB_VF_OFFSET + nextvfn * IGB_VF_STRIDE : 0;
>>       int ret;
>>       int i;
>>   
>> @@ -270,7 +273,7 @@ static void igbvf_pci_realize(PCIDevice *dev, Error **errp)
>>           hw_error("Failed to initialize AER capability");
>>       }
>>   
>> -    pcie_ari_init(dev, 0x150, 1);
>> +    pcie_ari_init(dev, 0x150, nextfn);
> 
> 
> 
> For this one I don't see why it matters at all:
> 
> The presence of Shadow Functions does not affect this field.
> For VFs, this field is undefined since VFs are located using First VF Offset (see § Section 9.3.3.9 ) and VF
> Stride (see § Section 9.3.3.10 ).

I missed the statements saying the field is undefined for VFs. I posted 
an alternative series ("[PATCH 0/3] pci: Fix ARI next function numbers") 
so please review it.

> 
> 
> 
> 
>>   }
>>   
>>   static void igbvf_pci_uninit(PCIDevice *dev)
>> -- 
>> 2.41.0
>
Re: [PATCH 3/4] igb: Fix ARI next function numbers
Posted by Michael S. Tsirkin 2 years, 5 months ago 
On Sun, Jul 02, 2023 at 05:38:42PM +0900, Akihiko Odaki wrote:
> On 2023/07/02 14:05, Michael S. Tsirkin wrote:
> > On Sat, Jul 01, 2023 at 04:01:21PM +0900, Akihiko Odaki wrote:
> > > The next function numbers are expected to form a linked list ending with
> > > 0.
> > > 
> > > Fixes: 3a977deebe ("Intrdocue igb device emulation")
> > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > > ---
> > >   hw/net/igb_core.h | 3 +++
> > >   hw/net/igb.c      | 4 +---
> > >   hw/net/igbvf.c    | 5 ++++-
> > >   3 files changed, 8 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/hw/net/igb_core.h b/hw/net/igb_core.h
> > > index 9cbbfd516b..e1dab76995 100644
> > > --- a/hw/net/igb_core.h
> > > +++ b/hw/net/igb_core.h
> > > @@ -49,6 +49,9 @@
> > >   #define IGB_NUM_QUEUES          (16)
> > >   #define IGB_NUM_VM_POOLS        (8)
> > > +#define IGB_VF_OFFSET           (0x80)
> > > +#define IGB_VF_STRIDE           (2)
> > > +
> > >   typedef struct IGBCore IGBCore;
> > >   enum { PHY_R = BIT(0),
> > > diff --git a/hw/net/igb.c b/hw/net/igb.c
> > > index 1c989d7677..543ca0114a 100644
> > > --- a/hw/net/igb.c
> > > +++ b/hw/net/igb.c
> > > @@ -81,8 +81,6 @@ struct IGBState {
> > >   };
> > >   #define IGB_CAP_SRIOV_OFFSET    (0x160)
> > > -#define IGB_VF_OFFSET           (0x80)
> > > -#define IGB_VF_STRIDE           (2)
> > >   #define E1000E_MMIO_IDX     0
> > >   #define E1000E_FLASH_IDX    1
> > > @@ -431,7 +429,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, IGB_VF_OFFSET);
> > >       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,
> > 
> > 
> > I think this change would break migrations from 8.0. No?
> 
> Well, I don't have a reason to concern more for this change than other bug
> fixes with behavioral changes observable from the guest.

Try it and see it fail.


> > 
> > 
> > More importantly your commit log says linked list should end
> > with 0, but you make it point at a VF instead.
> > 
> > 
> > > diff --git a/hw/net/igbvf.c b/hw/net/igbvf.c
> > > index 284ea61184..bf2f237ab5 100644
> > > --- a/hw/net/igbvf.c
> > > +++ b/hw/net/igbvf.c
> > > @@ -240,6 +240,9 @@ static const MemoryRegionOps mmio_ops = {
> > >   static void igbvf_pci_realize(PCIDevice *dev, Error **errp)
> > >   {
> > >       IgbVfState *s = IGBVF(dev);
> > > +    uint16_t nextvfn = pcie_sriov_vf_number(dev) + 1;
> > > +    uint16_t nextfn = nextvfn < IGB_MAX_VF_FUNCTIONS ?
> > > +                      IGB_VF_OFFSET + nextvfn * IGB_VF_STRIDE : 0;
> > >       int ret;
> > >       int i;
> > > @@ -270,7 +273,7 @@ static void igbvf_pci_realize(PCIDevice *dev, Error **errp)
> > >           hw_error("Failed to initialize AER capability");
> > >       }
> > > -    pcie_ari_init(dev, 0x150, 1);
> > > +    pcie_ari_init(dev, 0x150, nextfn);
> > 
> > 
> > 
> > For this one I don't see why it matters at all:
> > 
> > The presence of Shadow Functions does not affect this field.
> > For VFs, this field is undefined since VFs are located using First VF Offset (see § Section 9.3.3.9 ) and VF
> > Stride (see § Section 9.3.3.10 ).
> 
> I missed the statements saying the field is undefined for VFs. I posted an
> alternative series ("[PATCH 0/3] pci: Fix ARI next function numbers") so
> please review it.
> 
> > 
> > 
> > 
> > 
> > >   }
> > >   static void igbvf_pci_uninit(PCIDevice *dev)
> > > -- 
> > > 2.41.0
> >

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.