Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

vfio: Reset vfio_container in detaching last group #17

Merged
merged 1 commit into from
Aug 20, 2024

Conversation

minwooim
Copy link
Collaborator

If application follows the calling sequence, it fails in getting device fd as the following logs, Assumming that there's only one vfio group and container with a single device attached.

nvme_ctrl_init (e.g., unvme add )
nvme_close (e.g., unvme del )
nvme_ctrl_init

I nvme_pci_init (src/nvme/core.c:510): nvme/core: pci class code is 0x010802
I vfio_get_device_fd (src/iommu/vfio.c:430): iommu/vfio: vfio iommu group is /dev/vfio/5
I vfio_group_set_container (src/iommu/vfio.c:290): iommu/vfio: adding group '/dev/vfio/5' to container
D vfio_get_device_fd (src/iommu/vfio.c:438): iommu/vfio: failed to get device fd
D vfio_pci_open (src/vfio/pci.c:151): vfio/pci: failed to get device fd

This is because kernel removes container->iommu_driver by setting it to NULL if this is the last vfio group to be detached from the current container. This will fail in the following vfio kernel code:

int vfio_group_use_container(struct vfio_group *group) {
lockdep_assert_held(&group->group_lock);

    /*
     * The container fd has been assigned with VFIO_GROUP_SET_CONTAINER but
     * VFIO_SET_IOMMU hasn't been done yet. */ if (!group->container->iommu_driver) return -EINVAL;

...

VFIO_SET_IOMMU ioctl is required to re-initailize container->iommu instance. But, current libvfn is skipping vfio_iommu_type1_init in case vfio->iommu_set is set.

This patch resets the vfio_container instance when the last vfio group is detached from the current container just like kernel does.

@minwooim minwooim requested a review from birkelund August 19, 2024 03:18
@birkelund birkelund added the approved Approved for device testing label Aug 19, 2024
@birkelund
Copy link
Collaborator

Looks good @minwooim. Can you force push to trigger the device tests? I added the Approved label so they run.

If application follows the calling sequence, it fails in getting device
fd as the following logs, Assumming that there's only one vfio group and
container with a single device attached.

 nvme_ctrl_init (e.g., unvme add <bdf>)
 nvme_close     (e.g., unvme del <bdf>)
 nvme_ctrl_init

 I nvme_pci_init (src/nvme/core.c:510): nvme/core: pci class code is 0x010802
 I vfio_get_device_fd (src/iommu/vfio.c:430): iommu/vfio: vfio iommu group is /dev/vfio/5
 I vfio_group_set_container (src/iommu/vfio.c:290): iommu/vfio: adding group '/dev/vfio/5' to container
 D vfio_get_device_fd (src/iommu/vfio.c:438): iommu/vfio: failed to get device fd
 D vfio_pci_open (src/vfio/pci.c:151): vfio/pci: failed to get device fd

This is because kernel removes `container->iommu_driver` by setting it
to NULL if this is the last vfio group to be detached from the current
container.  This will fail in the following vfio kernel code:

int vfio_group_use_container(struct vfio_group *group)
{
        lockdep_assert_held(&group->group_lock);

        /*
         * The container fd has been assigned with VFIO_GROUP_SET_CONTAINER but
         * VFIO_SET_IOMMU hasn't been done yet.
         */
        if (!group->container->iommu_driver)
                return -EINVAL;

	...

VFIO_SET_IOMMU ioctl is required to re-initailize `container->iommu`
instance.  But, current `libvfn` is skipping vfio_iommu_type1_init in
case vfio->iommu_set is set.

This patch resets the vfio_container instance when the last vfio group
is detached from the current container just like kernel does.

Signed-off-by: Minwoo Im <[email protected]>
@minwooim minwooim force-pushed the vfio-close-enhancement branch from ae65a3f to 1ec7c6c Compare August 19, 2024 22:30
@birkelund birkelund merged commit 3fb1ff2 into SamsungDS:main Aug 20, 2024
23 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Approved for device testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants