On Wed, Jan 13, 2021 at 11:16 PM Leon Romanovsky <leon@xxxxxxxxxx> wrote: > > On Wed, Jan 13, 2021 at 12:00:00PM -0800, Alexander Duyck wrote: > > On Tue, Jan 12, 2021 at 10:09 PM Leon Romanovsky <leon@xxxxxxxxxx> wrote: > > > > > > On Tue, Jan 12, 2021 at 01:59:51PM -0800, Alexander Duyck wrote: > > > > On Mon, Jan 11, 2021 at 10:39 PM Leon Romanovsky <leon@xxxxxxxxxx> wrote: > > > > > > > > > > On Mon, Jan 11, 2021 at 11:30:33AM -0800, Alexander Duyck wrote: > > > > > > On Sun, Jan 10, 2021 at 7:12 AM Leon Romanovsky <leon@xxxxxxxxxx> wrote: > > > > > > > > > > > > > > From: Leon Romanovsky <leonro@xxxxxxxxxx> > > > > > > > <snip> > > > > > > > > > > Also I am not a big fan of the VF groping around looking for a PF > > > > interface as it means the interface will likely be exposed in the > > > > guest as well, but it just won't work. > > > > > > If you are referring to VF exposed to the VM, so in this case VF must be > > > bound too vfio driver, or any other driver, and won't allow MSI-X change. > > > If you are referring to PF exposed to the VM, it is very unlikely scenario > > > in real world and reserved for braves among us. Even in this case, the > > > set MSI-X won't work, because PF will be connected to the hypervisor driver > > > that doesn't support set_msix. > > > > > > So both cases are handled. > > > > I get that they are handled. However I am not a huge fan of the sysfs > > attributes for one device being dependent on another device. When you > > have to start searching for another device it just makes things messy. > > This is pretty common way, nothing new here. This is how writable fields within the device are handled. I am pretty sure this is the first sysfs entry that is providing a workaround via a device firmware to make the field editable that wasn't intended to be. So if in the future I define a device that has an MMIO register that allows me to edit configuration space should I just tie it into the same framework? That is kind of where I am going with my objection to this. It just seems like you are adding a backdoor to allow editing read-only configuration options. > > > > > > > > > > > > > > > > > > If you are calling this on the VFs then it doesn't really make any > > > > > > sense anyway since the VF is not a "VF PCI dev representor" and > > > > > > shouldn't be treated as such. In my opinion if we are going to be > > > > > > doing per-port resource limiting that is something that might make > > > > > > more sense as a part of the devlink configuration for the VF since the > > > > > > actual change won't be visible to an assigned device. > > > > > > > > > > https://lore.kernel.org/linux-pci/20210112061535.GB4678@unreal/ > > > > > > > > So the question I would have is if we are spawning the VFs and > > > > expecting them to have different configs or the same configuration? > > > > > > By default, they have same configuration. > > > > > > > I'm assuming in your case you are looking for a different > > > > configuration per port. Do I have that correct? > > > > > > No, per-VF as represents one device in the PCI world. For example, mlx5 > > > can have more than one physical port. > > > > Sorry, I meant per virtual function, not per port. > > Yes, PCI spec is clear, MSI-X vector count is per-device and in our case > it means per-VF. I think you overlooked the part about it being "read-only". It isn't really meant to be changed and that is what this patch set is providing. > > > > > > > > > > Where this gets ugly is that SR-IOV assumes a certain uniformity per > > > > VF so doing a per-VF custom limitation gets ugly pretty quick. > > > > > > I don't find any support for this "uniformity" claim in the PCI spec. > > > > I am referring to the PCI configuration space. Each VF ends up with > > some fixed amount of MMIO resources per function. So typically when > > you spawn VFs we had things setup so that all you do is say how many > > you want. > > > > > > I wonder if it would make more sense if we are going this route to just > > > > define a device-tree like schema that could be fed in to enable VFs > > > > instead of just using echo X > sriov_numvfs and then trying to fix > > > > things afterwards. Then you could define this and other features that > > > > I am sure you would need in the future via json-schema like is done in > > > > device-tree and write it once enabling the set of VFs that you need. > > > > > > Sorry, but this is overkill, it won't give us much and it doesn't fit > > > the VF usage model at all. > > > > > > Right now, all heavy users of SR-IOV are creating many VFs up to the maximum. > > > They do it with autoprobe disabled, because it is too time consuming to wait > > > till all VFs probe themselves and unbind them later. > > > > > > After that, they wait for incoming request to provision VM on VF, they set MAC > > > address, change MSI-X according to VM properties and bind that VF to new VM. > > > > > > So MSI-X change is done after VFs were created. > > > > So if I understand correctly based on your comments below you are > > dynamically changing the VF's MSI-X configuration space then? > > I'm changing "Table Size" from "7.7.2.2 Message Control Register for > MSI-X (Offset 02h)" and nothing more. > > If you do raw PCI read before and after, only this field will be changed. I would hope there is much more going on. Otherwise the VF hardware will be exploitable by a malicious driver in the guest since you could read/write to registers beyond the table and see some result. I am assuming the firmware doesn't allow triggering of any interrupts beyond the ones defined as being in the table.
