[PATCH] kvm-unit-tests: Build changes for illumos.

[PATCH] kvm-unit-tests: Build changes for illumos.

[Dan Cross]

We have begun using kvm-unit-tests to test Bhyve under
illumos.  We started by cross-compiling the tests on Linux
and transfering the binary artifacts to illumos machines,
but it proved more convenient to build them directly on
illumos.

This change modifies the build infrastructure to allow
building on illumos; I have also tested it on Linux.  The
required changes were pretty minimal: the most invasive
was switching from using the C compiler as a linker driver
to simply invoking the linker directly in two places.
This allows us to easily use gold instead of the Solaris
linker.

Signed-off-by: Dan Cross <cross@oxidecomputer.com>
---
 configure           | 5 +++--
 x86/Makefile.common | 6 +++---
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/configure b/configure
index 86c3095..7193811 100755
--- a/configure
+++ b/configure
@@ -15,6 +15,7 @@ objdump=objdump
 ar=ar
 addr2line=addr2line
 arch=$(uname -m | sed -e 's/i.86/i386/;s/arm64/aarch64/;s/arm.*/arm/;s/ppc64.*/ppc64/')
+os=$(uname -s)
 host=$arch
 cross_prefix=
 endian=""
@@ -317,9 +318,9 @@ EOF
   rm -f lib-test.{o,S}
 fi
 
-# require enhanced getopt
+# require enhanced getopt everywhere except illumos
 getopt -T > /dev/null
-if [ $? -ne 4 ]; then
+if [ $? -ne 4 ] && [ "$os" != "SunOS" ]; then
     echo "Enhanced getopt is not available, add it to your PATH?"
     exit 1
 fi
diff --git a/x86/Makefile.common b/x86/Makefile.common
index b903988..0a0f7b9 100644
--- a/x86/Makefile.common
+++ b/x86/Makefile.common
@@ -62,7 +62,7 @@ else
 .PRECIOUS: %.elf %.o
 
 %.elf: %.o $(FLATLIBS) $(SRCDIR)/x86/flat.lds $(cstart.o)
-	$(CC) $(CFLAGS) -nostdlib -o $@ -Wl,-T,$(SRCDIR)/x86/flat.lds \
+	$(LD) -T $(SRCDIR)/x86/flat.lds -nostdlib -o $@ \
 		$(filter %.o, $^) $(FLATLIBS)
 	@chmod a-x $@
 
@@ -98,8 +98,8 @@ test_cases: $(tests-common) $(tests)
 $(TEST_DIR)/%.o: CFLAGS += -std=gnu99 -ffreestanding -I $(SRCDIR)/lib -I $(SRCDIR)/lib/x86 -I lib
 
 $(TEST_DIR)/realmode.elf: $(TEST_DIR)/realmode.o
-	$(CC) -m32 -nostdlib -o $@ -Wl,-m,elf_i386 \
-	      -Wl,-T,$(SRCDIR)/$(TEST_DIR)/realmode.lds $^
+	$(LD) -m elf_i386 -nostdlib -o $@ \
+	      -T $(SRCDIR)/$(TEST_DIR)/realmode.lds $^
 
 $(TEST_DIR)/realmode.o: bits = $(if $(call cc-option,-m16,""),16,32)
 
-- 
2.36.1

Re: [PATCH] kvm-unit-tests: Build changes for illumos.

[Sean Christopherson]

On Thu, May 12, 2022, Dan Cross wrote:
> We have begun using kvm-unit-tests to test Bhyve under
> illumos.  We started by cross-compiling the tests on Linux
> and transfering the binary artifacts to illumos machines,
> but it proved more convenient to build them directly on
> illumos.
> 
> This change modifies the build infrastructure to allow
> building on illumos; I have also tested it on Linux.  The
> required changes were pretty minimal: the most invasive
> was switching from using the C compiler as a linker driver
> to simply invoking the linker directly in two places.
> This allows us to easily use gold instead of the Solaris
> linker.

Can you please split this into two patches?  One for the $(CC) => $(LD) change,
and one for the getopt thing.  The switch to $(LD) in particular could be valuable
irrespective of using a non-Linux OS.

> Signed-off-by: Dan Cross <cross@oxidecomputer.com>
> ---
>  configure           | 5 +++--
>  x86/Makefile.common | 6 +++---
>  2 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/configure b/configure
> index 86c3095..7193811 100755
> --- a/configure
> +++ b/configure
> @@ -15,6 +15,7 @@ objdump=objdump
>  ar=ar
>  addr2line=addr2line
>  arch=$(uname -m | sed -e 's/i.86/i386/;s/arm64/aarch64/;s/arm.*/arm/;s/ppc64.*/ppc64/')
> +os=$(uname -s)
>  host=$arch
>  cross_prefix=
>  endian=""
> @@ -317,9 +318,9 @@ EOF
>    rm -f lib-test.{o,S}
>  fi
>  
> -# require enhanced getopt
> +# require enhanced getopt everywhere except illumos
>  getopt -T > /dev/null
> -if [ $? -ne 4 ]; then
> +if [ $? -ne 4 ] && [ "$os" != "SunOS" ]; then

Presumably whatever "enhanced" features are being used are supported by illumos,
so rather than check the OS, why not improve the probing?

>      echo "Enhanced getopt is not available, add it to your PATH?"
>      exit 1
>  fi

[PATCH 0/2] kvm-unit-tests: Build changes to support illumos.

[Dan Cross]

We have begun using kvm-unit-tests to test Bhyve under
illumos.  We started by cross-compiling the tests on Linux
and transfering the binary artifacts to illumos machines,
but it proved more convenient to build them directly on
illumos.

This patch series modifies the build infrastructure to allow
building on illumos; I have also tested these changes on Linux.
The required changes were pretty minimal.

Dan Cross (2):
  kvm-unit-tests: invoke $LD explicitly in
  kvm-unit-tests: configure changes for illumos.

 configure           | 5 +++--
 x86/Makefile.common | 6 +++---
 2 files changed, 6 insertions(+), 5 deletions(-)

-- 
2.31.1

[PATCH 1/2] kvm-unit-tests: invoke $LD explicitly in

[Dan Cross]

This change modifies x86/Makefile.common to allow building on
illumos; I have also tested it on Linux.  This switches from
using the C compiler as a linker driver to simply invoking the
linker directly in two places, allowing us to easily use gold
instead of the Solaris linker.

Signed-off-by: Dan Cross <cross@oxidecomputer.com>
---
 x86/Makefile.common | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/x86/Makefile.common b/x86/Makefile.common
index b903988..0a0f7b9 100644
--- a/x86/Makefile.common
+++ b/x86/Makefile.common
@@ -62,7 +62,7 @@ else
 .PRECIOUS: %.elf %.o
 
 %.elf: %.o $(FLATLIBS) $(SRCDIR)/x86/flat.lds $(cstart.o)
