| Message ID | 20230307113621.64153-2-gautam.dawar@amd.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | sfc: add vDPA support for EF100 devices | expand |
| Context | Check | Description |
|---|---|---|
| netdev/series_format | success | Posting correctly formatted |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 20 this patch: 20 |
| netdev/cc_maintainers | success | CCed 7 of 7 maintainers |
| netdev/build_clang | success | Errors and warnings before: 18 this patch: 18 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/deprecated_api | success | None detected |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 20 this patch: 20 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 168 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Tue, Mar 7, 2023 at 7:36 PM Gautam Dawar <gautam.dawar@amd.com> wrote: > > A function personality defines the location and semantics of > registers in the BAR. EF100 NICs allow different personalities > of a PCIe function and changing it at run-time. A total of three > function personalities are defined as of now: EF100, vDPA and > None with EF100 being the default. > For now, vDPA net devices can be created on a EF100 virtual > function and the VF personality will be changed to vDPA in the > process. > > Co-developed-by: Martin Habets <habetsm.xilinx@gmail.com> > Signed-off-by: Martin Habets <habetsm.xilinx@gmail.com> > Signed-off-by: Gautam Dawar <gautam.dawar@amd.com> > --- > drivers/net/ethernet/sfc/ef100.c | 6 +- > drivers/net/ethernet/sfc/ef100_nic.c | 98 +++++++++++++++++++++++++++- > drivers/net/ethernet/sfc/ef100_nic.h | 11 ++++ > 3 files changed, 111 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/sfc/ef100.c b/drivers/net/ethernet/sfc/ef100.c > index 71aab3d0480f..c1c69783db7b 100644 > --- a/drivers/net/ethernet/sfc/ef100.c > +++ b/drivers/net/ethernet/sfc/ef100.c > @@ -429,8 +429,7 @@ static void ef100_pci_remove(struct pci_dev *pci_dev) > if (!efx) > return; > > - probe_data = container_of(efx, struct efx_probe_data, efx); > - ef100_remove_netdev(probe_data); > + efx_ef100_set_bar_config(efx, EF100_BAR_CONFIG_NONE); > #ifdef CONFIG_SFC_SRIOV > efx_fini_struct_tc(efx); > #endif > @@ -443,6 +442,7 @@ static void ef100_pci_remove(struct pci_dev *pci_dev) > pci_disable_pcie_error_reporting(pci_dev); > > pci_set_drvdata(pci_dev, NULL); > + probe_data = container_of(efx, struct efx_probe_data, efx); > efx_fini_struct(efx); > kfree(probe_data); > }; > @@ -508,7 +508,7 @@ static int ef100_pci_probe(struct pci_dev *pci_dev, > goto fail; > > efx->state = STATE_PROBED; > - rc = ef100_probe_netdev(probe_data); > + rc = efx_ef100_set_bar_config(efx, EF100_BAR_CONFIG_EF100); > if (rc) > goto fail; > > diff --git a/drivers/net/ethernet/sfc/ef100_nic.c b/drivers/net/ethernet/sfc/ef100_nic.c > index 4dc643b0d2db..8cbe5e0f4bdf 100644 > --- a/drivers/net/ethernet/sfc/ef100_nic.c > +++ b/drivers/net/ethernet/sfc/ef100_nic.c > @@ -772,6 +772,99 @@ static int efx_ef100_get_base_mport(struct efx_nic *efx) > return 0; > } > > +/* BAR configuration. > + * To change BAR configuration, tear down the current configuration (which > + * leaves the hardware in the PROBED state), and then initialise the new > + * BAR state. > + */ > +struct ef100_bar_config_ops { > + int (*init)(struct efx_probe_data *probe_data); > + void (*fini)(struct efx_probe_data *probe_data); > +}; > + > +static const struct ef100_bar_config_ops bar_config_ops[] = { > + [EF100_BAR_CONFIG_EF100] = { > + .init = ef100_probe_netdev, > + .fini = ef100_remove_netdev > + }, > +#ifdef CONFIG_SFC_VDPA > + [EF100_BAR_CONFIG_VDPA] = { > + .init = NULL, > + .fini = NULL > + }, > +#endif > + [EF100_BAR_CONFIG_NONE] = { > + .init = NULL, > + .fini = NULL > + }, > +}; This looks more like a mini bus implementation. I wonder if we can reuse an auxiliary bus here which is more user friendly for management tools. It might work like, during PCI probe, register an aux device ef100_nic.net . Then we will have two drivers that could be bound: 1) netdev 2) vdpa So the bar config setup could be delayed to the auxiliary driver probe. And we can register the mgmt device during probing the vdpa aux device. This complies with the sysfs based mgmt interface for driver core and allows more policy to be added on top e.g autoprobe/override etc. Thanks
On Fri, Mar 10, 2023 at 01:04:14PM +0800, Jason Wang wrote: > On Tue, Mar 7, 2023 at 7:36 PM Gautam Dawar <gautam.dawar@amd.com> wrote: > > > > A function personality defines the location and semantics of > > registers in the BAR. EF100 NICs allow different personalities > > of a PCIe function and changing it at run-time. A total of three > > function personalities are defined as of now: EF100, vDPA and > > None with EF100 being the default. > > For now, vDPA net devices can be created on a EF100 virtual > > function and the VF personality will be changed to vDPA in the > > process. > > > > Co-developed-by: Martin Habets <habetsm.xilinx@gmail.com> > > Signed-off-by: Martin Habets <habetsm.xilinx@gmail.com> > > Signed-off-by: Gautam Dawar <gautam.dawar@amd.com> > > --- > > drivers/net/ethernet/sfc/ef100.c | 6 +- > > drivers/net/ethernet/sfc/ef100_nic.c | 98 +++++++++++++++++++++++++++- > > drivers/net/ethernet/sfc/ef100_nic.h | 11 ++++ > > 3 files changed, 111 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/net/ethernet/sfc/ef100.c b/drivers/net/ethernet/sfc/ef100.c > > index 71aab3d0480f..c1c69783db7b 100644 > > --- a/drivers/net/ethernet/sfc/ef100.c > > +++ b/drivers/net/ethernet/sfc/ef100.c > > @@ -429,8 +429,7 @@ static void ef100_pci_remove(struct pci_dev *pci_dev) > > if (!efx) > > return; > > > > - probe_data = container_of(efx, struct efx_probe_data, efx); > > - ef100_remove_netdev(probe_data); > > + efx_ef100_set_bar_config(efx, EF100_BAR_CONFIG_NONE); > > #ifdef CONFIG_SFC_SRIOV > > efx_fini_struct_tc(efx); > > #endif > > @@ -443,6 +442,7 @@ static void ef100_pci_remove(struct pci_dev *pci_dev) > > pci_disable_pcie_error_reporting(pci_dev); > > > > pci_set_drvdata(pci_dev, NULL); > > + probe_data = container_of(efx, struct efx_probe_data, efx); > > efx_fini_struct(efx); > > kfree(probe_data); > > }; > > @@ -508,7 +508,7 @@ static int ef100_pci_probe(struct pci_dev *pci_dev, > > goto fail; > > > > efx->state = STATE_PROBED; > > - rc = ef100_probe_netdev(probe_data); > > + rc = efx_ef100_set_bar_config(efx, EF100_BAR_CONFIG_EF100); > > if (rc) > > goto fail; > > > > diff --git a/drivers/net/ethernet/sfc/ef100_nic.c b/drivers/net/ethernet/sfc/ef100_nic.c > > index 4dc643b0d2db..8cbe5e0f4bdf 100644 > > --- a/drivers/net/ethernet/sfc/ef100_nic.c > > +++ b/drivers/net/ethernet/sfc/ef100_nic.c > > @@ -772,6 +772,99 @@ static int efx_ef100_get_base_mport(struct efx_nic *efx) > > return 0; > > } > > > > +/* BAR configuration. > > + * To change BAR configuration, tear down the current configuration (which > > + * leaves the hardware in the PROBED state), and then initialise the new > > + * BAR state. > > + */ > > +struct ef100_bar_config_ops { > > + int (*init)(struct efx_probe_data *probe_data); > > + void (*fini)(struct efx_probe_data *probe_data); > > +}; > > + > > +static const struct ef100_bar_config_ops bar_config_ops[] = { > > + [EF100_BAR_CONFIG_EF100] = { > > + .init = ef100_probe_netdev, > > + .fini = ef100_remove_netdev > > + }, > > +#ifdef CONFIG_SFC_VDPA > > + [EF100_BAR_CONFIG_VDPA] = { > > + .init = NULL, > > + .fini = NULL > > + }, > > +#endif > > + [EF100_BAR_CONFIG_NONE] = { > > + .init = NULL, > > + .fini = NULL > > + }, > > +}; > > This looks more like a mini bus implementation. I wonder if we can > reuse an auxiliary bus here which is more user friendly for management > tools. When we were in the design phase of vDPA for EF100 it was still called virtbus, and the virtbus discussion was in full swing at that time. We could not afford to add risk to the project by depending on it, as it might not have been merged at all. If we were doing the same design now I would definitely consider using the auxiliary bus. Martin
在 2023/3/13 19:50, Martin Habets 写道: > On Fri, Mar 10, 2023 at 01:04:14PM +0800, Jason Wang wrote: >> On Tue, Mar 7, 2023 at 7:36 PM Gautam Dawar <gautam.dawar@amd.com> wrote: >>> A function personality defines the location and semantics of >>> registers in the BAR. EF100 NICs allow different personalities >>> of a PCIe function and changing it at run-time. A total of three >>> function personalities are defined as of now: EF100, vDPA and >>> None with EF100 being the default. >>> For now, vDPA net devices can be created on a EF100 virtual >>> function and the VF personality will be changed to vDPA in the >>> process. >>> >>> Co-developed-by: Martin Habets <habetsm.xilinx@gmail.com> >>> Signed-off-by: Martin Habets <habetsm.xilinx@gmail.com> >>> Signed-off-by: Gautam Dawar <gautam.dawar@amd.com> >>> --- >>> drivers/net/ethernet/sfc/ef100.c | 6 +- >>> drivers/net/ethernet/sfc/ef100_nic.c | 98 +++++++++++++++++++++++++++- >>> drivers/net/ethernet/sfc/ef100_nic.h | 11 ++++ >>> 3 files changed, 111 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/net/ethernet/sfc/ef100.c b/drivers/net/ethernet/sfc/ef100.c >>> index 71aab3d0480f..c1c69783db7b 100644 >>> --- a/drivers/net/ethernet/sfc/ef100.c >>> +++ b/drivers/net/ethernet/sfc/ef100.c >>> @@ -429,8 +429,7 @@ static void ef100_pci_remove(struct pci_dev *pci_dev) >>> if (!efx) >>> return; >>> >>> - probe_data = container_of(efx, struct efx_probe_data, efx); >>> - ef100_remove_netdev(probe_data); >>> + efx_ef100_set_bar_config(efx, EF100_BAR_CONFIG_NONE); >>> #ifdef CONFIG_SFC_SRIOV >>> efx_fini_struct_tc(efx); >>> #endif >>> @@ -443,6 +442,7 @@ static void ef100_pci_remove(struct pci_dev *pci_dev) >>> pci_disable_pcie_error_reporting(pci_dev); >>> >>> pci_set_drvdata(pci_dev, NULL); >>> + probe_data = container_of(efx, struct efx_probe_data, efx); >>> efx_fini_struct(efx); >>> kfree(probe_data); >>> }; >>> @@ -508,7 +508,7 @@ static int ef100_pci_probe(struct pci_dev *pci_dev, >>> goto fail; >>> >>> efx->state = STATE_PROBED; >>> - rc = ef100_probe_netdev(probe_data); >>> + rc = efx_ef100_set_bar_config(efx, EF100_BAR_CONFIG_EF100); >>> if (rc) >>> goto fail; >>> >>> diff --git a/drivers/net/ethernet/sfc/ef100_nic.c b/drivers/net/ethernet/sfc/ef100_nic.c >>> index 4dc643b0d2db..8cbe5e0f4bdf 100644 >>> --- a/drivers/net/ethernet/sfc/ef100_nic.c >>> +++ b/drivers/net/ethernet/sfc/ef100_nic.c >>> @@ -772,6 +772,99 @@ static int efx_ef100_get_base_mport(struct efx_nic *efx) >>> return 0; >>> } >>> >>> +/* BAR configuration. >>> + * To change BAR configuration, tear down the current configuration (which >>> + * leaves the hardware in the PROBED state), and then initialise the new >>> + * BAR state. >>> + */ >>> +struct ef100_bar_config_ops { >>> + int (*init)(struct efx_probe_data *probe_data); >>> + void (*fini)(struct efx_probe_data *probe_data); >>> +}; >>> + >>> +static const struct ef100_bar_config_ops bar_config_ops[] = { >>> + [EF100_BAR_CONFIG_EF100] = { >>> + .init = ef100_probe_netdev, >>> + .fini = ef100_remove_netdev >>> + }, >>> +#ifdef CONFIG_SFC_VDPA >>> + [EF100_BAR_CONFIG_VDPA] = { >>> + .init = NULL, >>> + .fini = NULL >>> + }, >>> +#endif >>> + [EF100_BAR_CONFIG_NONE] = { >>> + .init = NULL, >>> + .fini = NULL >>> + }, >>> +}; >> This looks more like a mini bus implementation. I wonder if we can >> reuse an auxiliary bus here which is more user friendly for management >> tools. > When we were in the design phase of vDPA for EF100 it was still called > virtbus, and the virtbus discussion was in full swing at that time. > We could not afford to add risk to the project by depending on it, as > it might not have been merged at all. Right. > If we were doing the same design now I would definitely consider using > the auxiliary bus. > > Martin But it's not late to do the change now. Auxiliary bus has been used by a lot of devices (even with vDPA device). The change looks not too complicated. This looks more scalable and convenient for management layer. Thanks
On Wed, Mar 15, 2023 at 01:11:23PM +0800, Jason Wang wrote: > > 在 2023/3/13 19:50, Martin Habets 写道: > > On Fri, Mar 10, 2023 at 01:04:14PM +0800, Jason Wang wrote: > > > On Tue, Mar 7, 2023 at 7:36 PM Gautam Dawar <gautam.dawar@amd.com> wrote: > > > > A function personality defines the location and semantics of > > > > registers in the BAR. EF100 NICs allow different personalities > > > > of a PCIe function and changing it at run-time. A total of three > > > > function personalities are defined as of now: EF100, vDPA and > > > > None with EF100 being the default. > > > > For now, vDPA net devices can be created on a EF100 virtual > > > > function and the VF personality will be changed to vDPA in the > > > > process. > > > > > > > > Co-developed-by: Martin Habets <habetsm.xilinx@gmail.com> > > > > Signed-off-by: Martin Habets <habetsm.xilinx@gmail.com> > > > > Signed-off-by: Gautam Dawar <gautam.dawar@amd.com> > > > > --- > > > > drivers/net/ethernet/sfc/ef100.c | 6 +- > > > > drivers/net/ethernet/sfc/ef100_nic.c | 98 +++++++++++++++++++++++++++- > > > > drivers/net/ethernet/sfc/ef100_nic.h | 11 ++++ > > > > 3 files changed, 111 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/net/ethernet/sfc/ef100.c b/drivers/net/ethernet/sfc/ef100.c > > > > index 71aab3d0480f..c1c69783db7b 100644 > > > > --- a/drivers/net/ethernet/sfc/ef100.c > > > > +++ b/drivers/net/ethernet/sfc/ef100.c > > > > @@ -429,8 +429,7 @@ static void ef100_pci_remove(struct pci_dev *pci_dev) > > > > if (!efx) > > > > return; > > > > > > > > - probe_data = container_of(efx, struct efx_probe_data, efx); > > > > - ef100_remove_netdev(probe_data); > > > > + efx_ef100_set_bar_config(efx, EF100_BAR_CONFIG_NONE); > > > > #ifdef CONFIG_SFC_SRIOV > > > > efx_fini_struct_tc(efx); > > > > #endif > > > > @@ -443,6 +442,7 @@ static void ef100_pci_remove(struct pci_dev *pci_dev) > > > > pci_disable_pcie_error_reporting(pci_dev); > > > > > > > > pci_set_drvdata(pci_dev, NULL); > > > > + probe_data = container_of(efx, struct efx_probe_data, efx); > > > > efx_fini_struct(efx); > > > > kfree(probe_data); > > > > }; > > > > @@ -508,7 +508,7 @@ static int ef100_pci_probe(struct pci_dev *pci_dev, > > > > goto fail; > > > > > > > > efx->state = STATE_PROBED; > > > > - rc = ef100_probe_netdev(probe_data); > > > > + rc = efx_ef100_set_bar_config(efx, EF100_BAR_CONFIG_EF100); > > > > if (rc) > > > > goto fail; > > > > > > > > diff --git a/drivers/net/ethernet/sfc/ef100_nic.c b/drivers/net/ethernet/sfc/ef100_nic.c > > > > index 4dc643b0d2db..8cbe5e0f4bdf 100644 > > > > --- a/drivers/net/ethernet/sfc/ef100_nic.c > > > > +++ b/drivers/net/ethernet/sfc/ef100_nic.c > > > > @@ -772,6 +772,99 @@ static int efx_ef100_get_base_mport(struct efx_nic *efx) > > > > return 0; > > > > } > > > > > > > > +/* BAR configuration. > > > > + * To change BAR configuration, tear down the current configuration (which > > > > + * leaves the hardware in the PROBED state), and then initialise the new > > > > + * BAR state. > > > > + */ > > > > +struct ef100_bar_config_ops { > > > > + int (*init)(struct efx_probe_data *probe_data); > > > > + void (*fini)(struct efx_probe_data *probe_data); > > > > +}; > > > > + > > > > +static const struct ef100_bar_config_ops bar_config_ops[] = { > > > > + [EF100_BAR_CONFIG_EF100] = { > > > > + .init = ef100_probe_netdev, > > > > + .fini = ef100_remove_netdev > > > > + }, > > > > +#ifdef CONFIG_SFC_VDPA > > > > + [EF100_BAR_CONFIG_VDPA] = { > > > > + .init = NULL, > > > > + .fini = NULL > > > > + }, > > > > +#endif > > > > + [EF100_BAR_CONFIG_NONE] = { > > > > + .init = NULL, > > > > + .fini = NULL > > > > + }, > > > > +}; > > > This looks more like a mini bus implementation. I wonder if we can > > > reuse an auxiliary bus here which is more user friendly for management > > > tools. > > When we were in the design phase of vDPA for EF100 it was still called > > virtbus, and the virtbus discussion was in full swing at that time. > > We could not afford to add risk to the project by depending on it, as > > it might not have been merged at all. > > > Right. > > > > If we were doing the same design now I would definitely consider using > > the auxiliary bus. > > > > Martin > > > But it's not late to do the change now. Auxiliary bus has been used by a lot > of devices (even with vDPA device). The change looks not too complicated. I'm surprised you think this would not be complicated. From my view it would require redesign, redevelopment and retest of vdpa which would take months. That is assuming we can get some of the resources back. > This looks more scalable and convenient for management layer. There is not much difference for the management layer, it uses the vdpa tool now and it would do so with the auxiliary bus. The difference is that with the auxiliary bus users would have to load another module for sfc vDPA support. Are we maybe on 2 different trains of thought here? Martin
On Thu, Mar 16, 2023 at 5:07 PM Martin Habets <habetsm.xilinx@gmail.com> wrote: > > On Wed, Mar 15, 2023 at 01:11:23PM +0800, Jason Wang wrote: > > > > 在 2023/3/13 19:50, Martin Habets 写道: > > > On Fri, Mar 10, 2023 at 01:04:14PM +0800, Jason Wang wrote: > > > > On Tue, Mar 7, 2023 at 7:36 PM Gautam Dawar <gautam.dawar@amd.com> wrote: > > > > > A function personality defines the location and semantics of > > > > > registers in the BAR. EF100 NICs allow different personalities > > > > > of a PCIe function and changing it at run-time. A total of three > > > > > function personalities are defined as of now: EF100, vDPA and > > > > > None with EF100 being the default. > > > > > For now, vDPA net devices can be created on a EF100 virtual > > > > > function and the VF personality will be changed to vDPA in the > > > > > process. > > > > > > > > > > Co-developed-by: Martin Habets <habetsm.xilinx@gmail.com> > > > > > Signed-off-by: Martin Habets <habetsm.xilinx@gmail.com> > > > > > Signed-off-by: Gautam Dawar <gautam.dawar@amd.com> > > > > > --- > > > > > drivers/net/ethernet/sfc/ef100.c | 6 +- > > > > > drivers/net/ethernet/sfc/ef100_nic.c | 98 +++++++++++++++++++++++++++- > > > > > drivers/net/ethernet/sfc/ef100_nic.h | 11 ++++ > > > > > 3 files changed, 111 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/drivers/net/ethernet/sfc/ef100.c b/drivers/net/ethernet/sfc/ef100.c > > > > > index 71aab3d0480f..c1c69783db7b 100644 > > > > > --- a/drivers/net/ethernet/sfc/ef100.c > > > > > +++ b/drivers/net/ethernet/sfc/ef100.c > > > > > @@ -429,8 +429,7 @@ static void ef100_pci_remove(struct pci_dev *pci_dev) > > > > > if (!efx) > > > > > return; > > > > > > > > > > - probe_data = container_of(efx, struct efx_probe_data, efx); > > > > > - ef100_remove_netdev(probe_data); > > > > > + efx_ef100_set_bar_config(efx, EF100_BAR_CONFIG_NONE); > > > > > #ifdef CONFIG_SFC_SRIOV > > > > > efx_fini_struct_tc(efx); > > > > > #endif > > > > > @@ -443,6 +442,7 @@ static void ef100_pci_remove(struct pci_dev *pci_dev) > > > > > pci_disable_pcie_error_reporting(pci_dev); > > > > > > > > > > pci_set_drvdata(pci_dev, NULL); > > > > > + probe_data = container_of(efx, struct efx_probe_data, efx); > > > > > efx_fini_struct(efx); > > > > > kfree(probe_data); > > > > > }; > > > > > @@ -508,7 +508,7 @@ static int ef100_pci_probe(struct pci_dev *pci_dev, > > > > > goto fail; > > > > > > > > > > efx->state = STATE_PROBED; > > > > > - rc = ef100_probe_netdev(probe_data); > > > > > + rc = efx_ef100_set_bar_config(efx, EF100_BAR_CONFIG_EF100); > > > > > if (rc) > > > > > goto fail; > > > > > > > > > > diff --git a/drivers/net/ethernet/sfc/ef100_nic.c b/drivers/net/ethernet/sfc/ef100_nic.c > > > > > index 4dc643b0d2db..8cbe5e0f4bdf 100644 > > > > > --- a/drivers/net/ethernet/sfc/ef100_nic.c > > > > > +++ b/drivers/net/ethernet/sfc/ef100_nic.c > > > > > @@ -772,6 +772,99 @@ static int efx_ef100_get_base_mport(struct efx_nic *efx) > > > > > return 0; > > > > > } > > > > > > > > > > +/* BAR configuration. > > > > > + * To change BAR configuration, tear down the current configuration (which > > > > > + * leaves the hardware in the PROBED state), and then initialise the new > > > > > + * BAR state. > > > > > + */ > > > > > +struct ef100_bar_config_ops { > > > > > + int (*init)(struct efx_probe_data *probe_data); > > > > > + void (*fini)(struct efx_probe_data *probe_data); > > > > > +}; > > > > > + > > > > > +static const struct ef100_bar_config_ops bar_config_ops[] = { > > > > > + [EF100_BAR_CONFIG_EF100] = { > > > > > + .init = ef100_probe_netdev, > > > > > + .fini = ef100_remove_netdev > > > > > + }, > > > > > +#ifdef CONFIG_SFC_VDPA > > > > > + [EF100_BAR_CONFIG_VDPA] = { > > > > > + .init = NULL, > > > > > + .fini = NULL > > > > > + }, > > > > > +#endif > > > > > + [EF100_BAR_CONFIG_NONE] = { > > > > > + .init = NULL, > > > > > + .fini = NULL > > > > > + }, > > > > > +}; > > > > This looks more like a mini bus implementation. I wonder if we can > > > > reuse an auxiliary bus here which is more user friendly for management > > > > tools. > > > When we were in the design phase of vDPA for EF100 it was still called > > > virtbus, and the virtbus discussion was in full swing at that time. > > > We could not afford to add risk to the project by depending on it, as > > > it might not have been merged at all. > > > > > > Right. > > > > > > > If we were doing the same design now I would definitely consider using > > > the auxiliary bus. > > > > > > Martin > > > > > > But it's not late to do the change now. Auxiliary bus has been used by a lot > > of devices (even with vDPA device). The change looks not too complicated. > > I'm surprised you think this would not be complicated. From my view it would > require redesign, redevelopment and retest of vdpa which would take months. That is > assuming we can get some of the resources back. I think I'm fine if we agree to do it sometime in the future. > > > This looks more scalable and convenient for management layer. > > There is not much difference for the management layer, it uses the vdpa tool now > and it would do so with the auxiliary bus. At the vDPA level it doesn't make too much difference. > The difference is that with the > auxiliary bus users would have to load another module for sfc vDPA support. The policy is fully under the control of the management instead of the hard-coding policy now, more below. > Are we maybe on 2 different trains of thought here? If I read the code correct, when VF is probed: 1) vDPA mgmtdev is registered 2) netdev is probed, bar config set to netdev This means when user want to create vDPA device 1) unregister netdev 2) vDPA is probed, bar config set to vDPA And when vDPA device is deleted: 1) unregister vDPA 2) netdev is probed, bat config set to netdev There would be a lot of side effects for the mandated policy of registering/unregistering netdevs like udev events etc when adding and removing vDPA devices. Thanks > > Martin >
On Fri, Mar 17, 2023 at 11:52:02AM +0800, Jason Wang wrote: > On Thu, Mar 16, 2023 at 5:07 PM Martin Habets <habetsm.xilinx@gmail.com> wrote: > > > > On Wed, Mar 15, 2023 at 01:11:23PM +0800, Jason Wang wrote: > > > > > > 在 2023/3/13 19:50, Martin Habets 写道: > > > > On Fri, Mar 10, 2023 at 01:04:14PM +0800, Jason Wang wrote: > > > > > On Tue, Mar 7, 2023 at 7:36 PM Gautam Dawar <gautam.dawar@amd.com> wrote: > > > > > > A function personality defines the location and semantics of > > > > > > registers in the BAR. EF100 NICs allow different personalities > > > > > > of a PCIe function and changing it at run-time. A total of three > > > > > > function personalities are defined as of now: EF100, vDPA and > > > > > > None with EF100 being the default. > > > > > > For now, vDPA net devices can be created on a EF100 virtual > > > > > > function and the VF personality will be changed to vDPA in the > > > > > > process. > > > > > > > > > > > > Co-developed-by: Martin Habets <habetsm.xilinx@gmail.com> > > > > > > Signed-off-by: Martin Habets <habetsm.xilinx@gmail.com> > > > > > > Signed-off-by: Gautam Dawar <gautam.dawar@amd.com> > > > > > > --- > > > > > > drivers/net/ethernet/sfc/ef100.c | 6 +- > > > > > > drivers/net/ethernet/sfc/ef100_nic.c | 98 +++++++++++++++++++++++++++- > > > > > > drivers/net/ethernet/sfc/ef100_nic.h | 11 ++++ > > > > > > 3 files changed, 111 insertions(+), 4 deletions(-) > > > > > > > > > > > > diff --git a/drivers/net/ethernet/sfc/ef100.c b/drivers/net/ethernet/sfc/ef100.c > > > > > > index 71aab3d0480f..c1c69783db7b 100644 > > > > > > --- a/drivers/net/ethernet/sfc/ef100.c > > > > > > +++ b/drivers/net/ethernet/sfc/ef100.c > > > > > > @@ -429,8 +429,7 @@ static void ef100_pci_remove(struct pci_dev *pci_dev) > > > > > > if (!efx) > > > > > > return; > > > > > > > > > > > > - probe_data = container_of(efx, struct efx_probe_data, efx); > > > > > > - ef100_remove_netdev(probe_data); > > > > > > + efx_ef100_set_bar_config(efx, EF100_BAR_CONFIG_NONE); > > > > > > #ifdef CONFIG_SFC_SRIOV > > > > > > efx_fini_struct_tc(efx); > > > > > > #endif > > > > > > @@ -443,6 +442,7 @@ static void ef100_pci_remove(struct pci_dev *pci_dev) > > > > > > pci_disable_pcie_error_reporting(pci_dev); > > > > > > > > > > > > pci_set_drvdata(pci_dev, NULL); > > > > > > + probe_data = container_of(efx, struct efx_probe_data, efx); > > > > > > efx_fini_struct(efx); > > > > > > kfree(probe_data); > > > > > > }; > > > > > > @@ -508,7 +508,7 @@ static int ef100_pci_probe(struct pci_dev *pci_dev, > > > > > > goto fail; > > > > > > > > > > > > efx->state = STATE_PROBED; > > > > > > - rc = ef100_probe_netdev(probe_data); > > > > > > + rc = efx_ef100_set_bar_config(efx, EF100_BAR_CONFIG_EF100); > > > > > > if (rc) > > > > > > goto fail; > > > > > > > > > > > > diff --git a/drivers/net/ethernet/sfc/ef100_nic.c b/drivers/net/ethernet/sfc/ef100_nic.c > > > > > > index 4dc643b0d2db..8cbe5e0f4bdf 100644 > > > > > > --- a/drivers/net/ethernet/sfc/ef100_nic.c > > > > > > +++ b/drivers/net/ethernet/sfc/ef100_nic.c > > > > > > @@ -772,6 +772,99 @@ static int efx_ef100_get_base_mport(struct efx_nic *efx) > > > > > > return 0; > > > > > > } > > > > > > > > > > > > +/* BAR configuration. > > > > > > + * To change BAR configuration, tear down the current configuration (which > > > > > > + * leaves the hardware in the PROBED state), and then initialise the new > > > > > > + * BAR state. > > > > > > + */ > > > > > > +struct ef100_bar_config_ops { > > > > > > + int (*init)(struct efx_probe_data *probe_data); > > > > > > + void (*fini)(struct efx_probe_data *probe_data); > > > > > > +}; > > > > > > + > > > > > > +static const struct ef100_bar_config_ops bar_config_ops[] = { > > > > > > + [EF100_BAR_CONFIG_EF100] = { > > > > > > + .init = ef100_probe_netdev, > > > > > > + .fini = ef100_remove_netdev > > > > > > + }, > > > > > > +#ifdef CONFIG_SFC_VDPA > > > > > > + [EF100_BAR_CONFIG_VDPA] = { > > > > > > + .init = NULL, > > > > > > + .fini = NULL > > > > > > + }, > > > > > > +#endif > > > > > > + [EF100_BAR_CONFIG_NONE] = { > > > > > > + .init = NULL, > > > > > > + .fini = NULL > > > > > > + }, > > > > > > +}; > > > > > This looks more like a mini bus implementation. I wonder if we can > > > > > reuse an auxiliary bus here which is more user friendly for management > > > > > tools. > > > > When we were in the design phase of vDPA for EF100 it was still called > > > > virtbus, and the virtbus discussion was in full swing at that time. > > > > We could not afford to add risk to the project by depending on it, as > > > > it might not have been merged at all. > > > > > > > > > Right. > > > > > > > > > > If we were doing the same design now I would definitely consider using > > > > the auxiliary bus. > > > > > > > > Martin > > > > > > > > > But it's not late to do the change now. Auxiliary bus has been used by a lot > > > of devices (even with vDPA device). The change looks not too complicated. > > > > I'm surprised you think this would not be complicated. From my view it would > > require redesign, redevelopment and retest of vdpa which would take months. That is > > assuming we can get some of the resources back. > > I think I'm fine if we agree to do it sometime in the future. We have projects on the roadmap that will need to use auxbus for this. The timeline for that is not clear to me at the moment. > > > > > This looks more scalable and convenient for management layer. > > > > There is not much difference for the management layer, it uses the vdpa tool now > > and it would do so with the auxiliary bus. > > At the vDPA level it doesn't make too much difference. > > > The difference is that with the > > auxiliary bus users would have to load another module for sfc vDPA support. > > The policy is fully under the control of the management instead of the > hard-coding policy now, more below. > > > Are we maybe on 2 different trains of thought here? > > If I read the code correct, when VF is probed: > > 1) vDPA mgmtdev is registered > 2) netdev is probed, bar config set to netdev > > This means when user want to create vDPA device > > 1) unregister netdev > 2) vDPA is probed, bar config set to vDPA > > And when vDPA device is deleted: > > 1) unregister vDPA > 2) netdev is probed, bat config set to netdev Your analysis is correct. We have a requirement to initially create a netdev for new VFs. This was done to maintain backward compatibility with earlier Solarflare NICs. For new products I can try to change this default behaviour. I definitely see a trend towards a core "PCI" layer and different layers on top of that. The auxiliary bus will have a significant role to play here. > There would be a lot of side effects for the mandated policy of > registering/unregistering netdevs like udev events etc when adding and > removing vDPA devices. I agree. During our testing we cam across versions of systemd that will cause a spike in the load when creating many netdevs. Martin > > Thanks > > > > > Martin > >
diff --git a/drivers/net/ethernet/sfc/ef100.c b/drivers/net/ethernet/sfc/ef100.c index 71aab3d0480f..c1c69783db7b 100644 --- a/drivers/net/ethernet/sfc/ef100.c +++ b/drivers/net/ethernet/sfc/ef100.c @@ -429,8 +429,7 @@ static void ef100_pci_remove(struct pci_dev *pci_dev) if (!efx) return; - probe_data = container_of(efx, struct efx_probe_data, efx); - ef100_remove_netdev(probe_data); + efx_ef100_set_bar_config(efx, EF100_BAR_CONFIG_NONE); #ifdef CONFIG_SFC_SRIOV efx_fini_struct_tc(efx); #endif @@ -443,6 +442,7 @@ static void ef100_pci_remove(struct pci_dev *pci_dev) pci_disable_pcie_error_reporting(pci_dev); pci_set_drvdata(pci_dev, NULL); + probe_data = container_of(efx, struct efx_probe_data, efx); efx_fini_struct(efx); kfree(probe_data); }; @@ -508,7 +508,7 @@ static int ef100_pci_probe(struct pci_dev *pci_dev, goto fail; efx->state = STATE_PROBED; - rc = ef100_probe_netdev(probe_data); + rc = efx_ef100_set_bar_config(efx, EF100_BAR_CONFIG_EF100); if (rc) goto fail; diff --git a/drivers/net/ethernet/sfc/ef100_nic.c b/drivers/net/ethernet/sfc/ef100_nic.c index 4dc643b0d2db..8cbe5e0f4bdf 100644 --- a/drivers/net/ethernet/sfc/ef100_nic.c +++ b/drivers/net/ethernet/sfc/ef100_nic.c @@ -772,6 +772,99 @@ static int efx_ef100_get_base_mport(struct efx_nic *efx) return 0; } +/* BAR configuration. + * To change BAR configuration, tear down the current configuration (which + * leaves the hardware in the PROBED state), and then initialise the new + * BAR state. + */ +struct ef100_bar_config_ops { + int (*init)(struct efx_probe_data *probe_data); + void (*fini)(struct efx_probe_data *probe_data); +}; + +static const struct ef100_bar_config_ops bar_config_ops[] = { + [EF100_BAR_CONFIG_EF100] = { + .init = ef100_probe_netdev, + .fini = ef100_remove_netdev + }, +#ifdef CONFIG_SFC_VDPA + [EF100_BAR_CONFIG_VDPA] = { + .init = NULL, + .fini = NULL + }, +#endif + [EF100_BAR_CONFIG_NONE] = { + .init = NULL, + .fini = NULL + }, +}; + +/* Keep this in sync with the definition of enum ef100_bar_config. */ +static char *bar_config_name[] = { + [EF100_BAR_CONFIG_NONE] = "None", + [EF100_BAR_CONFIG_EF100] = "EF100", + [EF100_BAR_CONFIG_VDPA] = "vDPA", +}; + +#ifdef CONFIG_SFC_VDPA +static bool efx_vdpa_supported(struct efx_nic *efx) +{ + return efx->type->is_vf; +} +#endif + +int efx_ef100_set_bar_config(struct efx_nic *efx, + enum ef100_bar_config new_config) +{ + const struct ef100_bar_config_ops *old_config_ops; + const struct ef100_bar_config_ops *new_config_ops; + struct ef100_nic_data *nic_data = efx->nic_data; + struct efx_probe_data *probe_data; + enum ef100_bar_config old_config; + int rc; + + if (WARN_ON_ONCE(nic_data->bar_config > EF100_BAR_CONFIG_VDPA)) + return -EINVAL; + +#ifdef CONFIG_SFC_VDPA + /* Current EF100 hardware supports vDPA on VFs only */ + if (new_config == EF100_BAR_CONFIG_VDPA && !efx_vdpa_supported(efx)) { + pci_err(efx->pci_dev, "vdpa over PF not supported : %s", + efx->name); + return -EOPNOTSUPP; + } +#endif + mutex_lock(&nic_data->bar_config_lock); + old_config = nic_data->bar_config; + if (new_config == old_config) { + mutex_unlock(&nic_data->bar_config_lock); + return 0; + } + + old_config_ops = &bar_config_ops[old_config]; + new_config_ops = &bar_config_ops[new_config]; + + probe_data = container_of(efx, struct efx_probe_data, efx); + if (old_config_ops->fini) + old_config_ops->fini(probe_data); + nic_data->bar_config = EF100_BAR_CONFIG_NONE; + + if (new_config_ops->init) { + rc = new_config_ops->init(probe_data); + if (rc) { + mutex_unlock(&nic_data->bar_config_lock); + return rc; + } + } + + nic_data->bar_config = new_config; + pci_dbg(efx->pci_dev, "BAR configuration changed to %s\n", + bar_config_name[new_config]); + mutex_unlock(&nic_data->bar_config_lock); + + return 0; +} + static int compare_versions(const char *a, const char *b) { int a_major, a_minor, a_point, a_patch; @@ -1025,6 +1118,7 @@ static int ef100_probe_main(struct efx_nic *efx) return -ENOMEM; efx->nic_data = nic_data; nic_data->efx = efx; + mutex_init(&nic_data->bar_config_lock); efx->max_vis = EF100_MAX_VIS; /* Populate design-parameter defaults */ @@ -1208,8 +1302,10 @@ void ef100_remove(struct efx_nic *efx) efx_mcdi_detach(efx); efx_mcdi_fini(efx); - if (nic_data) + if (nic_data) { efx_nic_free_buffer(efx, &nic_data->mcdi_buf); + mutex_destroy(&nic_data->bar_config_lock); + } kfree(nic_data); efx->nic_data = NULL; } diff --git a/drivers/net/ethernet/sfc/ef100_nic.h b/drivers/net/ethernet/sfc/ef100_nic.h index f1ed481c1260..4562982f2965 100644 --- a/drivers/net/ethernet/sfc/ef100_nic.h +++ b/drivers/net/ethernet/sfc/ef100_nic.h @@ -61,6 +61,13 @@ enum { EF100_STAT_COUNT }; +/* Keep this in sync with the contents of bar_config_name. */ +enum ef100_bar_config { + EF100_BAR_CONFIG_NONE, + EF100_BAR_CONFIG_EF100, + EF100_BAR_CONFIG_VDPA, +}; + struct ef100_nic_data { struct efx_nic *efx; struct efx_buffer mcdi_buf; @@ -71,6 +78,8 @@ struct ef100_nic_data { u16 warm_boot_count; u8 port_id[ETH_ALEN]; DECLARE_BITMAP(evq_phases, EFX_MAX_CHANNELS); + enum ef100_bar_config bar_config; + struct mutex bar_config_lock; /* lock to control access to bar config */ u64 stats[EF100_STAT_COUNT]; u32 base_mport; bool have_mport; /* base_mport was populated successfully */ @@ -95,4 +104,6 @@ int ef100_filter_table_probe(struct efx_nic *efx); int ef100_get_mac_address(struct efx_nic *efx, u8 *mac_address, int client_handle, bool empty_ok); int efx_ef100_lookup_client_id(struct efx_nic *efx, efx_qword_t pciefn, u32 *id); +int efx_ef100_set_bar_config(struct efx_nic *efx, + enum ef100_bar_config new_config); #endif /* EFX_EF100_NIC_H */