From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-x12a.google.com (mail-lf1-x12a.google.com [IPv6:2a00:1450:4864:20::12a]) by sourceware.org (Postfix) with ESMTPS id 10B003858434 for ; Thu, 13 Jul 2023 18:22:01 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 10B003858434 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=rivosinc.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=rivosinc.com Received: by mail-lf1-x12a.google.com with SMTP id 2adb3069b0e04-4fb73ba3b5dso1875680e87.1 for ; Thu, 13 Jul 2023 11:22:00 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=rivosinc-com.20221208.gappssmtp.com; s=20221208; t=1689272519; x=1691864519; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=r+LHghTHxw26N96/Gd760UPZHlNBZoetPSNa7jcwW1M=; b=5izGjSqugUsE3GKXz9lvkHt725oyryUpef6Az56iGxLNlAYuCXFp+5CjGYfixLxQ/r A6VbHoAkqrrbotcQRf8031GFxywfWrriN9I606h+V+WApozIbkbQu6mg+bZRcESw+3zG kCW1ASvxbgp3Tsw+QQ0dpXxezCuxz7698eNQ/u25gYBZ6rWsixpDkSSf18E7/slpy4ib 0J4ignFc/GW33pOPxjH0LFsFxWHBHkDZzZAnzmsC4qC42xeowd40IYP4UAwcWPsYfn5w TRdIxhHc33tyg1YHh9p1ftqFTljV18mtJzpC3OGAajFGJY3nDk0B1lvYhho5bKHAWIuf +RSQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689272519; x=1691864519; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=r+LHghTHxw26N96/Gd760UPZHlNBZoetPSNa7jcwW1M=; b=AZl6RqL/LcoyjVp+A4gx45GB0FzJhGVN0XEfeZLrh3hQ1ajZgPJQxjlqVSXphOItkp J2tFTBXjcNpulzjghtNJWQakwHCNTjVTF2IOEe6dRF7akbSCrwBfQ48zfuGiVHb06CRl YBjqxNlOU/S6/TRIqiC1GKWvAr8nd5mOTqeW/z8ompc1GjFc1ZNUpuVk5tl7rfJb2dXD OUGLkaZs4bBc8X+Du0WD9WO9QCVooHyP+inOWqG28Jx8Fwb/q5UOAPQ2XGg+dXpCvI2u MgUx4Do+uMorMvgR9R+ki5fYvT4SMJqQ58NmIA/AanncbKdjUp8MZsmvyq+bs0ROXJXf yJxw== X-Gm-Message-State: ABy/qLaeh422PrKcGz2yao/YlothOLrz9zohY+DN028R9KZLPLOicB4x BujNarBe3Glbg0EFcrKOjyNG3O/FyxffInytWvXqa8Wg7PEN6F0ehX4= X-Google-Smtp-Source: APBJJlGXQa9ypSNlnw9JKREch3UEYHESUl2uAWDeSlouOwInEsNPZpmES87vpJA1W9FR7rN53KvqPcE3E9w2MNVQT8o= X-Received: by 2002:a19:2d42:0:b0:4f8:4512:c846 with SMTP id t2-20020a192d42000000b004f84512c846mr1788533lft.49.1689272518742; Thu, 13 Jul 2023 11:21:58 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Evan Green Date: Thu, 13 Jul 2023 11:21:22 -0700 Message-ID: Subject: Re: [PATCH v5 3/4] riscv: Add ifunc-compatible hwprobe function To: Palmer Dabbelt Cc: fweimer@redhat.com, libc-alpha@sourceware.org, Vineet Gupta , slewis@rivosinc.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-8.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Thu, Jul 13, 2023 at 9:47=E2=80=AFAM Palmer Dabbelt wrote: > > On Thu, 13 Jul 2023 09:33:20 PDT (-0700), Evan Green wrote: > > On Thu, Jul 13, 2023 at 12:07=E2=80=AFAM Florian Weimer wrote: > >> > >> * Evan Green: > >> > >> > diff --git a/sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h b/sysdeps/u= nix/sysv/linux/riscv/sys/hwprobe.h > >> > index b27af5cb07..d739371806 100644 > >> > --- a/sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h > >> > +++ b/sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h > >> > @@ -67,6 +67,27 @@ extern int __riscv_hwprobe (struct riscv_hwprobe = *pairs, size_t pair_count, > >> > __fortified_attr_access (__read_write__, 1, 2) > >> > __fortified_attr_access (__read_only__, 4, 3); > >> > > >> > +extern int __riscv_hwprobe_weak (struct riscv_hwprobe *pairs, size_= t pair_count, > >> > + size_t cpu_count, unsigned long int *= cpus, > >> > + unsigned int flags) > >> > + __THROW __nonnull ((1)) __wur > >> > + __fortified_attr_access (__read_write__, 1, 2) > >> > + __fortified_attr_access (__read_only__, 4, 3); > >> > + > >> > +# ifdef weak_extern > >> > +weak_extern (__riscv_hwprobe_weak) > >> > +# else > >> > +# pragma weak __riscv_hwprobe_weak > >> > +# endif > >> > + > >> > +extern int __riscv_hwprobe_early (struct riscv_hwprobe *pairs, size= _t pair_count, > >> > + size_t cpu_count, unsigned long int = *cpus, > >> > + unsigned int flags) > >> > + __THROW __nonnull ((1)) __wur > >> > + __fortified_attr_access (__read_write__, 1, 2) > >> > + __fortified_attr_access (__read_only__, 4, 3) > >> > + __attribute__ ((__visibility__ ("hidden"))); > >> > + > >> > __END_DECLS > >> > > >> > #endif /* sys/hwprobe.h */ > >> > >> I would call the dynamic symbol ___riscv_hwprobe, not > >> __riscv_hwprobe_weak. The weak property is an implementation detail i= n > >> the C file that goes into libc_nonshared. And ___riscv_hwprobe does n= ot > >> need to be declared in an installed header, I think it's fine to offer > >> __riscv_hwprobe only. > > > > Ok. Just to reiterate what I'm planning to export to users, I've got > > __riscv_hwprobe() as a dynamic function from libc that non-ifunc > > callers can use without the overhead of checking the weak symbol, as > > well as __riscv_hwprobe_early() that ifunc selectors can use > > pre-relocation. Based on your comment, I'll rename the > > __riscv_hwprobe_weak alias to ___riscv_hwprobe. > > > >> > >> Use of INTERNAL_SYSCALL is correct because without relocations, errno > >> will not work, but it needs to match across all implementations. This > >> means using INTERNAL_VSYSCALL instead of INLINE_VSYSCALL. > > > > Oh yeah, otherwise the return values of __riscv_hwprobe_early() would > > suddenly change from -EINVAL to -1 once the symbol is available. So > > then we'll document errno is always left unchanged by this function. > > > >> > >> There needs to be manual entry that documents the exact interface > >> conventions. If the function returns 0 on success (no positive return > >> values), it may make sense to negate the result value that comes back > >> from the kernel because that matches what some pthread interfaces use. > >> We probabl use the -EINVAL convention on some external API (it's a big > >> library), but I think it's on the unusual side. > > > > Correct, it's 0 on success or negative error code on failure. I guess > > you're binning it in with pthreads because those functions also return > > error codes directly? I can do that. > > > > After discussing with Palmer, based on the timing of things we're > > thinking it's probably too late to try and rush this into the upcoming > > release. There was also a suggestion internally to try and get to the > > vDSO pointer early, though I don't understand how/if this is possible. > > I need to dig more on that one. I may still spin this today if I can, > > but we'll see. > > Ya, thanks. Given how late we are in the cycle I think it's best to > avoid rushing in any new ABI here, it's just too risky. > > The vDSO idea would be to pre-populate HWCAP2 with a pointer to the full > output of a riscv_hwprobe() run on all harts. We'd store that data in > the vDSO, but IIUC it'd fix the symbol lookup problem as it'd come in > via the auxvec so we wouldn't need to poke around for symbols. We > probably need to think through some extensibility issues as well, but I > think that doesn't change the core of the IFUNC discussion. Palmer and I chatted a bit more about this idea. The evolution of it was to define something like AT_RISCV_HWPROBE_FUNC whose value is a pointer to the vDSO function. The __riscv_hwprobe_early() in libc_nonshared can then just get the auxval and call it, with similar semantics as we have here of not setting errno, etc. Florian, one question about your other possible solution suggestion, reproduced below: > You could pass the function pointer to the IFUNC resolver > (which may require a marker symbol and GCC changes). I'm unsure what a marker symbol is, so I'm having trouble following this suggestion. I'd like to understand what that suggestion was, especially if you think it's better than what we're planning above. -Evan