-	$(CC) $(CFLAGS) -nostdlib -o $@ -Wl,-T,$(SRCDIR)/x86/flat.lds \
+	$(LD) -T $(SRCDIR)/x86/flat.lds -nostdlib -o $@ \
 		$(filter %.o, $^) $(FLATLIBS)
 	@chmod a-x $@
 
@@ -98,8 +98,8 @@ test_cases: $(tests-common) $(tests)
 $(TEST_DIR)/%.o: CFLAGS += -std=gnu99 -ffreestanding -I $(SRCDIR)/lib -I $(SRCDIR)/lib/x86 -I lib
 
 $(TEST_DIR)/realmode.elf: $(TEST_DIR)/realmode.o
-	$(CC) -m32 -nostdlib -o $@ -Wl,-m,elf_i386 \
-	      -Wl,-T,$(SRCDIR)/$(TEST_DIR)/realmode.lds $^
+	$(LD) -m elf_i386 -nostdlib -o $@ \
+	      -T $(SRCDIR)/$(TEST_DIR)/realmode.lds $^
 
 $(TEST_DIR)/realmode.o: bits = $(if $(call cc-option,-m16,""),16,32)
 
-- 
2.31.1

[PATCH 2/2] kvm-unit-tests: configure changes for illumos.

[Dan Cross]

This change modifies the `configure` script to run under illumos
by not probing for, `getopt -T` (illumos `getopt` supports the
required functionality, but exits with a different return status
when invoked with `-T`).

Signed-off-by: Dan Cross <cross@oxidecomputer.com>
---
 configure | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/configure b/configure
index 86c3095..7193811 100755
--- a/configure
+++ b/configure
@@ -15,6 +15,7 @@ objdump=objdump
 ar=ar
 addr2line=addr2line
 arch=$(uname -m | sed -e 's/i.86/i386/;s/arm64/aarch64/;s/arm.*/arm/;s/ppc64.*/ppc64/')
+os=$(uname -s)
 host=$arch
 cross_prefix=
 endian=""
@@ -317,9 +318,9 @@ EOF
   rm -f lib-test.{o,S}
 fi
 
-# require enhanced getopt
+# require enhanced getopt everywhere except illumos
 getopt -T > /dev/null
-if [ $? -ne 4 ]; then
+if [ $? -ne 4 ] && [ "$os" != "SunOS" ]; then
     echo "Enhanced getopt is not available, add it to your PATH?"
     exit 1
 fi
-- 
2.31.1

Re: [PATCH 2/2] kvm-unit-tests: configure changes for illumos.

[Sean Christopherson]

Adding the official KUT maintainers, they undoubtedly know more about the getopt
stuff than me.

On Fri, May 13, 2022, Dan Cross wrote:
> This change modifies the `configure` script to run under illumos

Nit, use imperative mood.  KUT follows the kernel's rules/guidelines for the most
part.  From Linux's Documentation/process/submitting-patches.rst:

  Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
  instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
  to do frotz", as if you are giving orders to the codebase to change
  its behaviour.


E.g.

  Exempt illumos, which reports itself as SunOS, from the `getopt -T` check
  for enhanced getopt.   blah blah blah... 

> by not probing for, `getopt -T` (illumos `getopt` supports the
> required functionality, but exits with a different return status
> when invoked with `-T`).
> 
> Signed-off-by: Dan Cross <cross@oxidecomputer.com>
> ---
>  configure | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/configure b/configure
> index 86c3095..7193811 100755
> --- a/configure
> +++ b/configure
> @@ -15,6 +15,7 @@ objdump=objdump
>  ar=ar
>  addr2line=addr2line
>  arch=$(uname -m | sed -e 's/i.86/i386/;s/arm64/aarch64/;s/arm.*/arm/;s/ppc64.*/ppc64/')
> +os=$(uname -s)
>  host=$arch
>  cross_prefix=
>  endian=""
> @@ -317,9 +318,9 @@ EOF
>    rm -f lib-test.{o,S}
>  fi
>  
> -# require enhanced getopt
> +# require enhanced getopt everywhere except illumos
>  getopt -T > /dev/null
> -if [ $? -ne 4 ]; then
> +if [ $? -ne 4 ] && [ "$os" != "SunOS" ]; then

