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

[Jiri Pirko <jiri@resnulli.us>]

Mon, Aug 12, 2024 at 06:24:12AM CEST, michal.swiatkowski@linux.intel.com wrote:
>On Sat, Aug 10, 2024 at 08:50:38AM +0200, Jiri Pirko wrote:
>> Fri, Aug 09, 2024 at 01:47:38PM CEST, michal.swiatkowski@linux.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@intel.com wrote:
>> >> >From: Piotr Raczynski <piotr.raczynski@intel.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?
>
>I didn't know about addr parameter. I will change it to always have the
>same number so.

Please make sure this is fixed in other ice places:

$ git grep "bus.func" drivers/net/ethernet/intel/ice/
drivers/net/ethernet/intel/ice/devlink/devlink_port.c:  attrs.phys.port_number = pf->hw.bus.func;
drivers/net/ethernet/intel/ice/devlink/devlink_port.c:  attrs.pci_vf.pf = pf->hw.bus.func;
drivers/net/ethernet/intel/ice/ice_debugfs.c:   if (pf->hw.bus.func)
drivers/net/ethernet/intel/ice/ice_fwlog.c:     if (hw->bus.func)
drivers/net/ethernet/intel/ice/ice_fwlog.c:     if (hw->bus.func)
drivers/net/ethernet/intel/ice/ice_main.c:      hw->bus.func = PCI_FUNC(pdev->devfn);

Calls for a -net/stable fix IMO.

Also, check other intel drivers as well. I see this is a pattern.

Thanks!


>
>> 
>> 
>> 
>> >
>> >> 
>> >> 
>> >> >+		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);
>> >> >+}
>> >> >+
>> >> 
>> >> [...]