From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf1-x42f.google.com (mail-pf1-x42f.google.com [IPv6:2607:f8b0:4864:20::42f]) by sourceware.org (Postfix) with ESMTPS id DE28E3858D1E for ; Thu, 13 Jul 2023 16:47:57 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org DE28E3858D1E 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-pf1-x42f.google.com with SMTP id d2e1a72fcca58-6687096c6ddso673066b3a.0 for ; Thu, 13 Jul 2023 09:47:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=rivosinc-com.20221208.gappssmtp.com; s=20221208; t=1689266876; x=1691858876; h=content-transfer-encoding:mime-version:message-id:to:from:cc :in-reply-to:subject:date:from:to:cc:subject:date:message-id :reply-to; bh=ONZxXmZnb4ul7Vnhh7OstASmmjhiawgYSoktXBN06V8=; b=ctj0ghEtWtyj/SAt9BctUzTamcPVaQQXGvGr/ZfGcr3NqvopXNc11wZq1OE2a8FhzZ 0HswNKHjRQsimvik1YUUrdvFBSCqFnHBii2o0p5IFxLmmfQ6/Ej3qUnm4/v4+IpSOtzA gs8p/slUiCemfGGZ0iTBzhxC6rdjuzQl2hWB/dBjLpKmKaQFLpndbDtWM+pPs3uCeMk5 r0KuzcEpshyuP7J8xFsLnMP4Rn5W+Cdq9lqXsT6lMUMoLGVciMChbi9ubYaLvSFEgaC/ sU+Xaj2MzQjf4nXmzig32UYb+o+Gp3cpuu3la/LX4CEuWX3Qh1kn6eLl8/kcJRSaSzVr jMPg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689266876; x=1691858876; h=content-transfer-encoding:mime-version:message-id:to:from:cc :in-reply-to:subject:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=ONZxXmZnb4ul7Vnhh7OstASmmjhiawgYSoktXBN06V8=; b=WScRgYSJkRbuOvHWJCwczG4Tz7hTcSpeUKfGdpGVCUt+ksnnXA83vnRhyyvqD+qEo1 aF83kIphqMgATBIyb91XMaFazKsCVdlpwx+BrV0fNAsWmhPtXkkKj6n1Fs5HPh3u8TOt fE8aTMqsGrPJiLGzabNh98bifNPhM5JTRcfuxsh6Or8/6h0J/r2Fk8ufPgr8NRBAbobj iJsCZugRSR9IVvUJzj+poovkFcCio8kDnkQTSMa3rK4kp0A9fH1oY+qarsMKO9fy2mZc tjOEgZkvqPsyQO7V5AA+Chqhvic8FffTeE8Ui4gA8FfJbFPvZste/zVxonXn/0f1VX9w 2h0Q== X-Gm-Message-State: ABy/qLbgyxwZGT1VyuF8DZU1QICl2jwKlWTuxEJostsJnL5PdTMpgD4S AcICTmbHPpHGtN9h5P2TCiCqFA== X-Google-Smtp-Source: APBJJlEGuE9XMp2o17pweTXvSm/cUnepOd+RAmks4vb0rNN5mYXs5lRIQjIPZq1tcfW+HRD2ARbQ5A== X-Received: by 2002:a05:6a00:1686:b0:67c:c8e4:8689 with SMTP id k6-20020a056a00168600b0067cc8e48689mr2051294pfc.12.1689266876471; Thu, 13 Jul 2023 09:47:56 -0700 (PDT) Received: from localhost ([50.38.6.230]) by smtp.gmail.com with ESMTPSA id 22-20020aa79216000000b006765cb3255asm5673024pfo.68.2023.07.13.09.47.55 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 13 Jul 2023 09:47:55 -0700 (PDT) Date: Thu, 13 Jul 2023 09:47:55 -0700 (PDT) X-Google-Original-Date: Thu, 13 Jul 2023 09:47:10 PDT (-0700) Subject: Re: [PATCH v5 3/4] riscv: Add ifunc-compatible hwprobe function In-Reply-To: CC: fweimer@redhat.com, libc-alpha@sourceware.org, Vineet Gupta , slewis@rivosinc.com From: Palmer Dabbelt To: Evan Green Message-ID: Mime-Version: 1.0 (MHng) Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-10.7 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, 13 Jul 2023 09:33:20 PDT (-0700), Evan Green wrote: > On Thu, Jul 13, 2023 at 12:07 AM Florian Weimer wrote: >> >> * Evan Green: >> >> > diff --git a/sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h b/sysdeps/unix/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 in >> the C file that goes into libc_nonshared. And ___riscv_hwprobe does not >> 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. We'd talked about this before, but it was just in the context of performance (it's essentially a pre-cached version of the syscall). Had I realized all these IFUNC-related symbol lookup headaches we probably would have done it the first time. We probably would have ended up with it anyway, though, so it's not super scary. The syscall would still be useful, both because it's sometimes easier to get at than HWCAP2 and because we can handle the more complex cases like heterogenous systems. It'll probably still be worth allowing IFUNC resolvers to plumb into the syscall, but if we can make life easier for users who just want the simple case that seems valuable too. > -Evan