lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Mon, 16 Dec 2019 09:01:53 +0000 From: Parav Pandit <parav@...lanox.com> To: Shannon Nelson <snelson@...sando.io>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>, "davem@...emloft.net" <davem@...emloft.net> Subject: Re: [PATCH v3 net-next 2/2] ionic: support sr-iov operations On 12/14/2019 12:25 AM, Shannon Nelson wrote: > Add the netdev ops for managing VFs. Since most of the > management work happens in the NIC firmware, the driver becomes > mostly a pass-through for the network stack commands that want > to control and configure the VFs. > > We also tweak ionic_station_set() a little to allow for > the VFs that start off with a zero'd mac address. > > Signed-off-by: Shannon Nelson <snelson@...sando.io> > --- > drivers/net/ethernet/pensando/ionic/ionic.h | 17 +- > .../ethernet/pensando/ionic/ionic_bus_pci.c | 101 ++++++++ > .../net/ethernet/pensando/ionic/ionic_dev.c | 58 +++++ > .../net/ethernet/pensando/ionic/ionic_dev.h | 7 + > .../net/ethernet/pensando/ionic/ionic_lif.c | 222 +++++++++++++++++- > .../net/ethernet/pensando/ionic/ionic_main.c | 4 + > 6 files changed, 401 insertions(+), 8 deletions(-) > > diff --git a/drivers/net/ethernet/pensando/ionic/ionic.h b/drivers/net/ethernet/pensando/ionic/ionic.h > index 98e102af7756..74b358f03599 100644 > --- a/drivers/net/ethernet/pensando/ionic/ionic.h > +++ b/drivers/net/ethernet/pensando/ionic/ionic.h > @@ -12,7 +12,7 @@ struct ionic_lif; > > #define IONIC_DRV_NAME "ionic" > #define IONIC_DRV_DESCRIPTION "Pensando Ethernet NIC Driver" > -#define IONIC_DRV_VERSION "0.18.0-k" > +#define IONIC_DRV_VERSION "0.20.0-k" > > #define PCI_VENDOR_ID_PENSANDO 0x1dd8 > > @@ -25,12 +25,25 @@ struct ionic_lif; > > #define DEVCMD_TIMEOUT 10 > > +struct ionic_vf { > + u16 index; > + u8 macaddr[6]; > + __le32 maxrate; > + __le16 vlanid; > + u8 spoofchk; > + u8 trusted; > + u8 linkstate; > + dma_addr_t stats_pa; > + struct ionic_lif_stats stats; > +}; > + > struct ionic { > struct pci_dev *pdev; > struct device *dev; > struct devlink_port dl_port; > struct ionic_dev idev; > struct mutex dev_cmd_lock; /* lock for dev_cmd operations */ > + struct rw_semaphore vf_op_lock; /* lock for VF operations */ It is better to place this semaphore adjucent to other related VF fields you have below as they are all used together. > struct dentry *dentry; > struct ionic_dev_bar bars[IONIC_BARS_MAX]; > unsigned int num_bars; > @@ -46,6 +59,8 @@ struct ionic { > DECLARE_BITMAP(intrs, IONIC_INTR_CTRL_REGS_MAX); > struct work_struct nb_work; > struct notifier_block nb; > + unsigned int num_vfs; If you intent to store this, store is as 'int' because that is what is coming from sriov_configure() at the moment. But it sholdn't be stored. More below. > + struct ionic_vf **vf; > struct timer_list watchdog_timer; > int watchdog_period; > }; > diff --git a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c > index 9a9ab8cb2cb3..b9a3e1e1d41e 100644 > --- a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c > +++ b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c > @@ -104,10 +104,101 @@ void ionic_bus_unmap_dbpage(struct ionic *ionic, void __iomem *page) > iounmap(page); > } > > +static void ionic_vf_dealloc(struct ionic *ionic) > +{ > + struct ionic_vf *v; > + int i; > + > + for (i = ionic->num_vfs - 1; i >= 0; i--) { Here you are checking for >= 0, so it must be 'int'. > + v = ionic->vf[i]; > + dma_unmap_single(ionic->dev, v->stats_pa, > + sizeof(v->stats), DMA_FROM_DEVICE); > + kfree(v); > + ionic->vf[i] = NULL; > + } > + > + ionic->num_vfs = 0; > + kfree(ionic->vf); > + ionic->vf = NULL; > +} > + > +static int ionic_vf_alloc(struct ionic *ionic, int num_vfs) > +{ > + struct ionic_vf *v; > + int err, i; > + int err; int i; > + ionic->vf = kcalloc(num_vfs, sizeof(struct ionic_vf *), GFP_KERNEL); > + if (!ionic->vf) > + return -ENOMEM; > + > + for (i = 0; i < num_vfs; i++) { > + v = kzalloc(sizeof(*v), GFP_KERNEL); > + if (!v) { > + err = -ENOMEM; > + goto err_out; > + } > + > + v->stats_pa = dma_map_single(ionic->dev, &v->stats, > + sizeof(v->stats), DMA_FROM_DEVICE); > + if (dma_mapping_error(ionic->dev, v->stats_pa)) { > + err = -ENODEV; > + kfree(v); > + ionic->vf[i] = NULL; > + goto err_out; > + } > + > + ionic->vf[i] = v; > + ionic->num_vfs++; > + } > + > + return 0; > + > +err_out: > + ionic_vf_dealloc(ionic); > + return err; > +} > + > +static int ionic_sriov_configure(struct pci_dev *pdev, int num_vfs) > +{ > + struct ionic *ionic = pci_get_drvdata(pdev); > + struct device *dev = ionic->dev; > + int ret = 0; > + > + down_write(&ionic->vf_op_lock); > + > + if (num_vfs > 0) { > + ret = pci_enable_sriov(pdev, num_vfs); > + if (ret) { > + dev_err(dev, "Cannot enable SRIOV: %d\n", ret); > + goto out; > + } > + > + ret = ionic_vf_alloc(ionic, num_vfs); > + if (ret) { > + dev_err(dev, "Cannot alloc VFs: %d\n", ret); > + pci_disable_sriov(pdev); > + goto out; > + } > + > + ret = num_vfs; > + goto out; > + } > + > + if (num_vfs == 0) { It should be the else of num_vfs > 0. > + pci_disable_sriov(pdev); > + ionic_vf_dealloc(ionic); > + } > + > +out: > + up_write(&ionic->vf_op_lock); > + return ret; > +} > + > static int ionic_probe(struct pci_dev *pdev, const struct pci_device_id *ent) > { > struct device *dev = &pdev->dev; > struct ionic *ionic; > + int num_vfs; > int err; > > ionic = ionic_devlink_alloc(dev); > @@ -118,6 +209,7 @@ static int ionic_probe(struct pci_dev *pdev, const struct pci_device_id *ent) > ionic->dev = dev; > pci_set_drvdata(pdev, ionic); > mutex_init(&ionic->dev_cmd_lock); > + init_rwsem(&ionic->vf_op_lock); > Better to initialize below before reading num_vfs, so all VF config code is adjcent to each other. > /* Query system for DMA addressing limitation for the device. */ > err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(IONIC_ADDR_LEN)); > @@ -206,6 +298,14 @@ static int ionic_probe(struct pci_dev *pdev, const struct pci_device_id *ent) > goto err_out_free_lifs; > } > > + num_vfs = pci_num_vf(pdev); > + if (num_vfs) { > + dev_info(dev, "%d VFs found already enabled\n", num_vfs); > + err = ionic_vf_alloc(ionic, num_vfs); > + if (err) > + dev_err(dev, "Cannot enable existing VFs: %d\n", err); > + } > + > err = ionic_lifs_register(ionic); > if (err) { > dev_err(dev, "Cannot register LIFs: %d, aborting\n", err); There should be error unwinding to perform vf_dealloc() where lifs_register or other functions fail in the probe sequence. Otherwise its memory leak bug. > @@ -279,6 +379,7 @@ static struct pci_driver ionic_driver = { > .id_table = ionic_id_table, > .probe = ionic_probe, > .remove = ionic_remove, > + .sriov_configure = ionic_sriov_configure, > }; > > int ionic_bus_register_driver(void) > diff --git a/drivers/net/ethernet/pensando/ionic/ionic_dev.c b/drivers/net/ethernet/pensando/ionic/ionic_dev.c > index 5f9d2ec70446..87f82f36812f 100644 > --- a/drivers/net/ethernet/pensando/ionic/ionic_dev.c > +++ b/drivers/net/ethernet/pensando/ionic/ionic_dev.c > @@ -286,6 +286,64 @@ void ionic_dev_cmd_port_pause(struct ionic_dev *idev, u8 pause_type) > ionic_dev_cmd_go(idev, &cmd); > } > > +/* VF commands */ > +int ionic_set_vf_config(struct ionic *ionic, int vf, u8 attr, u8 *data) > +{ > + union ionic_dev_cmd cmd = { > + .vf_setattr.opcode = IONIC_CMD_VF_SETATTR, > + .vf_setattr.attr = attr, > + .vf_setattr.vf_index = vf, > + }; > + int err; > + > + switch (attr) { > + case IONIC_VF_ATTR_SPOOFCHK: > + cmd.vf_setattr.spoofchk = *data; > + dev_dbg(ionic->dev, "%s: vf %d spoof %d\n", > + __func__, vf, *data); > + break; > + case IONIC_VF_ATTR_TRUST: > + cmd.vf_setattr.trust = *data; > + dev_dbg(ionic->dev, "%s: vf %d trust %d\n", > + __func__, vf, *data); > + break; > + case IONIC_VF_ATTR_LINKSTATE: > + cmd.vf_setattr.linkstate = *data; > + dev_dbg(ionic->dev, "%s: vf %d linkstate %d\n", > + __func__, vf, *data); > + break; > + case IONIC_VF_ATTR_MAC: > + ether_addr_copy(cmd.vf_setattr.macaddr, data); > + dev_dbg(ionic->dev, "%s: vf %d macaddr %pM\n", > + __func__, vf, data); > + break; > + case IONIC_VF_ATTR_VLAN: > + cmd.vf_setattr.vlanid = cpu_to_le16(*(u16 *)data); > + dev_dbg(ionic->dev, "%s: vf %d vlan %d\n", > + __func__, vf, *(u16 *)data); > + break; > + case IONIC_VF_ATTR_RATE: > + cmd.vf_setattr.maxrate = cpu_to_le32(*(u32 *)data); > + dev_dbg(ionic->dev, "%s: vf %d maxrate %d\n", > + __func__, vf, *(u32 *)data); > + break; > + case IONIC_VF_ATTR_STATSADDR: > + cmd.vf_setattr.stats_pa = cpu_to_le64(*(u64 *)data); > + dev_dbg(ionic->dev, "%s: vf %d stats_pa 0x%08llx\n", > + __func__, vf, *(u64 *)data); > + break; > + default: > + return -EINVAL; > + } > + > + mutex_lock(&ionic->dev_cmd_lock); > + ionic_dev_cmd_go(&ionic->idev, &cmd); > + err = ionic_dev_cmd_wait(ionic, DEVCMD_TIMEOUT); > + mutex_unlock(&ionic->dev_cmd_lock); > + > + return err; > +} > + > /* LIF commands */ > void ionic_dev_cmd_lif_identify(struct ionic_dev *idev, u8 type, u8 ver) > { > diff --git a/drivers/net/ethernet/pensando/ionic/ionic_dev.h b/drivers/net/ethernet/pensando/ionic/ionic_dev.h > index 4665c5dc5324..7838e342c4fd 100644 > --- a/drivers/net/ethernet/pensando/ionic/ionic_dev.h > +++ b/drivers/net/ethernet/pensando/ionic/ionic_dev.h > @@ -113,6 +113,12 @@ static_assert(sizeof(struct ionic_rxq_desc) == 16); > static_assert(sizeof(struct ionic_rxq_sg_desc) == 128); > static_assert(sizeof(struct ionic_rxq_comp) == 16); > > +/* SR/IOV */ > +static_assert(sizeof(struct ionic_vf_setattr_cmd) == 64); > +static_assert(sizeof(struct ionic_vf_setattr_comp) == 16); > +static_assert(sizeof(struct ionic_vf_getattr_cmd) == 64); > +static_assert(sizeof(struct ionic_vf_getattr_comp) == 16); > + > struct ionic_devinfo { > u8 asic_type; > u8 asic_rev; > @@ -275,6 +281,7 @@ void ionic_dev_cmd_port_autoneg(struct ionic_dev *idev, u8 an_enable); > void ionic_dev_cmd_port_fec(struct ionic_dev *idev, u8 fec_type); > void ionic_dev_cmd_port_pause(struct ionic_dev *idev, u8 pause_type); > > +int ionic_set_vf_config(struct ionic *ionic, int vf, u8 attr, u8 *data); > void ionic_dev_cmd_lif_identify(struct ionic_dev *idev, u8 type, u8 ver); > void ionic_dev_cmd_lif_init(struct ionic_dev *idev, u16 lif_index, > dma_addr_t addr); > diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c b/drivers/net/ethernet/pensando/ionic/ionic_lif.c > index 60fd14df49d7..6cd6ac1fff81 100644 > --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c > +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c > @@ -1616,6 +1616,202 @@ int ionic_stop(struct net_device *netdev) > return err; > } > > +static int ionic_get_vf_config(struct net_device *netdev, > + int vf, struct ifla_vf_info *ivf) > +{ > + struct ionic_lif *lif = netdev_priv(netdev); > + struct ionic *ionic = lif->ionic; > + > + if (vf >= ionic->num_vfs) > + return -EINVAL; > + Move this check as below after vf_op_lock as if (vf >= pci_num_vfs(pci_dev) return -EINVAL; Similarly for other friends function below.
Powered by blists - more mailing lists