netdev - Re: [PATCH net-next v3 03/15] ice: add basic devlink subfunctions support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZrcNvu6cXqQ-ybZu@nanopsycho.orion>
Date: Sat, 10 Aug 2024 08:50:38 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>
Cc: Tony Nguyen <anthony.l.nguyen@...el.com>, davem@...emloft.net,
	kuba@...nel.org, pabeni@...hat.com, edumazet@...gle.com,
	netdev@...r.kernel.org, Piotr Raczynski <piotr.raczynski@...el.com>,
	jiri@...dia.com, shayd@...dia.com, wojciech.drewek@...el.com,
	horms@...nel.org, sridhar.samudrala@...el.com,
	mateusz.polchlopek@...el.com, kalesh-anakkur.purayil@...adcom.com,
	michal.kubiak@...el.com, pio.raczynski@...il.com,
	przemyslaw.kitszel@...el.com, jacob.e.keller@...el.com,
	maciej.fijalkowski@...el.com,
	Rafal Romanowski <rafal.romanowski@...el.com>
Subject: Re: [PATCH net-next v3 03/15] ice: add basic devlink subfunctions
 support

Fri, Aug 09, 2024 at 01:47:38PM CEST, michal.swiatkowski@...ux.intel.com wrote:
>On Fri, Aug 09, 2024 at 01:16:28PM +0200, Jiri Pirko wrote:
>> Thu, Aug 08, 2024 at 07:30:49PM CEST, anthony.l.nguyen@...el.com wrote:
>> >From: Piotr Raczynski <piotr.raczynski@...el.com>
>> >
>> 
>> [...]
>> 
>> >+static int
>> >+ice_devlink_port_new_check_attr(struct ice_pf *pf,
>> >+				const struct devlink_port_new_attrs *new_attr,
>> >+				struct netlink_ext_ack *extack)
>> >+{
>> >+	if (new_attr->flavour != DEVLINK_PORT_FLAVOUR_PCI_SF) {
>> >+		NL_SET_ERR_MSG_MOD(extack, "Flavour other than pcisf is not supported");
>> >+		return -EOPNOTSUPP;
>> >+	}
>> >+
>> >+	if (new_attr->controller_valid) {
>> >+		NL_SET_ERR_MSG_MOD(extack, "Setting controller is not supported");
>> >+		return -EOPNOTSUPP;
>> >+	}
>> >+
>> >+	if (new_attr->port_index_valid) {
>> >+		NL_SET_ERR_MSG_MOD(extack, "Port index is invalid");
>> 
>> Nope, it is actually valid, but your driver does not support that.
>> Please fix the message.
>>
>
>Ok
>
>> 
>> >+		return -EOPNOTSUPP;
>> >+	}
>> >+
>> >+	if (new_attr->pfnum != pf->hw.bus.func) {
>> 
>> hw.bus.func, hmm. How about if you pass-through the PF to VM, can't the
>> func be anything? Will this still make sense? I don't think so. If you
>> get the PF number like this in the rest of the driver, you need to fix
>> this.
>>
>
>I can change it to our internal value. I wonder if it will be better. If
>I understand correctly, now:
>PF0 - xx.xx.0; PF1 - xx.xx.1
>
>I am doing pass-through PF1
>PF0 (on host) - xx.xx.0
>
>PF1 (on VM) - xx.xx.0 (there is one PF on VM, so for me it is more
>intuitive to have func 0)
>
>after I change:
>PF0 - xx.xx.0; PF1 - xx.xx.1
>
>pass-through PF1
>PF0 (on host) - xx.xx.0
>
>PF1 (on VM) - xx.xx.0, but user will have to use 1 as pf num
>
>Correct me if I am wrong.

Did you try this? I mean, you can check that easily with vng:
vng --qemu-opts="-device vfio-pci,host=5e:01.0,addr=01.04"

Then in the VM you see:
0000:00:01.4

Then, by your code, the pf number is 4, isn't it?

What am I missing?



>
>> 
>> 
>> >+		NL_SET_ERR_MSG_MOD(extack, "Incorrect pfnum supplied");
>> >+		return -EINVAL;
>> >+	}
>> >+
>> >+	if (!pci_msix_can_alloc_dyn(pf->pdev)) {
>> >+		NL_SET_ERR_MSG_MOD(extack, "Dynamic MSIX-X interrupt allocation is not supported");
>> >+		return -EOPNOTSUPP;
>> >+	}
>> >+
>> >+	return 0;
>> >+}
>> 
>> [...]
>> 
>> 
>> >+int ice_devlink_create_sf_port(struct ice_dynamic_port *dyn_port)
>> >+{
>> >+	struct devlink_port_attrs attrs = {};
>> >+	struct devlink_port *devlink_port;
>> >+	struct devlink *devlink;
>> >+	struct ice_vsi *vsi;
>> >+	struct ice_pf *pf;
>> >+
>> >+	vsi = dyn_port->vsi;
>> >+	pf = dyn_port->pf;
>> >+
>> >+	devlink_port = &dyn_port->devlink_port;
>> >+
>> >+	attrs.flavour = DEVLINK_PORT_FLAVOUR_PCI_SF;
>> >+	attrs.pci_sf.pf = pf->hw.bus.func;
>> 
>> Same here.
>> 
>> 
>> >+	attrs.pci_sf.sf = dyn_port->sfnum;
>> >+
>> >+	devlink_port_attrs_set(devlink_port, &attrs);
>> >+	devlink = priv_to_devlink(pf);
>> >+
>> >+	return devl_port_register_with_ops(devlink, devlink_port, vsi->idx,
>> >+					   &ice_devlink_port_sf_ops);
>> >+}
>> >+
>> 
>> [...]