From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS3215 2.6.0.0/16 X-Spam-Status: No, score=-3.9 required=3.0 tests=AWL,BAYES_00,BODY_8BITS, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,MAILING_LIST_MULTI, NICE_REPLY_A,RCVD_IN_DNSWL_MED,SPF_HELO_PASS,SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (server2.sourceware.org [IPv6:2620:52:3:1:0:246e:9693:128c]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by dcvr.yhbt.net (Postfix) with ESMTPS id 648241F4C1 for ; Tue, 29 Nov 2022 20:44:24 +0000 (UTC) Authentication-Results: dcvr.yhbt.net; dkim=pass (1024-bit key; secure) header.d=sourceware.org header.i=@sourceware.org header.b="cQErbDo0"; dkim-atps=neutral Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id C060C3858284 for ; Tue, 29 Nov 2022 20:44:22 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org C060C3858284 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1669754662; bh=smSTYCLHc1fxabF63Nc8ed0JPX2ucaX6Vzf3hP90dEk=; h=Date:Subject:To:References:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=cQErbDo0h5ls6cyu7pV7EbmmaPb1aSZSdePOXYXLcDk4cgEHGn2DnA5bqBL9tIQ1K kGPtuos6sYOBX+EwImiQ3jENrAuZ2xZT4/nv6PQ4/qPVOSqZuHMfHiTFFtIzpWWHBZ NVuf+/LJ38SZU62s1RNsLtiLZ9sjFijoWsy5cacA= Received: from mail-oa1-x2f.google.com (mail-oa1-x2f.google.com [IPv6:2001:4860:4864:20::2f]) by sourceware.org (Postfix) with ESMTPS id D02233858D1E for ; Tue, 29 Nov 2022 20:44:00 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org D02233858D1E Received: by mail-oa1-x2f.google.com with SMTP id 586e51a60fabf-141ca09c2fbso18553608fac.6 for ; Tue, 29 Nov 2022 12:44:00 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:organization:from:references :to:content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=smSTYCLHc1fxabF63Nc8ed0JPX2ucaX6Vzf3hP90dEk=; b=nIt/5518blPrnT7JtBrgaOmm0ayko8ioS/iMJQfyTbGKhxnsGaqdim7wp69gAgIy4D P1/L4Tbo4dQRkHWU2T7nlD9saLKY69/43Tki9B9EX7U021eTzBd96KZgbIpY0g9Rk9Kn ENYJxEsscf3jt4TszxDQXMfC8AANGpTt3ssUqbArLNdEVLKDtHg+0giFTTOei/50l0Lk LF64L4gguetD2t5pXLOqfzTXt+Yh/YUdzUAzffAGkEuHtX3vSgzIdihWy9oDqbTG3ybA cXwbs4dYg9vjEre4ji3leMyznbkUA/ihq3JNJEY7n3vWIUc7U7Mm5XeHCSbuwrG7yybx cbhw== X-Gm-Message-State: ANoB5pnNGeuUXPKsw4UGS+XItKOS1o2GdWNVV8TqzkXZ7xSuSNvq3Bcz 3r+9rAUl7LHd+m1F4DpqOCKI54UBhFuoCTkB X-Google-Smtp-Source: AA0mqf5NGcXHgDzs6Dh1LMun781NUSdBH+v9vrVlvGmhDAkymQrnGysuEzVFAp6wmLMpTsAB7UiIQA== X-Received: by 2002:a05:6870:171e:b0:133:3f1:6d45 with SMTP id h30-20020a056870171e00b0013303f16d45mr25868214oae.44.1669754639983; Tue, 29 Nov 2022 12:43:59 -0800 (PST) Received: from ?IPV6:2804:1b3:a7c1:f215:2982:53e8:7f84:95db? ([2804:1b3:a7c1:f215:2982:53e8:7f84:95db]) by smtp.gmail.com with ESMTPSA id g8-20020a4a6b08000000b0049f44db7b41sm5836280ooc.5.2022.11.29.12.43.58 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 29 Nov 2022 12:43:59 -0800 (PST) Message-ID: <21f073bc-8bb7-7346-fe6d-e1c0e872e8cb@linaro.org> Date: Tue, 29 Nov 2022 17:43:57 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.5.0 Subject: Re: [PATCH v2 05/11] powerpc64: Add the clone3 wrapper Content-Language: en-US To: Paul E Murphy , libc-alpha@sourceware.org References: <20221104190112.2566409-1-adhemerval.zanella@linaro.org> <20221104190112.2566409-6-adhemerval.zanella@linaro.org> Organization: Linaro In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Adhemerval Zanella Netto via Libc-alpha Reply-To: Adhemerval Zanella Netto Errors-To: libc-alpha-bounces+e=80x24.org@sourceware.org Sender: "Libc-alpha" On 28/11/22 13:38, Paul E Murphy wrote: > > > On 11/4/22 2:01 PM, Adhemerval Zanella via Libc-alpha wrote: >> It follows the internal signature: >> >>    extern int clone3 (struct clone_args *__cl_args, size_t __size, >>   int (*__func) (void *__arg), void *__arg); >> >> The powerpc64 ABI requires an initial stackframe so the child can >> store/restore the TOC.  Iit is create prior calling clone3 by > > trivial nit, s/Iit/It/. Ack. > >> adjusting the stack size (since kernel will compute the stack as >> stack plus size). >> >> Checked on powerpc64-linux-gnu (power8, kernel 6.0) and >> powerpc64le-linux-gnu (power9, kernel 4.18). >> --- >>   .../sysv/linux/powerpc/powerpc64/clone3.S     | 152 ++++++++++++++++++ >>   sysdeps/unix/sysv/linux/powerpc/sysdep.h      |   1 + >>   2 files changed, 153 insertions(+) >>   create mode 100644 sysdeps/unix/sysv/linux/powerpc/powerpc64/clone3.S >> >> diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc64/clone3.S b/sysdeps/unix/sysv/linux/powerpc/powerpc64/clone3.S >> new file mode 100644 >> index 0000000000..0fe2fe91db >> --- /dev/null >> +++ b/sysdeps/unix/sysv/linux/powerpc/powerpc64/clone3.S >> @@ -0,0 +1,152 @@ >> +/* The clone3 syscall wrapper.  Linux/powerpc64 version. >> +   Copyright (C) 2022 Free Software Foundation, Inc. >> + >> +   This file is part of the GNU C Library. >> + >> +   The GNU C Library is free software; you can redistribute it and/or >> +   modify it under the terms of the GNU Lesser General Public >> +   License as published by the Free Software Foundation; either >> +   version 2.1 of the License, or (at your option) any later version. >> + >> +   The GNU C Library is distributed in the hope that it will be useful, >> +   but WITHOUT ANY WARRANTY; without even the implied warranty of >> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU >> +   Lesser General Public License for more details. >> + >> +   You should have received a copy of the GNU Lesser General Public >> +   License along with the GNU C Library; if not, see >> +   .  */ >> + >> +#include >> +#define _ERRNO_H        1 >> +#include >> + >> +/* The userland implementation is: >> +   int clone3 (struct clone_args *cl_args, size_t size, >> +               int (*func)(void *arg), void *arg); >> + >> +   the kernel entry is: >> +   int clone3 (struct clone_args *cl_args, size_t size); >> + >> +   The parameters are passed in registers from userland: >> +   r3: cl_args >> +   r4: size >> +   r5: func >> +   r6: arg  */ >> + >> +        .text >> +ENTRY(__clone3) >> +    CALL_MCOUNT 4 >> + >> +    /* Sanity checks args.  */ >> +    cmpdi    cr0, r3, 0 >> +    cmpdi    cr1, r5, 0 >> +    cror    cr0*4+eq, cr1*4+eq, cr0*4+eq >> +    beq    cr0,L(badargs) >> + >> +    /* Save some regs in the "red zone".  */ > Is there reason to avoid explicitly stacking a frame in here? Also, does the red zone exist on ELFv1? > I don't think so, although it allows to tail call directly to __syscall_error. I used powerpc64 clone.S as example, which does something similar. >> +#ifdef USE_PPC_SCV >> +    std    r28, -24(r1) >> +    cfi_offset (r28, -24) >> +#endif >> +    std    r29, -16(r1) >> +    std    r30, -8(r1) >> +    cfi_offset (r29, -16) >> +    cfi_offset (r30, -8) >> + >> +    /* Save fn and args across syscall.  */ > trivial nit, s/fn/func/ s/args/arg/. Ack. > >> +    mr    r30, r5        /* Function in r30.  */ >> +    mr    r29, r6        /* Argument in r29.  */ >> + >> +    /* End FDE now, because in the child the unwind info will be >> +       wrong.  */ >> +    cfi_endproc >> + >> +    /* Setup a minimum stack fram for child.  It needs to first calculate > s/fram/frame/. Ack. > >> +       the effective stack address, write the start NULL frame-pointer, >> +       and update the frame size in the input cl_args.  */ >> +    ld    r7, 40(r3)    /* Load stack value.  */ >> +    ld    r8, 48(r3)    /* Load stack_size value.  */ >> +    li    r10, 0 >> +    add    r7, r7, r8    /* Calculate effective stack address.  */ >> +    std    r10, -FRAME_MIN_SIZE_PARM(r7) > This is clearing the backchain pointer? Maybe a small comment here. Yes, since this stack will be the initial one. I changed the comment to: /* Setup a minimum stack frame for child. It needs to first calculate the effective stack address, write the start empty backchain pointer, and update the frame size in the input cl_args. */ To make it more clear. > >> +    addi    r8, r8, -FRAME_MIN_SIZE_PARM >> +    std    r8, 48(r3) >> + >> +    /* Do the system call, the kernel expects: >> +       r0: system call numer >> +       r3: cl_args >> +       r4: size  */ >> +    li    r0, SYS_ify(clone3) >> +#ifdef USE_PPC_SCV >> +    CHECK_SCV_SUPPORT r28 0f >> +    /* This is equivalent to DO_CALL_SCV, but we cannot use the macro here >> +       because it uses CFI directives and we just called cfi_endproc.  */ >> +    mflr     r9 >> +    std     r9, FRAME_LR_SAVE(r1) >> +    .machine "push" >> +    .machine "power9" >> +    scv     0 >> +    .machine "pop" >> +    ld     r9, FRAME_LR_SAVE(r1) >> +    mtlr     r9 >> + >> +    /* When using scv, error is indicated by negative r3.  */ > With scv an, an error is a value -4095 <= x < 0. This test should be more specific. Ack, I changed the comment (btw clone.S has the same mistake). > >> +    cmpdi    cr1, r3, 0 >> +    b    1f >> +#endif >> +0:      DO_CALL_SC >> + >> +    /* With sc, error is indicated by cr0.SO.  */ >> +    cmpdi    cr1, r3, 0 >> +    crandc    cr1*4+eq, cr1*4+eq, cr0*4+so > + >> +1:    bne-    cr1,L(parent) >> + >> +    /* Child, load the function and arguments.  */ >> +    std    r2, FRAME_TOC_SAVE(r1) >> +    PPC64_LOAD_FUNCPTR r30 >> +    mr    r3, r29 >> +    bctrl >> +    ld    r2, FRAME_TOC_SAVE(r1) >> + >> +    li    r0, SYS_ify(exit) >> +    DO_CALL_SC > Minor nit, this should also use scv if supported. Ack (btw clone.S also use issues DO_CALL_SC). >> +    /* We won't ever get here but provide a nop so that the linker >> +       will insert a toc adjusting stub if necessary.  */ >> +    nop >> + >> +L(badargs): >> +    cfi_startproc >> +    li    r3, EINVAL >> +    TAIL_CALL_SYSCALL_ERROR >> + >> +L(parent): >> +    /* Check if svc is available.  */ > s/svc/scv/. Ack. > >> +    cmpdi cr1, r28, 0 >> + >> +    /* Parent.  Restore registers & return.  */ >> +#ifdef USE_PPC_SCV >> +    cfi_offset (r28, -24) >> +    ld    r28, -24(r1) >> +    cfi_restore (r28) >> +#endif >> +    cfi_offset (r29,-16) >> +    cfi_offset (r30,-8) >> +    ld    r29, -16(r1) >> +    ld    r30, -8(r1) >> +    cfi_restore (r29) >> +    cfi_restore (r30) >> + >> +#ifdef USE_PPC_SCV >> +    beq    cr1, 0f >> +    RET_SCV >> +    b    1f >> +#endif >> +0:    RET_SC >> +1:    TAIL_CALL_SYSCALL_ERROR >> + >> +PSEUDO_END (__clone3) >> + >> +libc_hidden_def (__clone3) >> +weak_alias (__clone3, clone3) >> diff --git a/sysdeps/unix/sysv/linux/powerpc/sysdep.h b/sysdeps/unix/sysv/linux/powerpc/sysdep.h >> index 9e44818978..af63b3974b 100644 >> --- a/sysdeps/unix/sysv/linux/powerpc/sysdep.h >> +++ b/sysdeps/unix/sysv/linux/powerpc/sysdep.h >> @@ -214,6 +214,7 @@ >>   #if defined(__PPC64__) || defined(__powerpc64__) >>   #define HAVE_CLOCK_GETRES64_VSYSCALL    "__kernel_clock_getres" >>   #define HAVE_CLOCK_GETTIME64_VSYSCALL    "__kernel_clock_gettime" >> +#define HAVE_CLONE3_WRAPPER        1 >>   #else >>   #define HAVE_CLOCK_GETRES_VSYSCALL    "__kernel_clock_getres" >>   #define HAVE_CLOCK_GETTIME_VSYSCALL    "__kernel_clock_gettime"