Add support for userspace device drivers with HW offload mode #322

kim-tae-kyung · 2024-07-27T03:22:30Z

Add support for SR-IOV Virtual Functions (VFs) using both hardware offload (switchdev) and userspace device drivers such as vfio-pci.
If the MAC address is provided via arguments, update the VF’s MAC address accordingly using netlink.

What this PR does / why we need it:

This feature is essential for scenarios where SR-IOV VFs require both userspace device drivers and hardware offload.
For instance, containerized network functions with Intel E810 may enable eswitch switchdev mode for hardware offload while using the vfio-pci driver for running DPDK or VPP.
Another use case is the KubeVirt SR-IOV Interface.
After merging this PR, users can create a KubeVirt VM supported by OVS hardware offload without modifying the KubeVirt source code.

Special notes for your reviewer:

Even in scenarios where userspace device drivers are employed, and there is no netdevice for the VF on the host, the CNI should still return the name and sandbox for the VF interface to be recognized by Multus.

Release note:

- Support HW offload with userspace device driver
- Support MAC address update in HW offload mode

kubevirt-bot · 2024-07-27T03:22:39Z

Hi @kim-tae-kyung. Thanks for your PR.

I'm waiting for a k8snetworkplumbingwg member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

phoracek · 2024-07-27T20:25:23Z

@kim-tae-kyung thanks for the PR.

@ykulazhenkov any chance you would be interested in reviewing this? Sadly, I'm not all that familiar with the SR-IOV portion of the project, and I don't feel confident reviewing this.

ykulazhenkov

Thx for the PR @kim-tae-kyung.

I added few comments/questions

ykulazhenkov · 2024-08-05T08:46:43Z

pkg/sriov/sriov.go

