[RFC PATCH 0/5] buildsys: Do not use internal fdt library when asked for the system one

[RFC PATCH 0/5] buildsys: Do not use internal fdt library when asked for the system one

[Philippe Mathieu-Daudé]

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

[RFC PATCH 1/5] hw/mem/nvdimm: Use Kconfig 'imply' instead of 'depends on'

[Philippe Mathieu-Daudé]

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

[RFC PATCH 2/5] Kconfig: Declare 'FDT' host symbol

[Philippe Mathieu-Daudé]

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

[RFC PATCH 3/5] hw/ppc: Have pSeries depends on libfdt (via host Kconfig FDT symbol)

[Philippe Mathieu-Daudé]

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

[RFC PATCH 4/5] hw/ppc/fdt: Drop dependency on libfdt

[Philippe Mathieu-Daudé]

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

[RFC PATCH 5/5] meson: Do not use internal fdt library if user asked for the system one

[Philippe Mathieu-Daudé]

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

Re: [RFC PATCH 0/5] buildsys: Do not use internal fdt library when asked for the system one

[Philippe Mathieu-Daudé]

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?

;)

Re: [RFC PATCH 1/5] hw/mem/nvdimm: Use Kconfig 'imply' instead of 'depends on'

[David Gibson]

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH 3/5] hw/ppc: Have pSeries depends on libfdt (via host Kconfig FDT symbol)

[David Gibson]

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH 4/5] hw/ppc/fdt: Drop dependency on libfdt

[David Gibson]

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH 0/5] buildsys: Do not use internal fdt library when asked for the system one

[Philippe Mathieu-Daudé]

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?
> 
> ;)
> 

Re: [RFC PATCH 1/5] hw/mem/nvdimm: Use Kconfig 'imply' instead of 'depends on'

[Philippe Mathieu-Daudé]

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
> 

Re: [RFC PATCH 1/5] hw/mem/nvdimm: Use Kconfig 'imply' instead of 'depends on'

[Thomas Huth]

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

Re: [RFC PATCH 1/5] hw/mem/nvdimm: Use Kconfig 'imply' instead of 'depends on'

[Philippe Mathieu-Daudé]

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

Re: [RFC PATCH 1/5] hw/mem/nvdimm: Use Kconfig 'imply' instead of 'depends on'

[Thomas Huth]

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>

Re: [RFC PATCH 1/5] hw/mem/nvdimm: Use Kconfig 'imply' instead of 'depends on'

[David Gibson]

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH 5/5] meson: Do not use internal fdt library if user asked for the system one

[Paolo Bonzini]

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

Re: [RFC PATCH 0/5] buildsys: Do not use internal fdt library when asked for the system one

[Paolo Bonzini]

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

Re: [RFC PATCH 2/5] Kconfig: Declare 'FDT' host symbol

[Paolo Bonzini]

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

Re: [RFC PATCH 3/5] hw/ppc: Have pSeries depends on libfdt (via host Kconfig FDT symbol)

[Paolo Bonzini]

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',
> 

Re: [RFC PATCH 4/5] hw/ppc/fdt: Drop dependency on libfdt

[Paolo Bonzini]

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)
> 

Re: [RFC PATCH 3/5] hw/ppc: Have pSeries depends on libfdt (via host Kconfig FDT symbol)

[Paolo Bonzini]

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

Re: [RFC PATCH 1/5] hw/mem/nvdimm: Use Kconfig 'imply' instead of 'depends on'

[Paolo Bonzini]

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
> 

Re: [RFC PATCH 3/5] hw/ppc: Have pSeries depends on libfdt (via host Kconfig FDT symbol)

[Philippe Mathieu-Daudé]

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!

Re: [RFC PATCH 4/5] hw/ppc/fdt: Drop dependency on libfdt

[David Gibson]

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH 3/5] hw/ppc: Have pSeries depends on libfdt (via host Kconfig FDT symbol)

[David Gibson]

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH 1/5] hw/mem/nvdimm: Use Kconfig 'imply' instead of 'depends on'

[Paolo Bonzini]

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

Re: [RFC PATCH 4/5] hw/ppc/fdt: Drop dependency on libfdt

[Paolo Bonzini]

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 1/5] hw/mem/nvdimm: Use Kconfig 'imply' instead of 'depends on'

[David Gibson]

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

On Thu, May 13, 2021 at 05:20:06PM +0200, Paolo Bonzini wrote:
> 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

Ah, ok, I get it now.  "imply" is a terrible word for this, but ok.

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

-- 
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: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH 4/5] hw/ppc/fdt: Drop dependency on libfdt

[David Gibson]

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH 4/5] hw/ppc/fdt: Drop dependency on libfdt

[Paolo Bonzini]

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

[Philippe Mathieu-Daudé]

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
>