From 6d32ec0745d31821eddfcf77a2a314ddb146c0e8 Mon Sep 17 00:00:00 2001 From: Andrea Panattoni Date: Fri, 25 Oct 2024 09:14:08 +0200 Subject: [PATCH] kernel: Set arguments based on CPU architecture Kernel arguments like `intel_iommu=on` does not have sense on AMD or ARM systems and some user might complain about their presence, though they are likely to be harmless. Also, on ARM systems the `iommu.passthrough` parameter is the one to use [1]. Improve `GHWLib` to bridge CPU information from the library. Add `CpuInfoProviderInterface` and inject it into the GenericPlugin to implement the per CPU vendor logic. [1] https://github.com/torvalds/linux/blob/master/Documentation/admin-guide/kernel-parameters.txt#L2343 Signed-off-by: Andrea Panattoni --- pkg/consts/constants.go | 7 ++-- pkg/helper/mock/mock_helper.go | 15 +++++++ pkg/host/internal/cpu/cpu.go | 40 ++++++++++++++++++ pkg/host/internal/lib/ghw/ghw.go | 8 ++++ pkg/host/internal/lib/ghw/mock/mock_ghw.go | 16 ++++++++ pkg/host/manager.go | 5 +++ pkg/host/mock/mock_host.go | 15 +++++++ pkg/host/types/interfaces.go | 13 ++++++ pkg/plugins/generic/generic_plugin.go | 27 ++++++++++++- pkg/plugins/generic/generic_plugin_test.go | 47 +++++++++++++++++----- 10 files changed, 178 insertions(+), 15 deletions(-) create mode 100644 pkg/host/internal/cpu/cpu.go diff --git a/pkg/consts/constants.go b/pkg/consts/constants.go index f3c076111..f7025c90d 100644 --- a/pkg/consts/constants.go +++ b/pkg/consts/constants.go @@ -121,9 +121,10 @@ const ( `IMPORT{program}="/etc/udev/switchdev-vf-link-name.sh $attr{phys_port_name}", ` + `NAME="%s_$env{NUMBER}"` - KernelArgPciRealloc = "pci=realloc" - KernelArgIntelIommu = "intel_iommu=on" - KernelArgIommuPt = "iommu=pt" + KernelArgPciRealloc = "pci=realloc" + KernelArgIntelIommu = "intel_iommu=on" + KernelArgIommuPt = "iommu=pt" + KernelArgIommuPassthrough = "iommu.passthrough=1" // Feature gates // ParallelNicConfigFeatureGate: allow to configure nics in parallel diff --git a/pkg/helper/mock/mock_helper.go b/pkg/helper/mock/mock_helper.go index cfca2a768..432d741be 100644 --- a/pkg/helper/mock/mock_helper.go +++ b/pkg/helper/mock/mock_helper.go @@ -351,6 +351,21 @@ func (mr *MockHostHelpersInterfaceMockRecorder) EnableService(service interface{ return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "EnableService", reflect.TypeOf((*MockHostHelpersInterface)(nil).EnableService), service) } +// GetCPUVendor mocks base method. +func (m *MockHostHelpersInterface) GetCPUVendor() (types.CPUVendor, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetCPUVendor") + ret0, _ := ret[0].(types.CPUVendor) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetCPUVendor indicates an expected call of GetCPUVendor. +func (mr *MockHostHelpersInterfaceMockRecorder) GetCPUVendor() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetCPUVendor", reflect.TypeOf((*MockHostHelpersInterface)(nil).GetCPUVendor)) +} + // GetCheckPointNodeState mocks base method. func (m *MockHostHelpersInterface) GetCheckPointNodeState() (*v1.SriovNetworkNodeState, error) { m.ctrl.T.Helper() diff --git a/pkg/host/internal/cpu/cpu.go b/pkg/host/internal/cpu/cpu.go new file mode 100644 index 000000000..fd02157e6 --- /dev/null +++ b/pkg/host/internal/cpu/cpu.go @@ -0,0 +1,40 @@ +package cpu + +import ( + "fmt" + + ghwPkg "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/host/internal/lib/ghw" + "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/host/types" +) + +type cpuInfoProvider struct { + ghwLib ghwPkg.GHWLib +} + +func New(ghwLib ghwPkg.GHWLib) *cpuInfoProvider { + return &cpuInfoProvider{ + ghwLib: ghwLib, + } +} + +func (c *cpuInfoProvider) GetCPUVendor() (types.CPUVendor, error) { + cpuInfo, err := c.ghwLib.CPU() + if err != nil { + return -1, fmt.Errorf("can't retrieve the CPU vendor: %w", err) + } + + if len(cpuInfo.Processors) == 0 { + return -1, fmt.Errorf("wrong CPU information retrieved: %v", cpuInfo) + } + + switch cpuInfo.Processors[0].Vendor { + case "GenuineIntel": + return types.CPUVendorIntel, nil + case "AuthenticAMD": + return types.CPUVendorAMD, nil + case "ARM": + return types.CPUVendorARM, nil + } + + return -1, fmt.Errorf("unknown CPU vendor: %s", cpuInfo.Processors[0].Vendor) +} diff --git a/pkg/host/internal/lib/ghw/ghw.go b/pkg/host/internal/lib/ghw/ghw.go index 6a6829604..d518977e4 100644 --- a/pkg/host/internal/lib/ghw/ghw.go +++ b/pkg/host/internal/lib/ghw/ghw.go @@ -2,6 +2,7 @@ package ghw import ( "github.com/jaypipes/ghw" + "github.com/jaypipes/ghw/pkg/cpu" ) func New() GHWLib { @@ -12,6 +13,9 @@ func New() GHWLib { type GHWLib interface { // PCI returns a pointer to an Info that provide methods to access info about devices PCI() (Info, error) + + // CPU returns a pointer to an Info that provide methods to access info about devices + CPU() (*cpu.Info, error) } // Info interface provide methods to access info about devices @@ -27,3 +31,7 @@ type libWrapper struct{} func (w *libWrapper) PCI() (Info, error) { return ghw.PCI() } + +func (w *libWrapper) CPU() (*cpu.Info, error) { + return ghw.CPU() +} diff --git a/pkg/host/internal/lib/ghw/mock/mock_ghw.go b/pkg/host/internal/lib/ghw/mock/mock_ghw.go index 2e2b4b5c5..9d6092362 100644 --- a/pkg/host/internal/lib/ghw/mock/mock_ghw.go +++ b/pkg/host/internal/lib/ghw/mock/mock_ghw.go @@ -9,6 +9,7 @@ import ( gomock "github.com/golang/mock/gomock" ghw "github.com/jaypipes/ghw" + cpu "github.com/jaypipes/ghw/pkg/cpu" ghw0 "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/host/internal/lib/ghw" ) @@ -35,6 +36,21 @@ func (m *MockGHWLib) EXPECT() *MockGHWLibMockRecorder { return m.recorder } +// CPU mocks base method. +func (m *MockGHWLib) CPU() (*cpu.Info, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "CPU") + ret0, _ := ret[0].(*cpu.Info) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// CPU indicates an expected call of CPU. +func (mr *MockGHWLibMockRecorder) CPU() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CPU", reflect.TypeOf((*MockGHWLib)(nil).CPU)) +} + // PCI mocks base method. func (m *MockGHWLib) PCI() (ghw0.Info, error) { m.ctrl.T.Helper() diff --git a/pkg/host/manager.go b/pkg/host/manager.go index 02a77a659..44bd45807 100644 --- a/pkg/host/manager.go +++ b/pkg/host/manager.go @@ -2,6 +2,7 @@ package host import ( "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/host/internal/bridge" + "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/host/internal/cpu" "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/host/internal/infiniband" "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/host/internal/kernel" "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/host/internal/lib/dputils" @@ -30,6 +31,7 @@ type HostManagerInterface interface { types.VdpaInterface types.InfinibandInterface types.BridgeInterface + types.CPUInfoProviderInterface } type hostManager struct { @@ -42,6 +44,7 @@ type hostManager struct { types.VdpaInterface types.InfinibandInterface types.BridgeInterface + types.CPUInfoProviderInterface } func NewHostManager(utilsInterface utils.CmdInterface) (HostManagerInterface, error) { @@ -61,6 +64,7 @@ func NewHostManager(utilsInterface utils.CmdInterface) (HostManagerInterface, er } br := bridge.New() sr := sriov.New(utilsInterface, k, n, u, v, ib, netlinkLib, dpUtils, sriovnetLib, ghwLib, br) + cpuInfoProvider := cpu.New(ghwLib) return &hostManager{ utilsInterface, k, @@ -71,5 +75,6 @@ func NewHostManager(utilsInterface utils.CmdInterface) (HostManagerInterface, er v, ib, br, + cpuInfoProvider, }, nil } diff --git a/pkg/host/mock/mock_host.go b/pkg/host/mock/mock_host.go index cb4d1480a..5ebed46aa 100644 --- a/pkg/host/mock/mock_host.go +++ b/pkg/host/mock/mock_host.go @@ -321,6 +321,21 @@ func (mr *MockHostManagerInterfaceMockRecorder) EnableService(service interface{ return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "EnableService", reflect.TypeOf((*MockHostManagerInterface)(nil).EnableService), service) } +// GetCPUVendor mocks base method. +func (m *MockHostManagerInterface) GetCPUVendor() (types.CPUVendor, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetCPUVendor") + ret0, _ := ret[0].(types.CPUVendor) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetCPUVendor indicates an expected call of GetCPUVendor. +func (mr *MockHostManagerInterfaceMockRecorder) GetCPUVendor() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetCPUVendor", reflect.TypeOf((*MockHostManagerInterface)(nil).GetCPUVendor)) +} + // GetCurrentKernelArgs mocks base method. func (m *MockHostManagerInterface) GetCurrentKernelArgs() (string, error) { m.ctrl.T.Helper() diff --git a/pkg/host/types/interfaces.go b/pkg/host/types/interfaces.go index 5918dca34..c6e0c8faf 100644 --- a/pkg/host/types/interfaces.go +++ b/pkg/host/types/interfaces.go @@ -187,3 +187,16 @@ type InfinibandInterface interface { // ConfigureVfGUID configures and sets a GUID for an IB VF device ConfigureVfGUID(vfAddr string, pfAddr string, vfID int, pfLink netlink.Link) error } + +type CPUVendor int + +const ( + CPUVendorIntel CPUVendor = iota + CPUVendorAMD + CPUVendorARM +) + +type CPUInfoProviderInterface interface { + // Retrieve the CPU vendor of the current system + GetCPUVendor() (CPUVendor, error) +} diff --git a/pkg/plugins/generic/generic_plugin.go b/pkg/plugins/generic/generic_plugin.go index 14b1903e5..552f8142a 100644 --- a/pkg/plugins/generic/generic_plugin.go +++ b/pkg/plugins/generic/generic_plugin.go @@ -13,6 +13,7 @@ import ( sriovnetworkv1 "github.com/k8snetworkplumbingwg/sriov-network-operator/api/v1" "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/consts" "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/helper" + hostTypes "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/host/types" plugin "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/plugins" "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/utils" "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/vars" @@ -419,9 +420,31 @@ func (p *GenericPlugin) shouldConfigureBridges() bool { func (p *GenericPlugin) addVfioDesiredKernelArg(state *sriovnetworkv1.SriovNetworkNodeState) { driverState := p.DriverStateMap[Vfio] + + kernelArgFnByCPUVendor := map[hostTypes.CPUVendor]func(){ + hostTypes.CPUVendorIntel: func() { + p.addToDesiredKernelArgs(consts.KernelArgIntelIommu) + p.addToDesiredKernelArgs(consts.KernelArgIommuPt) + }, + hostTypes.CPUVendorAMD: func() { + p.addToDesiredKernelArgs(consts.KernelArgIommuPt) + }, + hostTypes.CPUVendorARM: func() { + p.addToDesiredKernelArgs(consts.KernelArgIommuPassthrough) + }, + } + if !driverState.DriverLoaded && driverState.NeedDriverFunc(state, driverState) { - p.addToDesiredKernelArgs(consts.KernelArgIntelIommu) - p.addToDesiredKernelArgs(consts.KernelArgIommuPt) + cpuVendor, err := p.helpers.GetCPUVendor() + if err != nil { + log.Log.Error(err, "can't get CPU vendor, falling back to Intel") + cpuVendor = hostTypes.CPUVendorIntel + } + + addKernelArgFn := kernelArgFnByCPUVendor[cpuVendor] + if addKernelArgFn != nil { + addKernelArgFn() + } } } diff --git a/pkg/plugins/generic/generic_plugin_test.go b/pkg/plugins/generic/generic_plugin_test.go index 0d6701a64..0a6674712 100644 --- a/pkg/plugins/generic/generic_plugin_test.go +++ b/pkg/plugins/generic/generic_plugin_test.go @@ -10,6 +10,7 @@ import ( sriovnetworkv1 "github.com/k8snetworkplumbingwg/sriov-network-operator/api/v1" "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/consts" mock_helper "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/helper/mock" + hostTypes "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/host/types" plugin "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/plugins" "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/vars" ) @@ -850,8 +851,9 @@ var _ = Describe("Generic plugin", func() { Expect(changed).To(BeTrue()) }) - It("should detect changes on status due to missing kernel args", func() { - networkNodeState := &sriovnetworkv1.SriovNetworkNodeState{ + Context("Kernel Args", func() { + + vfioNetworkNodeState := &sriovnetworkv1.SriovNetworkNodeState{ Spec: sriovnetworkv1.SriovNetworkNodeStateSpec{ Interfaces: sriovnetworkv1.Interfaces{{ PciAddress: "0000:00:00.0", @@ -896,16 +898,41 @@ var _ = Describe("Generic plugin", func() { }, } - // Load required kernel args. - genericPlugin.(*GenericPlugin).addVfioDesiredKernelArg(networkNodeState) + It("should detect changes on status due to missing kernel args", func() { + hostHelper.EXPECT().GetCPUVendor().Return(hostTypes.CPUVendorIntel, nil) - hostHelper.EXPECT().GetCurrentKernelArgs().Return("", nil) - hostHelper.EXPECT().IsKernelArgsSet("", consts.KernelArgIntelIommu).Return(false) - hostHelper.EXPECT().IsKernelArgsSet("", consts.KernelArgIommuPt).Return(false) + // Load required kernel args. + genericPlugin.(*GenericPlugin).addVfioDesiredKernelArg(vfioNetworkNodeState) - changed, err := genericPlugin.CheckStatusChanges(networkNodeState) - Expect(err).ToNot(HaveOccurred()) - Expect(changed).To(BeTrue()) + Expect(genericPlugin.(*GenericPlugin).DesiredKernelArgs).To(Equal(map[string]bool{ + consts.KernelArgIntelIommu: false, + consts.KernelArgIommuPt: false, + })) + + hostHelper.EXPECT().GetCurrentKernelArgs().Return("", nil) + hostHelper.EXPECT().IsKernelArgsSet("", consts.KernelArgIntelIommu).Return(false) + hostHelper.EXPECT().IsKernelArgsSet("", consts.KernelArgIommuPt).Return(false) + + changed, err := genericPlugin.CheckStatusChanges(vfioNetworkNodeState) + Expect(err).ToNot(HaveOccurred()) + Expect(changed).To(BeTrue()) + }) + + It("should set the correct kernel args on AMD CPUs", func() { + hostHelper.EXPECT().GetCPUVendor().Return(hostTypes.CPUVendorAMD, nil) + genericPlugin.(*GenericPlugin).addVfioDesiredKernelArg(vfioNetworkNodeState) + Expect(genericPlugin.(*GenericPlugin).DesiredKernelArgs).To(Equal(map[string]bool{ + consts.KernelArgIommuPt: false, + })) + }) + + It("should set the correct kernel args on ARM CPUs", func() { + hostHelper.EXPECT().GetCPUVendor().Return(hostTypes.CPUVendorARM, nil) + genericPlugin.(*GenericPlugin).addVfioDesiredKernelArg(vfioNetworkNodeState) + Expect(genericPlugin.(*GenericPlugin).DesiredKernelArgs).To(Equal(map[string]bool{ + consts.KernelArgIommuPassthrough: false, + })) + }) }) It("should load vfio_pci driver", func() {