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

[Michal Swiatkowski <michal.swiatkowski@linux.intel.com>]

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.

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