[PATCH 0/2] Fixes for building on Solaris 11.4.42 CBE

[PATCH 0/2] Fixes for building on Solaris 11.4.42 CBE

[Andrew Deason]

With these minor fixes, I can build qemu on Solaris 11.4.42 CBE
(Oracle's new rolling release thing), using '--disable-rdma
--enable-modules --disable-dbus-display --target-list=x86_64-softmmu'.
I'm just interested in the guest agent right now, so that's all I've
tested (briefly), but the rest of the build wasn't hard to get
working. With this, the guest agent runs fine using isa-serial.

Andrew Deason (2):
  util/osdep: Avoid madvise proto on modern Solaris
  hw/i386/acpi-build: Avoid 'sun' identifier

 hw/i386/acpi-build.c | 4 ++--
 meson.build          | 3 +++
 util/osdep.c         | 2 ++
 3 files changed, 7 insertions(+), 2 deletions(-)

-- 
2.11.0

[PATCH 1/2] util/osdep: Avoid madvise proto on modern Solaris

[Andrew Deason]

On older Solaris releases, we didn't get a protype for madvise, and so
util/osdep.c provides its own prototype. Some time between the public
Solaris 11.4 release and Solaris 11.4.42 CBE, we started getting an
madvise prototype that looks like this:

    extern int madvise(void *, size_t, int);

Which conflicts with the prototype in util/osdeps.c. Instead of always
declaring this prototype, check if madvise() is already declared, so
we don't need to declare it ourselves.

Signed-off-by: Andrew Deason <adeason@sinenomine.net>
---
I'm not sure if a test is needed for this at all; that is, how much qemu
cares about earlier Solaris. The madvise prototype exists earlier in
Solaris 11 (I'm not sure when it started appearing usefully), but in
11.4 and earlier it was compatible with the char* prototype.

 meson.build  | 3 +++
 util/osdep.c | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/meson.build b/meson.build
index 2d6601467f..c29d49c7a1 100644
--- a/meson.build
+++ b/meson.build
@@ -322,20 +322,23 @@ if targetos == 'windows'
                                       include_directories: include_directories('.'))
   host_dsosuf = '.dll'
 elif targetos == 'darwin'
   coref = dependency('appleframeworks', modules: 'CoreFoundation')
   iokit = dependency('appleframeworks', modules: 'IOKit', required: false)
   host_dsosuf = '.dylib'
 elif targetos == 'sunos'
   socket = [cc.find_library('socket'),
             cc.find_library('nsl'),
             cc.find_library('resolv')]
+  config_host_data.set('HAVE_MADVISE_PROTO',
+                       cc.has_function('madvise',
+                                       prefix: '#include <sys/mman.h>'))
 elif targetos == 'haiku'
   socket = [cc.find_library('posix_error_mapper'),
             cc.find_library('network'),
             cc.find_library('bsd')]
 elif targetos == 'openbsd'
   if get_option('tcg').allowed() and target_dirs.length() > 0
     # Disable OpenBSD W^X if available
     emulator_link_args = cc.get_supported_link_arguments('-Wl,-z,wxneeded')
   endif
 endif
diff --git a/util/osdep.c b/util/osdep.c
index 7c4deda6fe..c99083372b 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -21,24 +21,26 @@
  * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
  * THE SOFTWARE.
  */
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 
 /* Needed early for CONFIG_BSD etc. */
 
 #ifdef CONFIG_SOLARIS
 #include <sys/statvfs.h>
+#ifndef HAVE_MADVISE_PROTO
 /* See MySQL bug #7156 (http://bugs.mysql.com/bug.php?id=7156) for
    discussion about Solaris header problems */
 extern int madvise(char *, size_t, int);
 #endif
+#endif
 
 #include "qemu-common.h"
 #include "qemu/cutils.h"
 #include "qemu/sockets.h"
 #include "qemu/error-report.h"
 #include "qemu/madvise.h"
 #include "qemu/mprotect.h"
 #include "qemu/hw-version.h"
 #include "monitor/monitor.h"
 
-- 
2.11.0

Re: [PATCH 1/2] util/osdep: Avoid madvise proto on modern Solaris

[Peter Maydell]

On Mon, 14 Mar 2022 at 16:12, Andrew Deason <adeason@sinenomine.net> wrote:
>
> On older Solaris releases, we didn't get a protype for madvise, and so
> util/osdep.c provides its own prototype. Some time between the public
> Solaris 11.4 release and Solaris 11.4.42 CBE, we started getting an
> madvise prototype that looks like this:
>
>     extern int madvise(void *, size_t, int);
>
> Which conflicts with the prototype in util/osdeps.c. Instead of always
> declaring this prototype, check if madvise() is already declared, so
> we don't need to declare it ourselves.
>
> Signed-off-by: Andrew Deason <adeason@sinenomine.net>
> ---
> I'm not sure if a test is needed for this at all; that is, how much qemu
> cares about earlier Solaris. The madvise prototype exists earlier in
> Solaris 11 (I'm not sure when it started appearing usefully), but in
> 11.4 and earlier it was compatible with the char* prototype.

>  #ifdef CONFIG_SOLARIS
>  #include <sys/statvfs.h>
> +#ifndef HAVE_MADVISE_PROTO
>  /* See MySQL bug #7156 (http://bugs.mysql.com/bug.php?id=7156) for
>     discussion about Solaris header problems */
>  extern int madvise(char *, size_t, int);
>  #endif
> +#endif

Rather than keeping this inside a CONFIG_SOLARIS and only doing
the meson.build test if targetos == sunos, I would prefer it if we
unconditionally determined two things in meson.build:
 (1) do we have madvise in the usual way? (this is what we would
     want CONFIG_MADVISE to be, and might even be what it actually is)
 (2) do we have madvise but only if we provide a prototype for it
     ourselves? (maybe CONFIG_MADVISE_NO_PROTO)

and then in osdep.h provide the prototype if CONFIG_MADVISE_NO_PROTO.
(osdep.h is where we provide "this is a fixup to the system headers"
portability workarounds, which this seems to be.)

This isn't the only .c file that directly calls madvise() :
softmmu/physmem.c does also. That looks like maybe a bug though:
perhaps it should be calling qemu_madvise()...

Side note: do you know why CONFIG_SOLARIS includes sys/statvfs.h ?
Is that unrelated to madvise() ?

thanks
-- PMM

Re: [PATCH 1/2] util/osdep: Avoid madvise proto on modern Solaris

[Andrew Deason]

On Mon, 14 Mar 2022 16:36:00 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

> On Mon, 14 Mar 2022 at 16:12, Andrew Deason <adeason@sinenomine.net> wrote:
> >  #ifdef CONFIG_SOLARIS
> >  #include <sys/statvfs.h>
> > +#ifndef HAVE_MADVISE_PROTO
> >  /* See MySQL bug #7156 (http://bugs.mysql.com/bug.php?id=7156) for
> >     discussion about Solaris header problems */
> >  extern int madvise(char *, size_t, int);
> >  #endif
> > +#endif
> 
> Rather than keeping this inside a CONFIG_SOLARIS and only doing
> the meson.build test if targetos == sunos, I would prefer it if we
> unconditionally determined two things in meson.build:
>  (1) do we have madvise in the usual way? (this is what we would
>      want CONFIG_MADVISE to be, and might even be what it actually is)
>  (2) do we have madvise but only if we provide a prototype for it
>      ourselves? (maybe CONFIG_MADVISE_NO_PROTO)

CONFIG_MADVISE is set if we can cc.links() something that calls
madvise(). If we're missing the prototype, that will fail with -Werror,
but I expect succeeds otherwise. If cc.links() just uses the cflags for
the build, then it seems like it might succeed or fail depending on
--enable-werror. I see some other tests give -Werror as an explicit
extra argument (HAVE_BROKEN_SIZE_MAX, and something for fuzzing); should
this be doing the same to make sure it fails for a missing prototype?

Also just to mention, if we don't care about older Solaris, the
prototype can just be unconditionally removed. It's pretty annoying to
even try to build qemu from git on Solaris 11.4 and earlier, because
many of the build requirements need to be installed/compiled manually
(notably python 3.6+, but iirc also ninja, meson, etc). So I haven't
really tried; there may be many other build issues there.

> Side note: do you know why CONFIG_SOLARIS includes sys/statvfs.h ?
> Is that unrelated to madvise() ?

I think so, it was added in commit 605686cd7ac, which predates madvise()
in that file. It does look like it could be removed from a quick glance.

Would you want me to add a commit to remove it? (Assuming it still
compiles okay.) Or just do that in the same commit as changing the
madvise prototype logic?

-- 
Andrew Deason
adeason@sinenomine.net

Re: [PATCH 1/2] util/osdep: Avoid madvise proto on modern Solaris

[Peter Maydell]

On Mon, 14 Mar 2022 at 18:18, Andrew Deason <adeason@sinenomine.net> wrote:
>
> On Mon, 14 Mar 2022 16:36:00 +0000
> Peter Maydell <peter.maydell@linaro.org> wrote:
>
> > On Mon, 14 Mar 2022 at 16:12, Andrew Deason <adeason@sinenomine.net> wrote:
> > >  #ifdef CONFIG_SOLARIS
> > >  #include <sys/statvfs.h>
> > > +#ifndef HAVE_MADVISE_PROTO
> > >  /* See MySQL bug #7156 (http://bugs.mysql.com/bug.php?id=7156) for
> > >     discussion about Solaris header problems */
> > >  extern int madvise(char *, size_t, int);
> > >  #endif
> > > +#endif
> >
> > Rather than keeping this inside a CONFIG_SOLARIS and only doing
> > the meson.build test if targetos == sunos, I would prefer it if we
> > unconditionally determined two things in meson.build:
> >  (1) do we have madvise in the usual way? (this is what we would
> >      want CONFIG_MADVISE to be, and might even be what it actually is)
> >  (2) do we have madvise but only if we provide a prototype for it
> >      ourselves? (maybe CONFIG_MADVISE_NO_PROTO)
>
> CONFIG_MADVISE is set if we can cc.links() something that calls
> madvise(). If we're missing the prototype, that will fail with -Werror,
> but I expect succeeds otherwise. If cc.links() just uses the cflags for
> the build, then it seems like it might succeed or fail depending on
> --enable-werror.

Mmm. I think that we wrote it that way because it was a straight
translation to meson of the previous configure-script madvise
detection code. I think it is equivalent to
config_host_data.set('CONFIG_MEMALIGN', cc.has_function('memalign'))
which also effectively does a pure "does this link?" test.
So maybe we want to keep CONFIG_MEMALIGN the way it is and add
a CONFIG_MISSING_MEMALIGN_PROTOTYPE or something. I think that
this is rapidly getting out of my depth as far as meson is concerned,
though.

> I see some other tests give -Werror as an explicit
> extra argument (HAVE_BROKEN_SIZE_MAX, and something for fuzzing); should
> this be doing the same to make sure it fails for a missing prototype?
>
> Also just to mention, if we don't care about older Solaris, the
> prototype can just be unconditionally removed. It's pretty annoying to
> even try to build qemu from git on Solaris 11.4 and earlier, because
> many of the build requirements need to be installed/compiled manually
> (notably python 3.6+, but iirc also ninja, meson, etc). So I haven't
> really tried; there may be many other build issues there.

Hard to say whether we do or don't care. We have had some
mailing list threads in the past. I think we might also care
a bit about Illumos, which might still have some issues that
Solaris proper has since fixed. Sometimes it's easier to hang on
to the portability workaround than to try to find out :-)

> > Side note: do you know why CONFIG_SOLARIS includes sys/statvfs.h ?
> > Is that unrelated to madvise() ?
>
> I think so, it was added in commit 605686cd7ac, which predates madvise()
> in that file. It does look like it could be removed from a quick glance.

Yeah, I think in commit 4a1418e07bdcfaa31 in 2009 we removed kqemu
support, which was the only thing using statvfs() in that file,
but forgot to remove the #include. Since that's an entirely separate
thing from madvise, feel free to either ignore it or send a separate patch.

-- PMM

Re: [PATCH 1/2] util/osdep: Avoid madvise proto on modern Solaris

[Daniel P. Berrangé]

On Mon, Mar 14, 2022 at 01:18:00PM -0500, Andrew Deason wrote:
> On Mon, 14 Mar 2022 16:36:00 +0000
> Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> > On Mon, 14 Mar 2022 at 16:12, Andrew Deason <adeason@sinenomine.net> wrote:
> > >  #ifdef CONFIG_SOLARIS
> > >  #include <sys/statvfs.h>
> > > +#ifndef HAVE_MADVISE_PROTO
> > >  /* See MySQL bug #7156 (http://bugs.mysql.com/bug.php?id=7156) for
> > >     discussion about Solaris header problems */
> > >  extern int madvise(char *, size_t, int);
> > >  #endif
> > > +#endif
> > 
> > Rather than keeping this inside a CONFIG_SOLARIS and only doing
> > the meson.build test if targetos == sunos, I would prefer it if we
> > unconditionally determined two things in meson.build:
> >  (1) do we have madvise in the usual way? (this is what we would
> >      want CONFIG_MADVISE to be, and might even be what it actually is)
> >  (2) do we have madvise but only if we provide a prototype for it
> >      ourselves? (maybe CONFIG_MADVISE_NO_PROTO)
> 
> CONFIG_MADVISE is set if we can cc.links() something that calls
> madvise(). If we're missing the prototype, that will fail with -Werror,
> but I expect succeeds otherwise. If cc.links() just uses the cflags for
> the build, then it seems like it might succeed or fail depending on
> --enable-werror. I see some other tests give -Werror as an explicit
> extra argument (HAVE_BROKEN_SIZE_MAX, and something for fuzzing); should
> this be doing the same to make sure it fails for a missing prototype?
> 
> Also just to mention, if we don't care about older Solaris, the
> prototype can just be unconditionally removed. It's pretty annoying to
> even try to build qemu from git on Solaris 11.4 and earlier, because
> many of the build requirements need to be installed/compiled manually
> (notably python 3.6+, but iirc also ninja, meson, etc). So I haven't
> really tried; there may be many other build issues there.

If we had a tiered support status, Solaris  would be tier 3
right now, as we have no testing of it at all. If it compiles
at any given point in time it is luck.

We have a general purpose platform support policy

  https://www.qemu.org/docs/master/about/build-platforms.html

where the common rule ends up being "the current major release,
and the previous major release for 2 years overlap".

The question is what counts as a major release from a Solaris
POV ? In terms of long life distros, our policy gives about
4-5 years of supportable life in the best case. I wouldn't
want to go beyond that ballpark for Solaris.  Can we come up
with an interpration of our policy to map to Solaris that
doesn't tie our hands for longer than 4-5 years worst case.


IOW, we certainly do NOT need to support arbitrarily old
Solaris. If madvise has done what we need for 4-5 years,
then we can likely not need to test for it, and just assume
its existance. This just requires someone to specify how
we interpret our build platform policy to exclude older
Solaris.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 1/2] util/osdep: Avoid madvise proto on modern Solaris

