From: Alexey Kardashevskiy <aik@ozlabs.ru>
SLOF receives a device tree and updates it with various properties
before switching to the guest kernel and QEMU is not aware of any changes
made by SLOF. Since there is no real RTAS (QEMU implements it), it makes
sense to pass the SLOF final device tree to QEMU to let it implement
RTAS related tasks better, such as PCI host bus adapter hotplug.
Specifially, now QEMU can find out the actual XICS phandle (for PHB
hotplug) and the RTAS linux,rtas-entry/base properties (for firmware
assisted NMI - FWNMI).
This stores the initial DT blob in the sPAPR machine and replaces it
in the KVMPPC_H_UPDATE_DT (new private hypercall) handler.
This adds an @update_dt_enabled machine property to allow backward
migration.
SLOF already has a hypercall since
https://github.com/aik/SLOF/commit/e6fc84652c9c0073f9183
This makes use of the new fdt_check_full() helper. In order to allow
the configure script to pick the correct DTC version, this adjusts
the DTC presense test.
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Reviewed-by: Greg Kurz <groug@kaod.org>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Greg Kurz <groug@kaod.org>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
configure | 2 +-
hw/ppc/spapr.c | 43 +++++++++++++++++++++++++++++++++++++++++-
hw/ppc/spapr_hcall.c | 42 +++++++++++++++++++++++++++++++++++++++++
hw/ppc/trace-events | 3 +++
include/hw/ppc/spapr.h | 7 ++++++-
5 files changed, 94 insertions(+), 3 deletions(-)
diff --git a/configure b/configure
index b9f34afc9e..8049b71eef 100755
--- a/configure
+++ b/configure
@@ -3939,7 +3939,7 @@ if test "$fdt" != "no" ; then
cat > $TMPC << EOF
#include <libfdt.h>
#include <libfdt_env.h>
-int main(void) { fdt_first_subnode(0, 0); return 0; }
+int main(void) { fdt_check_full(NULL, 0); return 0; }
EOF
if compile_prog "" "$fdt_libs" ; then
# system DTC is good - use it
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 5fba04e7b2..7e61f1e5ff 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1669,7 +1669,10 @@ static void spapr_machine_reset(void)
/* Load the fdt */
qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
cpu_physical_memory_write(fdt_addr, fdt, fdt_totalsize(fdt));
- g_free(fdt);
+ g_free(spapr->fdt_blob);
+ spapr->fdt_size = fdt_totalsize(fdt);
+ spapr->fdt_initial_size = spapr->fdt_size;
+ spapr->fdt_blob = fdt;
/* Set up the entry state */
spapr_cpu_set_entry_state(first_ppc_cpu, SPAPR_ENTRY_POINT, fdt_addr);
@@ -1920,6 +1923,39 @@ static const VMStateDescription vmstate_spapr_irq_map = {
},
};
+static bool spapr_dtb_needed(void *opaque)
+{
+ sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(opaque);
+
+ return smc->update_dt_enabled;
+}
+
+static int spapr_dtb_pre_load(void *opaque)
+{
+ sPAPRMachineState *spapr = (sPAPRMachineState *)opaque;
+
+ g_free(spapr->fdt_blob);
+ spapr->fdt_blob = NULL;
+ spapr->fdt_size = 0;
+
+ return 0;
+}
+
+static const VMStateDescription vmstate_spapr_dtb = {
+ .name = "spapr_dtb",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .needed = spapr_dtb_needed,
+ .pre_load = spapr_dtb_pre_load,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT32(fdt_initial_size, sPAPRMachineState),
+ VMSTATE_UINT32(fdt_size, sPAPRMachineState),
+ VMSTATE_VBUFFER_ALLOC_UINT32(fdt_blob, sPAPRMachineState, 0, NULL,
+ fdt_size),
+ VMSTATE_END_OF_LIST()
+ },
+};
+
static const VMStateDescription vmstate_spapr = {
.name = "spapr",
.version_id = 3,
@@ -1949,6 +1985,7 @@ static const VMStateDescription vmstate_spapr = {
&vmstate_spapr_cap_ibs,
&vmstate_spapr_irq_map,
&vmstate_spapr_cap_nested_kvm_hv,
+ &vmstate_spapr_dtb,
NULL
}
};
@@ -3931,6 +3968,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
hc->unplug = spapr_machine_device_unplug;
smc->dr_lmb_enabled = true;
+ smc->update_dt_enabled = true;
mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power9_v2.0");
mc->has_hotpluggable_cpus = true;
smc->resize_hpt_default = SPAPR_RESIZE_HPT_ENABLED;
@@ -4023,9 +4061,12 @@ DEFINE_SPAPR_MACHINE(4_0, "4.0", true);
*/
static void spapr_machine_3_1_class_options(MachineClass *mc)
{
+ sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
+
spapr_machine_4_0_class_options(mc);
compat_props_add(mc->compat_props, hw_compat_3_1, hw_compat_3_1_len);
mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power8_v2.0");
+ smc->update_dt_enabled = false;
}
DEFINE_SPAPR_MACHINE(3_1, "3.1", false);
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index f131c7e04c..1ae3e6ff5e 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1753,6 +1753,46 @@ static target_ulong h_get_cpu_characteristics(PowerPCCPU *cpu,
args[0] = characteristics;
args[1] = behaviour;
+ return H_SUCCESS;
+}
+
+static target_ulong h_update_dt(PowerPCCPU *cpu, sPAPRMachineState *spapr,
+ target_ulong opcode, target_ulong *args)
+{
+ target_ulong dt = ppc64_phys_to_real(args[0]);
+ struct fdt_header hdr = { 0 };
+ unsigned cb;
+ sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
+ void *fdt;
+
+ cpu_physical_memory_read(dt, &hdr, sizeof(hdr));
+ cb = fdt32_to_cpu(hdr.totalsize);
+
+ if (!smc->update_dt_enabled) {
+ return H_SUCCESS;
+ }
+
+ /* Check that the fdt did not grow out of proportion */
+ if (cb > spapr->fdt_initial_size * 2) {
+ trace_spapr_update_dt_failed_size(spapr->fdt_initial_size, cb,
+ fdt32_to_cpu(hdr.magic));
+ return H_PARAMETER;
+ }
+
+ fdt = g_malloc0(cb);
+ cpu_physical_memory_read(dt, fdt, cb);
+
+ /* Check the fdt consistency */
+ if (fdt_check_full(fdt, cb)) {
+ trace_spapr_update_dt_failed_check(spapr->fdt_initial_size, cb,
+ fdt32_to_cpu(hdr.magic));
+ return H_PARAMETER;
+ }
+
+ g_free(spapr->fdt_blob);
+ spapr->fdt_size = cb;
+ spapr->fdt_blob = fdt;
+ trace_spapr_update_dt(cb);
return H_SUCCESS;
}
@@ -1859,6 +1899,8 @@ static void hypercall_register_types(void)
/* ibm,client-architecture-support support */
spapr_register_hypercall(KVMPPC_H_CAS, h_client_architecture_support);
+ spapr_register_hypercall(KVMPPC_H_UPDATE_DT, h_update_dt);
+
/* Virtual Processor Home Node */
spapr_register_hypercall(H_HOME_NODE_ASSOCIATIVITY,
h_home_node_associativity);
diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
index dc5e65aee9..0af155ed32 100644
--- a/hw/ppc/trace-events
+++ b/hw/ppc/trace-events
@@ -22,6 +22,9 @@ spapr_cas_pvr_try(uint32_t pvr) "0x%x"
spapr_cas_pvr(uint32_t cur_pvr, bool explicit_match, uint32_t new_pvr) "current=0x%x, explicit_match=%u, new=0x%x"
spapr_h_resize_hpt_prepare(uint64_t flags, uint64_t shift) "flags=0x%"PRIx64", shift=%"PRIu64
spapr_h_resize_hpt_commit(uint64_t flags, uint64_t shift) "flags=0x%"PRIx64", shift=%"PRIu64
+spapr_update_dt(unsigned cb) "New blob %u bytes"
+spapr_update_dt_failed_size(unsigned cbold, unsigned cbnew, unsigned magic) "Old blob %u bytes, new blob %u bytes, magic 0x%x"
+spapr_update_dt_failed_check(unsigned cbold, unsigned cbnew, unsigned magic) "Old blob %u bytes, new blob %u bytes, magic 0x%x"
# hw/ppc/spapr_iommu.c
spapr_iommu_put(uint64_t liobn, uint64_t ioba, uint64_t tce, uint64_t ret) "liobn=0x%"PRIx64" ioba=0x%"PRIx64" tce=0x%"PRIx64" ret=%"PRId64
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index eb04300d3b..fd24e91bd8 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -103,6 +103,7 @@ struct sPAPRMachineClass {
/*< public >*/
bool dr_lmb_enabled; /* enable dynamic-reconfig/hotplug of LMBs */
+ bool update_dt_enabled; /* enable KVMPPC_H_UPDATE_DT */
bool use_ohci_by_default; /* use USB-OHCI instead of XHCI */
bool pre_2_10_has_unused_icps;
bool legacy_irq_allocation;
@@ -139,6 +140,9 @@ struct sPAPRMachineState {
int vrma_adjust;
ssize_t rtas_size;
void *rtas_blob;
+ uint32_t fdt_size;
+ uint32_t fdt_initial_size;
+ void *fdt_blob;
long kernel_size;
bool kernel_le;
uint32_t initrd_base;
@@ -481,7 +485,8 @@ struct sPAPRMachineState {
#define KVMPPC_H_LOGICAL_MEMOP (KVMPPC_HCALL_BASE + 0x1)
/* Client Architecture support */
#define KVMPPC_H_CAS (KVMPPC_HCALL_BASE + 0x2)
-#define KVMPPC_HCALL_MAX KVMPPC_H_CAS
+#define KVMPPC_H_UPDATE_DT (KVMPPC_HCALL_BASE + 0x3)
+#define KVMPPC_HCALL_MAX KVMPPC_H_UPDATE_DT
typedef struct sPAPRDeviceTreeUpdateHeader {
uint32_t version_id;
--
2.20.1
Now that I am checking out 4.0.0 rc's I see this diff is broken and
depends on a function libfdt does not expose. The breakage is
hidden by the fallback check in the configure script.
On 1/8/2019 5:45 PM, David Gibson wrote:
> From: Alexey Kardashevskiy <aik@ozlabs.ru>
>
> SLOF receives a device tree and updates it with various properties
> before switching to the guest kernel and QEMU is not aware of any changes
> made by SLOF. Since there is no real RTAS (QEMU implements it), it makes
> sense to pass the SLOF final device tree to QEMU to let it implement
> RTAS related tasks better, such as PCI host bus adapter hotplug.
>
> Specifially, now QEMU can find out the actual XICS phandle (for PHB
> hotplug) and the RTAS linux,rtas-entry/base properties (for firmware
> assisted NMI - FWNMI).
>
> This stores the initial DT blob in the sPAPR machine and replaces it
> in the KVMPPC_H_UPDATE_DT (new private hypercall) handler.
>
> This adds an @update_dt_enabled machine property to allow backward
> migration.
>
> SLOF already has a hypercall since
> https://github.com/aik/SLOF/commit/e6fc84652c9c0073f9183
>
> This makes use of the new fdt_check_full() helper. In order to allow
> the configure script to pick the correct DTC version, this adjusts
> the DTC presense test.
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> Reviewed-by: Greg Kurz <groug@kaod.org>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> configure | 2 +-
> hw/ppc/spapr.c | 43 +++++++++++++++++++++++++++++++++++++++++-
> hw/ppc/spapr_hcall.c | 42 +++++++++++++++++++++++++++++++++++++++++
> hw/ppc/trace-events | 3 +++
> include/hw/ppc/spapr.h | 7 ++++++-
> 5 files changed, 94 insertions(+), 3 deletions(-)
>
> diff --git a/configure b/configure
> index b9f34afc9e..8049b71eef 100755
> --- a/configure
> +++ b/configure
> @@ -3939,7 +3939,7 @@ if test "$fdt" != "no" ; then
> cat > $TMPC << EOF
> #include <libfdt.h>
> #include <libfdt_env.h>
> -int main(void) { fdt_first_subnode(0, 0); return 0; }
> +int main(void) { fdt_check_full(NULL, 0); return 0; }
> EOF
> if compile_prog "" "$fdt_libs" ; then
> # system DTC is good - use it
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 5fba04e7b2..7e61f1e5ff 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1669,7 +1669,10 @@ static void spapr_machine_reset(void)
> /* Load the fdt */
> qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
> cpu_physical_memory_write(fdt_addr, fdt, fdt_totalsize(fdt));
> - g_free(fdt);
> + g_free(spapr->fdt_blob);
> + spapr->fdt_size = fdt_totalsize(fdt);
> + spapr->fdt_initial_size = spapr->fdt_size;
> + spapr->fdt_blob = fdt;
>
> /* Set up the entry state */
> spapr_cpu_set_entry_state(first_ppc_cpu, SPAPR_ENTRY_POINT, fdt_addr);
> @@ -1920,6 +1923,39 @@ static const VMStateDescription vmstate_spapr_irq_map = {
> },
> };
>
> +static bool spapr_dtb_needed(void *opaque)
> +{
> + sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(opaque);
> +
> + return smc->update_dt_enabled;
> +}
> +
> +static int spapr_dtb_pre_load(void *opaque)
> +{
> + sPAPRMachineState *spapr = (sPAPRMachineState *)opaque;
> +
> + g_free(spapr->fdt_blob);
> + spapr->fdt_blob = NULL;
> + spapr->fdt_size = 0;
> +
> + return 0;
> +}
> +
> +static const VMStateDescription vmstate_spapr_dtb = {
> + .name = "spapr_dtb",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .needed = spapr_dtb_needed,
> + .pre_load = spapr_dtb_pre_load,
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT32(fdt_initial_size, sPAPRMachineState),
> + VMSTATE_UINT32(fdt_size, sPAPRMachineState),
> + VMSTATE_VBUFFER_ALLOC_UINT32(fdt_blob, sPAPRMachineState, 0, NULL,
> + fdt_size),
> + VMSTATE_END_OF_LIST()
> + },
> +};
> +
> static const VMStateDescription vmstate_spapr = {
> .name = "spapr",
> .version_id = 3,
> @@ -1949,6 +1985,7 @@ static const VMStateDescription vmstate_spapr = {
> &vmstate_spapr_cap_ibs,
> &vmstate_spapr_irq_map,
> &vmstate_spapr_cap_nested_kvm_hv,
> + &vmstate_spapr_dtb,
> NULL
> }
> };
> @@ -3931,6 +3968,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
> hc->unplug = spapr_machine_device_unplug;
>
> smc->dr_lmb_enabled = true;
> + smc->update_dt_enabled = true;
> mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power9_v2.0");
> mc->has_hotpluggable_cpus = true;
> smc->resize_hpt_default = SPAPR_RESIZE_HPT_ENABLED;
> @@ -4023,9 +4061,12 @@ DEFINE_SPAPR_MACHINE(4_0, "4.0", true);
> */
> static void spapr_machine_3_1_class_options(MachineClass *mc)
> {
> + sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
> +
> spapr_machine_4_0_class_options(mc);
> compat_props_add(mc->compat_props, hw_compat_3_1, hw_compat_3_1_len);
> mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power8_v2.0");
> + smc->update_dt_enabled = false;
> }
>
> DEFINE_SPAPR_MACHINE(3_1, "3.1", false);
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index f131c7e04c..1ae3e6ff5e 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1753,6 +1753,46 @@ static target_ulong h_get_cpu_characteristics(PowerPCCPU *cpu,
>
> args[0] = characteristics;
> args[1] = behaviour;
> + return H_SUCCESS;
> +}
> +
> +static target_ulong h_update_dt(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> + target_ulong opcode, target_ulong *args)
> +{
> + target_ulong dt = ppc64_phys_to_real(args[0]);
> + struct fdt_header hdr = { 0 };
> + unsigned cb;
> + sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> + void *fdt;
> +
> + cpu_physical_memory_read(dt, &hdr, sizeof(hdr));
> + cb = fdt32_to_cpu(hdr.totalsize);
> +
> + if (!smc->update_dt_enabled) {
> + return H_SUCCESS;
> + }
> +
> + /* Check that the fdt did not grow out of proportion */
> + if (cb > spapr->fdt_initial_size * 2) {
> + trace_spapr_update_dt_failed_size(spapr->fdt_initial_size, cb,
> + fdt32_to_cpu(hdr.magic));
> + return H_PARAMETER;
> + }
> +
> + fdt = g_malloc0(cb);
> + cpu_physical_memory_read(dt, fdt, cb);
> +
> + /* Check the fdt consistency */
> + if (fdt_check_full(fdt, cb)) {
> + trace_spapr_update_dt_failed_check(spapr->fdt_initial_size, cb,
> + fdt32_to_cpu(hdr.magic));
> + return H_PARAMETER;
> + }
> +
> + g_free(spapr->fdt_blob);
> + spapr->fdt_size = cb;
> + spapr->fdt_blob = fdt;
> + trace_spapr_update_dt(cb);
>
> return H_SUCCESS;
> }
> @@ -1859,6 +1899,8 @@ static void hypercall_register_types(void)
> /* ibm,client-architecture-support support */
> spapr_register_hypercall(KVMPPC_H_CAS, h_client_architecture_support);
>
> + spapr_register_hypercall(KVMPPC_H_UPDATE_DT, h_update_dt);
> +
> /* Virtual Processor Home Node */
> spapr_register_hypercall(H_HOME_NODE_ASSOCIATIVITY,
> h_home_node_associativity);
> diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
> index dc5e65aee9..0af155ed32 100644
> --- a/hw/ppc/trace-events
> +++ b/hw/ppc/trace-events
> @@ -22,6 +22,9 @@ spapr_cas_pvr_try(uint32_t pvr) "0x%x"
> spapr_cas_pvr(uint32_t cur_pvr, bool explicit_match, uint32_t new_pvr) "current=0x%x, explicit_match=%u, new=0x%x"
> spapr_h_resize_hpt_prepare(uint64_t flags, uint64_t shift) "flags=0x%"PRIx64", shift=%"PRIu64
> spapr_h_resize_hpt_commit(uint64_t flags, uint64_t shift) "flags=0x%"PRIx64", shift=%"PRIu64
> +spapr_update_dt(unsigned cb) "New blob %u bytes"
> +spapr_update_dt_failed_size(unsigned cbold, unsigned cbnew, unsigned magic) "Old blob %u bytes, new blob %u bytes, magic 0x%x"
> +spapr_update_dt_failed_check(unsigned cbold, unsigned cbnew, unsigned magic) "Old blob %u bytes, new blob %u bytes, magic 0x%x"
>
> # hw/ppc/spapr_iommu.c
> spapr_iommu_put(uint64_t liobn, uint64_t ioba, uint64_t tce, uint64_t ret) "liobn=0x%"PRIx64" ioba=0x%"PRIx64" tce=0x%"PRIx64" ret=%"PRId64
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index eb04300d3b..fd24e91bd8 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -103,6 +103,7 @@ struct sPAPRMachineClass {
>
> /*< public >*/
> bool dr_lmb_enabled; /* enable dynamic-reconfig/hotplug of LMBs */
> + bool update_dt_enabled; /* enable KVMPPC_H_UPDATE_DT */
> bool use_ohci_by_default; /* use USB-OHCI instead of XHCI */
> bool pre_2_10_has_unused_icps;
> bool legacy_irq_allocation;
> @@ -139,6 +140,9 @@ struct sPAPRMachineState {
> int vrma_adjust;
> ssize_t rtas_size;
> void *rtas_blob;
> + uint32_t fdt_size;
> + uint32_t fdt_initial_size;
> + void *fdt_blob;
> long kernel_size;
> bool kernel_le;
> uint32_t initrd_base;
> @@ -481,7 +485,8 @@ struct sPAPRMachineState {
> #define KVMPPC_H_LOGICAL_MEMOP (KVMPPC_HCALL_BASE + 0x1)
> /* Client Architecture support */
> #define KVMPPC_H_CAS (KVMPPC_HCALL_BASE + 0x2)
> -#define KVMPPC_HCALL_MAX KVMPPC_H_CAS
> +#define KVMPPC_H_UPDATE_DT (KVMPPC_HCALL_BASE + 0x3)
> +#define KVMPPC_HCALL_MAX KVMPPC_H_UPDATE_DT
>
> typedef struct sPAPRDeviceTreeUpdateHeader {
> uint32_t version_id;
I filed a bug report for libfdt..
https://github.com/dgibson/dtc/issues/27
On 3/24/2019 12:03 AM, Brad Smith wrote:
> Now that I am checking out 4.0.0 rc's I see this diff is broken and
> depends on a function libfdt does not expose. The breakage is
> hidden by the fallback check in the configure script.
>
> On 1/8/2019 5:45 PM, David Gibson wrote:
>> From: Alexey Kardashevskiy <aik@ozlabs.ru>
>>
>> SLOF receives a device tree and updates it with various properties
>> before switching to the guest kernel and QEMU is not aware of any
>> changes
>> made by SLOF. Since there is no real RTAS (QEMU implements it), it makes
>> sense to pass the SLOF final device tree to QEMU to let it implement
>> RTAS related tasks better, such as PCI host bus adapter hotplug.
>>
>> Specifially, now QEMU can find out the actual XICS phandle (for PHB
>> hotplug) and the RTAS linux,rtas-entry/base properties (for firmware
>> assisted NMI - FWNMI).
>>
>> This stores the initial DT blob in the sPAPR machine and replaces it
>> in the KVMPPC_H_UPDATE_DT (new private hypercall) handler.
>>
>> This adds an @update_dt_enabled machine property to allow backward
>> migration.
>>
>> SLOF already has a hypercall since
>> https://github.com/aik/SLOF/commit/e6fc84652c9c0073f9183
>>
>> This makes use of the new fdt_check_full() helper. In order to allow
>> the configure script to pick the correct DTC version, this adjusts
>> the DTC presense test.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> Reviewed-by: Greg Kurz <groug@kaod.org>
>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>> Signed-off-by: Greg Kurz <groug@kaod.org>
>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>> ---
>> configure | 2 +-
>> hw/ppc/spapr.c | 43 +++++++++++++++++++++++++++++++++++++++++-
>> hw/ppc/spapr_hcall.c | 42 +++++++++++++++++++++++++++++++++++++++++
>> hw/ppc/trace-events | 3 +++
>> include/hw/ppc/spapr.h | 7 ++++++-
>> 5 files changed, 94 insertions(+), 3 deletions(-)
>>
>> diff --git a/configure b/configure
>> index b9f34afc9e..8049b71eef 100755
>> --- a/configure
>> +++ b/configure
>> @@ -3939,7 +3939,7 @@ if test "$fdt" != "no" ; then
>> cat > $TMPC << EOF
>> #include <libfdt.h>
>> #include <libfdt_env.h>
>> -int main(void) { fdt_first_subnode(0, 0); return 0; }
>> +int main(void) { fdt_check_full(NULL, 0); return 0; }
>> EOF
>> if compile_prog "" "$fdt_libs" ; then
>> # system DTC is good - use it
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 5fba04e7b2..7e61f1e5ff 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -1669,7 +1669,10 @@ static void spapr_machine_reset(void)
>> /* Load the fdt */
>> qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
>> cpu_physical_memory_write(fdt_addr, fdt, fdt_totalsize(fdt));
>> - g_free(fdt);
>> + g_free(spapr->fdt_blob);
>> + spapr->fdt_size = fdt_totalsize(fdt);
>> + spapr->fdt_initial_size = spapr->fdt_size;
>> + spapr->fdt_blob = fdt;
>> /* Set up the entry state */
>> spapr_cpu_set_entry_state(first_ppc_cpu, SPAPR_ENTRY_POINT,
>> fdt_addr);
>> @@ -1920,6 +1923,39 @@ static const VMStateDescription
>> vmstate_spapr_irq_map = {
>> },
>> };
>> +static bool spapr_dtb_needed(void *opaque)
>> +{
>> + sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(opaque);
>> +
>> + return smc->update_dt_enabled;
>> +}
>> +
>> +static int spapr_dtb_pre_load(void *opaque)
>> +{
>> + sPAPRMachineState *spapr = (sPAPRMachineState *)opaque;
>> +
>> + g_free(spapr->fdt_blob);
>> + spapr->fdt_blob = NULL;
>> + spapr->fdt_size = 0;
>> +
>> + return 0;
>> +}
>> +
>> +static const VMStateDescription vmstate_spapr_dtb = {
>> + .name = "spapr_dtb",
>> + .version_id = 1,
>> + .minimum_version_id = 1,
>> + .needed = spapr_dtb_needed,
>> + .pre_load = spapr_dtb_pre_load,
>> + .fields = (VMStateField[]) {
>> + VMSTATE_UINT32(fdt_initial_size, sPAPRMachineState),
>> + VMSTATE_UINT32(fdt_size, sPAPRMachineState),
>> + VMSTATE_VBUFFER_ALLOC_UINT32(fdt_blob, sPAPRMachineState, 0,
>> NULL,
>> + fdt_size),
>> + VMSTATE_END_OF_LIST()
>> + },
>> +};
>> +
>> static const VMStateDescription vmstate_spapr = {
>> .name = "spapr",
>> .version_id = 3,
>> @@ -1949,6 +1985,7 @@ static const VMStateDescription vmstate_spapr = {
>> &vmstate_spapr_cap_ibs,
>> &vmstate_spapr_irq_map,
>> &vmstate_spapr_cap_nested_kvm_hv,
>> + &vmstate_spapr_dtb,
>> NULL
>> }
>> };
>> @@ -3931,6 +3968,7 @@ static void
>> spapr_machine_class_init(ObjectClass *oc, void *data)
>> hc->unplug = spapr_machine_device_unplug;
>> smc->dr_lmb_enabled = true;
>> + smc->update_dt_enabled = true;
>> mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power9_v2.0");
>> mc->has_hotpluggable_cpus = true;
>> smc->resize_hpt_default = SPAPR_RESIZE_HPT_ENABLED;
>> @@ -4023,9 +4061,12 @@ DEFINE_SPAPR_MACHINE(4_0, "4.0", true);
>> */
>> static void spapr_machine_3_1_class_options(MachineClass *mc)
>> {
>> + sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
>> +
>> spapr_machine_4_0_class_options(mc);
>> compat_props_add(mc->compat_props, hw_compat_3_1,
>> hw_compat_3_1_len);
>> mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power8_v2.0");
>> + smc->update_dt_enabled = false;
>> }
>> DEFINE_SPAPR_MACHINE(3_1, "3.1", false);
>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>> index f131c7e04c..1ae3e6ff5e 100644
>> --- a/hw/ppc/spapr_hcall.c
>> +++ b/hw/ppc/spapr_hcall.c
>> @@ -1753,6 +1753,46 @@ static target_ulong
>> h_get_cpu_characteristics(PowerPCCPU *cpu,
>> args[0] = characteristics;
>> args[1] = behaviour;
>> + return H_SUCCESS;
>> +}
>> +
>> +static target_ulong h_update_dt(PowerPCCPU *cpu, sPAPRMachineState
>> *spapr,
>> + target_ulong opcode, target_ulong
>> *args)
>> +{
>> + target_ulong dt = ppc64_phys_to_real(args[0]);
>> + struct fdt_header hdr = { 0 };
>> + unsigned cb;
>> + sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
>> + void *fdt;
>> +
>> + cpu_physical_memory_read(dt, &hdr, sizeof(hdr));
>> + cb = fdt32_to_cpu(hdr.totalsize);
>> +
>> + if (!smc->update_dt_enabled) {
>> + return H_SUCCESS;
>> + }
>> +
>> + /* Check that the fdt did not grow out of proportion */
>> + if (cb > spapr->fdt_initial_size * 2) {
>> + trace_spapr_update_dt_failed_size(spapr->fdt_initial_size, cb,
>> + fdt32_to_cpu(hdr.magic));
>> + return H_PARAMETER;
>> + }
>> +
>> + fdt = g_malloc0(cb);
>> + cpu_physical_memory_read(dt, fdt, cb);
>> +
>> + /* Check the fdt consistency */
>> + if (fdt_check_full(fdt, cb)) {
>> + trace_spapr_update_dt_failed_check(spapr->fdt_initial_size, cb,
>> + fdt32_to_cpu(hdr.magic));
>> + return H_PARAMETER;
>> + }
>> +
>> + g_free(spapr->fdt_blob);
>> + spapr->fdt_size = cb;
>> + spapr->fdt_blob = fdt;
>> + trace_spapr_update_dt(cb);
>> return H_SUCCESS;
>> }
>> @@ -1859,6 +1899,8 @@ static void hypercall_register_types(void)
>> /* ibm,client-architecture-support support */
>> spapr_register_hypercall(KVMPPC_H_CAS,
>> h_client_architecture_support);
>> + spapr_register_hypercall(KVMPPC_H_UPDATE_DT, h_update_dt);
>> +
>> /* Virtual Processor Home Node */
>> spapr_register_hypercall(H_HOME_NODE_ASSOCIATIVITY,
>> h_home_node_associativity);
>> diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
>> index dc5e65aee9..0af155ed32 100644
>> --- a/hw/ppc/trace-events
>> +++ b/hw/ppc/trace-events
>> @@ -22,6 +22,9 @@ spapr_cas_pvr_try(uint32_t pvr) "0x%x"
>> spapr_cas_pvr(uint32_t cur_pvr, bool explicit_match, uint32_t
>> new_pvr) "current=0x%x, explicit_match=%u, new=0x%x"
>> spapr_h_resize_hpt_prepare(uint64_t flags, uint64_t shift)
>> "flags=0x%"PRIx64", shift=%"PRIu64
>> spapr_h_resize_hpt_commit(uint64_t flags, uint64_t shift)
>> "flags=0x%"PRIx64", shift=%"PRIu64
>> +spapr_update_dt(unsigned cb) "New blob %u bytes"
>> +spapr_update_dt_failed_size(unsigned cbold, unsigned cbnew, unsigned
>> magic) "Old blob %u bytes, new blob %u bytes, magic 0x%x"
>> +spapr_update_dt_failed_check(unsigned cbold, unsigned cbnew,
>> unsigned magic) "Old blob %u bytes, new blob %u bytes, magic 0x%x"
>> # hw/ppc/spapr_iommu.c
>> spapr_iommu_put(uint64_t liobn, uint64_t ioba, uint64_t tce,
>> uint64_t ret) "liobn=0x%"PRIx64" ioba=0x%"PRIx64" tce=0x%"PRIx64"
>> ret=%"PRId64
>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>> index eb04300d3b..fd24e91bd8 100644
>> --- a/include/hw/ppc/spapr.h
>> +++ b/include/hw/ppc/spapr.h
>> @@ -103,6 +103,7 @@ struct sPAPRMachineClass {
>> /*< public >*/
>> bool dr_lmb_enabled; /* enable dynamic-reconfig/hotplug
>> of LMBs */
>> + bool update_dt_enabled; /* enable KVMPPC_H_UPDATE_DT */
>> bool use_ohci_by_default; /* use USB-OHCI instead of XHCI */
>> bool pre_2_10_has_unused_icps;
>> bool legacy_irq_allocation;
>> @@ -139,6 +140,9 @@ struct sPAPRMachineState {
>> int vrma_adjust;
>> ssize_t rtas_size;
>> void *rtas_blob;
>> + uint32_t fdt_size;
>> + uint32_t fdt_initial_size;
>> + void *fdt_blob;
>> long kernel_size;
>> bool kernel_le;
>> uint32_t initrd_base;
>> @@ -481,7 +485,8 @@ struct sPAPRMachineState {
>> #define KVMPPC_H_LOGICAL_MEMOP (KVMPPC_HCALL_BASE + 0x1)
>> /* Client Architecture support */
>> #define KVMPPC_H_CAS (KVMPPC_HCALL_BASE + 0x2)
>> -#define KVMPPC_HCALL_MAX KVMPPC_H_CAS
>> +#define KVMPPC_H_UPDATE_DT (KVMPPC_HCALL_BASE + 0x3)
>> +#define KVMPPC_HCALL_MAX KVMPPC_H_UPDATE_DT
>> typedef struct sPAPRDeviceTreeUpdateHeader {
>> uint32_t version_id;
On Sun, Mar 24, 2019 at 12:03:54AM -0400, Brad Smith wrote:
> Now that I am checking out 4.0.0 rc's I see this diff is broken and
> depends on a function libfdt does not expose. The breakage is
> hidden by the fallback check in the configure script.
Ah, bother. That keeps happening, unfortunately. I think it's
because so many people use libfdt embedded, rather than as a shared
library that we tend not to notice.
I guess we should figure out how to force the testsuite to link
against the shared library rather than static to test for this. I'll
look into it if I have time (which is a big if).
>
> On 1/8/2019 5:45 PM, David Gibson wrote:
> > From: Alexey Kardashevskiy <aik@ozlabs.ru>
> >
> > SLOF receives a device tree and updates it with various properties
> > before switching to the guest kernel and QEMU is not aware of any changes
> > made by SLOF. Since there is no real RTAS (QEMU implements it), it makes
> > sense to pass the SLOF final device tree to QEMU to let it implement
> > RTAS related tasks better, such as PCI host bus adapter hotplug.
> >
> > Specifially, now QEMU can find out the actual XICS phandle (for PHB
> > hotplug) and the RTAS linux,rtas-entry/base properties (for firmware
> > assisted NMI - FWNMI).
> >
> > This stores the initial DT blob in the sPAPR machine and replaces it
> > in the KVMPPC_H_UPDATE_DT (new private hypercall) handler.
> >
> > This adds an @update_dt_enabled machine property to allow backward
> > migration.
> >
> > SLOF already has a hypercall since
> > https://github.com/aik/SLOF/commit/e6fc84652c9c0073f9183
> >
> > This makes use of the new fdt_check_full() helper. In order to allow
> > the configure script to pick the correct DTC version, this adjusts
> > the DTC presense test.
> >
> > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> > Reviewed-by: Greg Kurz <groug@kaod.org>
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> > configure | 2 +-
> > hw/ppc/spapr.c | 43 +++++++++++++++++++++++++++++++++++++++++-
> > hw/ppc/spapr_hcall.c | 42 +++++++++++++++++++++++++++++++++++++++++
> > hw/ppc/trace-events | 3 +++
> > include/hw/ppc/spapr.h | 7 ++++++-
> > 5 files changed, 94 insertions(+), 3 deletions(-)
> >
> > diff --git a/configure b/configure
> > index b9f34afc9e..8049b71eef 100755
> > --- a/configure
> > +++ b/configure
> > @@ -3939,7 +3939,7 @@ if test "$fdt" != "no" ; then
> > cat > $TMPC << EOF
> > #include <libfdt.h>
> > #include <libfdt_env.h>
> > -int main(void) { fdt_first_subnode(0, 0); return 0; }
> > +int main(void) { fdt_check_full(NULL, 0); return 0; }
> > EOF
> > if compile_prog "" "$fdt_libs" ; then
> > # system DTC is good - use it
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 5fba04e7b2..7e61f1e5ff 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1669,7 +1669,10 @@ static void spapr_machine_reset(void)
> > /* Load the fdt */
> > qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
> > cpu_physical_memory_write(fdt_addr, fdt, fdt_totalsize(fdt));
> > - g_free(fdt);
> > + g_free(spapr->fdt_blob);
> > + spapr->fdt_size = fdt_totalsize(fdt);
> > + spapr->fdt_initial_size = spapr->fdt_size;
> > + spapr->fdt_blob = fdt;
> > /* Set up the entry state */
> > spapr_cpu_set_entry_state(first_ppc_cpu, SPAPR_ENTRY_POINT, fdt_addr);
> > @@ -1920,6 +1923,39 @@ static const VMStateDescription vmstate_spapr_irq_map = {
> > },
> > };
> > +static bool spapr_dtb_needed(void *opaque)
> > +{
> > + sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(opaque);
> > +
> > + return smc->update_dt_enabled;
> > +}
> > +
> > +static int spapr_dtb_pre_load(void *opaque)
> > +{
> > + sPAPRMachineState *spapr = (sPAPRMachineState *)opaque;
> > +
> > + g_free(spapr->fdt_blob);
> > + spapr->fdt_blob = NULL;
> > + spapr->fdt_size = 0;
> > +
> > + return 0;
> > +}
> > +
> > +static const VMStateDescription vmstate_spapr_dtb = {
> > + .name = "spapr_dtb",
> > + .version_id = 1,
> > + .minimum_version_id = 1,
> > + .needed = spapr_dtb_needed,
> > + .pre_load = spapr_dtb_pre_load,
> > + .fields = (VMStateField[]) {
> > + VMSTATE_UINT32(fdt_initial_size, sPAPRMachineState),
> > + VMSTATE_UINT32(fdt_size, sPAPRMachineState),
> > + VMSTATE_VBUFFER_ALLOC_UINT32(fdt_blob, sPAPRMachineState, 0, NULL,
> > + fdt_size),
> > + VMSTATE_END_OF_LIST()
> > + },
> > +};
> > +
> > static const VMStateDescription vmstate_spapr = {
> > .name = "spapr",
> > .version_id = 3,
> > @@ -1949,6 +1985,7 @@ static const VMStateDescription vmstate_spapr = {
> > &vmstate_spapr_cap_ibs,
> > &vmstate_spapr_irq_map,
> > &vmstate_spapr_cap_nested_kvm_hv,
> > + &vmstate_spapr_dtb,
> > NULL
> > }
> > };
> > @@ -3931,6 +3968,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
> > hc->unplug = spapr_machine_device_unplug;
> > smc->dr_lmb_enabled = true;
> > + smc->update_dt_enabled = true;
> > mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power9_v2.0");
> > mc->has_hotpluggable_cpus = true;
> > smc->resize_hpt_default = SPAPR_RESIZE_HPT_ENABLED;
> > @@ -4023,9 +4061,12 @@ DEFINE_SPAPR_MACHINE(4_0, "4.0", true);
> > */
> > static void spapr_machine_3_1_class_options(MachineClass *mc)
> > {
> > + sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
> > +
> > spapr_machine_4_0_class_options(mc);
> > compat_props_add(mc->compat_props, hw_compat_3_1, hw_compat_3_1_len);
> > mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power8_v2.0");
> > + smc->update_dt_enabled = false;
> > }
> > DEFINE_SPAPR_MACHINE(3_1, "3.1", false);
> > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> > index f131c7e04c..1ae3e6ff5e 100644
> > --- a/hw/ppc/spapr_hcall.c
> > +++ b/hw/ppc/spapr_hcall.c
> > @@ -1753,6 +1753,46 @@ static target_ulong h_get_cpu_characteristics(PowerPCCPU *cpu,
> > args[0] = characteristics;
> > args[1] = behaviour;
> > + return H_SUCCESS;
> > +}
> > +
> > +static target_ulong h_update_dt(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> > + target_ulong opcode, target_ulong *args)
> > +{
> > + target_ulong dt = ppc64_phys_to_real(args[0]);
> > + struct fdt_header hdr = { 0 };
> > + unsigned cb;
> > + sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> > + void *fdt;
> > +
> > + cpu_physical_memory_read(dt, &hdr, sizeof(hdr));
> > + cb = fdt32_to_cpu(hdr.totalsize);
> > +
> > + if (!smc->update_dt_enabled) {
> > + return H_SUCCESS;
> > + }
> > +
> > + /* Check that the fdt did not grow out of proportion */
> > + if (cb > spapr->fdt_initial_size * 2) {
> > + trace_spapr_update_dt_failed_size(spapr->fdt_initial_size, cb,
> > + fdt32_to_cpu(hdr.magic));
> > + return H_PARAMETER;
> > + }
> > +
> > + fdt = g_malloc0(cb);
> > + cpu_physical_memory_read(dt, fdt, cb);
> > +
> > + /* Check the fdt consistency */
> > + if (fdt_check_full(fdt, cb)) {
> > + trace_spapr_update_dt_failed_check(spapr->fdt_initial_size, cb,
> > + fdt32_to_cpu(hdr.magic));
> > + return H_PARAMETER;
> > + }
> > +
> > + g_free(spapr->fdt_blob);
> > + spapr->fdt_size = cb;
> > + spapr->fdt_blob = fdt;
> > + trace_spapr_update_dt(cb);
> > return H_SUCCESS;
> > }
> > @@ -1859,6 +1899,8 @@ static void hypercall_register_types(void)
> > /* ibm,client-architecture-support support */
> > spapr_register_hypercall(KVMPPC_H_CAS, h_client_architecture_support);
> > + spapr_register_hypercall(KVMPPC_H_UPDATE_DT, h_update_dt);
> > +
> > /* Virtual Processor Home Node */
> > spapr_register_hypercall(H_HOME_NODE_ASSOCIATIVITY,
> > h_home_node_associativity);
> > diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
> > index dc5e65aee9..0af155ed32 100644
> > --- a/hw/ppc/trace-events
> > +++ b/hw/ppc/trace-events
> > @@ -22,6 +22,9 @@ spapr_cas_pvr_try(uint32_t pvr) "0x%x"
> > spapr_cas_pvr(uint32_t cur_pvr, bool explicit_match, uint32_t new_pvr) "current=0x%x, explicit_match=%u, new=0x%x"
> > spapr_h_resize_hpt_prepare(uint64_t flags, uint64_t shift) "flags=0x%"PRIx64", shift=%"PRIu64
> > spapr_h_resize_hpt_commit(uint64_t flags, uint64_t shift) "flags=0x%"PRIx64", shift=%"PRIu64
> > +spapr_update_dt(unsigned cb) "New blob %u bytes"
> > +spapr_update_dt_failed_size(unsigned cbold, unsigned cbnew, unsigned magic) "Old blob %u bytes, new blob %u bytes, magic 0x%x"
> > +spapr_update_dt_failed_check(unsigned cbold, unsigned cbnew, unsigned magic) "Old blob %u bytes, new blob %u bytes, magic 0x%x"
> > # hw/ppc/spapr_iommu.c
> > spapr_iommu_put(uint64_t liobn, uint64_t ioba, uint64_t tce, uint64_t ret) "liobn=0x%"PRIx64" ioba=0x%"PRIx64" tce=0x%"PRIx64" ret=%"PRId64
> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > index eb04300d3b..fd24e91bd8 100644
> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -103,6 +103,7 @@ struct sPAPRMachineClass {
> > /*< public >*/
> > bool dr_lmb_enabled; /* enable dynamic-reconfig/hotplug of LMBs */
> > + bool update_dt_enabled; /* enable KVMPPC_H_UPDATE_DT */
> > bool use_ohci_by_default; /* use USB-OHCI instead of XHCI */
> > bool pre_2_10_has_unused_icps;
> > bool legacy_irq_allocation;
> > @@ -139,6 +140,9 @@ struct sPAPRMachineState {
> > int vrma_adjust;
> > ssize_t rtas_size;
> > void *rtas_blob;
> > + uint32_t fdt_size;
> > + uint32_t fdt_initial_size;
> > + void *fdt_blob;
> > long kernel_size;
> > bool kernel_le;
> > uint32_t initrd_base;
> > @@ -481,7 +485,8 @@ struct sPAPRMachineState {
> > #define KVMPPC_H_LOGICAL_MEMOP (KVMPPC_HCALL_BASE + 0x1)
> > /* Client Architecture support */
> > #define KVMPPC_H_CAS (KVMPPC_HCALL_BASE + 0x2)
> > -#define KVMPPC_HCALL_MAX KVMPPC_H_CAS
> > +#define KVMPPC_H_UPDATE_DT (KVMPPC_HCALL_BASE + 0x3)
> > +#define KVMPPC_HCALL_MAX KVMPPC_H_UPDATE_DT
> > typedef struct sPAPRDeviceTreeUpdateHeader {
> > uint32_t version_id;
>
--
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 Mon, 25 Mar 2019 11:53:47 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:
> On Sun, Mar 24, 2019 at 12:03:54AM -0400, Brad Smith wrote:
> > Now that I am checking out 4.0.0 rc's I see this diff is broken and
> > depends on a function libfdt does not expose. The breakage is
> > hidden by the fallback check in the configure script.
>
> Ah, bother. That keeps happening, unfortunately. I think it's
> because so many people use libfdt embedded, rather than as a shared
> library that we tend not to notice.
>
> I guess we should figure out how to force the testsuite to link
> against the shared library rather than static to test for this. I'll
> look into it if I have time (which is a big if).
>
I'll have a look later today.
> >
> > On 1/8/2019 5:45 PM, David Gibson wrote:
> > > From: Alexey Kardashevskiy <aik@ozlabs.ru>
> > >
> > > SLOF receives a device tree and updates it with various properties
> > > before switching to the guest kernel and QEMU is not aware of any changes
> > > made by SLOF. Since there is no real RTAS (QEMU implements it), it makes
> > > sense to pass the SLOF final device tree to QEMU to let it implement
> > > RTAS related tasks better, such as PCI host bus adapter hotplug.
> > >
> > > Specifially, now QEMU can find out the actual XICS phandle (for PHB
> > > hotplug) and the RTAS linux,rtas-entry/base properties (for firmware
> > > assisted NMI - FWNMI).
> > >
> > > This stores the initial DT blob in the sPAPR machine and replaces it
> > > in the KVMPPC_H_UPDATE_DT (new private hypercall) handler.
> > >
> > > This adds an @update_dt_enabled machine property to allow backward
> > > migration.
> > >
> > > SLOF already has a hypercall since
> > > https://github.com/aik/SLOF/commit/e6fc84652c9c0073f9183
> > >
> > > This makes use of the new fdt_check_full() helper. In order to allow
> > > the configure script to pick the correct DTC version, this adjusts
> > > the DTC presense test.
> > >
> > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> > > Reviewed-by: Greg Kurz <groug@kaod.org>
> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > ---
> > > configure | 2 +-
> > > hw/ppc/spapr.c | 43 +++++++++++++++++++++++++++++++++++++++++-
> > > hw/ppc/spapr_hcall.c | 42 +++++++++++++++++++++++++++++++++++++++++
> > > hw/ppc/trace-events | 3 +++
> > > include/hw/ppc/spapr.h | 7 ++++++-
> > > 5 files changed, 94 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/configure b/configure
> > > index b9f34afc9e..8049b71eef 100755
> > > --- a/configure
> > > +++ b/configure
> > > @@ -3939,7 +3939,7 @@ if test "$fdt" != "no" ; then
> > > cat > $TMPC << EOF
> > > #include <libfdt.h>
> > > #include <libfdt_env.h>
> > > -int main(void) { fdt_first_subnode(0, 0); return 0; }
> > > +int main(void) { fdt_check_full(NULL, 0); return 0; }
> > > EOF
> > > if compile_prog "" "$fdt_libs" ; then
> > > # system DTC is good - use it
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 5fba04e7b2..7e61f1e5ff 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -1669,7 +1669,10 @@ static void spapr_machine_reset(void)
> > > /* Load the fdt */
> > > qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
> > > cpu_physical_memory_write(fdt_addr, fdt, fdt_totalsize(fdt));
> > > - g_free(fdt);
> > > + g_free(spapr->fdt_blob);
> > > + spapr->fdt_size = fdt_totalsize(fdt);
> > > + spapr->fdt_initial_size = spapr->fdt_size;
> > > + spapr->fdt_blob = fdt;
> > > /* Set up the entry state */
> > > spapr_cpu_set_entry_state(first_ppc_cpu, SPAPR_ENTRY_POINT, fdt_addr);
> > > @@ -1920,6 +1923,39 @@ static const VMStateDescription vmstate_spapr_irq_map = {
> > > },
> > > };
> > > +static bool spapr_dtb_needed(void *opaque)
> > > +{
> > > + sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(opaque);
> > > +
> > > + return smc->update_dt_enabled;
> > > +}
> > > +
> > > +static int spapr_dtb_pre_load(void *opaque)
> > > +{
> > > + sPAPRMachineState *spapr = (sPAPRMachineState *)opaque;
> > > +
> > > + g_free(spapr->fdt_blob);
> > > + spapr->fdt_blob = NULL;
> > > + spapr->fdt_size = 0;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static const VMStateDescription vmstate_spapr_dtb = {
> > > + .name = "spapr_dtb",
> > > + .version_id = 1,
> > > + .minimum_version_id = 1,
> > > + .needed = spapr_dtb_needed,
> > > + .pre_load = spapr_dtb_pre_load,
> > > + .fields = (VMStateField[]) {
> > > + VMSTATE_UINT32(fdt_initial_size, sPAPRMachineState),
> > > + VMSTATE_UINT32(fdt_size, sPAPRMachineState),
> > > + VMSTATE_VBUFFER_ALLOC_UINT32(fdt_blob, sPAPRMachineState, 0, NULL,
> > > + fdt_size),
> > > + VMSTATE_END_OF_LIST()
> > > + },
> > > +};
> > > +
> > > static const VMStateDescription vmstate_spapr = {
> > > .name = "spapr",
> > > .version_id = 3,
> > > @@ -1949,6 +1985,7 @@ static const VMStateDescription vmstate_spapr = {
> > > &vmstate_spapr_cap_ibs,
> > > &vmstate_spapr_irq_map,
> > > &vmstate_spapr_cap_nested_kvm_hv,
> > > + &vmstate_spapr_dtb,
> > > NULL
> > > }
> > > };
> > > @@ -3931,6 +3968,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
> > > hc->unplug = spapr_machine_device_unplug;
> > > smc->dr_lmb_enabled = true;
> > > + smc->update_dt_enabled = true;
> > > mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power9_v2.0");
> > > mc->has_hotpluggable_cpus = true;
> > > smc->resize_hpt_default = SPAPR_RESIZE_HPT_ENABLED;
> > > @@ -4023,9 +4061,12 @@ DEFINE_SPAPR_MACHINE(4_0, "4.0", true);
> > > */
> > > static void spapr_machine_3_1_class_options(MachineClass *mc)
> > > {
> > > + sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
> > > +
> > > spapr_machine_4_0_class_options(mc);
> > > compat_props_add(mc->compat_props, hw_compat_3_1, hw_compat_3_1_len);
> > > mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power8_v2.0");
> > > + smc->update_dt_enabled = false;
> > > }
> > > DEFINE_SPAPR_MACHINE(3_1, "3.1", false);
> > > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> > > index f131c7e04c..1ae3e6ff5e 100644
> > > --- a/hw/ppc/spapr_hcall.c
> > > +++ b/hw/ppc/spapr_hcall.c
> > > @@ -1753,6 +1753,46 @@ static target_ulong h_get_cpu_characteristics(PowerPCCPU *cpu,
> > > args[0] = characteristics;
> > > args[1] = behaviour;
> > > + return H_SUCCESS;
> > > +}
> > > +
> > > +static target_ulong h_update_dt(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> > > + target_ulong opcode, target_ulong *args)
> > > +{
> > > + target_ulong dt = ppc64_phys_to_real(args[0]);
> > > + struct fdt_header hdr = { 0 };
> > > + unsigned cb;
> > > + sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> > > + void *fdt;
> > > +
> > > + cpu_physical_memory_read(dt, &hdr, sizeof(hdr));
> > > + cb = fdt32_to_cpu(hdr.totalsize);
> > > +
> > > + if (!smc->update_dt_enabled) {
> > > + return H_SUCCESS;
> > > + }
> > > +
> > > + /* Check that the fdt did not grow out of proportion */
> > > + if (cb > spapr->fdt_initial_size * 2) {
> > > + trace_spapr_update_dt_failed_size(spapr->fdt_initial_size, cb,
> > > + fdt32_to_cpu(hdr.magic));
> > > + return H_PARAMETER;
> > > + }
> > > +
> > > + fdt = g_malloc0(cb);
> > > + cpu_physical_memory_read(dt, fdt, cb);
> > > +
> > > + /* Check the fdt consistency */
> > > + if (fdt_check_full(fdt, cb)) {
> > > + trace_spapr_update_dt_failed_check(spapr->fdt_initial_size, cb,
> > > + fdt32_to_cpu(hdr.magic));
> > > + return H_PARAMETER;
> > > + }
> > > +
> > > + g_free(spapr->fdt_blob);
> > > + spapr->fdt_size = cb;
> > > + spapr->fdt_blob = fdt;
> > > + trace_spapr_update_dt(cb);
> > > return H_SUCCESS;
> > > }
> > > @@ -1859,6 +1899,8 @@ static void hypercall_register_types(void)
> > > /* ibm,client-architecture-support support */
> > > spapr_register_hypercall(KVMPPC_H_CAS, h_client_architecture_support);
> > > + spapr_register_hypercall(KVMPPC_H_UPDATE_DT, h_update_dt);
> > > +
> > > /* Virtual Processor Home Node */
> > > spapr_register_hypercall(H_HOME_NODE_ASSOCIATIVITY,
> > > h_home_node_associativity);
> > > diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
> > > index dc5e65aee9..0af155ed32 100644
> > > --- a/hw/ppc/trace-events
> > > +++ b/hw/ppc/trace-events
> > > @@ -22,6 +22,9 @@ spapr_cas_pvr_try(uint32_t pvr) "0x%x"
> > > spapr_cas_pvr(uint32_t cur_pvr, bool explicit_match, uint32_t new_pvr) "current=0x%x, explicit_match=%u, new=0x%x"
> > > spapr_h_resize_hpt_prepare(uint64_t flags, uint64_t shift) "flags=0x%"PRIx64", shift=%"PRIu64
> > > spapr_h_resize_hpt_commit(uint64_t flags, uint64_t shift) "flags=0x%"PRIx64", shift=%"PRIu64
> > > +spapr_update_dt(unsigned cb) "New blob %u bytes"
> > > +spapr_update_dt_failed_size(unsigned cbold, unsigned cbnew, unsigned magic) "Old blob %u bytes, new blob %u bytes, magic 0x%x"
> > > +spapr_update_dt_failed_check(unsigned cbold, unsigned cbnew, unsigned magic) "Old blob %u bytes, new blob %u bytes, magic 0x%x"
> > > # hw/ppc/spapr_iommu.c
> > > spapr_iommu_put(uint64_t liobn, uint64_t ioba, uint64_t tce, uint64_t ret) "liobn=0x%"PRIx64" ioba=0x%"PRIx64" tce=0x%"PRIx64" ret=%"PRId64
> > > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > > index eb04300d3b..fd24e91bd8 100644
> > > --- a/include/hw/ppc/spapr.h
> > > +++ b/include/hw/ppc/spapr.h
> > > @@ -103,6 +103,7 @@ struct sPAPRMachineClass {
> > > /*< public >*/
> > > bool dr_lmb_enabled; /* enable dynamic-reconfig/hotplug of LMBs */
> > > + bool update_dt_enabled; /* enable KVMPPC_H_UPDATE_DT */
> > > bool use_ohci_by_default; /* use USB-OHCI instead of XHCI */
> > > bool pre_2_10_has_unused_icps;
> > > bool legacy_irq_allocation;
> > > @@ -139,6 +140,9 @@ struct sPAPRMachineState {
> > > int vrma_adjust;
> > > ssize_t rtas_size;
> > > void *rtas_blob;
> > > + uint32_t fdt_size;
> > > + uint32_t fdt_initial_size;
> > > + void *fdt_blob;
> > > long kernel_size;
> > > bool kernel_le;
> > > uint32_t initrd_base;
> > > @@ -481,7 +485,8 @@ struct sPAPRMachineState {
> > > #define KVMPPC_H_LOGICAL_MEMOP (KVMPPC_HCALL_BASE + 0x1)
> > > /* Client Architecture support */
> > > #define KVMPPC_H_CAS (KVMPPC_HCALL_BASE + 0x2)
> > > -#define KVMPPC_HCALL_MAX KVMPPC_H_CAS
> > > +#define KVMPPC_H_UPDATE_DT (KVMPPC_HCALL_BASE + 0x3)
> > > +#define KVMPPC_HCALL_MAX KVMPPC_H_UPDATE_DT
> > > typedef struct sPAPRDeviceTreeUpdateHeader {
> > > uint32_t version_id;
> >
>
On Mon, 25 Mar 2019 11:53:47 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:
> On Sun, Mar 24, 2019 at 12:03:54AM -0400, Brad Smith wrote:
> > Now that I am checking out 4.0.0 rc's I see this diff is broken and
> > depends on a function libfdt does not expose. The breakage is
> > hidden by the fallback check in the configure script.
>
> Ah, bother. That keeps happening, unfortunately. I think it's
> because so many people use libfdt embedded, rather than as a shared
> library that we tend not to notice.
>
It's a bit more complicated. I do have latest libfdt packages on my laptop:
libfdt-1.4.7-2.fc28.x86_64
libfdt-devel-1.4.7-2.fc28.x86_64
but I still end up using the embedded one and the build doesn't spot
the missing symbols.
This happens because of several reasons:
1) configure unconditionally falls back to embedded if an error occurs with
the system packages. And, as reported by Brad, the current 1.4.7 packages
are broken indeed:
$ objdump -T /usr/lib64/libfdt-1.4.7.so | grep fdt_check_full
$
2) when building embedded, we only build the archive, not the shared lib.
> I guess we should figure out how to force the testsuite to link
> against the shared library rather than static to test for this. I'll
> look into it if I have time (which is a big if).
>
I think we should rather build the embedded shared library using
the 'libfdt' rule of the top-level makefile of the dtc sub-module
and have QEMU to be linked against this share library instead of
the static one. AFAIK, this is what gcc does when it finds both
.a and .so.
And then we must do some -rpath magic to ensure that QEMU does
use the embedded share library, and not the system one.
The following proof-of-concept patch gives the idea:
---------------------------------------------------------------------
diff --git a/Makefile b/Makefile
index d8dad39c5db1..4cb19e4177a8 100644
--- a/Makefile
+++ b/Makefile
@@ -449,12 +449,14 @@ subdir-%:
$(call quiet-command,$(MAKE) $(SUBDIR_MAKEFLAGS) -C $* V="$(V)" TARGET_DIR="$*/" all,)
DTC_MAKE_ARGS=-I$(SRC_PATH)/dtc VPATH=$(SRC_PATH)/dtc -C dtc V="$(V)" LIBFDT_srcdir=$(SRC_PATH)/dtc/libfdt
-DTC_CFLAGS=$(CFLAGS) $(QEMU_CFLAGS)
+DTC_CFLAGS=$(CFLAGS) $(QEMU_CFLAGS) -fPIC
DTC_CPPFLAGS=-I$(BUILD_DIR)/dtc -I$(SRC_PATH)/dtc -I$(SRC_PATH)/dtc/libfdt
+DTC_LDFLAGS=-Wl,-rpath -Wl,"$(BUILD_DIR)"/dtc $(LDFLAGS)
subdir-dtc: .git-submodule-status dtc/libfdt dtc/tests
- $(call quiet-command,$(MAKE) $(DTC_MAKE_ARGS) CPPFLAGS="$(DTC_CPPFLAGS)" CFLAGS="$(DTC_CFLAGS)" LDFLAGS="$(LDFLAGS)" ARFLAGS="$(ARFLAGS)" CC="$(CC)" AR="$(AR)" LD="$(LD)" $(SUBDIR_MAKEFLAGS) libfdt/libfdt.a,)
-
+ $(call quiet-command,$(MAKE) $(DTC_MAKE_ARGS) CPPFLAGS="$(DTC_CPPFLAGS)" CFLAGS="$(DTC_CFLAGS)" LDFLAGS="$(LDFLAGS)" ARFLAGS="$(ARFLAGS)" CC="$(CC)" AR="$(AR)" LD="$(LD)" $(SUBDIR_MAKEFLAGS) libfdt,)
+ $(call quiet-command, cd $(BUILD_DIR)/dtc/libfdt && \
+ ln -fs libfdt-*.so libfdt.so && ln -fs libfdt-*.so libfdt.so.1)
dtc/%: .git-submodule-status
@mkdir -p $@
diff --git a/configure b/configure
index c5032425e6d3..a9815841b09d 100755
--- a/configure
+++ b/configure
@@ -4049,7 +4049,8 @@ EOF
symlink "$source_path/dtc/scripts" "dtc/scripts"
fi
fdt_cflags="-I\$(SRC_PATH)/dtc/libfdt"
- fdt_ldflags="-L\$(BUILD_DIR)/dtc/libfdt"
+ fdt_rpath="\$(BUILD_DIR)/dtc/libfdt"
+ fdt_ldflags="-L$fdt_rpath -Wl,-rpath -Wl,$fdt_rpath"
fdt_libs="$fdt_libs"
elif test "$fdt" = "yes" ; then
# Not a git build & no libfdt found, prompt for system install
---------------------------------------------------------------------
It detects the missing symbol:
LINK ppc64-softmmu/qemu-system-ppc64
hw/ppc/spapr_hcall.o: In function `h_update_dt':
/home/greg/Work/qemu/qemu-master/hw/ppc/spapr_hcall.c:1784: undefined reference to `fdt_check_full'
It makes sure we use the embedded library, after it got fixed, ie. adding
fdt_check_full to dtc/libfdt/version.lds :
[greg@bahia obj]$ ldd ppc64-softmmu/qemu-system-ppc64 | grep fdt
libfdt.so.1 => /home/greg/Work/qemu/qemu-master/.mbuild-master/obj/dtc/libfdt/libfdt.so.1 (0x00007f68c5e90000)
Note that I wanted to use the install-lib rule of dtc but unfortunately it
depends on all, ie. tries to rebuild the whole dtc package whereas we only
need libfdt. I'll try to find a cleaner way to create the libfdt.so and
libfdt.so.1 symlinks.
Is this approach sane enough ?
> >
> > On 1/8/2019 5:45 PM, David Gibson wrote:
> > > From: Alexey Kardashevskiy <aik@ozlabs.ru>
> > >
> > > SLOF receives a device tree and updates it with various properties
> > > before switching to the guest kernel and QEMU is not aware of any changes
> > > made by SLOF. Since there is no real RTAS (QEMU implements it), it makes
> > > sense to pass the SLOF final device tree to QEMU to let it implement
> > > RTAS related tasks better, such as PCI host bus adapter hotplug.
> > >
> > > Specifially, now QEMU can find out the actual XICS phandle (for PHB
> > > hotplug) and the RTAS linux,rtas-entry/base properties (for firmware
> > > assisted NMI - FWNMI).
> > >
> > > This stores the initial DT blob in the sPAPR machine and replaces it
> > > in the KVMPPC_H_UPDATE_DT (new private hypercall) handler.
> > >
> > > This adds an @update_dt_enabled machine property to allow backward
> > > migration.
> > >
> > > SLOF already has a hypercall since
> > > https://github.com/aik/SLOF/commit/e6fc84652c9c0073f9183
> > >
> > > This makes use of the new fdt_check_full() helper. In order to allow
> > > the configure script to pick the correct DTC version, this adjusts
> > > the DTC presense test.
> > >
> > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> > > Reviewed-by: Greg Kurz <groug@kaod.org>
> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > ---
> > > configure | 2 +-
> > > hw/ppc/spapr.c | 43 +++++++++++++++++++++++++++++++++++++++++-
> > > hw/ppc/spapr_hcall.c | 42 +++++++++++++++++++++++++++++++++++++++++
> > > hw/ppc/trace-events | 3 +++
> > > include/hw/ppc/spapr.h | 7 ++++++-
> > > 5 files changed, 94 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/configure b/configure
> > > index b9f34afc9e..8049b71eef 100755
> > > --- a/configure
> > > +++ b/configure
> > > @@ -3939,7 +3939,7 @@ if test "$fdt" != "no" ; then
> > > cat > $TMPC << EOF
> > > #include <libfdt.h>
> > > #include <libfdt_env.h>
> > > -int main(void) { fdt_first_subnode(0, 0); return 0; }
> > > +int main(void) { fdt_check_full(NULL, 0); return 0; }
> > > EOF
> > > if compile_prog "" "$fdt_libs" ; then
> > > # system DTC is good - use it
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 5fba04e7b2..7e61f1e5ff 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -1669,7 +1669,10 @@ static void spapr_machine_reset(void)
> > > /* Load the fdt */
> > > qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
> > > cpu_physical_memory_write(fdt_addr, fdt, fdt_totalsize(fdt));
> > > - g_free(fdt);
> > > + g_free(spapr->fdt_blob);
> > > + spapr->fdt_size = fdt_totalsize(fdt);
> > > + spapr->fdt_initial_size = spapr->fdt_size;
> > > + spapr->fdt_blob = fdt;
> > > /* Set up the entry state */
> > > spapr_cpu_set_entry_state(first_ppc_cpu, SPAPR_ENTRY_POINT, fdt_addr);
> > > @@ -1920,6 +1923,39 @@ static const VMStateDescription vmstate_spapr_irq_map = {
> > > },
> > > };
> > > +static bool spapr_dtb_needed(void *opaque)
> > > +{
> > > + sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(opaque);
> > > +
> > > + return smc->update_dt_enabled;
> > > +}
> > > +
> > > +static int spapr_dtb_pre_load(void *opaque)
> > > +{
> > > + sPAPRMachineState *spapr = (sPAPRMachineState *)opaque;
> > > +
> > > + g_free(spapr->fdt_blob);
> > > + spapr->fdt_blob = NULL;
> > > + spapr->fdt_size = 0;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static const VMStateDescription vmstate_spapr_dtb = {
> > > + .name = "spapr_dtb",
> > > + .version_id = 1,
> > > + .minimum_version_id = 1,
> > > + .needed = spapr_dtb_needed,
> > > + .pre_load = spapr_dtb_pre_load,
> > > + .fields = (VMStateField[]) {
> > > + VMSTATE_UINT32(fdt_initial_size, sPAPRMachineState),
> > > + VMSTATE_UINT32(fdt_size, sPAPRMachineState),
> > > + VMSTATE_VBUFFER_ALLOC_UINT32(fdt_blob, sPAPRMachineState, 0, NULL,
> > > + fdt_size),
> > > + VMSTATE_END_OF_LIST()
> > > + },
> > > +};
> > > +
> > > static const VMStateDescription vmstate_spapr = {
> > > .name = "spapr",
> > > .version_id = 3,
> > > @@ -1949,6 +1985,7 @@ static const VMStateDescription vmstate_spapr = {
> > > &vmstate_spapr_cap_ibs,
> > > &vmstate_spapr_irq_map,
> > > &vmstate_spapr_cap_nested_kvm_hv,
> > > + &vmstate_spapr_dtb,
> > > NULL
> > > }
> > > };
> > > @@ -3931,6 +3968,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
> > > hc->unplug = spapr_machine_device_unplug;
> > > smc->dr_lmb_enabled = true;
> > > + smc->update_dt_enabled = true;
> > > mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power9_v2.0");
> > > mc->has_hotpluggable_cpus = true;
> > > smc->resize_hpt_default = SPAPR_RESIZE_HPT_ENABLED;
> > > @@ -4023,9 +4061,12 @@ DEFINE_SPAPR_MACHINE(4_0, "4.0", true);
> > > */
> > > static void spapr_machine_3_1_class_options(MachineClass *mc)
> > > {
> > > + sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
> > > +
> > > spapr_machine_4_0_class_options(mc);
> > > compat_props_add(mc->compat_props, hw_compat_3_1, hw_compat_3_1_len);
> > > mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power8_v2.0");
> > > + smc->update_dt_enabled = false;
> > > }
> > > DEFINE_SPAPR_MACHINE(3_1, "3.1", false);
> > > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> > > index f131c7e04c..1ae3e6ff5e 100644
> > > --- a/hw/ppc/spapr_hcall.c
> > > +++ b/hw/ppc/spapr_hcall.c
> > > @@ -1753,6 +1753,46 @@ static target_ulong h_get_cpu_characteristics(PowerPCCPU *cpu,
> > > args[0] = characteristics;
> > > args[1] = behaviour;
> > > + return H_SUCCESS;
> > > +}
> > > +
> > > +static target_ulong h_update_dt(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> > > + target_ulong opcode, target_ulong *args)
> > > +{
> > > + target_ulong dt = ppc64_phys_to_real(args[0]);
> > > + struct fdt_header hdr = { 0 };
> > > + unsigned cb;
> > > + sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> > > + void *fdt;
> > > +
> > > + cpu_physical_memory_read(dt, &hdr, sizeof(hdr));
> > > + cb = fdt32_to_cpu(hdr.totalsize);
> > > +
> > > + if (!smc->update_dt_enabled) {
> > > + return H_SUCCESS;
> > > + }
> > > +
> > > + /* Check that the fdt did not grow out of proportion */
> > > + if (cb > spapr->fdt_initial_size * 2) {
> > > + trace_spapr_update_dt_failed_size(spapr->fdt_initial_size, cb,
> > > + fdt32_to_cpu(hdr.magic));
> > > + return H_PARAMETER;
> > > + }
> > > +
> > > + fdt = g_malloc0(cb);
> > > + cpu_physical_memory_read(dt, fdt, cb);
> > > +
> > > + /* Check the fdt consistency */
> > > + if (fdt_check_full(fdt, cb)) {
> > > + trace_spapr_update_dt_failed_check(spapr->fdt_initial_size, cb,
> > > + fdt32_to_cpu(hdr.magic));
> > > + return H_PARAMETER;
> > > + }
> > > +
> > > + g_free(spapr->fdt_blob);
> > > + spapr->fdt_size = cb;
> > > + spapr->fdt_blob = fdt;
> > > + trace_spapr_update_dt(cb);
> > > return H_SUCCESS;
> > > }
> > > @@ -1859,6 +1899,8 @@ static void hypercall_register_types(void)
> > > /* ibm,client-architecture-support support */
> > > spapr_register_hypercall(KVMPPC_H_CAS, h_client_architecture_support);
> > > + spapr_register_hypercall(KVMPPC_H_UPDATE_DT, h_update_dt);
> > > +
> > > /* Virtual Processor Home Node */
> > > spapr_register_hypercall(H_HOME_NODE_ASSOCIATIVITY,
> > > h_home_node_associativity);
> > > diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
> > > index dc5e65aee9..0af155ed32 100644
> > > --- a/hw/ppc/trace-events
> > > +++ b/hw/ppc/trace-events
> > > @@ -22,6 +22,9 @@ spapr_cas_pvr_try(uint32_t pvr) "0x%x"
> > > spapr_cas_pvr(uint32_t cur_pvr, bool explicit_match, uint32_t new_pvr) "current=0x%x, explicit_match=%u, new=0x%x"
> > > spapr_h_resize_hpt_prepare(uint64_t flags, uint64_t shift) "flags=0x%"PRIx64", shift=%"PRIu64
> > > spapr_h_resize_hpt_commit(uint64_t flags, uint64_t shift) "flags=0x%"PRIx64", shift=%"PRIu64
> > > +spapr_update_dt(unsigned cb) "New blob %u bytes"
> > > +spapr_update_dt_failed_size(unsigned cbold, unsigned cbnew, unsigned magic) "Old blob %u bytes, new blob %u bytes, magic 0x%x"
> > > +spapr_update_dt_failed_check(unsigned cbold, unsigned cbnew, unsigned magic) "Old blob %u bytes, new blob %u bytes, magic 0x%x"
> > > # hw/ppc/spapr_iommu.c
> > > spapr_iommu_put(uint64_t liobn, uint64_t ioba, uint64_t tce, uint64_t ret) "liobn=0x%"PRIx64" ioba=0x%"PRIx64" tce=0x%"PRIx64" ret=%"PRId64
> > > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > > index eb04300d3b..fd24e91bd8 100644
> > > --- a/include/hw/ppc/spapr.h
> > > +++ b/include/hw/ppc/spapr.h
> > > @@ -103,6 +103,7 @@ struct sPAPRMachineClass {
> > > /*< public >*/
> > > bool dr_lmb_enabled; /* enable dynamic-reconfig/hotplug of LMBs */
> > > + bool update_dt_enabled; /* enable KVMPPC_H_UPDATE_DT */
> > > bool use_ohci_by_default; /* use USB-OHCI instead of XHCI */
> > > bool pre_2_10_has_unused_icps;
> > > bool legacy_irq_allocation;
> > > @@ -139,6 +140,9 @@ struct sPAPRMachineState {
> > > int vrma_adjust;
> > > ssize_t rtas_size;
> > > void *rtas_blob;
> > > + uint32_t fdt_size;
> > > + uint32_t fdt_initial_size;
> > > + void *fdt_blob;
> > > long kernel_size;
> > > bool kernel_le;
> > > uint32_t initrd_base;
> > > @@ -481,7 +485,8 @@ struct sPAPRMachineState {
> > > #define KVMPPC_H_LOGICAL_MEMOP (KVMPPC_HCALL_BASE + 0x1)
> > > /* Client Architecture support */
> > > #define KVMPPC_H_CAS (KVMPPC_HCALL_BASE + 0x2)
> > > -#define KVMPPC_HCALL_MAX KVMPPC_H_CAS
> > > +#define KVMPPC_H_UPDATE_DT (KVMPPC_HCALL_BASE + 0x3)
> > > +#define KVMPPC_HCALL_MAX KVMPPC_H_UPDATE_DT
> > > typedef struct sPAPRDeviceTreeUpdateHeader {
> > > uint32_t version_id;
> >
>
On Mon, Mar 25, 2019 at 05:33:21PM +0100, Greg Kurz wrote:
> On Mon, 25 Mar 2019 11:53:47 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
>
> > On Sun, Mar 24, 2019 at 12:03:54AM -0400, Brad Smith wrote:
> > > Now that I am checking out 4.0.0 rc's I see this diff is broken and
> > > depends on a function libfdt does not expose. The breakage is
> > > hidden by the fallback check in the configure script.
> >
> > Ah, bother. That keeps happening, unfortunately. I think it's
> > because so many people use libfdt embedded, rather than as a shared
> > library that we tend not to notice.
> >
>
> It's a bit more complicated. I do have latest libfdt packages on my laptop:
>
> libfdt-1.4.7-2.fc28.x86_64
> libfdt-devel-1.4.7-2.fc28.x86_64
>
> but I still end up using the embedded one and the build doesn't spot
> the missing symbols.
Sorry, I wasn't clear. I wasn't meaning in the context of qemu, but
for dtc generally. A large portion of the users are things like
u-boot that have to use dtc embedded, rather than as a shared
library. That's why we tend not to notice missing symbols from the
version script upstream.
> This happens because of several reasons:
>
> 1) configure unconditionally falls back to embedded if an error occurs with
> the system packages. And, as reported by Brad, the current 1.4.7 packages
> are broken indeed:
>
> $ objdump -T /usr/lib64/libfdt-1.4.7.so | grep fdt_check_full
> $
>
> 2) when building embedded, we only build the archive, not the shared lib.
>
> > I guess we should figure out how to force the testsuite to link
> > against the shared library rather than static to test for this. I'll
> > look into it if I have time (which is a big if).
> >
>
> I think we should rather build the embedded shared library using
> the 'libfdt' rule of the top-level makefile of the dtc sub-module
> and have QEMU to be linked against this share library instead of
> the static one. AFAIK, this is what gcc does when it finds both
> .a and .so.
Actually, I don't think this is a good idea. It means the resulting
qemu build would have to be installed with the libfdt image as well.
As well as complicating the install path, that means that the qemu
build will now actively conflict with a packaged libfdt, rather than
merely suboptimally failing to use it.
> And then we must do some -rpath magic to ensure that QEMU does
> use the embedded share library, and not the system one.
>
> The following proof-of-concept patch gives the idea:
>
> ---------------------------------------------------------------------
> diff --git a/Makefile b/Makefile
> index d8dad39c5db1..4cb19e4177a8 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -449,12 +449,14 @@ subdir-%:
> $(call quiet-command,$(MAKE) $(SUBDIR_MAKEFLAGS) -C $* V="$(V)" TARGET_DIR="$*/" all,)
>
> DTC_MAKE_ARGS=-I$(SRC_PATH)/dtc VPATH=$(SRC_PATH)/dtc -C dtc V="$(V)" LIBFDT_srcdir=$(SRC_PATH)/dtc/libfdt
> -DTC_CFLAGS=$(CFLAGS) $(QEMU_CFLAGS)
> +DTC_CFLAGS=$(CFLAGS) $(QEMU_CFLAGS) -fPIC
> DTC_CPPFLAGS=-I$(BUILD_DIR)/dtc -I$(SRC_PATH)/dtc -I$(SRC_PATH)/dtc/libfdt
> +DTC_LDFLAGS=-Wl,-rpath -Wl,"$(BUILD_DIR)"/dtc $(LDFLAGS)
>
> subdir-dtc: .git-submodule-status dtc/libfdt dtc/tests
> - $(call quiet-command,$(MAKE) $(DTC_MAKE_ARGS) CPPFLAGS="$(DTC_CPPFLAGS)" CFLAGS="$(DTC_CFLAGS)" LDFLAGS="$(LDFLAGS)" ARFLAGS="$(ARFLAGS)" CC="$(CC)" AR="$(AR)" LD="$(LD)" $(SUBDIR_MAKEFLAGS) libfdt/libfdt.a,)
> -
> + $(call quiet-command,$(MAKE) $(DTC_MAKE_ARGS) CPPFLAGS="$(DTC_CPPFLAGS)" CFLAGS="$(DTC_CFLAGS)" LDFLAGS="$(LDFLAGS)" ARFLAGS="$(ARFLAGS)" CC="$(CC)" AR="$(AR)" LD="$(LD)" $(SUBDIR_MAKEFLAGS) libfdt,)
> + $(call quiet-command, cd $(BUILD_DIR)/dtc/libfdt && \
> + ln -fs libfdt-*.so libfdt.so && ln -fs libfdt-*.so libfdt.so.1)
> dtc/%: .git-submodule-status
> @mkdir -p $@
>
> diff --git a/configure b/configure
> index c5032425e6d3..a9815841b09d 100755
> --- a/configure
> +++ b/configure
> @@ -4049,7 +4049,8 @@ EOF
> symlink "$source_path/dtc/scripts" "dtc/scripts"
> fi
> fdt_cflags="-I\$(SRC_PATH)/dtc/libfdt"
> - fdt_ldflags="-L\$(BUILD_DIR)/dtc/libfdt"
> + fdt_rpath="\$(BUILD_DIR)/dtc/libfdt"
> + fdt_ldflags="-L$fdt_rpath -Wl,-rpath -Wl,$fdt_rpath"
> fdt_libs="$fdt_libs"
> elif test "$fdt" = "yes" ; then
> # Not a git build & no libfdt found, prompt for system install
> ---------------------------------------------------------------------
>
> It detects the missing symbol:
>
> LINK ppc64-softmmu/qemu-system-ppc64
> hw/ppc/spapr_hcall.o: In function `h_update_dt':
> /home/greg/Work/qemu/qemu-master/hw/ppc/spapr_hcall.c:1784: undefined reference to `fdt_check_full'
>
> It makes sure we use the embedded library, after it got fixed, ie. adding
> fdt_check_full to dtc/libfdt/version.lds :
>
> [greg@bahia obj]$ ldd ppc64-softmmu/qemu-system-ppc64 | grep fdt
> libfdt.so.1 => /home/greg/Work/qemu/qemu-master/.mbuild-master/obj/dtc/libfdt/libfdt.so.1 (0x00007f68c5e90000)
>
> Note that I wanted to use the install-lib rule of dtc but unfortunately it
> depends on all, ie. tries to rebuild the whole dtc package whereas we only
> need libfdt. I'll try to find a cleaner way to create the libfdt.so and
> libfdt.so.1 symlinks.
>
> Is this approach sane enough ?
>
> > >
> > > On 1/8/2019 5:45 PM, David Gibson wrote:
> > > > From: Alexey Kardashevskiy <aik@ozlabs.ru>
> > > >
> > > > SLOF receives a device tree and updates it with various properties
> > > > before switching to the guest kernel and QEMU is not aware of any changes
> > > > made by SLOF. Since there is no real RTAS (QEMU implements it), it makes
> > > > sense to pass the SLOF final device tree to QEMU to let it implement
> > > > RTAS related tasks better, such as PCI host bus adapter hotplug.
> > > >
> > > > Specifially, now QEMU can find out the actual XICS phandle (for PHB
> > > > hotplug) and the RTAS linux,rtas-entry/base properties (for firmware
> > > > assisted NMI - FWNMI).
> > > >
> > > > This stores the initial DT blob in the sPAPR machine and replaces it
> > > > in the KVMPPC_H_UPDATE_DT (new private hypercall) handler.
> > > >
> > > > This adds an @update_dt_enabled machine property to allow backward
> > > > migration.
> > > >
> > > > SLOF already has a hypercall since
> > > > https://github.com/aik/SLOF/commit/e6fc84652c9c0073f9183
> > > >
> > > > This makes use of the new fdt_check_full() helper. In order to allow
> > > > the configure script to pick the correct DTC version, this adjusts
> > > > the DTC presense test.
> > > >
> > > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> > > > Reviewed-by: Greg Kurz <groug@kaod.org>
> > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > > ---
> > > > configure | 2 +-
> > > > hw/ppc/spapr.c | 43 +++++++++++++++++++++++++++++++++++++++++-
> > > > hw/ppc/spapr_hcall.c | 42 +++++++++++++++++++++++++++++++++++++++++
> > > > hw/ppc/trace-events | 3 +++
> > > > include/hw/ppc/spapr.h | 7 ++++++-
> > > > 5 files changed, 94 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/configure b/configure
> > > > index b9f34afc9e..8049b71eef 100755
> > > > --- a/configure
> > > > +++ b/configure
> > > > @@ -3939,7 +3939,7 @@ if test "$fdt" != "no" ; then
> > > > cat > $TMPC << EOF
> > > > #include <libfdt.h>
> > > > #include <libfdt_env.h>
> > > > -int main(void) { fdt_first_subnode(0, 0); return 0; }
> > > > +int main(void) { fdt_check_full(NULL, 0); return 0; }
> > > > EOF
> > > > if compile_prog "" "$fdt_libs" ; then
> > > > # system DTC is good - use it
> > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > index 5fba04e7b2..7e61f1e5ff 100644
> > > > --- a/hw/ppc/spapr.c
> > > > +++ b/hw/ppc/spapr.c
> > > > @@ -1669,7 +1669,10 @@ static void spapr_machine_reset(void)
> > > > /* Load the fdt */
> > > > qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
> > > > cpu_physical_memory_write(fdt_addr, fdt, fdt_totalsize(fdt));
> > > > - g_free(fdt);
> > > > + g_free(spapr->fdt_blob);
> > > > + spapr->fdt_size = fdt_totalsize(fdt);
> > > > + spapr->fdt_initial_size = spapr->fdt_size;
> > > > + spapr->fdt_blob = fdt;
> > > > /* Set up the entry state */
> > > > spapr_cpu_set_entry_state(first_ppc_cpu, SPAPR_ENTRY_POINT, fdt_addr);
> > > > @@ -1920,6 +1923,39 @@ static const VMStateDescription vmstate_spapr_irq_map = {
> > > > },
> > > > };
> > > > +static bool spapr_dtb_needed(void *opaque)
> > > > +{
> > > > + sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(opaque);
> > > > +
> > > > + return smc->update_dt_enabled;
> > > > +}
> > > > +
> > > > +static int spapr_dtb_pre_load(void *opaque)
> > > > +{
> > > > + sPAPRMachineState *spapr = (sPAPRMachineState *)opaque;
> > > > +
> > > > + g_free(spapr->fdt_blob);
> > > > + spapr->fdt_blob = NULL;
> > > > + spapr->fdt_size = 0;
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static const VMStateDescription vmstate_spapr_dtb = {
> > > > + .name = "spapr_dtb",
> > > > + .version_id = 1,
> > > > + .minimum_version_id = 1,
> > > > + .needed = spapr_dtb_needed,
> > > > + .pre_load = spapr_dtb_pre_load,
> > > > + .fields = (VMStateField[]) {
> > > > + VMSTATE_UINT32(fdt_initial_size, sPAPRMachineState),
> > > > + VMSTATE_UINT32(fdt_size, sPAPRMachineState),
> > > > + VMSTATE_VBUFFER_ALLOC_UINT32(fdt_blob, sPAPRMachineState, 0, NULL,
> > > > + fdt_size),
> > > > + VMSTATE_END_OF_LIST()
> > > > + },
> > > > +};
> > > > +
> > > > static const VMStateDescription vmstate_spapr = {
> > > > .name = "spapr",
> > > > .version_id = 3,
> > > > @@ -1949,6 +1985,7 @@ static const VMStateDescription vmstate_spapr = {
> > > > &vmstate_spapr_cap_ibs,
> > > > &vmstate_spapr_irq_map,
> > > > &vmstate_spapr_cap_nested_kvm_hv,
> > > > + &vmstate_spapr_dtb,
> > > > NULL
> > > > }
> > > > };
> > > > @@ -3931,6 +3968,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
> > > > hc->unplug = spapr_machine_device_unplug;
> > > > smc->dr_lmb_enabled = true;
> > > > + smc->update_dt_enabled = true;
> > > > mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power9_v2.0");
> > > > mc->has_hotpluggable_cpus = true;
> > > > smc->resize_hpt_default = SPAPR_RESIZE_HPT_ENABLED;
> > > > @@ -4023,9 +4061,12 @@ DEFINE_SPAPR_MACHINE(4_0, "4.0", true);
> > > > */
> > > > static void spapr_machine_3_1_class_options(MachineClass *mc)
> > > > {
> > > > + sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
> > > > +
> > > > spapr_machine_4_0_class_options(mc);
> > > > compat_props_add(mc->compat_props, hw_compat_3_1, hw_compat_3_1_len);
> > > > mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power8_v2.0");
> > > > + smc->update_dt_enabled = false;
> > > > }
> > > > DEFINE_SPAPR_MACHINE(3_1, "3.1", false);
> > > > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> > > > index f131c7e04c..1ae3e6ff5e 100644
> > > > --- a/hw/ppc/spapr_hcall.c
> > > > +++ b/hw/ppc/spapr_hcall.c
> > > > @@ -1753,6 +1753,46 @@ static target_ulong h_get_cpu_characteristics(PowerPCCPU *cpu,
> > > > args[0] = characteristics;
> > > > args[1] = behaviour;
> > > > + return H_SUCCESS;
> > > > +}
> > > > +
> > > > +static target_ulong h_update_dt(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> > > > + target_ulong opcode, target_ulong *args)
> > > > +{
> > > > + target_ulong dt = ppc64_phys_to_real(args[0]);
> > > > + struct fdt_header hdr = { 0 };
> > > > + unsigned cb;
> > > > + sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> > > > + void *fdt;
> > > > +
> > > > + cpu_physical_memory_read(dt, &hdr, sizeof(hdr));
> > > > + cb = fdt32_to_cpu(hdr.totalsize);
> > > > +
> > > > + if (!smc->update_dt_enabled) {
> > > > + return H_SUCCESS;
> > > > + }
> > > > +
> > > > + /* Check that the fdt did not grow out of proportion */
> > > > + if (cb > spapr->fdt_initial_size * 2) {
> > > > + trace_spapr_update_dt_failed_size(spapr->fdt_initial_size, cb,
> > > > + fdt32_to_cpu(hdr.magic));
> > > > + return H_PARAMETER;
> > > > + }
> > > > +
> > > > + fdt = g_malloc0(cb);
> > > > + cpu_physical_memory_read(dt, fdt, cb);
> > > > +
> > > > + /* Check the fdt consistency */
> > > > + if (fdt_check_full(fdt, cb)) {
> > > > + trace_spapr_update_dt_failed_check(spapr->fdt_initial_size, cb,
> > > > + fdt32_to_cpu(hdr.magic));
> > > > + return H_PARAMETER;
> > > > + }
> > > > +
> > > > + g_free(spapr->fdt_blob);
> > > > + spapr->fdt_size = cb;
> > > > + spapr->fdt_blob = fdt;
> > > > + trace_spapr_update_dt(cb);
> > > > return H_SUCCESS;
> > > > }
> > > > @@ -1859,6 +1899,8 @@ static void hypercall_register_types(void)
> > > > /* ibm,client-architecture-support support */
> > > > spapr_register_hypercall(KVMPPC_H_CAS, h_client_architecture_support);
> > > > + spapr_register_hypercall(KVMPPC_H_UPDATE_DT, h_update_dt);
> > > > +
> > > > /* Virtual Processor Home Node */
> > > > spapr_register_hypercall(H_HOME_NODE_ASSOCIATIVITY,
> > > > h_home_node_associativity);
> > > > diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
> > > > index dc5e65aee9..0af155ed32 100644
> > > > --- a/hw/ppc/trace-events
> > > > +++ b/hw/ppc/trace-events
> > > > @@ -22,6 +22,9 @@ spapr_cas_pvr_try(uint32_t pvr) "0x%x"
> > > > spapr_cas_pvr(uint32_t cur_pvr, bool explicit_match, uint32_t new_pvr) "current=0x%x, explicit_match=%u, new=0x%x"
> > > > spapr_h_resize_hpt_prepare(uint64_t flags, uint64_t shift) "flags=0x%"PRIx64", shift=%"PRIu64
> > > > spapr_h_resize_hpt_commit(uint64_t flags, uint64_t shift) "flags=0x%"PRIx64", shift=%"PRIu64
> > > > +spapr_update_dt(unsigned cb) "New blob %u bytes"
> > > > +spapr_update_dt_failed_size(unsigned cbold, unsigned cbnew, unsigned magic) "Old blob %u bytes, new blob %u bytes, magic 0x%x"
> > > > +spapr_update_dt_failed_check(unsigned cbold, unsigned cbnew, unsigned magic) "Old blob %u bytes, new blob %u bytes, magic 0x%x"
> > > > # hw/ppc/spapr_iommu.c
> > > > spapr_iommu_put(uint64_t liobn, uint64_t ioba, uint64_t tce, uint64_t ret) "liobn=0x%"PRIx64" ioba=0x%"PRIx64" tce=0x%"PRIx64" ret=%"PRId64
> > > > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > > > index eb04300d3b..fd24e91bd8 100644
> > > > --- a/include/hw/ppc/spapr.h
> > > > +++ b/include/hw/ppc/spapr.h
> > > > @@ -103,6 +103,7 @@ struct sPAPRMachineClass {
> > > > /*< public >*/
> > > > bool dr_lmb_enabled; /* enable dynamic-reconfig/hotplug of LMBs */
> > > > + bool update_dt_enabled; /* enable KVMPPC_H_UPDATE_DT */
> > > > bool use_ohci_by_default; /* use USB-OHCI instead of XHCI */
> > > > bool pre_2_10_has_unused_icps;
> > > > bool legacy_irq_allocation;
> > > > @@ -139,6 +140,9 @@ struct sPAPRMachineState {
> > > > int vrma_adjust;
> > > > ssize_t rtas_size;
> > > > void *rtas_blob;
> > > > + uint32_t fdt_size;
> > > > + uint32_t fdt_initial_size;
> > > > + void *fdt_blob;
> > > > long kernel_size;
> > > > bool kernel_le;
> > > > uint32_t initrd_base;
> > > > @@ -481,7 +485,8 @@ struct sPAPRMachineState {
> > > > #define KVMPPC_H_LOGICAL_MEMOP (KVMPPC_HCALL_BASE + 0x1)
> > > > /* Client Architecture support */
> > > > #define KVMPPC_H_CAS (KVMPPC_HCALL_BASE + 0x2)
> > > > -#define KVMPPC_HCALL_MAX KVMPPC_H_CAS
> > > > +#define KVMPPC_H_UPDATE_DT (KVMPPC_HCALL_BASE + 0x3)
> > > > +#define KVMPPC_HCALL_MAX KVMPPC_H_UPDATE_DT
> > > > typedef struct sPAPRDeviceTreeUpdateHeader {
> > > > uint32_t version_id;
> > >
> >
>
--
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 Tue, 26 Mar 2019 10:47:15 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:
> On Mon, Mar 25, 2019 at 05:33:21PM +0100, Greg Kurz wrote:
> > On Mon, 25 Mar 2019 11:53:47 +1100
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >
> > > On Sun, Mar 24, 2019 at 12:03:54AM -0400, Brad Smith wrote:
> > > > Now that I am checking out 4.0.0 rc's I see this diff is broken and
> > > > depends on a function libfdt does not expose. The breakage is
> > > > hidden by the fallback check in the configure script.
> > >
> > > Ah, bother. That keeps happening, unfortunately. I think it's
> > > because so many people use libfdt embedded, rather than as a shared
> > > library that we tend not to notice.
> > >
> >
> > It's a bit more complicated. I do have latest libfdt packages on my laptop:
> >
> > libfdt-1.4.7-2.fc28.x86_64
> > libfdt-devel-1.4.7-2.fc28.x86_64
> >
> > but I still end up using the embedded one and the build doesn't spot
> > the missing symbols.
>
> Sorry, I wasn't clear. I wasn't meaning in the context of qemu, but
> for dtc generally. A large portion of the users are things like
> u-boot that have to use dtc embedded, rather than as a shared
> library. That's why we tend not to notice missing symbols from the
> version script upstream.
>
Ok, I get it.
> > This happens because of several reasons:
> >
> > 1) configure unconditionally falls back to embedded if an error occurs with
> > the system packages. And, as reported by Brad, the current 1.4.7 packages
> > are broken indeed:
> >
> > $ objdump -T /usr/lib64/libfdt-1.4.7.so | grep fdt_check_full
> > $
> >
> > 2) when building embedded, we only build the archive, not the shared lib.
> >
> > > I guess we should figure out how to force the testsuite to link
> > > against the shared library rather than static to test for this. I'll
> > > look into it if I have time (which is a big if).
> > >
> >
> > I think we should rather build the embedded shared library using
> > the 'libfdt' rule of the top-level makefile of the dtc sub-module
> > and have QEMU to be linked against this share library instead of
> > the static one. AFAIK, this is what gcc does when it finds both
> > .a and .so.
>
> Actually, I don't think this is a good idea. It means the resulting
> qemu build would have to be installed with the libfdt image as well.
> As well as complicating the install path, that means that the qemu
> build will now actively conflict with a packaged libfdt, rather than
> merely suboptimally failing to use it.
>
Yes you're right: the resulting QEMU shouldn't be installed, especially
if it has a RPATH poiting to the build directory.
This being said, if someone wants to build AND install QEMU, shouldn't
she rely exclusively on installed libfdt packages ? In other words,
shouldn't the embedded libfdt be a QEMU developper only thing ? What
are the real life use cases for embedded libfdt ?
> > And then we must do some -rpath magic to ensure that QEMU does
> > use the embedded share library, and not the system one.
> >
> > The following proof-of-concept patch gives the idea:
> >
> > ---------------------------------------------------------------------
> > diff --git a/Makefile b/Makefile
> > index d8dad39c5db1..4cb19e4177a8 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -449,12 +449,14 @@ subdir-%:
> > $(call quiet-command,$(MAKE) $(SUBDIR_MAKEFLAGS) -C $* V="$(V)" TARGET_DIR="$*/" all,)
> >
> > DTC_MAKE_ARGS=-I$(SRC_PATH)/dtc VPATH=$(SRC_PATH)/dtc -C dtc V="$(V)" LIBFDT_srcdir=$(SRC_PATH)/dtc/libfdt
> > -DTC_CFLAGS=$(CFLAGS) $(QEMU_CFLAGS)
> > +DTC_CFLAGS=$(CFLAGS) $(QEMU_CFLAGS) -fPIC
> > DTC_CPPFLAGS=-I$(BUILD_DIR)/dtc -I$(SRC_PATH)/dtc -I$(SRC_PATH)/dtc/libfdt
> > +DTC_LDFLAGS=-Wl,-rpath -Wl,"$(BUILD_DIR)"/dtc $(LDFLAGS)
> >
> > subdir-dtc: .git-submodule-status dtc/libfdt dtc/tests
> > - $(call quiet-command,$(MAKE) $(DTC_MAKE_ARGS) CPPFLAGS="$(DTC_CPPFLAGS)" CFLAGS="$(DTC_CFLAGS)" LDFLAGS="$(LDFLAGS)" ARFLAGS="$(ARFLAGS)" CC="$(CC)" AR="$(AR)" LD="$(LD)" $(SUBDIR_MAKEFLAGS) libfdt/libfdt.a,)
> > -
> > + $(call quiet-command,$(MAKE) $(DTC_MAKE_ARGS) CPPFLAGS="$(DTC_CPPFLAGS)" CFLAGS="$(DTC_CFLAGS)" LDFLAGS="$(LDFLAGS)" ARFLAGS="$(ARFLAGS)" CC="$(CC)" AR="$(AR)" LD="$(LD)" $(SUBDIR_MAKEFLAGS) libfdt,)
> > + $(call quiet-command, cd $(BUILD_DIR)/dtc/libfdt && \
> > + ln -fs libfdt-*.so libfdt.so && ln -fs libfdt-*.so libfdt.so.1)
> > dtc/%: .git-submodule-status
> > @mkdir -p $@
> >
> > diff --git a/configure b/configure
> > index c5032425e6d3..a9815841b09d 100755
> > --- a/configure
> > +++ b/configure
> > @@ -4049,7 +4049,8 @@ EOF
> > symlink "$source_path/dtc/scripts" "dtc/scripts"
> > fi
> > fdt_cflags="-I\$(SRC_PATH)/dtc/libfdt"
> > - fdt_ldflags="-L\$(BUILD_DIR)/dtc/libfdt"
> > + fdt_rpath="\$(BUILD_DIR)/dtc/libfdt"
> > + fdt_ldflags="-L$fdt_rpath -Wl,-rpath -Wl,$fdt_rpath"
> > fdt_libs="$fdt_libs"
> > elif test "$fdt" = "yes" ; then
> > # Not a git build & no libfdt found, prompt for system install
> > ---------------------------------------------------------------------
> >
> > It detects the missing symbol:
> >
> > LINK ppc64-softmmu/qemu-system-ppc64
> > hw/ppc/spapr_hcall.o: In function `h_update_dt':
> > /home/greg/Work/qemu/qemu-master/hw/ppc/spapr_hcall.c:1784: undefined reference to `fdt_check_full'
> >
> > It makes sure we use the embedded library, after it got fixed, ie. adding
> > fdt_check_full to dtc/libfdt/version.lds :
> >
> > [greg@bahia obj]$ ldd ppc64-softmmu/qemu-system-ppc64 | grep fdt
> > libfdt.so.1 => /home/greg/Work/qemu/qemu-master/.mbuild-master/obj/dtc/libfdt/libfdt.so.1 (0x00007f68c5e90000)
> >
> > Note that I wanted to use the install-lib rule of dtc but unfortunately it
> > depends on all, ie. tries to rebuild the whole dtc package whereas we only
> > need libfdt. I'll try to find a cleaner way to create the libfdt.so and
> > libfdt.so.1 symlinks.
> >
> > Is this approach sane enough ?
> >
> > > >
> > > > On 1/8/2019 5:45 PM, David Gibson wrote:
> > > > > From: Alexey Kardashevskiy <aik@ozlabs.ru>
> > > > >
> > > > > SLOF receives a device tree and updates it with various properties
> > > > > before switching to the guest kernel and QEMU is not aware of any changes
> > > > > made by SLOF. Since there is no real RTAS (QEMU implements it), it makes
> > > > > sense to pass the SLOF final device tree to QEMU to let it implement
> > > > > RTAS related tasks better, such as PCI host bus adapter hotplug.
> > > > >
> > > > > Specifially, now QEMU can find out the actual XICS phandle (for PHB
> > > > > hotplug) and the RTAS linux,rtas-entry/base properties (for firmware
> > > > > assisted NMI - FWNMI).
> > > > >
> > > > > This stores the initial DT blob in the sPAPR machine and replaces it
> > > > > in the KVMPPC_H_UPDATE_DT (new private hypercall) handler.
> > > > >
> > > > > This adds an @update_dt_enabled machine property to allow backward
> > > > > migration.
> > > > >
> > > > > SLOF already has a hypercall since
> > > > > https://github.com/aik/SLOF/commit/e6fc84652c9c0073f9183
> > > > >
> > > > > This makes use of the new fdt_check_full() helper. In order to allow
> > > > > the configure script to pick the correct DTC version, this adjusts
> > > > > the DTC presense test.
> > > > >
> > > > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> > > > > Reviewed-by: Greg Kurz <groug@kaod.org>
> > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > > > ---
> > > > > configure | 2 +-
> > > > > hw/ppc/spapr.c | 43 +++++++++++++++++++++++++++++++++++++++++-
> > > > > hw/ppc/spapr_hcall.c | 42 +++++++++++++++++++++++++++++++++++++++++
> > > > > hw/ppc/trace-events | 3 +++
> > > > > include/hw/ppc/spapr.h | 7 ++++++-
> > > > > 5 files changed, 94 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/configure b/configure
> > > > > index b9f34afc9e..8049b71eef 100755
> > > > > --- a/configure
> > > > > +++ b/configure
> > > > > @@ -3939,7 +3939,7 @@ if test "$fdt" != "no" ; then
> > > > > cat > $TMPC << EOF
> > > > > #include <libfdt.h>
> > > > > #include <libfdt_env.h>
> > > > > -int main(void) { fdt_first_subnode(0, 0); return 0; }
> > > > > +int main(void) { fdt_check_full(NULL, 0); return 0; }
> > > > > EOF
> > > > > if compile_prog "" "$fdt_libs" ; then
> > > > > # system DTC is good - use it
> > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > > index 5fba04e7b2..7e61f1e5ff 100644
> > > > > --- a/hw/ppc/spapr.c
> > > > > +++ b/hw/ppc/spapr.c
> > > > > @@ -1669,7 +1669,10 @@ static void spapr_machine_reset(void)
> > > > > /* Load the fdt */
> > > > > qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
> > > > > cpu_physical_memory_write(fdt_addr, fdt, fdt_totalsize(fdt));
> > > > > - g_free(fdt);
> > > > > + g_free(spapr->fdt_blob);
> > > > > + spapr->fdt_size = fdt_totalsize(fdt);
> > > > > + spapr->fdt_initial_size = spapr->fdt_size;
> > > > > + spapr->fdt_blob = fdt;
> > > > > /* Set up the entry state */
> > > > > spapr_cpu_set_entry_state(first_ppc_cpu, SPAPR_ENTRY_POINT, fdt_addr);
> > > > > @@ -1920,6 +1923,39 @@ static const VMStateDescription vmstate_spapr_irq_map = {
> > > > > },
> > > > > };
> > > > > +static bool spapr_dtb_needed(void *opaque)
> > > > > +{
> > > > > + sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(opaque);
> > > > > +
> > > > > + return smc->update_dt_enabled;
> > > > > +}
> > > > > +
> > > > > +static int spapr_dtb_pre_load(void *opaque)
> > > > > +{
> > > > > + sPAPRMachineState *spapr = (sPAPRMachineState *)opaque;
> > > > > +
> > > > > + g_free(spapr->fdt_blob);
> > > > > + spapr->fdt_blob = NULL;
> > > > > + spapr->fdt_size = 0;
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > +static const VMStateDescription vmstate_spapr_dtb = {
> > > > > + .name = "spapr_dtb",
> > > > > + .version_id = 1,
> > > > > + .minimum_version_id = 1,
> > > > > + .needed = spapr_dtb_needed,
> > > > > + .pre_load = spapr_dtb_pre_load,
> > > > > + .fields = (VMStateField[]) {
> > > > > + VMSTATE_UINT32(fdt_initial_size, sPAPRMachineState),
> > > > > + VMSTATE_UINT32(fdt_size, sPAPRMachineState),
> > > > > + VMSTATE_VBUFFER_ALLOC_UINT32(fdt_blob, sPAPRMachineState, 0, NULL,
> > > > > + fdt_size),
> > > > > + VMSTATE_END_OF_LIST()
> > > > > + },
> > > > > +};
> > > > > +
> > > > > static const VMStateDescription vmstate_spapr = {
> > > > > .name = "spapr",
> > > > > .version_id = 3,
> > > > > @@ -1949,6 +1985,7 @@ static const VMStateDescription vmstate_spapr = {
> > > > > &vmstate_spapr_cap_ibs,
> > > > > &vmstate_spapr_irq_map,
> > > > > &vmstate_spapr_cap_nested_kvm_hv,
> > > > > + &vmstate_spapr_dtb,
> > > > > NULL
> > > > > }
> > > > > };
> > > > > @@ -3931,6 +3968,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
> > > > > hc->unplug = spapr_machine_device_unplug;
> > > > > smc->dr_lmb_enabled = true;
> > > > > + smc->update_dt_enabled = true;
> > > > > mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power9_v2.0");
> > > > > mc->has_hotpluggable_cpus = true;
> > > > > smc->resize_hpt_default = SPAPR_RESIZE_HPT_ENABLED;
> > > > > @@ -4023,9 +4061,12 @@ DEFINE_SPAPR_MACHINE(4_0, "4.0", true);
> > > > > */
> > > > > static void spapr_machine_3_1_class_options(MachineClass *mc)
> > > > > {
> > > > > + sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
> > > > > +
> > > > > spapr_machine_4_0_class_options(mc);
> > > > > compat_props_add(mc->compat_props, hw_compat_3_1, hw_compat_3_1_len);
> > > > > mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power8_v2.0");
> > > > > + smc->update_dt_enabled = false;
> > > > > }
> > > > > DEFINE_SPAPR_MACHINE(3_1, "3.1", false);
> > > > > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> > > > > index f131c7e04c..1ae3e6ff5e 100644
> > > > > --- a/hw/ppc/spapr_hcall.c
> > > > > +++ b/hw/ppc/spapr_hcall.c
> > > > > @@ -1753,6 +1753,46 @@ static target_ulong h_get_cpu_characteristics(PowerPCCPU *cpu,
> > > > > args[0] = characteristics;
> > > > > args[1] = behaviour;
> > > > > + return H_SUCCESS;
> > > > > +}
> > > > > +
> > > > > +static target_ulong h_update_dt(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> > > > > + target_ulong opcode, target_ulong *args)
> > > > > +{
> > > > > + target_ulong dt = ppc64_phys_to_real(args[0]);
> > > > > + struct fdt_header hdr = { 0 };
> > > > > + unsigned cb;
> > > > > + sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> > > > > + void *fdt;
> > > > > +
> > > > > + cpu_physical_memory_read(dt, &hdr, sizeof(hdr));
> > > > > + cb = fdt32_to_cpu(hdr.totalsize);
> > > > > +
> > > > > + if (!smc->update_dt_enabled) {
> > > > > + return H_SUCCESS;
> > > > > + }
> > > > > +
> > > > > + /* Check that the fdt did not grow out of proportion */
> > > > > + if (cb > spapr->fdt_initial_size * 2) {
> > > > > + trace_spapr_update_dt_failed_size(spapr->fdt_initial_size, cb,
> > > > > + fdt32_to_cpu(hdr.magic));
> > > > > + return H_PARAMETER;
> > > > > + }
> > > > > +
> > > > > + fdt = g_malloc0(cb);
> > > > > + cpu_physical_memory_read(dt, fdt, cb);
> > > > > +
> > > > > + /* Check the fdt consistency */
> > > > > + if (fdt_check_full(fdt, cb)) {
> > > > > + trace_spapr_update_dt_failed_check(spapr->fdt_initial_size, cb,
> > > > > + fdt32_to_cpu(hdr.magic));
> > > > > + return H_PARAMETER;
> > > > > + }
> > > > > +
> > > > > + g_free(spapr->fdt_blob);
> > > > > + spapr->fdt_size = cb;
> > > > > + spapr->fdt_blob = fdt;
> > > > > + trace_spapr_update_dt(cb);
> > > > > return H_SUCCESS;
> > > > > }
> > > > > @@ -1859,6 +1899,8 @@ static void hypercall_register_types(void)
> > > > > /* ibm,client-architecture-support support */
> > > > > spapr_register_hypercall(KVMPPC_H_CAS, h_client_architecture_support);
> > > > > + spapr_register_hypercall(KVMPPC_H_UPDATE_DT, h_update_dt);
> > > > > +
> > > > > /* Virtual Processor Home Node */
> > > > > spapr_register_hypercall(H_HOME_NODE_ASSOCIATIVITY,
> > > > > h_home_node_associativity);
> > > > > diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
> > > > > index dc5e65aee9..0af155ed32 100644
> > > > > --- a/hw/ppc/trace-events
> > > > > +++ b/hw/ppc/trace-events
> > > > > @@ -22,6 +22,9 @@ spapr_cas_pvr_try(uint32_t pvr) "0x%x"
> > > > > spapr_cas_pvr(uint32_t cur_pvr, bool explicit_match, uint32_t new_pvr) "current=0x%x, explicit_match=%u, new=0x%x"
> > > > > spapr_h_resize_hpt_prepare(uint64_t flags, uint64_t shift) "flags=0x%"PRIx64", shift=%"PRIu64
> > > > > spapr_h_resize_hpt_commit(uint64_t flags, uint64_t shift) "flags=0x%"PRIx64", shift=%"PRIu64
> > > > > +spapr_update_dt(unsigned cb) "New blob %u bytes"
> > > > > +spapr_update_dt_failed_size(unsigned cbold, unsigned cbnew, unsigned magic) "Old blob %u bytes, new blob %u bytes, magic 0x%x"
> > > > > +spapr_update_dt_failed_check(unsigned cbold, unsigned cbnew, unsigned magic) "Old blob %u bytes, new blob %u bytes, magic 0x%x"
> > > > > # hw/ppc/spapr_iommu.c
> > > > > spapr_iommu_put(uint64_t liobn, uint64_t ioba, uint64_t tce, uint64_t ret) "liobn=0x%"PRIx64" ioba=0x%"PRIx64" tce=0x%"PRIx64" ret=%"PRId64
> > > > > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > > > > index eb04300d3b..fd24e91bd8 100644
> > > > > --- a/include/hw/ppc/spapr.h
> > > > > +++ b/include/hw/ppc/spapr.h
> > > > > @@ -103,6 +103,7 @@ struct sPAPRMachineClass {
> > > > > /*< public >*/
> > > > > bool dr_lmb_enabled; /* enable dynamic-reconfig/hotplug of LMBs */
> > > > > + bool update_dt_enabled; /* enable KVMPPC_H_UPDATE_DT */
> > > > > bool use_ohci_by_default; /* use USB-OHCI instead of XHCI */
> > > > > bool pre_2_10_has_unused_icps;
> > > > > bool legacy_irq_allocation;
> > > > > @@ -139,6 +140,9 @@ struct sPAPRMachineState {
> > > > > int vrma_adjust;
> > > > > ssize_t rtas_size;
> > > > > void *rtas_blob;
> > > > > + uint32_t fdt_size;
> > > > > + uint32_t fdt_initial_size;
> > > > > + void *fdt_blob;
> > > > > long kernel_size;
> > > > > bool kernel_le;
> > > > > uint32_t initrd_base;
> > > > > @@ -481,7 +485,8 @@ struct sPAPRMachineState {
> > > > > #define KVMPPC_H_LOGICAL_MEMOP (KVMPPC_HCALL_BASE + 0x1)
> > > > > /* Client Architecture support */
> > > > > #define KVMPPC_H_CAS (KVMPPC_HCALL_BASE + 0x2)
> > > > > -#define KVMPPC_HCALL_MAX KVMPPC_H_CAS
> > > > > +#define KVMPPC_H_UPDATE_DT (KVMPPC_HCALL_BASE + 0x3)
> > > > > +#define KVMPPC_HCALL_MAX KVMPPC_H_UPDATE_DT
> > > > > typedef struct sPAPRDeviceTreeUpdateHeader {
> > > > > uint32_t version_id;
> > > >
> > >
> >
>
>
>
On Tue, Mar 26, 2019 at 08:09:53AM +0100, Greg Kurz wrote: > On Tue, 26 Mar 2019 10:47:15 +1100 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > On Mon, Mar 25, 2019 at 05:33:21PM +0100, Greg Kurz wrote: > > > On Mon, 25 Mar 2019 11:53:47 +1100 > > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > > > On Sun, Mar 24, 2019 at 12:03:54AM -0400, Brad Smith wrote: > > > > > Now that I am checking out 4.0.0 rc's I see this diff is broken and > > > > > depends on a function libfdt does not expose. The breakage is > > > > > hidden by the fallback check in the configure script. > > > > > > > > Ah, bother. That keeps happening, unfortunately. I think it's > > > > because so many people use libfdt embedded, rather than as a shared > > > > library that we tend not to notice. > > > > > > > > > > It's a bit more complicated. I do have latest libfdt packages on my laptop: > > > > > > libfdt-1.4.7-2.fc28.x86_64 > > > libfdt-devel-1.4.7-2.fc28.x86_64 > > > > > > but I still end up using the embedded one and the build doesn't spot > > > the missing symbols. > > > > Sorry, I wasn't clear. I wasn't meaning in the context of qemu, but > > for dtc generally. A large portion of the users are things like > > u-boot that have to use dtc embedded, rather than as a shared > > library. That's why we tend not to notice missing symbols from the > > version script upstream. > > > > Ok, I get it. > > > > This happens because of several reasons: > > > > > > 1) configure unconditionally falls back to embedded if an error occurs with > > > the system packages. And, as reported by Brad, the current 1.4.7 packages > > > are broken indeed: > > > > > > $ objdump -T /usr/lib64/libfdt-1.4.7.so | grep fdt_check_full > > > $ > > > > > > 2) when building embedded, we only build the archive, not the shared lib. > > > > > > > I guess we should figure out how to force the testsuite to link > > > > against the shared library rather than static to test for this. I'll > > > > look into it if I have time (which is a big if). > > > > > > > > > > I think we should rather build the embedded shared library using > > > the 'libfdt' rule of the top-level makefile of the dtc sub-module > > > and have QEMU to be linked against this share library instead of > > > the static one. AFAIK, this is what gcc does when it finds both > > > .a and .so. > > > > Actually, I don't think this is a good idea. It means the resulting > > qemu build would have to be installed with the libfdt image as well. > > As well as complicating the install path, that means that the qemu > > build will now actively conflict with a packaged libfdt, rather than > > merely suboptimally failing to use it. > > Yes you're right: the resulting QEMU shouldn't be installed, especially > if it has a RPATH poiting to the build directory. > > This being said, if someone wants to build AND install QEMU, shouldn't > she rely exclusively on installed libfdt packages ? In other words, > shouldn't the embedded libfdt be a QEMU developper only thing ? What > are the real life use cases for embedded libfdt ? I don't think we should insist on that, although it's certainly the way distros will usually work. If someone wants to build and install qemu locally, I don't think we should insist they first work out how to install a new enough libfdt for it to use. Likewise a limited purpose distro for whom qemu is the only user of libfdt might not want to package it separately. -- 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 Wed, 27 Mar 2019 11:56:10 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:
> On Tue, Mar 26, 2019 at 08:09:53AM +0100, Greg Kurz wrote:
> > On Tue, 26 Mar 2019 10:47:15 +1100
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >
> > > On Mon, Mar 25, 2019 at 05:33:21PM +0100, Greg Kurz wrote:
> > > > On Mon, 25 Mar 2019 11:53:47 +1100
> > > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > >
> > > > > On Sun, Mar 24, 2019 at 12:03:54AM -0400, Brad Smith wrote:
> > > > > > Now that I am checking out 4.0.0 rc's I see this diff is broken and
> > > > > > depends on a function libfdt does not expose. The breakage is
> > > > > > hidden by the fallback check in the configure script.
> > > > >
> > > > > Ah, bother. That keeps happening, unfortunately. I think it's
> > > > > because so many people use libfdt embedded, rather than as a shared
> > > > > library that we tend not to notice.
> > > > >
> > > >
> > > > It's a bit more complicated. I do have latest libfdt packages on my laptop:
> > > >
> > > > libfdt-1.4.7-2.fc28.x86_64
> > > > libfdt-devel-1.4.7-2.fc28.x86_64
> > > >
> > > > but I still end up using the embedded one and the build doesn't spot
> > > > the missing symbols.
> > >
> > > Sorry, I wasn't clear. I wasn't meaning in the context of qemu, but
> > > for dtc generally. A large portion of the users are things like
> > > u-boot that have to use dtc embedded, rather than as a shared
> > > library. That's why we tend not to notice missing symbols from the
> > > version script upstream.
> > >
> >
> > Ok, I get it.
> >
> > > > This happens because of several reasons:
> > > >
> > > > 1) configure unconditionally falls back to embedded if an error occurs with
> > > > the system packages. And, as reported by Brad, the current 1.4.7 packages
> > > > are broken indeed:
> > > >
> > > > $ objdump -T /usr/lib64/libfdt-1.4.7.so | grep fdt_check_full
> > > > $
> > > >
> > > > 2) when building embedded, we only build the archive, not the shared lib.
> > > >
> > > > > I guess we should figure out how to force the testsuite to link
> > > > > against the shared library rather than static to test for this. I'll
> > > > > look into it if I have time (which is a big if).
> > > > >
> > > >
> > > > I think we should rather build the embedded shared library using
> > > > the 'libfdt' rule of the top-level makefile of the dtc sub-module
> > > > and have QEMU to be linked against this share library instead of
> > > > the static one. AFAIK, this is what gcc does when it finds both
> > > > .a and .so.
> > >
> > > Actually, I don't think this is a good idea. It means the resulting
> > > qemu build would have to be installed with the libfdt image as well.
> > > As well as complicating the install path, that means that the qemu
> > > build will now actively conflict with a packaged libfdt, rather than
> > > merely suboptimally failing to use it.
> >
> > Yes you're right: the resulting QEMU shouldn't be installed, especially
> > if it has a RPATH poiting to the build directory.
> >
> > This being said, if someone wants to build AND install QEMU, shouldn't
> > she rely exclusively on installed libfdt packages ? In other words,
> > shouldn't the embedded libfdt be a QEMU developper only thing ? What
> > are the real life use cases for embedded libfdt ?
>
> I don't think we should insist on that, although it's certainly the
> way distros will usually work. If someone wants to build and install
> qemu locally, I don't think we should insist they first work out how
> to install a new enough libfdt for it to use.
>
> Likewise a limited purpose distro for whom qemu is the only user of
> libfdt might not want to package it separately.
>
Fair enough. There's still one minor fix to make in configure for
the tarball case though:
# Not a git build & no libfdt found, prompt for system install
error_exit "DTC (libfdt) version >= 1.4.2 not present." \
"Please install the DTC (libfdt) devel package"
Which version should that be ? Do you plan to release 1.5.0 or
whatever anytime soon ?
On Wed, Mar 27, 2019 at 08:09:26AM +0100, Greg Kurz wrote: > On Wed, 27 Mar 2019 11:56:10 +1100 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > On Tue, Mar 26, 2019 at 08:09:53AM +0100, Greg Kurz wrote: > > > On Tue, 26 Mar 2019 10:47:15 +1100 > > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > > > On Mon, Mar 25, 2019 at 05:33:21PM +0100, Greg Kurz wrote: > > > > > On Mon, 25 Mar 2019 11:53:47 +1100 > > > > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > > > > > > > On Sun, Mar 24, 2019 at 12:03:54AM -0400, Brad Smith wrote: > > > > > > > Now that I am checking out 4.0.0 rc's I see this diff is broken and > > > > > > > depends on a function libfdt does not expose. The breakage is > > > > > > > hidden by the fallback check in the configure script. > > > > > > > > > > > > Ah, bother. That keeps happening, unfortunately. I think it's > > > > > > because so many people use libfdt embedded, rather than as a shared > > > > > > library that we tend not to notice. > > > > > > > > > > > > > > > > It's a bit more complicated. I do have latest libfdt packages on my laptop: > > > > > > > > > > libfdt-1.4.7-2.fc28.x86_64 > > > > > libfdt-devel-1.4.7-2.fc28.x86_64 > > > > > > > > > > but I still end up using the embedded one and the build doesn't spot > > > > > the missing symbols. > > > > > > > > Sorry, I wasn't clear. I wasn't meaning in the context of qemu, but > > > > for dtc generally. A large portion of the users are things like > > > > u-boot that have to use dtc embedded, rather than as a shared > > > > library. That's why we tend not to notice missing symbols from the > > > > version script upstream. > > > > > > > > > > Ok, I get it. > > > > > > > > This happens because of several reasons: > > > > > > > > > > 1) configure unconditionally falls back to embedded if an error occurs with > > > > > the system packages. And, as reported by Brad, the current 1.4.7 packages > > > > > are broken indeed: > > > > > > > > > > $ objdump -T /usr/lib64/libfdt-1.4.7.so | grep fdt_check_full > > > > > $ > > > > > > > > > > 2) when building embedded, we only build the archive, not the shared lib. > > > > > > > > > > > I guess we should figure out how to force the testsuite to link > > > > > > against the shared library rather than static to test for this. I'll > > > > > > look into it if I have time (which is a big if). > > > > > > > > > > > > > > > > I think we should rather build the embedded shared library using > > > > > the 'libfdt' rule of the top-level makefile of the dtc sub-module > > > > > and have QEMU to be linked against this share library instead of > > > > > the static one. AFAIK, this is what gcc does when it finds both > > > > > .a and .so. > > > > > > > > Actually, I don't think this is a good idea. It means the resulting > > > > qemu build would have to be installed with the libfdt image as well. > > > > As well as complicating the install path, that means that the qemu > > > > build will now actively conflict with a packaged libfdt, rather than > > > > merely suboptimally failing to use it. > > > > > > Yes you're right: the resulting QEMU shouldn't be installed, especially > > > if it has a RPATH poiting to the build directory. > > > > > > This being said, if someone wants to build AND install QEMU, shouldn't > > > she rely exclusively on installed libfdt packages ? In other words, > > > shouldn't the embedded libfdt be a QEMU developper only thing ? What > > > are the real life use cases for embedded libfdt ? > > > > I don't think we should insist on that, although it's certainly the > > way distros will usually work. If someone wants to build and install > > qemu locally, I don't think we should insist they first work out how > > to install a new enough libfdt for it to use. > > > > Likewise a limited purpose distro for whom qemu is the only user of > > libfdt might not want to package it separately. > > > > Fair enough. There's still one minor fix to make in configure for > the tarball case though: > > # Not a git build & no libfdt found, prompt for system install > error_exit "DTC (libfdt) version >= 1.4.2 not present." \ > "Please install the DTC (libfdt) devel package" > > Which version should that be ? Do you plan to release 1.5.0 or > whatever anytime soon ? I released 1.5.0 about 3 weeks ago. -- 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 Wed, 27 Mar 2019 19:38:27 +1100 David Gibson <david@gibson.dropbear.id.au> wrote: > On Wed, Mar 27, 2019 at 08:09:26AM +0100, Greg Kurz wrote: > > On Wed, 27 Mar 2019 11:56:10 +1100 > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > On Tue, Mar 26, 2019 at 08:09:53AM +0100, Greg Kurz wrote: > > > > On Tue, 26 Mar 2019 10:47:15 +1100 > > > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > > > > > On Mon, Mar 25, 2019 at 05:33:21PM +0100, Greg Kurz wrote: > > > > > > On Mon, 25 Mar 2019 11:53:47 +1100 > > > > > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > > > > > > > > > On Sun, Mar 24, 2019 at 12:03:54AM -0400, Brad Smith wrote: > > > > > > > > Now that I am checking out 4.0.0 rc's I see this diff is broken and > > > > > > > > depends on a function libfdt does not expose. The breakage is > > > > > > > > hidden by the fallback check in the configure script. > > > > > > > > > > > > > > Ah, bother. That keeps happening, unfortunately. I think it's > > > > > > > because so many people use libfdt embedded, rather than as a shared > > > > > > > library that we tend not to notice. > > > > > > > > > > > > > > > > > > > It's a bit more complicated. I do have latest libfdt packages on my laptop: > > > > > > > > > > > > libfdt-1.4.7-2.fc28.x86_64 > > > > > > libfdt-devel-1.4.7-2.fc28.x86_64 > > > > > > > > > > > > but I still end up using the embedded one and the build doesn't spot > > > > > > the missing symbols. > > > > > > > > > > Sorry, I wasn't clear. I wasn't meaning in the context of qemu, but > > > > > for dtc generally. A large portion of the users are things like > > > > > u-boot that have to use dtc embedded, rather than as a shared > > > > > library. That's why we tend not to notice missing symbols from the > > > > > version script upstream. > > > > > > > > > > > > > Ok, I get it. > > > > > > > > > > This happens because of several reasons: > > > > > > > > > > > > 1) configure unconditionally falls back to embedded if an error occurs with > > > > > > the system packages. And, as reported by Brad, the current 1.4.7 packages > > > > > > are broken indeed: > > > > > > > > > > > > $ objdump -T /usr/lib64/libfdt-1.4.7.so | grep fdt_check_full > > > > > > $ > > > > > > > > > > > > 2) when building embedded, we only build the archive, not the shared lib. > > > > > > > > > > > > > I guess we should figure out how to force the testsuite to link > > > > > > > against the shared library rather than static to test for this. I'll > > > > > > > look into it if I have time (which is a big if). > > > > > > > > > > > > > > > > > > > I think we should rather build the embedded shared library using > > > > > > the 'libfdt' rule of the top-level makefile of the dtc sub-module > > > > > > and have QEMU to be linked against this share library instead of > > > > > > the static one. AFAIK, this is what gcc does when it finds both > > > > > > .a and .so. > > > > > > > > > > Actually, I don't think this is a good idea. It means the resulting > > > > > qemu build would have to be installed with the libfdt image as well. > > > > > As well as complicating the install path, that means that the qemu > > > > > build will now actively conflict with a packaged libfdt, rather than > > > > > merely suboptimally failing to use it. > > > > > > > > Yes you're right: the resulting QEMU shouldn't be installed, especially > > > > if it has a RPATH poiting to the build directory. > > > > > > > > This being said, if someone wants to build AND install QEMU, shouldn't > > > > she rely exclusively on installed libfdt packages ? In other words, > > > > shouldn't the embedded libfdt be a QEMU developper only thing ? What > > > > are the real life use cases for embedded libfdt ? > > > > > > I don't think we should insist on that, although it's certainly the > > > way distros will usually work. If someone wants to build and install > > > qemu locally, I don't think we should insist they first work out how > > > to install a new enough libfdt for it to use. > > > > > > Likewise a limited purpose distro for whom qemu is the only user of > > > libfdt might not want to package it separately. > > > > > > > Fair enough. There's still one minor fix to make in configure for > > the tarball case though: > > > > # Not a git build & no libfdt found, prompt for system install > > error_exit "DTC (libfdt) version >= 1.4.2 not present." \ > > "Please install the DTC (libfdt) devel package" > > > > Which version should that be ? Do you plan to release 1.5.0 or > > whatever anytime soon ? > > I released 1.5.0 about 3 weeks ago. > Hmm... I do see the version bumping patch in https://github.com/dgibson/dtc/ but no trace of 1.5.0 in https://github.com/dgibson/dtc/releases... so I'm not sure if 1.5.0 is actually released. If it is released indeed, then I guess we need a 1.5.1 with the version.lds fix.
On Wed, Mar 27, 2019 at 10:44:03AM +0100, Greg Kurz wrote:
> On Wed, 27 Mar 2019 19:38:27 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
>
> > On Wed, Mar 27, 2019 at 08:09:26AM +0100, Greg Kurz wrote:
> > > On Wed, 27 Mar 2019 11:56:10 +1100
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >
> > > > On Tue, Mar 26, 2019 at 08:09:53AM +0100, Greg Kurz wrote:
> > > > > On Tue, 26 Mar 2019 10:47:15 +1100
> > > > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > > >
> > > > > > On Mon, Mar 25, 2019 at 05:33:21PM +0100, Greg Kurz wrote:
> > > > > > > On Mon, 25 Mar 2019 11:53:47 +1100
> > > > > > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > > > > >
> > > > > > > > On Sun, Mar 24, 2019 at 12:03:54AM -0400, Brad Smith wrote:
> > > > > > > > > Now that I am checking out 4.0.0 rc's I see this diff is broken and
> > > > > > > > > depends on a function libfdt does not expose. The breakage is
> > > > > > > > > hidden by the fallback check in the configure script.
> > > > > > > >
> > > > > > > > Ah, bother. That keeps happening, unfortunately. I think it's
> > > > > > > > because so many people use libfdt embedded, rather than as a shared
> > > > > > > > library that we tend not to notice.
> > > > > > > >
> > > > > > >
> > > > > > > It's a bit more complicated. I do have latest libfdt packages on my laptop:
> > > > > > >
> > > > > > > libfdt-1.4.7-2.fc28.x86_64
> > > > > > > libfdt-devel-1.4.7-2.fc28.x86_64
> > > > > > >
> > > > > > > but I still end up using the embedded one and the build doesn't spot
> > > > > > > the missing symbols.
> > > > > >
> > > > > > Sorry, I wasn't clear. I wasn't meaning in the context of qemu, but
> > > > > > for dtc generally. A large portion of the users are things like
> > > > > > u-boot that have to use dtc embedded, rather than as a shared
> > > > > > library. That's why we tend not to notice missing symbols from the
> > > > > > version script upstream.
> > > > > >
> > > > >
> > > > > Ok, I get it.
> > > > >
> > > > > > > This happens because of several reasons:
> > > > > > >
> > > > > > > 1) configure unconditionally falls back to embedded if an error occurs with
> > > > > > > the system packages. And, as reported by Brad, the current 1.4.7 packages
> > > > > > > are broken indeed:
> > > > > > >
> > > > > > > $ objdump -T /usr/lib64/libfdt-1.4.7.so | grep fdt_check_full
> > > > > > > $
> > > > > > >
> > > > > > > 2) when building embedded, we only build the archive, not the shared lib.
> > > > > > >
> > > > > > > > I guess we should figure out how to force the testsuite to link
> > > > > > > > against the shared library rather than static to test for this. I'll
> > > > > > > > look into it if I have time (which is a big if).
> > > > > > > >
> > > > > > >
> > > > > > > I think we should rather build the embedded shared library using
> > > > > > > the 'libfdt' rule of the top-level makefile of the dtc sub-module
> > > > > > > and have QEMU to be linked against this share library instead of
> > > > > > > the static one. AFAIK, this is what gcc does when it finds both
> > > > > > > .a and .so.
> > > > > >
> > > > > > Actually, I don't think this is a good idea. It means the resulting
> > > > > > qemu build would have to be installed with the libfdt image as well.
> > > > > > As well as complicating the install path, that means that the qemu
> > > > > > build will now actively conflict with a packaged libfdt, rather than
> > > > > > merely suboptimally failing to use it.
> > > > >
> > > > > Yes you're right: the resulting QEMU shouldn't be installed, especially
> > > > > if it has a RPATH poiting to the build directory.
> > > > >
> > > > > This being said, if someone wants to build AND install QEMU, shouldn't
> > > > > she rely exclusively on installed libfdt packages ? In other words,
> > > > > shouldn't the embedded libfdt be a QEMU developper only thing ? What
> > > > > are the real life use cases for embedded libfdt ?
> > > >
> > > > I don't think we should insist on that, although it's certainly the
> > > > way distros will usually work. If someone wants to build and install
> > > > qemu locally, I don't think we should insist they first work out how
> > > > to install a new enough libfdt for it to use.
> > > >
> > > > Likewise a limited purpose distro for whom qemu is the only user of
> > > > libfdt might not want to package it separately.
> > > >
> > >
> > > Fair enough. There's still one minor fix to make in configure for
> > > the tarball case though:
> > >
> > > # Not a git build & no libfdt found, prompt for system install
> > > error_exit "DTC (libfdt) version >= 1.4.2 not present." \
> > > "Please install the DTC (libfdt) devel package"
> > >
> > > Which version should that be ? Do you plan to release 1.5.0 or
> > > whatever anytime soon ?
> >
> > I released 1.5.0 about 3 weeks ago.
> >
>
> Hmm... I do see the version bumping patch in https://github.com/dgibson/dtc/
> but no trace of 1.5.0 in https://github.com/dgibson/dtc/releases... so
> I'm not sure if 1.5.0 is actually released. If it is released indeed, then
> I guess we need a 1.5.1 with the version.lds fix.
Looks like I forgot to push the tag to github. github is not the
primary site for dtc, just a mirror. The primary git tree is at
git://git.kernel.org/pub/scm/utils/dtc/dtc.git
and releases are at:
https://mirrors.edge.kernel.org/pub/software/utils/dtc/
--
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 Wed, 27 Mar 2019 23:06:42 +1100 David Gibson <david@gibson.dropbear.id.au> wrote: > On Wed, Mar 27, 2019 at 10:44:03AM +0100, Greg Kurz wrote: > > On Wed, 27 Mar 2019 19:38:27 +1100 > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > On Wed, Mar 27, 2019 at 08:09:26AM +0100, Greg Kurz wrote: > > > > On Wed, 27 Mar 2019 11:56:10 +1100 > > > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > > > > > On Tue, Mar 26, 2019 at 08:09:53AM +0100, Greg Kurz wrote: > > > > > > On Tue, 26 Mar 2019 10:47:15 +1100 > > > > > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > > > > > > > > > On Mon, Mar 25, 2019 at 05:33:21PM +0100, Greg Kurz wrote: > > > > > > > > On Mon, 25 Mar 2019 11:53:47 +1100 > > > > > > > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > > > > > > > > > > > > > On Sun, Mar 24, 2019 at 12:03:54AM -0400, Brad Smith wrote: > > > > > > > > > > Now that I am checking out 4.0.0 rc's I see this diff is broken and > > > > > > > > > > depends on a function libfdt does not expose. The breakage is > > > > > > > > > > hidden by the fallback check in the configure script. > > > > > > > > > > > > > > > > > > Ah, bother. That keeps happening, unfortunately. I think it's > > > > > > > > > because so many people use libfdt embedded, rather than as a shared > > > > > > > > > library that we tend not to notice. > > > > > > > > > > > > > > > > > > > > > > > > > It's a bit more complicated. I do have latest libfdt packages on my laptop: > > > > > > > > > > > > > > > > libfdt-1.4.7-2.fc28.x86_64 > > > > > > > > libfdt-devel-1.4.7-2.fc28.x86_64 > > > > > > > > > > > > > > > > but I still end up using the embedded one and the build doesn't spot > > > > > > > > the missing symbols. > > > > > > > > > > > > > > Sorry, I wasn't clear. I wasn't meaning in the context of qemu, but > > > > > > > for dtc generally. A large portion of the users are things like > > > > > > > u-boot that have to use dtc embedded, rather than as a shared > > > > > > > library. That's why we tend not to notice missing symbols from the > > > > > > > version script upstream. > > > > > > > > > > > > > > > > > > > Ok, I get it. > > > > > > > > > > > > > > This happens because of several reasons: > > > > > > > > > > > > > > > > 1) configure unconditionally falls back to embedded if an error occurs with > > > > > > > > the system packages. And, as reported by Brad, the current 1.4.7 packages > > > > > > > > are broken indeed: > > > > > > > > > > > > > > > > $ objdump -T /usr/lib64/libfdt-1.4.7.so | grep fdt_check_full > > > > > > > > $ > > > > > > > > > > > > > > > > 2) when building embedded, we only build the archive, not the shared lib. > > > > > > > > > > > > > > > > > I guess we should figure out how to force the testsuite to link > > > > > > > > > against the shared library rather than static to test for this. I'll > > > > > > > > > look into it if I have time (which is a big if). > > > > > > > > > > > > > > > > > > > > > > > > > I think we should rather build the embedded shared library using > > > > > > > > the 'libfdt' rule of the top-level makefile of the dtc sub-module > > > > > > > > and have QEMU to be linked against this share library instead of > > > > > > > > the static one. AFAIK, this is what gcc does when it finds both > > > > > > > > .a and .so. > > > > > > > > > > > > > > Actually, I don't think this is a good idea. It means the resulting > > > > > > > qemu build would have to be installed with the libfdt image as well. > > > > > > > As well as complicating the install path, that means that the qemu > > > > > > > build will now actively conflict with a packaged libfdt, rather than > > > > > > > merely suboptimally failing to use it. > > > > > > > > > > > > Yes you're right: the resulting QEMU shouldn't be installed, especially > > > > > > if it has a RPATH poiting to the build directory. > > > > > > > > > > > > This being said, if someone wants to build AND install QEMU, shouldn't > > > > > > she rely exclusively on installed libfdt packages ? In other words, > > > > > > shouldn't the embedded libfdt be a QEMU developper only thing ? What > > > > > > are the real life use cases for embedded libfdt ? > > > > > > > > > > I don't think we should insist on that, although it's certainly the > > > > > way distros will usually work. If someone wants to build and install > > > > > qemu locally, I don't think we should insist they first work out how > > > > > to install a new enough libfdt for it to use. > > > > > > > > > > Likewise a limited purpose distro for whom qemu is the only user of > > > > > libfdt might not want to package it separately. > > > > > > > > > > > > > Fair enough. There's still one minor fix to make in configure for > > > > the tarball case though: > > > > > > > > # Not a git build & no libfdt found, prompt for system install > > > > error_exit "DTC (libfdt) version >= 1.4.2 not present." \ > > > > "Please install the DTC (libfdt) devel package" > > > > > > > > Which version should that be ? Do you plan to release 1.5.0 or > > > > whatever anytime soon ? > > > > > > I released 1.5.0 about 3 weeks ago. > > > > > > > Hmm... I do see the version bumping patch in https://github.com/dgibson/dtc/ > > but no trace of 1.5.0 in https://github.com/dgibson/dtc/releases... so > > I'm not sure if 1.5.0 is actually released. If it is released indeed, then > > I guess we need a 1.5.1 with the version.lds fix. > > Looks like I forgot to push the tag to github. github is not the > primary site for dtc, just a mirror. The primary git tree is at > git://git.kernel.org/pub/scm/utils/dtc/dtc.git > and releases are at: > https://mirrors.edge.kernel.org/pub/software/utils/dtc/ > Oh... thanks for the info :) So we're up for 1.5.1 with a usable libfdt.so ?
On Wed, Mar 27, 2019 at 01:36:59PM +0100, Greg Kurz wrote: > On Wed, 27 Mar 2019 23:06:42 +1100 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > On Wed, Mar 27, 2019 at 10:44:03AM +0100, Greg Kurz wrote: > > > On Wed, 27 Mar 2019 19:38:27 +1100 > > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > > > On Wed, Mar 27, 2019 at 08:09:26AM +0100, Greg Kurz wrote: > > > > > On Wed, 27 Mar 2019 11:56:10 +1100 > > > > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > > > > > > > On Tue, Mar 26, 2019 at 08:09:53AM +0100, Greg Kurz wrote: > > > > > > > On Tue, 26 Mar 2019 10:47:15 +1100 > > > > > > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > > > > > > > > > > > On Mon, Mar 25, 2019 at 05:33:21PM +0100, Greg Kurz wrote: > > > > > > > > > On Mon, 25 Mar 2019 11:53:47 +1100 > > > > > > > > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > > > > > > > > > > > > > > > On Sun, Mar 24, 2019 at 12:03:54AM -0400, Brad Smith wrote: > > > > > > > > > > > Now that I am checking out 4.0.0 rc's I see this diff is broken and > > > > > > > > > > > depends on a function libfdt does not expose. The breakage is > > > > > > > > > > > hidden by the fallback check in the configure script. > > > > > > > > > > > > > > > > > > > > Ah, bother. That keeps happening, unfortunately. I think it's > > > > > > > > > > because so many people use libfdt embedded, rather than as a shared > > > > > > > > > > library that we tend not to notice. > > > > > > > > > > > > > > > > > > > > > > > > > > > > It's a bit more complicated. I do have latest libfdt packages on my laptop: > > > > > > > > > > > > > > > > > > libfdt-1.4.7-2.fc28.x86_64 > > > > > > > > > libfdt-devel-1.4.7-2.fc28.x86_64 > > > > > > > > > > > > > > > > > > but I still end up using the embedded one and the build doesn't spot > > > > > > > > > the missing symbols. > > > > > > > > > > > > > > > > Sorry, I wasn't clear. I wasn't meaning in the context of qemu, but > > > > > > > > for dtc generally. A large portion of the users are things like > > > > > > > > u-boot that have to use dtc embedded, rather than as a shared > > > > > > > > library. That's why we tend not to notice missing symbols from the > > > > > > > > version script upstream. > > > > > > > > > > > > > > > > > > > > > > Ok, I get it. > > > > > > > > > > > > > > > > This happens because of several reasons: > > > > > > > > > > > > > > > > > > 1) configure unconditionally falls back to embedded if an error occurs with > > > > > > > > > the system packages. And, as reported by Brad, the current 1.4.7 packages > > > > > > > > > are broken indeed: > > > > > > > > > > > > > > > > > > $ objdump -T /usr/lib64/libfdt-1.4.7.so | grep fdt_check_full > > > > > > > > > $ > > > > > > > > > > > > > > > > > > 2) when building embedded, we only build the archive, not the shared lib. > > > > > > > > > > > > > > > > > > > I guess we should figure out how to force the testsuite to link > > > > > > > > > > against the shared library rather than static to test for this. I'll > > > > > > > > > > look into it if I have time (which is a big if). > > > > > > > > > > > > > > > > > > > > > > > > > > > > I think we should rather build the embedded shared library using > > > > > > > > > the 'libfdt' rule of the top-level makefile of the dtc sub-module > > > > > > > > > and have QEMU to be linked against this share library instead of > > > > > > > > > the static one. AFAIK, this is what gcc does when it finds both > > > > > > > > > .a and .so. > > > > > > > > > > > > > > > > Actually, I don't think this is a good idea. It means the resulting > > > > > > > > qemu build would have to be installed with the libfdt image as well. > > > > > > > > As well as complicating the install path, that means that the qemu > > > > > > > > build will now actively conflict with a packaged libfdt, rather than > > > > > > > > merely suboptimally failing to use it. > > > > > > > > > > > > > > Yes you're right: the resulting QEMU shouldn't be installed, especially > > > > > > > if it has a RPATH poiting to the build directory. > > > > > > > > > > > > > > This being said, if someone wants to build AND install QEMU, shouldn't > > > > > > > she rely exclusively on installed libfdt packages ? In other words, > > > > > > > shouldn't the embedded libfdt be a QEMU developper only thing ? What > > > > > > > are the real life use cases for embedded libfdt ? > > > > > > > > > > > > I don't think we should insist on that, although it's certainly the > > > > > > way distros will usually work. If someone wants to build and install > > > > > > qemu locally, I don't think we should insist they first work out how > > > > > > to install a new enough libfdt for it to use. > > > > > > > > > > > > Likewise a limited purpose distro for whom qemu is the only user of > > > > > > libfdt might not want to package it separately. > > > > > > > > > > > > > > > > Fair enough. There's still one minor fix to make in configure for > > > > > the tarball case though: > > > > > > > > > > # Not a git build & no libfdt found, prompt for system install > > > > > error_exit "DTC (libfdt) version >= 1.4.2 not present." \ > > > > > "Please install the DTC (libfdt) devel package" > > > > > > > > > > Which version should that be ? Do you plan to release 1.5.0 or > > > > > whatever anytime soon ? > > > > > > > > I released 1.5.0 about 3 weeks ago. > > > > > > > > > > Hmm... I do see the version bumping patch in https://github.com/dgibson/dtc/ > > > but no trace of 1.5.0 in https://github.com/dgibson/dtc/releases... so > > > I'm not sure if 1.5.0 is actually released. If it is released indeed, then > > > I guess we need a 1.5.1 with the version.lds fix. > > > > Looks like I forgot to push the tag to github. github is not the > > primary site for dtc, just a mirror. The primary git tree is at > > git://git.kernel.org/pub/scm/utils/dtc/dtc.git > > and releases are at: > > https://mirrors.edge.kernel.org/pub/software/utils/dtc/ > > > > Oh... thanks for the info :) > > So we're up for 1.5.1 with a usable libfdt.so ? Yeah, I guess so. -- 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
© 2016 - 2025 Red Hat, Inc.