Re: [PATCH] virtio_pci: support enabling VFs

[Alexander Duyck]

On Wed, May 30, 2018 at 8:20 PM, Tiwei Bie <tiwei.bie@xxxxxxxxx> wrote:
> On Thu, May 31, 2018 at 01:11:37AM +0800, Rustad, Mark D wrote:
>> On May 30, 2018, at 9:54 AM, Duyck, Alexander H
>> <alexander.h.duyck@xxxxxxxxx> wrote:
>>
>> > On Wed, 2018-05-30 at 09:44 -0700, Rustad, Mark D wrote:
>> > > On May 30, 2018, at 9:22 AM, Michael S. Tsirkin <mst@xxxxxxxxxx> wrote:
>> > >
>> > > > > +static int virtio_pci_sriov_configure(struct pci_dev *pci_dev, int
>> > > > > num_vfs)
>> > > > > +{
>> > > > > +     struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
>> > > > > +     struct virtio_device *vdev = &vp_dev->vdev;
>> > > > > +     int (*sriov_configure)(struct pci_dev *pci_dev, int num_vfs);
>> > > > > +
>> > > > > +     if (!(vdev->config->get_status(vdev) & VIRTIO_CONFIG_S_DRIVER_OK))
>> > > > > +             return -EBUSY;
>> > > > > +
>> > > > > +     if (!__virtio_test_bit(vdev, VIRTIO_F_SR_IOV))
>> > > > > +             return -EINVAL;
>> > > > > +
>> > > > > +     sriov_configure = pci_sriov_configure_simple;
>> > > > > +     if (sriov_configure == NULL)
>> > > > > +             return -ENOENT;
>> > > >
>> > > > BTW what is all this trickery in aid of?
>> > >
>> > > When SR-IOV support is not compiled into the kernel,
>> > > pci_sriov_configure_simple is #defined as NULL. This allows it to compile
>> > > in that case, even though there is utterly no way for it to be called in
>> > > that case. It is an alternative to #ifs in the code.
>> >
>> > Why even have the call though? I would wrap all of this in an #ifdef
>> > and strip it out since you couldn't support SR-IOV if it isn't present
>> > in the kernel anyway.
>>
>> I am inclined to agree. In this case, the presence of #ifdefs I think would
>> be clearer. As written, someone will want to get rid of the pointer only to
>> create a build problem when SR-IOV is not configured.
>
> In my opinion, maybe it would be better to make
> pci_sriov_configure_simple() always available
> just like other sriov functions.
>
> Based on the comments in the original patch:
>
> https://patchwork.kernel.org/patch/10353197/
> """
> +/* this is expected to be used as a function pointer, just define as NULL */
> +#define pci_sriov_configure_simple NULL
> """
>
> This function could be defined as NULL just because
> it was expected to be used as a function pointer.
> But actually it could be called directly as a
> function, just like this case.
>
> So I prefer to make this function always available
> just like other sriov functions.
>
> Best regards,
> Tiwei Bie

The fact that you are having to add additional code kind of implies
that maybe this doesn't fall into the pci_sriov_configure_simple case
anymore. The PF itself is defining what the VF can and can't do via
the feature flags you are testing for.

For example how is the bit of code below valid if the kernel itself
doesn't support SR-IOV:
+static void vp_transport_features(struct virtio_device *vdev, u64 features)
+{
+       struct virtio_pci_device *vp_dev = to_vp_device(vdev);
+       struct pci_dev *pci_dev = vp_dev->pci_dev;
+
+       if ((features & BIT_ULL(VIRTIO_F_SR_IOV)) &&
+                       pci_find_ext_capability(pci_dev, PCI_EXT_CAP_ID_SRIOV))
+               __virtio_set_bit(vdev, VIRTIO_F_SR_IOV);
+}
+

It really seems like we should be wrapping these functions at the very
minimum so that they don't imply you have SR-IOV support when it isn't
supported in the kernel.

Also it seems like we should be disabling the VFs if the driver is
unbound from this interface. We need to add logic to disable SR-IOV if
the driver is removed. What we don't want to do is leave VFs allocated
and then have the potential for us to unbind/rebind the driver as the
new driver may change the negotiated features.

- Alex