Re: [Qemu-devel] [PATCH v5 4/8] linux-user: Split out preadv, pwritev, readv, writev, pread64, pwrite64

[Laurent Vivier]

On 11/01/2019 22:31, Richard Henderson wrote:
> On 1/11/19 2:17 AM, Laurent Vivier wrote:
> > On 19/12/2018 05:21, Richard Henderson wrote:
> > > Signed-off-by: Richard Henderson <address@hidden>
> > > ---
> > >   linux-user/syscall-defs.h     |  14 ++++
> > >   linux-user/syscall-file.inc.c | 124 ++++++++++++++++++++++++++++++++++
> > >   linux-user/syscall.c          |  93 -------------------------
> > >   linux-user/strace.list        |  18 -----
> > >   4 files changed, 138 insertions(+), 111 deletions(-)
> > ...
> > > diff --git a/linux-user/syscall-file.inc.c b/linux-user/syscall-file.inc.c
> > > index 11e75044c1..410a763eee 100644
> > > --- a/linux-user/syscall-file.inc.c
> > > +++ b/linux-user/syscall-file.inc.c
> > > @@ -315,6 +315,104 @@ SYSCALL_IMPL(openat)
> > ...
> > > +
> > > +/*
> > > + * Both preadv and pwritev merge args 4/5 into a 64-bit offset.
> > > + * Moreover, the parts are *always* in little-endian order.
> > > + */
> > > +#if TARGET_ABI_BITS == 32
> > > +SYSCALL_ARGS(preadv_pwritev)
> > > +{
> > > +    /* We have already assigned out[0-2].  */
> > > +    abi_ulong lo = in[3], hi = in[4];
> > > +    out[3] = ((hi << (TARGET_ABI_BITS - 1)) << 1) | lo;
> > > +    return def;
> > > +}
> > > +#else
> > > +#define args_preadv_pwritev NULL
> > > +#endif
> > > +
> > > +/* Perform the inverse operation for the host.  */
> > > +static inline void host_offset64_low_high(unsigned long *l, unsigned long *h,
> > > +                                          uint64_t off)
> > > +{
> > > +    *l = off;
> > > +    *h = (off >> (HOST_LONG_BITS - 1)) >> 1;
> > > +}
> >
> >
> > I have an error with preadv() on a 32bit target (powerpc, LTP test preadv02).
> >
> > It works if I use:
> >
> > static inline void host_offset64_low_high(unsigned long *hlow,
> >                                            unsigned long *hhigh,
> >                                            abi_ulong tlow,
> >                                            abi_ulong thigh)
> > {
> >      uint64_t off = tlow |
> >                     ((unsigned long long)thigh << TARGET_LONG_BITS / 2) <<
> >                     TARGET_LONG_BITS / 2;
> >      
> >      *hlow = off;
> >      *hhigh = (off >> HOST_LONG_BITS / 2) >> HOST_LONG_BITS / 2;
> > }
>
> This doesn't make any sense.  Where are "tlow" and "thigh" coming from?
>
> I think the bug will be
>
>   SYSCALL_ARGS(preadv_pwritev)
>   {
>       /* We have already assigned out[0-2].  */
>       abi_ulong lo = in[3], hi = in[4];
> -    out[3] = ((hi << (TARGET_ABI_BITS - 1)) << 1) | lo;
> +    out[3] = (((uint64_t)hi << (TARGET_ABI_BITS - 1)) << 1) | lo;
>       return def;
>   }

Yes, this change fixes the problem.

Thanks,
Laurent