Re: [Qemu-devel] [ARM/FDPIC v2 3/4] linux-user: ARM-FDPIC: Add support of FDPIC for ARM.

[Christophe Lyon]

On 23/04/2018 14:49, Peter Maydell wrote:
> On 23 April 2018 at 08:51, Christophe Lyon <address@hidden> wrote:
> > Add FDPIC info into image_info structure since interpreter info is on
> > stack and needs to be saved to be accessed later on.
> >
> > Co-Authored-By:  Mickaël Guêné <address@hidden>
> > Signed-off-by: Christophe Lyon <address@hidden>
> >
> > diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> > index 76d7718..1ee1e38 100644
> > --- a/linux-user/elfload.c
> > +++ b/linux-user/elfload.c
> > @@ -78,6 +78,11 @@ enum {
> >    */
> >   #define personality(pers)       (pers & PER_MASK)
> >
> > +int info_is_fdpic(struct image_info *info)
> > +{
> > +    return (info->personality == PER_LINUX_FDPIC);
> > +}
> > +
> >   /* this flag is uneffective under linux too, should be deleted */
> >   #ifndef MAP_DENYWRITE
> >   #define MAP_DENYWRITE 0
> > @@ -287,6 +292,24 @@ static inline void init_thread(struct target_pt_regs *regs,
> >       /* For uClinux PIC binaries.  */
> >       /* XXX: Linux does this only on ARM with no MMU (do we care ?) */
> >       regs->uregs[10] = infop->start_data;
> > +
> > +    /* Support ARM FDPIC.  */
> > +    if (info_is_fdpic(infop)) {
> > +            /* As described in the ABI document, r7 points to the loadmap info
> > +             * prepared by the kernel. If an interpreter is needed, r8 points
> > +             * to the interpreter loadmap and r9 points to the interpreter
> > +             * PT_DYNAMIC info. If no interpreter is needed, r8 is zer0, and
>
> "zero"
>
> > +             * r9 points to the main program PT_DYNAMIC info.  */
>
> Something odd seems to have happened to the indentation here.
>
> Nit: */ should be on a line of its own.
>
> > +            regs->uregs[7] = infop->loadmap_addr;
> > +            if (infop->interpreter_loadmap_addr) {
> > +                /* Executable is dynamically loaded.  */
> > +                regs->uregs[8] = infop->interpreter_loadmap_addr;
> > +                regs->uregs[9] = infop->interpreter_pt_dynamic_addr;
> > +            } else {
> > +                regs->uregs[8] = 0;
> > +                regs->uregs[9] = infop->pt_dynamic_addr;
> > +            }
> > +        }
> >   }
> >
> >   #define ELF_NREG    18
> > @@ -1745,6 +1768,11 @@ static abi_ulong create_elf_tables(abi_ulong p, int 
> > argc, int envc,
> >           if (interp_info) {
> >               interp_info->other_info = info;
> >               sp = loader_build_fdpic_loadmap(interp_info, sp);
> > +            info->interpreter_loadmap_addr = interp_info->loadmap_addr;
> > +            info->interpreter_pt_dynamic_addr = interp_info->pt_dynamic_addr;
> > +        } else {
> > +            info->interpreter_loadmap_addr = 0;
> > +            info->interpreter_pt_dynamic_addr = 0;
> >           }
> >       }
> >
> > diff --git a/linux-user/main.c b/linux-user/main.c
> > index 2acac36..3579f0e 100644
> > --- a/linux-user/main.c
> > +++ b/linux-user/main.c
> > @@ -4893,6 +4893,9 @@ int main(int argc, char **argv, char **envp)
> >               env->cp15.sctlr_el[1] |= SCTLR_B;
> >           }
> >   #endif
> > +
> > +        /* Are we running an FDPIC binary?  */
> > +        ((TaskState *)thread_cpu->opaque)->is_fdpic = info_is_fdpic(info);
> >       }
> >   #elif defined(TARGET_SPARC)
> >       {
> > diff --git a/linux-user/qemu.h b/linux-user/qemu.h
> > index da3b517..a2ed148 100644
> > --- a/linux-user/qemu.h
> > +++ b/linux-user/qemu.h
> > @@ -57,6 +57,8 @@ struct image_info {
> >           uint16_t        nsegs;
> >           void           *loadsegs;
> >           abi_ulong       pt_dynamic_addr;
> > +        abi_ulong       interpreter_loadmap_addr;
> > +        abi_ulong       interpreter_pt_dynamic_addr;
> >           struct image_info *other_info;
> >   };
> >
> > @@ -145,6 +147,9 @@ typedef struct TaskState {
> >        */
> >       int signal_pending;
> >
> > +    /* We need to know if we have an FDPIC binary to adapt signal
> > +     * syscalls.  */
> > +    int is_fdpic;
>
> Looking at the TaskState struct it already has a pointer
> to the image_info struct. So we could just have something like
>      bool task_is_fdpic(TaskState *ts)
>      {
>          return ts->info->personality == PER_LINUX_FDPIC;
>      }
>
> and save the need to set up and test a separate flag, I think.

This patch defines and uses info_is_fdpic, the next one in the
series uses information for TaskState, so I suppose it's OK
not to add this new flag as you suggest, and in the next patch
use info_is_fdpic(ts->info) instead of adding task_is_fdpic() ?

Thanks,

Christophe

> Otherwise looks good.
>
> thanks
> -- PMM
> .