| Message ID | 20230705022421.13115-3-akihiko.odaki@daynix.com |
|---|---|
| State | New |
| Headers | show |
| Series | pcie: Fix ARI next function numbers | expand |
> On 05-Jul-2023, at 7:54 AM, Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > > The current implementers of ARI are all SR-IOV devices. The ARI next > function number field is undefined for VF according to PCI Express Base > Specification Revision 5.0 Version 1.0 section 9.3.7.7. The PF should > end the linked list formed with the field by specifying 0 according to > section 7.8.7.2. Section 7.8.7.2 ARI Capability Register (Offset 04h), I see only this Next Function Number - This field indicates the Function Number of the next higher numbered Function in the Device, or 00h if there are no higher numbered Functions. Function 0 starts this linked list of Functions. I do not see anything specifically for PF. What am I missing? > > For migration, the field will keep having 1 as its value on the old > virt models. > > Fixes: 2503461691 ("pcie: Add some SR/IOV API documentation in docs/pcie_sriov.txt") > Fixes: 44c2c09488 ("hw/nvme: Add support for SR-IOV") > Fixes: 3a977deebe ("Intrdocue igb device emulation") > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > --- > include/hw/pci/pci.h | 2 ++ > hw/core/machine.c | 1 + > hw/pci/pci.c | 2 ++ > hw/pci/pcie.c | 2 +- > 4 files changed, 6 insertions(+), 1 deletion(-) > > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h > index e6d0574a29..9c5b5eb206 100644 > --- a/include/hw/pci/pci.h > +++ b/include/hw/pci/pci.h > @@ -209,6 +209,8 @@ enum { > QEMU_PCIE_CAP_CXL = (1 << QEMU_PCIE_CXL_BITNR), > #define QEMU_PCIE_ERR_UNC_MASK_BITNR 11 > QEMU_PCIE_ERR_UNC_MASK = (1 << QEMU_PCIE_ERR_UNC_MASK_BITNR), > +#define QEMU_PCIE_ARI_NEXTFN_1_BITNR 12 > + QEMU_PCIE_ARI_NEXTFN_1 = (1 << QEMU_PCIE_ARI_NEXTFN_1_BITNR), > }; > > typedef struct PCIINTxRoute { > diff --git a/hw/core/machine.c b/hw/core/machine.c > index 46f8f9a2b0..f0d35c6401 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"}, > + { TYPE_PCI_DEVICE, "x-pcie-ari-nextfn-1", "on" }, > }; > const size_t hw_compat_8_0_len = G_N_ELEMENTS(hw_compat_8_0); > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index e2eb4c3b4a..45a9bc0da8 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -82,6 +82,8 @@ static Property pci_props[] = { > DEFINE_PROP_UINT32("acpi-index", PCIDevice, acpi_index, 0), > DEFINE_PROP_BIT("x-pcie-err-unc-mask", PCIDevice, cap_present, > QEMU_PCIE_ERR_UNC_MASK_BITNR, true), > + DEFINE_PROP_BIT("x-pcie-ari-nextfn-1", PCIDevice, cap_present, > + QEMU_PCIE_ARI_NEXTFN_1_BITNR, false), > DEFINE_PROP_END_OF_LIST() > }; > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > index 9a3f6430e8..cf09e03a10 100644 > --- a/hw/pci/pcie.c > +++ b/hw/pci/pcie.c > @@ -1030,7 +1030,7 @@ void pcie_sync_bridge_lnk(PCIDevice *bridge_dev) > /* ARI */ > void pcie_ari_init(PCIDevice *dev, uint16_t offset) > { > - uint16_t nextfn = 1; > + uint16_t nextfn = dev->cap_present & QEMU_PCIE_ARI_NEXTFN_1 ? 1 : 0; > > pcie_add_capability(dev, PCI_EXT_CAP_ID_ARI, PCI_ARI_VER, > offset, PCI_ARI_SIZEOF); > -- > 2.41.0 >
On 2023/07/10 16:51, Ani Sinha wrote: > > >> On 05-Jul-2023, at 7:54 AM, Akihiko Odaki <akihiko.odaki@daynix.com> wrote: >> >> The current implementers of ARI are all SR-IOV devices. The ARI next >> function number field is undefined for VF according to PCI Express Base >> Specification Revision 5.0 Version 1.0 section 9.3.7.7. The PF should >> end the linked list formed with the field by specifying 0 according to >> section 7.8.7.2. > > Section 7.8.7.2 ARI Capability Register (Offset 04h), I see only this > > Next Function Number - This field indicates the Function Number of the next higher numbered Function in the Device, or 00h if there are no higher numbered Functions. Function 0 starts this linked list of Functions. > > I do not see anything specifically for PF. What am I missing? It's not specific to PF, but in general the linked list of Functions needs to end with 0. > >> >> For migration, the field will keep having 1 as its value on the old >> virt models. >> >> Fixes: 2503461691 ("pcie: Add some SR/IOV API documentation in docs/pcie_sriov.txt") >> Fixes: 44c2c09488 ("hw/nvme: Add support for SR-IOV") >> Fixes: 3a977deebe ("Intrdocue igb device emulation") >> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> >> --- >> include/hw/pci/pci.h | 2 ++ >> hw/core/machine.c | 1 + >> hw/pci/pci.c | 2 ++ >> hw/pci/pcie.c | 2 +- >> 4 files changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h >> index e6d0574a29..9c5b5eb206 100644 >> --- a/include/hw/pci/pci.h >> +++ b/include/hw/pci/pci.h >> @@ -209,6 +209,8 @@ enum { >> QEMU_PCIE_CAP_CXL = (1 << QEMU_PCIE_CXL_BITNR), >> #define QEMU_PCIE_ERR_UNC_MASK_BITNR 11 >> QEMU_PCIE_ERR_UNC_MASK = (1 << QEMU_PCIE_ERR_UNC_MASK_BITNR), >> +#define QEMU_PCIE_ARI_NEXTFN_1_BITNR 12 >> + QEMU_PCIE_ARI_NEXTFN_1 = (1 << QEMU_PCIE_ARI_NEXTFN_1_BITNR), >> }; >> >> typedef struct PCIINTxRoute { >> diff --git a/hw/core/machine.c b/hw/core/machine.c >> index 46f8f9a2b0..f0d35c6401 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"}, >> + { TYPE_PCI_DEVICE, "x-pcie-ari-nextfn-1", "on" }, >> }; >> const size_t hw_compat_8_0_len = G_N_ELEMENTS(hw_compat_8_0); >> >> diff --git a/hw/pci/pci.c b/hw/pci/pci.c >> index e2eb4c3b4a..45a9bc0da8 100644 >> --- a/hw/pci/pci.c >> +++ b/hw/pci/pci.c >> @@ -82,6 +82,8 @@ static Property pci_props[] = { >> DEFINE_PROP_UINT32("acpi-index", PCIDevice, acpi_index, 0), >> DEFINE_PROP_BIT("x-pcie-err-unc-mask", PCIDevice, cap_present, >> QEMU_PCIE_ERR_UNC_MASK_BITNR, true), >> + DEFINE_PROP_BIT("x-pcie-ari-nextfn-1", PCIDevice, cap_present, >> + QEMU_PCIE_ARI_NEXTFN_1_BITNR, false), >> DEFINE_PROP_END_OF_LIST() >> }; >> >> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c >> index 9a3f6430e8..cf09e03a10 100644 >> --- a/hw/pci/pcie.c >> +++ b/hw/pci/pcie.c >> @@ -1030,7 +1030,7 @@ void pcie_sync_bridge_lnk(PCIDevice *bridge_dev) >> /* ARI */ >> void pcie_ari_init(PCIDevice *dev, uint16_t offset) >> { >> - uint16_t nextfn = 1; >> + uint16_t nextfn = dev->cap_present & QEMU_PCIE_ARI_NEXTFN_1 ? 1 : 0; >> >> pcie_add_capability(dev, PCI_EXT_CAP_ID_ARI, PCI_ARI_VER, >> offset, PCI_ARI_SIZEOF); >> -- >> 2.41.0 >> >
> On 10-Jul-2023, at 1:23 PM, Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > > On 2023/07/10 16:51, Ani Sinha wrote: >>> On 05-Jul-2023, at 7:54 AM, Akihiko Odaki <akihiko.odaki@daynix.com> wrote: >>> >>> The current implementers of ARI are all SR-IOV devices. The ARI next >>> function number field is undefined for VF according to PCI Express Base >>> Specification Revision 5.0 Version 1.0 section 9.3.7.7. The PF should >>> end the linked list formed with the field by specifying 0 according to >>> section 7.8.7.2. >> Section 7.8.7.2 ARI Capability Register (Offset 04h), I see only this >> Next Function Number - This field indicates the Function Number of the next higher numbered Function in the Device, or 00h if there are no higher numbered Functions. Function 0 starts this linked list of Functions. >> I do not see anything specifically for PF. What am I missing? > > It's not specific to PF, but in general the linked list of Functions needs to end with 0. OK so the language is confusing here. Maybe just say that the next function number should be 00h if there are no higher numbered functions, without saying anything about PF. > >>> >>> For migration, the field will keep having 1 as its value on the old >>> virt models. >>> >>> Fixes: 2503461691 ("pcie: Add some SR/IOV API documentation in docs/pcie_sriov.txt") >>> Fixes: 44c2c09488 ("hw/nvme: Add support for SR-IOV") >>> Fixes: 3a977deebe ("Intrdocue igb device emulation") >>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> >>> --- >>> include/hw/pci/pci.h | 2 ++ >>> hw/core/machine.c | 1 + >>> hw/pci/pci.c | 2 ++ >>> hw/pci/pcie.c | 2 +- >>> 4 files changed, 6 insertions(+), 1 deletion(-) >>> >>> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h >>> index e6d0574a29..9c5b5eb206 100644 >>> --- a/include/hw/pci/pci.h >>> +++ b/include/hw/pci/pci.h >>> @@ -209,6 +209,8 @@ enum { >>> QEMU_PCIE_CAP_CXL = (1 << QEMU_PCIE_CXL_BITNR), >>> #define QEMU_PCIE_ERR_UNC_MASK_BITNR 11 >>> QEMU_PCIE_ERR_UNC_MASK = (1 << QEMU_PCIE_ERR_UNC_MASK_BITNR), >>> +#define QEMU_PCIE_ARI_NEXTFN_1_BITNR 12 >>> + QEMU_PCIE_ARI_NEXTFN_1 = (1 << QEMU_PCIE_ARI_NEXTFN_1_BITNR), >>> }; >>> >>> typedef struct PCIINTxRoute { >>> diff --git a/hw/core/machine.c b/hw/core/machine.c >>> index 46f8f9a2b0..f0d35c6401 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"}, >>> + { TYPE_PCI_DEVICE, "x-pcie-ari-nextfn-1", "on" }, >>> }; >>> const size_t hw_compat_8_0_len = G_N_ELEMENTS(hw_compat_8_0); >>> >>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c >>> index e2eb4c3b4a..45a9bc0da8 100644 >>> --- a/hw/pci/pci.c >>> +++ b/hw/pci/pci.c >>> @@ -82,6 +82,8 @@ static Property pci_props[] = { >>> DEFINE_PROP_UINT32("acpi-index", PCIDevice, acpi_index, 0), >>> DEFINE_PROP_BIT("x-pcie-err-unc-mask", PCIDevice, cap_present, >>> QEMU_PCIE_ERR_UNC_MASK_BITNR, true), >>> + DEFINE_PROP_BIT("x-pcie-ari-nextfn-1", PCIDevice, cap_present, >>> + QEMU_PCIE_ARI_NEXTFN_1_BITNR, false), >>> DEFINE_PROP_END_OF_LIST() >>> }; >>> >>> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c >>> index 9a3f6430e8..cf09e03a10 100644 >>> --- a/hw/pci/pcie.c >>> +++ b/hw/pci/pcie.c >>> @@ -1030,7 +1030,7 @@ void pcie_sync_bridge_lnk(PCIDevice *bridge_dev) >>> /* ARI */ >>> void pcie_ari_init(PCIDevice *dev, uint16_t offset) >>> { >>> - uint16_t nextfn = 1; >>> + uint16_t nextfn = dev->cap_present & QEMU_PCIE_ARI_NEXTFN_1 ? 1 : 0; >>> >>> pcie_add_capability(dev, PCI_EXT_CAP_ID_ARI, PCI_ARI_VER, >>> offset, PCI_ARI_SIZEOF); >>> -- >>> 2.41.0 >>> >
On Mon, Jul 10, 2023 at 01:21:50PM +0530, Ani Sinha wrote: > > > > On 05-Jul-2023, at 7:54 AM, Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > > > > The current implementers of ARI are all SR-IOV devices. The ARI next > > function number field is undefined for VF according to PCI Express Base > > Specification Revision 5.0 Version 1.0 section 9.3.7.7. The PF should > > end the linked list formed with the field by specifying 0 according to > > section 7.8.7.2. > > Section 7.8.7.2 ARI Capability Register (Offset 04h), I see only this > > Next Function Number - This field indicates the Function Number of the next higher numbered Function in the Device, or 00h if there are no higher numbered Functions. Function 0 starts this linked list of Functions. > > I do not see anything specifically for PF. What am I missing? This is *only* for PFs. There's separate text explaining that VFs use NumVFs VFOffset and VFStride. > > > > For migration, the field will keep having 1 as its value on the old > > virt models. > > > > Fixes: 2503461691 ("pcie: Add some SR/IOV API documentation in docs/pcie_sriov.txt") > > Fixes: 44c2c09488 ("hw/nvme: Add support for SR-IOV") > > Fixes: 3a977deebe ("Intrdocue igb device emulation") > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > > --- > > include/hw/pci/pci.h | 2 ++ > > hw/core/machine.c | 1 + > > hw/pci/pci.c | 2 ++ > > hw/pci/pcie.c | 2 +- > > 4 files changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h > > index e6d0574a29..9c5b5eb206 100644 > > --- a/include/hw/pci/pci.h > > +++ b/include/hw/pci/pci.h > > @@ -209,6 +209,8 @@ enum { > > QEMU_PCIE_CAP_CXL = (1 << QEMU_PCIE_CXL_BITNR), > > #define QEMU_PCIE_ERR_UNC_MASK_BITNR 11 > > QEMU_PCIE_ERR_UNC_MASK = (1 << QEMU_PCIE_ERR_UNC_MASK_BITNR), > > +#define QEMU_PCIE_ARI_NEXTFN_1_BITNR 12 > > + QEMU_PCIE_ARI_NEXTFN_1 = (1 << QEMU_PCIE_ARI_NEXTFN_1_BITNR), > > }; > > > > typedef struct PCIINTxRoute { > > diff --git a/hw/core/machine.c b/hw/core/machine.c > > index 46f8f9a2b0..f0d35c6401 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"}, > > + { TYPE_PCI_DEVICE, "x-pcie-ari-nextfn-1", "on" }, > > }; > > const size_t hw_compat_8_0_len = G_N_ELEMENTS(hw_compat_8_0); > > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > > index e2eb4c3b4a..45a9bc0da8 100644 > > --- a/hw/pci/pci.c > > +++ b/hw/pci/pci.c > > @@ -82,6 +82,8 @@ static Property pci_props[] = { > > DEFINE_PROP_UINT32("acpi-index", PCIDevice, acpi_index, 0), > > DEFINE_PROP_BIT("x-pcie-err-unc-mask", PCIDevice, cap_present, > > QEMU_PCIE_ERR_UNC_MASK_BITNR, true), > > + DEFINE_PROP_BIT("x-pcie-ari-nextfn-1", PCIDevice, cap_present, > > + QEMU_PCIE_ARI_NEXTFN_1_BITNR, false), > > DEFINE_PROP_END_OF_LIST() > > }; > > > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > > index 9a3f6430e8..cf09e03a10 100644 > > --- a/hw/pci/pcie.c > > +++ b/hw/pci/pcie.c > > @@ -1030,7 +1030,7 @@ void pcie_sync_bridge_lnk(PCIDevice *bridge_dev) > > /* ARI */ > > void pcie_ari_init(PCIDevice *dev, uint16_t offset) > > { > > - uint16_t nextfn = 1; > > + uint16_t nextfn = dev->cap_present & QEMU_PCIE_ARI_NEXTFN_1 ? 1 : 0; > > > > pcie_add_capability(dev, PCI_EXT_CAP_ID_ARI, PCI_ARI_VER, > > offset, PCI_ARI_SIZEOF); > > -- > > 2.41.0 > >
> On 10-Jul-2023, at 2:46 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Mon, Jul 10, 2023 at 01:21:50PM +0530, Ani Sinha wrote: >> >> >>> On 05-Jul-2023, at 7:54 AM, Akihiko Odaki <akihiko.odaki@daynix.com> wrote: >>> >>> The current implementers of ARI are all SR-IOV devices. The ARI next >>> function number field is undefined for VF according to PCI Express Base >>> Specification Revision 5.0 Version 1.0 section 9.3.7.7. The PF should >>> end the linked list formed with the field by specifying 0 according to >>> section 7.8.7.2. >> >> Section 7.8.7.2 ARI Capability Register (Offset 04h), I see only this >> >> Next Function Number - This field indicates the Function Number of the next higher numbered Function in the Device, or 00h if there are no higher numbered Functions. Function 0 starts this linked list of Functions. >> >> I do not see anything specifically for PF. What am I missing? > > This is *only* for PFs. I think this covers both SRIOV and non SRIOV cases both. This is a general case for all devices, PF or other non-SRIOV capable devices. > There's separate text explaining that > VFs use NumVFs VFOffset and VFStride. > > >>> >>> For migration, the field will keep having 1 as its value on the old >>> virt models. >>> >>> Fixes: 2503461691 ("pcie: Add some SR/IOV API documentation in docs/pcie_sriov.txt") >>> Fixes: 44c2c09488 ("hw/nvme: Add support for SR-IOV") >>> Fixes: 3a977deebe ("Intrdocue igb device emulation") >>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> >>> --- >>> include/hw/pci/pci.h | 2 ++ >>> hw/core/machine.c | 1 + >>> hw/pci/pci.c | 2 ++ >>> hw/pci/pcie.c | 2 +- >>> 4 files changed, 6 insertions(+), 1 deletion(-) >>> >>> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h >>> index e6d0574a29..9c5b5eb206 100644 >>> --- a/include/hw/pci/pci.h >>> +++ b/include/hw/pci/pci.h >>> @@ -209,6 +209,8 @@ enum { >>> QEMU_PCIE_CAP_CXL = (1 << QEMU_PCIE_CXL_BITNR), >>> #define QEMU_PCIE_ERR_UNC_MASK_BITNR 11 >>> QEMU_PCIE_ERR_UNC_MASK = (1 << QEMU_PCIE_ERR_UNC_MASK_BITNR), >>> +#define QEMU_PCIE_ARI_NEXTFN_1_BITNR 12 >>> + QEMU_PCIE_ARI_NEXTFN_1 = (1 << QEMU_PCIE_ARI_NEXTFN_1_BITNR), >>> }; >>> >>> typedef struct PCIINTxRoute { >>> diff --git a/hw/core/machine.c b/hw/core/machine.c >>> index 46f8f9a2b0..f0d35c6401 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"}, >>> + { TYPE_PCI_DEVICE, "x-pcie-ari-nextfn-1", "on" }, >>> }; >>> const size_t hw_compat_8_0_len = G_N_ELEMENTS(hw_compat_8_0); >>> >>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c >>> index e2eb4c3b4a..45a9bc0da8 100644 >>> --- a/hw/pci/pci.c >>> +++ b/hw/pci/pci.c >>> @@ -82,6 +82,8 @@ static Property pci_props[] = { >>> DEFINE_PROP_UINT32("acpi-index", PCIDevice, acpi_index, 0), >>> DEFINE_PROP_BIT("x-pcie-err-unc-mask", PCIDevice, cap_present, >>> QEMU_PCIE_ERR_UNC_MASK_BITNR, true), >>> + DEFINE_PROP_BIT("x-pcie-ari-nextfn-1", PCIDevice, cap_present, >>> + QEMU_PCIE_ARI_NEXTFN_1_BITNR, false), >>> DEFINE_PROP_END_OF_LIST() >>> }; >>> >>> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c >>> index 9a3f6430e8..cf09e03a10 100644 >>> --- a/hw/pci/pcie.c >>> +++ b/hw/pci/pcie.c >>> @@ -1030,7 +1030,7 @@ void pcie_sync_bridge_lnk(PCIDevice *bridge_dev) >>> /* ARI */ >>> void pcie_ari_init(PCIDevice *dev, uint16_t offset) >>> { >>> - uint16_t nextfn = 1; >>> + uint16_t nextfn = dev->cap_present & QEMU_PCIE_ARI_NEXTFN_1 ? 1 : 0; >>> >>> pcie_add_capability(dev, PCI_EXT_CAP_ID_ARI, PCI_ARI_VER, >>> offset, PCI_ARI_SIZEOF); >>> -- >>> 2.41.0 >>> >
On Mon, Jul 10, 2023 at 02:49:55PM +0530, Ani Sinha wrote: > > > > On 10-Jul-2023, at 2:46 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Mon, Jul 10, 2023 at 01:21:50PM +0530, Ani Sinha wrote: > >> > >> > >>> On 05-Jul-2023, at 7:54 AM, Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > >>> > >>> The current implementers of ARI are all SR-IOV devices. The ARI next > >>> function number field is undefined for VF according to PCI Express Base > >>> Specification Revision 5.0 Version 1.0 section 9.3.7.7. The PF should > >>> end the linked list formed with the field by specifying 0 according to > >>> section 7.8.7.2. > >> > >> Section 7.8.7.2 ARI Capability Register (Offset 04h), I see only this > >> > >> Next Function Number - This field indicates the Function Number of the next higher numbered Function in the Device, or 00h if there are no higher numbered Functions. Function 0 starts this linked list of Functions. > >> > >> I do not see anything specifically for PF. What am I missing? > > > > This is *only* for PFs. > > I think this covers both SRIOV and non SRIOV cases both. This is a > general case for all devices, PF or other non-SRIOV capable devices. "this" being what? I'm talking about the pci spec text you quoted. check out the sriov spec: Next Function Number – VFs are located using First VF Offset (see Section 3.3.9) and VF Stride (see Section 3.3.10). > > There's separate text explaining that > > VFs use NumVFs VFOffset and VFStride. > > > > > >>> > >>> For migration, the field will keep having 1 as its value on the old > >>> virt models. > >>> > >>> Fixes: 2503461691 ("pcie: Add some SR/IOV API documentation in docs/pcie_sriov.txt") > >>> Fixes: 44c2c09488 ("hw/nvme: Add support for SR-IOV") > >>> Fixes: 3a977deebe ("Intrdocue igb device emulation") > >>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > >>> --- > >>> include/hw/pci/pci.h | 2 ++ > >>> hw/core/machine.c | 1 + > >>> hw/pci/pci.c | 2 ++ > >>> hw/pci/pcie.c | 2 +- > >>> 4 files changed, 6 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h > >>> index e6d0574a29..9c5b5eb206 100644 > >>> --- a/include/hw/pci/pci.h > >>> +++ b/include/hw/pci/pci.h > >>> @@ -209,6 +209,8 @@ enum { > >>> QEMU_PCIE_CAP_CXL = (1 << QEMU_PCIE_CXL_BITNR), > >>> #define QEMU_PCIE_ERR_UNC_MASK_BITNR 11 > >>> QEMU_PCIE_ERR_UNC_MASK = (1 << QEMU_PCIE_ERR_UNC_MASK_BITNR), > >>> +#define QEMU_PCIE_ARI_NEXTFN_1_BITNR 12 > >>> + QEMU_PCIE_ARI_NEXTFN_1 = (1 << QEMU_PCIE_ARI_NEXTFN_1_BITNR), > >>> }; > >>> > >>> typedef struct PCIINTxRoute { > >>> diff --git a/hw/core/machine.c b/hw/core/machine.c > >>> index 46f8f9a2b0..f0d35c6401 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"}, > >>> + { TYPE_PCI_DEVICE, "x-pcie-ari-nextfn-1", "on" }, > >>> }; > >>> const size_t hw_compat_8_0_len = G_N_ELEMENTS(hw_compat_8_0); > >>> > >>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c > >>> index e2eb4c3b4a..45a9bc0da8 100644 > >>> --- a/hw/pci/pci.c > >>> +++ b/hw/pci/pci.c > >>> @@ -82,6 +82,8 @@ static Property pci_props[] = { > >>> DEFINE_PROP_UINT32("acpi-index", PCIDevice, acpi_index, 0), > >>> DEFINE_PROP_BIT("x-pcie-err-unc-mask", PCIDevice, cap_present, > >>> QEMU_PCIE_ERR_UNC_MASK_BITNR, true), > >>> + DEFINE_PROP_BIT("x-pcie-ari-nextfn-1", PCIDevice, cap_present, > >>> + QEMU_PCIE_ARI_NEXTFN_1_BITNR, false), > >>> DEFINE_PROP_END_OF_LIST() > >>> }; > >>> > >>> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > >>> index 9a3f6430e8..cf09e03a10 100644 > >>> --- a/hw/pci/pcie.c > >>> +++ b/hw/pci/pcie.c > >>> @@ -1030,7 +1030,7 @@ void pcie_sync_bridge_lnk(PCIDevice *bridge_dev) > >>> /* ARI */ > >>> void pcie_ari_init(PCIDevice *dev, uint16_t offset) > >>> { > >>> - uint16_t nextfn = 1; > >>> + uint16_t nextfn = dev->cap_present & QEMU_PCIE_ARI_NEXTFN_1 ? 1 : 0; > >>> > >>> pcie_add_capability(dev, PCI_EXT_CAP_ID_ARI, PCI_ARI_VER, > >>> offset, PCI_ARI_SIZEOF); > >>> -- > >>> 2.41.0 > >>> > >
> On 10-Jul-2023, at 3:14 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Mon, Jul 10, 2023 at 02:49:55PM +0530, Ani Sinha wrote: >> >> >>> On 10-Jul-2023, at 2:46 PM, Michael S. Tsirkin <mst@redhat.com> wrote: >>> >>> On Mon, Jul 10, 2023 at 01:21:50PM +0530, Ani Sinha wrote: >>>> >>>> >>>>> On 05-Jul-2023, at 7:54 AM, Akihiko Odaki <akihiko.odaki@daynix.com> wrote: >>>>> >>>>> The current implementers of ARI are all SR-IOV devices. The ARI next >>>>> function number field is undefined for VF according to PCI Express Base >>>>> Specification Revision 5.0 Version 1.0 section 9.3.7.7. The PF should >>>>> end the linked list formed with the field by specifying 0 according to >>>>> section 7.8.7.2. >>>> >>>> Section 7.8.7.2 ARI Capability Register (Offset 04h), I see only this >>>> >>>> Next Function Number - This field indicates the Function Number of the next higher numbered Function in the Device, or 00h if there are no higher numbered Functions. Function 0 starts this linked list of Functions. >>>> >>>> I do not see anything specifically for PF. What am I missing? >>> >>> This is *only* for PFs. >> >> I think this covers both SRIOV and non SRIOV cases both. This is a >> general case for all devices, PF or other non-SRIOV capable devices. > > "this" being what? “this” is the following line I quoted above: "Next Function Number - This field indicates the Function Number of the next higher numbered Function in the Device, or 00h if there are no higher numbered Functions. Function 0 starts this linked list of Functions.” I think it applies for all devices in general (except VFs). > I'm talking about the pci spec text > you quoted. > > check out the sriov spec: > Next Function Number – VFs are located using First > VF Offset (see Section 3.3.9) and VF Stride (see > Section 3.3.10). > > > >>> There's separate text explaining that >>> VFs use NumVFs VFOffset and VFStride. >>> >>> >>>>> >>>>> For migration, the field will keep having 1 as its value on the old >>>>> virt models. >>>>> >>>>> Fixes: 2503461691 ("pcie: Add some SR/IOV API documentation in docs/pcie_sriov.txt") >>>>> Fixes: 44c2c09488 ("hw/nvme: Add support for SR-IOV") >>>>> Fixes: 3a977deebe ("Intrdocue igb device emulation") >>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> >>>>> --- >>>>> include/hw/pci/pci.h | 2 ++ >>>>> hw/core/machine.c | 1 + >>>>> hw/pci/pci.c | 2 ++ >>>>> hw/pci/pcie.c | 2 +- >>>>> 4 files changed, 6 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h >>>>> index e6d0574a29..9c5b5eb206 100644 >>>>> --- a/include/hw/pci/pci.h >>>>> +++ b/include/hw/pci/pci.h >>>>> @@ -209,6 +209,8 @@ enum { >>>>> QEMU_PCIE_CAP_CXL = (1 << QEMU_PCIE_CXL_BITNR), >>>>> #define QEMU_PCIE_ERR_UNC_MASK_BITNR 11 >>>>> QEMU_PCIE_ERR_UNC_MASK = (1 << QEMU_PCIE_ERR_UNC_MASK_BITNR), >>>>> +#define QEMU_PCIE_ARI_NEXTFN_1_BITNR 12 >>>>> + QEMU_PCIE_ARI_NEXTFN_1 = (1 << QEMU_PCIE_ARI_NEXTFN_1_BITNR), >>>>> }; >>>>> >>>>> typedef struct PCIINTxRoute { >>>>> diff --git a/hw/core/machine.c b/hw/core/machine.c >>>>> index 46f8f9a2b0..f0d35c6401 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"}, >>>>> + { TYPE_PCI_DEVICE, "x-pcie-ari-nextfn-1", "on" }, >>>>> }; >>>>> const size_t hw_compat_8_0_len = G_N_ELEMENTS(hw_compat_8_0); >>>>> >>>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c >>>>> index e2eb4c3b4a..45a9bc0da8 100644 >>>>> --- a/hw/pci/pci.c >>>>> +++ b/hw/pci/pci.c >>>>> @@ -82,6 +82,8 @@ static Property pci_props[] = { >>>>> DEFINE_PROP_UINT32("acpi-index", PCIDevice, acpi_index, 0), >>>>> DEFINE_PROP_BIT("x-pcie-err-unc-mask", PCIDevice, cap_present, >>>>> QEMU_PCIE_ERR_UNC_MASK_BITNR, true), >>>>> + DEFINE_PROP_BIT("x-pcie-ari-nextfn-1", PCIDevice, cap_present, >>>>> + QEMU_PCIE_ARI_NEXTFN_1_BITNR, false), >>>>> DEFINE_PROP_END_OF_LIST() >>>>> }; >>>>> >>>>> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c >>>>> index 9a3f6430e8..cf09e03a10 100644 >>>>> --- a/hw/pci/pcie.c >>>>> +++ b/hw/pci/pcie.c >>>>> @@ -1030,7 +1030,7 @@ void pcie_sync_bridge_lnk(PCIDevice *bridge_dev) >>>>> /* ARI */ >>>>> void pcie_ari_init(PCIDevice *dev, uint16_t offset) >>>>> { >>>>> - uint16_t nextfn = 1; >>>>> + uint16_t nextfn = dev->cap_present & QEMU_PCIE_ARI_NEXTFN_1 ? 1 : 0; >>>>> >>>>> pcie_add_capability(dev, PCI_EXT_CAP_ID_ARI, PCI_ARI_VER, >>>>> offset, PCI_ARI_SIZEOF); >>>>> -- >>>>> 2.41.0 >>>>> >>> >
On Mon, Jul 10, 2023 at 03:44:04PM +0530, Ani Sinha wrote: > > > > On 10-Jul-2023, at 3:14 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Mon, Jul 10, 2023 at 02:49:55PM +0530, Ani Sinha wrote: > >> > >> > >>> On 10-Jul-2023, at 2:46 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > >>> > >>> On Mon, Jul 10, 2023 at 01:21:50PM +0530, Ani Sinha wrote: > >>>> > >>>> > >>>>> On 05-Jul-2023, at 7:54 AM, Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > >>>>> > >>>>> The current implementers of ARI are all SR-IOV devices. The ARI next > >>>>> function number field is undefined for VF according to PCI Express Base > >>>>> Specification Revision 5.0 Version 1.0 section 9.3.7.7. The PF should > >>>>> end the linked list formed with the field by specifying 0 according to > >>>>> section 7.8.7.2. > >>>> > >>>> Section 7.8.7.2 ARI Capability Register (Offset 04h), I see only this > >>>> > >>>> Next Function Number - This field indicates the Function Number of the next higher numbered Function in the Device, or 00h if there are no higher numbered Functions. Function 0 starts this linked list of Functions. > >>>> > >>>> I do not see anything specifically for PF. What am I missing? > >>> > >>> This is *only* for PFs. > >> > >> I think this covers both SRIOV and non SRIOV cases both. This is a > >> general case for all devices, PF or other non-SRIOV capable devices. > > > > "this" being what? > > “this” is the following line I quoted above: > > "Next Function Number - This field indicates the Function Number of the next higher numbered Function in the Device, or 00h if there are no higher numbered Functions. Function 0 starts this linked list of Functions.” > > I think it applies for all devices in general (except VFs). For all functions of devices with ARI support, yes. Does not apply to VFs. > > I'm talking about the pci spec text > > you quoted. > > > > check out the sriov spec: > > Next Function Number – VFs are located using First > > VF Offset (see Section 3.3.9) and VF Stride (see > > Section 3.3.10). > > > > > > > >>> There's separate text explaining that > >>> VFs use NumVFs VFOffset and VFStride. > >>> > >>> > >>>>> > >>>>> For migration, the field will keep having 1 as its value on the old > >>>>> virt models. > >>>>> > >>>>> Fixes: 2503461691 ("pcie: Add some SR/IOV API documentation in docs/pcie_sriov.txt") > >>>>> Fixes: 44c2c09488 ("hw/nvme: Add support for SR-IOV") > >>>>> Fixes: 3a977deebe ("Intrdocue igb device emulation") > >>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > >>>>> --- > >>>>> include/hw/pci/pci.h | 2 ++ > >>>>> hw/core/machine.c | 1 + > >>>>> hw/pci/pci.c | 2 ++ > >>>>> hw/pci/pcie.c | 2 +- > >>>>> 4 files changed, 6 insertions(+), 1 deletion(-) > >>>>> > >>>>> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h > >>>>> index e6d0574a29..9c5b5eb206 100644 > >>>>> --- a/include/hw/pci/pci.h > >>>>> +++ b/include/hw/pci/pci.h > >>>>> @@ -209,6 +209,8 @@ enum { > >>>>> QEMU_PCIE_CAP_CXL = (1 << QEMU_PCIE_CXL_BITNR), > >>>>> #define QEMU_PCIE_ERR_UNC_MASK_BITNR 11 > >>>>> QEMU_PCIE_ERR_UNC_MASK = (1 << QEMU_PCIE_ERR_UNC_MASK_BITNR), > >>>>> +#define QEMU_PCIE_ARI_NEXTFN_1_BITNR 12 > >>>>> + QEMU_PCIE_ARI_NEXTFN_1 = (1 << QEMU_PCIE_ARI_NEXTFN_1_BITNR), > >>>>> }; > >>>>> > >>>>> typedef struct PCIINTxRoute { > >>>>> diff --git a/hw/core/machine.c b/hw/core/machine.c > >>>>> index 46f8f9a2b0..f0d35c6401 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"}, > >>>>> + { TYPE_PCI_DEVICE, "x-pcie-ari-nextfn-1", "on" }, > >>>>> }; > >>>>> const size_t hw_compat_8_0_len = G_N_ELEMENTS(hw_compat_8_0); > >>>>> > >>>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c > >>>>> index e2eb4c3b4a..45a9bc0da8 100644 > >>>>> --- a/hw/pci/pci.c > >>>>> +++ b/hw/pci/pci.c > >>>>> @@ -82,6 +82,8 @@ static Property pci_props[] = { > >>>>> DEFINE_PROP_UINT32("acpi-index", PCIDevice, acpi_index, 0), > >>>>> DEFINE_PROP_BIT("x-pcie-err-unc-mask", PCIDevice, cap_present, > >>>>> QEMU_PCIE_ERR_UNC_MASK_BITNR, true), > >>>>> + DEFINE_PROP_BIT("x-pcie-ari-nextfn-1", PCIDevice, cap_present, > >>>>> + QEMU_PCIE_ARI_NEXTFN_1_BITNR, false), > >>>>> DEFINE_PROP_END_OF_LIST() > >>>>> }; > >>>>> > >>>>> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > >>>>> index 9a3f6430e8..cf09e03a10 100644 > >>>>> --- a/hw/pci/pcie.c > >>>>> +++ b/hw/pci/pcie.c > >>>>> @@ -1030,7 +1030,7 @@ void pcie_sync_bridge_lnk(PCIDevice *bridge_dev) > >>>>> /* ARI */ > >>>>> void pcie_ari_init(PCIDevice *dev, uint16_t offset) > >>>>> { > >>>>> - uint16_t nextfn = 1; > >>>>> + uint16_t nextfn = dev->cap_present & QEMU_PCIE_ARI_NEXTFN_1 ? 1 : 0; > >>>>> > >>>>> pcie_add_capability(dev, PCI_EXT_CAP_ID_ARI, PCI_ARI_VER, > >>>>> offset, PCI_ARI_SIZEOF); > >>>>> -- > >>>>> 2.41.0 > >>>>> > >>> > >
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h index e6d0574a29..9c5b5eb206 100644 --- a/include/hw/pci/pci.h +++ b/include/hw/pci/pci.h @@ -209,6 +209,8 @@ enum { QEMU_PCIE_CAP_CXL = (1 << QEMU_PCIE_CXL_BITNR), #define QEMU_PCIE_ERR_UNC_MASK_BITNR 11 QEMU_PCIE_ERR_UNC_MASK = (1 << QEMU_PCIE_ERR_UNC_MASK_BITNR), +#define QEMU_PCIE_ARI_NEXTFN_1_BITNR 12 + QEMU_PCIE_ARI_NEXTFN_1 = (1 << QEMU_PCIE_ARI_NEXTFN_1_BITNR), }; typedef struct PCIINTxRoute { diff --git a/hw/core/machine.c b/hw/core/machine.c index 46f8f9a2b0..f0d35c6401 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"}, + { TYPE_PCI_DEVICE, "x-pcie-ari-nextfn-1", "on" }, }; const size_t hw_compat_8_0_len = G_N_ELEMENTS(hw_compat_8_0); diff --git a/hw/pci/pci.c b/hw/pci/pci.c index e2eb4c3b4a..45a9bc0da8 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -82,6 +82,8 @@ static Property pci_props[] = { DEFINE_PROP_UINT32("acpi-index", PCIDevice, acpi_index, 0), DEFINE_PROP_BIT("x-pcie-err-unc-mask", PCIDevice, cap_present, QEMU_PCIE_ERR_UNC_MASK_BITNR, true), + DEFINE_PROP_BIT("x-pcie-ari-nextfn-1", PCIDevice, cap_present, + QEMU_PCIE_ARI_NEXTFN_1_BITNR, false), DEFINE_PROP_END_OF_LIST() }; diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c index 9a3f6430e8..cf09e03a10 100644 --- a/hw/pci/pcie.c +++ b/hw/pci/pcie.c @@ -1030,7 +1030,7 @@ void pcie_sync_bridge_lnk(PCIDevice *bridge_dev) /* ARI */ void pcie_ari_init(PCIDevice *dev, uint16_t offset) { - uint16_t nextfn = 1; + uint16_t nextfn = dev->cap_present & QEMU_PCIE_ARI_NEXTFN_1 ? 1 : 0; pcie_add_capability(dev, PCI_EXT_CAP_ID_ARI, PCI_ARI_VER, offset, PCI_ARI_SIZEOF);
The current implementers of ARI are all SR-IOV devices. The ARI next function number field is undefined for VF according to PCI Express Base Specification Revision 5.0 Version 1.0 section 9.3.7.7. The PF should end the linked list formed with the field by specifying 0 according to section 7.8.7.2. For migration, the field will keep having 1 as its value on the old virt models. Fixes: 2503461691 ("pcie: Add some SR/IOV API documentation in docs/pcie_sriov.txt") Fixes: 44c2c09488 ("hw/nvme: Add support for SR-IOV") Fixes: 3a977deebe ("Intrdocue igb device emulation") Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> --- include/hw/pci/pci.h | 2 ++ hw/core/machine.c | 1 + hw/pci/pci.c | 2 ++ hw/pci/pcie.c | 2 +- 4 files changed, 6 insertions(+), 1 deletion(-)