Message ID | 20230701070133.24877-4-akihiko.odaki@daynix.com |
---|---|
State | New |
Headers | show |
Series | pci: Compare function number and ARI next function number | expand |
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
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 >
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 > >
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)
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(-)