What does illumos return for `getopt -T`?  Unless it's a direct collision with
the "old" getopt, why not check for illumos' return?  The SunOS check could be
kept (or not).  E.g. IMO this is much more self-documenting (though does $? get
clobbered by the check?  I'm terrible at shell scripts...).

if [ $? -ne 4 ] && [ "$os" != "SunOS" || $? -ne 666 ]; then

  Test if your getopt(1) is this enhanced version or an old version. This
  generates no output, and sets the error status to 4. Other implementations of
  getopt(1), and this version if the environment variable GETOPT_COMPATIBLE is
  set, will return '--' and error status 0.

>      echo "Enhanced getopt is not available, add it to your PATH?"
>      exit 1
>  fi
> -- 
> 2.31.1
> 

Re: [PATCH 2/2] kvm-unit-tests: configure changes for illumos.

[Thomas Huth]

On 13/05/2022 16.34, Sean Christopherson wrote:
> Adding the official KUT maintainers, they undoubtedly know more about the getopt
> stuff than me.
> 
> On Fri, May 13, 2022, Dan Cross wrote:
>> This change modifies the `configure` script to run under illumos
> 
> Nit, use imperative mood.  KUT follows the kernel's rules/guidelines for the most
> part.  From Linux's Documentation/process/submitting-patches.rst:
> 
>    Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
>    instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
>    to do frotz", as if you are giving orders to the codebase to change
>    its behaviour.
> 
> 
> E.g.
> 
>    Exempt illumos, which reports itself as SunOS, from the `getopt -T` check
>    for enhanced getopt.   blah blah blah...
> 
>> by not probing for, `getopt -T` (illumos `getopt` supports the
>> required functionality, but exits with a different return status
>> when invoked with `-T`).
>>
>> Signed-off-by: Dan Cross <cross@oxidecomputer.com>
>> ---
>>   configure | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/configure b/configure
>> index 86c3095..7193811 100755
>> --- a/configure
>> +++ b/configure
>> @@ -15,6 +15,7 @@ objdump=objdump
>>   ar=ar
>>   addr2line=addr2line
>>   arch=$(uname -m | sed -e 's/i.86/i386/;s/arm64/aarch64/;s/arm.*/arm/;s/ppc64.*/ppc64/')
>> +os=$(uname -s)
>>   host=$arch
>>   cross_prefix=
>>   endian=""
>> @@ -317,9 +318,9 @@ EOF
>>     rm -f lib-test.{o,S}
>>   fi
>>   
>> -# require enhanced getopt
>> +# require enhanced getopt everywhere except illumos
>>   getopt -T > /dev/null
>> -if [ $? -ne 4 ]; then
>> +if [ $? -ne 4 ] && [ "$os" != "SunOS" ]; then
> 
> What does illumos return for `getopt -T`?  Unless it's a direct collision with
> the "old" getopt, why not check for illumos' return?  The SunOS check could be
> kept (or not).  E.g. IMO this is much more self-documenting (though does $? get
> clobbered by the check?  I'm terrible at shell scripts...).

According to https://illumos.org/man/1/getopt :

  NOTES

        getopt will not be supported in the next major release.
        ...

So even if we apply this fix now, this will likely break soon again. Is 
there another solution to this problem?

  Thomas

Re: [PATCH 2/2] kvm-unit-tests: configure changes for illumos.

[Dan Cross]

On Thu, May 19, 2022 at 5:53 AM Thomas Huth <thuth@redhat.com> wrote:
> On 13/05/2022 16.34, Sean Christopherson wrote:
> > [snip]
>
> According to https://illumos.org/man/1/getopt :
>
>   NOTES
>
>         getopt will not be supported in the next major release.
>         ...
>
> So even if we apply this fix now, this will likely break soon again. Is
> there another solution to this problem?

I wouldn't put too much stock into that; that note came from Solaris
and has been in the man page for 30 years or more [*]. I think there
are too many shell scripts in the wild using `getopt` for it to ever be
removed. Indeed, if anything this highlights something to clean up
on the illumos side by removing that from the man page.

        - Dan C.

[*] e.g. https://www.freebsd.org/cgi/man.cgi?query=getopt&apropos=0&sektion=0&manpath=SunOS+5.5.1&arch=default&format=html
from 1992!

Re: [PATCH 2/2] kvm-unit-tests: configure changes for illumos.

[Thomas Huth]

On 24/05/2022 23.20, Dan Cross wrote:
> On Thu, May 19, 2022 at 5:53 AM Thomas Huth <thuth@redhat.com> wrote:
>> On 13/05/2022 16.34, Sean Christopherson wrote:
>>> [snip]
>>
>> According to https://illumos.org/man/1/getopt :
>>
>>    NOTES
>>
>>          getopt will not be supported in the next major release.
>>          ...
>>
>> So even if we apply this fix now, this will likely break soon again. Is
>> there another solution to this problem?
> 
> I wouldn't put too much stock into that; that note came from Solaris
> and has been in the man page for 30 years or more [*].

:-)

> I think there
> are too many shell scripts in the wild using `getopt` for it to ever be
> removed. Indeed, if anything this highlights something to clean up
> on the illumos side by removing that from the man page.

Yup, cleaning up that man page sounds like a good idea then, indeed.

  Thomas

Re: [PATCH 2/2] kvm-unit-tests: configure changes for illumos.

[Dan Cross]

On Fri, May 13, 2022 at 10:35 AM Sean Christopherson <seanjc@google.com> wrote:
> Adding the official KUT maintainers, they undoubtedly know more about the getopt
> stuff than me.

Thanks.

> On Fri, May 13, 2022, Dan Cross wrote:
> > This change modifies the `configure` script to run under illumos
>
> Nit, use imperative mood.  KUT follows the kernel's rules/guidelines for the most
> part.  From Linux's Documentation/process/submitting-patches.rst:
> [snip]

Done locally, but see below.

> > diff --git a/configure b/configure
> > index 86c3095..7193811 100755
> > --- a/configure
> > +++ b/configure
> > @@ -15,6 +15,7 @@ objdump=objdump
> >  ar=ar
> >  addr2line=addr2line
> >  arch=$(uname -m | sed -e 's/i.86/i386/;s/arm64/aarch64/;s/arm.*/arm/;s/ppc64.*/ppc64/')
> > +os=$(uname -s)
> >  host=$arch
> >  cross_prefix=
> >  endian=""
> > @@ -317,9 +318,9 @@ EOF
> >    rm -f lib-test.{o,S}
> >  fi
> >
> > -# require enhanced getopt
> > +# require enhanced getopt everywhere except illumos
> >  getopt -T > /dev/null
> > -if [ $? -ne 4 ]; then
> > +if [ $? -ne 4 ] && [ "$os" != "SunOS" ]; then
>
> What does illumos return for `getopt -T`?

Sadly, it returns "0".  I was wrong in my earlier explorations
because I did not realize that `configure` does not use `getopt`
aside from that one check, which is repeated in `run_tests.sh`.

I would argue that the most straight-forward way to deal with
this is to just remove the check for "getopt" from "configure",
which doesn't otherwise use "getopt".  The only place it is
used is in `run_tests.sh`, which is unlikely to be used directly
for illumos, and repeats the check anyway.

> Unless it's a direct collision with
> the "old" getopt, why not check for illumos' return?

It collides with "legacy" getopt. :-(

> The SunOS check could be
> kept (or not).  E.g. IMO this is much more self-documenting (though does $? get
> clobbered by the check?  I'm terrible at shell scripts...).

I think it is more or less moot now, but "$?" does get clobbered
by the check.

> if [ $? -ne 4 ] && [ "$os" != "SunOS" || $? -ne 666 ]; then
>
>   Test if your getopt(1) is this enhanced version or an old version. This
>   generates no output, and sets the error status to 4. Other implementations of
>   getopt(1), and this version if the environment variable GETOPT_COMPATIBLE is
>   set, will return '--' and error status 0.

Sigh. That's precisely what the illumos version does. :-(

But it's perhaps worth pointing out that `getopt` isn't used
by `configure` aside from just that check. Does it, perhaps,
make more sense just to remove it from `configure` entirely?

        - Dan C.

Re: [PATCH 2/2] kvm-unit-tests: configure changes for illumos.

[Thomas Huth]

On 24/05/2022 23.22, Dan Cross wrote:
> On Fri, May 13, 2022 at 10:35 AM Sean Christopherson <seanjc@google.com> wrote:
...
>>> diff --git a/configure b/configure
>>> index 86c3095..7193811 100755
>>> --- a/configure
>>> +++ b/configure
>>> @@ -15,6 +15,7 @@ objdump=objdump
>>>   ar=ar
>>>   addr2line=addr2line
>>>   arch=$(uname -m | sed -e 's/i.86/i386/;s/arm64/aarch64/;s/arm.*/arm/;s/ppc64.*/ppc64/')
>>> +os=$(uname -s)
>>>   host=$arch
>>>   cross_prefix=
>>>   endian=""
>>> @@ -317,9 +318,9 @@ EOF
>>>     rm -f lib-test.{o,S}
>>>   fi
>>>
>>> -# require enhanced getopt
>>> +# require enhanced getopt everywhere except illumos
>>>   getopt -T > /dev/null
>>> -if [ $? -ne 4 ]; then
>>> +if [ $? -ne 4 ] && [ "$os" != "SunOS" ]; then
>>
>> What does illumos return for `getopt -T`?
> 
> Sadly, it returns "0".  I was wrong in my earlier explorations
> because I did not realize that `configure` does not use `getopt`
> aside from that one check, which is repeated in `run_tests.sh`.
> 
> I would argue that the most straight-forward way to deal with
> this is to just remove the check for "getopt" from "configure",
> which doesn't otherwise use "getopt".  The only place it is
> used is in `run_tests.sh`, which is unlikely to be used directly
> for illumos, and repeats the check anyway.

Fine for me if we remove the check from configure, or turn it into a warning 
instead ("Enhanced getopt is not available, you won't be able to use the 
run_tests.sh script" or so).

  Thomas

Re: [PATCH 2/2] kvm-unit-tests: configure changes for illumos.

[Andrew Jones]

On Wed, May 25, 2022 at 09:44:33AM +0200, Thomas Huth wrote:
> On 24/05/2022 23.22, Dan Cross wrote:
> > On Fri, May 13, 2022 at 10:35 AM Sean Christopherson <seanjc@google.com> wrote:
> ...
> > > > diff --git a/configure b/configure
> > > > index 86c3095..7193811 100755
> > > > --- a/configure
> > > > +++ b/configure
> > > > @@ -15,6 +15,7 @@ objdump=objdump
> > > >   ar=ar
> > > >   addr2line=addr2line
> > > >   arch=$(uname -m | sed -e 's/i.86/i386/;s/arm64/aarch64/;s/arm.*/arm/;s/ppc64.*/ppc64/')
> > > > +os=$(uname -s)
> > > >   host=$arch
> > > >   cross_prefix=
> > > >   endian=""
> > > > @@ -317,9 +318,9 @@ EOF
> > > >     rm -f lib-test.{o,S}
> > > >   fi
> > > > 
> > > > -# require enhanced getopt
> > > > +# require enhanced getopt everywhere except illumos
> > > >   getopt -T > /dev/null
> > > > -if [ $? -ne 4 ]; then
> > > > +if [ $? -ne 4 ] && [ "$os" != "SunOS" ]; then
> > > 
> > > What does illumos return for `getopt -T`?
> > 
> > Sadly, it returns "0".  I was wrong in my earlier explorations
> > because I did not realize that `configure` does not use `getopt`
> > aside from that one check, which is repeated in `run_tests.sh`.
> > 
> > I would argue that the most straight-forward way to deal with
> > this is to just remove the check for "getopt" from "configure",
> > which doesn't otherwise use "getopt".  The only place it is
> > used is in `run_tests.sh`, which is unlikely to be used directly
> > for illumos, and repeats the check anyway.
> 
> Fine for me if we remove the check from configure, or turn it into a warning
> instead ("Enhanced getopt is not available, you won't be able to use the
> run_tests.sh script" or so).
>

Ack for simply changing the configure error to a warning for now. Ideally
we'd limit the dependencies this project has though. So maybe rewriting
the run_tests.sh command line parser without getopt would be the better
thing to do.

Thanks,
drew

[PATCH 0/2] kvm-unit-tests: Build changes to support illumos.

[Dan Cross]

We have begun using kvm-unit-tests to test Bhyve under
illumos.  We started by cross-compiling the tests on Linux
and transfering the binary artifacts to illumos machines,
but it proved more convenient to build them directly on
illumos.

This patch series modifies the build infrastructure to allow
building on illumos; the changes were minimal.  I have also
tested these changes on Linux to ensure no regressions.

Dan Cross (2):
  kvm-unit-tests: invoke $LD explicitly in
  kvm-unit-tests: configure changes for illumos.

 configure           | 5 +++--
 x86/Makefile.common | 6 +++---
 2 files changed, 6 insertions(+), 5 deletions(-)

-- 
2.31.1

[PATCH 1/2] kvm-unit-tests: invoke $LD explicitly in

[Dan Cross]

Change x86/Makefile.common to invoke the linker directly instead
of using the C compiler as a linker driver.

This supports building on illumos, allowing the user to use
gold instead of the Solaris linker.  Tested on Linux and illumos.

Signed-off-by: Dan Cross <cross@oxidecomputer.com>
---
 x86/Makefile.common | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/x86/Makefile.common b/x86/Makefile.common
index b903988..0a0f7b9 100644
--- a/x86/Makefile.common
+++ b/x86/Makefile.common
@@ -62,7 +62,7 @@ else
 .PRECIOUS: %.elf %.o
 
 %.elf: %.o $(FLATLIBS) $(SRCDIR)/x86/flat.lds $(cstart.o)
-	$(CC) $(CFLAGS) -nostdlib -o $@ -Wl,-T,$(SRCDIR)/x86/flat.lds \
+	$(LD) -T $(SRCDIR)/x86/flat.lds -nostdlib -o $@ \
 		$(filter %.o, $^) $(FLATLIBS)
 	@chmod a-x $@
 
@@ -98,8 +98,8 @@ test_cases: $(tests-common) $(tests)
 $(TEST_DIR)/%.o: CFLAGS += -std=gnu99 -ffreestanding -I $(SRCDIR)/lib -I $(SRCDIR)/lib/x86 -I lib
 
 $(TEST_DIR)/realmode.elf: $(TEST_DIR)/realmode.o
-	$(CC) -m32 -nostdlib -o $@ -Wl,-m,elf_i386 \
-	      -Wl,-T,$(SRCDIR)/$(TEST_DIR)/realmode.lds $^
+	$(LD) -m elf_i386 -nostdlib -o $@ \
+	      -T $(SRCDIR)/$(TEST_DIR)/realmode.lds $^
 
 $(TEST_DIR)/realmode.o: bits = $(if $(call cc-option,-m16,""),16,32)
 
-- 
2.31.2

Re: [PATCH 1/2] kvm-unit-tests: invoke $LD explicitly in

[Sean Christopherson]

On Thu, May 26, 2022, Dan Cross wrote:
> Change x86/Makefile.common to invoke the linker directly instead
> of using the C compiler as a linker driver.
> 
> This supports building on illumos, allowing the user to use
> gold instead of the Solaris linker.  Tested on Linux and illumos.
> 
> Signed-off-by: Dan Cross <cross@oxidecomputer.com>
> ---
>  x86/Makefile.common | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/x86/Makefile.common b/x86/Makefile.common
> index b903988..0a0f7b9 100644
> --- a/x86/Makefile.common
> +++ b/x86/Makefile.common
> @@ -62,7 +62,7 @@ else
>  .PRECIOUS: %.elf %.o
>  
>  %.elf: %.o $(FLATLIBS) $(SRCDIR)/x86/flat.lds $(cstart.o)
> -	$(CC) $(CFLAGS) -nostdlib -o $@ -Wl,-T,$(SRCDIR)/x86/flat.lds \
> +	$(LD) -T $(SRCDIR)/x86/flat.lds -nostdlib -o $@ \
>  		$(filter %.o, $^) $(FLATLIBS)
>  	@chmod a-x $@
>  
> @@ -98,8 +98,8 @@ test_cases: $(tests-common) $(tests)
>  $(TEST_DIR)/%.o: CFLAGS += -std=gnu99 -ffreestanding -I $(SRCDIR)/lib -I $(SRCDIR)/lib/x86 -I lib
>  
>  $(TEST_DIR)/realmode.elf: $(TEST_DIR)/realmode.o
> -	$(CC) -m32 -nostdlib -o $@ -Wl,-m,elf_i386 \
> -	      -Wl,-T,$(SRCDIR)/$(TEST_DIR)/realmode.lds $^
> +	$(LD) -m elf_i386 -nostdlib -o $@ \
> +	      -T $(SRCDIR)/$(TEST_DIR)/realmode.lds $^

AFAICT, this is functionally a nop, and both 32-bit and 64-bit are happy, so,

Reviewed-and-tested-by: Sean Christopherson <seanjc@google.com>

Re: [PATCH 1/2] kvm-unit-tests: invoke $LD explicitly in

[Thomas Huth]

On 26/05/2022 19.39, Dan Cross wrote:
> Change x86/Makefile.common to invoke the linker directly instead
> of using the C compiler as a linker driver.
> 
> This supports building on illumos, allowing the user to use
> gold instead of the Solaris linker.  Tested on Linux and illumos.
> 
> Signed-off-by: Dan Cross <cross@oxidecomputer.com>
> ---
>   x86/Makefile.common | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/x86/Makefile.common b/x86/Makefile.common
> index b903988..0a0f7b9 100644
> --- a/x86/Makefile.common
> +++ b/x86/Makefile.common
> @@ -62,7 +62,7 @@ else
>   .PRECIOUS: %.elf %.o
>   
>   %.elf: %.o $(FLATLIBS) $(SRCDIR)/x86/flat.lds $(cstart.o)
> -	$(CC) $(CFLAGS) -nostdlib -o $@ -Wl,-T,$(SRCDIR)/x86/flat.lds \
> +	$(LD) -T $(SRCDIR)/x86/flat.lds -nostdlib -o $@ \
>   		$(filter %.o, $^) $(FLATLIBS)
>   	@chmod a-x $@


  Hi,

something seems to be missing here - this is failing our 32-bit
CI job:

  https://gitlab.com/thuth/kvm-unit-tests/-/jobs/2531237708

ld -T /builds/thuth/kvm-unit-tests/x86/flat.lds -nostdlib -o x86/taskswitch2.elf \
	x86/taskswitch2.o x86/cstart.o lib/libcflat.a
ld: i386 architecture of input file `x86/taskswitch.o' is incompatible with i386:x86-64 output
ld: i386 architecture of input file `x86/cstart.o' is incompatible with i386:x86-64 output
ld: i386 architecture of input file `lib/libcflat.a(argv.o)' is incompatible with i386:x86-64 output
ld: i386 architecture of input file `lib/libcflat.a(printf.o)' is incompatible with i386:x86-64 output
...

You can find the job definition in the .gitlab-ci.yml file (it's
basically just about running "configure" with --arch=i386).

  Thomas

Re: [PATCH 1/2] kvm-unit-tests: invoke $LD explicitly in

[Dan Cross]

On Wed, Jun 1, 2022 at 3:03 AM Thomas Huth <thuth@redhat.com> wrote:
> On 26/05/2022 19.39, Dan Cross wrote:
> > Change x86/Makefile.common to invoke the linker directly instead
> > of using the C compiler as a linker driver.
> >
> > This supports building on illumos, allowing the user to use
> > gold instead of the Solaris linker.  Tested on Linux and illumos.
> >
> > Signed-off-by: Dan Cross <cross@oxidecomputer.com>
> > ---
> >   x86/Makefile.common | 6 +++---
> >   1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/x86/Makefile.common b/x86/Makefile.common
> > index b903988..0a0f7b9 100644
> > --- a/x86/Makefile.common
> > +++ b/x86/Makefile.common
> > @@ -62,7 +62,7 @@ else
> >   .PRECIOUS: %.elf %.o
> >
> >   %.elf: %.o $(FLATLIBS) $(SRCDIR)/x86/flat.lds $(cstart.o)
> > -     $(CC) $(CFLAGS) -nostdlib -o $@ -Wl,-T,$(SRCDIR)/x86/flat.lds \
> > +     $(LD) -T $(SRCDIR)/x86/flat.lds -nostdlib -o $@ \
> >               $(filter %.o, $^) $(FLATLIBS)
> >       @chmod a-x $@
>
>
>   Hi,
>
> something seems to be missing here - this is failing our 32-bit
> CI job:
>
>   https://gitlab.com/thuth/kvm-unit-tests/-/jobs/2531237708
>
> ld -T /builds/thuth/kvm-unit-tests/x86/flat.lds -nostdlib -o x86/taskswitch2.elf \
>         x86/taskswitch2.o x86/cstart.o lib/libcflat.a
> ld: i386 architecture of input file `x86/taskswitch.o' is incompatible with i386:x86-64 output
> ld: i386 architecture of input file `x86/cstart.o' is incompatible with i386:x86-64 output
> ld: i386 architecture of input file `lib/libcflat.a(argv.o)' is incompatible with i386:x86-64 output
> ld: i386 architecture of input file `lib/libcflat.a(printf.o)' is incompatible with i386:x86-64 output
> ...
>
> You can find the job definition in the .gitlab-ci.yml file (it's
> basically just about running "configure" with --arch=i386).

Thanks. I think the easiest way to fix this is to plumb an
argument through to the linker that expands to `-m elf_i386`
on 32-bit, and possibly, `-m elf_x86_64` on 64-bit. The
architecture specific Makefiles set the `ldarch` variable,
and that doesn't seem used anywhere, but I see that's set
to match the string accepted by the linker scripts/objcopy,
not something acceptable to `-m`. However, one can add
something to `LDARGS`, but I see that that's set to include
the contents of CFLAGS, which means it includes flags
that are not directly consumable  by the linker itself.

I think the simplest way forward is to introduce a new
variable in x86/Makefile.i386 and x86/Makefile.x86_64 and
refer to that in x86/Makefile.common; possibly `LDEXTRA`?
I've got an updated patch here that does that, and it seems
to work (building under both illumos and arch locally), but
before I send up another patchset, let me know if that
sounds acceptable?

        - Dan C.

Re: [PATCH 1/2] kvm-unit-tests: invoke $LD explicitly in

[Andrew Jones]

On Wed, Jun 01, 2022 at 01:09:13PM -0400, Dan Cross wrote:
> On Wed, Jun 1, 2022 at 3:03 AM Thomas Huth <thuth@redhat.com> wrote:
> > On 26/05/2022 19.39, Dan Cross wrote:
> > > Change x86/Makefile.common to invoke the linker directly instead
> > > of using the C compiler as a linker driver.
> > >
> > > This supports building on illumos, allowing the user to use
> > > gold instead of the Solaris linker.  Tested on Linux and illumos.
> > >
> > > Signed-off-by: Dan Cross <cross@oxidecomputer.com>
> > > ---
> > >   x86/Makefile.common | 6 +++---
> > >   1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/x86/Makefile.common b/x86/Makefile.common
> > > index b903988..0a0f7b9 100644
> > > --- a/x86/Makefile.common
> > > +++ b/x86/Makefile.common
> > > @@ -62,7 +62,7 @@ else
> > >   .PRECIOUS: %.elf %.o
> > >
> > >   %.elf: %.o $(FLATLIBS) $(SRCDIR)/x86/flat.lds $(cstart.o)
> > > -     $(CC) $(CFLAGS) -nostdlib -o $@ -Wl,-T,$(SRCDIR)/x86/flat.lds \
> > > +     $(LD) -T $(SRCDIR)/x86/flat.lds -nostdlib -o $@ \
> > >               $(filter %.o, $^) $(FLATLIBS)
> > >       @chmod a-x $@
> >
> >
> >   Hi,
> >
> > something seems to be missing here - this is failing our 32-bit
> > CI job:
> >
> >   https://gitlab.com/thuth/kvm-unit-tests/-/jobs/2531237708
> >
> > ld -T /builds/thuth/kvm-unit-tests/x86/flat.lds -nostdlib -o x86/taskswitch2.elf \
> >         x86/taskswitch2.o x86/cstart.o lib/libcflat.a
> > ld: i386 architecture of input file `x86/taskswitch.o' is incompatible with i386:x86-64 output
> > ld: i386 architecture of input file `x86/cstart.o' is incompatible with i386:x86-64 output
> > ld: i386 architecture of input file `lib/libcflat.a(argv.o)' is incompatible with i386:x86-64 output
> > ld: i386 architecture of input file `lib/libcflat.a(printf.o)' is incompatible with i386:x86-64 output
> > ...
> >
> > You can find the job definition in the .gitlab-ci.yml file (it's
> > basically just about running "configure" with --arch=i386).
> 
> Thanks. I think the easiest way to fix this is to plumb an
> argument through to the linker that expands to `-m elf_i386`
> on 32-bit, and possibly, `-m elf_x86_64` on 64-bit. The
> architecture specific Makefiles set the `ldarch` variable,
> and that doesn't seem used anywhere, but I see that's set
> to match the string accepted by the linker scripts/objcopy,
> not something acceptable to `-m`. However, one can add
> something to `LDARGS`, but I see that that's set to include
> the contents of CFLAGS, which means it includes flags
> that are not directly consumable  by the linker itself.
> 
> I think the simplest way forward is to introduce a new
> variable in x86/Makefile.i386 and x86/Makefile.x86_64 and
> refer to that in x86/Makefile.common; possibly `LDEXTRA`?

We do that for arm, but call that argument arch_LDFLAGS.

> I've got an updated patch here that does that, and it seems
> to work (building under both illumos and arch locally), but
> before I send up another patchset, let me know if that
> sounds acceptable?

It does to me.

Thanks,
drew

Re: [PATCH 1/2] kvm-unit-tests: invoke $LD explicitly in

[Dan Cross]

On Wed, Jun 1, 2022 at 1:43 PM Andrew Jones <drjones@redhat.com> wrote:
> On Wed, Jun 01, 2022 at 01:09:13PM -0400, Dan Cross wrote:
> > Thanks. I think the easiest way to fix this is to plumb an
> > argument through to the linker that expands to `-m elf_i386`
> > on 32-bit, and possibly, `-m elf_x86_64` on 64-bit. The
> > architecture specific Makefiles set the `ldarch` variable,
> > and that doesn't seem used anywhere, but I see that's set
> > to match the string accepted by the linker scripts/objcopy,
> > not something acceptable to `-m`. However, one can add
> > something to `LDARGS`, but I see that that's set to include
> > the contents of CFLAGS, which means it includes flags
> > that are not directly consumable  by the linker itself.
> >
> > I think the simplest way forward is to introduce a new
> > variable in x86/Makefile.i386 and x86/Makefile.x86_64 and
> > refer to that in x86/Makefile.common; possibly `LDEXTRA`?
>
> We do that for arm, but call that argument arch_LDFLAGS.

Ah cool; I see the pattern now.  Okay, I've modified x86 to
follow ARM in this regard and tested locally.

> > I've got an updated patch here that does that, and it seems
> > to work (building under both illumos and arch locally), but
> > before I send up another patchset, let me know if that
> > sounds acceptable?
>
> It does to me.
>
> Thanks,
> drew

Great. Next patch rev inbound.

        - Dan C.

[PATCH v4 0/2] kvm-unit-tests: Build changes to support illumos.

[Dan Cross]

We have begun using kvm-unit-tests to test Bhyve under
illumos.  We started by cross-compiling the tests on Linux
and transfering the binary artifacts to illumos machines,
but it proved more convenient to build them directly on
illumos.

This patch series modifies the build infrastructure to allow
building on illumos; the changes were minimal.  I have also
tested these changes on Linux to ensure no regressions.

Dan Cross (2):
  kvm-unit-tests: configure changes for illumos.
  kvm-unit-tests: invoke $LD explicitly

 configure           | 6 +++---
 x86/Makefile.common | 9 +++++----
 x86/Makefile.i386   | 1 +
 x86/Makefile.x86_64 | 2 ++
 4 files changed, 11 insertions(+), 7 deletions(-)

-- 
2.36.1

[PATCH v4 1/2] kvm-unit-tests: configure changes for illumos.

[Dan Cross]

Warn, don't fail, if the check for `getopt -T` fails in the `configure`
script.

Aside from this check, `configure` does not use `getopt`, so don't
fail to run if `getopt -T` doesn't indicate support for  the extended
Linux version.  Getopt is only used in `run_tests.sh`, which tests for
extended getopt anyway, but emit a warning here.

Signed-off-by: Dan Cross <cross@oxidecomputer.com>
---
 configure | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/configure b/configure
index 23085da..5b7daac 100755
--- a/configure
+++ b/configure
@@ -318,11 +318,11 @@ EOF
   rm -f lib-test.{o,S}
 fi
 
-# require enhanced getopt
+# warn if enhanced getopt is unavailable
 getopt -T > /dev/null
 if [ $? -ne 4 ]; then
-    echo "Enhanced getopt is not available, add it to your PATH?"
-    exit 1
+    echo "Without enhanced getopt you won't be able to use run_tests.sh."
+    echo "Add it to your PATH?"
 fi
 
 # Are we in a separate build tree? If so, link the Makefile
-- 
2.36.1

[PATCH v4 2/2] kvm-unit-tests: invoke $LD explicitly

[Dan Cross]

Change x86/Makefile.common to invoke the linker directly instead
of using the C compiler as a linker driver.  Plumb LDFLAGS through
to the linker.

This supports building on illumos, allowing the user to use
gold instead of the Solaris linker.  Tested on Linux and illumos.

Signed-off-by: Dan Cross <cross@oxidecomputer.com>
---
 x86/Makefile.common | 9 +++++----
 x86/Makefile.i386   | 1 +
 x86/Makefile.x86_64 | 2 ++
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/x86/Makefile.common b/x86/Makefile.common
index b903988..a600c72 100644
--- a/x86/Makefile.common
+++ b/x86/Makefile.common
@@ -30,7 +30,7 @@ endif
 
 OBJDIRS += lib/x86
 
-$(libcflat): LDFLAGS += -nostdlib
+$(libcflat): LDFLAGS += -nostdlib $(arch_LDFLAGS)
 $(libcflat): CFLAGS += -ffreestanding -I $(SRCDIR)/lib -I lib
 
 COMMON_CFLAGS += -m$(bits)
@@ -61,8 +61,9 @@ else
 # We want to keep intermediate file: %.elf and %.o
 .PRECIOUS: %.elf %.o
 
+%.elf: LDFLAGS = -nostdlib $(arch_LDFLAGS)
 %.elf: %.o $(FLATLIBS) $(SRCDIR)/x86/flat.lds $(cstart.o)
-	$(CC) $(CFLAGS) -nostdlib -o $@ -Wl,-T,$(SRCDIR)/x86/flat.lds \
+	$(LD) $(LDFLAGS) -T $(SRCDIR)/x86/flat.lds -o $@ \
 		$(filter %.o, $^) $(FLATLIBS)
 	@chmod a-x $@
 
@@ -98,8 +99,8 @@ test_cases: $(tests-common) $(tests)
 $(TEST_DIR)/%.o: CFLAGS += -std=gnu99 -ffreestanding -I $(SRCDIR)/lib -I $(SRCDIR)/lib/x86 -I lib
 
 $(TEST_DIR)/realmode.elf: $(TEST_DIR)/realmode.o
-	$(CC) -m32 -nostdlib -o $@ -Wl,-m,elf_i386 \
-	      -Wl,-T,$(SRCDIR)/$(TEST_DIR)/realmode.lds $^
+	$(LD) -m elf_i386 -nostdlib -o $@ \
+	      -T $(SRCDIR)/$(TEST_DIR)/realmode.lds $^
 
 $(TEST_DIR)/realmode.o: bits = $(if $(call cc-option,-m16,""),16,32)
 
diff --git a/x86/Makefile.i386 b/x86/Makefile.i386
index 7d19d55..0a845e6 100644
--- a/x86/Makefile.i386
+++ b/x86/Makefile.i386
@@ -4,6 +4,7 @@ ldarch = elf32-i386
 exe = flat
 bin = elf
 COMMON_CFLAGS += -mno-sse -mno-sse2
+arch_LDFLAGS = -m elf_i386
 
 cflatobjs += lib/x86/setjmp32.o lib/ldiv32.o
 
diff --git a/x86/Makefile.x86_64 b/x86/Makefile.x86_64
index f18c1e2..e19284a 100644
--- a/x86/Makefile.x86_64
+++ b/x86/Makefile.x86_64
@@ -6,9 +6,11 @@ exe = efi
 bin = so
 FORMAT = efi-app-x86_64
 cstart.o = $(TEST_DIR)/efi/efistart64.o
+arch_LDFLAGS = ''
 else
 exe = flat
 bin = elf
+arch_LDFLAGS = -m elf_x86_64
 endif
 
 fcf_protection_full := $(call cc-option, -fcf-protection=full,)
-- 
2.36.1

Re: [PATCH v4 0/2] kvm-unit-tests: Build changes to support illumos.

[Thomas Huth]

On 01/06/2022 23.57, Dan Cross wrote:
> We have begun using kvm-unit-tests to test Bhyve under
> illumos.  We started by cross-compiling the tests on Linux
> and transfering the binary artifacts to illumos machines,
> but it proved more convenient to build them directly on
> illumos.
> 
> This patch series modifies the build infrastructure to allow
> building on illumos; the changes were minimal.  I have also
> tested these changes on Linux to ensure no regressions.

Thanks, this version survived the CI, so I've applied it now.

  Thomas

Re: [PATCH v4 0/2] kvm-unit-tests: Build changes to support illumos.

[Dan Cross]

On Thu, Jun 2, 2022 at 6:53 AM Thomas Huth <thuth@redhat.com> wrote:
> On 01/06/2022 23.57, Dan Cross wrote:
> > We have begun using kvm-unit-tests to test Bhyve under
> > illumos.  We started by cross-compiling the tests on Linux
> > and transfering the binary artifacts to illumos machines,
> > but it proved more convenient to build them directly on
> > illumos.
> >
> > This patch series modifies the build infrastructure to allow
> > building on illumos; the changes were minimal.  I have also
> > tested these changes on Linux to ensure no regressions.
>
> Thanks, this version survived the CI, so I've applied it now.

Thank you very much!

        - Dan C.

[PATCH 2/2] kvm-unit-tests: configure changes for illumos.

[Dan Cross]

Warn, don't fail, if the check for `getopt -T` fails in the `configure`
script.

Aside from this check, `configure` does not use `getopt`, so don't
fail to run if `getopt -T` doesn't indicate support for  the extended
Linux version.  Getopt is only used in `run_tests.sh`, which tests for
extended getopt anyway, but emit a warning here.

Signed-off-by: Dan Cross <cross@oxidecomputer.com>
---
 configure | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/configure b/configure
index 23085da..5b7daac 100755
--- a/configure
+++ b/configure
@@ -318,11 +318,11 @@ EOF
   rm -f lib-test.{o,S}
 fi
 
-# require enhanced getopt
+# warn if enhanced getopt is unavailable
 getopt -T > /dev/null
 if [ $? -ne 4 ]; then
-    echo "Enhanced getopt is not available, add it to your PATH?"
-    exit 1
+    echo "Without enhanced getopt you won't be able to use run_tests.sh."
+    echo "Add it to your PATH?"
 fi
 
 # Are we in a separate build tree? If so, link the Makefile
-- 
2.31.2

Re: [PATCH 2/2] kvm-unit-tests: configure changes for illumos.

[Sean Christopherson]

In the future, please tag each new posting with a version number, e.g. no version
number for the first posting, than v2, v3, v4, etc... for each subsequent posting.
It took me a while to figure what was what.

On Thu, May 26, 2022, Dan Cross wrote:
> Warn, don't fail, if the check for `getopt -T` fails in the `configure`
> script.
> 
> Aside from this check, `configure` does not use `getopt`, so don't
> fail to run if `getopt -T` doesn't indicate support for  the extended
> Linux version.  Getopt is only used in `run_tests.sh`, which tests for
> extended getopt anyway, but emit a warning here.

Why not simply move the check to run_tests.sh?  I can't imaging it's performance
sensitive, and I doubt I'm the only one that builds tests on one system and runs
them on a completely different system.

Re: [PATCH 2/2] kvm-unit-tests: configure changes for illumos.

[Dan Cross]

On Thu, May 26, 2022 at 5:23 PM Sean Christopherson <seanjc@google.com> wrote:
> In the future, please tag each new posting with a version number, e.g. no version
> number for the first posting, than v2, v3, v4, etc... for each subsequent posting.
> It took me a while to figure what was what.

Ah, sorry about that. Sure thing.

> On Thu, May 26, 2022, Dan Cross wrote:
> > Warn, don't fail, if the check for `getopt -T` fails in the `configure`
> > script.
> >
> > Aside from this check, `configure` does not use `getopt`, so don't
> > fail to run if `getopt -T` doesn't indicate support for  the extended
> > Linux version.  Getopt is only used in `run_tests.sh`, which tests for
> > extended getopt anyway, but emit a warning here.
>
> Why not simply move the check to run_tests.sh?  I can't imaging it's performance
> sensitive, and I doubt I'm the only one that builds tests on one system and runs
> them on a completely different system.

`run_tests.sh` already has the test. Changing it to a warning here
was at the suggestion of Thomas and Drew.

        - Dan C.

Re: [PATCH 2/2] kvm-unit-tests: configure changes for illumos.

[Sean Christopherson]

On Thu, May 26, 2022, Dan Cross wrote:
> On Thu, May 26, 2022 at 5:23 PM Sean Christopherson <seanjc@google.com> wrote:
> > Why not simply move the check to run_tests.sh?  I can't imaging it's performance
> > sensitive, and I doubt I'm the only one that builds tests on one system and runs
> > them on a completely different system.
> 
> `run_tests.sh` already has the test. Changing it to a warning here
> was at the suggestion of Thomas and Drew.

Ah, hence the earlier comment about removing the check entirely.  Either works
for me, thanks!

Re: [PATCH 2/2] kvm-unit-tests: configure changes for illumos.

[Dan Cross]

On Fri, May 27, 2022 at 10:41 AM Sean Christopherson <seanjc@google.com> wrote:
> On Thu, May 26, 2022, Dan Cross wrote:
> > On Thu, May 26, 2022 at 5:23 PM Sean Christopherson <seanjc@google.com> wrote:
> > > Why not simply move the check to run_tests.sh?  I can't imaging it's performance
> > > sensitive, and I doubt I'm the only one that builds tests on one system and runs
> > > them on a completely different system.
> >
> > `run_tests.sh` already has the test. Changing it to a warning here
> > was at the suggestion of Thomas and Drew.
>
> Ah, hence the earlier comment about removing the check entirely.  Either works
> for me, thanks!

Thanks for the helpful comments on the review!

        - Dan C.

Re: [PATCH 2/2] kvm-unit-tests: configure changes for illumos.

[Dan Cross]

On Thu, May 26, 2022 at 3:12 AM Andrew Jones <drjones@redhat.com> wrote:
> On Wed, May 25, 2022 at 09:44:33AM +0200, Thomas Huth wrote:
> > [snip]
> > Fine for me if we remove the check from configure, or turn it into a warning
> > instead ("Enhanced getopt is not available, you won't be able to use the
> > run_tests.sh script" or so).
>
> Ack for simply changing the configure error to a warning for now. Ideally
> we'd limit the dependencies this project has though. So maybe rewriting
> the run_tests.sh command line parser without getopt would be the better
> thing to do.

Sounds good. New patches inbound. Thank you!

        - Dan C.

Fwd: [PATCH] kvm-unit-tests: Build changes for illumos.

[Dan Cross]

[Sean, sorry for the dup; I forgot to switch to plaintext mode]

On Thu, May 12, 2022 at 6:05 PM Sean Christopherson <seanjc@google.com> wrote:
> On Thu, May 12, 2022, Dan Cross wrote:
> > We have begun using kvm-unit-tests to test Bhyve under
> > illumos.  We started by cross-compiling the tests on Linux
> > and transfering the binary artifacts to illumos machines,
> > but it proved more convenient to build them directly on
> > illumos.
> >
> > This change modifies the build infrastructure to allow
> > building on illumos; I have also tested it on Linux.  The
> > required changes were pretty minimal: the most invasive
> > was switching from using the C compiler as a linker driver
> > to simply invoking the linker directly in two places.
> > This allows us to easily use gold instead of the Solaris
> > linker.
>
> Can you please split this into two patches?  One for the $(CC) => $(LD) change,
> and one for the getopt thing.  The switch to $(LD) in particular could be valuable
> irrespective of using a non-Linux OS.

Done.

> [snip]
>
> > +# require enhanced getopt everywhere except illumos
> >  getopt -T > /dev/null
> > -if [ $? -ne 4 ]; then
> > +if [ $? -ne 4 ] && [ "$os" != "SunOS" ]; then
>
> Presumably whatever "enhanced" features are being used are supported by illumos,
> so rather than check the OS, why not improve the probing?

Actually, it looks like I've got some egg on my face here. It appears
that `getopt`
is not used in `configure` beyond this check; presumably this exists
to avoid surprises
when running `run_tests.sh`, which does use extended getopt (for long
options) and
repeats this same test. I had not noticed since `configure` ran
successfully, and we are
not presently using `run_tests.sh` on illumos as we use a rather
different VMM than
qemu. That patch should probably be dropped (or changed to remove the
getopt check
in `configure` entirely, if folks are fine deferring needing to have
it to the invocation of
`run_tests.sh`...). Or I just convince folks to provide a GNU getopt
package on illumos.
Let me know what is preferred.

        - Dan C.