On Wed, 6 Nov 2024 12:21:03 +0200
Yishai Hadas <yishaih@xxxxxxxxxx> wrote:
> On 06/11/2024 0:47, Alex Williamson wrote:
> > On Mon, 4 Nov 2024 12:21:29 +0200
> > Yishai Hadas <yishaih@xxxxxxxxxx> wrote:
> >> diff --git a/drivers/vfio/pci/virtio/main.c b/drivers/vfio/pci/virtio/main.c
> >> index b5d3a8c5bbc9..e2cdf2d48200 100644
> >> --- a/drivers/vfio/pci/virtio/main.c
> >> +++ b/drivers/vfio/pci/virtio/main.c
> > ...
> >> @@ -485,16 +478,66 @@ static bool virtiovf_bar0_exists(struct pci_dev *pdev)
> >> return res->flags;
> >> }
> >>
> >> +static int virtiovf_pci_init_device(struct vfio_device *core_vdev)
> >> +{
> >> + struct virtiovf_pci_core_device *virtvdev = container_of(core_vdev,
> >> + struct virtiovf_pci_core_device, core_device.vdev);
> >> + struct pci_dev *pdev;
> >> + bool sup_legacy_io;
> >> + bool sup_lm;
> >> + int ret;
> >> +
> >> + ret = vfio_pci_core_init_dev(core_vdev);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + pdev = virtvdev->core_device.pdev;
> >> + sup_legacy_io = virtio_pci_admin_has_legacy_io(pdev) &&
> >> + !virtiovf_bar0_exists(pdev);
> >> + sup_lm = virtio_pci_admin_has_dev_parts(pdev);
> >> +
> >> + /*
> >> + * If the device is not capable to this driver functionality, fallback
> >> + * to the default vfio-pci ops
> >> + */
> >> + if (!sup_legacy_io && !sup_lm) {
> >> + core_vdev->ops = &virtiovf_vfio_pci_ops;
> >> + return 0;
> >> + }
> >> +
> >> + if (sup_legacy_io) {
> >> + ret = virtiovf_read_notify_info(virtvdev);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + virtvdev->bar0_virtual_buf_size = VIRTIO_PCI_CONFIG_OFF(true) +
> >> + virtiovf_get_device_config_size(pdev->device);
> >> + BUILD_BUG_ON(!is_power_of_2(virtvdev->bar0_virtual_buf_size));
> >> + virtvdev->bar0_virtual_buf = kzalloc(virtvdev->bar0_virtual_buf_size,
> >> + GFP_KERNEL);
> >> + if (!virtvdev->bar0_virtual_buf)
> >> + return -ENOMEM;
> >> + mutex_init(&virtvdev->bar_mutex);
> >> + }
> >> +
> >> + if (sup_lm)
> >> + virtiovf_set_migratable(virtvdev);
> >> +
> >> + if (sup_lm && !sup_legacy_io)
> >> + core_vdev->ops = &virtiovf_vfio_pci_lm_ops;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> static int virtiovf_pci_probe(struct pci_dev *pdev,
> >> const struct pci_device_id *id)
> >> {
> >> - const struct vfio_device_ops *ops = &virtiovf_vfio_pci_ops;
> >> struct virtiovf_pci_core_device *virtvdev;
> >> + const struct vfio_device_ops *ops;
> >> int ret;
> >>
> >> - if (pdev->is_virtfn && virtio_pci_admin_has_legacy_io(pdev) &&
> >> - !virtiovf_bar0_exists(pdev))
> >> - ops = &virtiovf_vfio_pci_tran_ops;
> >> + ops = (pdev->is_virtfn) ? &virtiovf_vfio_pci_tran_lm_ops :
> >> + &virtiovf_vfio_pci_ops;
> >
> > I can't figure out why we moved the more thorough ops setup to the
> > .init() callback of the ops themselves. Clearly we can do the legacy
> > IO and BAR0 test here and the dev parts test uses the same mechanisms
> > as the legacy IO test, so it seems we could know sup_legacy_io and
> > sup_lm here. I think we can even do virtiovf_set_migratable() here
> > after virtvdev is allocated below.
> >
>
> Setting the 'ops' as part of the probe() seems actually doable,
> including calling virtiovf_set_migratable() following the virtiodev
> allocation below.
>
> The main issue with that approach will be the init part of the legacy IO
> (i.e. virtiovf_init_legacy_io()) as part of virtiovf_pci_init_device().
>
> Assuming that we don't want to repeat calling
> virtiovf_support_legacy_io() as part of virtiovf_pci_init_device() to
> know whether legacy IO is supported, we can consider calling
> virtiovf_init_legacy_io() as part of the probe() as well, which IMO
> doesn't look clean as it's actually seems to match the init flow.
>
> Alternatively, we can consider checking inside
> virtiovf_pci_init_device() whether the 'ops' actually equals the 'tran'
> ones and then call it.
>
> Something like the below.
>
> static int virtiovf_pci_init_device(struct vfio_device *core_vdev)
> {
> ...
>
> #ifdef CONFIG_VIRTIO_PCI_ADMIN_LEGACY
> if (core_vdev->ops == &virtiovf_vfio_pci_tran_lm_ops)
> return virtiovf_init_legacy_io(virtvdev);
> #endif
>
> return 0;
> }
>
> Do you prefer the above approach rather than current V1 code which has a
> single check as part of virtiovf_init_legacy_io() ?
If ops is properly configured and set-migratable is done in probe,
then doesn't only the legacy ops .init callback need to init the legacy
setup? The non-legacy, migration ops structure would just use
vfio_pci_core_init_dev.
>
> > I think the API to vfio core also suggests we shouldn't be modifying the
> > ops pointer after the core device is allocated.
>
> Any pointer for that ?
> Do we actually see a problem with replacing the 'ops' as part of the
> init flow ?
What makes it that way to me is that it's an argument to and set by the
object constructor. The ops callbacks should be considered live once
set. It's probably safe to do as you've done here because the
constructor calls the init callback directly, so we don't have any
races. However as Jason agreed, it's generally a pattern to avoid and I
think we can rather easily do so here. Thanks,
Alex
