Re: [Qemu-devel] [PULL 02/29] ppc/spapr: Receive and store device tree blob from SLOF

[Greg Kurz]

On Tue, 26 Mar 2019 10:47:15 +1100
David Gibson <address@hidden> 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 <address@hidden> 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 :
> > 
> > address@hidden 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 <address@hidden>
> > > > > 
> > > > > 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 <address@hidden>
> > > > > Reviewed-by: Greg Kurz <address@hidden>
> > > > > Signed-off-by: David Gibson <address@hidden>
> > > > > Signed-off-by: Greg Kurz <address@hidden>
> > > > > Signed-off-by: David Gibson <address@hidden>
> > > > > ---
> > > > >   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;    
> > > >     
> > >   
> >   
> 
> 
>

pgpD0IYZmKPmK.pgp
Description: OpenPGP digital signature