Skip to content
This repository has been archived by the owner on Jun 28, 2024. It is now read-only.

vfio: Extend the current VFIO test for GPU and other devices #5729

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

zvonkok
Copy link
Contributor

@zvonkok zvonkok commented Jul 21, 2023

The current VFIO test only works with a virtualized network device.
Extend the VFIO tests so that "any" device that is passed-through can be tested.

@katacontainersbot katacontainersbot added the size/small Small and simple task label Jul 21, 2023
@zvonkok zvonkok force-pushed the vfio-gpu branch 5 times, most recently from c6d17fe to 0edd498 Compare July 21, 2023 09:50
@zvonkok
Copy link
Contributor Author

zvonkok commented Jul 21, 2023

/test-vfio

@zvonkok zvonkok changed the title vfio: Extend the current VFIO test for GPUs vfio: Extend the current VFIO test for GPU and other devices Jul 21, 2023
@zvonkok
Copy link
Contributor Author

zvonkok commented Jul 23, 2023

/test-vfio

@katacontainersbot katacontainersbot added size/medium Average sized task and removed size/small Small and simple task labels Jul 23, 2023
@katacontainersbot katacontainersbot added size/tiny Smallest and simplest task and removed size/medium Average sized task labels Jul 23, 2023
@zvonkok zvonkok force-pushed the vfio-gpu branch 2 times, most recently from 933bbc1 to 73871b1 Compare July 23, 2023 15:36
@zvonkok
Copy link
Contributor Author

zvonkok commented Jul 23, 2023

/test-vfio

@zvonkok
Copy link
Contributor Author

zvonkok commented Jul 23, 2023

/test-vfio

@katacontainersbot katacontainersbot added size/medium Average sized task and removed size/tiny Smallest and simplest task labels Jul 23, 2023
@zvonkok
Copy link
Contributor Author

zvonkok commented Jul 23, 2023

/test-vfio

@katacontainersbot katacontainersbot added size/tiny Smallest and simplest task and removed size/medium Average sized task labels Jul 23, 2023
@zvonkok
Copy link
Contributor Author

zvonkok commented Jul 23, 2023

/test-vfio

@zvonkok
Copy link
Contributor Author

zvonkok commented Jul 23, 2023

/test-vfio

@zvonkok
Copy link
Contributor Author

zvonkok commented Jul 23, 2023

/test

@zvonkok
Copy link
Contributor Author

zvonkok commented Jul 23, 2023

Added cold_plug_vfio test besides hot_plug_vfio test.

@zvonkok
Copy link
Contributor Author

zvonkok commented Jul 24, 2023

/retest-s390x

Copy link
Member

@BbolroC BbolroC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @zvonkok !

functional/vfio/run.sh Outdated Show resolved Hide resolved
functional/vfio/run.sh Outdated Show resolved Hide resolved
functional/vfio/run.sh Outdated Show resolved Hide resolved
functional/vfio/run.sh Outdated Show resolved Hide resolved
functional/vfio/run.sh Outdated Show resolved Hide resolved
@zvonkok
Copy link
Contributor Author

zvonkok commented Jul 24, 2023

/test

@zvonkok
Copy link
Contributor Author

zvonkok commented Jul 25, 2023

@GabyCT PTAL Thanks!

functional/vfio/run.sh Outdated Show resolved Hide resolved
@zvonkok
Copy link
Contributor Author

zvonkok commented Jul 25, 2023

/test-vfio

@zvonkok
Copy link
Contributor Author

zvonkok commented Jul 25, 2023

@jepio if the config variable is not available it will be not set, updating the sed command ...

@jepio
Copy link
Member

jepio commented Jul 25, 2023

@jepio if the config variable is not available it will be not set, updating the sed command ...

I don't think the sed expression is wrong, I think there is some logic error here:

        if [ "${VFIO_COLDPLUG}" != "bridge-port" ]; then

wouldn't you want this to check against no-port? Same for VFIO_HOTPLUG.

@zvonkok
Copy link
Contributor Author

zvonkok commented Jul 25, 2023

@jepio if you look at the config file generated there is no cold_plug_vfio variable to update.

10:59:50 hot_plug_vfio = "no-port"

The cold-plug test-case is started without the cold_plug_vfio=.... set.

@zvonkok
Copy link
Contributor Author

zvonkok commented Jul 25, 2023

Need to update the logic, a VFIO test can have both hot-plug and cold-plug set to the same value. One may want to test

hot_plug_vfio="root-port"

first and then

cold_plug_vfio="root-port"

@zvonkok
Copy link
Contributor Author

zvonkok commented Jul 25, 2023

So essentially we only need one VFIO_PORT setting since we test hot and cold plug in one run.

@zvonkok
Copy link
Contributor Author

zvonkok commented Jul 25, 2023

/test-vfio

Fix default port assignment with an explicit value in the TOML
It was implicit before and now the runtime will bail out if we did not set it explicitly.
Update the configuration and set it. This will make the intent more clear.

Fixes: kata-containers#5726

Signed-off-by: Zvonko Kaiser <zkaiser@nvidia.com>
@zvonkok
Copy link
Contributor Author

zvonkok commented Jul 25, 2023

/test-vfio

@katacontainersbot katacontainersbot added size/large Task of significant size and removed size/tiny Smallest and simplest task labels Jul 25, 2023
@jepio
Copy link
Member

jepio commented Jul 26, 2023

Looks like there's an actual bug in handling /dev/vfio/vfio in coldplug vfio mode

@zvonkok
Copy link
Contributor Author

zvonkok commented Jul 26, 2023

Yep, we need a "simple" runtime fix before merging this.

@zvonkok
Copy link
Contributor Author

zvonkok commented Jul 26, 2023

cold-plug vfio_mode=guest-kernel works as expected,
cold-plug vfio_mode=vfio misses the check for the VFIO control file.

@zvonkok
Copy link
Contributor Author

zvonkok commented Jul 31, 2023

During testing why it fails for cold_plug I found a bug in the hot_plug implementation. It depends in which order you supplied the vfio-control-device and the vfio-device-group.
1st vfio-control-device 2nd vfio-device-group -> works
2st vfio-device-group 2nd vfio-control-group -> breaks

@zvonkok
Copy link
Contributor Author

zvonkok commented Aug 2, 2023

/test-vfio

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
size/large Task of significant size
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants