[PATCH v3 0/2] Prepartion for SR-IOV PowerVM Enablement

[PATCH v3 0/2] Prepartion for SR-IOV PowerVM Enablement

[Bryant G. Ly]

This patch series is to prepare for enabling SR-IOV
on pseries. It separates the calls to be machine dependent
and does not change any current functionality.

The patch linked below which is currently in review is a dependency:
https://patchwork.kernel.org/patch/9882915/

v1 - Initial patch
v2 - Addressed Bjorn's comment on creating a highly platform
     dependent global exported symbol.
v3 - Based patch off linux-ppc/master

Bryant G. Ly (2):
  powerpc/kernel: Separate SR-IOV Calls
  pseries/eeh: Add Pseries pcibios_bus_add_device

 arch/powerpc/include/asm/machdep.h           |  7 ++++++
 arch/powerpc/include/asm/pci-bridge.h        |  4 +---
 arch/powerpc/kernel/eeh_driver.c             |  4 ++--
 arch/powerpc/kernel/pci-common.c             | 23 +++++++++++++++++++
 arch/powerpc/kernel/pci_dn.c                 |  6 -----
 arch/powerpc/platforms/powernv/eeh-powernv.c | 33 ++++++++++++++--------------
 arch/powerpc/platforms/powernv/pci-ioda.c    |  6 +++--
 arch/powerpc/platforms/pseries/eeh_pseries.c | 24 ++++++++++++++++++++
 8 files changed, 78 insertions(+), 29 deletions(-)

-- 
2.11.0 (Apple Git-81)

[PATCH v3 1/2] powerpc/kernel: Separate SR-IOV Calls

[Bryant G. Ly]

SR-IOV can now be enabled in PowerNV platforms and Pseries
platforms. Therefore, the appropriate calls were moved to
machine dependent code instead of definition at compile time.

Signed-off-by: Bryant G. Ly <bryantly@linux.vnet.ibm.com>
Signed-off-by: Juan J. Alvarez <jjalvare@us.ibm.com>
---
 arch/powerpc/include/asm/machdep.h           |  7 ++++++
 arch/powerpc/include/asm/pci-bridge.h        |  4 +---
 arch/powerpc/kernel/eeh_driver.c             |  4 ++--
 arch/powerpc/kernel/pci-common.c             | 23 +++++++++++++++++++
 arch/powerpc/kernel/pci_dn.c                 |  6 -----
 arch/powerpc/platforms/powernv/eeh-powernv.c | 33 ++++++++++++++--------------
 arch/powerpc/platforms/powernv/pci-ioda.c    |  6 +++--
 7 files changed, 54 insertions(+), 29 deletions(-)

diff --git a/arch/powerpc/include/asm/machdep.h b/arch/powerpc/include/asm/machdep.h
index 73b92017b6d7..20f68d36af8c 100644
--- a/arch/powerpc/include/asm/machdep.h
+++ b/arch/powerpc/include/asm/machdep.h
@@ -172,11 +172,18 @@ struct machdep_calls {
 	/* Called after scan and before resource survey */
 	void (*pcibios_fixup_phb)(struct pci_controller *hose);
 
+	/* Called after device has been added to bus and
+	 * before sysfs has been created
+	 */
+	void (*pcibios_bus_add_device)(struct pci_dev *pdev);
+
 	resource_size_t (*pcibios_default_alignment)(void);
 
 #ifdef CONFIG_PCI_IOV
 	void (*pcibios_fixup_sriov)(struct pci_dev *pdev);
 	resource_size_t (*pcibios_iov_resource_alignment)(struct pci_dev *, int resno);
+	int (*pcibios_sriov_enable)(struct pci_dev *pdev, u16 num_vfs);
+	int (*pcibios_sriov_disable)(struct pci_dev *pdev);
 #endif /* CONFIG_PCI_IOV */
 
 	/* Called to shutdown machine specific hardware not already controlled
diff --git a/arch/powerpc/include/asm/pci-bridge.h b/arch/powerpc/include/asm/pci-bridge.h
index 0b8aa1fe2d5f..323628ca4d6d 100644
--- a/arch/powerpc/include/asm/pci-bridge.h
+++ b/arch/powerpc/include/asm/pci-bridge.h
@@ -203,10 +203,9 @@ struct pci_dn {
 	struct eeh_dev *edev;		/* eeh device */
 #endif
 #define IODA_INVALID_PE		0xFFFFFFFF
-#ifdef CONFIG_PPC_POWERNV
 	unsigned int pe_number;
-	int     vf_index;		/* VF index in the PF */
 #ifdef CONFIG_PCI_IOV
+	int     vf_index;		/* VF index in the PF */
 	u16     vfs_expanded;		/* number of VFs IOV BAR expanded */
 	u16     num_vfs;		/* number of VFs enabled*/
 	unsigned int *pe_num_map;	/* PE# for the first VF PE or array */
@@ -215,7 +214,6 @@ struct pci_dn {
 	int     (*m64_map)[PCI_SRIOV_NUM_BARS];
 #endif /* CONFIG_PCI_IOV */
 	int	mps;			/* Maximum Payload Size */
-#endif
 	struct list_head child_list;
 	struct list_head list;
 };
diff --git a/arch/powerpc/kernel/eeh_driver.c b/arch/powerpc/kernel/eeh_driver.c
index 8b840191df59..f2d1b369974d 100644
--- a/arch/powerpc/kernel/eeh_driver.c
+++ b/arch/powerpc/kernel/eeh_driver.c
@@ -440,7 +440,7 @@ static void *eeh_add_virt_device(void *data, void *userdata)
 			return NULL;
 	}
 
-#ifdef CONFIG_PPC_POWERNV
+#ifdef CONFIG_PCI_IOV
 	pci_iov_add_virtfn(edev->physfn, pdn->vf_index, 0);
 #endif
 	return NULL;
@@ -496,7 +496,7 @@ static void *eeh_rmv_device(void *data, void *userdata)
 		(*removed)++;
 
 	if (edev->physfn) {
-#ifdef CONFIG_PPC_POWERNV
+#ifdef CONFIG_PCI_IOV
 		struct pci_dn *pdn = eeh_dev_to_pdn(edev);
 
 		pci_iov_remove_virtfn(edev->physfn, pdn->vf_index, 0);
diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
index 02831a396419..d45b956d2e3a 100644
--- a/arch/powerpc/kernel/pci-common.c
+++ b/arch/powerpc/kernel/pci-common.c
@@ -249,8 +249,31 @@ resource_size_t pcibios_iov_resource_alignment(struct pci_dev *pdev, int resno)
 
 	return pci_iov_resource_size(pdev, resno);
 }
+
+int pcibios_sriov_enable(struct pci_dev *pdev, u16 num_vfs)
+{
+	if (ppc_md.pcibios_sriov_enable)
+		return ppc_md.pcibios_sriov_enable(pdev, num_vfs);
+
+	return 0;
+}
+
+int pcibios_sriov_disable(struct pci_dev *pdev)
+{
+	if (ppc_md.pcibios_sriov_disable)
+		return ppc_md.pcibios_sriov_disable(pdev);
+
+	return 0;
+}
+
 #endif /* CONFIG_PCI_IOV */
 
