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.od...@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
> >