[Andrew Deason]

On Mon, 14 Mar 2022 19:01:06 +0000
Daniel P. Berrangé <berrange@redhat.com> wrote:

> We have a general purpose platform support policy
> 
>   https://www.qemu.org/docs/master/about/build-platforms.html
> 
> where the common rule ends up being "the current major release,
> and the previous major release for 2 years overlap".
> 
> The question is what counts as a major release from a Solaris
> POV ? In terms of long life distros, our policy gives about
> 4-5 years of supportable life in the best case. I wouldn't
> want to go beyond that ballpark for Solaris.  Can we come up
> with an interpration of our policy to map to Solaris that
> doesn't tie our hands for longer than 4-5 years worst case.

FWIW, some relevant Solaris version numbers, as I understand it:

11.4.42 CBE: public release March 2022. (Non-production use only,
rolling release schedule.)

11.4: public release August 2018.

11.3: public release October 2015.

Going by the minor version number (11.3 vs 11.4) sounds similar to Linux
distros; they've come out every few years in the past. But I don't know
how this is going to look with the CBE stuff in the future, or if anyone
knows (it's very new).

> IOW, we certainly do NOT need to support arbitrarily old
> Solaris. If madvise has done what we need for 4-5 years,
> then we can likely not need to test for it, and just assume
> its existance. This just requires someone to specify how
> we interpret our build platform policy to exclude older
> Solaris.

Specifically for the madvise() prototype workaround, I looked a little
bit into what version changes actually matter. I think all of Solaris 11
is probably okay without the workaround (I can check back to Solaris
11.1, but just by looking at the mman.h header, not actually testing a
build), because we specify -D__EXTENSIONS__.

Illumos and Solaris 10 look like they would need the workaround.

So practically speaking for this patchset, it seems more like a question
of Illumos support.

-- 
Andrew Deason
adeason@sinenomine.net

[PATCH 2/2] hw/i386/acpi-build: Avoid 'sun' identifier

[Andrew Deason]

On Solaris, 'sun' is #define'd to 1, which causes errors if a variable
is named 'sun'. Slightly change the name of the var for the Slot User
Number so we can build on Solaris.

Signed-off-by: Andrew Deason <adeason@sinenomine.net>
---
 hw/i386/acpi-build.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 4ad4d7286c..dcf6ece3d0 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -576,32 +576,32 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
 }
 
 Aml *aml_pci_device_dsm(void)
 {
     Aml *method, *UUID, *ifctx, *ifctx1, *ifctx2, *ifctx3, *elsectx;
     Aml *acpi_index = aml_local(0);
     Aml *zero = aml_int(0);
     Aml *bnum = aml_arg(4);
     Aml *func = aml_arg(2);
     Aml *rev = aml_arg(1);
-    Aml *sun = aml_arg(5);
+    Aml *sunum = aml_arg(5);
 
     method = aml_method("PDSM", 6, AML_SERIALIZED);
 
     /*
      * PCI Firmware Specification 3.1
      * 4.6.  _DSM Definitions for PCI
      */
     UUID = aml_touuid("E5C937D0-3553-4D7A-9117-EA4D19C3434D");
     ifctx = aml_if(aml_equal(aml_arg(0), UUID));
     {
-        aml_append(ifctx, aml_store(aml_call2("AIDX", bnum, sun), acpi_index));
+        aml_append(ifctx, aml_store(aml_call2("AIDX", bnum, sunum), acpi_index));
         ifctx1 = aml_if(aml_equal(func, zero));
         {
             uint8_t byte_list[1];
 
             ifctx2 = aml_if(aml_equal(rev, aml_int(2)));
             {
                 /*
                  * advertise function 7 if device has acpi-index
                  * acpi_index values:
                  *            0: not present (default value)
-- 
2.11.0