default-configs/devices/ppc64-softmmu.mak | 1 - meson.build | 2 +- Kconfig.host | 3 +++ hw/arm/Kconfig | 1 + hw/i386/Kconfig | 1 + hw/mem/Kconfig | 2 -- hw/ppc/Kconfig | 2 ++ hw/ppc/meson.build | 8 ++++---- 8 files changed, 12 insertions(+), 8 deletions(-)
Attempt to fix the issue reported by John when building
with an outdated libfdt.
For now it changes:
hw/ppc/spapr_hcall.c: In function =E2=80=98h_update_dt=E2=80=99:
hw/ppc/spapr_hcall.c:1966:9: warning: implicit declaration of function =E2=
=80=98fdt_check_full=E2=80=99; did you mean =E2=80=98fdt_check_header=E2=80=
=99? [-Wimplicit-function-declaration]
1966 | if (fdt_check_full(fdt, cb)) {
| ^~~~~~~~~~~~~~
| fdt_check_header
hw/ppc/spapr_hcall.c:1966:9: warning: nested extern declaration of =E2=80=
=98fdt_check_full=E2=80=99 [-Wnested-externs]
[...]
/usr/bin/ld: libqemu-ppc64-softmmu.fa.p/hw_ppc_spapr_hcall.c.o: in function=
`h_update_dt':
hw/ppc/spapr_hcall.c:1966: undefined reference to `fdt_check_full'
collect2: error: ld returned 1 exit status
by:
qemu/meson.build:1352:4: ERROR: Running configure command failed.
The following clauses were found for PSERIES
CONFIG_PSERIES=3Dy
config PSERIES depends on FDT
which is not better, but one step at a time...
John said: https://gitlab.com/qemu-project/qemu/-/issues/255#note_572421108
Distributions usually don't used embedded copies of libraries,
so the configure script should require the correct minimum version.
Personally I'd rather allow users to build the most of QEMU with what is
available, that is all possible machines except pSeries, making pSeries
machine selected by default and deselected if not possible, with this
change:
-- >8 --
diff --git a/default-configs/devices/ppc64-softmmu.mak b/default-configs/devi=
ces/ppc64-softmmu.mak
index cca52665d90..62339661fca 100644
--- a/default-configs/devices/ppc64-softmmu.mak
+++ b/default-configs/devices/ppc64-softmmu.mak
@@ -5,6 +5,3 @@ include ppc-softmmu.mak
# For PowerNV
CONFIG_POWERNV=3Dy
-
-# For pSeries
-CONFIG_PSERIES=3Dy
diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
index 3935b73456f..706debd4fee 100644
--- a/hw/ppc/Kconfig
+++ b/hw/ppc/Kconfig
@@ -1,5 +1,6 @@
config PSERIES
bool
+ default y
depends on FDT
imply PCI_DEVICES
imply TEST_DEVICES
---
But I suppose it breaks user expectations.
Thoughts?
Regards,
Phil.
Philippe Mathieu-Daud=C3=A9 (5):
hw/mem/nvdimm: Use Kconfig 'imply' instead of 'depends on'
Kconfig: Declare 'FDT' host symbol
hw/ppc: Have pSeries depends on libfdt (via host Kconfig FDT symbol)
hw/ppc/fdt: Drop dependency on libfdt
meson: Do not use internal fdt library if user asked for the system
one
default-configs/devices/ppc64-softmmu.mak | 1 -
meson.build | 2 +-
Kconfig.host | 3 +++
hw/arm/Kconfig | 1 +
hw/i386/Kconfig | 1 +
hw/mem/Kconfig | 2 --
hw/ppc/Kconfig | 2 ++
hw/ppc/meson.build | 8 ++++----
8 files changed, 12 insertions(+), 8 deletions(-)
--=20
2.26.3
On 5/11/21 5:53 PM, Philippe Mathieu-Daudé wrote: > Attempt to fix the issue reported by John when building > with an outdated libfdt. Unencoded version of this cover: For now it changes: hw/ppc/spapr_hcall.c: In function ‘h_update_dt’: hw/ppc/spapr_hcall.c:1966:9: warning: implicit declaration of function ‘fdt_check_full’; did you mean ‘fdt_check_header’? [-Wimplicit-function-declaration] 1966 | if (fdt_check_full(fdt, cb)) { | ^~~~~~~~~~~~~~ | fdt_check_header hw/ppc/spapr_hcall.c:1966:9: warning: nested extern declaration of ‘fdt_check_full’ [-Wnested-externs] [...] /usr/bin/ld: libqemu-ppc64-softmmu.fa.p/hw_ppc_spapr_hcall.c.o: in function `h_update_dt': hw/ppc/spapr_hcall.c:1966: undefined reference to `fdt_check_full' collect2: error: ld returned 1 exit status by: qemu/meson.build:1352:4: ERROR: Running configure command failed. The following clauses were found for PSERIES CONFIG_PSERIES=y config PSERIES depends on FDT which is not better, but one step at a time... John said: https://gitlab.com/qemu-project/qemu/-/issues/255#note_572421108 Distributions usually don't used embedded copies of libraries, so the configure script should require the correct minimum version. Personally I'd rather allow users to build the most of QEMU with what is available, that is all possible machines except pSeries, making pSeries machine selected by default and deselected if not possible, with this change: -- >8 -- diff --git a/default-configs/devices/ppc64-softmmu.mak b/default-configs/devices/ppc64-softmmu.mak index cca52665d90..62339661fca 100644 --- a/default-configs/devices/ppc64-softmmu.mak +++ b/default-configs/devices/ppc64-softmmu.mak @@ -5,6 +5,3 @@ include ppc-softmmu.mak # For PowerNV CONFIG_POWERNV=y - -# For pSeries -CONFIG_PSERIES=y diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig index 3935b73456f..706debd4fee 100644 --- a/hw/ppc/Kconfig +++ b/hw/ppc/Kconfig @@ -1,5 +1,6 @@ config PSERIES bool + default y depends on FDT imply PCI_DEVICES imply TEST_DEVICES --- But I suppose it breaks user expectations. Thoughts? ;)
Hi Paolo, On 5/11/21 5:57 PM, Philippe Mathieu-Daudé wrote: > On 5/11/21 5:53 PM, Philippe Mathieu-Daudé wrote: >> Attempt to fix the issue reported by John when building >> with an outdated libfdt. > > Unencoded version of this cover: > > For now it changes: > > hw/ppc/spapr_hcall.c: In function ‘h_update_dt’: > hw/ppc/spapr_hcall.c:1966:9: warning: implicit declaration of function > ‘fdt_check_full’; did you mean ‘fdt_check_header’? > [-Wimplicit-function-declaration] > 1966 | if (fdt_check_full(fdt, cb)) { > | ^~~~~~~~~~~~~~ > | fdt_check_header > hw/ppc/spapr_hcall.c:1966:9: warning: nested extern declaration of > ‘fdt_check_full’ [-Wnested-externs] > [...] > /usr/bin/ld: libqemu-ppc64-softmmu.fa.p/hw_ppc_spapr_hcall.c.o: in > function `h_update_dt': > hw/ppc/spapr_hcall.c:1966: undefined reference to `fdt_check_full' > collect2: error: ld returned 1 exit status > > by: > > qemu/meson.build:1352:4: ERROR: Running configure command failed. > The following clauses were found for PSERIES > > CONFIG_PSERIES=y > config PSERIES depends on FDT > This is triggered with: fdt support: NO having: default-configs/targets/ppc64-softmmu.mak:6:TARGET_NEED_FDT=y So this code doesn't seem to work: if not fdt.found() and fdt_required.length() > 0 error('fdt not available but required by targets ' + ', '.join(fdt_required)) endif BTW I disagree FDT is target-dependent, it is machine-dependent IMO. > which is not better, but one step at a time... > > John said: https://gitlab.com/qemu-project/qemu/-/issues/255#note_572421108 > > Distributions usually don't used embedded copies of libraries, > so the configure script should require the correct minimum version. > > Personally I'd rather allow users to build the most of QEMU with what is > available, that is all possible machines except pSeries, making pSeries > machine selected by default and deselected if not possible, with this > change: > > -- >8 -- > diff --git a/default-configs/devices/ppc64-softmmu.mak > b/default-configs/devices/ppc64-softmmu.mak > index cca52665d90..62339661fca 100644 > --- a/default-configs/devices/ppc64-softmmu.mak > +++ b/default-configs/devices/ppc64-softmmu.mak > @@ -5,6 +5,3 @@ include ppc-softmmu.mak > > # For PowerNV > CONFIG_POWERNV=y > - > -# For pSeries > -CONFIG_PSERIES=y > diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig > index 3935b73456f..706debd4fee 100644 > --- a/hw/ppc/Kconfig > +++ b/hw/ppc/Kconfig > @@ -1,5 +1,6 @@ > config PSERIES > bool > + default y > depends on FDT > imply PCI_DEVICES > imply TEST_DEVICES > --- > > But I suppose it breaks user expectations. > > Thoughts? > > ;) >
On 12/05/21 05:56, Philippe Mathieu-Daudé wrote: >> qemu/meson.build:1352:4: ERROR: Running configure command failed. >> The following clauses were found for PSERIES >> >> CONFIG_PSERIES=y >> config PSERIES depends on FDT >> > This is triggered with: > > fdt support: NO > > having: > > default-configs/targets/ppc64-softmmu.mak:6:TARGET_NEED_FDT=y > > So this code doesn't seem to work: > > if not fdt.found() and fdt_required.length() > 0 > error('fdt not available but required by targets ' + ', > '.join(fdt_required)) > endif > > BTW I disagree FDT is target-dependent, it is machine-dependent IMO. > I agree. It is much better to depend on FDT consistently for machines that require it. Paolo
Per the kconfig.rst:
A device should be listed [...] ``imply`` if (depending on
the QEMU command line) the board may or may not be started
without it.
This is the case with the NVDIMM device, so use the 'imply'
weak reverse dependency to select the symbol.
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
default-configs/devices/ppc64-softmmu.mak | 1 -
hw/arm/Kconfig | 1 +
hw/i386/Kconfig | 1 +
hw/mem/Kconfig | 2 --
hw/ppc/Kconfig | 1 +
5 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/default-configs/devices/ppc64-softmmu.mak b/default-configs/devices/ppc64-softmmu.mak
index ae0841fa3a1..cca52665d90 100644
--- a/default-configs/devices/ppc64-softmmu.mak
+++ b/default-configs/devices/ppc64-softmmu.mak
@@ -8,4 +8,3 @@ CONFIG_POWERNV=y
# For pSeries
CONFIG_PSERIES=y
-CONFIG_NVDIMM=y
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index b887f6a5b17..67723d9ea6a 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -6,6 +6,7 @@ config ARM_VIRT
imply VFIO_PLATFORM
imply VFIO_XGMAC
imply TPM_TIS_SYSBUS
+ imply NVDIMM
select ARM_GIC
select ACPI
select ARM_SMMUV3
diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig
index 7f91f30877f..66838fa397b 100644
--- a/hw/i386/Kconfig
+++ b/hw/i386/Kconfig
@@ -23,6 +23,7 @@ config PC
imply TPM_TIS_ISA
imply VGA_PCI
imply VIRTIO_VGA
+ imply NVDIMM
select FDC
select I8259
select I8254
diff --git a/hw/mem/Kconfig b/hw/mem/Kconfig
index a0ef2cf648e..8b19fdc49f1 100644
--- a/hw/mem/Kconfig
+++ b/hw/mem/Kconfig
@@ -7,6 +7,4 @@ config MEM_DEVICE
config NVDIMM
bool
- default y
- depends on (PC || PSERIES || ARM_VIRT)
select MEM_DEVICE
diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
index e51e0e5e5ac..66e0b15d9ef 100644
--- a/hw/ppc/Kconfig
+++ b/hw/ppc/Kconfig
@@ -3,6 +3,7 @@ config PSERIES
imply PCI_DEVICES
imply TEST_DEVICES
imply VIRTIO_VGA
+ imply NVDIMM
select DIMM
select PCI
select SPAPR_VSCSI
--
2.26.3
On Tue, May 11, 2021 at 05:53:50PM +0200, Philippe Mathieu-Daudé wrote: > Per the kconfig.rst: > > A device should be listed [...] ``imply`` if (depending on > the QEMU command line) the board may or may not be started > without it. > > This is the case with the NVDIMM device, so use the 'imply' > weak reverse dependency to select the symbol. Uh.. It should definitely be possible to start a pseries machine without NVDIMM. I would have guessed the same for PC. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > default-configs/devices/ppc64-softmmu.mak | 1 - > hw/arm/Kconfig | 1 + > hw/i386/Kconfig | 1 + > hw/mem/Kconfig | 2 -- > hw/ppc/Kconfig | 1 + > 5 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/default-configs/devices/ppc64-softmmu.mak b/default-configs/devices/ppc64-softmmu.mak > index ae0841fa3a1..cca52665d90 100644 > --- a/default-configs/devices/ppc64-softmmu.mak > +++ b/default-configs/devices/ppc64-softmmu.mak > @@ -8,4 +8,3 @@ CONFIG_POWERNV=y > > # For pSeries > CONFIG_PSERIES=y > -CONFIG_NVDIMM=y > diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig > index b887f6a5b17..67723d9ea6a 100644 > --- a/hw/arm/Kconfig > +++ b/hw/arm/Kconfig > @@ -6,6 +6,7 @@ config ARM_VIRT > imply VFIO_PLATFORM > imply VFIO_XGMAC > imply TPM_TIS_SYSBUS > + imply NVDIMM > select ARM_GIC > select ACPI > select ARM_SMMUV3 > diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig > index 7f91f30877f..66838fa397b 100644 > --- a/hw/i386/Kconfig > +++ b/hw/i386/Kconfig > @@ -23,6 +23,7 @@ config PC > imply TPM_TIS_ISA > imply VGA_PCI > imply VIRTIO_VGA > + imply NVDIMM > select FDC > select I8259 > select I8254 > diff --git a/hw/mem/Kconfig b/hw/mem/Kconfig > index a0ef2cf648e..8b19fdc49f1 100644 > --- a/hw/mem/Kconfig > +++ b/hw/mem/Kconfig > @@ -7,6 +7,4 @@ config MEM_DEVICE > > config NVDIMM > bool > - default y > - depends on (PC || PSERIES || ARM_VIRT) > select MEM_DEVICE > diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig > index e51e0e5e5ac..66e0b15d9ef 100644 > --- a/hw/ppc/Kconfig > +++ b/hw/ppc/Kconfig > @@ -3,6 +3,7 @@ config PSERIES > imply PCI_DEVICES > imply TEST_DEVICES > imply VIRTIO_VGA > + imply NVDIMM > select DIMM > select PCI > select SPAPR_VSCSI -- 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
On 5/12/21 4:24 AM, David Gibson wrote: > On Tue, May 11, 2021 at 05:53:50PM +0200, Philippe Mathieu-Daudé wrote: >> Per the kconfig.rst: >> >> A device should be listed [...] ``imply`` if (depending on >> the QEMU command line) the board may or may not be started >> without it. >> >> This is the case with the NVDIMM device, so use the 'imply' >> weak reverse dependency to select the symbol. > > Uh.. It should definitely be possible to start a pseries machine > without NVDIMM. I would have guessed the same for PC. Yes, this is what this patch does. With it we can build with: CONFIG_NVDIMM=n > >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> default-configs/devices/ppc64-softmmu.mak | 1 - >> hw/arm/Kconfig | 1 + >> hw/i386/Kconfig | 1 + >> hw/mem/Kconfig | 2 -- >> hw/ppc/Kconfig | 1 + >> 5 files changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/default-configs/devices/ppc64-softmmu.mak b/default-configs/devices/ppc64-softmmu.mak >> index ae0841fa3a1..cca52665d90 100644 >> --- a/default-configs/devices/ppc64-softmmu.mak >> +++ b/default-configs/devices/ppc64-softmmu.mak >> @@ -8,4 +8,3 @@ CONFIG_POWERNV=y >> >> # For pSeries >> CONFIG_PSERIES=y >> -CONFIG_NVDIMM=y >> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig >> index b887f6a5b17..67723d9ea6a 100644 >> --- a/hw/arm/Kconfig >> +++ b/hw/arm/Kconfig >> @@ -6,6 +6,7 @@ config ARM_VIRT >> imply VFIO_PLATFORM >> imply VFIO_XGMAC >> imply TPM_TIS_SYSBUS >> + imply NVDIMM >> select ARM_GIC >> select ACPI >> select ARM_SMMUV3 >> diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig >> index 7f91f30877f..66838fa397b 100644 >> --- a/hw/i386/Kconfig >> +++ b/hw/i386/Kconfig >> @@ -23,6 +23,7 @@ config PC >> imply TPM_TIS_ISA >> imply VGA_PCI >> imply VIRTIO_VGA >> + imply NVDIMM >> select FDC >> select I8259 >> select I8254 >> diff --git a/hw/mem/Kconfig b/hw/mem/Kconfig >> index a0ef2cf648e..8b19fdc49f1 100644 >> --- a/hw/mem/Kconfig >> +++ b/hw/mem/Kconfig >> @@ -7,6 +7,4 @@ config MEM_DEVICE >> >> config NVDIMM >> bool >> - default y >> - depends on (PC || PSERIES || ARM_VIRT) >> select MEM_DEVICE >> diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig >> index e51e0e5e5ac..66e0b15d9ef 100644 >> --- a/hw/ppc/Kconfig >> +++ b/hw/ppc/Kconfig >> @@ -3,6 +3,7 @@ config PSERIES >> imply PCI_DEVICES >> imply TEST_DEVICES >> imply VIRTIO_VGA >> + imply NVDIMM >> select DIMM >> select PCI >> select SPAPR_VSCSI >
On 12/05/2021 05.57, Philippe Mathieu-Daudé wrote: > On 5/12/21 4:24 AM, David Gibson wrote: >> On Tue, May 11, 2021 at 05:53:50PM +0200, Philippe Mathieu-Daudé wrote: >>> Per the kconfig.rst: >>> >>> A device should be listed [...] ``imply`` if (depending on >>> the QEMU command line) the board may or may not be started >>> without it. >>> >>> This is the case with the NVDIMM device, so use the 'imply' >>> weak reverse dependency to select the symbol. >> >> Uh.. It should definitely be possible to start a pseries machine >> without NVDIMM. I would have guessed the same for PC. > > Yes, this is what this patch does. With it we can build with: > CONFIG_NVDIMM=n But with "imply" you could end up with a PSERIES that does not have NVDIMM when also using --without-default-devices, couldn't you? Why don't you use "select" instead of "imply" ? Thomas
On 5/12/21 6:53 AM, Thomas Huth wrote: > On 12/05/2021 05.57, Philippe Mathieu-Daudé wrote: >> On 5/12/21 4:24 AM, David Gibson wrote: >>> On Tue, May 11, 2021 at 05:53:50PM +0200, Philippe Mathieu-Daudé wrote: >>>> Per the kconfig.rst: >>>> >>>> A device should be listed [...] ``imply`` if (depending on >>>> the QEMU command line) the board may or may not be started >>>> without it. >>>> >>>> This is the case with the NVDIMM device, so use the 'imply' >>>> weak reverse dependency to select the symbol. >>> >>> Uh.. It should definitely be possible to start a pseries machine >>> without NVDIMM. I would have guessed the same for PC. >> >> Yes, this is what this patch does. With it we can build with: >> CONFIG_NVDIMM=n > > But with "imply" you could end up with a PSERIES that does not have > NVDIMM when also using --without-default-devices, couldn't you? Not currently, because of the CONFIG_NVDIMM=y. > Why don't you use "select" instead of "imply" ? Because as David said earlier: "It should definitely be possible to start a pseries machine without NVDIMM."
On 12/05/2021 07.08, Philippe Mathieu-Daudé wrote: > On 5/12/21 6:53 AM, Thomas Huth wrote: >> On 12/05/2021 05.57, Philippe Mathieu-Daudé wrote: >>> On 5/12/21 4:24 AM, David Gibson wrote: >>>> On Tue, May 11, 2021 at 05:53:50PM +0200, Philippe Mathieu-Daudé wrote: >>>>> Per the kconfig.rst: >>>>> >>>>> A device should be listed [...] ``imply`` if (depending on >>>>> the QEMU command line) the board may or may not be started >>>>> without it. >>>>> >>>>> This is the case with the NVDIMM device, so use the 'imply' >>>>> weak reverse dependency to select the symbol. >>>> >>>> Uh.. It should definitely be possible to start a pseries machine >>>> without NVDIMM. I would have guessed the same for PC. >>> >>> Yes, this is what this patch does. With it we can build with: >>> CONFIG_NVDIMM=n >> >> But with "imply" you could end up with a PSERIES that does not have >> NVDIMM when also using --without-default-devices, couldn't you? > > Not currently, because of the CONFIG_NVDIMM=y. > >> Why don't you use "select" instead of "imply" ? > > Because as David said earlier: > > "It should definitely be possible to start a pseries machine > without NVDIMM." Oops, sorry, looks like I did not have enough coffee yet and misread David's comment ... patch looks fine, indeed. Reviewed-by: Thomas Huth <thuth@redhat.com>
On Wed, May 12, 2021 at 06:53:22AM +0200, Thomas Huth wrote: > On 12/05/2021 05.57, Philippe Mathieu-Daudé wrote: > > On 5/12/21 4:24 AM, David Gibson wrote: > > > On Tue, May 11, 2021 at 05:53:50PM +0200, Philippe Mathieu-Daudé wrote: > > > > Per the kconfig.rst: > > > > > > > > A device should be listed [...] ``imply`` if (depending on > > > > the QEMU command line) the board may or may not be started > > > > without it. > > > > > > > > This is the case with the NVDIMM device, so use the 'imply' > > > > weak reverse dependency to select the symbol. > > > > > > Uh.. It should definitely be possible to start a pseries machine > > > without NVDIMM. I would have guessed the same for PC. > > > > Yes, this is what this patch does. With it we can build with: > > CONFIG_NVDIMM=n > > But with "imply" you could end up with a PSERIES that does not have NVDIMM > when also using --without-default-devices, couldn't you? Why don't you use > "select" instead of "imply" ? Oh.. clearly I misunderstand the semantics of "imply". If we don't need NVDIMM for PSERIES, why does there need to be any Kconfig connection between them at all? -- 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
On 12/05/21 09:02, David Gibson wrote: >> But with "imply" you could end up with a PSERIES that does not have NVDIMM >> when also using --without-default-devices, couldn't you? Why don't you use >> "select" instead of "imply" ? > Oh.. clearly I misunderstand the semantics of "imply". If we don't > need NVDIMM for PSERIES, why does there need to be any Kconfig > connection between them at all? Because you still want it in the binary by default (i.e. unless --without-default-devices). Basically, config PSERIES imply NVDIMM is the same as config NVDIMM default y if PSERIES Both of them are a way to say "PSERIES can work with NVDIMM so you want to include it unless you want some fine tuning". In Linux "imply" is very rarely used, while in QEMU it's quite common because it keeps the many per-board defaults close together. Paolo
On 11/05/21 17:53, Philippe Mathieu-Daudé wrote: > Per the kconfig.rst: > > A device should be listed [...] ``imply`` if (depending on > the QEMU command line) the board may or may not be started > without it. > > This is the case with the NVDIMM device, so use the 'imply' > weak reverse dependency to select the symbol. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> Unrelated to the rest, so I've queued this one. Paolo > --- > default-configs/devices/ppc64-softmmu.mak | 1 - > hw/arm/Kconfig | 1 + > hw/i386/Kconfig | 1 + > hw/mem/Kconfig | 2 -- > hw/ppc/Kconfig | 1 + > 5 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/default-configs/devices/ppc64-softmmu.mak b/default-configs/devices/ppc64-softmmu.mak > index ae0841fa3a1..cca52665d90 100644 > --- a/default-configs/devices/ppc64-softmmu.mak > +++ b/default-configs/devices/ppc64-softmmu.mak > @@ -8,4 +8,3 @@ CONFIG_POWERNV=y > > # For pSeries > CONFIG_PSERIES=y > -CONFIG_NVDIMM=y > diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig > index b887f6a5b17..67723d9ea6a 100644 > --- a/hw/arm/Kconfig > +++ b/hw/arm/Kconfig > @@ -6,6 +6,7 @@ config ARM_VIRT > imply VFIO_PLATFORM > imply VFIO_XGMAC > imply TPM_TIS_SYSBUS > + imply NVDIMM > select ARM_GIC > select ACPI > select ARM_SMMUV3 > diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig > index 7f91f30877f..66838fa397b 100644 > --- a/hw/i386/Kconfig > +++ b/hw/i386/Kconfig > @@ -23,6 +23,7 @@ config PC > imply TPM_TIS_ISA > imply VGA_PCI > imply VIRTIO_VGA > + imply NVDIMM > select FDC > select I8259 > select I8254 > diff --git a/hw/mem/Kconfig b/hw/mem/Kconfig > index a0ef2cf648e..8b19fdc49f1 100644 > --- a/hw/mem/Kconfig > +++ b/hw/mem/Kconfig > @@ -7,6 +7,4 @@ config MEM_DEVICE > > config NVDIMM > bool > - default y > - depends on (PC || PSERIES || ARM_VIRT) > select MEM_DEVICE > diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig > index e51e0e5e5ac..66e0b15d9ef 100644 > --- a/hw/ppc/Kconfig > +++ b/hw/ppc/Kconfig > @@ -3,6 +3,7 @@ config PSERIES > imply PCI_DEVICES > imply TEST_DEVICES > imply VIRTIO_VGA > + imply NVDIMM > select DIMM > select PCI > select SPAPR_VSCSI >
The CONFIG_FDT symbol depends on the availability of the
fdt library on the host. To be able to have other symbols
depends on it, declare it symbol in Kconfig.host.
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
Kconfig.host | 3 +++
1 file changed, 3 insertions(+)
diff --git a/Kconfig.host b/Kconfig.host
index 24255ef4419..0a512696865 100644
--- a/Kconfig.host
+++ b/Kconfig.host
@@ -41,3 +41,6 @@ config PVRDMA
config MULTIPROCESS_ALLOWED
bool
imply MULTIPROCESS
+
+config FDT
+ bool
--
2.26.3
On 11/05/21 17:53, Philippe Mathieu-Daudé wrote: > The CONFIG_FDT symbol depends on the availability of the > fdt library on the host. To be able to have other symbols > depends on it, declare it symbol in Kconfig.host. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > Kconfig.host | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/Kconfig.host b/Kconfig.host > index 24255ef4419..0a512696865 100644 > --- a/Kconfig.host > +++ b/Kconfig.host > @@ -41,3 +41,6 @@ config PVRDMA > config MULTIPROCESS_ALLOWED > bool > imply MULTIPROCESS > + > +config FDT > + bool > You need to add it to host_kconfig as well, don't you? Paolo
Since commit fea35ca4b8e ("ppc/spapr: Receive and store device
tree blob from SLOF") the pSeries machine depends on the libfdt
fdt_check_full() call, which is available in libfdt v1.4.7.
Add the corresponding Kconfig dependency.
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
hw/ppc/Kconfig | 1 +
hw/ppc/meson.build | 4 ++--
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
index 66e0b15d9ef..3935b73456f 100644
--- a/hw/ppc/Kconfig
+++ b/hw/ppc/Kconfig
@@ -1,5 +1,6 @@
config PSERIES
bool
+ depends on FDT
imply PCI_DEVICES
imply TEST_DEVICES
imply VIRTIO_VGA
diff --git a/hw/ppc/meson.build b/hw/ppc/meson.build
index 86d6f379d1c..e82a6b4105b 100644
--- a/hw/ppc/meson.build
+++ b/hw/ppc/meson.build
@@ -9,7 +9,7 @@
ppc_ss.add(when: 'CONFIG_FW_CFG_PPC', if_true: files('fw_cfg.c'))
# IBM pSeries (sPAPR)
-ppc_ss.add(when: 'CONFIG_PSERIES', if_true: files(
+ppc_ss.add(when: 'CONFIG_PSERIES', if_true: [files(
'spapr.c',
'spapr_caps.c',
'spapr_vio.c',
@@ -28,7 +28,7 @@
'spapr_rtas_ddw.c',
'spapr_numa.c',
'pef.c',
-))
+), fdt])
ppc_ss.add(when: 'CONFIG_SPAPR_RNG', if_true: files('spapr_rng.c'))
ppc_ss.add(when: ['CONFIG_PSERIES', 'CONFIG_LINUX'], if_true: files(
'spapr_pci_vfio.c',
--
2.26.3
On Tue, May 11, 2021 at 05:53:52PM +0200, Philippe Mathieu-Daudé wrote: > Since commit fea35ca4b8e ("ppc/spapr: Receive and store device > tree blob from SLOF") the pSeries machine depends on the libfdt > fdt_check_full() call, which is available in libfdt v1.4.7. > > Add the corresponding Kconfig dependency. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> I don't love making this conditional. Pseries is by far the best tested and most widely used ppc machine type, so it seems like it would break expectations to not compile this in rather than giving an error saying you need a newer libfdt. > --- > hw/ppc/Kconfig | 1 + > hw/ppc/meson.build | 4 ++-- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig > index 66e0b15d9ef..3935b73456f 100644 > --- a/hw/ppc/Kconfig > +++ b/hw/ppc/Kconfig > @@ -1,5 +1,6 @@ > config PSERIES > bool > + depends on FDT > imply PCI_DEVICES > imply TEST_DEVICES > imply VIRTIO_VGA > diff --git a/hw/ppc/meson.build b/hw/ppc/meson.build > index 86d6f379d1c..e82a6b4105b 100644 > --- a/hw/ppc/meson.build > +++ b/hw/ppc/meson.build > @@ -9,7 +9,7 @@ > ppc_ss.add(when: 'CONFIG_FW_CFG_PPC', if_true: files('fw_cfg.c')) > > # IBM pSeries (sPAPR) > -ppc_ss.add(when: 'CONFIG_PSERIES', if_true: files( > +ppc_ss.add(when: 'CONFIG_PSERIES', if_true: [files( > 'spapr.c', > 'spapr_caps.c', > 'spapr_vio.c', > @@ -28,7 +28,7 @@ > 'spapr_rtas_ddw.c', > 'spapr_numa.c', > 'pef.c', > -)) > +), fdt]) > ppc_ss.add(when: 'CONFIG_SPAPR_RNG', if_true: files('spapr_rng.c')) > ppc_ss.add(when: ['CONFIG_PSERIES', 'CONFIG_LINUX'], if_true: files( > 'spapr_pci_vfio.c', -- 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
On 12/05/21 04:27, David Gibson wrote: > On Tue, May 11, 2021 at 05:53:52PM +0200, Philippe Mathieu-Daudé wrote: >> Since commit fea35ca4b8e ("ppc/spapr: Receive and store device >> tree blob from SLOF") the pSeries machine depends on the libfdt >> fdt_check_full() call, which is available in libfdt v1.4.7. >> >> Add the corresponding Kconfig dependency. >> >> Signed-off-by: Philippe Mathieu-Daudé<philmd@redhat.com> > I don't love making this conditional. Pseries is by far the best > tested and most widely used ppc machine type, so it seems like it > would break expectations to not compile this in rather than giving an > error saying you need a newer libfdt. It's not conditional; if libfdt is not found, scripts/minikconf.py will tell you about the contradiction between CONFIG_PSERIES=y and "CONFIG_PSERIES depends on FDT". So we still have the same "fdt_required" logic that is already in meson.build, but expressed as Kconfig rules instead of a random line in default-configs/targets. Paolo
On Wed, May 12, 2021 at 10:01:20AM +0200, Paolo Bonzini wrote: > On 12/05/21 04:27, David Gibson wrote: > > On Tue, May 11, 2021 at 05:53:52PM +0200, Philippe Mathieu-Daudé wrote: > > > Since commit fea35ca4b8e ("ppc/spapr: Receive and store device > > > tree blob from SLOF") the pSeries machine depends on the libfdt > > > fdt_check_full() call, which is available in libfdt v1.4.7. > > > > > > Add the corresponding Kconfig dependency. > > > > > > Signed-off-by: Philippe Mathieu-Daudé<philmd@redhat.com> > > I don't love making this conditional. Pseries is by far the best > > tested and most widely used ppc machine type, so it seems like it > > would break expectations to not compile this in rather than giving an > > error saying you need a newer libfdt. > > It's not conditional; if libfdt is not found, scripts/minikconf.py will tell > you about the contradiction between CONFIG_PSERIES=y and "CONFIG_PSERIES > depends on FDT". > > So we still have the same "fdt_required" logic that is already in > meson.build, but expressed as Kconfig rules instead of a random line in > default-configs/targets. Oh, ok. -- 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
On 11/05/21 17:53, Philippe Mathieu-Daudé wrote: > Since commit fea35ca4b8e ("ppc/spapr: Receive and store device > tree blob from SLOF") the pSeries machine depends on the libfdt > fdt_check_full() call, which is available in libfdt v1.4.7. > > Add the corresponding Kconfig dependency. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> This is not the only one though. In particular, there should be a "depends on" also for MIPS_BOSTON (hw/mips), E500 (hw/ppc), POWERNV, PPC440 (hw/ppc), (hw/ppc), SAM460EX (hw/ppc), VIRTEX (hw/ppc), RX_GDBSIM (hw/rx), XTENSA_XTFPGA (hw/xtensa). Once you do this, TARGET_NEED_FDT can go away for PPC, MIPS and. The remaining ones use fdt functions in hw/*/boot.c so they need libfdt unconditionally RX (and TARGET_NEED_FDT should be added to default-configs/targets/nios2-softmmu.mak for the same reason). Paolo > --- > hw/ppc/Kconfig | 1 + > hw/ppc/meson.build | 4 ++-- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig > index 66e0b15d9ef..3935b73456f 100644 > --- a/hw/ppc/Kconfig > +++ b/hw/ppc/Kconfig > @@ -1,5 +1,6 @@ > config PSERIES > bool > + depends on FDT > imply PCI_DEVICES > imply TEST_DEVICES > imply VIRTIO_VGA > diff --git a/hw/ppc/meson.build b/hw/ppc/meson.build > index 86d6f379d1c..e82a6b4105b 100644 > --- a/hw/ppc/meson.build > +++ b/hw/ppc/meson.build > @@ -9,7 +9,7 @@ > ppc_ss.add(when: 'CONFIG_FW_CFG_PPC', if_true: files('fw_cfg.c')) > > # IBM pSeries (sPAPR) > -ppc_ss.add(when: 'CONFIG_PSERIES', if_true: files( > +ppc_ss.add(when: 'CONFIG_PSERIES', if_true: [files( > 'spapr.c', > 'spapr_caps.c', > 'spapr_vio.c', > @@ -28,7 +28,7 @@ > 'spapr_rtas_ddw.c', > 'spapr_numa.c', > 'pef.c', > -)) > +), fdt]) > ppc_ss.add(when: 'CONFIG_SPAPR_RNG', if_true: files('spapr_rng.c')) > ppc_ss.add(when: ['CONFIG_PSERIES', 'CONFIG_LINUX'], if_true: files( > 'spapr_pci_vfio.c', >
On 5/12/21 9:45 AM, Paolo Bonzini wrote: > On 11/05/21 17:53, Philippe Mathieu-Daudé wrote: >> Since commit fea35ca4b8e ("ppc/spapr: Receive and store device >> tree blob from SLOF") the pSeries machine depends on the libfdt >> fdt_check_full() call, which is available in libfdt v1.4.7. >> >> Add the corresponding Kconfig dependency. >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > This is not the only one though. In particular, there should be a > "depends on" also for MIPS_BOSTON (hw/mips), E500 (hw/ppc), POWERNV, > PPC440 (hw/ppc), (hw/ppc), SAM460EX (hw/ppc), VIRTEX (hw/ppc), RX_GDBSIM > (hw/rx), XTENSA_XTFPGA (hw/xtensa). > > Once you do this, TARGET_NEED_FDT can go away for PPC, MIPS and. The > remaining ones use fdt functions in hw/*/boot.c so they need libfdt > unconditionally RX (and TARGET_NEED_FDT should be added to > default-configs/targets/nios2-softmmu.mak for the same reason). Got it, thanks!
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é <philmd@redhat.com>
---
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)
--
2.26.3
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é <philmd@redhat.com> 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
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é <philmd@redhat.com> > > 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) >
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é <philmd@redhat.com> > > > > 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
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
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
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
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 >
If the user explicitly asked for the system libfdt library, but
the library is not usable (usually too old), we should not silently
default to the internal one.
Respect the user decision, and only default to 'internal' if in
auto mode.
Fixes: fbb4121d592 ("dtc: Convert Makefile bits to meson bits")
Reported-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
BugLink: https://bugs.launchpad.net/qemu/+bug/1907427
Fixes: https://gitlab.com/qemu-project/qemu/-/issues/255
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
meson.build | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/meson.build b/meson.build
index 0b41ff41188..1ffb4bccdb2 100644
--- a/meson.build
+++ b/meson.build
@@ -1612,7 +1612,7 @@
int main(void) { fdt_check_full(NULL, 0); return 0; }''',
dependencies: fdt)
fdt_opt = 'system'
- elif have_internal
+ elif have_internal and fdt_opt in ['enabled', 'auto']
fdt_opt = 'internal'
else
fdt_opt = 'disabled'
--
2.26.3
On 11/05/21 17:53, Philippe Mathieu-Daudé wrote: > diff --git a/meson.build b/meson.build > index 0b41ff41188..1ffb4bccdb2 100644 > --- a/meson.build > +++ b/meson.build > @@ -1612,7 +1612,7 @@ > int main(void) { fdt_check_full(NULL, 0); return 0; }''', > dependencies: fdt) > fdt_opt = 'system' > - elif have_internal > + elif have_internal and fdt_opt in ['enabled', 'auto'] > fdt_opt = 'internal' > else This will disable FDT silently for --enable-fdt=system instead of failing the build. What about: diff --git a/meson.build b/meson.build index 60040cd7cf..efb38ca780 100644 --- a/meson.build +++ b/meson.build @@ -1610,11 +1610,18 @@ if have_system fdt = cc.find_library('fdt', kwargs: static_kwargs, required: fdt_opt == 'system' or fdt_opt == 'enabled' and not have_internal) - if fdt.found() and cc.links(''' + if fdt.found() and not cc.links(''' #include <libfdt.h> #include <libfdt_env.h> int main(void) { fdt_check_full(NULL, 0); return 0; }''', dependencies: fdt) + if fdt_opt == 'system' or + fdt_opt == 'enabled' and not have_internal then + error('libfdt is too old, version 1.5.1 required') + endif + fdt = not_found + endif + if fdt.found() fdt_opt = 'system' elif have_internal fdt_opt = 'internal' Paolo
© 2016 - 2024 Red Hat, Inc.