[Qemu-devel] [PATCH v2 1/1] configure: dtc: Probe for libfdt_env.h

[Qemu-devel] [PATCH v2 1/1] configure: dtc: Probe for libfdt_env.h

[peter.crosthwaite]

From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Currently QEMU provides a local clone of the file libfdt_env.h in
/include. This file is supposed to come with the libfdt package and is
only needed for broken installs of libfdt. Now that we have submodule
dtc, just ignore these broken installs and prompt for the dtc submodule
install instead. QEMU's local libfdt_env.h is removed accordingly.

Manifests as a bug when building QEMU with modern libfdt. The new
version of libfdt does not compile when QEMUs libfdt_env.h takes
precedence over the hosts.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
changed since V1:
Just fallback to submodule rather than provide libfdt_env.h

I suggest this patch goes in behind PMMs patches to make dtc
compulsory for PPC/MB/ARM to avoid silent regressions for current
users of dtc 1.3.0.

To replicate bug (install latest dtc from src to host and make):
git submodule update --init dtc
cd dtc
git checkout master
make
sudo make install PREFIX=/usr/local
cd ..
./configure --target-list="arm-softmmu" --enable-fdt
make

 configure            |  2 ++
 include/libfdt_env.h | 36 ------------------------------------
 2 files changed, 2 insertions(+), 36 deletions(-)
 delete mode 100644 include/libfdt_env.h

diff --git a/configure b/configure
index eb74510..87bc9a7 100755
--- a/configure
+++ b/configure
@@ -2519,7 +2519,9 @@ fi
 # fdt probe
 if test "$fdt" != "no" ; then
   fdt_libs="-lfdt"
+  # explicitly check for libfdt_env.h as it is missing in some stable installs
   cat > $TMPC << EOF
+#include <libfdt_env.h>
 int main(void) { return 0; }
 EOF
   if compile_prog "" "$fdt_libs" ; then
diff --git a/include/libfdt_env.h b/include/libfdt_env.h
deleted file mode 100644
index 3667d4c..0000000
--- a/include/libfdt_env.h
+++ /dev/null
@@ -1,36 +0,0 @@
-/*
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License, version 2, as
- * published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, see <http://www.gnu.org/licenses/>.
- *
- * Copyright IBM Corp. 2008
- * Authors: Hollis Blanchard <hollisb@us.ibm.com>
- *
- */
-
-#ifndef _LIBFDT_ENV_H
-#define _LIBFDT_ENV_H
-
-#include "qemu/bswap.h"
-
-#ifdef HOST_WORDS_BIGENDIAN
-#define fdt32_to_cpu(x)  (x)
-#define cpu_to_fdt32(x)  (x)
-#define fdt64_to_cpu(x)  (x)
-#define cpu_to_fdt64(x)  (x)
-#else
-#define fdt32_to_cpu(x)  bswap32(x)
-#define cpu_to_fdt32(x)  bswap32(x)
-#define fdt64_to_cpu(x)  bswap64(x)
-#define cpu_to_fdt64(x)  bswap64(x)
-#endif
-
-#endif /* _LIBFDT_ENV_H */
-- 
1.8.3.rc1.44.gb387c77.dirty

Re: [Qemu-devel] [PATCH v2 1/1] configure: dtc: Probe for libfdt_env.h

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 845 bytes --]

On Mon, May 27, 2013 at 02:20:57PM +1000, peter.crosthwaite@xilinx.com wrote:
> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> 
> Currently QEMU provides a local clone of the file libfdt_env.h in
> /include. This file is supposed to come with the libfdt package and is

So, this patch might be the right thing anyway, but the statement
above is entirely false.  libfdt_env.h is explicitly intended to be
supplied by the build environment embedding libfdt.  The version
provided in the libfdt package is a example version which will do
(although it might not be ideal) for userspace builds with a
sufficiently normal C library.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [Qemu-devel] [PATCH v2 1/1] configure: dtc: Probe for libfdt_env.h

[Peter Maydell]

On 29 May 2013 09:18, David Gibson <david@gibson.dropbear.id.au> wrote:
> On Mon, May 27, 2013 at 02:20:57PM +1000, peter.crosthwaite@xilinx.com wrote:
>> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> Currently QEMU provides a local clone of the file libfdt_env.h in
>> /include. This file is supposed to come with the libfdt package and is
>
> So, this patch might be the right thing anyway, but the statement
> above is entirely false.  libfdt_env.h is explicitly intended to be
> supplied by the build environment embedding libfdt.  The version
> provided in the libfdt package is a example version which will do
> (although it might not be ideal) for userspace builds with a
> sufficiently normal C library.

(You're upstream for libfdt, yes?)

If you're expecting that build environments provide a libfdt_env.h
then shouldn't you be avoiding making breaking changes to libfdt
that require build environments to fix their libfdt_env.h?
This came up in the first place because we no longer build with
new libfdt...

(conversely, if we're supposed to update our libfdt_env.h as we
go along, is there a changelog that documents needed changes
and what needs to be present for compatibility with older versions
of libfdt?)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 1/1] configure: dtc: Probe for libfdt_env.h

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2474 bytes --]

On Wed, May 29, 2013 at 10:14:19AM +0100, Peter Maydell wrote:
> On 29 May 2013 09:18, David Gibson <david@gibson.dropbear.id.au> wrote:
> > On Mon, May 27, 2013 at 02:20:57PM +1000, peter.crosthwaite@xilinx.com wrote:
> >> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> >> Currently QEMU provides a local clone of the file libfdt_env.h in
> >> /include. This file is supposed to come with the libfdt package and is
> >
> > So, this patch might be the right thing anyway, but the statement
> > above is entirely false.  libfdt_env.h is explicitly intended to be
> > supplied by the build environment embedding libfdt.  The version
> > provided in the libfdt package is a example version which will do
> > (although it might not be ideal) for userspace builds with a
> > sufficiently normal C library.
> 
> (You're upstream for libfdt, yes?)

More or less, yes.

> If you're expecting that build environments provide a libfdt_env.h
> then shouldn't you be avoiding making breaking changes to libfdt
> that require build environments to fix their libfdt_env.h?
> This came up in the first place because we no longer build with
> new libfdt...

I missed that discussion, what exactly was the problem?  It's possible
we did something silly in libfdt.  Then again, although we certainly
want to keep changes which require updates to libfdt_env.h rare, I'm
not going to rule out extensions to libfdt which add new (minor)
external dependencies, and therefore changes to libfdt_env.h.

That said, I did put my correction a bit too strongly.  While
libfdt_env.h is notionally always provided by the surrounding
environment, if the packaged one works for your environment, it's
probably a good idea to use it.  That should be the case for almost
all userspace builds - providing a custom libfdt_env.h is more
intended for kernels, bootloaders and other build environments with
peculiar constraints.

> (conversely, if we're supposed to update our libfdt_env.h as we
> go along, is there a changelog that documents needed changes
> and what needs to be present for compatibility with older versions
> of libfdt?)

Heh.  There definitely should be a document covering what needs to be
provided by libfdt_env.h, but alas there is not.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [Qemu-devel] [PATCH v2 1/1] configure: dtc: Probe for libfdt_env.h

[Peter Crosthwaite]

Hi David,

On Wed, May 29, 2013 at 7:31 PM, David Gibson
<david@gibson.dropbear.id.au> wrote:
> On Wed, May 29, 2013 at 10:14:19AM +0100, Peter Maydell wrote:
>> On 29 May 2013 09:18, David Gibson <david@gibson.dropbear.id.au> wrote:
>> > On Mon, May 27, 2013 at 02:20:57PM +1000, peter.crosthwaite@xilinx.com wrote:
>> >> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> >> Currently QEMU provides a local clone of the file libfdt_env.h in
>> >> /include. This file is supposed to come with the libfdt package and is
>> >
>> > So, this patch might be the right thing anyway, but the statement
>> > above is entirely false.  libfdt_env.h is explicitly intended to be
>> > supplied by the build environment embedding libfdt.  The version
>> > provided in the libfdt package is a example version which will do
>> > (although it might not be ideal) for userspace builds with a
>> > sufficiently normal C library.
>>
>> (You're upstream for libfdt, yes?)
>
> More or less, yes.
>
>> If you're expecting that build environments provide a libfdt_env.h
>> then shouldn't you be avoiding making breaking changes to libfdt
>> that require build environments to fix their libfdt_env.h?
>> This came up in the first place because we no longer build with
>> new libfdt...
>
> I missed that discussion, what exactly was the problem?

In file included from /usr/local/include/libfdt.h:55,
                 from
/home/peterc/Petalogix/Internal/plgx_src/qemu-arm-microblazeel/device_tree.c:28:
/usr/local/include/fdt.h:7: error: expected specifier-qualifier-list
before ‘fdt32_t’
/usr/local/include/fdt.h:26: error: expected specifier-qualifier-list
before ‘fdt64_t’
/usr/local/include/fdt.h:31: error: expected specifier-qualifier-list
before ‘fdt32_t’
/usr/local/include/fdt.h:36: error: expected specifier-qualifier-list
before ‘fdt32_t’

The type fdt32_t (and friends) is undefined.

Heres the patch DTC side that introduces these types.

commit feafcd972cb744750a65728440c99526e6199a6d
Author: Kim Phillips <kim.phillips@freescale.com>
Date:   Wed Nov 28 17:33:01 2012 -0600

    dtc/libfdt: introduce fdt types for annotation by endian checkers

    Projects such as linux and u-boot run sparse on libfdt.  libfdt
    contains the notion of endianness via usage of endian conversion
    functions such as fdt32_to_cpu.  As such, in order to pass endian
    checks, libfdt has to annotate its fdt variables such that sparse
    can warn when mixing bitwise and regular integers.  This patch adds
    these new fdtXX_t types and, ifdef __CHECKER__ (a symbol sparse
    defines), includes the bitwise annotation.

    Signed-off-by: Kim Phillips <kim.phillips@freescale.com>
    Acked-by: David Gibson <david@gibson.dropbear.id.au>


> Then again, although we certainly
> want to keep changes which require updates to libfdt_env.h rare, I'm
> not going to rule out extensions to libfdt which add new (minor)
> external dependencies, and therefore changes to libfdt_env.h.
>
> That said, I did put my correction a bit too strongly.  While
> libfdt_env.h is notionally always provided by the surrounding
> environment, if the packaged one works for your environment, it's
> probably a good idea to use it.

I think we should be able to consider QEMU as normal enough to use
prepacked DTC and just fix the commit message?

Regards,
peter

  That should be the case for almost
> all userspace builds - providing a custom libfdt_env.h is more
> intended for kernels, bootloaders and other build environments with
> peculiar constraints.
>
>> (conversely, if we're supposed to update our libfdt_env.h as we
>> go along, is there a changelog that documents needed changes
>> and what needs to be present for compatibility with older versions
>> of libfdt?)
>
> Heh.  There definitely should be a document covering what needs to be
> provided by libfdt_env.h, but alas there is not.
>
> --
> David Gibson                    | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
>                                 | _way_ _around_!
> http://www.ozlabs.org/~dgibson

Re: [Qemu-devel] [PATCH v2 1/1] configure: dtc: Probe for libfdt_env.h

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3343 bytes --]

On Fri, May 31, 2013 at 11:48:47AM +1000, Peter Crosthwaite wrote:
> Hi David,
> 
> On Wed, May 29, 2013 at 7:31 PM, David Gibson
> <david@gibson.dropbear.id.au> wrote:
> > On Wed, May 29, 2013 at 10:14:19AM +0100, Peter Maydell wrote:
> >> On 29 May 2013 09:18, David Gibson <david@gibson.dropbear.id.au> wrote:
> >> > On Mon, May 27, 2013 at 02:20:57PM +1000, peter.crosthwaite@xilinx.com wrote:
> >> >> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> >> >> Currently QEMU provides a local clone of the file libfdt_env.h in
> >> >> /include. This file is supposed to come with the libfdt package and is
> >> >
> >> > So, this patch might be the right thing anyway, but the statement
> >> > above is entirely false.  libfdt_env.h is explicitly intended to be
> >> > supplied by the build environment embedding libfdt.  The version
> >> > provided in the libfdt package is a example version which will do
> >> > (although it might not be ideal) for userspace builds with a
> >> > sufficiently normal C library.
> >>
> >> (You're upstream for libfdt, yes?)
> >
> > More or less, yes.
> >
> >> If you're expecting that build environments provide a libfdt_env.h
> >> then shouldn't you be avoiding making breaking changes to libfdt
> >> that require build environments to fix their libfdt_env.h?
> >> This came up in the first place because we no longer build with
> >> new libfdt...
> >
> > I missed that discussion, what exactly was the problem?
> 
> In file included from /usr/local/include/libfdt.h:55,
>                  from
> /home/peterc/Petalogix/Internal/plgx_src/qemu-arm-microblazeel/device_tree.c:28:
> /usr/local/include/fdt.h:7: error: expected specifier-qualifier-list
> before ‘fdt32_t’
> /usr/local/include/fdt.h:26: error: expected specifier-qualifier-list
> before ‘fdt64_t’
> /usr/local/include/fdt.h:31: error: expected specifier-qualifier-list
> before ‘fdt32_t’
> /usr/local/include/fdt.h:36: error: expected specifier-qualifier-list
> before ‘fdt32_t’
> 
> The type fdt32_t (and friends) is undefined.
> 
> Heres the patch DTC side that introduces these types.
> 
> commit feafcd972cb744750a65728440c99526e6199a6d
> Author: Kim Phillips <kim.phillips@freescale.com>
> Date:   Wed Nov 28 17:33:01 2012 -0600
> 
>     dtc/libfdt: introduce fdt types for annotation by endian
>     checkers

Ah, that one.  Basically, we decided that the benefit gained was worth
the pain of the required change to libfdt_env.h

> > Then again, although we certainly
> > want to keep changes which require updates to libfdt_env.h rare, I'm
> > not going to rule out extensions to libfdt which add new (minor)
> > external dependencies, and therefore changes to libfdt_env.h.
> >
> > That said, I did put my correction a bit too strongly.  While
> > libfdt_env.h is notionally always provided by the surrounding
> > environment, if the packaged one works for your environment, it's
> > probably a good idea to use it.
> 
> I think we should be able to consider QEMU as normal enough to use
> prepacked DTC and just fix the commit message?

I concur.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [Qemu-devel] [PATCH v2 1/1] configure: dtc: Probe for libfdt_env.h

[Paolo Bonzini]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Il 31/05/2013 05:32, David Gibson ha scritto:
>>> Then again, although we certainly want to keep changes which 
>>> require updates to libfdt_env.h rare, I'm not going to rule
>>> out extensions to libfdt which add new (minor) external 
>>> dependencies, and therefore changes to libfdt_env.h.
>>> 
>>> That said, I did put my correction a bit too strongly.  While 
>>> libfdt_env.h is notionally always provided by the surrounding 
>>> environment, if the packaged one works for your environment, 
>>> it's probably a good idea to use it.
>> 
>> I think we should be able to consider QEMU as normal enough to 
>> use prepacked DTC and just fix the commit message?
> 
> I concur.

Please don't.  Fedora is not going to use the bundled dtc because of a
policy against bundling, and Fedora's dtc package doesn't include
libfdt_env.h.

If David says that the changes are rare enough, let's just fix the
bundled libfdt_env.h.

Paolo
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJRqF6TAAoJEBvWZb6bTYbycyYQAJswKGTnYtac7eJb2s7+I48R
TwX8NDbsLXyuyrBRNtwK/yLuSIdXKZeyhXmJNWHtvf2KH0aSgP3+FKHnoOrc0m/U
7ZMfIqquDkqx0AspSIeEmp3Q+i03oyI+nRzop9tpyBAPDGQm6VsAJzggwEBjfAUJ
+Ww+1g4UTfF/9Nwjj9EMryS85gMQLwd52t2mIcxcEN22XucZbuF6OmFOk4Tt7EW1
ZbBMm+J8vFOFu77ejpRE03SsfN6vd2FDFgyo+nTGay89hlVKxUbSkmxsmVqcwpqb
rV9fk5r266+50SW5X/EzJLAyVtilXZSDB84wSRQFVP8vUKKGwxz1q56rEpuwfdBW
JYB69woigiC4RmYtgueGy2tnP5xSTXKopo5QjKyjcyJlqpF1xQEd8YVh5gKBYKxG
Hla2uEM/QOmMFR0jf9kj0kyIF2seK7PGMA3M+W5OFyB1YKWaVJIjFW/r13g8Zfjb
MkA2LirNWzLo5H/mgkYLrC2sa9e6zVbc7+IPgj7t0mIBOQN1L4V50n81xlQNalGq
/NGuyz7v3qbtQVCKbS2C+Jtoq2/uvHyY0jd7AytRRRCO9KA/mine1gpXX96T9PXB
kjJO3ASHEJLT8GblsFDtb9NWHKoO/jrUOWuKM/4OmL4aFdSTYY3+z3/+5PlJhhWY
jPFsbspFp+aEqJPox3kT
=znA9
-----END PGP SIGNATURE-----

Re: [Qemu-devel] [PATCH v2 1/1] configure: dtc: Probe for libfdt_env.h

[Peter Maydell]

On 31 May 2013 09:25, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Please don't.  Fedora is not going to use the bundled dtc because of a
> policy against bundling, and Fedora's dtc package doesn't include
> libfdt_env.h.

It sounds like Fedora's dtc package is broken then -- are you
going to fix it?

> If David says that the changes are rare enough, let's just fix the
> bundled libfdt_env.h.

...I don't have an objection to this if we can make it
work for all versions of libfdt, and if it's a way of
using libfdt that upstream expects and is happy to support.

Mostly I'd just like it to be clear how upstream expects us
to use libfdt and how upstream expects distributions to
package libfdt, and then make sure we're in line with that.
It seems like a lot of the problem is that people (us and
distros) have been guessing in the absence of documentation :-(

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 1/1] configure: dtc: Probe for libfdt_env.h

[Paolo Bonzini]

Il 31/05/2013 11:44, Peter Maydell ha scritto:
> On 31 May 2013 09:25, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Please don't.  Fedora is not going to use the bundled dtc because of a
>> policy against bundling, and Fedora's dtc package doesn't include
>> libfdt_env.h.
> 
> It sounds like Fedora's dtc package is broken then -- are you
> going to fix it?

I'm not the packager, I can open a bug. :)

>> If David says that the changes are rare enough, let's just fix the
>> bundled libfdt_env.h.
> 
> ...I don't have an objection to this if we can make it
> work for all versions of libfdt, and if it's a way of
> using libfdt that upstream expects and is happy to support.

Former, I don't know.  Latter, yes.

Paolo

Re: [Qemu-devel] [PATCH v2 1/1] configure: dtc: Probe for libfdt_env.h

[Peter Crosthwaite]

Hi,

On Fri, May 31, 2013 at 7:54 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 31/05/2013 11:44, Peter Maydell ha scritto:
>> On 31 May 2013 09:25, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> Please don't.  Fedora is not going to use the bundled dtc because of a
>>> policy against bundling, and Fedora's dtc package doesn't include
>>> libfdt_env.h.
>>

V1 of this patch was attempting to address this - it wont break your
fedora case. It that more acceptable? The consensus in the V1
discussion was to just give up on ancient versions of dtc and such
users fall back to submodule dtc.

Regards,
Peter

>> It sounds like Fedora's dtc package is broken then -- are you
>> going to fix it?
>
> I'm not the packager, I can open a bug. :)
>
>>> If David says that the changes are rare enough, let's just fix the
>>> bundled libfdt_env.h.
>>
>> ...I don't have an objection to this if we can make it
>> work for all versions of libfdt, and if it's a way of
>> using libfdt that upstream expects and is happy to support.
>
> Former, I don't know.  Latter, yes.
>
> Paolo
>

Re: [Qemu-devel] [PATCH v2 1/1] configure: dtc: Probe for libfdt_env.h

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1643 bytes --]

On Fri, May 31, 2013 at 10:44:33AM +0100, Peter Maydell wrote:
> On 31 May 2013 09:25, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > Please don't.  Fedora is not going to use the bundled dtc because of a
> > policy against bundling, and Fedora's dtc package doesn't include
> > libfdt_env.h.
> 
> It sounds like Fedora's dtc package is broken then -- are you
> going to fix it?
> 
> > If David says that the changes are rare enough, let's just fix the
> > bundled libfdt_env.h.
> 
> ...I don't have an objection to this if we can make it
> work for all versions of libfdt, and if it's a way of
> using libfdt that upstream expects and is happy to support.
> 
> Mostly I'd just like it to be clear how upstream expects us
> to use libfdt and how upstream expects distributions to
> package libfdt, and then make sure we're in line with that.
> It seems like a lot of the problem is that people (us and
> distros) have been guessing in the absence of documentation :-(

Yeah.  Sorry.  Patches welcome..?

The key point of confusion here is that it is the *builder* of libfdt
that's expected to provide libfdt_env.h, not the user.  So if you're
pulling in a prebuilt libfdt from elsewhere, you should be pulling in
libfdt_env.h from the same elsewhere.

So, a distro packaging libfdt must provide the libfdt_env.h it built
with - which in this case I'd expect to be the standard/default
version from the libfdt tree.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [Qemu-devel] [PATCH v2 1/1] configure: dtc: Probe for libfdt_env.h

[Peter Crosthwaite]

HI David,

On Sat, Jun 1, 2013 at 3:38 PM, David Gibson
<david@gibson.dropbear.id.au> wrote:
> On Fri, May 31, 2013 at 10:44:33AM +0100, Peter Maydell wrote:
>> On 31 May 2013 09:25, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> > Please don't.  Fedora is not going to use the bundled dtc because of a
>> > policy against bundling, and Fedora's dtc package doesn't include
>> > libfdt_env.h.
>>
>> It sounds like Fedora's dtc package is broken then -- are you
>> going to fix it?
>>
>> > If David says that the changes are rare enough, let's just fix the
>> > bundled libfdt_env.h.
>>
>> ...I don't have an objection to this if we can make it
>> work for all versions of libfdt, and if it's a way of
>> using libfdt that upstream expects and is happy to support.
>>
>> Mostly I'd just like it to be clear how upstream expects us
>> to use libfdt and how upstream expects distributions to
>> package libfdt, and then make sure we're in line with that.
>> It seems like a lot of the problem is that people (us and
>> distros) have been guessing in the absence of documentation :-(
>
> Yeah.  Sorry.  Patches welcome..?
>
> The key point of confusion here is that it is the *builder* of libfdt
> that's expected to provide libfdt_env.h, not the user.

Then isn't my commit message right after all? This patch is not fixing
QEMUs build of libfdt (submodule), it is fixing the case where it
attempts to use a pre-installed one.

Regards,
Peter

> So if you're
> pulling in a prebuilt libfdt from elsewhere, you should be pulling in
> libfdt_env.h from the same elsewhere.
>
> So, a distro packaging libfdt must provide the libfdt_env.h it built
> with - which in this case I'd expect to be the standard/default
> version from the libfdt tree.
>
> --
> David Gibson                    | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
>                                 | _way_ _around_!
> http://www.ozlabs.org/~dgibson

Re: [Qemu-devel] [PATCH v2 1/1] configure: dtc: Probe for libfdt_env.h

[Peter Crosthwaite]

Ping!

We are stuck in limbo here and I would like to get either V1 or V2
(this patch) of this through.

Paolo,

If you are not happy with this patch, does V1 address your concerns
(which was its original intention).

Regards,
Peter

On Sun, Jun 2, 2013 at 9:13 AM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> HI David,
>
> On Sat, Jun 1, 2013 at 3:38 PM, David Gibson
> <david@gibson.dropbear.id.au> wrote:
>> On Fri, May 31, 2013 at 10:44:33AM +0100, Peter Maydell wrote:
>>> On 31 May 2013 09:25, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> > Please don't.  Fedora is not going to use the bundled dtc because of a
>>> > policy against bundling, and Fedora's dtc package doesn't include
>>> > libfdt_env.h.
>>>
>>> It sounds like Fedora's dtc package is broken then -- are you
>>> going to fix it?
>>>
>>> > If David says that the changes are rare enough, let's just fix the
>>> > bundled libfdt_env.h.
>>>
>>> ...I don't have an objection to this if we can make it
>>> work for all versions of libfdt, and if it's a way of
>>> using libfdt that upstream expects and is happy to support.
>>>
>>> Mostly I'd just like it to be clear how upstream expects us
>>> to use libfdt and how upstream expects distributions to
>>> package libfdt, and then make sure we're in line with that.
>>> It seems like a lot of the problem is that people (us and
>>> distros) have been guessing in the absence of documentation :-(
>>
>> Yeah.  Sorry.  Patches welcome..?
>>
>> The key point of confusion here is that it is the *builder* of libfdt
>> that's expected to provide libfdt_env.h, not the user.
>
> Then isn't my commit message right after all? This patch is not fixing
> QEMUs build of libfdt (submodule), it is fixing the case where it
> attempts to use a pre-installed one.
>
> Regards,
> Peter
>
>> So if you're
>> pulling in a prebuilt libfdt from elsewhere, you should be pulling in
>> libfdt_env.h from the same elsewhere.
>>
>> So, a distro packaging libfdt must provide the libfdt_env.h it built
>> with - which in this case I'd expect to be the standard/default
>> version from the libfdt tree.
>>
>> --
>> David Gibson                    | I'll have my music baroque, and my code
>> david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
>>                                 | _way_ _around_!
>> http://www.ozlabs.org/~dgibson

Re: [Qemu-devel] [PATCH v2 1/1] configure: dtc: Probe for libfdt_env.h

[Peter Maydell]

On 7 June 2013 03:54, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> Ping!
>
> We are stuck in limbo here and I would like to get either V1 or V2
> (this patch) of this through.
>
> Paolo,
>
> If you are not happy with this patch, does V1 address your concerns
> (which was its original intention).

Given what David has said I think this v2 is correct and
v1 is wrong. If a distro-supplied libfdt has to provide
a libfdt_env.h then we are correct to check for it before
use (with fallback to the submodule). If Fedora has a
requirement to use its packaged libfdt then they can
achieve that by fixing their libfdt package.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 1/1] configure: dtc: Probe for libfdt_env.h

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1559 bytes --]

On Fri, May 31, 2013 at 10:25:55AM +0200, Paolo Bonzini wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Il 31/05/2013 05:32, David Gibson ha scritto:
> >>> Then again, although we certainly want to keep changes which 
> >>> require updates to libfdt_env.h rare, I'm not going to rule
> >>> out extensions to libfdt which add new (minor) external 
> >>> dependencies, and therefore changes to libfdt_env.h.
> >>> 
> >>> That said, I did put my correction a bit too strongly.  While 
> >>> libfdt_env.h is notionally always provided by the surrounding 
> >>> environment, if the packaged one works for your environment, 
> >>> it's probably a good idea to use it.
> >> 
> >> I think we should be able to consider QEMU as normal enough to 
> >> use prepacked DTC and just fix the commit message?
> > 
> > I concur.
> 
> Please don't.  Fedora is not going to use the bundled dtc because of a
> policy against bundling, and Fedora's dtc package doesn't include
> libfdt_env.h.

Yeah, that makes Fedora's dtc (well, libfdt) package broken.

> If David says that the changes are rare enough, let's just fix the
> bundled libfdt_env.h.

That's definitely not right.  If you're using an externally packagd
build of libfdt, you must use that external package's libfdt_env.h as
well (which in this case is the default version).

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [Qemu-devel] [PATCH v2 1/1] configure: dtc: Probe for libfdt_env.h

[Peter Maydell]

On 1 June 2013 06:22, David Gibson <david@gibson.dropbear.id.au> wrote:
> On Fri, May 31, 2013 at 10:25:55AM +0200, Paolo Bonzini wrote:
>> Fedora's dtc package doesn't include
>> libfdt_env.h.
>
> Yeah, that makes Fedora's dtc (well, libfdt) package broken.
>
>> If David says that the changes are rare enough, let's just fix the
>> bundled libfdt_env.h.
>
> That's definitely not right.  If you're using an externally packagd
> build of libfdt, you must use that external package's libfdt_env.h as
> well (which in this case is the default version).

Debian/Ubuntu's packaged libfdt doesn't ship with libfdt_env.h
either. Is there anybody who's packaged libfdt who *has* provided
libfdt_env.h ?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 1/1] configure: dtc: Probe for libfdt_env.h

[Peter Crosthwaite]

Hi All,

On Sat, Jun 1, 2013 at 6:26 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 1 June 2013 06:22, David Gibson <david@gibson.dropbear.id.au> wrote:
>> On Fri, May 31, 2013 at 10:25:55AM +0200, Paolo Bonzini wrote:
>>> Fedora's dtc package doesn't include
>>> libfdt_env.h.
>>
>> Yeah, that makes Fedora's dtc (well, libfdt) package broken.
>>
>>> If David says that the changes are rare enough, let's just fix the
>>> bundled libfdt_env.h.
>>
>> That's definitely not right.  If you're using an externally packagd
>> build of libfdt, you must use that external package's libfdt_env.h as
>> well (which in this case is the default version).
>
> Debian/Ubuntu's packaged libfdt doesn't ship with libfdt_env.h
> either. Is there anybody who's packaged libfdt who *has* provided
> libfdt_env.h ?
>

Unlikely. The most recently tagged version of DTC does not have the
patch to the installer fixed.

This can be fixed long term by releasing an updated dtc - can we tag something?

The short-term fix is V1 of this patch, which plays nice with the
broken installs while not breaking modern dtc installed locally.

Regards,
Peter

> thanks
> -- PMM
>