Re: [RFC PATCH 4/5] hw/ppc/fdt: Drop dependency on libfdt
On 5/14/21 7:29 AM, Paolo Bonzini wrote: > On 14/05/21 01:35, David Gibson wrote: >>> "FDT" is set by meson.build if the library is available, LIBFDT is >>> set by >>> the board to link with the library. In other words CONFIG_FDT is >>> per-build, >>> while CONFIG_LIBFDT is per-target. >> Oof... that's highly non-obvious. Could we call it HAVE_LIBFDT and >> USE_LIBFDT instead? Just to be sure I understood correctly, for the next version I'll use HAVE_LIBFDT for the build-related logic (Meson) and USE_LIBFDT for the per-target dependencies (Kconfig). (Only reply if I misunderstood). > Certainly okay by me. > > Paolo >
Re: [RFC PATCH 4/5] hw/ppc/fdt: Drop dependency on libfdt
On 14/05/21 01:35, David Gibson wrote: "FDT" is set by meson.build if the library is available, LIBFDT is set by the board to link with the library. In other words CONFIG_FDT is per-build, while CONFIG_LIBFDT is per-target. Oof... that's highly non-obvious. Could we call it HAVE_LIBFDT and USE_LIBFDT instead? Certainly okay by me. Paolo
Re: [RFC PATCH 4/5] hw/ppc/fdt: Drop dependency on libfdt
On Thu, May 13, 2021 at 05:26:37PM +0200, Paolo Bonzini wrote: > On 13/05/21 05:46, David Gibson wrote: > > > The patch makes sense in general. The file is only needed for pseries and > > > powernv, not for e.g. e500 which does need fdt. > > > > Yes, agreed. > > > > > I would get rid of FDT_PPC completely. First, before patch 3, you can > > > move > > > fdt.c to PSERIES and POWERNV (it's too small to need its own Kconfig > > > symbol) > > > and only leave > > > > > > ppc_ss.add(when: 'CONFIG_FDT_PPC', if_true: fdt) > > > > Uh... why do we need even this? > > To tell Meson that a board requires QEMU to be linked with libfdt. This > symbol is then renamed to CONFIG_LIBFDT once it can be used with all targets > (rather than just hw/ppc). Oh, I thought CONFIG_LIBFDT already did that. > > > Since you are at it, remove the silly #ifdef TARGET_PPC64 in the > > > hw/ppc/fdt.c file. > > > > > > Then in patch 3: > > > > > > - add to Kconfig.host > > > > > > config FDT > > > bool > > > > > > config LIBFDT > > > bool > > > depends on FDT > > > > Um.. I'm not sure what semantic difference you're envisaging between > > FDT and LIBFDT. > > "FDT" is set by meson.build if the library is available, LIBFDT is set by > the board to link with the library. In other words CONFIG_FDT is per-build, > while CONFIG_LIBFDT is per-target. Oof... that's highly non-obvious. Could we call it HAVE_LIBFDT and USE_LIBFDT instead? > If a board selects LIBFDT but the library is not available, minikconf will > report a contradiction due to "CONFIG_PSERIES=y" -> "config PSERIES select > LIBFDT" -> "config LIBFDT depends on FDT" -> "CONFIG_FDT=n". > > > > - for all the boards I listed in my review, add "select LIBFDT" in > > > addition > > > to "depends on FDT". > > This is actually unnecessary---"depends on FDT" is not needed in the boards > because LIBFDT already has the dependency. > > Paolo > -- 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 signature.asc Description: PGP signature
Re: [RFC PATCH 4/5] hw/ppc/fdt: Drop dependency on libfdt
On 13/05/21 05:46, David Gibson wrote: The patch makes sense in general. The file is only needed for pseries and powernv, not for e.g. e500 which does need fdt. Yes, agreed. I would get rid of FDT_PPC completely. First, before patch 3, you can move fdt.c to PSERIES and POWERNV (it's too small to need its own Kconfig symbol) and only leave ppc_ss.add(when: 'CONFIG_FDT_PPC', if_true: fdt) Uh... why do we need even this? To tell Meson that a board requires QEMU to be linked with libfdt. This symbol is then renamed to CONFIG_LIBFDT once it can be used with all targets (rather than just hw/ppc). Since you are at it, remove the silly #ifdef TARGET_PPC64 in the hw/ppc/fdt.c file. Then in patch 3: - add to Kconfig.host config FDT bool config LIBFDT bool depends on FDT Um.. I'm not sure what semantic difference you're envisaging between FDT and LIBFDT. "FDT" is set by meson.build if the library is available, LIBFDT is set by the board to link with the library. In other words CONFIG_FDT is per-build, while CONFIG_LIBFDT is per-target. If a board selects LIBFDT but the library is not available, minikconf will report a contradiction due to "CONFIG_PSERIES=y" -> "config PSERIES select LIBFDT" -> "config LIBFDT depends on FDT" -> "CONFIG_FDT=n". - for all the boards I listed in my review, add "select LIBFDT" in addition to "depends on FDT". This is actually unnecessary---"depends on FDT" is not needed in the boards because LIBFDT already has the dependency. Paolo
Re: [RFC PATCH 4/5] hw/ppc/fdt: Drop dependency on libfdt
On Wed, May 12, 2021 at 09:59:00AM +0200, Paolo Bonzini wrote:
> On 12/05/21 04:30, David Gibson wrote:
> > On Tue, May 11, 2021 at 05:53:53PM +0200, Philippe Mathieu-Daudé wrote:
> > > hw/ppc/fdt.c defines the ppc_create_page_sizes_prop() function,
> > > which is unrelated to the libfdt. Remove the incorrect library
> > > dependency on the file.
> > >
> > > Signed-off-by: Philippe Mathieu-Daudé
> >
> > This is definitely wrong as it stands. AFAICT this doesn't add a
> > build of hw/ppc/fdt.c anywhere, but it is definitely needed by both
> > pseries and powernv machine types, who select FDT_PPC for this exact
> > reason.
> >
> > I will grant you that it is badly named. It is in fact related to
> > libfdt, just rather indirectly.
>
> The patch makes sense in general. The file is only needed for pseries and
> powernv, not for e.g. e500 which does need fdt.
Yes, agreed.
> I would get rid of FDT_PPC completely. First, before patch 3, you can move
> fdt.c to PSERIES and POWERNV (it's too small to need its own Kconfig symbol)
> and only leave
>
>ppc_ss.add(when: 'CONFIG_FDT_PPC', if_true: fdt)
Uh... why do we need even this?
> Since you are at it, remove the silly #ifdef TARGET_PPC64 in the
> hw/ppc/fdt.c file.
>
> Then in patch 3:
>
> - add to Kconfig.host
>
> config FDT
>bool
>
> config LIBFDT
>bool
>depends on FDT
Um.. I'm not sure what semantic difference you're envisaging between
FDT and LIBFDT.
> - for all the boards I listed in my review, add "select LIBFDT" in addition
> to "depends on FDT".
>
> - add to meson.build
>
> softmmu_ss.add(when: 'CONFIG_LIBFDT', if_true: fdt)
>
> Paolo
>
> > > ---
> > > hw/ppc/meson.build | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/hw/ppc/meson.build b/hw/ppc/meson.build
> > > index e82a6b4105b..580e6e42c8a 100644
> > > --- a/hw/ppc/meson.build
> > > +++ b/hw/ppc/meson.build
> > > @@ -3,9 +3,9 @@
> > > 'ppc.c',
> > > 'ppc_booke.c',
> > > ))
> > > -ppc_ss.add(when: 'CONFIG_FDT_PPC', if_true: [files(
> > > +ppc_ss.add(when: 'CONFIG_FDT_PPC', if_true: files(
> > > 'fdt.c',
> > > -), fdt])
> > > +))
> > > ppc_ss.add(when: 'CONFIG_FW_CFG_PPC', if_true: files('fw_cfg.c'))
> > > # IBM pSeries (sPAPR)
> >
>
--
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
signature.asc
Description: PGP signature
Re: [RFC PATCH 4/5] hw/ppc/fdt: Drop dependency on libfdt
On 12/05/21 04:30, David Gibson wrote:
On Tue, May 11, 2021 at 05:53:53PM +0200, Philippe Mathieu-Daudé wrote:
hw/ppc/fdt.c defines the ppc_create_page_sizes_prop() function,
which is unrelated to the libfdt. Remove the incorrect library
dependency on the file.
Signed-off-by: Philippe Mathieu-Daudé
This is definitely wrong as it stands. AFAICT this doesn't add a
build of hw/ppc/fdt.c anywhere, but it is definitely needed by both
pseries and powernv machine types, who select FDT_PPC for this exact
reason.
I will grant you that it is badly named. It is in fact related to
libfdt, just rather indirectly.
The patch makes sense in general. The file is only needed for pseries
and powernv, not for e.g. e500 which does need fdt.
I would get rid of FDT_PPC completely. First, before patch 3, you can
move fdt.c to PSERIES and POWERNV (it's too small to need its own
Kconfig symbol) and only leave
ppc_ss.add(when: 'CONFIG_FDT_PPC', if_true: fdt)
Since you are at it, remove the silly #ifdef TARGET_PPC64 in the
hw/ppc/fdt.c file.
Then in patch 3:
- add to Kconfig.host
config FDT
bool
config LIBFDT
bool
depends on FDT
- for all the boards I listed in my review, add "select LIBFDT" in
addition to "depends on FDT".
- add to meson.build
softmmu_ss.add(when: 'CONFIG_LIBFDT', if_true: fdt)
Paolo
---
hw/ppc/meson.build | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/ppc/meson.build b/hw/ppc/meson.build
index e82a6b4105b..580e6e42c8a 100644
--- a/hw/ppc/meson.build
+++ b/hw/ppc/meson.build
@@ -3,9 +3,9 @@
'ppc.c',
'ppc_booke.c',
))
-ppc_ss.add(when: 'CONFIG_FDT_PPC', if_true: [files(
+ppc_ss.add(when: 'CONFIG_FDT_PPC', if_true: files(
'fdt.c',
-), fdt])
+))
ppc_ss.add(when: 'CONFIG_FW_CFG_PPC', if_true: files('fw_cfg.c'))
# IBM pSeries (sPAPR)
Re: [RFC PATCH 4/5] hw/ppc/fdt: Drop dependency on libfdt
On Tue, May 11, 2021 at 05:53:53PM +0200, Philippe Mathieu-Daudé wrote:
> hw/ppc/fdt.c defines the ppc_create_page_sizes_prop() function,
> which is unrelated to the libfdt. Remove the incorrect library
> dependency on the file.
>
> Signed-off-by: Philippe Mathieu-Daudé
This is definitely wrong as it stands. AFAICT this doesn't add a
build of hw/ppc/fdt.c anywhere, but it is definitely needed by both
pseries and powernv machine types, who select FDT_PPC for this exact
reason.
I will grant you that it is badly named. It is in fact related to
libfdt, just rather indirectly.
> ---
> hw/ppc/meson.build | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/ppc/meson.build b/hw/ppc/meson.build
> index e82a6b4105b..580e6e42c8a 100644
> --- a/hw/ppc/meson.build
> +++ b/hw/ppc/meson.build
> @@ -3,9 +3,9 @@
>'ppc.c',
>'ppc_booke.c',
> ))
> -ppc_ss.add(when: 'CONFIG_FDT_PPC', if_true: [files(
> +ppc_ss.add(when: 'CONFIG_FDT_PPC', if_true: files(
>'fdt.c',
> -), fdt])
> +))
> ppc_ss.add(when: 'CONFIG_FW_CFG_PPC', if_true: files('fw_cfg.c'))
>
> # IBM pSeries (sPAPR)
--
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
signature.asc
Description: PGP signature
