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

From: KY Srinivasan
Date: Fri Apr 15 2016 - 12:01:19 EST

Next message: Denys Vlasenko: "[PATCH] nfs: make nfs_initialise_sb() static and deinline it"
Previous message: Arnaldo Carvalho de Melo: "Re: [PATCH 02/10] perf tools: Add perf_data_file__switch() helper"
In reply to: Alexander Duyck: "Re: [PATCH net-next 2/2] intel: ixgbevf: Support Windows hosts (Hyper-V)"
Next in thread: KY Srinivasan: "RE: [PATCH net-next 2/2] intel: ixgbevf: Support Windows hosts (Hyper-V)"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

> -----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.
> >
> >> > 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.
>
> >>
> >> > +/**
> >> > * 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.
> >>
> >> > @@ -776,22 +885,51 @@ static const struct ixgbe_mac_operations
> >> ixgbevf_mac_ops = {
> >> > .set_vfta = ixgbevf_set_vfta_vf,
> >> > };
> >> >
> >> > +static const struct ixgbe_mac_operations ixgbevf_hv_mac_ops = {
> >> > + .init_hw = ixgbevf_init_hw_vf,
> >> > + .reset_hw = ixgbevf_hv_reset_hw_vf,
> >> > + .start_hw = ixgbevf_start_hw_vf,
> >> > + .get_mac_addr = ixgbevf_get_mac_addr_vf,
> >> > + .stop_adapter = ixgbevf_stop_hw_vf,
> >> > + .setup_link = ixgbevf_setup_mac_link_vf,
> >> > + .check_link = ixgbevf_hv_check_mac_link_vf,
> >> > +};
> >>
> >> You might want to consider implementing a set_rar call that will
> >> return an error if you try to change the address to anything that is
> >> not the permanent addr.
> >
> > Ok; will do.
> >>
> >> > const struct ixgbevf_info ixgbevf_82599_vf_info = {
> >> > .mac = ixgbe_mac_82599_vf,
> >> > .mac_ops = &ixgbevf_mac_ops,
> >> > };
> >> >
> >> > +const struct ixgbevf_info ixgbevf_82599_vf_hv_info = {
> >> > + .mac = ixgbe_mac_82599_vf,
> >> > + .mac_ops = &ixgbevf_hv_mac_ops,
> >> > +};
> >> > +
> >> > const struct ixgbevf_info ixgbevf_X540_vf_info = {
> >> > .mac = ixgbe_mac_X540_vf,
> >> > .mac_ops = &ixgbevf_mac_ops,
> >> > };
> >> >
> >> > +const struct ixgbevf_info ixgbevf_X540_vf_hv_info = {
> >> > + .mac = ixgbe_mac_X540_vf,
> >> > + .mac_ops = &ixgbevf_hv_mac_ops,
> >> > +};
> >> > +
> >> > const struct ixgbevf_info ixgbevf_X550_vf_info = {
> >> > .mac = ixgbe_mac_X550_vf,
> >> > .mac_ops = &ixgbevf_mac_ops,
> >> > };
> >> >
> >> > +const struct ixgbevf_info ixgbevf_X550_vf_hv_info = {
> >> > + .mac = ixgbe_mac_X550_vf,
> >> > + .mac_ops = &ixgbevf_hv_mac_ops,
> >> > +};
> >> > +
> >> > const struct ixgbevf_info ixgbevf_X550EM_x_vf_info = {
> >> > .mac = ixgbe_mac_X550EM_x_vf,
> >> > .mac_ops = &ixgbevf_mac_ops,
> >> > };
> >> > +
> >> > +const struct ixgbevf_info ixgbevf_X550EM_x_vf_hv_info = {
> >> > + .mac = ixgbe_mac_X550EM_x_vf,
> >> > + .mac_ops = &ixgbevf_hv_mac_ops,
> >> > +};
> >> > diff --git a/drivers/net/ethernet/intel/ixgbevf/vf.h
> >> b/drivers/net/ethernet/intel/ixgbevf/vf.h
> >> > index ef9f773..7242a97 100644
> >> > --- a/drivers/net/ethernet/intel/ixgbevf/vf.h
> >> > +++ b/drivers/net/ethernet/intel/ixgbevf/vf.h
> >> > @@ -32,7 +32,9 @@
> >> > #include <linux/interrupt.h>
> >> > #include <linux/if_ether.h>
> >> > #include <linux/netdevice.h>
> >> > +#include <asm/hypervisor.h>
> >> >
> >> > +#include <asm/hypervisor.h>
> >>
> >> I don't think you need to include this twice. Also this is a arch
> >> specific header file. You might want to move this to vf.c since that
> >> is where you are using the code it provides. Then you can probably
> >> wrap it in an X86 build check so that you don't break the build on
> >> other architectures.
> >
> > I will fix this. Thank you for your detailed comments.
>
> Yeah, if we can get away from including this entirely it would be
> preferred. Odds are we probably don't need it since the device ID is
> unique to HyperV hosts anyway.
>
> I'll keep an eye out for the next patch set.

Thank you; really appreciate all your help.

Regards,

K. Y
>
> - Alex

Next message: Denys Vlasenko: "[PATCH] nfs: make nfs_initialise_sb() static and deinline it"
Previous message: Arnaldo Carvalho de Melo: "Re: [PATCH 02/10] perf tools: Add perf_data_file__switch() helper"
In reply to: Alexander Duyck: "Re: [PATCH net-next 2/2] intel: ixgbevf: Support Windows hosts (Hyper-V)"
Next in thread: KY Srinivasan: "RE: [PATCH net-next 2/2] intel: ixgbevf: Support Windows hosts (Hyper-V)"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]