| Message ID | 20170922141928.49141-3-bryantly@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Fri, Sep 22, 2017 at 09:19:28AM -0500, Bryant G. Ly wrote: > This patch adds the machine dependent call for > pcibios_bus_add_device, since the previous patch > separated the calls out between the PowerNV and PowerVM. > > The difference here is that for the PowerVM environment > we do not want match_driver set because in this environment > we do not want the VF device drivers to load immediately, due to > firmware loading the device node when VF device is assigned to the > logical partition. > > This patch will depend on the patch linked below, which is under > review. > > https://patchwork.kernel.org/patch/9882915/ > > Signed-off-by: Bryant G. Ly <bryantly@linux.vnet.ibm.com> > Signed-off-by: Juan J. Alvarez <jjalvare@us.ibm.com> > --- > arch/powerpc/platforms/pseries/eeh_pseries.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/powerpc/platforms/pseries/eeh_pseries.c > index 6b812ad990e4..45946ee90985 100644 > --- a/arch/powerpc/platforms/pseries/eeh_pseries.c > +++ b/arch/powerpc/platforms/pseries/eeh_pseries.c > @@ -64,6 +64,27 @@ static unsigned char slot_errbuf[RTAS_ERROR_LOG_MAX]; > static DEFINE_SPINLOCK(slot_errbuf_lock); > static int eeh_error_buf_size; > > +void pseries_pcibios_bus_add_device(struct pci_dev *pdev) > +{ > + struct pci_dn *pdn = pci_get_pdn(pdev); > + > + if (!pdev->is_virtfn) > + return; > + > + pdn->device_id = pdev->device; > + pdn->vendor_id = pdev->vendor; > + pdn->class_code = pdev->class; > + > + /* > + * The following operations will fail if VF's sysfs files > + * aren't created or its resources aren't finalized. > + */ > + eeh_add_device_early(pdn); > + eeh_add_device_late(pdev); > + eeh_sysfs_add_device(pdev); > + pdev->match_driver = -1; match_driver is a bool, which should be assigned "true" or "false". > +} > + > /** > * pseries_eeh_init - EEH platform dependent initialization > * > @@ -120,6 +141,9 @@ static int pseries_eeh_init(void) > /* Set EEH probe mode */ > eeh_add_flag(EEH_PROBE_MODE_DEVTREE | EEH_ENABLE_IO_FOR_LOG); > > + /* Set EEH machine dependent code */ > + ppc_md.pcibios_bus_add_device = pseries_pcibios_bus_add_device; > + > return 0; > } > > -- > 2.11.0 (Apple Git-81) >
Bjorn Helgaas <helgaas@kernel.org> writes: > On Fri, Sep 22, 2017 at 09:19:28AM -0500, Bryant G. Ly wrote: >> This patch adds the machine dependent call for >> pcibios_bus_add_device, since the previous patch >> separated the calls out between the PowerNV and PowerVM. >> >> The difference here is that for the PowerVM environment >> we do not want match_driver set because in this environment >> we do not want the VF device drivers to load immediately, due to >> firmware loading the device node when VF device is assigned to the >> logical partition. >> >> This patch will depend on the patch linked below, which is under >> review. >> >> https://patchwork.kernel.org/patch/9882915/ >> >> Signed-off-by: Bryant G. Ly <bryantly@linux.vnet.ibm.com> >> Signed-off-by: Juan J. Alvarez <jjalvare@us.ibm.com> >> --- >> arch/powerpc/platforms/pseries/eeh_pseries.c | 24 ++++++++++++++++++++++++ >> 1 file changed, 24 insertions(+) >> >> diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/powerpc/platforms/pseries/eeh_pseries.c >> index 6b812ad990e4..45946ee90985 100644 >> --- a/arch/powerpc/platforms/pseries/eeh_pseries.c >> +++ b/arch/powerpc/platforms/pseries/eeh_pseries.c >> @@ -64,6 +64,27 @@ static unsigned char slot_errbuf[RTAS_ERROR_LOG_MAX]; >> static DEFINE_SPINLOCK(slot_errbuf_lock); >> static int eeh_error_buf_size; >> >> +void pseries_pcibios_bus_add_device(struct pci_dev *pdev) >> +{ >> + struct pci_dn *pdn = pci_get_pdn(pdev); >> + >> + if (!pdev->is_virtfn) >> + return; >> + >> + pdn->device_id = pdev->device; >> + pdn->vendor_id = pdev->vendor; >> + pdn->class_code = pdev->class; >> + >> + /* >> + * The following operations will fail if VF's sysfs files >> + * aren't created or its resources aren't finalized. >> + */ >> + eeh_add_device_early(pdn); >> + eeh_add_device_late(pdev); >> + eeh_sysfs_add_device(pdev); >> + pdev->match_driver = -1; > > match_driver is a bool, which should be assigned "true" or "false". Above he mentioned a dependency on: [04/10] PCI: extend pci device match_driver state https://patchwork.kernel.org/patch/9882915/ Which makes it an int. Or has that patch been rejected or something? cheers
On Thu, Oct 12, 2017 at 03:09:53PM +1100, Michael Ellerman wrote: > Bjorn Helgaas <helgaas@kernel.org> writes: > > > On Fri, Sep 22, 2017 at 09:19:28AM -0500, Bryant G. Ly wrote: > >> This patch adds the machine dependent call for > >> pcibios_bus_add_device, since the previous patch > >> separated the calls out between the PowerNV and PowerVM. > >> > >> The difference here is that for the PowerVM environment > >> we do not want match_driver set because in this environment > >> we do not want the VF device drivers to load immediately, due to > >> firmware loading the device node when VF device is assigned to the > >> logical partition. > >> > >> This patch will depend on the patch linked below, which is under > >> review. > >> > >> https://patchwork.kernel.org/patch/9882915/ > >> > >> Signed-off-by: Bryant G. Ly <bryantly@linux.vnet.ibm.com> > >> Signed-off-by: Juan J. Alvarez <jjalvare@us.ibm.com> > >> --- > >> arch/powerpc/platforms/pseries/eeh_pseries.c | 24 ++++++++++++++++++++++++ > >> 1 file changed, 24 insertions(+) > >> > >> diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/powerpc/platforms/pseries/eeh_pseries.c > >> index 6b812ad990e4..45946ee90985 100644 > >> --- a/arch/powerpc/platforms/pseries/eeh_pseries.c > >> +++ b/arch/powerpc/platforms/pseries/eeh_pseries.c > >> @@ -64,6 +64,27 @@ static unsigned char slot_errbuf[RTAS_ERROR_LOG_MAX]; > >> static DEFINE_SPINLOCK(slot_errbuf_lock); > >> static int eeh_error_buf_size; > >> > >> +void pseries_pcibios_bus_add_device(struct pci_dev *pdev) > >> +{ > >> + struct pci_dn *pdn = pci_get_pdn(pdev); > >> + > >> + if (!pdev->is_virtfn) > >> + return; > >> + > >> + pdn->device_id = pdev->device; > >> + pdn->vendor_id = pdev->vendor; > >> + pdn->class_code = pdev->class; > >> + > >> + /* > >> + * The following operations will fail if VF's sysfs files > >> + * aren't created or its resources aren't finalized. > >> + */ > >> + eeh_add_device_early(pdn); > >> + eeh_add_device_late(pdev); > >> + eeh_sysfs_add_device(pdev); > >> + pdev->match_driver = -1; > > > > match_driver is a bool, which should be assigned "true" or "false". > > Above he mentioned a dependency on: > > [04/10] PCI: extend pci device match_driver state > https://patchwork.kernel.org/patch/9882915/ > > > Which makes it an int. Oh, right, I missed that, thanks. > Or has that patch been rejected or something? I haven't *rejected* it, but it's low on my priority list, so you shouldn't depend on it unless it adds functionality you really need. If I did apply that particular patch, I would want some rework because it currently obfuscates the match_driver logic. There's no clue when reading the code what -1/0/1 mean. Apparently here you *do* want the "-1 means the PCI core will never set match_driver to 1" functionality, so maybe you do depend on it. If that's the case, how to you ever bind a driver to these VFs? The changelog says you don't want VF drivers to load *immediately*, so I assume you do want them to load eventually. Bjorn
On 10/12/17 1:29 PM, Bjorn Helgaas wrote: > On Thu, Oct 12, 2017 at 03:09:53PM +1100, Michael Ellerman wrote: >> Bjorn Helgaas <helgaas@kernel.org> writes: >> >>> On Fri, Sep 22, 2017 at 09:19:28AM -0500, Bryant G. Ly wrote: >>>> This patch adds the machine dependent call for >>>> pcibios_bus_add_device, since the previous patch >>>> separated the calls out between the PowerNV and PowerVM. >>>> >>>> The difference here is that for the PowerVM environment >>>> we do not want match_driver set because in this environment >>>> we do not want the VF device drivers to load immediately, due to >>>> firmware loading the device node when VF device is assigned to the >>>> logical partition. >>>> >>>> This patch will depend on the patch linked below, which is under >>>> review. >>>> >>>> https://patchwork.kernel.org/patch/9882915/ >>>> >>>> Signed-off-by: Bryant G. Ly <bryantly@linux.vnet.ibm.com> >>>> Signed-off-by: Juan J. Alvarez <jjalvare@us.ibm.com> >>>> --- >>>> arch/powerpc/platforms/pseries/eeh_pseries.c | 24 ++++++++++++++++++++++++ >>>> 1 file changed, 24 insertions(+) >>>> >>>> diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/powerpc/platforms/pseries/eeh_pseries.c >>>> index 6b812ad990e4..45946ee90985 100644 >>>> --- a/arch/powerpc/platforms/pseries/eeh_pseries.c >>>> +++ b/arch/powerpc/platforms/pseries/eeh_pseries.c >>>> @@ -64,6 +64,27 @@ static unsigned char slot_errbuf[RTAS_ERROR_LOG_MAX]; >>>> static DEFINE_SPINLOCK(slot_errbuf_lock); >>>> static int eeh_error_buf_size; >>>> >>>> +void pseries_pcibios_bus_add_device(struct pci_dev *pdev) >>>> +{ >>>> + struct pci_dn *pdn = pci_get_pdn(pdev); >>>> + >>>> + if (!pdev->is_virtfn) >>>> + return; >>>> + >>>> + pdn->device_id = pdev->device; >>>> + pdn->vendor_id = pdev->vendor; >>>> + pdn->class_code = pdev->class; >>>> + >>>> + /* >>>> + * The following operations will fail if VF's sysfs files >>>> + * aren't created or its resources aren't finalized. >>>> + */ >>>> + eeh_add_device_early(pdn); >>>> + eeh_add_device_late(pdev); >>>> + eeh_sysfs_add_device(pdev); >>>> + pdev->match_driver = -1; >>> match_driver is a bool, which should be assigned "true" or "false". >> Above he mentioned a dependency on: >> >> [04/10] PCI: extend pci device match_driver state >> https://patchwork.kernel.org/patch/9882915/ >> >> >> Which makes it an int. > Oh, right, I missed that, thanks. > >> Or has that patch been rejected or something? > I haven't *rejected* it, but it's low on my priority list, so you > shouldn't depend on it unless it adds functionality you really need. > If I did apply that particular patch, I would want some rework because > it currently obfuscates the match_driver logic. There's no clue when > reading the code what -1/0/1 mean. So do you prefer enum's? - If so I can make a change for that. > Apparently here you *do* want the "-1 means the PCI core will never > set match_driver to 1" functionality, so maybe you do depend on it. We depend on the patch because we want that ability to never set match_driver, for SRIOV on PowerVM. > > If that's the case, how to you ever bind a driver to these VFs? The > changelog says you don't want VF drivers to load *immediately*, so I > assume you do want them to load eventually. > The VF's that get dynamically created within the configure SR-IOV call, on the Pseries Platform, wont be matched with a driver. - We do not want it to match. The Power Hypervisor will load the VFs. The VF's will get assigned(by the user) via the HMC or Novalink in this environment which will then trigger PHYP to load the VF device node to the device tree. -Bryant
[+cc Alex, Bodong, Eli, Saeed] On Thu, Oct 12, 2017 at 02:59:23PM -0500, Bryant G. Ly wrote: > On 10/12/17 1:29 PM, Bjorn Helgaas wrote: > >On Thu, Oct 12, 2017 at 03:09:53PM +1100, Michael Ellerman wrote: > >>Bjorn Helgaas <helgaas@kernel.org> writes: > >> > >>>On Fri, Sep 22, 2017 at 09:19:28AM -0500, Bryant G. Ly wrote: > >>>>This patch adds the machine dependent call for > >>>>pcibios_bus_add_device, since the previous patch > >>>>separated the calls out between the PowerNV and PowerVM. > >>>> > >>>>The difference here is that for the PowerVM environment > >>>>we do not want match_driver set because in this environment > >>>>we do not want the VF device drivers to load immediately, due to > >>>>firmware loading the device node when VF device is assigned to the > >>>>logical partition. > >>>> > >>>>This patch will depend on the patch linked below, which is under > >>>>review. > >>>> > >>>>https://patchwork.kernel.org/patch/9882915/ > >>>> > >>>>Signed-off-by: Bryant G. Ly <bryantly@linux.vnet.ibm.com> > >>>>Signed-off-by: Juan J. Alvarez <jjalvare@us.ibm.com> > >>>>--- > >>>> arch/powerpc/platforms/pseries/eeh_pseries.c | 24 ++++++++++++++++++++++++ > >>>> 1 file changed, 24 insertions(+) > >>>> > >>>>diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/powerpc/platforms/pseries/eeh_pseries.c > >>>>index 6b812ad990e4..45946ee90985 100644 > >>>>--- a/arch/powerpc/platforms/pseries/eeh_pseries.c > >>>>+++ b/arch/powerpc/platforms/pseries/eeh_pseries.c > >>>>@@ -64,6 +64,27 @@ static unsigned char slot_errbuf[RTAS_ERROR_LOG_MAX]; > >>>> static DEFINE_SPINLOCK(slot_errbuf_lock); > >>>> static int eeh_error_buf_size; > >>>>+void pseries_pcibios_bus_add_device(struct pci_dev *pdev) > >>>>+{ > >>>>+ struct pci_dn *pdn = pci_get_pdn(pdev); > >>>>+ > >>>>+ if (!pdev->is_virtfn) > >>>>+ return; > >>>>+ > >>>>+ pdn->device_id = pdev->device; > >>>>+ pdn->vendor_id = pdev->vendor; > >>>>+ pdn->class_code = pdev->class; > >>>>+ > >>>>+ /* > >>>>+ * The following operations will fail if VF's sysfs files > >>>>+ * aren't created or its resources aren't finalized. > >>>>+ */ > >>>>+ eeh_add_device_early(pdn); > >>>>+ eeh_add_device_late(pdev); > >>>>+ eeh_sysfs_add_device(pdev); > >>>>+ pdev->match_driver = -1; > >>>match_driver is a bool, which should be assigned "true" or "false". > >>Above he mentioned a dependency on: > >> > >> [04/10] PCI: extend pci device match_driver state > >> https://patchwork.kernel.org/patch/9882915/ > >> > >> > >>Which makes it an int. > >Oh, right, I missed that, thanks. > > > >>Or has that patch been rejected or something? > >I haven't *rejected* it, but it's low on my priority list, so you > >shouldn't depend on it unless it adds functionality you really need. > >If I did apply that particular patch, I would want some rework because > >it currently obfuscates the match_driver logic. There's no clue when > >reading the code what -1/0/1 mean. > So do you prefer enum's? - If so I can make a change for that. > >Apparently here you *do* want the "-1 means the PCI core will never > >set match_driver to 1" functionality, so maybe you do depend on it. > We depend on the patch because we want that ability to never set > match_driver, > for SRIOV on PowerVM. Is this really new PowerVM-specific functionality? ISTR recent discussions about inhibiting driver binding in a generic way, e.g., http://lkml.kernel.org/r/1490022874-54718-1-git-send-email-bodong@mellanox.com > >If that's the case, how to you ever bind a driver to these VFs? The > >changelog says you don't want VF drivers to load *immediately*, so I > >assume you do want them to load eventually. > > > The VF's that get dynamically created within the configure SR-IOV > call, on the Pseries Platform, wont be matched with a driver. - We > do not want it to match. > > The Power Hypervisor will load the VFs. The VF's will get > assigned(by the user) via the HMC or Novalink in this environment > which will then trigger PHYP to load the VF device node to the > device tree. I don't know what it means for the Hypervisor to "load the VFs." Can you explain that in PCI-speak? The things I know about are: - we set PCI_SRIOV_CTRL_VFE in the PF, which enables VFs - now the VFs respond to config accesses - the PCI core enumerates the VFs by reading their config space - the PCI core builds pci_dev structs for the VFs - the PCI core adds these pci_devs to the bus - we try to bind drivers to the VFs - the VF driver probe function may read VF config space and VF BARs - the VF may be assigned to a guest VM Where does "loading the VFs" fit in? I don't know what HMC, Novalink, or PHYP are. I don't *need* to know what they are, as long as you can explain what's happening in terms of the PCI concepts and generic Linux VMs and device assignment. Bjorn
On 2017-10-12 22:34, Bjorn Helgaas wrote: > [+cc Alex, Bodong, Eli, Saeed] > > On Thu, Oct 12, 2017 at 02:59:23PM -0500, Bryant G. Ly wrote: >> On 10/12/17 1:29 PM, Bjorn Helgaas wrote: >> >On Thu, Oct 12, 2017 at 03:09:53PM +1100, Michael Ellerman wrote: >> >>Bjorn Helgaas <helgaas@kernel.org> writes: >> >> >> >>>On Fri, Sep 22, 2017 at 09:19:28AM -0500, Bryant G. Ly wrote: >> >>>>This patch adds the machine dependent call for >> >>>>pcibios_bus_add_device, since the previous patch >> >>>>separated the calls out between the PowerNV and PowerVM. >> >>>> >> >>>>The difference here is that for the PowerVM environment >> >>>>we do not want match_driver set because in this environment >> >>>>we do not want the VF device drivers to load immediately, due to >> >>>>firmware loading the device node when VF device is assigned to the >> >>>>logical partition. >> >>>> >> >>>>This patch will depend on the patch linked below, which is under >> >>>>review. >> >>>> >> >>>>https://patchwork.kernel.org/patch/9882915/ >> >>>> >> >>>>Signed-off-by: Bryant G. Ly <bryantly@linux.vnet.ibm.com> >> >>>>Signed-off-by: Juan J. Alvarez <jjalvare@us.ibm.com> >> >>>>--- >> >>>> arch/powerpc/platforms/pseries/eeh_pseries.c | 24 ++++++++++++++++++++++++ >> >>>> 1 file changed, 24 insertions(+) >> >>>> >> >>>>diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/powerpc/platforms/pseries/eeh_pseries.c >> >>>>index 6b812ad990e4..45946ee90985 100644 >> >>>>--- a/arch/powerpc/platforms/pseries/eeh_pseries.c >> >>>>+++ b/arch/powerpc/platforms/pseries/eeh_pseries.c >> >>>>@@ -64,6 +64,27 @@ static unsigned char slot_errbuf[RTAS_ERROR_LOG_MAX]; >> >>>> static DEFINE_SPINLOCK(slot_errbuf_lock); >> >>>> static int eeh_error_buf_size; >> >>>>+void pseries_pcibios_bus_add_device(struct pci_dev *pdev) >> >>>>+{ >> >>>>+ struct pci_dn *pdn = pci_get_pdn(pdev); >> >>>>+ >> >>>>+ if (!pdev->is_virtfn) >> >>>>+ return; >> >>>>+ >> >>>>+ pdn->device_id = pdev->device; >> >>>>+ pdn->vendor_id = pdev->vendor; >> >>>>+ pdn->class_code = pdev->class; >> >>>>+ >> >>>>+ /* >> >>>>+ * The following operations will fail if VF's sysfs files >> >>>>+ * aren't created or its resources aren't finalized. >> >>>>+ */ >> >>>>+ eeh_add_device_early(pdn); >> >>>>+ eeh_add_device_late(pdev); >> >>>>+ eeh_sysfs_add_device(pdev); >> >>>>+ pdev->match_driver = -1; >> >>>match_driver is a bool, which should be assigned "true" or "false". >> >>Above he mentioned a dependency on: >> >> >> >> [04/10] PCI: extend pci device match_driver state >> >> https://patchwork.kernel.org/patch/9882915/ >> >> >> >> >> >>Which makes it an int. >> >Oh, right, I missed that, thanks. >> > >> >>Or has that patch been rejected or something? >> >I haven't *rejected* it, but it's low on my priority list, so you >> >shouldn't depend on it unless it adds functionality you really need. >> >If I did apply that particular patch, I would want some rework because >> >it currently obfuscates the match_driver logic. There's no clue when >> >reading the code what -1/0/1 mean. >> So do you prefer enum's? - If so I can make a change for that. >> >Apparently here you *do* want the "-1 means the PCI core will never >> >set match_driver to 1" functionality, so maybe you do depend on it. >> We depend on the patch because we want that ability to never set >> match_driver, >> for SRIOV on PowerVM. > > Is this really new PowerVM-specific functionality? ISTR recent > discussions > about inhibiting driver binding in a generic way, e.g., > http://lkml.kernel.org/r/1490022874-54718-1-git-send-email-bodong@mellanox.com > >> >If that's the case, how to you ever bind a driver to these VFs? The >> >changelog says you don't want VF drivers to load *immediately*, so I >> >assume you do want them to load eventually. >> > >> The VF's that get dynamically created within the configure SR-IOV >> call, on the Pseries Platform, wont be matched with a driver. - We >> do not want it to match. >> >> The Power Hypervisor will load the VFs. The VF's will get >> assigned(by the user) via the HMC or Novalink in this environment >> which will then trigger PHYP to load the VF device node to the >> device tree. > > I don't know what it means for the Hypervisor to "load the VFs." Can > you explain that in PCI-speak? > > The things I know about are: > > - we set PCI_SRIOV_CTRL_VFE in the PF, which enables VFs > - now the VFs respond to config accesses > - the PCI core enumerates the VFs by reading their config space > - the PCI core builds pci_dev structs for the VFs > - the PCI core adds these pci_devs to the bus > - we try to bind drivers to the VFs > - the VF driver probe function may read VF config space and VF BARs > - the VF may be assigned to a guest VM > > Where does "loading the VFs" fit in? I don't know what HMC, Novalink, > or PHYP are. I don't *need* to know what they are, as long as you can > explain what's happening in terms of the PCI concepts and generic Linux > VMs > and device assignment. > > Bjorn The VFs will be hotplugged into the VM separately from the enable SR-IOV, so the driver will load as part of the hotplug operation. Steve
On 2017-10-13 06:53, Steven Royer wrote: > On 2017-10-12 22:34, Bjorn Helgaas wrote: >> [+cc Alex, Bodong, Eli, Saeed] >> >> On Thu, Oct 12, 2017 at 02:59:23PM -0500, Bryant G. Ly wrote: >>> On 10/12/17 1:29 PM, Bjorn Helgaas wrote: >>> >On Thu, Oct 12, 2017 at 03:09:53PM +1100, Michael Ellerman wrote: >>> >>Bjorn Helgaas <helgaas@kernel.org> writes: >>> >> >>> >>>On Fri, Sep 22, 2017 at 09:19:28AM -0500, Bryant G. Ly wrote: >>> >>>>This patch adds the machine dependent call for >>> >>>>pcibios_bus_add_device, since the previous patch >>> >>>>separated the calls out between the PowerNV and PowerVM. >>> >>>> >>> >>>>The difference here is that for the PowerVM environment >>> >>>>we do not want match_driver set because in this environment >>> >>>>we do not want the VF device drivers to load immediately, due to >>> >>>>firmware loading the device node when VF device is assigned to the >>> >>>>logical partition. >>> >>>> >>> >>>>This patch will depend on the patch linked below, which is under >>> >>>>review. >>> >>>> >>> >>>>https://patchwork.kernel.org/patch/9882915/ >>> >>>> >>> >>>>Signed-off-by: Bryant G. Ly <bryantly@linux.vnet.ibm.com> >>> >>>>Signed-off-by: Juan J. Alvarez <jjalvare@us.ibm.com> >>> >>>>--- >>> >>>> arch/powerpc/platforms/pseries/eeh_pseries.c | 24 ++++++++++++++++++++++++ >>> >>>> 1 file changed, 24 insertions(+) >>> >>>> >>> >>>>diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/powerpc/platforms/pseries/eeh_pseries.c >>> >>>>index 6b812ad990e4..45946ee90985 100644 >>> >>>>--- a/arch/powerpc/platforms/pseries/eeh_pseries.c >>> >>>>+++ b/arch/powerpc/platforms/pseries/eeh_pseries.c >>> >>>>@@ -64,6 +64,27 @@ static unsigned char slot_errbuf[RTAS_ERROR_LOG_MAX]; >>> >>>> static DEFINE_SPINLOCK(slot_errbuf_lock); >>> >>>> static int eeh_error_buf_size; >>> >>>>+void pseries_pcibios_bus_add_device(struct pci_dev *pdev) >>> >>>>+{ >>> >>>>+ struct pci_dn *pdn = pci_get_pdn(pdev); >>> >>>>+ >>> >>>>+ if (!pdev->is_virtfn) >>> >>>>+ return; >>> >>>>+ >>> >>>>+ pdn->device_id = pdev->device; >>> >>>>+ pdn->vendor_id = pdev->vendor; >>> >>>>+ pdn->class_code = pdev->class; >>> >>>>+ >>> >>>>+ /* >>> >>>>+ * The following operations will fail if VF's sysfs files >>> >>>>+ * aren't created or its resources aren't finalized. >>> >>>>+ */ >>> >>>>+ eeh_add_device_early(pdn); >>> >>>>+ eeh_add_device_late(pdev); >>> >>>>+ eeh_sysfs_add_device(pdev); >>> >>>>+ pdev->match_driver = -1; >>> >>>match_driver is a bool, which should be assigned "true" or "false". >>> >>Above he mentioned a dependency on: >>> >> >>> >> [04/10] PCI: extend pci device match_driver state >>> >> https://patchwork.kernel.org/patch/9882915/ >>> >> >>> >> >>> >>Which makes it an int. >>> >Oh, right, I missed that, thanks. >>> > >>> >>Or has that patch been rejected or something? >>> >I haven't *rejected* it, but it's low on my priority list, so you >>> >shouldn't depend on it unless it adds functionality you really need. >>> >If I did apply that particular patch, I would want some rework because >>> >it currently obfuscates the match_driver logic. There's no clue when >>> >reading the code what -1/0/1 mean. >>> So do you prefer enum's? - If so I can make a change for that. >>> >Apparently here you *do* want the "-1 means the PCI core will never >>> >set match_driver to 1" functionality, so maybe you do depend on it. >>> We depend on the patch because we want that ability to never set >>> match_driver, >>> for SRIOV on PowerVM. >> >> Is this really new PowerVM-specific functionality? ISTR recent >> discussions >> about inhibiting driver binding in a generic way, e.g., >> http://lkml.kernel.org/r/1490022874-54718-1-git-send-email-bodong@mellanox.com >> >>> >If that's the case, how to you ever bind a driver to these VFs? The >>> >changelog says you don't want VF drivers to load *immediately*, so I >>> >assume you do want them to load eventually. >>> > >>> The VF's that get dynamically created within the configure SR-IOV >>> call, on the Pseries Platform, wont be matched with a driver. - We >>> do not want it to match. >>> >>> The Power Hypervisor will load the VFs. The VF's will get >>> assigned(by the user) via the HMC or Novalink in this environment >>> which will then trigger PHYP to load the VF device node to the >>> device tree. >> >> I don't know what it means for the Hypervisor to "load the VFs." Can >> you explain that in PCI-speak? >> >> The things I know about are: >> >> - we set PCI_SRIOV_CTRL_VFE in the PF, which enables VFs >> - now the VFs respond to config accesses >> - the PCI core enumerates the VFs by reading their config space >> - the PCI core builds pci_dev structs for the VFs >> - the PCI core adds these pci_devs to the bus >> - we try to bind drivers to the VFs >> - the VF driver probe function may read VF config space and VF BARs >> - the VF may be assigned to a guest VM >> >> Where does "loading the VFs" fit in? I don't know what HMC, Novalink, >> or PHYP are. I don't *need* to know what they are, as long as you can >> explain what's happening in terms of the PCI concepts and generic >> Linux VMs >> and device assignment. >> >> Bjorn > > The VFs will be hotplugged into the VM separately from the enable > SR-IOV, so the driver will load as part of the hotplug operation. > > Steve One more point of clarification: when the hotplug happens, the VF will show up on a virtual PCI bus that is not directly correlated to the real PCI bus that the PF is on. On that virtual PCI bus, the driver will match because it won't be set to -1. Steve
On Fri, 13 Oct 2017 07:01:48 -0500 Steven Royer <seroyer@linux.vnet.ibm.com> wrote: > On 2017-10-13 06:53, Steven Royer wrote: > > On 2017-10-12 22:34, Bjorn Helgaas wrote: > >> [+cc Alex, Bodong, Eli, Saeed] > >> > >> On Thu, Oct 12, 2017 at 02:59:23PM -0500, Bryant G. Ly wrote: > >>> On 10/12/17 1:29 PM, Bjorn Helgaas wrote: > >>> >On Thu, Oct 12, 2017 at 03:09:53PM +1100, Michael Ellerman wrote: > >>> >>Bjorn Helgaas <helgaas@kernel.org> writes: > >>> >> > >>> >>>On Fri, Sep 22, 2017 at 09:19:28AM -0500, Bryant G. Ly wrote: > >>> >>>>This patch adds the machine dependent call for > >>> >>>>pcibios_bus_add_device, since the previous patch > >>> >>>>separated the calls out between the PowerNV and PowerVM. > >>> >>>> > >>> >>>>The difference here is that for the PowerVM environment > >>> >>>>we do not want match_driver set because in this environment > >>> >>>>we do not want the VF device drivers to load immediately, due to > >>> >>>>firmware loading the device node when VF device is assigned to the > >>> >>>>logical partition. > >>> >>>> > >>> >>>>This patch will depend on the patch linked below, which is under > >>> >>>>review. > >>> >>>> > >>> >>>>https://patchwork.kernel.org/patch/9882915/ > >>> >>>> > >>> >>>>Signed-off-by: Bryant G. Ly <bryantly@linux.vnet.ibm.com> > >>> >>>>Signed-off-by: Juan J. Alvarez <jjalvare@us.ibm.com> > >>> >>>>--- > >>> >>>> arch/powerpc/platforms/pseries/eeh_pseries.c | 24 ++++++++++++++++++++++++ > >>> >>>> 1 file changed, 24 insertions(+) > >>> >>>> > >>> >>>>diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/powerpc/platforms/pseries/eeh_pseries.c > >>> >>>>index 6b812ad990e4..45946ee90985 100644 > >>> >>>>--- a/arch/powerpc/platforms/pseries/eeh_pseries.c > >>> >>>>+++ b/arch/powerpc/platforms/pseries/eeh_pseries.c > >>> >>>>@@ -64,6 +64,27 @@ static unsigned char slot_errbuf[RTAS_ERROR_LOG_MAX]; > >>> >>>> static DEFINE_SPINLOCK(slot_errbuf_lock); > >>> >>>> static int eeh_error_buf_size; > >>> >>>>+void pseries_pcibios_bus_add_device(struct pci_dev *pdev) > >>> >>>>+{ > >>> >>>>+ struct pci_dn *pdn = pci_get_pdn(pdev); > >>> >>>>+ > >>> >>>>+ if (!pdev->is_virtfn) > >>> >>>>+ return; > >>> >>>>+ > >>> >>>>+ pdn->device_id = pdev->device; > >>> >>>>+ pdn->vendor_id = pdev->vendor; > >>> >>>>+ pdn->class_code = pdev->class; > >>> >>>>+ > >>> >>>>+ /* > >>> >>>>+ * The following operations will fail if VF's sysfs files > >>> >>>>+ * aren't created or its resources aren't finalized. > >>> >>>>+ */ > >>> >>>>+ eeh_add_device_early(pdn); > >>> >>>>+ eeh_add_device_late(pdev); > >>> >>>>+ eeh_sysfs_add_device(pdev); > >>> >>>>+ pdev->match_driver = -1; > >>> >>>match_driver is a bool, which should be assigned "true" or "false". > >>> >>Above he mentioned a dependency on: > >>> >> > >>> >> [04/10] PCI: extend pci device match_driver state > >>> >> https://patchwork.kernel.org/patch/9882915/ > >>> >> > >>> >> > >>> >>Which makes it an int. > >>> >Oh, right, I missed that, thanks. > >>> > > >>> >>Or has that patch been rejected or something? > >>> >I haven't *rejected* it, but it's low on my priority list, so you > >>> >shouldn't depend on it unless it adds functionality you really need. > >>> >If I did apply that particular patch, I would want some rework because > >>> >it currently obfuscates the match_driver logic. There's no clue when > >>> >reading the code what -1/0/1 mean. > >>> So do you prefer enum's? - If so I can make a change for that. > >>> >Apparently here you *do* want the "-1 means the PCI core will never > >>> >set match_driver to 1" functionality, so maybe you do depend on it. > >>> We depend on the patch because we want that ability to never set > >>> match_driver, > >>> for SRIOV on PowerVM. > >> > >> Is this really new PowerVM-specific functionality? ISTR recent > >> discussions > >> about inhibiting driver binding in a generic way, e.g., > >> http://lkml.kernel.org/r/1490022874-54718-1-git-send-email-bodong@mellanox.com > >> > >>> >If that's the case, how to you ever bind a driver to these VFs? The > >>> >changelog says you don't want VF drivers to load *immediately*, so I > >>> >assume you do want them to load eventually. > >>> > > >>> The VF's that get dynamically created within the configure SR-IOV > >>> call, on the Pseries Platform, wont be matched with a driver. - We > >>> do not want it to match. > >>> > >>> The Power Hypervisor will load the VFs. The VF's will get > >>> assigned(by the user) via the HMC or Novalink in this environment > >>> which will then trigger PHYP to load the VF device node to the > >>> device tree. > >> > >> I don't know what it means for the Hypervisor to "load the VFs." Can > >> you explain that in PCI-speak? > >> > >> The things I know about are: > >> > >> - we set PCI_SRIOV_CTRL_VFE in the PF, which enables VFs > >> - now the VFs respond to config accesses > >> - the PCI core enumerates the VFs by reading their config space > >> - the PCI core builds pci_dev structs for the VFs > >> - the PCI core adds these pci_devs to the bus > >> - we try to bind drivers to the VFs > >> - the VF driver probe function may read VF config space and VF BARs > >> - the VF may be assigned to a guest VM > >> > >> Where does "loading the VFs" fit in? I don't know what HMC, Novalink, > >> or PHYP are. I don't *need* to know what they are, as long as you can > >> explain what's happening in terms of the PCI concepts and generic > >> Linux VMs > >> and device assignment. > >> > >> Bjorn > > > > The VFs will be hotplugged into the VM separately from the enable > > SR-IOV, so the driver will load as part of the hotplug operation. > > > > Steve > > One more point of clarification: when the hotplug happens, the VF will > show up on a virtual PCI bus that is not directly correlated to the real > PCI bus that the PF is on. On that virtual PCI bus, the driver will > match because it won't be set to -1. I'm pretty lost too, but I think what's being said is that the paravirtualized SR-IOV enable creates VFs according to the SR-IOV offset and stride capabilities of the PF, but we're supposed to ignore those (why are we even creating pci_devs for them?) and the hypervisor will actually hotplug the VFs somewhere else. How's that still SR-IOV? Why wouldn't the hypervisor just add the real VFs that we're supposed to use at the offset and stride indicated by the PF SR-IOV capability and mask the VFs that we're not supposed to see? Thanks, Alex
On 10/13/17 1:05 PM, Alex Williamson wrote: > On Fri, 13 Oct 2017 07:01:48 -0500 > Steven Royer <seroyer@linux.vnet.ibm.com> wrote: > >> On 2017-10-13 06:53, Steven Royer wrote: >>> On 2017-10-12 22:34, Bjorn Helgaas wrote: >>>> [+cc Alex, Bodong, Eli, Saeed] >>>> >>>> On Thu, Oct 12, 2017 at 02:59:23PM -0500, Bryant G. Ly wrote: >>>>> On 10/12/17 1:29 PM, Bjorn Helgaas wrote: >>>>>> On Thu, Oct 12, 2017 at 03:09:53PM +1100, Michael Ellerman wrote: >>>>>>> Bjorn Helgaas <helgaas@kernel.org> writes: >>>>>>> >>>>>>>> On Fri, Sep 22, 2017 at 09:19:28AM -0500, Bryant G. Ly wrote: >>>>>>>>> This patch adds the machine dependent call for >>>>>>>>> pcibios_bus_add_device, since the previous patch >>>>>>>>> separated the calls out between the PowerNV and PowerVM. >>>>>>>>> >>>>>>>>> The difference here is that for the PowerVM environment >>>>>>>>> we do not want match_driver set because in this environment >>>>>>>>> we do not want the VF device drivers to load immediately, due to >>>>>>>>> firmware loading the device node when VF device is assigned to the >>>>>>>>> logical partition. >>>>>>>>> >>>>>>>>> This patch will depend on the patch linked below, which is under >>>>>>>>> review. >>>>>>>>> >>>>>>>>> https://patchwork.kernel.org/patch/9882915/ >>>>>>>>> >>>>>>>>> Signed-off-by: Bryant G. Ly <bryantly@linux.vnet.ibm.com> >>>>>>>>> Signed-off-by: Juan J. Alvarez <jjalvare@us.ibm.com> >>>>>>>>> --- >>>>>>>>> arch/powerpc/platforms/pseries/eeh_pseries.c | 24 ++++++++++++++++++++++++ >>>>>>>>> 1 file changed, 24 insertions(+) >>>>>>>>> >>>>>>>>> diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/powerpc/platforms/pseries/eeh_pseries.c >>>>>>>>> index 6b812ad990e4..45946ee90985 100644 >>>>>>>>> --- a/arch/powerpc/platforms/pseries/eeh_pseries.c >>>>>>>>> +++ b/arch/powerpc/platforms/pseries/eeh_pseries.c >>>>>>>>> @@ -64,6 +64,27 @@ static unsigned char slot_errbuf[RTAS_ERROR_LOG_MAX]; >>>>>>>>> static DEFINE_SPINLOCK(slot_errbuf_lock); >>>>>>>>> static int eeh_error_buf_size; >>>>>>>>> +void pseries_pcibios_bus_add_device(struct pci_dev *pdev) >>>>>>>>> +{ >>>>>>>>> + struct pci_dn *pdn = pci_get_pdn(pdev); >>>>>>>>> + >>>>>>>>> + if (!pdev->is_virtfn) >>>>>>>>> + return; >>>>>>>>> + >>>>>>>>> + pdn->device_id = pdev->device; >>>>>>>>> + pdn->vendor_id = pdev->vendor; >>>>>>>>> + pdn->class_code = pdev->class; >>>>>>>>> + >>>>>>>>> + /* >>>>>>>>> + * The following operations will fail if VF's sysfs files >>>>>>>>> + * aren't created or its resources aren't finalized. >>>>>>>>> + */ >>>>>>>>> + eeh_add_device_early(pdn); >>>>>>>>> + eeh_add_device_late(pdev); >>>>>>>>> + eeh_sysfs_add_device(pdev); >>>>>>>>> + pdev->match_driver = -1; >>>>>>>> match_driver is a bool, which should be assigned "true" or "false". >>>>>>> Above he mentioned a dependency on: >>>>>>> >>>>>>> [04/10] PCI: extend pci device match_driver state >>>>>>> https://patchwork.kernel.org/patch/9882915/ >>>>>>> >>>>>>> >>>>>>> Which makes it an int. >>>>>> Oh, right, I missed that, thanks. >>>>>> >>>>>>> Or has that patch been rejected or something? >>>>>> I haven't *rejected* it, but it's low on my priority list, so you >>>>>> shouldn't depend on it unless it adds functionality you really need. >>>>>> If I did apply that particular patch, I would want some rework because >>>>>> it currently obfuscates the match_driver logic. There's no clue when >>>>>> reading the code what -1/0/1 mean. >>>>> So do you prefer enum's? - If so I can make a change for that. >>>>>> Apparently here you *do* want the "-1 means the PCI core will never >>>>>> set match_driver to 1" functionality, so maybe you do depend on it. >>>>> We depend on the patch because we want that ability to never set >>>>> match_driver, >>>>> for SRIOV on PowerVM. >>>> Is this really new PowerVM-specific functionality? ISTR recent >>>> discussions >>>> about inhibiting driver binding in a generic way, e.g., >>>> http://lkml.kernel.org/r/1490022874-54718-1-git-send-email-bodong@mellanox.com >>>> >>>>>> If that's the case, how to you ever bind a driver to these VFs? The >>>>>> changelog says you don't want VF drivers to load *immediately*, so I >>>>>> assume you do want them to load eventually. >>>>>> >>>>> The VF's that get dynamically created within the configure SR-IOV >>>>> call, on the Pseries Platform, wont be matched with a driver. - We >>>>> do not want it to match. >>>>> >>>>> The Power Hypervisor will load the VFs. The VF's will get >>>>> assigned(by the user) via the HMC or Novalink in this environment >>>>> which will then trigger PHYP to load the VF device node to the >>>>> device tree. >>>> I don't know what it means for the Hypervisor to "load the VFs." Can >>>> you explain that in PCI-speak? >>>> >>>> The things I know about are: >>>> >>>> - we set PCI_SRIOV_CTRL_VFE in the PF, which enables VFs >>>> - now the VFs respond to config accesses >>>> - the PCI core enumerates the VFs by reading their config space >>>> - the PCI core builds pci_dev structs for the VFs >>>> - the PCI core adds these pci_devs to the bus >>>> - we try to bind drivers to the VFs >>>> - the VF driver probe function may read VF config space and VF BARs >>>> - the VF may be assigned to a guest VM >>>> >>>> Where does "loading the VFs" fit in? I don't know what HMC, Novalink, >>>> or PHYP are. I don't *need* to know what they are, as long as you can >>>> explain what's happening in terms of the PCI concepts and generic >>>> Linux VMs >>>> and device assignment. >>>> >>>> Bjorn >>> The VFs will be hotplugged into the VM separately from the enable >>> SR-IOV, so the driver will load as part of the hotplug operation. >>> >>> Steve >> One more point of clarification: when the hotplug happens, the VF will >> show up on a virtual PCI bus that is not directly correlated to the real >> PCI bus that the PF is on. On that virtual PCI bus, the driver will >> match because it won't be set to -1. So lets refer to Bjorn's list of things for SRIOV. - we set PCI_SRIOV_CTRL_VFE in the PF, which enables VFs - now the VFs respond to config accesses - the PCI core enumerates the VFs by reading their config space - the PCI core builds pci_dev structs for the VFs - the PCI core adds these pci_devs to the bus So everything is the same up to here. - we try to bind drivers to the VFs - the VF driver probe function may read VF config space and VF BARs - the VF may be assigned to a guest VM PowerVM environment is very different than traditional KVM in terms of SRIOV. In our environment the VFs are not usable or view-able by the Hosting Partition in this case Linux. This is a very important point in that the Host CAN NOT do anything to any of the VFs available. So like existing way of enabling SRIOV we still rely on the PF driver to enable VFs - but in this case the attachment phase is done via a user action via a management console in our case (novalink or hmc) triggered event that will essentially act like a hotplug. So in the fine details of that user triggered action the system firmware will bind the VFs, allowing resources to be allocated to the VF. - Which essentially does all the attaching as we know it today but is managed by PHYP not by the kernel. > I'm pretty lost too, but I think what's being said is that the > paravirtualized SR-IOV enable creates VFs according to the SR-IOV > offset and stride capabilities of the PF, but we're supposed to ignore > those (why are we even creating pci_devs for them?) and the hypervisor > will actually hotplug the VFs somewhere else. How's that still > SR-IOV? Why wouldn't the hypervisor just add the real VFs that we're > supposed to use at the offset and stride indicated by the PF SR-IOV > capability and mask the VFs that we're not supposed to see? Thanks, > > Alex > In our current environment (PSeries) we still need to manage the VFs say when doing error recovery. Therefore, we need pci_devs to be mapped to system resources in this case a PE (Partitionable Endpoint). There are more patches that will map the system resources to a pci_dn structure. This patch was merely the start of separating PowerVM and PowerNV configure sriov call. -Bryant
"Bryant G. Ly" <bryantly@linux.vnet.ibm.com> writes: > On 10/12/17 1:29 PM, Bjorn Helgaas wrote: ... >> >> If that's the case, how to you ever bind a driver to these VFs? The >> changelog says you don't want VF drivers to load *immediately*, so I >> assume you do want them to load eventually. >> > The VF's that get dynamically created within the configure SR-IOV call, > on the Pseries Platform, wont be matched with a driver. - We do not > want it to match. > > The Power Hypervisor will load the VFs. The VF's will get assigned(by > the user) > via the HMC or Novalink in this environment which will > then trigger PHYP to load the VF device node to the device tree. What about the other "Power Hypervisor"? ie. KVM running on Power. We also use the pseries platform when running under KVM. cheers
On Fri, Oct 13, 2017 at 02:12:32PM -0500, Bryant G. Ly wrote: > > > On 10/13/17 1:05 PM, Alex Williamson wrote: > >On Fri, 13 Oct 2017 07:01:48 -0500 > >Steven Royer <seroyer@linux.vnet.ibm.com> wrote: > > > >>On 2017-10-13 06:53, Steven Royer wrote: > >>>On 2017-10-12 22:34, Bjorn Helgaas wrote: > >>>>[+cc Alex, Bodong, Eli, Saeed] > >>>> > >>>>On Thu, Oct 12, 2017 at 02:59:23PM -0500, Bryant G. Ly wrote: > >>>>>On 10/12/17 1:29 PM, Bjorn Helgaas wrote: > >>>>>>On Thu, Oct 12, 2017 at 03:09:53PM +1100, Michael Ellerman wrote: > >>>>>>>Bjorn Helgaas <helgaas@kernel.org> writes: > >>>>>>>>On Fri, Sep 22, 2017 at 09:19:28AM -0500, Bryant G. Ly wrote: > >>>>>>reading the code what -1/0/1 mean. > >>>>>>Apparently here you *do* want the "-1 means the PCI core will never > >>>>>>set match_driver to 1" functionality, so maybe you do depend on it. > >>>>>We depend on the patch because we want that ability to never set > >>>>>match_driver, > >>>>>for SRIOV on PowerVM. > >>>>Is this really new PowerVM-specific functionality? ISTR recent > >>>>discussions > >>>>about inhibiting driver binding in a generic way, e.g., > >>>>http://lkml.kernel.org/r/1490022874-54718-1-git-send-email-bodong@mellanox.com > >>>>>>If that's the case, how to you ever bind a driver to these VFs? The > >>>>>>changelog says you don't want VF drivers to load *immediately*, so I > >>>>>>assume you do want them to load eventually. > >>>>>The VF's that get dynamically created within the configure SR-IOV > >>>>>call, on the Pseries Platform, wont be matched with a driver. - We > >>>>>do not want it to match. > >>>>> > >>>>>The Power Hypervisor will load the VFs. The VF's will get > >>>>>assigned(by the user) via the HMC or Novalink in this environment > >>>>>which will then trigger PHYP to load the VF device node to the > >>>>>device tree. > >>>>I don't know what it means for the Hypervisor to "load the VFs." Can > >>>>you explain that in PCI-speak? > >>>> > >>>>The things I know about are: > >>>> > >>>> - we set PCI_SRIOV_CTRL_VFE in the PF, which enables VFs > >>>> - now the VFs respond to config accesses > >>>> - the PCI core enumerates the VFs by reading their config space > >>>> - the PCI core builds pci_dev structs for the VFs > >>>> - the PCI core adds these pci_devs to the bus > >>>> - we try to bind drivers to the VFs > >>>> - the VF driver probe function may read VF config space and VF BARs > >>>> - the VF may be assigned to a guest VM > >>>> > >>>>Where does "loading the VFs" fit in? I don't know what HMC, Novalink, > >>>>or PHYP are. I don't *need* to know what they are, as long as you can > >>>>explain what's happening in terms of the PCI concepts and generic > >>>>Linux VMs > >>>>and device assignment. > >>>> > >>>>Bjorn > >>>The VFs will be hotplugged into the VM separately from the enable > >>>SR-IOV, so the driver will load as part of the hotplug operation. > >>> > >>>Steve > >>One more point of clarification: when the hotplug happens, the VF will > >>show up on a virtual PCI bus that is not directly correlated to the real > >>PCI bus that the PF is on. On that virtual PCI bus, the driver will > >>match because it won't be set to -1. > So lets refer to Bjorn's list of things for SRIOV. > > - we set PCI_SRIOV_CTRL_VFE in the PF, which enables VFs > - now the VFs respond to config accesses > - the PCI core enumerates the VFs by reading their config space > - the PCI core builds pci_dev structs for the VFs > - the PCI core adds these pci_devs to the bus > > So everything is the same up to here. > - we try to bind drivers to the VFs > - the VF driver probe function may read VF config space and VF BARs > - the VF may be assigned to a guest VM > > PowerVM environment is very different than traditional KVM in terms > of SRIOV. In our environment the VFs are not usable or view-able by > the Hosting Partition in this case Linux. This is a very important > point in that the Host CAN NOT do anything to any of the VFs > available. This is where I get confused. I guess the Linux that sets PCI_SRIOV_CTRL_VFE to enable the VFs can also perform config accesses to the VFs, since it can enumerate them and build pci_dev structs for them, right? And the Linux in the "Hosting Partition" is a guest that cannot see a VF until a management console attaches the VF to the Hosting Partition? I'm not a VFIO or KVM expert but that sounds vaguely like what they would do when assigning a VF to a guest. > So like existing way of enabling SRIOV we still rely on the PF driver to > enable VFs - but in this case the attachment phase is done via a user > action via a management console in our case (novalink or hmc) triggered > event that will essentially act like a hotplug. > > So in the fine details of that user triggered action the system > firmware will bind the VFs, allowing resources to be allocated to > the VF. - Which essentially does all the attaching as we know it > today but is managed by PHYP not by the kernel. What exactly does "firmware binding the VFs" mean? I guess this must mean assigning a VF to a partition, injecting a hotplug add event to that partition, and making the VF visible in config space? Bjorn
Adding Juan back into the cc: jjalvare@linux.vnet.ibm.com On 10/16/17 10:38 PM, Michael Ellerman wrote: > "Bryant G. Ly" <bryantly@linux.vnet.ibm.com> writes: >> On 10/12/17 1:29 PM, Bjorn Helgaas wrote: > ... >>> If that's the case, how to you ever bind a driver to these VFs? The >>> changelog says you don't want VF drivers to load *immediately*, so I >>> assume you do want them to load eventually. >>> >> The VF's that get dynamically created within the configure SR-IOV call, >> on the Pseries Platform, wont be matched with a driver. - We do not >> want it to match. >> >> The Power Hypervisor will load the VFs. The VF's will get assigned(by >> the user) > via the HMC or Novalink in this environment which will >> then trigger PHYP to load the VF device node to the device tree. > What about the other "Power Hypervisor"? ie. KVM running on Power. This path is only exercised when configuring SR-IOV for Pseries LPAR, therefore it will not affect PowerNV or KVM(opal). Which is the reason for the separation of calls to the machine dependent stuff. > We also use the pseries platform when running under KVM. > > cheers > If anyone plans to enable SR-IOV on Pseries platform, firmware must provide the resources to enable the VFs and map them with system resources. A new version of the PAPR Document will be added to document these system resources. Lastly, we were not aware that there is an intention to enable SR-IOV in adapters assigned to a VM with Pseries running on KVM. Furthermore, this could be left as a todo for the future if this type of configuration is needed. -Bryant
On 10/17/17 8:51 AM, Bjorn Helgaas wrote: > This is where I get confused. I guess the Linux that sets > PCI_SRIOV_CTRL_VFE to enable the VFs can also perform config accesses > to the VFs, since it can enumerate them and build pci_dev structs for > them, right? Correct, we are not changing anything related to how the VF gets initialized in the kernel. > > And the Linux in the "Hosting Partition" is a guest that cannot see a > VF until a management console attaches the VF to the Hosting > Partition? Correct, this is how PHYP does normal adapter assignment. > I'm not a VFIO or KVM expert but that sounds vaguely like > what they would do when assigning a VF to a guest. I do not know the specifics on VFIO and KVM. However, in this case there is no KVM running on the Linux LPAR. PHYP owns all the system resources. > >> So like existing way of enabling SRIOV we still rely on the PF driver to >> enable VFs - but in this case the attachment phase is done via a user >> action via a management console in our case (novalink or hmc) triggered >> event that will essentially act like a hotplug. >> >> So in the fine details of that user triggered action the system >> firmware will bind the VFs, allowing resources to be allocated to >> the VF. - Which essentially does all the attaching as we know it >> today but is managed by PHYP not by the kernel. > What exactly does "firmware binding the VFs" mean? I guess this must > mean assigning a VF to a partition, injecting a hotplug add event to > that partition, and making the VF visible in config space? Firmware basically adds a device node to the device tree as defined in the (PAPR) Power Architecture Platform Requirements document. Juan
On Tue, Oct 17, 2017 at 09:33:34AM -0500, Juan Alvarez wrote: > On 10/17/17 8:51 AM, Bjorn Helgaas wrote: > > This is where I get confused. I guess the Linux that sets > > PCI_SRIOV_CTRL_VFE to enable the VFs can also perform config accesses > > to the VFs, since it can enumerate them and build pci_dev structs for > > them, right? > > Correct, we are not changing anything related to how the VF gets initialized > in the kernel. > > > > And the Linux in the "Hosting Partition" is a guest that cannot see a > > VF until a management console attaches the VF to the Hosting > > Partition? > > Correct, this is how PHYP does normal adapter assignment. > > > I'm not a VFIO or KVM expert but that sounds vaguely like > > what they would do when assigning a VF to a guest. > > I do not know the specifics on VFIO and KVM. However, in this > case there is no KVM running on the Linux LPAR. PHYP owns all > the system resources. To pop back up to the top of the stack, I think the main point of this patch is to keep from binding a driver to the VFs in the kernel that set PCI_SRIOV_CTRL_VFE. This patch does that by setting pdev->match_driver to -1 in powerpc-specific code. I think all systems, not just powerpc, want to prevent this VF driver binding, so I hope there's a generic solution that everybody can use. > >> So like existing way of enabling SRIOV we still rely on the PF driver to > >> enable VFs - but in this case the attachment phase is done via a user > >> action via a management console in our case (novalink or hmc) triggered > >> event that will essentially act like a hotplug. > >> > >> So in the fine details of that user triggered action the system > >> firmware will bind the VFs, allowing resources to be allocated to > >> the VF. - Which essentially does all the attaching as we know it > >> today but is managed by PHYP not by the kernel. > > > > What exactly does "firmware binding the VFs" mean? I guess this must > > mean assigning a VF to a partition, injecting a hotplug add event to > > that partition, and making the VF visible in config space? > > Firmware basically adds a device node to the device tree as defined > in the (PAPR) Power Architecture Platform Requirements document. From the point of view of the kernel that consumes this device tree and owns the VF, I guess this looks like a hot-add. It would be nice if this could be exposed to that kernel by having the firmware inject a normal PCIe hotplug interrupt, but I'm guessing it's not that simple. But if I understand correctly, this patch series isn't concerned with that kernel; it's concerned with the kernel that owns the PF and sets PCI_SRIOV_CTRL_VFE. Bjorn
On 10/17/17 11:26 AM, Bjorn Helgaas wrote: > To pop back up to the top of the stack, I think the main point of this > patch is to keep from binding a driver to the VFs in the kernel that > set PCI_SRIOV_CTRL_VFE. This patch does that by setting > pdev->match_driver to -1 in powerpc-specific code. Correct, to add to your comment, we will only add to the PSeries platform (in PowerPC) configure SR-IOV path. > I think all systems, not just powerpc, want to prevent this VF driver > binding, so I hope there's a generic solution that everybody can use. The patch that we made this change dependent on: https://patchwork.kernel.org/patch/9882915/ Is a generic form of not binding a pci device to a device driver and can be used in another environment if needed. > >>>> So like existing way of enabling SRIOV we still rely on the PF driver to >>>> enable VFs - but in this case the attachment phase is done via a user >>>> action via a management console in our case (novalink or hmc) triggered >>>> event that will essentially act like a hotplug. >>>> >>>> So in the fine details of that user triggered action the system >>>> firmware will bind the VFs, allowing resources to be allocated to >>>> the VF. - Which essentially does all the attaching as we know it >>>> today but is managed by PHYP not by the kernel. >>> What exactly does "firmware binding the VFs" mean? I guess this must >>> mean assigning a VF to a partition, injecting a hotplug add event to >>> that partition, and making the VF visible in config space? >> Firmware basically adds a device node to the device tree as defined >> in the (PAPR) Power Architecture Platform Requirements document. > From the point of view of the kernel that consumes this device tree > and owns the VF, I guess this looks like a hot-add. Correct, this is intended. We want to minimize change and only focus on configure SR-IOV path in the PF on PSeries platform. > It would be nice > if this could be exposed to that kernel by having the firmware inject > a normal PCIe hotplug interrupt, but I'm guessing it's not that > simple. I don't understand entirely your suggestion. However, in the case of interrupts PHYP owns all the resources and will be associated accordingly with a partitionable endpoint (PE). > But if I understand correctly, this patch series isn't concerned with > that kernel; it's concerned with the kernel that owns the PF and sets > PCI_SRIOV_CTRL_VFE. Correct, we will enable SR-IOV in the PF through the Linux kernel and map system resources for the new VFs in powerpc platform specific kernel code. Furthermore, not binding the device drivers for the VFs that get mapped within the configure SR-IOV path is a requirement for this PSeries SR-IOV environment. Juan J. Alvarez
On Tue, Oct 17, 2017 at 12:23:01PM -0500, Juan Alvarez wrote: > On 10/17/17 11:26 AM, Bjorn Helgaas wrote: > > To pop back up to the top of the stack, I think the main point of this > > patch is to keep from binding a driver to the VFs in the kernel that > > set PCI_SRIOV_CTRL_VFE. This patch does that by setting > > pdev->match_driver to -1 in powerpc-specific code. > Correct, to add to your comment, we will only add to the PSeries platform > (in PowerPC) configure SR-IOV path. > > I think all systems, not just powerpc, want to prevent this VF driver > > binding, so I hope there's a generic solution that everybody can use. > The patch that we made this change dependent on: > > https://patchwork.kernel.org/patch/9882915/ > > Is a generic form of not binding a pci device to a device driver > and can be used in another environment if needed. Right. But that patch isn't really on the path to inclusion (mainly because it's test framework and doesn't fix a bug or add support for new hardware or features). I'm suggesting that maybe there should be a generic way to prevent binding to VF devices that works for everybody and doesn't require any arch-specific code at all. > >>>> So like existing way of enabling SRIOV we still rely on the PF driver to > >>>> enable VFs - but in this case the attachment phase is done via a user > >>>> action via a management console in our case (novalink or hmc) triggered > >>>> event that will essentially act like a hotplug. > >>>> > >>>> So in the fine details of that user triggered action the system > >>>> firmware will bind the VFs, allowing resources to be allocated to > >>>> the VF. - Which essentially does all the attaching as we know it > >>>> today but is managed by PHYP not by the kernel. > >>> What exactly does "firmware binding the VFs" mean? I guess this must > >>> mean assigning a VF to a partition, injecting a hotplug add event to > >>> that partition, and making the VF visible in config space? > >> Firmware basically adds a device node to the device tree as defined > >> in the (PAPR) Power Architecture Platform Requirements document. > > From the point of view of the kernel that consumes this device tree > > and owns the VF, I guess this looks like a hot-add. > Correct, this is intended. We want to minimize change and only focus > on configure SR-IOV path in the PF on PSeries platform. > > It would be nice > > if this could be exposed to that kernel by having the firmware inject > > a normal PCIe hotplug interrupt, but I'm guessing it's not that > > simple. > > I don't understand entirely your suggestion. However, in the case of > interrupts PHYP owns all the resources and will be associated accordingly > with a partitionable endpoint (PE). Assume you have a Linux kernel, and PHYP assigns a VF to it. What happens in that Linux kernel? How does it discover this new device? I'm suggesting that it would be cool if that kernel received a hotplug interrupt from the Downstream Port leading to the new VF, and the pciehp driver could read the Slot Status register and see that "Presence Detect Changed" was set. If that happened, the pciehp driver should automatically enumerate the new device and there wouldn't be much if any powerpc-specific code in the path. Bjorn
On 10/17/17 1:52 PM, Bjorn Helgaas wrote: > Right. But that patch isn't really on the path to inclusion (mainly > because it's test framework and doesn't fix a bug or add support for > new hardware or features). I was not aware of this decision and this will cause changes to this patch. > > I'm suggesting that maybe there should be a generic way to prevent > binding to VF devices that works for everybody and doesn't require any > arch-specific code at all. The patch that you have suggested in kernel 4.12 is also a generic way. https://marc.info/?l=linux-kernel&m=149002335105804&w=2 Perhaps we can use the same constructs that this patch uses at our level. Nonetheless, that will take a rework to this patch and possibly an export of a function to set drivers_autoprobe globally. I know that was frowned upon on first email thread. Let me know if this is an acceptable solution. >>>>>> So like existing way of enabling SRIOV we still rely on the PF driver to >>>>>> enable VFs - but in this case the attachment phase is done via a user >>>>>> action via a management console in our case (novalink or hmc) triggered >>>>>> event that will essentially act like a hotplug. >>>>>> >>>>>> So in the fine details of that user triggered action the system >>>>>> firmware will bind the VFs, allowing resources to be allocated to >>>>>> the VF. - Which essentially does all the attaching as we know it >>>>>> today but is managed by PHYP not by the kernel. >>>>> What exactly does "firmware binding the VFs" mean? I guess this must >>>>> mean assigning a VF to a partition, injecting a hotplug add event to >>>>> that partition, and making the VF visible in config space? >>>> Firmware basically adds a device node to the device tree as defined >>>> in the (PAPR) Power Architecture Platform Requirements document. >>> From the point of view of the kernel that consumes this device tree >>> and owns the VF, I guess this looks like a hot-add. >> Correct, this is intended. We want to minimize change and only focus >> on configure SR-IOV path in the PF on PSeries platform. >>> It would be nice >>> if this could be exposed to that kernel by having the firmware inject >>> a normal PCIe hotplug interrupt, but I'm guessing it's not that >>> simple. >> I don't understand entirely your suggestion. However, in the case of >> interrupts PHYP owns all the resources and will be associated accordingly >> with a partitionable endpoint (PE). > Assume you have a Linux kernel, and PHYP assigns a VF to it. What > happens in that Linux kernel? When kernel boots it goes through the lists of buses in the device tree. For each (physical,virtual) bus the kernel reads the list of devices. After resources are read and assigned it will probe the devices. If the kernel is up and running then this will start its flow through the dlpar add bus code. Which then will read the device node and same operation as boot will entail of going through the list of devices under the bus. One should note that dynamically created VFs in configure SR-IOV path will have is_virtfn set and others do not, therefore pcibios_bus_add_device will only be exercised in certain cases. > How does it discover this new device? This is treated as a normal hot plug operation for the Operating System that the device gets assigned which can be IBMi, AIX or Linux in this environment. > > I'm suggesting that it would be cool if that kernel received a hotplug > interrupt from the Downstream Port leading to the new VF, and the > pciehp driver could read the Slot Status register and see that > "Presence Detect Changed" was set. If that happened, the pciehp > driver should automatically enumerate the new device and there > wouldn't be much if any powerpc-specific code in the path. To re-state Steve's earlier comment, PHYP will enumerate the virtual pci bus and is not directly correlated to the real PCI bus that the PF is on. We have no control of the pci bus enumeration for the device node added to the device tree. Juan J. Alvarez
On 18/10/17 01:11, Bryant G. Ly wrote: > Adding Juan back into the cc: jjalvare@linux.vnet.ibm.com > > > On 10/16/17 10:38 PM, Michael Ellerman wrote: >> "Bryant G. Ly" <bryantly@linux.vnet.ibm.com> writes: >>> On 10/12/17 1:29 PM, Bjorn Helgaas wrote: >> ... >>>> If that's the case, how to you ever bind a driver to these VFs? The >>>> changelog says you don't want VF drivers to load *immediately*, so I >>>> assume you do want them to load eventually. >>>> >>> The VF's that get dynamically created within the configure SR-IOV call, >>> on the Pseries Platform, wont be matched with a driver. - We do not >>> want it to match. >>> >>> The Power Hypervisor will load the VFs. The VF's will get assigned(by >>> the user) > via the HMC or Novalink in this environment which will >>> then trigger PHYP to load the VF device node to the device tree. >> What about the other "Power Hypervisor"? ie. KVM running on Power. > This path is only exercised when configuring SR-IOV for Pseries LPAR, > therefore it will not affect PowerNV or KVM(opal). PowerNV KVM guest is a pseries machine so this code will execute there. > Which is the reason for > the separation of calls to the machine dependent stuff. >> We also use the pseries platform when running under KVM. >> >> cheers >> > If anyone plans to enable SR-IOV on Pseries platform, firmware must provide the > resources to enable the VFs and map them with system resources. This is what the PowerNV platform does. > A new version > of the PAPR Document will be added to document these system resources. The guest simply gets yet another PCI device, how is IOV different here? In regard of EEH, the API does not change afaik, it is up to the hypervisor (KVM+QEMU) to handle IOV case correctly. > Lastly, > we were not aware that there is an intention to enable SR-IOV in adapters assigned > to a VM with Pseries running on KVM. There is no any special enablement of IOV for a VM on powernv, once configured in the powernv host, we can just pass VFs to QEMU and therefore to a pseries guest, it is just a normal PCI device. Do you assign a PF to a VM and create VFs from inside the VM? Or only pHyp is allowed to do that? Sorry, I know nothing about pHyp on this matter. > Furthermore, this could be left as a todo > for the future if this type of configuration is needed.
On 18/10/17 12:01, Alexey Kardashevskiy wrote: > On 18/10/17 01:11, Bryant G. Ly wrote: >> Adding Juan back into the cc: jjalvare@linux.vnet.ibm.com >> >> >> On 10/16/17 10:38 PM, Michael Ellerman wrote: >>> "Bryant G. Ly" <bryantly@linux.vnet.ibm.com> writes: >>>> On 10/12/17 1:29 PM, Bjorn Helgaas wrote: >>> ... >>>>> If that's the case, how to you ever bind a driver to these VFs? The >>>>> changelog says you don't want VF drivers to load *immediately*, so I >>>>> assume you do want them to load eventually. >>>>> >>>> The VF's that get dynamically created within the configure SR-IOV call, >>>> on the Pseries Platform, wont be matched with a driver. - We do not >>>> want it to match. >>>> >>>> The Power Hypervisor will load the VFs. The VF's will get assigned(by >>>> the user) > via the HMC or Novalink in this environment which will >>>> then trigger PHYP to load the VF device node to the device tree. >>> What about the other "Power Hypervisor"? ie. KVM running on Power. >> This path is only exercised when configuring SR-IOV for Pseries LPAR, >> therefore it will not affect PowerNV or KVM(opal). > > PowerNV KVM guest is a pseries machine so this code will execute there. > >> Which is the reason for >> the separation of calls to the machine dependent stuff. >>> We also use the pseries platform when running under KVM. >>> >>> cheers >>> >> If anyone plans to enable SR-IOV on Pseries platform, firmware must provide the >> resources to enable the VFs and map them with system resources. > > This is what the PowerNV platform does. > >> A new version >> of the PAPR Document will be added to document these system resources. > > The guest simply gets yet another PCI device, how is IOV different here? > > In regard of EEH, the API does not change afaik, it is up to the hypervisor > (KVM+QEMU) to handle IOV case correctly. > > >> Lastly, >> we were not aware that there is an intention to enable SR-IOV in adapters assigned >> to a VM with Pseries running on KVM. > > There is no any special enablement of IOV for a VM on powernv, once > configured in the powernv host, we can just pass VFs to QEMU and therefore > to a pseries guest, it is just a normal PCI device. > > Do you assign a PF to a VM and create VFs from inside the VM? Or only pHyp > is allowed to do that? Sorry, I know nothing about pHyp on this matter. > Never mind this last question, I've just read the entire thread about this hosting partition thing.
On 10/17/17 8:36 PM, Alexey Kardashevskiy wrote: > PowerNV KVM guest is a pseries machine so this code will execute there. > The configure sriov path will fail and not enable sriov if resources are not met. I.e. the IOV Bar is not set in PF IOV Resources, which in this case gets assigned by firmware. We have separated the calls to put PowerNV and PSeries as machine dependent calls. Furthermore, we are adding device node properties in the device tree to identify if this is an SR-IOV slot on Phyp Pseries platform. Verification will be in place to distinguish between platforms that support SR-IOV in PSeries. >> Which is the reason for >> the separation of calls to the machine dependent stuff. >>> We also use the pseries platform when running under KVM. >>> >>> cheers >>> >> If anyone plans to enable SR-IOV on Pseries platform, firmware must provide the >> resources to enable the VFs and map them with system resources. > This is what the PowerNV platform does. Again, we have separated the machine dependent calls to different platforms. In our case we don't use opal and our dependent on phyp to associate resources. > >> A new version >> of the PAPR Document will be added to document these system resources. > The guest simply gets yet another PCI device, how is IOV different here? > > In regard of EEH, the API does not change afaik, it is up to the hypervisor > (KVM+QEMU) to handle IOV case correctly. I don't understand your question entirely, can you rephrase?
On Tue, Oct 17, 2017 at 06:13:35PM -0500, Juan Alvarez wrote: > On 10/17/17 1:52 PM, Bjorn Helgaas wrote: > > I'm suggesting that maybe there should be a generic way to prevent > > binding to VF devices that works for everybody and doesn't require any > > arch-specific code at all. > > The patch that you have suggested in kernel 4.12 is also a generic way. > > https://marc.info/?l=linux-kernel&m=149002335105804&w=2 The patch mentioned above is now upstream: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=0e7df22401a3 > Perhaps we can use the same constructs that this patch uses at our level. > Nonetheless, that will take a rework to this patch and possibly an export > of a function to set drivers_autoprobe globally. I know that was frowned > upon on first email thread. Let me know if this is an acceptable > solution. I would definitely like to see you use a solution that is somehow connected with the sriov->drivers_autoprobe mechanism added by 0e7df22401a3. 0e7df22401a3 is a per-PF solution, and you're looking for a system-wide switch. I could imagine a system-wide flag exposed in sysfs and used by pci_device_can_probe(). Or maybe some arch hook in the pci_device_add() path that clears sriov->drivers_autoprobe for every PF. I think the important thing is that it is coordinated as much as possible with other driver binding controls and not unnecessarily arch-dependent. Bjorn
diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/powerpc/platforms/pseries/eeh_pseries.c index 6b812ad990e4..45946ee90985 100644 --- a/arch/powerpc/platforms/pseries/eeh_pseries.c +++ b/arch/powerpc/platforms/pseries/eeh_pseries.c @@ -64,6 +64,27 @@ static unsigned char slot_errbuf[RTAS_ERROR_LOG_MAX]; static DEFINE_SPINLOCK(slot_errbuf_lock); static int eeh_error_buf_size; +void pseries_pcibios_bus_add_device(struct pci_dev *pdev) +{ + struct pci_dn *pdn = pci_get_pdn(pdev); + + if (!pdev->is_virtfn) + return; + + pdn->device_id = pdev->device; + pdn->vendor_id = pdev->vendor; + pdn->class_code = pdev->class; + + /* + * The following operations will fail if VF's sysfs files + * aren't created or its resources aren't finalized. + */ + eeh_add_device_early(pdn); + eeh_add_device_late(pdev); + eeh_sysfs_add_device(pdev); + pdev->match_driver = -1; +} + /** * pseries_eeh_init - EEH platform dependent initialization * @@ -120,6 +141,9 @@ static int pseries_eeh_init(void) /* Set EEH probe mode */ eeh_add_flag(EEH_PROBE_MODE_DEVTREE | EEH_ENABLE_IO_FOR_LOG); + /* Set EEH machine dependent code */ + ppc_md.pcibios_bus_add_device = pseries_pcibios_bus_add_device; + return 0; }