Re: [PATCH][KVM-AUTOTEST][REPOST] Add ability to install custom kernel modules

[Mike Burns <mburns@redhat.com>]

Uri Lublin wrote:
> On 05/21/2009 03:29 AM, Mike Burns wrote:
>> See comment in control file for details of implementation
>>
>> Signed-off-by: Mike Burns<mburns@redhat.com>
>> ---
>>   client/tests/kvm_runtest_2/control        |    6 ++++++
>>   client/tests/kvm_runtest_2/kvm_install.py |   11 +++++++++--
>>   2 files changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/client/tests/kvm_runtest_2/control
>> b/client/tests/kvm_runtest_2/control
>> index d6e26bc..437de4c 100644
>> --- a/client/tests/kvm_runtest_2/control
>> +++ b/client/tests/kvm_runtest_2/control
>> @@ -74,6 +74,12 @@ params = {
>>
>>       ## Custom install
>>       "install_script": 'custom_kvm_install.sh param1'
>> +
>> +    ## Additional kernel modules to install
>> +    ## Must be a space separated list of values
>> +    ## Installed in the order they are listed.
>> +    ## to install mod1.ko, mod2.ko, mod3.ko, you would set like this:
>> +    #"additional_modules": 'mod1 mod2 mod3'
>>   }
>>
>>   # Comment the job.run_test line if you do not want to install kvm
>> on the host.
>> diff --git a/client/tests/kvm_runtest_2/kvm_install.py
>> b/client/tests/kvm_runtest_2/kvm_install.py
>> index 392ef0c..80354f5 100755
>> --- a/client/tests/kvm_runtest_2/kvm_install.py
>> +++ b/client/tests/kvm_runtest_2/kvm_install.py
>> @@ -106,7 +106,7 @@ def run_kvm_install(test, params, env):
>>
>>       # load kvm modules (unless requested not to)
>>       if params.get('load_modules', "yes") == "yes":
>> -        __load_kvm_modules()
>> +        __load_kvm_modules(params)
>>       else:
>>           kvm_log.info("user requested not to load kvm modules")
>>
>> @@ -209,7 +209,7 @@ def __install_kvm_from_local_tarball(test,
>> srcdir, tarball):
>>       __install_kvm(test, srcdir)
>>
>>
>> -def __load_kvm_modules():
>> +def __load_kvm_modules(params):
>>       kvm_log.info("Detecting CPU vendor...")
>>       vendor = "intel"
>>       if os.system("grep vmx /proc/cpuinfo 1>/dev/null") != 0:
>> @@ -237,6 +237,13 @@ def __load_kvm_modules():
>>           os.chdir("x86")
>>       utils.system("/sbin/insmod ./kvm.ko&&  sleep 1&&  /sbin/insmod
>> ./kvm-%s.ko" % vendor)
>>
>> +    #Add additional modules specified in params by "additional_modules"
>> +    #Modules must be named<value>.ko and be located in the
>> +    #same location as kvm and kvm-vendor modules
>> +    for module in params.get("additional_modules","").split():
>> +      kvm_log.info("Installing module \"%s\"" % module)
>> +      utils.system("/sbin/insmod ./%s.ko" % module )
>> +
>>       #elif self.config.load_modules == "no":
>>           #kvm_log.info("user requested not to load kvm modules")
>>
>
> Hi Mike,
>
> Can you load those kernel modules before running kvm-autotest
> (something like a setup script) ? And clean up when the run completes
> (cleanup script) ?
>
> How would those modules get into the directory where kvm.ko is built.
>
> Who is in charge to unload those modules ? What happen if we have 2
> consecutive runs of kvm-autotest (the second insmod would fail,
> wouldn't it) ?
>
> This may belong to a separate patch, but I think it's a good idea to
> support module-parameters for each of those modules (specifically I
> think of ept/npt and other params of kvm*.ko)
>
> Sorry for the late reply,
>     Uri.
Uri,

The thought here was to work in conjunction with the custom install
option that was sent in a previous patch.   The script would handle
placing the modules in the correct locations.

As for cleaning up modules, the patch to make the removal of kvm modules
more robust will handle it as long as the additional modules depend in
some way on kvm or one of its dependent modules.  Lucas was reworking
this a bit to put it in a more generic place in the autotest framework,
but I'm not sure exactly where that is.  The code will now remove all
modules recursively by detecting the dependent modules and removing
those first. 

I agree that module parameters should be handled in a separate patch.

Mike