+		// set VF MAC address if provided, otherwise get it from the VF
+		// use the PF netlink, since there is no netdevice for the VF
+		if mac != "" {
+			if err := netlink.LinkSetVfHardwareAddr(pfLink, vfIdx, hwaddr); err != nil {


In the sriov-cni hardware address for VFs is configured for userspace drivers also. I think we need to do the same.

https://github.com/k8snetworkplumbingwg/sriov-cni/blob/176bd65bbe9129203f51dc686e2f13a179dc3da5/cmd/sriov/main.go#L106

https://github.com/k8snetworkplumbingwg/sriov-cni/blob/176bd65bbe9129203f51dc686e2f13a179dc3da5/pkg/sriov/sriov.go#L258

For userspace - configure mac with LinkSetVfHardwareAddr
For kernel net - configure mac with LinkSetVfHardwareAddr + LinkSetHardwareAddr (for VF net dev)

Done. For kernel net, execute ip link set $link address $hwaddr (LinkSetHardwareAddr) as well as ip link set $link vf $vf mac $hwaddr (LinkSetVfHardwareAddr)

ovs-cni/pkg/sriov/sriov.go

Lines 200 to 206 in c65a4e6

// if MAC address is provided, set it to the VF by using PF netlink

// which is accessible in the host namespace, not in the container namespace

if hwaddr != nil {

if err := netlink.LinkSetVfHardwareAddr(pfLink, vfIdx, hwaddr); err != nil {

return err

}

}

ykulazhenkov · 2024-08-05T08:49:56Z

pkg/sriov/sriov.go

-		_, err = renameLink(vfNetdevice, contIface.Name)
+	// in userspace driver mode, configure the smart VF netdevice via PF netlink
+	// otherwise, configure the smart VF netdevice directly in the container namespace
+	if !dpdkMode {


Does it make sense to create a separate functions for dpdk and non-dpdk scenarios and call these functions from the SetupSriovInterface?

Done. I create two functions, setupKernelSriovContIface and setupUserspaceSriovContIface.
Thanks to you, the code is now cleaner.

ovs-cni/pkg/sriov/sriov.go

Lines 292 to 302 in 074e965

if !userspaceMode {

// configure the smart VF netdevice directly in the container namespace

if err = setupKernelSriovContIface(contNetns, contIface, deviceID, ifName, mtu); err != nil {

return nil, nil, err

}

} else {

// configure the smart VF netdevice via PF netlink

if err = setupUserspaceSriovContIface(contNetns, contIface, pfLink, vfIdx, ifName); err != nil {

return nil, nil, err

}

}

ykulazhenkov · 2024-08-05T08:50:52Z

pkg/plugin/plugin.go

@@ -672,6 +700,11 @@ func CmdCheck(args *skel.CmdArgs) error {
 		return err
 	}

+	// TODO: CmdCheck for userspace driver


Do you plan to implement this check?

No, not in this PR. I will make a separate commit for CmdCheck later.

ykulazhenkov · 2024-08-05T09:14:11Z

pkg/plugin/plugin.go

 			}
 		}
+		// remove the VF representor port from the bridge
+		repIfName, err := sriov.GetNetRepresentor(cache.Netconf.DeviceID)


Do we need this? I think this https://github.com/kim-tae-kyung/ovs-cni/blob/4c01576a432fc30847f349413e52c704c4d7a449/pkg/plugin/plugin.go#L601 line should handle VF rep removal.

Done. Your review is correct. My code was a duplicate implementation, so I have removed that part.

ovs-cni/pkg/plugin/plugin.go

Lines 610 to 621 in 074e965

if sriov.IsOvsHardwareOffloadEnabled(cache.Netconf.DeviceID) {

// there is no network interface in case of userspace driver, so OrigIfName is empty

if !cache.UserspaceMode {

err = sriov.ReleaseVF(args, cache.OrigIfName)

if err != nil {

// try to reset vf into original state as much as possible in case of error

if err := sriov.ResetVF(args, cache.Netconf.DeviceID, cache.OrigIfName); err != nil {

log.Printf("Failed best-effort cleanup of VF %s: %v", cache.OrigIfName, err)

}

}

}

} else {

ykulazhenkov · 2024-08-05T10:01:41Z

pkg/types/types.go

@@ -40,6 +40,7 @@ type NetConf struct {
 	SocketFile             string   `json:"socket_file"`
 	LinkStateCheckRetries  int      `json:"link_state_check_retries"`
 	LinkStateCheckInterval int      `json:"link_state_check_interval"`
+	DPDKMode               bool     `json:"-,omitempty"`


It is not clear what you trying to achieve with `json:"-,omitempty"` syntax. If you want to hide the field from the API, then the correct syntax is just `json:"-"`. Currently DPDKMode renders as "-" in the cached json.

I see two options here:

use meaningful json filed name for DPDKMode in NetConf struct

move DPDKMode field to the CachedNetConf struct

The first option will pollute the user API. My personal preference is to use opt 2.

Done. I also prefer the second option.
Additionally, I changed DPDKMode to UserspaceMode, as the VF can be used not only for containerized DPDK but also for containerized QEMU, such as in KubeVirt.

ovs-cni/pkg/types/types.go

Lines 74 to 83 in 074e965

// CachedNetConf containing NetConfig, original smartnic vf interface name

// and kernel/userspace device driver mode of the smartnic vf interface

// (the last two are set only in case of ovs hareware offload scenario).

// this is intended to be used only for storing and retrieving config

// to/from a data store (example file cache).

type CachedNetConf struct {

Netconf *NetConf

OrigIfName string

UserspaceMode bool

}

kim-tae-kyung · 2024-08-10T00:12:09Z

@ykulazhenkov Thanks for your review!
I have reflected your comments. You made me realize that there were missing features and duplicated codes.

ykulazhenkov

@kim-tae-kyung thanks for addressing my comments.

I added one important comment and one nit.

ykulazhenkov · 2024-08-12T09:32:11Z

pkg/plugin/plugin.go

+		if err != nil {
+			return err
+		}
+	} else {


nit: if you remove the else branch the code will have the same behavior.
If you will decide to drop the else branch, you can also change line 295 to userspaceMode := false to be more explicitly

Done. I agree. The else branch was wired.

ovs-cni/pkg/plugin/plugin.go

Lines 294 to 301 in 05df2fe

// check if the device driver is the type of userspace driver

userspaceMode := false

if sriov.IsOvsHardwareOffloadEnabled(netconf.DeviceID) {

userspaceMode, err = sriov.HasUserspaceDriver(netconf.DeviceID)

if err != nil {

return err

}

}

ykulazhenkov · 2024-08-12T09:44:27Z

pkg/plugin/plugin.go

+		return err
+	}
+
+	if err := validateCache(cache, netconf); err != nil {


The cache validation may fail in case if "bridge name auto discovery" is used. Please, check the comment on the line 689. You need to move cache validation call to be after getBridgeName() is called.

Done. It was my mistake. Thanks for your comment.
I moved the IPAM check below the cache validation, instead of moving the cache validation up.

ovs-cni/pkg/plugin/plugin.go

Lines 690 to 698 in 05df2fe

// run the IPAM plugin

// userspace driver does not support IPAM plugin,

// because there is no network interface for the VF on the host

if netconf.NetConf.IPAM.Type != "" && !cache.UserspaceMode {

err = ipam.ExecCheck(netconf.NetConf.IPAM.Type, args.StdinData)

if err != nil {

return fmt.Errorf("failed to check with IPAM plugin type %q: %v", netconf.NetConf.IPAM.Type, err)

}

}

Add support for SR-IOV VFs using both hardware offload (switchdev) and userspace device driver such as vfio-pci. Signed-off-by: Taekyung Kim <kim.tae.kyung@sk.com>

If the MAC address is provided from args, update the MAC address of the VF to the provided MAC address via netlink. Signed-off-by: Taekyung Kim <kim.tae.kyung@sk.com>

ykulazhenkov

Thx, looks good to me

ykulazhenkov · 2024-08-12T11:48:50Z

@phoracek The PR looks good to me. Can you trigger the CI check?

phoracek · 2024-08-12T12:10:21Z

/test pull-e2e-ovs-cni

phoracek · 2024-08-12T13:44:41Z

Thanks a lot for the review @ykulazhenkov

@SchSeba do you also want to take a look before we merge this?

SchSeba

LGTM

nice work!

kubevirt-bot · 2024-08-15T11:50:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kim-tae-kyung, SchSeba, ykulazhenkov

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [SchSeba]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

phoracek · 2024-08-15T11:53:22Z

/lgtm

kim-tae-kyung · 2024-08-15T12:45:07Z

Thanks to All!

kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Jul 27, 2024

kubevirt-bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 27, 2024

ykulazhenkov reviewed Aug 5, 2024

View reviewed changes

kim-tae-kyung force-pushed the vfio-switchdev branch from 02ced28 to c65a4e6 Compare August 9, 2024 23:46

ykulazhenkov reviewed Aug 12, 2024

View reviewed changes

kim-tae-kyung added 2 commits August 12, 2024 20:39

Add support for userspace drivers with HW offload mode

05df2fe

Add support for SR-IOV VFs using both hardware offload (switchdev) and userspace device driver such as vfio-pci. Signed-off-by: Taekyung Kim <kim.tae.kyung@sk.com>

Update MAC address if provided in HW offload mode

5c039e4

If the MAC address is provided from args, update the MAC address of the VF to the provided MAC address via netlink. Signed-off-by: Taekyung Kim <kim.tae.kyung@sk.com>

kim-tae-kyung force-pushed the vfio-switchdev branch from c65a4e6 to 5c039e4 Compare August 12, 2024 11:40

ykulazhenkov approved these changes Aug 12, 2024

View reviewed changes

SchSeba approved these changes Aug 15, 2024

View reviewed changes

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 15, 2024

kubevirt-bot assigned phoracek Aug 15, 2024

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 15, 2024

kubevirt-bot merged commit d25ad9c into k8snetworkplumbingwg:main Aug 15, 2024
5 checks passed

phoracek mentioned this pull request Nov 13, 2024

Support for go uplift to 1.22.7 version and have fix for CVE-2024-24790 #331

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for userspace device drivers with HW offload mode #322

Add support for userspace device drivers with HW offload mode #322

kim-tae-kyung commented Jul 27, 2024

kubevirt-bot commented Jul 27, 2024

phoracek commented Jul 27, 2024

ykulazhenkov left a comment

ykulazhenkov Aug 5, 2024

kim-tae-kyung Aug 9, 2024

ykulazhenkov Aug 5, 2024

kim-tae-kyung Aug 9, 2024

ykulazhenkov Aug 5, 2024

kim-tae-kyung Aug 6, 2024

ykulazhenkov Aug 5, 2024

kim-tae-kyung Aug 10, 2024

ykulazhenkov Aug 5, 2024

kim-tae-kyung Aug 10, 2024

kim-tae-kyung commented Aug 10, 2024

ykulazhenkov left a comment

ykulazhenkov Aug 12, 2024

kim-tae-kyung Aug 12, 2024

ykulazhenkov Aug 12, 2024

kim-tae-kyung Aug 12, 2024

ykulazhenkov left a comment

ykulazhenkov commented Aug 12, 2024 •

edited

Loading

phoracek commented Aug 12, 2024

phoracek commented Aug 12, 2024

SchSeba left a comment

kubevirt-bot commented Aug 15, 2024

phoracek commented Aug 15, 2024

kim-tae-kyung commented Aug 15, 2024

	// if MAC address is provided, set it to the VF by using PF netlink
	// which is accessible in the host namespace, not in the container namespace
	if hwaddr != nil {
	if err := netlink.LinkSetVfHardwareAddr(pfLink, vfIdx, hwaddr); err != nil {
	return err
	}
	}

	if !userspaceMode {
	// configure the smart VF netdevice directly in the container namespace
	if err = setupKernelSriovContIface(contNetns, contIface, deviceID, ifName, mtu); err != nil {
	return nil, nil, err
	}
	} else {
	// configure the smart VF netdevice via PF netlink
	if err = setupUserspaceSriovContIface(contNetns, contIface, pfLink, vfIdx, ifName); err != nil {
	return nil, nil, err
	}
	}

	if sriov.IsOvsHardwareOffloadEnabled(cache.Netconf.DeviceID) {
	// there is no network interface in case of userspace driver, so OrigIfName is empty
	if !cache.UserspaceMode {
	err = sriov.ReleaseVF(args, cache.OrigIfName)
	if err != nil {
	// try to reset vf into original state as much as possible in case of error
	if err := sriov.ResetVF(args, cache.Netconf.DeviceID, cache.OrigIfName); err != nil {
	log.Printf("Failed best-effort cleanup of VF %s: %v", cache.OrigIfName, err)
	}
	}
	}
	} else {

	// CachedNetConf containing NetConfig, original smartnic vf interface name
	// and kernel/userspace device driver mode of the smartnic vf interface
	// (the last two are set only in case of ovs hareware offload scenario).
	// this is intended to be used only for storing and retrieving config
	// to/from a data store (example file cache).
	type CachedNetConf struct {
	Netconf *NetConf
	OrigIfName string
	UserspaceMode bool
	}

	// check if the device driver is the type of userspace driver
	userspaceMode := false
	if sriov.IsOvsHardwareOffloadEnabled(netconf.DeviceID) {
	userspaceMode, err = sriov.HasUserspaceDriver(netconf.DeviceID)
	if err != nil {
	return err
	}
	}

	// run the IPAM plugin
	// userspace driver does not support IPAM plugin,
	// because there is no network interface for the VF on the host
	if netconf.NetConf.IPAM.Type != "" && !cache.UserspaceMode {
	err = ipam.ExecCheck(netconf.NetConf.IPAM.Type, args.StdinData)
	if err != nil {
	return fmt.Errorf("failed to check with IPAM plugin type %q: %v", netconf.NetConf.IPAM.Type, err)
	}
	}

Add support for userspace device drivers with HW offload mode #322

Add support for userspace device drivers with HW offload mode #322

Conversation

kim-tae-kyung commented Jul 27, 2024

kubevirt-bot commented Jul 27, 2024

phoracek commented Jul 27, 2024

ykulazhenkov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kim-tae-kyung commented Aug 10, 2024

ykulazhenkov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ykulazhenkov left a comment

Choose a reason for hiding this comment

ykulazhenkov commented Aug 12, 2024 • edited Loading

phoracek commented Aug 12, 2024

phoracek commented Aug 12, 2024

SchSeba left a comment

Choose a reason for hiding this comment

kubevirt-bot commented Aug 15, 2024

phoracek commented Aug 15, 2024

kim-tae-kyung commented Aug 15, 2024

ykulazhenkov commented Aug 12, 2024 •

edited

Loading