On Sat, Oct 21, 2017 at 9:59 AM, Yuval Mintz <yuv...@mellanox.com> wrote: >> On Fri, Oct 20, 2017 at 10:10 AM, Jiri Pirko <j...@resnulli.us> wrote: >> > Fri, Oct 20, 2017 at 04:03:55PM CEST, steven.l...@broadcom.com wrote: >> >>On Thu, Oct 19, 2017 at 5:39 PM, Jiri Pirko <j...@resnulli.us> wrote: >> >>> Thu, Oct 19, 2017 at 10:32:21PM CEST, yuv...@mellanox.com wrote: >> >>>>> Adding DEVLINK_PERM_CONFIG_MSIX_VECTORS_PER_VF >> permanent >> >>>>> config >> >>>>> parameter. Defines number of MSI-X vectors allocated per VF. >> >>>>> Value is permanent (stored in NVRAM), so becomes the new default >> >>>>> value for this device. >> >>>> >> >>>>Sounds like you're having this enforce the same configuration for all >> child VFs. >> >>> >> >>> Yeah, this sounds like per-port config. >> >>> >> >> >> >>Well, it gets a little tricky here. I assume some cards handle this >> >>per-port. Other cards might handle this per PF, where PF may not >> >>always correspond 1:1 with a port. And some cards maybe just allow a >> >>single value for this parameter for the entire card, covering all >> >>ports/PFs. >> >> >> >>To keep things simple and as general as possible, it made sense to set >> >>all parameters on a per-PCI device level. As I mentioned in my >> >>cover-letter, the devices most likely to use these proposed commands >> >>do not have a single "whole asic" PCI b/d/f with internal mechanism >> >>for accessing ports - most expose each port (and each function on each >> >>port) as a separate PCI b/d/f, with no separate "whole asic" PCI >> >>b/d/f. That's how the BCM cards work, and I think that's how the MLNX >> >>cards work, and others that would be likely to use these cmds. >> >> >> >>So, to summarize, you direct the command to the PCI b/d/f you want to >> >>target. Does this make sense? >> > >> > So you plan to have 1 devlink instance for each vf? Not sure that >> > does sound right to me :/ >> > >> >> For the commands proposed in this patchset, AFAIK they all apply on a >> per-PF or broader, i.e. per-port or whole-card, granularity, since >> they affect permanent config that applies at boot-up. So, no, the VFs >> don't really come into play here. > > Regardless of whether you're planning on having VFs as devlink instances, > the actual attribute question remains - > you're proposing an attribute that forces all VFs to have the same value. > This probably suits your PCI core limitations but other vendors might have > a different capability set, and accepting this design limitation now would > muck all future extension attempts of such attributes. > > I think VF configurations should be planned in advance for supporting a > per-VF Configuration whenever it's possible - even if not required [/possible] > by the one pushing the new attribute. >
The commands being added in this patch are for permanent (i.e. NVRAM) config - essentially setting the new default values for various features of the device at boot-up. At that initialization time, no VFs are yet instantiated. So my perspective was, in general (not just for our specific device / design), it doesn't seem like permanent config parameters would be set on individual VFs. That was what my previous comment was trying to convey. If that assumption is wrong, though, and there is some device that has NVRAM config that is set per-VF, I assume the user would instantiate the VF and then call the devlink API on the pci device corresponding to the VF they with to affect, and I think the model proposed still works. Are you suggesting adding a mechanism to set NVRAM parameters on a per-VF basis, without instantiating the VF first? I would prefer not adding such a mechanism unless/until there's a use case for it.
