RE: [PATCH net-next 2/2] intel: ixgbevf: Support Windows hosts (Hyper-V)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----Original Message-----
> From: KY Srinivasan
> Sent: Friday, April 15, 2016 9:01 AM
> To: 'Alexander Duyck' <alexander.duyck@xxxxxxxxx>
> Cc: David Miller <davem@xxxxxxxxxxxxx>; Netdev
> <netdev@xxxxxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx;
> devel@xxxxxxxxxxxxxxxxxxxxxx; olaf@xxxxxxxxx; Robo Bot
> <apw@xxxxxxxxxxxxx>; Jason Wang <jasowang@xxxxxxxxxx>;
> eli@xxxxxxxxxxxx; jackm@xxxxxxxxxxxx; yevgenyp@xxxxxxxxxxxx; John
> Ronciak <john.ronciak@xxxxxxxxx>; intel-wired-lan <intel-wired-
> lan@xxxxxxxxxxxxxxxx>
> Subject: RE: [PATCH net-next 2/2] intel: ixgbevf: Support Windows hosts
> (Hyper-V)
> 
> 
> 
> > -----Original Message-----
> > From: Alexander Duyck [mailto:alexander.duyck@xxxxxxxxx]
> > Sent: Friday, April 15, 2016 8:40 AM
> > To: KY Srinivasan <kys@xxxxxxxxxxxxx>
> > Cc: David Miller <davem@xxxxxxxxxxxxx>; Netdev
> > <netdev@xxxxxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx;
> > devel@xxxxxxxxxxxxxxxxxxxxxx; olaf@xxxxxxxxx; Robo Bot
> > <apw@xxxxxxxxxxxxx>; Jason Wang <jasowang@xxxxxxxxxx>;
> > eli@xxxxxxxxxxxx; jackm@xxxxxxxxxxxx; yevgenyp@xxxxxxxxxxxx; John
> > Ronciak <john.ronciak@xxxxxxxxx>; intel-wired-lan <intel-wired-
> > lan@xxxxxxxxxxxxxxxx>
> > Subject: Re: [PATCH net-next 2/2] intel: ixgbevf: Support Windows hosts
> > (Hyper-V)
> >
> > On Thu, Apr 14, 2016 at 7:49 PM, KY Srinivasan <kys@xxxxxxxxxxxxx>
> wrote:
> > >
> > >
> > >> -----Original Message-----
> > >> From: Alexander Duyck [mailto:alexander.duyck@xxxxxxxxx]
> > >> Sent: Thursday, April 14, 2016 4:18 PM
> > >> To: KY Srinivasan <kys@xxxxxxxxxxxxx>
> > >> Cc: David Miller <davem@xxxxxxxxxxxxx>; Netdev
> > >> <netdev@xxxxxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx;
> > >> devel@xxxxxxxxxxxxxxxxxxxxxx; olaf@xxxxxxxxx; Robo Bot
> > >> <apw@xxxxxxxxxxxxx>; Jason Wang <jasowang@xxxxxxxxxx>;
> > >> eli@xxxxxxxxxxxx; jackm@xxxxxxxxxxxx; yevgenyp@xxxxxxxxxxxx;
> > John
> > >> Ronciak <john.ronciak@xxxxxxxxx>; intel-wired-lan <intel-wired-
> > >> lan@xxxxxxxxxxxxxxxx>
> > >> Subject: Re: [PATCH net-next 2/2] intel: ixgbevf: Support Windows
> hosts
> > >> (Hyper-V)
> > >>
> > >> On Thu, Apr 14, 2016 at 4:55 PM, K. Y. Srinivasan <kys@xxxxxxxxxxxxx>
> > >> wrote:
> > >> > On Hyper-V, the VF/PF communication is a via software mediated
> path
> > >> > as opposed to the hardware mailbox. Make the necessary
> > >> > adjustments to support Hyper-V.
> > >> >
> > >> > Signed-off-by: K. Y. Srinivasan <kys@xxxxxxxxxxxxx>
> > >> > ---
> > >> >  drivers/net/ethernet/intel/ixgbevf/ixgbevf.h      |   11 ++
> > >> >  drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |   56 ++++++---
> > >> >  drivers/net/ethernet/intel/ixgbevf/mbx.c          |   12 ++
> > >> >  drivers/net/ethernet/intel/ixgbevf/vf.c           |  138
> > >> +++++++++++++++++++++
> > >> >  drivers/net/ethernet/intel/ixgbevf/vf.h           |    2 +
> > >> >  5 files changed, 201 insertions(+), 18 deletions(-)
> > >> >
> > >> > diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
> > >> b/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
> > >> > index 5ac60ee..f8d2a0b 100644
> > >> > --- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
> > >> > +++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
> > >> > @@ -460,9 +460,13 @@ enum ixbgevf_state_t {
> > >> >
> > >> >  enum ixgbevf_boards {
> > >> >         board_82599_vf,
> > >> > +       board_82599_vf_hv,
> > >> >         board_X540_vf,
> > >> > +       board_X540_vf_hv,
> > >> >         board_X550_vf,
> > >> > +       board_X550_vf_hv,
> > >> >         board_X550EM_x_vf,
> > >> > +       board_X550EM_x_vf_hv,
> > >> >  };
> > >> >
> > >> >  enum ixgbevf_xcast_modes {
> > >> > @@ -477,6 +481,13 @@ extern const struct ixgbevf_info
> > >> ixgbevf_X550_vf_info;
> > >> >  extern const struct ixgbevf_info ixgbevf_X550EM_x_vf_info;
> > >> >  extern const struct ixgbe_mbx_operations ixgbevf_mbx_ops;
> > >> >
> > >> > +
> > >> > +extern const struct ixgbevf_info ixgbevf_82599_vf_hv_info;
> > >> > +extern const struct ixgbevf_info ixgbevf_X540_vf_hv_info;
> > >> > +extern const struct ixgbevf_info ixgbevf_X550_vf_hv_info;
> > >> > +extern const struct ixgbevf_info ixgbevf_X550EM_x_vf_hv_info;
> > >> > +extern const struct ixgbe_mbx_operations ixgbevf_hv_mbx_ops;
> > >> > +
> > >> >  /* needed by ethtool.c */
> > >> >  extern const char ixgbevf_driver_name[];
> > >> >  extern const char ixgbevf_driver_version[];
> > >> > diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
> > >> b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
> > >> > index 007cbe0..4a0ffac 100644
> > >> > --- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
> > >> > +++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
> > >> > @@ -49,6 +49,7 @@
> > >> >  #include <linux/if.h>
> > >> >  #include <linux/if_vlan.h>
> > >> >  #include <linux/prefetch.h>
> > >> > +#include <asm/mshyperv.h>
> > >> >
> > >> >  #include "ixgbevf.h"
> > >> >
> > >> > @@ -62,10 +63,14 @@ static char ixgbevf_copyright[] =
> > >> >         "Copyright (c) 2009 - 2015 Intel Corporation.";
> > >> >
> > >> >  static const struct ixgbevf_info *ixgbevf_info_tbl[] = {
> > >> > -       [board_82599_vf] = &ixgbevf_82599_vf_info,
> > >> > -       [board_X540_vf]  = &ixgbevf_X540_vf_info,
> > >> > -       [board_X550_vf]  = &ixgbevf_X550_vf_info,
> > >> > -       [board_X550EM_x_vf] = &ixgbevf_X550EM_x_vf_info,
> > >> > +       [board_82599_vf]        = &ixgbevf_82599_vf_info,
> > >> > +       [board_82599_vf_hv]     = &ixgbevf_82599_vf_hv_info,
> > >> > +       [board_X540_vf]         = &ixgbevf_X540_vf_info,
> > >> > +       [board_X540_vf_hv]      = &ixgbevf_X540_vf_hv_info,
> > >> > +       [board_X550_vf]         = &ixgbevf_X550_vf_info,
> > >> > +       [board_X550_vf_hv]      = &ixgbevf_X550_vf_hv_info,
> > >> > +       [board_X550EM_x_vf]     = &ixgbevf_X550EM_x_vf_info,
> > >> > +       [board_X550EM_x_vf_hv]  = &ixgbevf_X550EM_x_vf_hv_info,
> > >> >  };
> > >> >
> > >> >  /* ixgbevf_pci_tbl - PCI Device ID Table
> > >> > @@ -78,9 +83,13 @@ static const struct ixgbevf_info
> > *ixgbevf_info_tbl[] =
> > >> {
> > >> >   */
> > >> >  static const struct pci_device_id ixgbevf_pci_tbl[] = {
> > >> >         {PCI_VDEVICE(INTEL, IXGBE_DEV_ID_82599_VF), board_82599_vf
> },
> > >> > +       {PCI_VDEVICE(INTEL, IXGBE_DEV_ID_82599_VF_HV),
> > >> board_82599_vf_hv },
> > >> >         {PCI_VDEVICE(INTEL, IXGBE_DEV_ID_X540_VF), board_X540_vf },
> > >> > +       {PCI_VDEVICE(INTEL, IXGBE_DEV_ID_X540_VF_HV),
> > >> board_X540_vf_hv },
> > >> >         {PCI_VDEVICE(INTEL, IXGBE_DEV_ID_X550_VF), board_X550_vf },
> > >> > +       {PCI_VDEVICE(INTEL, IXGBE_DEV_ID_X550_VF_HV),
> > >> board_X550_vf_hv },
> > >> >         {PCI_VDEVICE(INTEL, IXGBE_DEV_ID_X550EM_X_VF),
> > >> board_X550EM_x_vf },
> > >> > +       {PCI_VDEVICE(INTEL, IXGBE_DEV_ID_X550EM_X_VF_HV),
> > >> board_X550EM_x_vf_hv},
> > >> >         /* required last entry */
> > >> >         {0, }
> > >> >  };
> > >> > @@ -1809,12 +1818,13 @@ static int ixgbevf_vlan_rx_add_vid(struct
> > >> net_device *netdev,
> > >> >  {
> > >> >         struct ixgbevf_adapter *adapter = netdev_priv(netdev);
> > >> >         struct ixgbe_hw *hw = &adapter->hw;
> > >> > -       int err;
> > >> > +       int err = 0;
> > >> >
> > >> >         spin_lock_bh(&adapter->mbx_lock);
> > >> >
> > >> >         /* add VID to filter table */
> > >> > -       err = hw->mac.ops.set_vfta(hw, vid, 0, true);
> > >> > +       if (hw->mac.ops.set_vfta)
> > >> > +               err = hw->mac.ops.set_vfta(hw, vid, 0, true);
> > >> >
> > >> >         spin_unlock_bh(&adapter->mbx_lock);
> > >> >
> > >> > @@ -1835,12 +1845,13 @@ static int ixgbevf_vlan_rx_kill_vid(struct
> > >> net_device *netdev,
> > >> >  {
> > >> >         struct ixgbevf_adapter *adapter = netdev_priv(netdev);
> > >> >         struct ixgbe_hw *hw = &adapter->hw;
> > >> > -       int err;
> > >> > +       int err = 0;
> > >> >
> > >> >         spin_lock_bh(&adapter->mbx_lock);
> > >> >
> > >> >         /* remove VID from filter table */
> > >> > -       err = hw->mac.ops.set_vfta(hw, vid, 0, false);
> > >> > +       if (hw->mac.ops.set_vfta)
> > >> > +               err = hw->mac.ops.set_vfta(hw, vid, 0, false);
> > >> >
> > >> >         spin_unlock_bh(&adapter->mbx_lock);
> > >> >
> > >> > @@ -1873,14 +1884,16 @@ static int
> ixgbevf_write_uc_addr_list(struct
> > >> net_device *netdev)
> > >> >                 struct netdev_hw_addr *ha;
> > >> >
> > >> >                 netdev_for_each_uc_addr(ha, netdev) {
> > >> > -                       hw->mac.ops.set_uc_addr(hw, ++count, ha->addr);
> > >> > +                       if (hw->mac.ops.set_uc_addr)
> > >> > +                               hw->mac.ops.set_uc_addr(hw, ++count, ha->addr);
> > >> >                         udelay(200);
> > >> >                 }
> > >> >         } else {
> > >> >                 /* If the list is empty then send message to PF driver to
> > >> >                  * clear all MAC VLANs on this VF.
> > >> >                  */
> > >> > -               hw->mac.ops.set_uc_addr(hw, 0, NULL);
> > >> > +               if (hw->mac.ops.set_uc_addr)
> > >> > +                       hw->mac.ops.set_uc_addr(hw, 0, NULL);
> > >> >         }
> > >> >
> > >> >         return count;
> > >>
> > >> So if I am understanding this correctly it looks like you cannot read
> > >> or write any addresses for this device.  Would I be correct in
> > >> assuming that you get to have the one unicast address that is provided
> > >> by the PF and that is it?
> > >
> > > That is what I have been told by the Windows folks at Intel.
> >
> > Yeah, so I didn't see the other half of this patchset that has you
> > using a software interface for the multicast and broadcast traffic.
> > What I would recommend doing is just writing up a stub function you
> > can put in vf.c that will allow you to avoid having to add all these
> > if checks to the code.
> >
> > >> If so we may want to look at possibly returning some sort of error on
> > >> these calls so that we can configure the device to indicate that we
> > >> cannot support any of these filters.
> > >
> > > I will do that. So, I am going to check the device ID and return an error.
> > > Would IXGBE_NOT_IMPLEMENTED be appropriate?
> >
> > I'd say don't bother returning an error.  You can probably just stub
> > out the function since if I recall correctly it is a void function
> > anyway and doesn't provide a return value.
> >
> > >>
> > >> > @@ -1908,10 +1921,13 @@ static void ixgbevf_set_rx_mode(struct
> > >> net_device *netdev)
> > >> >
> > >> >         spin_lock_bh(&adapter->mbx_lock);
> > >> >
> > >> > -       hw->mac.ops.update_xcast_mode(hw, netdev, xcast_mode);
> > >> > +       if (hw->mac.ops.update_mc_addr_list)
> > >> > +               if (hw->mac.ops.update_xcast_mode)
> > >> > +                       hw->mac.ops.update_xcast_mode(hw, netdev,
> > xcast_mode);
> > >> >
> > >> >         /* reprogram multicast list */
> > >> > -       hw->mac.ops.update_mc_addr_list(hw, netdev);
> > >> > +       if (hw->mac.ops.update_mc_addr_list)
> > >> > +               hw->mac.ops.update_mc_addr_list(hw, netdev);
> > >> >
> > >> >         ixgbevf_write_uc_addr_list(netdev);
> > >> >
> >
> > Same thing for the multicast list as well.
> >
> > >> > @@ -2074,10 +2090,13 @@ static void ixgbevf_up_complete(struct
> > >> ixgbevf_adapter *adapter)
> > >> >
> > >> >         spin_lock_bh(&adapter->mbx_lock);
> > >> >
> > >> > -       if (is_valid_ether_addr(hw->mac.addr))
> > >> > -               hw->mac.ops.set_rar(hw, 0, hw->mac.addr, 0);
> > >> > -       else
> > >> > -               hw->mac.ops.set_rar(hw, 0, hw->mac.perm_addr, 0);
> > >> > +       if (is_valid_ether_addr(hw->mac.addr)) {
> > >> > +               if (hw->mac.ops.set_rar)
> > >> > +                       hw->mac.ops.set_rar(hw, 0, hw->mac.addr, 0);
> > >> > +       } else {
> > >> > +               if (hw->mac.ops.set_rar)
> > >> > +                       hw->mac.ops.set_rar(hw, 0, hw->mac.perm_addr, 0);
> > >> > +       }
> > >> >
> > >> >         spin_unlock_bh(&adapter->mbx_lock);
> > >> >
> > >>
> > >> Same here.  We shouldn't let the user set a MAC address that we
> cannot
> > >> support.  We should be returning an error.
> > >
> > > Yes; I will return an error.
> >
> > This is the one that needs to return an error.  That way we should be
> > able to prevent MAC address changes.
> >
> > >>
> > >> > @@ -3672,14 +3691,15 @@ static int ixgbevf_set_mac(struct
> net_device
> > >> *netdev, void *p)
> > >> >         struct ixgbevf_adapter *adapter = netdev_priv(netdev);
> > >> >         struct ixgbe_hw *hw = &adapter->hw;
> > >> >         struct sockaddr *addr = p;
> > >> > -       int err;
> > >> > +       int err = 0;
> > >> >
> > >> >         if (!is_valid_ether_addr(addr->sa_data))
> > >> >                 return -EADDRNOTAVAIL;
> > >> >
> > >> >         spin_lock_bh(&adapter->mbx_lock);
> > >> >
> > >> > -       err = hw->mac.ops.set_rar(hw, 0, addr->sa_data, 0);
> > >> > +       if (hw->mac.ops.set_rar)
> > >> > +               err = hw->mac.ops.set_rar(hw, 0, addr->sa_data, 0);
> > >> >
> > >> >         spin_unlock_bh(&adapter->mbx_lock);
> > >> >
> > >>
> > >> Specifically here.  If hw->mac.ops.set_rar is NULL we cannot allow any
> > >> MAC address change so we should probably return "-EADDRNOTAVAIL".
> > >
> > > Will do.
> > >>
> > >> > diff --git a/drivers/net/ethernet/intel/ixgbevf/mbx.c
> > >> b/drivers/net/ethernet/intel/ixgbevf/mbx.c
> > >> > index dc68fea..298a0da 100644
> > >> > --- a/drivers/net/ethernet/intel/ixgbevf/mbx.c
> > >> > +++ b/drivers/net/ethernet/intel/ixgbevf/mbx.c
> > >> > @@ -346,3 +346,15 @@ const struct ixgbe_mbx_operations
> > >> ixgbevf_mbx_ops = {
> > >> >         .check_for_rst  = ixgbevf_check_for_rst_vf,
> > >> >  };
> > >> >
> > >> > +/**
> > >> > + * Mailbox operations when running on Hyper-V.
> > >> > + * On Hyper-V, PF/VF communiction is not through the
> > >> > + * hardware mailbox; this communication is through
> > >> > + * a software mediated path.
> > >> > + * Most mail box operations are noop while running on
> > >> > + * Hyper-V.
> > >> > + */
> > >> > +const struct ixgbe_mbx_operations ixgbevf_hv_mbx_ops = {
> > >> > +       .init_params    = ixgbevf_init_mbx_params_vf,
> > >> > +       .check_for_rst  = ixgbevf_check_for_rst_vf,
> > >> > +};
> > >>
> > >> I'd say if you aren't going to use a mailbox then don't initialize it.
> > >> The code was based off of the same code that runs the ixgbe driver.
> > >> It should be able to function without a mailbox provided the necessary
> > >> calls are updated in the ixgbe_mac_operations that are used by the
> > >> hyperv VF.
> > >>
> > > Ok; I will change the code.

As I am working on addressing your comments, while most mailbox operations are not
used, the mechanism for checking for reset (check_for_rst) is identical even on Hyper-V.
So keeping Hyper-V specific mailbox operations may actually result in fewer changes.
I will shortly send you the updated patch for your review.
 
> > >
> > >> > diff --git a/drivers/net/ethernet/intel/ixgbevf/vf.c
> > >> b/drivers/net/ethernet/intel/ixgbevf/vf.c
> > >> > index 4d613a4..92397fd 100644
> > >> > --- a/drivers/net/ethernet/intel/ixgbevf/vf.c
> > >> > +++ b/drivers/net/ethernet/intel/ixgbevf/vf.c
> > >> > @@ -27,6 +27,13 @@
> > >> >  #include "vf.h"
> > >> >  #include "ixgbevf.h"
> > >> >
> > >> > +/*
> > >> > + * On Hyper-V, to reset, we need to read from this offset
> > >> > + * from the PCI config space. This is the mechanism used on
> > >> > + * Hyper-V to support PF/VF communication.
> > >> > + */
> > >> > +#define IXGBE_HV_RESET_OFFSET           0x201
> > >> > +
> > >> >  /**
> > >> >   *  ixgbevf_start_hw_vf - Prepare hardware for Tx/Rx
> > >> >   *  @hw: pointer to hardware structure
> > >> > @@ -126,6 +133,23 @@ static s32 ixgbevf_reset_hw_vf(struct
> > ixgbe_hw
> > >> *hw)
> > >> >  }
> > >> >
> > >> >  /**
> > >> > + * Hyper-V variant; the VF/PF communication is through the PCI
> > >> > + * config space.
> > >> > + */
> > >> > +static s32 ixgbevf_hv_reset_hw_vf(struct ixgbe_hw *hw)
> > >> > +{
> > >> > +       int i;
> > >> > +       struct ixgbevf_adapter *adapter = hw->back;
> > >> > +
> > >> > +       for (i = 0; i < 6; i++)
> > >> > +               pci_read_config_byte(adapter->pdev,
> > >> > +                                    (i + IXGBE_HV_RESET_OFFSET),
> > >> > +                                    &hw->mac.perm_addr[i]);
> > >> > +
> > >> > +       return 0;
> > >> > +}
> > >> > +
> > >>
> > >> This bit can be problematic.  What about the case where
> PCI_MMCONFIG
> > >> is not defined.  In such a case it will kill this function without
> > >> reporting an error other than maybe a MAC address that is all 0s or
> > >> all FF's.
> > >>
> > >> You might want to add some sort of check here with some message if
> > >> such a situation occurs just so it can be easier to debug.
> > >
> > > I am a little confused here. This function will only execute when we are
> > handling Hyper-V
> > > device IDs (while running on Hyper-V). Hyper-V PCI pass-through driver
> will
> > support the
> > > config space access.
> >
> > The kernel has a configuration option called CONFIG_PCI_MMCONFIG.  On
> > x86 it is usually enabled by default, but it can be disabled.
> >
> > Right now this bit of code is dependent on it being enabled in order
> > to function correctly.  Otherwise you will only have access to the
> > first 256 bytes of the PCI configuration space.  You might want to
> > explore the possibility of a Kconfig option that would allow you to
> > only support the HyperV VFs if CONFIG_PCI_MMCONFIG is enabled.
> 
> Our PCI pss-through driver depends on this functionality. I will make sure we
> Make that dependency more explicit.
> >
> > >>
> > >> > +/**
> > >> >   *  ixgbevf_stop_hw_vf - Generic stop Tx/Rx units
> > >> >   *  @hw: pointer to hardware structure
> > >> >   *
> > >> > @@ -656,6 +680,68 @@ out:
> > >> >  }
> > >> >
> > >> >  /**
> > >> > + * Hyper-V variant; there is no mailbox communication.
> > >> > + */
> > >> > +static s32 ixgbevf_hv_check_mac_link_vf(struct ixgbe_hw *hw,
> > >> > +                                       ixgbe_link_speed *speed,
> > >> > +                                       bool *link_up,
> > >> > +                                       bool autoneg_wait_to_complete)
> > >> > +{
> > >> > +       struct ixgbe_mbx_info *mbx = &hw->mbx;
> > >> > +       struct ixgbe_mac_info *mac = &hw->mac;
> > >> > +       s32 ret_val = 0;
> > >> > +       u32 links_reg;
> > >> > +
> > >> > +       /* If we were hit with a reset drop the link */
> > >> > +       if (!mbx->ops.check_for_rst(hw) || !mbx->timeout)
> > >> > +               mac->get_link_status = true;
> > >> > +
> > >> > +       if (!mac->get_link_status)
> > >> > +               goto out;
> > >> > +
> > >> > +       /* if link status is down no point in checking to see if pf is up */
> > >> > +       links_reg = IXGBE_READ_REG(hw, IXGBE_VFLINKS);
> > >> > +       if (!(links_reg & IXGBE_LINKS_UP))
> > >> > +               goto out;
> > >> > +
> > >> > +       /* for SFP+ modules and DA cables on 82599 it can take up to
> > 500usecs
> > >> > +        * before the link status is correct
> > >> > +        */
> > >> > +       if (mac->type == ixgbe_mac_82599_vf) {
> > >> > +               int i;
> > >> > +
> > >> > +               for (i = 0; i < 5; i++) {
> > >> > +                       udelay(100);
> > >> > +                       links_reg = IXGBE_READ_REG(hw, IXGBE_VFLINKS);
> > >> > +
> > >> > +                       if (!(links_reg & IXGBE_LINKS_UP))
> > >> > +                               goto out;
> > >> > +               }
> > >> > +       }
> > >> > +
> > >> > +       switch (links_reg & IXGBE_LINKS_SPEED_82599) {
> > >> > +       case IXGBE_LINKS_SPEED_10G_82599:
> > >> > +               *speed = IXGBE_LINK_SPEED_10GB_FULL;
> > >> > +               break;
> > >> > +       case IXGBE_LINKS_SPEED_1G_82599:
> > >> > +               *speed = IXGBE_LINK_SPEED_1GB_FULL;
> > >> > +               break;
> > >> > +       case IXGBE_LINKS_SPEED_100_82599:
> > >> > +               *speed = IXGBE_LINK_SPEED_100_FULL;
> > >> > +               break;
> > >> > +       }
> > >> > +
> > >> > +       /* if we passed all the tests above then the link is up and we no
> > >> > +        * longer need to check for link
> > >> > +        */
> > >> > +       mac->get_link_status = false;
> > >> > +
> > >> > +out:
> > >> > +       *link_up = !mac->get_link_status;
> > >> > +       return ret_val;
> > >> > +}
> > >> > +
> > >>
> > >> How does this handle the PF resetting?  Seems like you are going to be
> > >> generating Tx hangs in such a case.
> > >
> > > I am not sure how the Windows PF driver communicates this information.
> > > Do you have any suggestions for this.
> >
> > You might want to take a look at the fm10k_get_host_state_generic
> > function.  You could probably do something like the trick we did there
> > with the TXDCTL in order to detect cases where the PF has reset the VF
> > so that you could then reset your guest once the PF has come back up.
> > That way at least you would report link down instead of Tx hang if the
> > host has reset you.
> 
> I will take a look at your example; thank you.

The code you have in fm10k_get_host_state_generic function is somewhat similar
to what I have in the function ixgbevf_hv_check_mac_link_vf(). We drop the link
if the PF has reset us. I will look at this code some more.

> >
> > >>
> > >> > +/**
> > >> >   *  ixgbevf_rlpml_set_vf - Set the maximum receive packet length
> > >> >   *  @hw: pointer to the HW structure
> > >> >   *  @max_size: value to assign to max frame size
> > >> > @@ -663,6 +749,19 @@ out:
> > >> >  void ixgbevf_rlpml_set_vf(struct ixgbe_hw *hw, u16 max_size)
> > >> >  {
> > >> >         u32 msgbuf[2];
> > >> > +       u32 reg;
> > >> > +
> > >> > +       if (x86_hyper == &x86_hyper_ms_hyperv) {
> > >> > +               /*
> > >> > +                * If we are on Hyper-V, we implement
> > >> > +                * this functionality differently.
> > >> > +                */
> > >> > +               reg =  IXGBE_READ_REG(hw, IXGBE_VFRXDCTL(0));
> > >> > +               /* CRC == 4 */
> > >> > +               reg |= ((max_size + 4) | IXGBE_RXDCTL_RLPML_EN);
> > >> > +               IXGBE_WRITE_REG(hw, IXGBE_VFRXDCTL(0), reg);
> > >> > +               return;
> > >> > +       }
> > >> >
> > >> >         msgbuf[0] = IXGBE_VF_SET_LPE;
> > >> >         msgbuf[1] = max_size;
> > >>
> > >> You would be better off just implementing a hyperv version of this
> > >> function and avoiding the mailbox call entirely.
> > > Ok; will do.
> > >
> > >>
> > >> > @@ -679,6 +778,16 @@ int ixgbevf_negotiate_api_version(struct
> > >> ixgbe_hw *hw, int api)
> > >> >         int err;
> > >> >         u32 msg[3];
> > >> >
> > >> > +       if (x86_hyper == &x86_hyper_ms_hyperv) {
> > >> > +               /*
> > >> > +                * Hyper-V only supports api version ixgbe_mbox_api_10
> > >> > +                */
> > >> > +               if (api != ixgbe_mbox_api_10)
> > >> > +                       return IXGBE_ERR_INVALID_ARGUMENT;
> > >> > +
> > >> > +               return 0;
> > >> > +       }
> > >> > +
> > >> >         /* Negotiate the mailbox API version */
> > >> >         msg[0] = IXGBE_VF_API_NEGOTIATE;
> > >> >         msg[1] = api;
> > >>
> > >> Same here.  Just implement a hyperv version of this function instead
> > >> of splicing into the existing call.  Also you will need to wrap this
> > >> code up so that we can build on all architectures, not just x86.
> > >
> > > Ok; will do.

I now have a check for Hyper-V that is based on the mailbox operations
and that should address compilation issues on non x86 platforms. I will post the new set shortly.

Regards,

K. Y
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel



[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux