All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] kvm-unit-tests: Build changes for illumos.
@ 2022-05-12 20:45 Dan Cross
  2022-05-12 22:05 ` Sean Christopherson
  0 siblings, 1 reply; 31+ messages in thread
From: Dan Cross @ 2022-05-12 20:45 UTC (permalink / raw)
  To: kvm; +Cc: 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


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* Re: [PATCH] kvm-unit-tests: Build changes for illumos.
  2022-05-12 20:45 [PATCH] kvm-unit-tests: Build changes for illumos Dan Cross
@ 2022-05-12 22:05 ` Sean Christopherson
  2022-05-13  1:07   ` [PATCH 0/2] kvm-unit-tests: Build changes to support illumos Dan Cross
       [not found]   ` <CAA9fzEGdi0k8bkyXQwvt6gFd-gwHNNFF7A89U4DhtGHjKqe4AQ@mail.gmail.com>
  0 siblings, 2 replies; 31+ messages in thread
From: Sean Christopherson @ 2022-05-12 22:05 UTC (permalink / raw)
  To: Dan Cross; +Cc: kvm

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH 0/2] kvm-unit-tests: Build changes to support illumos.
  2022-05-12 22:05 ` Sean Christopherson
@ 2022-05-13  1:07   ` Dan Cross
  2022-05-13  1:07     ` [PATCH 1/2] kvm-unit-tests: invoke $LD explicitly in Dan Cross
  2022-05-13  1:07     ` [PATCH 2/2] kvm-unit-tests: configure changes for illumos Dan Cross
       [not found]   ` <CAA9fzEGdi0k8bkyXQwvt6gFd-gwHNNFF7A89U4DhtGHjKqe4AQ@mail.gmail.com>
  1 sibling, 2 replies; 31+ messages in thread
From: Dan Cross @ 2022-05-13  1:07 UTC (permalink / raw)
  To: kvm; +Cc: Sean Christopherson, 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


^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH 1/2] kvm-unit-tests: invoke $LD explicitly in
  2022-05-13  1:07   ` [PATCH 0/2] kvm-unit-tests: Build changes to support illumos Dan Cross
@ 2022-05-13  1:07     ` Dan Cross
  2022-05-13  1:07     ` [PATCH 2/2] kvm-unit-tests: configure changes for illumos Dan Cross
  1 sibling, 0 replies; 31+ messages in thread
From: Dan Cross @ 2022-05-13  1:07 UTC (permalink / raw)
  To: kvm; +Cc: Sean Christopherson, 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


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 2/2] kvm-unit-tests: configure changes for illumos.
  2022-05-13  1:07   ` [PATCH 0/2] kvm-unit-tests: Build changes to support illumos Dan Cross
  2022-05-13  1:07     ` [PATCH 1/2] kvm-unit-tests: invoke $LD explicitly in Dan Cross
@ 2022-05-13  1:07     ` Dan Cross
  2022-05-13 14:34       ` Sean Christopherson
  1 sibling, 1 reply; 31+ messages in thread
From: Dan Cross @ 2022-05-13  1:07 UTC (permalink / raw)
  To: kvm; +Cc: Sean Christopherson, 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


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* Fwd: [PATCH] kvm-unit-tests: Build changes for illumos.
       [not found]   ` <CAA9fzEGdi0k8bkyXQwvt6gFd-gwHNNFF7A89U4DhtGHjKqe4AQ@mail.gmail.com>
@ 2022-05-13  1:27     ` Dan Cross
  0 siblings, 0 replies; 31+ messages in thread
From: Dan Cross @ 2022-05-13  1:27 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: kvm

[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.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 2/2] kvm-unit-tests: configure changes for illumos.
  2022-05-13  1:07     ` [PATCH 2/2] kvm-unit-tests: configure changes for illumos Dan Cross
@ 2022-05-13 14:34       ` Sean Christopherson
  2022-05-19  9:53         ` Thomas Huth
  2022-05-24 21:22         ` Dan Cross
  0 siblings, 2 replies; 31+ messages in thread
From: Sean Christopherson @ 2022-05-13 14:34 UTC (permalink / raw)
  To: Dan Cross; +Cc: kvm, Paolo Bonzini, Thomas Huth, Andrew Jones

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
> 

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 2/2] kvm-unit-tests: configure changes for illumos.
  2022-05-13 14:34       ` Sean Christopherson
@ 2022-05-19  9:53         ` Thomas Huth
  2022-05-24 21:20           ` Dan Cross
  2022-05-24 21:22         ` Dan Cross
  1 sibling, 1 reply; 31+ messages in thread
From: Thomas Huth @ 2022-05-19  9:53 UTC (permalink / raw)
  To: Sean Christopherson, Dan Cross; +Cc: kvm, Paolo Bonzini, Andrew Jones

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


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 2/2] kvm-unit-tests: configure changes for illumos.
  2022-05-19  9:53         ` Thomas Huth
@ 2022-05-24 21:20           ` Dan Cross
  2022-05-25  7:41             ` Thomas Huth
  0 siblings, 1 reply; 31+ messages in thread
From: Dan Cross @ 2022-05-24 21:20 UTC (permalink / raw)
  To: Thomas Huth; +Cc: Sean Christopherson, kvm, Paolo Bonzini, Andrew Jones

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!

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 2/2] kvm-unit-tests: configure changes for illumos.
  2022-05-13 14:34       ` Sean Christopherson
  2022-05-19  9:53         ` Thomas Huth
@ 2022-05-24 21:22         ` Dan Cross
  2022-05-25  7:44           ` Thomas Huth
  1 sibling, 1 reply; 31+ messages in thread
From: Dan Cross @ 2022-05-24 21:22 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: kvm, Paolo Bonzini, Thomas Huth, Andrew Jones

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.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 2/2] kvm-unit-tests: configure changes for illumos.
  2022-05-24 21:20           ` Dan Cross
@ 2022-05-25  7:41             ` Thomas Huth
  0 siblings, 0 replies; 31+ messages in thread
From: Thomas Huth @ 2022-05-25  7:41 UTC (permalink / raw)
  To: Dan Cross; +Cc: Sean Christopherson, kvm, Paolo Bonzini, Andrew Jones

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



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 2/2] kvm-unit-tests: configure changes for illumos.
  2022-05-24 21:22         ` Dan Cross
@ 2022-05-25  7:44           ` Thomas Huth
  2022-05-26  7:11             ` Andrew Jones
  0 siblings, 1 reply; 31+ messages in thread
From: Thomas Huth @ 2022-05-25  7:44 UTC (permalink / raw)
  To: Dan Cross, Sean Christopherson; +Cc: kvm, Paolo Bonzini, Andrew Jones

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


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 2/2] kvm-unit-tests: configure changes for illumos.
  2022-05-25  7:44           ` Thomas Huth
@ 2022-05-26  7:11             ` Andrew Jones
  2022-05-26 17:39               ` [PATCH 0/2] kvm-unit-tests: Build changes to support illumos Dan Cross
  2022-05-26 17:40               ` Dan Cross
  0 siblings, 2 replies; 31+ messages in thread
From: Andrew Jones @ 2022-05-26  7:11 UTC (permalink / raw)
  To: Thomas Huth; +Cc: Dan Cross, Sean Christopherson, kvm, Paolo Bonzini

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


^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH 0/2] kvm-unit-tests: Build changes to support illumos.
  2022-05-26  7:11             ` Andrew Jones
@ 2022-05-26 17:39               ` Dan Cross
  2022-05-26 17:39                 ` [PATCH 1/2] kvm-unit-tests: invoke $LD explicitly in Dan Cross
  2022-05-26 17:39                 ` [PATCH 2/2] kvm-unit-tests: configure changes for illumos Dan Cross
  2022-05-26 17:40               ` Dan Cross
  1 sibling, 2 replies; 31+ messages in thread
From: Dan Cross @ 2022-05-26 17:39 UTC (permalink / raw)
  To: kvm
  Cc: Sean Christopherson, Andrew Jones, Thomas Huth, Paolo Bonzini, 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


^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH 1/2] kvm-unit-tests: invoke $LD explicitly in
  2022-05-26 17:39               ` [PATCH 0/2] kvm-unit-tests: Build changes to support illumos Dan Cross
@ 2022-05-26 17:39                 ` Dan Cross
  2022-05-26 21:17                   ` Sean Christopherson
  2022-06-01  7:03                   ` Thomas Huth
  2022-05-26 17:39                 ` [PATCH 2/2] kvm-unit-tests: configure changes for illumos Dan Cross
  1 sibling, 2 replies; 31+ messages in thread
From: Dan Cross @ 2022-05-26 17:39 UTC (permalink / raw)
  To: kvm
  Cc: Sean Christopherson, Andrew Jones, Thomas Huth, Paolo Bonzini, 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


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 2/2] kvm-unit-tests: configure changes for illumos.
  2022-05-26 17:39               ` [PATCH 0/2] kvm-unit-tests: Build changes to support illumos Dan Cross
  2022-05-26 17:39                 ` [PATCH 1/2] kvm-unit-tests: invoke $LD explicitly in Dan Cross
@ 2022-05-26 17:39                 ` Dan Cross
  2022-05-26 21:23                   ` Sean Christopherson
  1 sibling, 1 reply; 31+ messages in thread
From: Dan Cross @ 2022-05-26 17:39 UTC (permalink / raw)
  To: kvm
  Cc: Sean Christopherson, Andrew Jones, Thomas Huth, Paolo Bonzini, 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


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* Re: [PATCH 2/2] kvm-unit-tests: configure changes for illumos.
  2022-05-26  7:11             ` Andrew Jones
  2022-05-26 17:39               ` [PATCH 0/2] kvm-unit-tests: Build changes to support illumos Dan Cross
@ 2022-05-26 17:40               ` Dan Cross
  1 sibling, 0 replies; 31+ messages in thread
From: Dan Cross @ 2022-05-26 17:40 UTC (permalink / raw)
  To: Andrew Jones; +Cc: Thomas Huth, Sean Christopherson, kvm, Paolo Bonzini

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.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 1/2] kvm-unit-tests: invoke $LD explicitly in
  2022-05-26 17:39                 ` [PATCH 1/2] kvm-unit-tests: invoke $LD explicitly in Dan Cross
@ 2022-05-26 21:17                   ` Sean Christopherson
  2022-06-01  7:03                   ` Thomas Huth
  1 sibling, 0 replies; 31+ messages in thread
From: Sean Christopherson @ 2022-05-26 21:17 UTC (permalink / raw)
  To: Dan Cross; +Cc: kvm, Andrew Jones, Thomas Huth, Paolo Bonzini

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>

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 2/2] kvm-unit-tests: configure changes for illumos.
  2022-05-26 17:39                 ` [PATCH 2/2] kvm-unit-tests: configure changes for illumos Dan Cross
@ 2022-05-26 21:23                   ` Sean Christopherson
  2022-05-26 22:17                     ` Dan Cross
  0 siblings, 1 reply; 31+ messages in thread
From: Sean Christopherson @ 2022-05-26 21:23 UTC (permalink / raw)
  To: Dan Cross; +Cc: kvm, Andrew Jones, Thomas Huth, Paolo Bonzini

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.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 2/2] kvm-unit-tests: configure changes for illumos.
  2022-05-26 21:23                   ` Sean Christopherson
@ 2022-05-26 22:17                     ` Dan Cross
  2022-05-27 14:41                       ` Sean Christopherson
  0 siblings, 1 reply; 31+ messages in thread
From: Dan Cross @ 2022-05-26 22:17 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: kvm, Andrew Jones, Thomas Huth, Paolo Bonzini

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.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 2/2] kvm-unit-tests: configure changes for illumos.
  2022-05-26 22:17                     ` Dan Cross
@ 2022-05-27 14:41                       ` Sean Christopherson
  2022-05-31 17:02                         ` Dan Cross
  0 siblings, 1 reply; 31+ messages in thread
From: Sean Christopherson @ 2022-05-27 14:41 UTC (permalink / raw)
  To: Dan Cross; +Cc: kvm, Andrew Jones, Thomas Huth, Paolo Bonzini

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!

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 2/2] kvm-unit-tests: configure changes for illumos.
  2022-05-27 14:41                       ` Sean Christopherson
@ 2022-05-31 17:02                         ` Dan Cross
  0 siblings, 0 replies; 31+ messages in thread
From: Dan Cross @ 2022-05-31 17:02 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: kvm, Andrew Jones, Thomas Huth, Paolo Bonzini

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.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 1/2] kvm-unit-tests: invoke $LD explicitly in
  2022-05-26 17:39                 ` [PATCH 1/2] kvm-unit-tests: invoke $LD explicitly in Dan Cross
  2022-05-26 21:17                   ` Sean Christopherson
@ 2022-06-01  7:03                   ` Thomas Huth
  2022-06-01 17:09                     ` Dan Cross
  1 sibling, 1 reply; 31+ messages in thread
From: Thomas Huth @ 2022-06-01  7:03 UTC (permalink / raw)
  To: Dan Cross, kvm; +Cc: Sean Christopherson, Andrew Jones, Paolo Bonzini

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


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 1/2] kvm-unit-tests: invoke $LD explicitly in
  2022-06-01  7:03                   ` Thomas Huth
@ 2022-06-01 17:09                     ` Dan Cross
  2022-06-01 17:43                       ` Andrew Jones
  0 siblings, 1 reply; 31+ messages in thread
From: Dan Cross @ 2022-06-01 17:09 UTC (permalink / raw)
  To: Thomas Huth; +Cc: kvm, Sean Christopherson, Andrew Jones, Paolo Bonzini

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.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 1/2] kvm-unit-tests: invoke $LD explicitly in
  2022-06-01 17:09                     ` Dan Cross
@ 2022-06-01 17:43                       ` Andrew Jones
  2022-06-01 21:51                         ` Dan Cross
  0 siblings, 1 reply; 31+ messages in thread
From: Andrew Jones @ 2022-06-01 17:43 UTC (permalink / raw)
  To: Dan Cross; +Cc: Thomas Huth, kvm, Sean Christopherson, Paolo Bonzini

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


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 1/2] kvm-unit-tests: invoke $LD explicitly in
  2022-06-01 17:43                       ` Andrew Jones
@ 2022-06-01 21:51                         ` Dan Cross
  2022-06-01 21:57                           ` [PATCH v4 0/2] kvm-unit-tests: Build changes to support illumos Dan Cross
  0 siblings, 1 reply; 31+ messages in thread
From: Dan Cross @ 2022-06-01 21:51 UTC (permalink / raw)
  To: Andrew Jones; +Cc: Thomas Huth, kvm, Sean Christopherson, Paolo Bonzini

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.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH v4 0/2] kvm-unit-tests: Build changes to support illumos.
  2022-06-01 21:51                         ` Dan Cross
@ 2022-06-01 21:57                           ` Dan Cross
  2022-06-01 21:57                             ` [PATCH v4 1/2] kvm-unit-tests: configure changes for illumos Dan Cross
                                               ` (2 more replies)
  0 siblings, 3 replies; 31+ messages in thread
From: Dan Cross @ 2022-06-01 21:57 UTC (permalink / raw)
  To: kvm
  Cc: Andrew Jones, Thomas Huth, Sean Christopherson, Paolo Bonzini, 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


^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH v4 1/2] kvm-unit-tests: configure changes for illumos.
  2022-06-01 21:57                           ` [PATCH v4 0/2] kvm-unit-tests: Build changes to support illumos Dan Cross
@ 2022-06-01 21:57                             ` Dan Cross
  2022-06-01 21:57                             ` [PATCH v4 2/2] kvm-unit-tests: invoke $LD explicitly Dan Cross
  2022-06-02 10:53                             ` [PATCH v4 0/2] kvm-unit-tests: Build changes to support illumos Thomas Huth
  2 siblings, 0 replies; 31+ messages in thread
From: Dan Cross @ 2022-06-01 21:57 UTC (permalink / raw)
  To: kvm
  Cc: Andrew Jones, Thomas Huth, Sean Christopherson, Paolo Bonzini, 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


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v4 2/2] kvm-unit-tests: invoke $LD explicitly
  2022-06-01 21:57                           ` [PATCH v4 0/2] kvm-unit-tests: Build changes to support illumos Dan Cross
  2022-06-01 21:57                             ` [PATCH v4 1/2] kvm-unit-tests: configure changes for illumos Dan Cross
@ 2022-06-01 21:57                             ` Dan Cross
  2022-06-02 10:53                             ` [PATCH v4 0/2] kvm-unit-tests: Build changes to support illumos Thomas Huth
  2 siblings, 0 replies; 31+ messages in thread
From: Dan Cross @ 2022-06-01 21:57 UTC (permalink / raw)
  To: kvm
  Cc: Andrew Jones, Thomas Huth, Sean Christopherson, Paolo Bonzini, 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


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* Re: [PATCH v4 0/2] kvm-unit-tests: Build changes to support illumos.
  2022-06-01 21:57                           ` [PATCH v4 0/2] kvm-unit-tests: Build changes to support illumos Dan Cross
  2022-06-01 21:57                             ` [PATCH v4 1/2] kvm-unit-tests: configure changes for illumos Dan Cross
  2022-06-01 21:57                             ` [PATCH v4 2/2] kvm-unit-tests: invoke $LD explicitly Dan Cross
@ 2022-06-02 10:53                             ` Thomas Huth
  2022-06-02 11:32                               ` Dan Cross
  2 siblings, 1 reply; 31+ messages in thread
From: Thomas Huth @ 2022-06-02 10:53 UTC (permalink / raw)
  To: Dan Cross, kvm; +Cc: Andrew Jones, Sean Christopherson, Paolo Bonzini

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



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v4 0/2] kvm-unit-tests: Build changes to support illumos.
  2022-06-02 10:53                             ` [PATCH v4 0/2] kvm-unit-tests: Build changes to support illumos Thomas Huth
@ 2022-06-02 11:32                               ` Dan Cross
  0 siblings, 0 replies; 31+ messages in thread
From: Dan Cross @ 2022-06-02 11:32 UTC (permalink / raw)
  To: Thomas Huth; +Cc: kvm, Andrew Jones, Sean Christopherson, Paolo Bonzini

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.

^ permalink raw reply	[flat|nested] 31+ messages in thread

end of thread, other threads:[~2022-06-02 11:32 UTC | newest]

Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-12 20:45 [PATCH] kvm-unit-tests: Build changes for illumos Dan Cross
2022-05-12 22:05 ` Sean Christopherson
2022-05-13  1:07   ` [PATCH 0/2] kvm-unit-tests: Build changes to support illumos Dan Cross
2022-05-13  1:07     ` [PATCH 1/2] kvm-unit-tests: invoke $LD explicitly in Dan Cross
2022-05-13  1:07     ` [PATCH 2/2] kvm-unit-tests: configure changes for illumos Dan Cross
2022-05-13 14:34       ` Sean Christopherson
2022-05-19  9:53         ` Thomas Huth
2022-05-24 21:20           ` Dan Cross
2022-05-25  7:41             ` Thomas Huth
2022-05-24 21:22         ` Dan Cross
2022-05-25  7:44           ` Thomas Huth
2022-05-26  7:11             ` Andrew Jones
2022-05-26 17:39               ` [PATCH 0/2] kvm-unit-tests: Build changes to support illumos Dan Cross
2022-05-26 17:39                 ` [PATCH 1/2] kvm-unit-tests: invoke $LD explicitly in Dan Cross
2022-05-26 21:17                   ` Sean Christopherson
2022-06-01  7:03                   ` Thomas Huth
2022-06-01 17:09                     ` Dan Cross
2022-06-01 17:43                       ` Andrew Jones
2022-06-01 21:51                         ` Dan Cross
2022-06-01 21:57                           ` [PATCH v4 0/2] kvm-unit-tests: Build changes to support illumos Dan Cross
2022-06-01 21:57                             ` [PATCH v4 1/2] kvm-unit-tests: configure changes for illumos Dan Cross
2022-06-01 21:57                             ` [PATCH v4 2/2] kvm-unit-tests: invoke $LD explicitly Dan Cross
2022-06-02 10:53                             ` [PATCH v4 0/2] kvm-unit-tests: Build changes to support illumos Thomas Huth
2022-06-02 11:32                               ` Dan Cross
2022-05-26 17:39                 ` [PATCH 2/2] kvm-unit-tests: configure changes for illumos Dan Cross
2022-05-26 21:23                   ` Sean Christopherson
2022-05-26 22:17                     ` Dan Cross
2022-05-27 14:41                       ` Sean Christopherson
2022-05-31 17:02                         ` Dan Cross
2022-05-26 17:40               ` Dan Cross
     [not found]   ` <CAA9fzEGdi0k8bkyXQwvt6gFd-gwHNNFF7A89U4DhtGHjKqe4AQ@mail.gmail.com>
2022-05-13  1:27     ` Fwd: [PATCH] kvm-unit-tests: Build " Dan Cross

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.