| 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
| ||
|
Message-ID: <CAE9FiQURffr_rMxBFG3JsR1an+XqH7u_Vy+chqQqj0EHuqfjcw@mail.gmail.com>
Date: Fri, 21 Sep 2012 12:23:50 -0700
From: Yinghai Lu <yinghai@...nel.org>
To: Ben Hutchings <bhutchings@...arflare.com>
Cc: "Rose, Gregory V" <gregory.v.rose@...el.com>,
Bjorn Helgaas <bhelgaas@...gle.com>,
Yuval Mintz <yuvalmin@...adcom.com>,
"davem@...emloft.net" <davem@...emloft.net>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
Ariel Elior <ariele@...adcom.com>,
Eilon Greenstein <eilong@...adcom.com>,
linux-pci <linux-pci@...r.kernel.org>
Subject: Re: New commands to configure IOV features
On Fri, Sep 21, 2012 at 10:35 AM, Ben Hutchings
<bhutchings@...arflare.com> wrote:
> On Thu, 2012-09-20 at 22:50 -0700, Yinghai Lu wrote:
> [...]
>> Index: linux-2.6/include/linux/pci.h
>> ===================================================================
>> --- linux-2.6.orig/include/linux/pci.h
>> +++ linux-2.6/include/linux/pci.h
>> @@ -663,6 +663,7 @@ struct pci_driver {
>> const struct pci_device_id *id_table; /* must be non-NULL for probe to be called */
>> int (*probe) (struct pci_dev *dev, const struct pci_device_id *id); /* New device inserted */
>> void (*remove) (struct pci_dev *dev); /* Device removed (NULL if not a hot-plug capable driver) */
>> + void (*set_max_vfs) (struct pci_dev *dev); /* enable sriov */
>> int (*suspend) (struct pci_dev *dev, pm_message_t state); /* Device suspended */
>> int (*suspend_late) (struct pci_dev *dev, pm_message_t state);
>> int (*resume_early) (struct pci_dev *dev);
>
> Not sure about this; the driver may fail to enable those VFs and it
> would be nice to be able to report that failure directly.
>
> That said, this is 'max_vfs' (a limit) and if the driver fails to set up
> all the VFs then the *limit* may still be changed.
>
>> Subject: [PATCH] PCI: Add max_vfs in sysfs per pci device where supports
>>
>> driver later need to check dev->max_vfs in /sys
>>
>> Signed-off-by: Yinghai Lu <yinghai@...nel.org>
> [...]
>> +static ssize_t
>> +max_vfs_store(struct device *dev, struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + unsigned long val;
>> + struct pci_dev *pdev = to_pci_dev(dev);
>> +
>> + if (strict_strtoul(buf, 0, &val) < 0)
>> + return -EINVAL;
>> +
>> + pdev->max_vfs = val;
>> +
>> + if (pdev->is_added) {
>
> No locking required here?
should be removed.
>
>> + int err;
>> + err = device_schedule_callback(dev, max_vfs_callback);
>
> Any particular reason this should be a callback?
no, just copied that from bus rescan.
>
> [...]
>> Index: linux-2.6/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> ===================================================================
>> --- linux-2.6.orig/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> +++ linux-2.6/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> @@ -129,13 +129,6 @@ static struct notifier_block dca_notifie
>> };
>> #endif
>>
>> -#ifdef CONFIG_PCI_IOV
>> -static unsigned int max_vfs;
>> -module_param(max_vfs, uint, 0);
>> -MODULE_PARM_DESC(max_vfs,
>> - "Maximum number of virtual functions to allocate per
>> physical function - default is zero and maximum value is 63");
>> -#endif /* CONFIG_PCI_IOV */
>> -
>> static unsigned int allow_unsupported_sfp;
>> module_param(allow_unsupported_sfp, uint, 0);
>> MODULE_PARM_DESC(allow_unsupported_sfp,
>> @@ -4528,7 +4521,7 @@ static int __devinit ixgbe_sw_init(struc
>> #ifdef CONFIG_PCI_IOV
>> /* assign number of SR-IOV VFs */
>> if (hw->mac.type != ixgbe_mac_82598EB)
>> - adapter->num_vfs = (max_vfs > 63) ? 0 : max_vfs;
>> + adapter->num_vfs = (pdev->max_vfs > 63) ? 0 : pdev->max_vfs;
>
> We are trying to make all SR-IOV capable drivers work the same, so this
> weird limiting behaviour should not be retained.
>
> So I think the correct assignment is:
> adapter->num_vfs = min(pdev->max_vfs, 63);
this will treat >63 as 63. old one treat > 63 as 0 aka disabled.
looks your version is more reasonable...
>
>> #endif
>> /* enable itr by default in dynamic mode */
>> @@ -7249,8 +7242,9 @@ static int __devinit ixgbe_probe(struct
>>
>> #ifdef CONFIG_PCI_IOV
>> ixgbe_enable_sriov(adapter, ii);
>> -
>> #endif
>> + adapter->ixgbe_info = ii;
>> +
>> netdev->features = NETIF_F_SG |
>> NETIF_F_IP_CSUM |
>> NETIF_F_IPV6_CSUM |
>> @@ -7720,11 +7714,42 @@ static const struct pci_error_handlers i
>> .resume = ixgbe_io_resume,
>> };
>>
>> +static void ixgbe_set_max_vfs(struct pci_dev *pdev)
>> +{
>> +#ifdef CONFIG_PCI_IOV
>> + struct ixgbe_adapter *adapter = pci_get_drvdata(pdev);
>> + struct net_device *netdev = adapter->netdev;
>> + struct ixgbe_hw *hw = &adapter->hw;
>> + int num_vfs = 0;
>> +
>> + /* assign number of SR-IOV VFs */
>> + if (hw->mac.type != ixgbe_mac_82598EB)
>> + num_vfs = (pdev->max_vfs > 63) ? 0 : pdev->max_vfs;
>> +
>> + /* no change */
>> + if (adapter->num_vfs == num_vfs)
>> + return;
>> +
>> + if (!num_vfs) {
>> + /* disable sriov */
>> + ixgbe_disable_sriov(adapter);
>> + adapter->num_vfs = 0;
>> + } else if (!adapter->num_vfs && num_vfs) {
>> + /* enable sriov */
>> + adapter->num_vfs = num_vfs;
>> + ixgbe_enable_sriov(adapter, adapter->ixgbe_info);
>> + } else {
>> + /* increase or decrease */
>
> Indeed, increase or decrease is not supported either in our PCI API or
> in the SR-IOV spec. I think I would prefer the PCI core to filter out
> unsupported changes (i.e. not call set_max_vfs and maybe report an
> error), but I'm not sure about that.
should still call set_max_vfs, and let it set finally valid max_vfs.
pci driver should know better which max_vfs is better for exact ...
-Yinghai
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists