On Tue, 26 Mar 2019 10:47:15 +1100 David Gibson <da...@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 <da...@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 <a...@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 <a...@ozlabs.ru> > > > > > Reviewed-by: Greg Kurz <gr...@kaod.org> > > > > > Signed-off-by: David Gibson <da...@gibson.dropbear.id.au> > > > > > Signed-off-by: Greg Kurz <gr...@kaod.org> > > > > > Signed-off-by: David Gibson <da...@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; > > > > > > > > > > > >
pgpPrvK1ud8xN.pgp
Description: OpenPGP digital signature