| 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: <43F901BD926A4E43B106BF17856F07559A5CD81F@orsmsx508.amr.corp.intel.com>
Date: Wed, 23 Dec 2009 13:36:12 -0800
From: "Rose, Gregory V" <gregory.v.rose@...el.com>
To: Ben Hutchings <bhutchings@...arflare.com>,
"Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"gospo@...hat.com" <gospo@...hat.com>
Subject: RE: [RFC PATCH v2 02/12] ixgbevf: 82599 Virtual Function core
functions and header
>-----Original Message-----
>From: Ben Hutchings [mailto:bhutchings@...arflare.com]
>Sent: Wednesday, December 23, 2009 12:22 PM
>To: Kirsher, Jeffrey T
>Cc: netdev@...r.kernel.org; gospo@...hat.com; Rose, Gregory V
>Subject: Re: [RFC PATCH v2 02/12] ixgbevf: 82599 Virtual Function core
>functions and header
>
>On Fri, 2009-12-18 at 14:51 -0800, Jeff Kirsher wrote:
>> From: Greg Rose <gregory.v.rose@...el.com>
>>
>> This module and header file contain the core functions for the 82599
>> virtual function device.
>[...]
>> diff --git a/drivers/net/ixgbevf/vf.c b/drivers/net/ixgbevf/vf.c
>> new file mode 100644
>> index 0000000..38784eb
>> --- /dev/null
>> +++ b/drivers/net/ixgbevf/vf.c
>[...]
>> +/**
>> + * ixgbevf_set_rar_vf - set device MAC address
>> + * @hw: pointer to hardware structure
>> + * @index: Receive address register to write
>> + * @addr: Address to put into receive address register
>> + * @vmdq: VMDq "set" or "pool" index
>> + **/
>> +static s32 ixgbevf_set_rar_vf(struct ixgbe_hw *hw, u32 index, u8
>*addr,
>> + u32 vmdq)
>> +{
>> + struct ixgbe_mbx_info *mbx = &hw->mbx;
>> + u32 msgbuf[3];
>> + u8 *msg_addr = (u8 *)(&msgbuf[1]);
>> + s32 ret_val;
>> +
>> + memset(msgbuf, 0, 12);
>
>Seems like it would be clearer to write sizeof(msgbuf) rather than 12.
Sure, we can do that.
>But does it even matter that you clear the padding bytes?
I don't know, bits getting or'ed in and masked out all over the place and
MAC addresses that span 6 bytes over 2 32 bit words... It doesn't hurt right?
>
>[...]
>> +/**
>> + * ixgbevf_update_mc_addr_list_vf - Update Multicast addresses
>> + * @hw: pointer to the HW structure
>> + * @mc_addr_list: array of multicast addresses to program
>> + * @mc_addr_count: number of multicast addresses to program
>> + * @next: caller supplied function to return next address in list
>> + *
>> + * Updates the Multicast Table Array.
>> + **/
>> +static s32 ixgbevf_update_mc_addr_list_vf(struct ixgbe_hw *hw, u8
>*mc_addr_list,
>> + u32 mc_addr_count,
>> + ixgbe_mc_addr_itr next)
>> +{
>> + struct ixgbe_mbx_info *mbx = &hw->mbx;
>> + u32 msgbuf[IXGBE_VFMAILBOX_SIZE];
>> + u16 *vector_list = (u16 *)&msgbuf[1];
>> + u32 vector;
>> + u32 cnt, i;
>> + u32 vmdq;
>> +
>> + /* Each entry in the list uses 1 16 bit word. We have 30
>> + * 16 bit words available in our HW msg buffer (minus 1 for the
>> + * msg type). That's 30 hash values if we pack 'em right. If
>> + * there are more than 30 MC addresses to add then punt the
>> + * extras for now and then add code to handle more than 30 later.
>> + * It would be unusual for a server to request that many multi-
>cast
>> + * addresses except for in large enterprise network environments.
>> + */
>> + cnt = (mc_addr_count > 30) ? 30 : mc_addr_count;
>
>What, you don't want to support large enterprise networks now? ;-)
>You should switch to all-multicast behaviour rather than dropping
>multicast addresses.
In a future release yes, for now no.
>
>[...]
>> +/**
>> + * ixgbevf_get_mac_addr_vf - Read device MAC address
>> + * @hw: pointer to the HW structure
>
>The mac_addr parameter is missing.
>
>> + **/
>> +static s32 ixgbevf_get_mac_addr_vf(struct ixgbe_hw *hw, u8 *mac_addr)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < IXGBE_ETH_LENGTH_OF_ADDRESS; i++)
>> + mac_addr[i] = hw->mac.perm_addr[i];
>
>memcpy()?
Sure, why not.
>
>[...]
>> +/**
>> + * ixgbevf_setup_mac_link_vf - Setup MAC link settings
>> + * @hw: pointer to hardware structure
>> + * @speed: new link speed
>> + * @autoneg: true if autonegotiation enabled
>> + * @autoneg_wait_to_complete: true when waiting for completion is
>needed
>> + *
>> + * Set the link speed in the AUTOC register and restarts link.
>> + **/
>
>This description is obviously not correct.
Hah... you're obviously correct.
;-)
>
>> +static s32 ixgbevf_setup_mac_link_vf(struct ixgbe_hw *hw,
>> + ixgbe_link_speed speed, bool autoneg,
>> + bool autoneg_wait_to_complete)
>> +{
>> + return 0;
>> +}
>[...]
>> diff --git a/drivers/net/ixgbevf/vf.h b/drivers/net/ixgbevf/vf.h
>> new file mode 100644
>> index 0000000..799600e
>> --- /dev/null
>> +++ b/drivers/net/ixgbevf/vf.h
>[...]
>> +struct ixgbe_mac_operations {
>> + s32 (*init_hw)(struct ixgbe_hw *);
>> + s32 (*reset_hw)(struct ixgbe_hw *);
>> + s32 (*start_hw)(struct ixgbe_hw *);
>> + s32 (*clear_hw_cntrs)(struct ixgbe_hw *);
>> + enum ixgbe_media_type (*get_media_type)(struct ixgbe_hw *);
>> + u32 (*get_supported_physical_layer)(struct ixgbe_hw *);
>> + s32 (*get_mac_addr)(struct ixgbe_hw *, u8 *);
>> + s32 (*stop_adapter)(struct ixgbe_hw *);
>> + s32 (*get_bus_info)(struct ixgbe_hw *);
>> +
>> + /* Link */
>> + s32 (*setup_link)(struct ixgbe_hw *, ixgbe_link_speed, bool,
>bool);
>> + s32 (*check_link)(struct ixgbe_hw *, ixgbe_link_speed *, bool *,
>bool);
>> + s32 (*get_link_capabilities)(struct ixgbe_hw *, ixgbe_link_speed
>*,
>> + bool *);
>> +
>> + /* RAR, Multicast, VLAN */
>> + s32 (*set_rar)(struct ixgbe_hw *, u32, u8 *, u32);
>> + s32 (*init_rx_addrs)(struct ixgbe_hw *);
>> + s32 (*update_mc_addr_list)(struct ixgbe_hw *, u8 *, u32,
>> + ixgbe_mc_addr_itr);
>> + s32 (*enable_mc)(struct ixgbe_hw *);
>> + s32 (*disable_mc)(struct ixgbe_hw *);
>> + s32 (*clear_vfta)(struct ixgbe_hw *);
>> + s32 (*set_vfta)(struct ixgbe_hw *, u32, u32, bool);
>> +};
>> +
>> +enum ixgbe_mac_type {
>> + ixgbe_mac_unknown = 0,
>> + ixgbe_mac_82599_vf,
>> + ixgbe_num_macs
>> +};
>
>Is this abstraction really justified, given you have only one
>implementation?
Leaving room for future development.
>
>[...]
>> +struct ixgbe_mbx_operations {
>> + s32 (*init_params)(struct ixgbe_hw *hw);
>> + s32 (*read)(struct ixgbe_hw *, u32 *, u16);
>> + s32 (*write)(struct ixgbe_hw *, u32 *, u16);
>> + s32 (*read_posted)(struct ixgbe_hw *, u32 *, u16);
>> + s32 (*write_posted)(struct ixgbe_hw *, u32 *, u16);
>> + s32 (*check_for_msg)(struct ixgbe_hw *);
>> + s32 (*check_for_ack)(struct ixgbe_hw *);
>> + s32 (*check_for_rst)(struct ixgbe_hw *);
>> +};
>[...]
>
>Ditto for this.
We wanted some level of indirection for the mailbox stuff because this
Linux VF driver may run on top of hypervisors/VMMs in which they don't wish to
use our HW based mailbox for messaging between the PF and VF. This facilitates
that.
Thanks for the comments.
Regards,
- Greg
Powered by blists - more mailing lists