Re: [kvm-unit-tests RFC PATCH 4/5] scripts: Generate kvmtool standalone tests

On Wed, Sep 08, 2021 at 04:37:39PM +0100, Alexandru Elisei wrote:
> Hi Drew,
> 
> On 9/7/21 11:21 AM, Andrew Jones wrote:
> > On Fri, Jul 02, 2021 at 05:31:21PM +0100, Alexandru Elisei wrote:
> >> Add support for the standalone target when running kvm-unit-tests under
> >> kvmtool.
> >>
> >> Example command line invocation:
> >>
> >> $ ./configure --target=kvmtool
> >> $ make clean && make standalone
> >>
> >> Signed-off-by: Alexandru Elisei <[email protected]>
> >> ---
> >>  scripts/mkstandalone.sh | 14 +++++++-------
> >>  1 file changed, 7 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
> >> index 16f461c06842..d84bdb7e278c 100755
> >> --- a/scripts/mkstandalone.sh
> >> +++ b/scripts/mkstandalone.sh
> >> @@ -44,6 +44,10 @@ generate_test ()
> >>  	config_export ARCH_NAME
> >>  	config_export PROCESSOR
> >>  
> >> +	if [ "$ARCH" = "arm64" ] || [ "$ARCH" = "arm" ]; then
> >> +		config_export TARGET
> >> +	fi
> > Should export unconditionally, since we'll want TARGET set
> > unconditionally.
> 
> Yes, will do.
> 
> >
> >> +
> >>  	echo "echo BUILD_HEAD=$(cat build-head)"
> >>  
> >>  	if [ ! -f $kernel ]; then
> >> @@ -59,7 +63,7 @@ generate_test ()
> >>  		echo 'export FIRMWARE'
> >>  	fi
> >>  
> >> -	if [ "$ENVIRON_DEFAULT" = "yes" ] && [ "$ERRATATXT" ]; then
> >> +	if [ "$TARGET" != "kvmtool" ] && [ "$ENVIRON_DEFAULT" = "yes" ] && [ "$ERRATATXT" ]; then
> > I think it would be better to ensure that ENVIRON_DEFAULT is "no" for
> > TARGET=kvmtool in configure.
> 
> From looking at the code, it is my understanding that with ENVIRON_DEFAULT=yes, an
> initrd file is generated with the contents of erratatxt and other information, in
> a key=value pair format. This initrd is then passed on to the test (please correct
> me if I'm wrong). With ENVIRON_DEFAULT=no (set via ./configure
> --disable-default-environ), this initrd is not generated.
> 
> kvmtool doesn't have support for passing an initrd when loading firmware, so yes,
> I believe the default should be no.
> 
> However, I have two questions:
> 
> 1. What happens when the user specifically enables the default environ via
> ./configure --enable-default-environ --target=kvmtool? In my opinion, that should
> be an error because the user wants something that is not possible with kvmtool
> (loading an image with --firmware in kvmtool means that the initrd image it not
> loaded into the guest memory and no node is generated for it in the dtb), but I
> would like to hear your thoughts about it.

As part of the forcing ENVIRON_DEFAULT to "no" for kvmtool in configure an
error should be generated if a user tries to explicitly enable it.

> 
> 2. If the default environment is disabled, is it still possible for an user to
> pass an initrd via other means? I couldn't find where that is implemented, so I'm
> guessing it's not possible.

Yes, a user could have a KVM_UNIT_TESTS_ENV environment variable set when
they launch the tests. If that variable points to a file then it will get
passed as an initrd. I guess you should also report a warning in arm/run
if KVM_UNIT_TESTS_ENV is set which states that the environment file will
be ignored when running with kvmtool.

There aren't currently any other ways to invoke the addition of the
-initrd command line option, because so far we only support passing a
single file to test (the environment "file"). If we ever want to pass
more files, then we'd need to create a simple file system on the initrd
and make it possible to add -initrd even when no environment is desired.
But, that may never happen.

Thanks,
drew

> 
> Thanks,
> 
> Alex
> 
> >
> >
> >>  		temp_file ERRATATXT "$ERRATATXT"
> >>  		echo 'export ERRATATXT'
> >>  	fi
> >> @@ -95,12 +99,8 @@ function mkstandalone()
> >>  	echo Written $standalone.
> >>  }
> >>  
> >> -if [ "$TARGET" = "kvmtool" ]; then
> >> -	echo "Standalone tests not supported with kvmtool"
> >> -	exit 2
> >> -fi
> >> -
> >> -if [ "$ENVIRON_DEFAULT" = "yes" ] && [ "$ERRATATXT" ] && [ ! -f "$ERRATATXT" ]; then
> >> +if [ "$TARGET" != "kvmtool" ] && [ "$ENVIRON_DEFAULT" = "yes" ] && \
> >> +		[ "$ERRATATXT" ] && [ ! -f "$ERRATATXT" ]; then
> >>  	echo "$ERRATATXT not found. (ERRATATXT=$ERRATATXT)" >&2
> >>  	exit 2
> >>  fi
> >> -- 
> >> 2.32.0
> >>
> > Thanks,
> > drew 
> >
>