+void pcibios_bus_add_device(struct pci_dev *pdev)
+{
+	if (ppc_md.pcibios_bus_add_device)
+		ppc_md.pcibios_bus_add_device(pdev);
+}
+
 static resource_size_t pcibios_io_size(const struct pci_controller *hose)
 {
 #ifdef CONFIG_PPC64
diff --git a/arch/powerpc/kernel/pci_dn.c b/arch/powerpc/kernel/pci_dn.c
index 0e395afbf0f4..ab147a1909c8 100644
--- a/arch/powerpc/kernel/pci_dn.c
+++ b/arch/powerpc/kernel/pci_dn.c
@@ -156,10 +156,8 @@ static struct pci_dn *add_one_dev_pci_data(struct pci_dn *parent,
 	pdn->parent = parent;
 	pdn->busno = busno;
 	pdn->devfn = devfn;
-#ifdef CONFIG_PPC_POWERNV
 	pdn->vf_index = vf_index;
 	pdn->pe_number = IODA_INVALID_PE;
-#endif
 	INIT_LIST_HEAD(&pdn->child_list);
 	INIT_LIST_HEAD(&pdn->list);
 	list_add_tail(&pdn->list, &parent->child_list);
@@ -226,9 +224,7 @@ void remove_dev_pci_data(struct pci_dev *pdev)
 	 */
 	if (pdev->is_virtfn) {
 		pdn = pci_get_pdn(pdev);
-#ifdef CONFIG_PPC_POWERNV
 		pdn->pe_number = IODA_INVALID_PE;
-#endif
 		return;
 	}
 
@@ -294,9 +290,7 @@ struct pci_dn *pci_add_device_node_info(struct pci_controller *hose,
 		return NULL;
 	dn->data = pdn;
 	pdn->phb = hose;
-#ifdef CONFIG_PPC_POWERNV
 	pdn->pe_number = IODA_INVALID_PE;
-#endif
 	regs = of_get_property(dn, "reg", NULL);
 	if (regs) {
 		u32 addr = of_read_number(regs, 1);
diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c b/arch/powerpc/platforms/powernv/eeh-powernv.c
index 8864065eba22..9de55bcf484a 100644
--- a/arch/powerpc/platforms/powernv/eeh-powernv.c
+++ b/arch/powerpc/platforms/powernv/eeh-powernv.c
@@ -44,6 +44,22 @@
 static bool pnv_eeh_nb_init = false;
 static int eeh_event_irq = -EINVAL;
 
+void pnv_pcibios_bus_add_device(struct pci_dev *pdev)
+{
+	struct pci_dn *pdn = pci_get_pdn(pdev);
+
+	if (!pdev->is_virtfn)
+		return;
+
+	/*
+	 * 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);
+}
+
 static int pnv_eeh_init(void)
 {
 	struct pci_controller *hose;
@@ -87,6 +103,7 @@ static int pnv_eeh_init(void)
 	}
 
 	eeh_set_pe_aux_size(max_diag_size);
+	ppc_md.pcibios_bus_add_device = pnv_pcibios_bus_add_device;
 
 	return 0;
 }
@@ -1747,22 +1764,6 @@ static struct eeh_ops pnv_eeh_ops = {
 	.restore_config		= pnv_eeh_restore_config
 };
 
-void pcibios_bus_add_device(struct pci_dev *pdev)
-{
-	struct pci_dn *pdn = pci_get_pdn(pdev);
-
-	if (!pdev->is_virtfn)
-		return;
-
-	/*
-	 * 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);
-}
-
 #ifdef CONFIG_PCI_IOV
 static void pnv_pci_fixup_vf_mps(struct pci_dev *pdev)
 {
diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
index 57f9e55f4352..f7fed25e06ba 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
@@ -1674,7 +1674,7 @@ int pnv_pci_sriov_enable(struct pci_dev *pdev, u16 num_vfs)
 	return ret;
 }
 
-int pcibios_sriov_disable(struct pci_dev *pdev)
+int pnv_pcibios_sriov_disable(struct pci_dev *pdev)
 {
 	pnv_pci_sriov_disable(pdev);
 
@@ -1683,7 +1683,7 @@ int pcibios_sriov_disable(struct pci_dev *pdev)
 	return 0;
 }
 
-int pcibios_sriov_enable(struct pci_dev *pdev, u16 num_vfs)
+int pnv_pcibios_sriov_enable(struct pci_dev *pdev, u16 num_vfs)
 {
 	/* Allocate PCI data */
 	add_dev_pci_data(pdev);
@@ -4002,6 +4002,8 @@ static void __init pnv_pci_init_ioda_phb(struct device_node *np,
 #ifdef CONFIG_PCI_IOV
 	ppc_md.pcibios_fixup_sriov = pnv_pci_ioda_fixup_iov_resources;
 	ppc_md.pcibios_iov_resource_alignment = pnv_pci_iov_resource_alignment;
+	ppc_md.pcibios_sriov_enable = pnv_pcibios_sriov_enable;
+	ppc_md.pcibios_sriov_disable = pnv_pcibios_sriov_disable;
 #endif
 
 	pci_add_flags(PCI_REASSIGN_ALL_RSRC);
-- 
2.11.0 (Apple Git-81)

[PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Bryant G. Ly]

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;
+}
+
 /**
  * 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)

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Bjorn Helgaas]

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)
> 

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Michael Ellerman]

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

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Bjorn Helgaas]

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

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Bryant G. Ly]

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

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Bjorn Helgaas]

[+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

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Steven Royer]

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

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Steven Royer]

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

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Alex Williamson]

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

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Bryant G. Ly]

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

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Bjorn Helgaas]

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

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Juan Alvarez]

[-- Attachment #1: Type: text/plain, Size: 1741 bytes --]

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 Linux 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) logical partition.
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 J. Alvarez

[-- Attachment #2: Type: text/html, Size: 2683 bytes --]

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Juan Alvarez]

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

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Bjorn Helgaas]

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

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Juan Alvarez]

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

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Bjorn Helgaas]

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

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Juan Alvarez]

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

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Bjorn Helgaas]

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

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Michael Ellerman]

"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

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Bryant G. Ly]

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

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Alexey Kardashevskiy]

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.




-- 
Alexey

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Alexey Kardashevskiy]

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.



-- 
Alexey

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Juan Alvarez]

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?

Re: [PATCH v3 0/2] Prepartion for SR-IOV PowerVM Enablement

[Bjorn Helgaas]

On Fri, Sep 22, 2017 at 09:19:26AM -0500, Bryant G. Ly wrote:
> This patch series is to prepare for enabling SR-IOV
> on pseries. It separates the calls to be machine dependent
> and does not change any current functionality.
> 
> The patch linked below which is currently in review is a dependency:
> https://patchwork.kernel.org/patch/9882915/
> 
> v1 - Initial patch
> v2 - Addressed Bjorn's comment on creating a highly platform
>      dependent global exported symbol.
> v3 - Based patch off linux-ppc/master
> 
> Bryant G. Ly (2):
>   powerpc/kernel: Separate SR-IOV Calls
>   pseries/eeh: Add Pseries pcibios_bus_add_device
> 
>  arch/powerpc/include/asm/machdep.h           |  7 ++++++
>  arch/powerpc/include/asm/pci-bridge.h        |  4 +---
>  arch/powerpc/kernel/eeh_driver.c             |  4 ++--
>  arch/powerpc/kernel/pci-common.c             | 23 +++++++++++++++++++
>  arch/powerpc/kernel/pci_dn.c                 |  6 -----
>  arch/powerpc/platforms/powernv/eeh-powernv.c | 33 ++++++++++++++--------------
>  arch/powerpc/platforms/powernv/pci-ioda.c    |  6 +++--
>  arch/powerpc/platforms/pseries/eeh_pseries.c | 24 ++++++++++++++++++++
>  8 files changed, 78 insertions(+), 29 deletions(-)

I assume this will go via a powerpc tree, since it doesn't touch
anything in drivers/pci or any PCI core interfaces.