* Re: [RFC 06/19] ktf: A simple debugfs interface to test results
From: Greg Kroah-Hartman @ 2019-08-15 10:52 UTC (permalink / raw)
To: Knut Omang
Cc: linux-kselftest, linux-kernel, linux-doc, linux-kbuild,
Shuah Khan, Jonathan Corbet, Masahiro Yamada, Michal Marek,
Shreyans Devendra Doshi, Alan Maguire, Brendan Higgins,
Kevin Hilman, Hidenori Yamaji, Frank Rowand, Timothy Bird,
Luis Chamberlain, Theodore Ts'o, Daniel Vetter, Stephen Boyd
In-Reply-To: <9629068a41a160de0145a18dd22924bce70f37fe.camel@oracle.com>
On Thu, Aug 15, 2019 at 12:35:26PM +0200, Knut Omang wrote:
> On Thu, 2019-08-15 at 10:49 +0200, Greg Kroah-Hartman wrote:
> > > I perfectly agree with you that reducing the hole for a race condition
> > > is generally a bad idea, but from the above mail thread
> > > it seems that's the only available choice for older kernels?
> >
> > I have no idea, but please, do not use that pattern of code as it is
> > racy in all kernels, from all of time.
>
> Ok, will remove it :-)
>
> I tried in vain to find the commit from Al Viro that made the code safe,
> to identify which kernels that are safe from this issue,
> but he has a **lot** of commits, do you have a clue for what/where to look?
>
> It will be good to have a mention/comment on this for future reference,
> like the earliest kernel version where this is safe.
Always use a "newer" kernel to be "safe" and you will be fine :)
> Maybe we can even get rid of some more of the remaining of these too..
> (I notice there's 65 cases of 'if (!try_module_get(THIS_MODULE))'
> right now)
Something to put on a TODO list somewhere...
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH] Documentation: crypto: crypto_engine: Fix Sphinx warning
From: Herbert Xu @ 2019-08-15 12:06 UTC (permalink / raw)
To: Jonathan Neuschäfer
Cc: linux-doc, David S. Miller, Jonathan Corbet, linux-crypto,
linux-kernel
In-Reply-To: <20190808163011.13468-1-j.neuschaefer@gmx.net>
On Thu, Aug 08, 2019 at 06:30:11PM +0200, Jonathan Neuschäfer wrote:
> This fixes the following Sphinx warning:
>
> Documentation/crypto/crypto_engine.rst:2:
> WARNING: Explicit markup ends without a blank line; unexpected unindent.
>
> Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> ---
> Documentation/crypto/crypto_engine.rst | 1 +
> 1 file changed, 1 insertion(+)
Patch applied. Thanks.
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: Non-random RDRAND Re: [PATCH] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h
From: Lendacky, Thomas @ 2019-08-15 13:01 UTC (permalink / raw)
To: Pavel Machek, tytso@mit.edu, nhorman@tuxdriver.com
Cc: linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org,
linux-pm@vger.kernel.org, x86@kernel.org, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, Rafael J . Wysocki, Chen Yu,
Jonathan Corbet
In-Reply-To: <20190814232434.GA31769@amd>
On 8/14/19 6:24 PM, Pavel Machek wrote:
> On Wed 2019-08-14 21:17:41, Lendacky, Thomas wrote:
>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>
>> There have been reports of RDRAND issues after resuming from suspend on
>> some AMD family 15h and family 16h systems. This issue stems from BIOS
>> not performing the proper steps during resume to ensure RDRAND continues
>> to function properly.
>
> Burn it with fire!
>
> I mean... people were afraid RDRAND would be backdoored, and you now
> confirm ... it indeed _is_ backdoored? /., here's news for you!
>
> So what is the impact? Does it give random-looking but predictable
> numbers after resume? Does it give all zeros? Something else?
See this article:
https://www.phoronix.com/scan.php?page=news_item&px=AMD-CPUs-RdRand-Suspend
Thanks,
Tom
>
>>
>> + rdrand_force [X86]
>> + On certain AMD processors, the advertisement of the
>> + RDRAND instruction has been disabled by the kernel
>> + because of buggy BIOS support, specifically around the
>> + suspend/resume path. This option allows for overriding
>> + that decision if it is known that the BIOS support for
>> + RDRAND is not buggy on the system.
>
> But this is not how we normally deal with buggy BIOSes. We don't want
> user to have to decide this...
>
> Should we introduce black-list or white-list of BIOS versions?
>
> Hmm. Actually.
>
> You are the CPU vendor. Surely you can tell us how to init RDRAND in
> kernel if BIOS failed to do that... can you?
>
> Pavel
>
^ permalink raw reply
* Re: [PATCH] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h
From: Lendacky, Thomas @ 2019-08-15 13:47 UTC (permalink / raw)
To: Borislav Petkov
Cc: linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org,
linux-pm@vger.kernel.org, x86@kernel.org, Thomas Gleixner,
Ingo Molnar, Rafael J . Wysocki, Pavel Machek, Chen Yu,
Jonathan Corbet
In-Reply-To: <20190815071940.GB15313@zn.tnic>
On 8/15/19 2:21 AM, Borislav Petkov wrote:
> On Wed, Aug 14, 2019 at 09:17:41PM +0000, Lendacky, Thomas wrote:
>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>
>> There have been reports of RDRAND issues after resuming from suspend on
>> some AMD family 15h and family 16h systems. This issue stems from BIOS
>> not performing the proper steps during resume to ensure RDRAND continues
>> to function properly.
>
> If this happens only during suspend/resume, this probably should
> be done only on configurations which have CONFIG_SUSPEND and/or
> CONFIG_HIBERNATION enabled. I'm assuming BIOS does init it properly
Sure, that makes sense. I'll tie it to CONFIG_PM_SLEEP since that is what
arch/x86/power/cpu.c is dependent on.
> at least during boot - I mean, they should've passed some sort of a
> certification.
>
> OTOH, if the breakage happens on resume, they clearly didn't test the
> BIOS suspend/resume. I mean, I'm not at all surprised - it is f*cking
> BIOS. News at 11.
>
>> RDRAND support is indicated by CPUID Fn00000001_ECX[30]. This bit can be
>> reset by clearing MSR C001_1004[62]. Any software that checks for RDRAND
>> support using CPUID, including the kernel, will believe that RDRAND is
>> not supported.
>>
>> Update the CPU initialization to clear the RDRAND CPUID bit for any family
>> 15h and 16h processor that supports RDRAND. If it is known that the family
>> 15h or family 16h system does not have an RDRAND resume issue or that the
>> system will not be placed in suspend, the "rdrand_force" kernel parameter
>> can be used to stop the clearing of the RDRAND CPUID bit.
>>
>> Additionally, update the suspend and resume path to save and restore the
>> MSR C001_1004 value to ensure that the RDRAND CPUID setting remains in
>> place after resuming from suspend.
>>
>> Note, that clearing the RDRAND CPUID bit does not prevent a processor
>> that normally supports the RDRAND instruction from executing the RDRAND
>> instruction. So any code that determined the support based on family and
>> model won't #UD.
>>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>> ---
>> .../admin-guide/kernel-parameters.txt | 8 ++
>> arch/x86/include/asm/msr-index.h | 1 +
>> arch/x86/kernel/cpu/amd.c | 42 ++++++++++
>> arch/x86/power/cpu.c | 83 ++++++++++++++++---
>> 4 files changed, 121 insertions(+), 13 deletions(-)
>>
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> index 47d981a86e2f..f47eb33958c1 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -4090,6 +4090,14 @@
>> Run specified binary instead of /init from the ramdisk,
>> used for early userspace startup. See initrd.
>>
>> + rdrand_force [X86]
>> + On certain AMD processors, the advertisement of the
>> + RDRAND instruction has been disabled by the kernel
>> + because of buggy BIOS support, specifically around the
>> + suspend/resume path. This option allows for overriding
>> + that decision if it is known that the BIOS support for
>> + RDRAND is not buggy on the system.
>> +
>> rdt= [HW,X86,RDT]
>> Turn on/off individual RDT features. List is:
>> cmt, mbmtotal, mbmlocal, l3cat, l3cdp, l2cat, l2cdp,
>> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
>> index 6b4fc2788078..29ae2b66b9e9 100644
>> --- a/arch/x86/include/asm/msr-index.h
>> +++ b/arch/x86/include/asm/msr-index.h
>> @@ -381,6 +381,7 @@
>> #define MSR_AMD64_PATCH_LEVEL 0x0000008b
>> #define MSR_AMD64_TSC_RATIO 0xc0000104
>> #define MSR_AMD64_NB_CFG 0xc001001f
>> +#define MSR_AMD64_CPUID_FN_00000001 0xc0011004
>
> I know the PPR has all the 0s but let's write it
>
> MSR_AMD64_CPUID_FN_1
>
> so that it is readable in the kernel.
Ok, will do.
>
>> #define MSR_AMD64_PATCH_LOADER 0xc0010020
>> #define MSR_AMD64_OSVW_ID_LENGTH 0xc0010140
>> #define MSR_AMD64_OSVW_STATUS 0xc0010141
>> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
>> index 3afe07d602dd..86ff1464302b 100644
>> --- a/arch/x86/kernel/cpu/amd.c
>> +++ b/arch/x86/kernel/cpu/amd.c
>> @@ -804,6 +804,40 @@ static void init_amd_ln(struct cpuinfo_x86 *c)
>> msr_set_bit(MSR_AMD64_DE_CFG, 31);
>> }
>>
>> +static bool rdrand_force;
>> +
>> +static int __init rdrand_force_cmdline(char *str)
>> +{
>> + rdrand_force = true;
>> +
>> + return 0;
>> +}
>> +early_param("rdrand_force", rdrand_force_cmdline);
>
> Let's make this a more generic param:
>
> rdrand=force[, ...]
>
> in case we wanna add some more opts here later.
Sure, I can do that. Do we want to tie this into the nordrand option and
add rdrand=off or keep that separate?
>
>> +
>> +static void init_hide_rdrand(struct cpuinfo_x86 *c)
>
> clear_rdrand_cpuid_bit()
>
> is what this function does.
Ok.
>
>> +{
>> + /*
>> + * The nordrand option can clear X86_FEATURE_RDRAND, so check for
>> + * RDRAND support using the CPUID function directly.
>> + */
>> + if (!(cpuid_ecx(1) & BIT(30)) || rdrand_force)
>> + return;
>> +
>> + msr_clear_bit(MSR_AMD64_CPUID_FN_00000001, 62);
>> + clear_cpu_cap(c, X86_FEATURE_RDRAND);
>> + pr_info_once("hiding RDRAND via CPUID\n");
>
> No need for that I guess - that's visible in /proc/cpuinfo.
I think this is a clearer indication that the action has taken place.
>
>> +}
>> +
>> +static void init_amd_jg(struct cpuinfo_x86 *c)
>> +{
>> + /*
>> + * Some BIOS implementations do not restore proper RDRAND support
>> + * across suspend and resume. Check on whether to hide the RDRAND
>> + * instruction support via CPUID.
>> + */
>> + init_hide_rdrand(c);
>> +}
>> +
>> static void init_amd_bd(struct cpuinfo_x86 *c)
>> {
>> u64 value;
>> @@ -818,6 +852,13 @@ static void init_amd_bd(struct cpuinfo_x86 *c)
>> wrmsrl_safe(MSR_F15H_IC_CFG, value);
>> }
>> }
>> +
>> + /*
>> + * Some BIOS implementations do not restore proper RDRAND support
>> + * across suspend and resume. Check on whether to hide the RDRAND
>> + * instruction support via CPUID.
>> + */
>> + init_hide_rdrand(c);
>> }
>>
>> static void init_amd_zn(struct cpuinfo_x86 *c)
>> @@ -860,6 +901,7 @@ static void init_amd(struct cpuinfo_x86 *c)
>> case 0x10: init_amd_gh(c); break;
>> case 0x12: init_amd_ln(c); break;
>> case 0x15: init_amd_bd(c); break;
>> + case 0x16: init_amd_jg(c); break;
>> case 0x17: init_amd_zn(c); break;
>> }
>> diff --git a/arch/x86/power/cpu.c b/arch/x86/power/cpu.c
>> index 1c58d8982728..146c4fd90c3d 100644
>> --- a/arch/x86/power/cpu.c
>> +++ b/arch/x86/power/cpu.c
>> @@ -12,6 +12,7 @@
>> #include <linux/smp.h>
>> #include <linux/perf_event.h>
>> #include <linux/tboot.h>
>> +#include <linux/dmi.h>
>>
>> #include <asm/pgtable.h>
>> #include <asm/proto.h>
>> @@ -23,7 +24,7 @@
>> #include <asm/debugreg.h>
>> #include <asm/cpu.h>
>> #include <asm/mmu_context.h>
>> -#include <linux/dmi.h>
>> +#include <asm/cpu_device_id.h>
>>
>> #ifdef CONFIG_X86_32
>> __visible unsigned long saved_context_ebx;
>> @@ -393,15 +394,14 @@ static int __init bsp_pm_check_init(void)
>>
>> core_initcall(bsp_pm_check_init);
>>
>> -static int msr_init_context(const u32 *msr_id, const int total_num)
>> +static int msr_build_context(const u32 *msr_id, const int num)
>> {
>> - int i = 0;
>> + struct saved_msrs *saved_msrs = &saved_context.saved_msrs;
>> struct saved_msr *msr_array;
>> + int total_num;
>> + int i, j;
>>
>> - if (saved_context.saved_msrs.array || saved_context.saved_msrs.num > 0) {
>> - pr_err("x86/pm: MSR quirk already applied, please check your DMI match table.\n");
>> - return -EINVAL;
>> - }
>> + total_num = saved_msrs->num + num;
>>
>> msr_array = kmalloc_array(total_num, sizeof(struct saved_msr), GFP_KERNEL);
>> if (!msr_array) {
>> @@ -409,19 +409,27 @@ static int msr_init_context(const u32 *msr_id, const int total_num)
>> return -ENOMEM;
>> }
>>
>> - for (i = 0; i < total_num; i++) {
>> - msr_array[i].info.msr_no = msr_id[i];
>> + if (saved_msrs->array) {
>> + /* Copy previous MSR save requests */
>> + memcpy(msr_array, saved_msrs->array,
>> + sizeof(struct saved_msr) * saved_msrs->num);
>
> Why do you need to copy those? Why can't you use the infrastructure like
> msr_initialize_bdw() does?
Not sure what you mean. We can't use the DMI stuff for this. So now, with
the x86 family checks, if anyone adds some DMI stuff or x86 family stuff
in the future that matches both the DMI and x86 family checks, this will
be called more than once and so you need to copy any previous settings and
add the new ones.
>
>> + kfree(saved_msrs->array);
>> + }
>> +
>> + for (i = saved_msrs->num, j = 0; i < total_num; i++, j++) {
>> + msr_array[i].info.msr_no = msr_id[j];
>> msr_array[i].valid = false;
>> msr_array[i].info.reg.q = 0;
>> }
>> - saved_context.saved_msrs.num = total_num;
>> - saved_context.saved_msrs.array = msr_array;
>> + saved_msrs->num = total_num;
>> + saved_msrs->array = msr_array;
>>
>> return 0;
>> }
>>
>> /*
>> - * The following section is a quirk framework for problematic BIOSen:
>> + * The following sections are a quirk framework for problematic BIOSen:
>> * Sometimes MSRs are modified by the BIOSen after suspended to
>> * RAM, this might cause unexpected behavior after wakeup.
>> * Thus we save/restore these specified MSRs across suspend/resume
>> @@ -436,7 +444,7 @@ static int msr_initialize_bdw(const struct dmi_system_id *d)
>> u32 bdw_msr_id[] = { MSR_IA32_THERM_CONTROL };
>>
>> pr_info("x86/pm: %s detected, MSR saving is needed during suspending.\n", d->ident);
>> - return msr_init_context(bdw_msr_id, ARRAY_SIZE(bdw_msr_id));
>> + return msr_build_context(bdw_msr_id, ARRAY_SIZE(bdw_msr_id));
>> }
>>
>> static const struct dmi_system_id msr_save_dmi_table[] = {
>> @@ -451,9 +459,58 @@ static const struct dmi_system_id msr_save_dmi_table[] = {
>> {}
>> };
>>
>> +static int msr_save_cpuid_features(const struct x86_cpu_id *c)
>> +{
>> + u32 cpuid_msr_id[] = {
>> + MSR_AMD64_CPUID_FN_00000001,
>> + };
>> +
>> + pr_info("x86/pm: family %#hx cpu detected, MSR saving is needed during suspending.\n",
>> + c->family);
>> +
>> + return msr_build_context(cpuid_msr_id, ARRAY_SIZE(cpuid_msr_id));
>> +}
>> +
>> +static const struct x86_cpu_id msr_save_cpu_table[] = {
>> + {
>> + .vendor = X86_VENDOR_AMD,
>> + .family = 0x15,
>> + .model = X86_MODEL_ANY,
>> + .feature = X86_FEATURE_ANY,
>> + .driver_data = (kernel_ulong_t)msr_save_cpuid_features,
>> + },
>> + {
>> + .vendor = X86_VENDOR_AMD,
>> + .family = 0x16,
>> + .model = X86_MODEL_ANY,
>> + .feature = X86_FEATURE_ANY,
>> + .driver_data = (kernel_ulong_t)msr_save_cpuid_features,
>> + },
>> + {}
>
> I think you can make that table a single entry by setting
>
> .vendor = X86_VENDOR_AMD,
> ...
> .feature = X86_FEATURE_RDRAND,
>
> and then checking family in msr_save_cpuid_features().
Except that X86_FEATURE_RDRAND isn't set anymore. I could create a new
software feature that is set when the CPUID bit is cleared if that's
preferred.
Thanks,
Tom
>
> Thx.
>
^ permalink raw reply
* [PATCH v3 -rcu] workqueue: Convert for_each_wq to use built-in list check
From: Joel Fernandes (Google) @ 2019-08-15 14:18 UTC (permalink / raw)
To: linux-kernel
Cc: Joel Fernandes (Google), Greg Kroah-Hartman, Jonathan Corbet,
Josh Triplett, Lai Jiangshan, linux-doc, Mathieu Desnoyers,
Paul E. McKenney, Rafael J. Wysocki, rcu, Steven Rostedt,
Tejun Heo
list_for_each_entry_rcu now has support to check for RCU reader sections
as well as lock. Just use the support in it, instead of explicitly
checking in the caller.
Acked-by: Tejun Heo <tj@kernel.org>
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
v1->v3: Changed lock_is_held() to lockdep_is_held()
kernel/workqueue.c | 10 ++--------
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 601d61150b65..e882477ebf6e 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -364,11 +364,6 @@ static void workqueue_sysfs_unregister(struct workqueue_struct *wq);
!lockdep_is_held(&wq_pool_mutex), \
"RCU or wq_pool_mutex should be held")
-#define assert_rcu_or_wq_mutex(wq) \
- RCU_LOCKDEP_WARN(!rcu_read_lock_held() && \
- !lockdep_is_held(&wq->mutex), \
- "RCU or wq->mutex should be held")
-
#define assert_rcu_or_wq_mutex_or_pool_mutex(wq) \
RCU_LOCKDEP_WARN(!rcu_read_lock_held() && \
!lockdep_is_held(&wq->mutex) && \
@@ -425,9 +420,8 @@ static void workqueue_sysfs_unregister(struct workqueue_struct *wq);
* ignored.
*/
#define for_each_pwq(pwq, wq) \
- list_for_each_entry_rcu((pwq), &(wq)->pwqs, pwqs_node) \
- if (({ assert_rcu_or_wq_mutex(wq); false; })) { } \
- else
+ list_for_each_entry_rcu((pwq), &(wq)->pwqs, pwqs_node, \
+ lockdep_is_held(&(wq->mutex)))
#ifdef CONFIG_DEBUG_OBJECTS_WORK
--
2.23.0.rc1.153.gdeed80330f-goog
^ permalink raw reply related
* Re: [PATCH v3 -rcu] workqueue: Convert for_each_wq to use built-in list check
From: Matthew Wilcox @ 2019-08-15 14:57 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: linux-kernel, Greg Kroah-Hartman, Jonathan Corbet, Josh Triplett,
Lai Jiangshan, linux-doc, Mathieu Desnoyers, Paul E. McKenney,
Rafael J. Wysocki, rcu, Steven Rostedt, Tejun Heo
In-Reply-To: <20190815141842.GB20599@google.com>
On Thu, Aug 15, 2019 at 10:18:42AM -0400, Joel Fernandes (Google) wrote:
> list_for_each_entry_rcu now has support to check for RCU reader sections
> as well as lock. Just use the support in it, instead of explicitly
> checking in the caller.
...
> #define assert_rcu_or_wq_mutex_or_pool_mutex(wq) \
> RCU_LOCKDEP_WARN(!rcu_read_lock_held() && \
> !lockdep_is_held(&wq->mutex) && \
Can't you also get rid of this macro?
It's used in one place:
static struct pool_workqueue *unbound_pwq_by_node(struct workqueue_struct *wq,
int node)
{
assert_rcu_or_wq_mutex_or_pool_mutex(wq);
/*
* XXX: @node can be NUMA_NO_NODE if CPU goes offline while a
* delayed item is pending. The plan is to keep CPU -> NODE
* mapping valid and stable across CPU on/offlines. Once that
* happens, this workaround can be removed.
*/
if (unlikely(node == NUMA_NO_NODE))
return wq->dfl_pwq;
return rcu_dereference_raw(wq->numa_pwq_tbl[node]);
}
Shouldn't we delete that assert and use
+ return rcu_dereference_check(wq->numa_pwq_tbl[node],
+ lockdep_is_held(&wq->mutex) ||
+ lockdep_is_held(&wq_pool_mutex));
^ permalink raw reply
* Re: Non-random RDRAND Re: [PATCH] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h
From: Theodore Y. Ts'o @ 2019-08-15 15:12 UTC (permalink / raw)
To: Pavel Machek
Cc: Lendacky, Thomas, nhorman, linux-kernel@vger.kernel.org,
linux-doc@vger.kernel.org, linux-pm@vger.kernel.org,
x86@kernel.org, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Rafael J . Wysocki, Chen Yu, Jonathan Corbet
In-Reply-To: <20190814232434.GA31769@amd>
On Thu, Aug 15, 2019 at 01:24:35AM +0200, Pavel Machek wrote:
> Burn it with fire!
>
> I mean... people were afraid RDRAND would be backdoored, and you now
> confirm ... it indeed _is_ backdoored? /., here's news for you!
To be fair to AMD, I wouldn't call it a backdoor. Hanlon's razor is
applicable here:
"Never attribute to malice that which can be adequately
explained by neglect."
(Sometimes other words are used instead of neglect, but i'm trying to
be nice.)
- Ted
P.S. Also applicable:
https://www.youtube.com/watch?v=XZxzJGgox_E
^ permalink raw reply
* Re: [PATCH v3 -rcu] workqueue: Convert for_each_wq to use built-in list check
From: Joel Fernandes @ 2019-08-15 15:24 UTC (permalink / raw)
To: Matthew Wilcox
Cc: linux-kernel, Greg Kroah-Hartman, Jonathan Corbet, Josh Triplett,
Lai Jiangshan, linux-doc, Mathieu Desnoyers, Paul E. McKenney,
Rafael J. Wysocki, rcu, Steven Rostedt, Tejun Heo
In-Reply-To: <20190815145749.GA18474@bombadil.infradead.org>
On Thu, Aug 15, 2019 at 07:57:49AM -0700, Matthew Wilcox wrote:
> On Thu, Aug 15, 2019 at 10:18:42AM -0400, Joel Fernandes (Google) wrote:
> > list_for_each_entry_rcu now has support to check for RCU reader sections
> > as well as lock. Just use the support in it, instead of explicitly
> > checking in the caller.
>
> ...
>
> > #define assert_rcu_or_wq_mutex_or_pool_mutex(wq) \
> > RCU_LOCKDEP_WARN(!rcu_read_lock_held() && \
> > !lockdep_is_held(&wq->mutex) && \
>
> Can't you also get rid of this macro?
Could be. But that should be a different patch. I am only cleaning up the RCU
list lockdep checking in this series since the series introduces that
concept). Please feel free to send a patch for the same.
Arguably, keeping the macro around also can be beneficial in the future.
> It's used in one place:
>
> static struct pool_workqueue *unbound_pwq_by_node(struct workqueue_struct *wq,
> int node)
> {
> assert_rcu_or_wq_mutex_or_pool_mutex(wq);
>
> /*
> * XXX: @node can be NUMA_NO_NODE if CPU goes offline while a
> * delayed item is pending. The plan is to keep CPU -> NODE
> * mapping valid and stable across CPU on/offlines. Once that
> * happens, this workaround can be removed.
> */
> if (unlikely(node == NUMA_NO_NODE))
> return wq->dfl_pwq;
>
> return rcu_dereference_raw(wq->numa_pwq_tbl[node]);
> }
>
> Shouldn't we delete that assert and use
>
> + return rcu_dereference_check(wq->numa_pwq_tbl[node],
> + lockdep_is_held(&wq->mutex) ||
> + lockdep_is_held(&wq_pool_mutex));
Makes sense. This API also does sparse checking. Also hopefully no sparse
issues show up because rcu_dereference_check() but anyone such issues should
be fixed as well.
thanks,
- Joel
>
^ permalink raw reply
* Re: [PATCH] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h
From: Borislav Petkov @ 2019-08-15 15:34 UTC (permalink / raw)
To: Lendacky, Thomas
Cc: linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org,
linux-pm@vger.kernel.org, x86@kernel.org, Thomas Gleixner,
Ingo Molnar, Rafael J . Wysocki, Pavel Machek, Chen Yu,
Jonathan Corbet
In-Reply-To: <768aa720-1db1-81ca-4d0d-adf31f4d134b@amd.com>
On Thu, Aug 15, 2019 at 01:47:24PM +0000, Lendacky, Thomas wrote:
> Sure, I can do that. Do we want to tie this into the nordrand option and
> add rdrand=off or keep that separate?
Yeah, I was looking at that this morning and I'd say keep 'em separate
because if you have to tie, you need to export functions and then
there's
setup_clear_cpu_cap(X86_FEATURE_RDSEED);
in the nordrand callback but then F15h and F16h don't have RDSEED and
people would wonder, why clear RDSEED on AMD, blabla... so keeping them
separate saves us all that.
> I think this is a clearer indication that the action has taken place.
Yeah, but what does that bring us? You wanna know this now, while
testing. Once that whole effort is done, it is a useless printing of
info which you have in cpuinfo already.
> Not sure what you mean. We can't use the DMI stuff for this. So now, with
> the x86 family checks, if anyone adds some DMI stuff or x86 family stuff
> in the future that matches both the DMI and x86 family checks, this will
> be called more than once and so you need to copy any previous settings and
> add the new ones.
I had a suspicion that it was something like that. Ok, this is not a
big structure currently so I guess it is fine but if it keeps growing,
it would need a proper redesign like making it a list and callbacks
doing list_add_tail() for MSRs which get added. It would avoid that
kmalloc and copying which is silly. Please put a comment ontop why we're
copying.
> Except that X86_FEATURE_RDRAND isn't set anymore. I could create a new
> software feature that is set when the CPUID bit is cleared if that's
> preferred.
Nah, let's leave it like you had it.
Thx.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply
* [PATCH v8 0/2] arm64 tagged address ABI
From: Catalin Marinas @ 2019-08-15 15:43 UTC (permalink / raw)
To: linux-arm-kernel, linux-mm
Cc: Andrew Morton, Vincenzo Frascino, Will Deacon, Andrey Konovalov,
Szabolcs Nagy, Kevin Brodsky, Dave P Martin, Dave Hansen,
linux-doc, linux-arch
Hi,
This series contains an update to the arm64 tagged address ABI
documentation posted here (v7):
http://lkml.kernel.org/r/20190807155321.9648-1-catalin.marinas@arm.com
together some adjustments to Andrey's patches (already queued through
different trees) following the discussions on the ABI documents:
http://lkml.kernel.org/r/cover.1563904656.git.andreyknvl@google.com
If there are not objections, I propose that that patch 1 (mm: untag user
pointers in mmap...) goes via the mm tree while the other 4 are routed
via the arm64 tree.
Changes in v8:
- removed mmap/munmap/mremap/brk from the list of syscalls not accepting
tagged pointers
- added ioctl() to the list of syscalls not accepting tagged pointers
- added shmat/shmdt to a list of syscalls not accepting tagged pointers
- prctl() now requires all unused arguments to be 0
- note about two-stage ABI relaxation since even without the prctl()
opt-in, the tag is still ignored on a few syscalls (untagged_addr() in
the kernel is unconditional)
- compilable example code together with syscall use
- added a note on tag preservation in the tagged-pointers.rst document
- various rewordings and cleanups
Catalin Marinas (3):
mm: untag user pointers in mmap/munmap/mremap/brk
arm64: Tighten the PR_{SET,GET}_TAGGED_ADDR_CTRL prctl() unused
arguments
arm64: Change the tagged_addr sysctl control semantics to only prevent
the opt-in
Vincenzo Frascino (2):
arm64: Define Documentation/arm64/tagged-address-abi.rst
arm64: Relax Documentation/arm64/tagged-pointers.rst
Documentation/arm64/tagged-address-abi.rst | 155 +++++++++++++++++++++
Documentation/arm64/tagged-pointers.rst | 23 ++-
arch/arm64/kernel/process.c | 17 ++-
kernel/sys.c | 4 +
mm/mmap.c | 5 +
mm/mremap.c | 6 +-
6 files changed, 191 insertions(+), 19 deletions(-)
create mode 100644 Documentation/arm64/tagged-address-abi.rst
^ permalink raw reply
* [PATCH v8 1/5] mm: untag user pointers in mmap/munmap/mremap/brk
From: Catalin Marinas @ 2019-08-15 15:43 UTC (permalink / raw)
To: linux-arm-kernel, linux-mm
Cc: Andrew Morton, Vincenzo Frascino, Will Deacon, Andrey Konovalov,
Szabolcs Nagy, Kevin Brodsky, Dave P Martin, Dave Hansen,
linux-doc, linux-arch
In-Reply-To: <20190815154403.16473-1-catalin.marinas@arm.com>
There isn't a good reason to differentiate between the user address
space layout modification syscalls and the other memory
permission/attributes ones (e.g. mprotect, madvise) w.r.t. the tagged
address ABI. Untag the user addresses on entry to these functions.
Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
---
mm/mmap.c | 5 +++++
mm/mremap.c | 6 +-----
2 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/mm/mmap.c b/mm/mmap.c
index 7e8c3e8ae75f..b766b633b7ae 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -201,6 +201,8 @@ SYSCALL_DEFINE1(brk, unsigned long, brk)
bool downgraded = false;
LIST_HEAD(uf);
+ brk = untagged_addr(brk);
+
if (down_write_killable(&mm->mmap_sem))
return -EINTR;
@@ -1573,6 +1575,8 @@ unsigned long ksys_mmap_pgoff(unsigned long addr, unsigned long len,
struct file *file = NULL;
unsigned long retval;
+ addr = untagged_addr(addr);
+
if (!(flags & MAP_ANONYMOUS)) {
audit_mmap_fd(fd, flags);
file = fget(fd);
@@ -2874,6 +2878,7 @@ EXPORT_SYMBOL(vm_munmap);
SYSCALL_DEFINE2(munmap, unsigned long, addr, size_t, len)
{
+ addr = untagged_addr(addr);
profile_munmap(addr);
return __vm_munmap(addr, len, true);
}
diff --git a/mm/mremap.c b/mm/mremap.c
index 64c9a3b8be0a..1fc8a29fbe3f 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -606,12 +606,8 @@ SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned long, old_len,
LIST_HEAD(uf_unmap_early);
LIST_HEAD(uf_unmap);
- /*
- * Architectures may interpret the tag passed to mmap as a background
- * colour for the corresponding vma. For mremap we don't allow tagged
- * new_addr to preserve similar behaviour to mmap.
- */
addr = untagged_addr(addr);
+ new_addr = untagged_addr(new_addr);
if (flags & ~(MREMAP_FIXED | MREMAP_MAYMOVE))
return ret;
^ permalink raw reply related
* [PATCH v8 2/5] arm64: Tighten the PR_{SET,GET}_TAGGED_ADDR_CTRL prctl() unused arguments
From: Catalin Marinas @ 2019-08-15 15:44 UTC (permalink / raw)
To: linux-arm-kernel, linux-mm
Cc: Andrew Morton, Vincenzo Frascino, Will Deacon, Andrey Konovalov,
Szabolcs Nagy, Kevin Brodsky, Dave P Martin, Dave Hansen,
linux-doc, linux-arch
In-Reply-To: <20190815154403.16473-1-catalin.marinas@arm.com>
Require that arg{3,4,5} of the PR_{SET,GET}_TAGGED_ADDR_CTRL prctl and
arg2 of the PR_GET_TAGGED_ADDR_CTRL prctl() are zero rather than ignored
for future extensions.
Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
---
kernel/sys.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/kernel/sys.c b/kernel/sys.c
index c6c4d5358bd3..ec48396b4943 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -2499,9 +2499,13 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
error = PAC_RESET_KEYS(me, arg2);
break;
case PR_SET_TAGGED_ADDR_CTRL:
+ if (arg3 || arg4 || arg5)
+ return -EINVAL;
error = SET_TAGGED_ADDR_CTRL(arg2);
break;
case PR_GET_TAGGED_ADDR_CTRL:
+ if (arg2 || arg3 || arg4 || arg5)
+ return -EINVAL;
error = GET_TAGGED_ADDR_CTRL();
break;
default:
^ permalink raw reply related
* [PATCH v8 3/5] arm64: Change the tagged_addr sysctl control semantics to only prevent the opt-in
From: Catalin Marinas @ 2019-08-15 15:44 UTC (permalink / raw)
To: linux-arm-kernel, linux-mm
Cc: Andrew Morton, Vincenzo Frascino, Will Deacon, Andrey Konovalov,
Szabolcs Nagy, Kevin Brodsky, Dave P Martin, Dave Hansen,
linux-doc, linux-arch
In-Reply-To: <20190815154403.16473-1-catalin.marinas@arm.com>
First rename the sysctl control to abi.tagged_addr_disabled and make it
default off (zero). When abi.tagged_addr_disabled == 1, only block the
enabling of the TBI ABI via prctl(PR_SET_TAGGED_ADDR_CTRL, PR_TAGGED_ADDR_ENABLE).
Getting the status of the ABI or disabling it is still allowed.
Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
---
arch/arm64/kernel/process.c | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 76b7c55026aa..03689c0beb34 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -579,17 +579,22 @@ void arch_setup_new_exec(void)
/*
* Control the relaxed ABI allowing tagged user addresses into the kernel.
*/
-static unsigned int tagged_addr_prctl_allowed = 1;
+static unsigned int tagged_addr_disabled;
long set_tagged_addr_ctrl(unsigned long arg)
{
- if (!tagged_addr_prctl_allowed)
- return -EINVAL;
if (is_compat_task())
return -EINVAL;
if (arg & ~PR_TAGGED_ADDR_ENABLE)
return -EINVAL;
+ /*
+ * Do not allow the enabling of the tagged address ABI if globally
+ * disabled via sysctl abi.tagged_addr_disabled.
+ */
+ if (arg & PR_TAGGED_ADDR_ENABLE && tagged_addr_disabled)
+ return -EINVAL;
+
update_thread_flag(TIF_TAGGED_ADDR, arg & PR_TAGGED_ADDR_ENABLE);
return 0;
@@ -597,8 +602,6 @@ long set_tagged_addr_ctrl(unsigned long arg)
long get_tagged_addr_ctrl(void)
{
- if (!tagged_addr_prctl_allowed)
- return -EINVAL;
if (is_compat_task())
return -EINVAL;
@@ -618,9 +621,9 @@ static int one = 1;
static struct ctl_table tagged_addr_sysctl_table[] = {
{
- .procname = "tagged_addr",
+ .procname = "tagged_addr_disabled",
.mode = 0644,
- .data = &tagged_addr_prctl_allowed,
+ .data = &tagged_addr_disabled,
.maxlen = sizeof(int),
.proc_handler = proc_dointvec_minmax,
.extra1 = &zero,
^ permalink raw reply related
* [PATCH v8 4/5] arm64: Define Documentation/arm64/tagged-address-abi.rst
From: Catalin Marinas @ 2019-08-15 15:44 UTC (permalink / raw)
To: linux-arm-kernel, linux-mm
Cc: Andrew Morton, Vincenzo Frascino, Will Deacon, Andrey Konovalov,
Szabolcs Nagy, Kevin Brodsky, Dave P Martin, Dave Hansen,
linux-doc, linux-arch
In-Reply-To: <20190815154403.16473-1-catalin.marinas@arm.com>
From: Vincenzo Frascino <vincenzo.frascino@arm.com>
On AArch64 the TCR_EL1.TBI0 bit is set by default, allowing userspace
(EL0) to perform memory accesses through 64-bit pointers with a non-zero
top byte. Introduce the document describing the relaxation of the
syscall ABI that allows userspace to pass certain tagged pointers to
kernel syscalls.
Cc: Will Deacon <will.deacon@arm.com>
Cc: Andrey Konovalov <andreyknvl@google.com>
Cc: Szabolcs Nagy <szabolcs.nagy@arm.com>
Cc: Kevin Brodsky <kevin.brodsky@arm.com>
Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
Co-developed-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
---
Documentation/arm64/tagged-address-abi.rst | 155 +++++++++++++++++++++
1 file changed, 155 insertions(+)
create mode 100644 Documentation/arm64/tagged-address-abi.rst
diff --git a/Documentation/arm64/tagged-address-abi.rst b/Documentation/arm64/tagged-address-abi.rst
new file mode 100644
index 000000000000..8808337775d6
--- /dev/null
+++ b/Documentation/arm64/tagged-address-abi.rst
@@ -0,0 +1,155 @@
+==========================
+AArch64 TAGGED ADDRESS ABI
+==========================
+
+Authors: Vincenzo Frascino <vincenzo.frascino@arm.com>
+ Catalin Marinas <catalin.marinas@arm.com>
+
+Date: 15 August 2019
+
+This document describes the usage and semantics of the Tagged Address
+ABI on AArch64 Linux.
+
+1. Introduction
+---------------
+
+On AArch64 the TCR_EL1.TBI0 bit is set by default, allowing userspace
+(EL0) to perform memory accesses through 64-bit pointers with a non-zero
+top byte. This document describes the relaxation of the syscall ABI that
+allows userspace to pass certain tagged pointers to kernel syscalls.
+
+2. AArch64 Tagged Address ABI
+-----------------------------
+
+From the kernel syscall interface perspective and for the purposes of
+this document, a "valid tagged pointer" is a pointer with a potentially
+non-zero top-byte that references an address in the user process address
+space obtained in one of the following ways:
+
+- mmap() done by the process itself (or its parent), where either:
+
+ - flags have the **MAP_ANONYMOUS** bit set
+ - the file descriptor refers to a regular file (including those
+ returned by memfd_create()) or **/dev/zero**
+
+- brk() system call done by the process itself (i.e. the heap area
+ between the initial location of the program break at process creation
+ and its current location).
+
+- any memory mapped by the kernel in the address space of the process
+ during creation and with the same restrictions as for mmap() above
+ (e.g. data, bss, stack).
+
+The AArch64 Tagged Address ABI has two stages of relaxation depending
+how the user addresses are used by the kernel:
+
+1. User addresses not accessed by the kernel but used for address space
+ management (e.g. mmap(), mprotect(), madvise()). The use of valid
+ tagged pointers in this context is always allowed.
+
+2. User addresses accessed by the kernel (e.g. write()). This ABI
+ relaxation is disabled by default and the application thread needs to
+ explicitly enable it via **prctl()** as follows:
+
+ - **PR_SET_TAGGED_ADDR_CTRL**: enable or disable the AArch64 Tagged
+ Address ABI for the calling thread.
+
+ The (unsigned int) arg2 argument is a bit mask describing the
+ control mode used:
+
+ - **PR_TAGGED_ADDR_ENABLE**: enable AArch64 Tagged Address ABI.
+ Default status is disabled.
+
+ Arguments arg3, arg4, and arg5 must be 0.
+
+ - **PR_GET_TAGGED_ADDR_CTRL**: get the status of the AArch64 Tagged
+ Address ABI for the calling thread.
+
+ Arguments arg2, arg3, arg4, and arg5 must be 0.
+
+ The ABI properties described above are thread-scoped, inherited on
+ clone() and fork() and cleared on exec().
+
+ Calling prctl(PR_SET_TAGGED_ADDR_CTRL, PR_TAGGED_ADDR_ENABLE, 0, 0, 0)
+ returns -EINVAL if the AArch64 Tagged Address ABI is globally disabled
+ by sysctl abi.tagged_addr_disabled=1. The default sysctl
+ abi.tagged_addr_disabled configuration is 0.
+
+When the AArch64 Tagged Address ABI is enabled for a thread, the
+following behaviours are guaranteed:
+
+- All syscalls except the cases mentioned in section 3 can accept any
+ valid tagged pointer.
+
+- The syscall behaviour is undefined for invalid tagged pointers: it may
+ result in an error code being returned, a (fatal) signal being raised,
+ or other modes of failure.
+
+- A valid tagged pointer has the same semantics as the corresponding
+ untagged pointer.
+
+A definition of the meaning of tagged pointers on AArch64 can be found
+in Documentation/arm64/tagged-pointers.rst.
+
+3. AArch64 Tagged Address ABI Exceptions
+-----------------------------------------
+
+The following system call parameters must be untagged regardless of the
+ABI relaxation:
+
+- prctl() other than arguments pointing to user structures to be
+ accessed by the kernel.
+
+- ioctl() other than arguments pointing to user structures to be
+ accessed by the kernel.
+
+- shmat() and shmdt().
+
+Any attempt to use non-zero tagged pointers may result in an error code
+being returned, a (fatal) signal being raised, or other modes of
+failure.
+
+4. Example of correct usage
+---------------------------
+.. code-block:: c
+
+ #include <stdlib.h>
+ #include <string.h>
+ #include <unistd.h>
+ #include <sys/mman.h>
+ #include <sys/prctl.h>
+
+ #define PR_SET_TAGGED_ADDR_CTRL 55
+ #define PR_TAGGED_ADDR_ENABLE (1UL << 0)
+
+ #define TAG_SHIFT 56
+
+ int main(void)
+ {
+ int tbi_enabled = 0;
+ unsigned long tag = 0;
+ char *ptr;
+
+ /* check/enable the tagged address ABI */
+ if (!prctl(PR_SET_TAGGED_ADDR_CTRL, PR_TAGGED_ADDR_ENABLE, 0, 0, 0))
+ tbi_enabled = 1;
+
+ /* memory allocation */
+ ptr = mmap(NULL, sysconf(_SC_PAGE_SIZE), PROT_READ | PROT_WRITE,
+ MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+ if (ptr == MAP_FAILED)
+ return 1;
+
+ /* set a non-zero tag if the ABI is available */
+ if (tbi_enabled)
+ tag = rand() & 0xff;
+ ptr = (char *)((unsigned long)ptr | (tag << TAG_SHIFT));
+
+ /* memory access to a tagged address */
+ strcpy(ptr, "tagged pointer\n");
+
+ /* syscall with a tagged pointer */
+ write(1, ptr, strlen(ptr));
+
+ return 0;
+ }
^ permalink raw reply related
* [PATCH v8 5/5] arm64: Relax Documentation/arm64/tagged-pointers.rst
From: Catalin Marinas @ 2019-08-15 15:44 UTC (permalink / raw)
To: linux-arm-kernel, linux-mm
Cc: Andrew Morton, Vincenzo Frascino, Will Deacon, Andrey Konovalov,
Szabolcs Nagy, Kevin Brodsky, Dave P Martin, Dave Hansen,
linux-doc, linux-arch
In-Reply-To: <20190815154403.16473-1-catalin.marinas@arm.com>
From: Vincenzo Frascino <vincenzo.frascino@arm.com>
On AArch64 the TCR_EL1.TBI0 bit is set by default, allowing userspace
(EL0) to perform memory accesses through 64-bit pointers with a non-zero
top byte. However, such pointers were not allowed at the user-kernel
syscall ABI boundary.
With the Tagged Address ABI patchset, it is now possible to pass tagged
pointers to the syscalls. Relax the requirements described in
tagged-pointers.rst to be compliant with the behaviours guaranteed by
the AArch64 Tagged Address ABI.
Cc: Will Deacon <will.deacon@arm.com>
Cc: Andrey Konovalov <andreyknvl@google.com>
Cc: Szabolcs Nagy <szabolcs.nagy@arm.com>
Cc: Kevin Brodsky <kevin.brodsky@arm.com>
Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
Co-developed-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
---
Documentation/arm64/tagged-pointers.rst | 23 ++++++++++++++++-------
1 file changed, 16 insertions(+), 7 deletions(-)
diff --git a/Documentation/arm64/tagged-pointers.rst b/Documentation/arm64/tagged-pointers.rst
index 2acdec3ebbeb..fd5306019e91 100644
--- a/Documentation/arm64/tagged-pointers.rst
+++ b/Documentation/arm64/tagged-pointers.rst
@@ -20,7 +20,9 @@ Passing tagged addresses to the kernel
--------------------------------------
All interpretation of userspace memory addresses by the kernel assumes
-an address tag of 0x00.
+an address tag of 0x00, unless the application enables the AArch64
+Tagged Address ABI explicitly
+(Documentation/arm64/tagged-address-abi.rst).
This includes, but is not limited to, addresses found in:
@@ -33,13 +35,15 @@ This includes, but is not limited to, addresses found in:
- the frame pointer (x29) and frame records, e.g. when interpreting
them to generate a backtrace or call graph.
-Using non-zero address tags in any of these locations may result in an
-error code being returned, a (fatal) signal being raised, or other modes
-of failure.
+Using non-zero address tags in any of these locations when the
+userspace application did not enable the AArch64 Tagged Address ABI may
+result in an error code being returned, a (fatal) signal being raised,
+or other modes of failure.
-For these reasons, passing non-zero address tags to the kernel via
-system calls is forbidden, and using a non-zero address tag for sp is
-strongly discouraged.
+For these reasons, when the AArch64 Tagged Address ABI is disabled,
+passing non-zero address tags to the kernel via system calls is
+forbidden, and using a non-zero address tag for sp is strongly
+discouraged.
Programs maintaining a frame pointer and frame records that use non-zero
address tags may suffer impaired or inaccurate debug and profiling
@@ -59,6 +63,11 @@ be preserved.
The architecture prevents the use of a tagged PC, so the upper byte will
be set to a sign-extension of bit 55 on exception return.
+This behaviour is maintained when the AArch64 Tagged Address ABI is
+enabled. In addition, with the exceptions above, the kernel will
+preserve any non-zero tags passed by the user via syscalls and stored in
+kernel data structures (e.g. set_robust_list(), sigaltstack()).
+
Other considerations
--------------------
^ permalink raw reply related
* Re: [PATCH v8 4/5] arm64: Define Documentation/arm64/tagged-address-abi.rst
From: Kevin Brodsky @ 2019-08-15 16:54 UTC (permalink / raw)
To: Catalin Marinas, linux-arm-kernel, linux-mm
Cc: Andrew Morton, Vincenzo Frascino, Will Deacon, Andrey Konovalov,
Szabolcs Nagy, Dave P Martin, Dave Hansen, linux-doc, linux-arch
In-Reply-To: <20190815154403.16473-5-catalin.marinas@arm.com>
On 15/08/2019 16:44, Catalin Marinas wrote:
> From: Vincenzo Frascino <vincenzo.frascino@arm.com>
>
> On AArch64 the TCR_EL1.TBI0 bit is set by default, allowing userspace
> (EL0) to perform memory accesses through 64-bit pointers with a non-zero
> top byte. Introduce the document describing the relaxation of the
> syscall ABI that allows userspace to pass certain tagged pointers to
> kernel syscalls.
>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Andrey Konovalov <andreyknvl@google.com>
> Cc: Szabolcs Nagy <szabolcs.nagy@arm.com>
> Cc: Kevin Brodsky <kevin.brodsky@arm.com>
> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
> Co-developed-by: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> ---
> Documentation/arm64/tagged-address-abi.rst | 155 +++++++++++++++++++++
> 1 file changed, 155 insertions(+)
> create mode 100644 Documentation/arm64/tagged-address-abi.rst
>
> diff --git a/Documentation/arm64/tagged-address-abi.rst b/Documentation/arm64/tagged-address-abi.rst
> new file mode 100644
> index 000000000000..8808337775d6
> --- /dev/null
> +++ b/Documentation/arm64/tagged-address-abi.rst
> @@ -0,0 +1,155 @@
> +==========================
> +AArch64 TAGGED ADDRESS ABI
> +==========================
> +
> +Authors: Vincenzo Frascino <vincenzo.frascino@arm.com>
> + Catalin Marinas <catalin.marinas@arm.com>
> +
> +Date: 15 August 2019
> +
> +This document describes the usage and semantics of the Tagged Address
> +ABI on AArch64 Linux.
> +
> +1. Introduction
> +---------------
> +
> +On AArch64 the TCR_EL1.TBI0 bit is set by default, allowing userspace
> +(EL0) to perform memory accesses through 64-bit pointers with a non-zero
> +top byte. This document describes the relaxation of the syscall ABI that
> +allows userspace to pass certain tagged pointers to kernel syscalls.
> +
> +2. AArch64 Tagged Address ABI
> +-----------------------------
> +
> +From the kernel syscall interface perspective and for the purposes of
> +this document, a "valid tagged pointer" is a pointer with a potentially
> +non-zero top-byte that references an address in the user process address
> +space obtained in one of the following ways:
> +
> +- mmap() done by the process itself (or its parent), where either:
The "parent" aspect is a useful addition, but technically, the mapping may have been
established by any process indirectly forked from the current process, not just its
immediate parent. I wonder if there is a better way to formulate this, to avoid this
complication. Maybe simply "mmap() syscall" (syscalls are always made from userspace,
and any mapping requested by userspace is eligible here)?
> +
> + - flags have the **MAP_ANONYMOUS** bit set
> + - the file descriptor refers to a regular file (including those
> + returned by memfd_create()) or **/dev/zero**
> +
> +- brk() system call done by the process itself (i.e. the heap area
Same idea.
> + between the initial location of the program break at process creation
> + and its current location).
> +
> +- any memory mapped by the kernel in the address space of the process
> + during creation and with the same restrictions as for mmap() above
> + (e.g. data, bss, stack).
> +
> +The AArch64 Tagged Address ABI has two stages of relaxation depending
> +how the user addresses are used by the kernel:
> +
> +1. User addresses not accessed by the kernel but used for address space
> + management (e.g. mmap(), mprotect(), madvise()). The use of valid
> + tagged pointers in this context is always allowed.
> +
> +2. User addresses accessed by the kernel (e.g. write()). This ABI
> + relaxation is disabled by default and the application thread needs to
> + explicitly enable it via **prctl()** as follows:
> +
> + - **PR_SET_TAGGED_ADDR_CTRL**: enable or disable the AArch64 Tagged
> + Address ABI for the calling thread.
> +
> + The (unsigned int) arg2 argument is a bit mask describing the
> + control mode used:
> +
> + - **PR_TAGGED_ADDR_ENABLE**: enable AArch64 Tagged Address ABI.
> + Default status is disabled.
> +
> + Arguments arg3, arg4, and arg5 must be 0.
> +
> + - **PR_GET_TAGGED_ADDR_CTRL**: get the status of the AArch64 Tagged
> + Address ABI for the calling thread.
> +
> + Arguments arg2, arg3, arg4, and arg5 must be 0.
> +
> + The ABI properties described above are thread-scoped, inherited on
> + clone() and fork() and cleared on exec().
> +
> + Calling prctl(PR_SET_TAGGED_ADDR_CTRL, PR_TAGGED_ADDR_ENABLE, 0, 0, 0)
> + returns -EINVAL if the AArch64 Tagged Address ABI is globally disabled
> + by sysctl abi.tagged_addr_disabled=1. The default sysctl
> + abi.tagged_addr_disabled configuration is 0.
> +
> +When the AArch64 Tagged Address ABI is enabled for a thread, the
> +following behaviours are guaranteed:
> +
> +- All syscalls except the cases mentioned in section 3 can accept any
> + valid tagged pointer.
> +
> +- The syscall behaviour is undefined for invalid tagged pointers: it may
> + result in an error code being returned, a (fatal) signal being raised,
> + or other modes of failure.
> +
> +- A valid tagged pointer has the same semantics as the corresponding
> + untagged pointer.
> +
> +A definition of the meaning of tagged pointers on AArch64 can be found
> +in Documentation/arm64/tagged-pointers.rst.
> +
> +3. AArch64 Tagged Address ABI Exceptions
> +-----------------------------------------
> +
> +The following system call parameters must be untagged regardless of the
> +ABI relaxation:
> +
> +- prctl() other than arguments pointing to user structures to be
> + accessed by the kernel.
> +
> +- ioctl() other than arguments pointing to user structures to be
> + accessed by the kernel.
Isn't "user structures" too restrictive? For instance, PR_SET_NAME takes a char *,
and there's no reason not allow it to be tagged. Maybe a more generic "user data"?
There is the additional issue of user struct's containing pointers, I guess the
restriction should apply recursively...
Otherwise, the ABI looks pretty good to me, especially the new address space
management / user data distinction.
Kevin
> +
> +- shmat() and shmdt().
> +
> +Any attempt to use non-zero tagged pointers may result in an error code
> +being returned, a (fatal) signal being raised, or other modes of
> +failure.
> +
> +4. Example of correct usage
> +---------------------------
> +.. code-block:: c
> +
> + #include <stdlib.h>
> + #include <string.h>
> + #include <unistd.h>
> + #include <sys/mman.h>
> + #include <sys/prctl.h>
> +
> + #define PR_SET_TAGGED_ADDR_CTRL 55
> + #define PR_TAGGED_ADDR_ENABLE (1UL << 0)
> +
> + #define TAG_SHIFT 56
> +
> + int main(void)
> + {
> + int tbi_enabled = 0;
> + unsigned long tag = 0;
> + char *ptr;
> +
> + /* check/enable the tagged address ABI */
> + if (!prctl(PR_SET_TAGGED_ADDR_CTRL, PR_TAGGED_ADDR_ENABLE, 0, 0, 0))
> + tbi_enabled = 1;
> +
> + /* memory allocation */
> + ptr = mmap(NULL, sysconf(_SC_PAGE_SIZE), PROT_READ | PROT_WRITE,
> + MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> + if (ptr == MAP_FAILED)
> + return 1;
> +
> + /* set a non-zero tag if the ABI is available */
> + if (tbi_enabled)
> + tag = rand() & 0xff;
> + ptr = (char *)((unsigned long)ptr | (tag << TAG_SHIFT));
> +
> + /* memory access to a tagged address */
> + strcpy(ptr, "tagged pointer\n");
> +
> + /* syscall with a tagged pointer */
> + write(1, ptr, strlen(ptr));
> +
> + return 0;
> + }
^ permalink raw reply
* [PATCH 1/2] trace-vmscan-postprocess: sync with tracepoints updates
From: Florian Schmidt @ 2019-08-15 17:13 UTC (permalink / raw)
To: linux-doc@vger.kernel.org; +Cc: Florian Schmidt
In-Reply-To: <20190815164840.1141-1-florian.schmidt@nutanix.com>
mm_vmscan_{direct_reclaim_begin,wakeup_kswapd,lru_isolate,lru_shrink_active}
changed their output to the point where the script throws warnings and
errors. Update it to be properly in line with those changes.
Signed-off-by: Florian Schmidt <florian.schmidt@nutanix.com>
---
.../postprocess/trace-vmscan-postprocess.pl | 23 +++++++++----------
1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/Documentation/trace/postprocess/trace-vmscan-postprocess.pl b/Documentation/trace/postprocess/trace-vmscan-postprocess.pl
index 995da15b16ca..6c4e3fde9447 100644
--- a/Documentation/trace/postprocess/trace-vmscan-postprocess.pl
+++ b/Documentation/trace/postprocess/trace-vmscan-postprocess.pl
@@ -107,14 +107,14 @@ GetOptions(
);
# Defaults for dynamically discovered regex's
-my $regex_direct_begin_default = 'order=([0-9]*) may_writepage=([0-9]*) gfp_flags=([A-Z_|]*)';
+my $regex_direct_begin_default = 'order=([0-9]*) gfp_flags=([A-Z_|]*)';
my $regex_direct_end_default = 'nr_reclaimed=([0-9]*)';
my $regex_kswapd_wake_default = 'nid=([0-9]*) order=([0-9]*)';
my $regex_kswapd_sleep_default = 'nid=([0-9]*)';
-my $regex_wakeup_kswapd_default = 'nid=([0-9]*) zid=([0-9]*) order=([0-9]*) gfp_flags=([A-Z_|]*)';
-my $regex_lru_isolate_default = 'isolate_mode=([0-9]*) classzone_idx=([0-9]*) order=([0-9]*) nr_requested=([0-9]*) nr_scanned=([0-9]*) nr_skipped=([0-9]*) nr_taken=([0-9]*) lru=([a-z_]*)';
+my $regex_wakeup_kswapd_default = 'nid=([0-9]*) order=([0-9]*) gfp_flags=([A-Z_|]*)';
+my $regex_lru_isolate_default = 'isolate_mode=([0-9]*) classzone=([0-9]*) order=([0-9]*) nr_requested=([0-9]*) nr_scanned=([0-9]*) nr_skipped=([0-9]*) nr_taken=([0-9]*) lru=([a-z_]*)';
my $regex_lru_shrink_inactive_default = 'nid=([0-9]*) nr_scanned=([0-9]*) nr_reclaimed=([0-9]*) nr_dirty=([0-9]*) nr_writeback=([0-9]*) nr_congested=([0-9]*) nr_immediate=([0-9]*) nr_activate_anon=([0-9]*) nr_activate_file=([0-9]*) nr_ref_keep=([0-9]*) nr_unmap_fail=([0-9]*) priority=([0-9]*) flags=([A-Z_|]*)';
-my $regex_lru_shrink_active_default = 'lru=([A-Z_]*) nr_scanned=([0-9]*) nr_rotated=([0-9]*) priority=([0-9]*)';
+my $regex_lru_shrink_active_default = 'nid=([0-9]*) nr_active=([0-9]*) nr_deactivated=([0-9]*) nr_referenced=([0-9]*) priority=([0-9]*) flags=([A-Z_]*)';
my $regex_writepage_default = 'page=([0-9a-f]*) pfn=([0-9]*) flags=([A-Z_|]*)';
# Dyanically discovered regex
@@ -184,8 +184,7 @@ sub generate_traceevent_regex {
$regex_direct_begin = generate_traceevent_regex(
"vmscan/mm_vmscan_direct_reclaim_begin",
$regex_direct_begin_default,
- "order", "may_writepage",
- "gfp_flags");
+ "order", "gfp_flags");
$regex_direct_end = generate_traceevent_regex(
"vmscan/mm_vmscan_direct_reclaim_end",
$regex_direct_end_default,
@@ -201,11 +200,11 @@ $regex_kswapd_sleep = generate_traceevent_regex(
$regex_wakeup_kswapd = generate_traceevent_regex(
"vmscan/mm_vmscan_wakeup_kswapd",
$regex_wakeup_kswapd_default,
- "nid", "zid", "order", "gfp_flags");
+ "nid", "order", "gfp_flags");
$regex_lru_isolate = generate_traceevent_regex(
"vmscan/mm_vmscan_lru_isolate",
$regex_lru_isolate_default,
- "isolate_mode", "classzone_idx", "order",
+ "isolate_mode", "classzone", "order",
"nr_requested", "nr_scanned", "nr_skipped", "nr_taken",
"lru");
$regex_lru_shrink_inactive = generate_traceevent_regex(
@@ -218,9 +217,9 @@ $regex_lru_shrink_inactive = generate_traceevent_regex(
$regex_lru_shrink_active = generate_traceevent_regex(
"vmscan/mm_vmscan_lru_shrink_active",
$regex_lru_shrink_active_default,
- "nid", "zid",
- "lru",
- "nr_scanned", "nr_rotated", "priority");
+ "nid",
+ "nr_taken", "nr_active", "nr_deactivated", "nr_referenced",
+ "priority", "flags");
$regex_writepage = generate_traceevent_regex(
"vmscan/mm_vmscan_writepage",
$regex_writepage_default,
@@ -371,7 +370,7 @@ EVENT_PROCESS:
print " $regex_wakeup_kswapd\n";
next;
}
- my $order = $3;
+ my $order = $2;
$perprocesspid{$process_pid}->{MM_VMSCAN_WAKEUP_KSWAPD_PERORDER}[$order]++;
} elsif ($tracepoint eq "mm_vmscan_lru_isolate") {
$details = $6;
--
2.23.0.rc1
^ permalink raw reply related
* [PATCH 0/2] trace-vmscan-postprocess: fix parsing and output
From: Florian Schmidt @ 2019-08-15 17:13 UTC (permalink / raw)
To: linux-doc@vger.kernel.org; +Cc: Florian Schmidt
This patch series updates trace-vmscan-postprocess.pl to work without
throwing warnings and errors which stem from updates to several trace
points.
3481c37ffa1d ("mm/vmscan: drop may_writepage and classzone_idx from
direct reclaim begin template") removed "may_writepage" from
mm_vmscan_direct_reclaim_begin, and 3b775998eca7
("include/trace/events/vmscan.h: drop zone id from kswapd tracepoints")
removed "zid" from mm_vmscan_wakeup_kswapd. The output of
mm_vmscan_lru_isolate and mm_vmscan_lru_shrink_active seems to never
have matched the format of the trace point output since they were
created, or at least for as long as I can tell. Patch 1 aligns the
format parsing of the perl script with the current output of the trace
points.
In addition, the tables that are printed by the script were not properly
aligned any more, so patch 2 fixes the spacing.
A side remark: parsing the trace output for mm_vmscan_lru_shrink_active
has been in the script ever since it was created in 2010, but at no
point the parsed output was ever used for anything. I updated the
parsing code now, but I wonder if we could just get rid of that part...
Florian Schmidt (2):
trace-vmscan-postprocess: sync with tracepoints updates
trace-vmscan-postprocess: fix output table spacing
.../postprocess/trace-vmscan-postprocess.pl | 29 +++++++++----------
1 file changed, 14 insertions(+), 15 deletions(-)
--
2.23.0.rc1
^ permalink raw reply
* [PATCH 2/2] trace-vmscan-postprocess: fix output table spacing
From: Florian Schmidt @ 2019-08-15 17:14 UTC (permalink / raw)
To: linux-doc@vger.kernel.org; +Cc: Florian Schmidt
In-Reply-To: <20190815164840.1141-1-florian.schmidt@nutanix.com>
Fix spacing so that both the headers in themselves, as well as the
output of the two tables related to each other, are properly aligned.
Signed-off-by: Florian Schmidt <florian.schmidt@nutanix.com>
---
Documentation/trace/postprocess/trace-vmscan-postprocess.pl | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/Documentation/trace/postprocess/trace-vmscan-postprocess.pl b/Documentation/trace/postprocess/trace-vmscan-postprocess.pl
index 6c4e3fde9447..5a5d70029c41 100644
--- a/Documentation/trace/postprocess/trace-vmscan-postprocess.pl
+++ b/Documentation/trace/postprocess/trace-vmscan-postprocess.pl
@@ -575,8 +575,8 @@ sub dump_stats {
# Print out kswapd activity
printf("\n");
- printf("%-" . $max_strlen . "s %8s %10s %8s %8s %8s %8s\n", "Kswapd", "Kswapd", "Order", "Pages", "Pages", "Pages", "Pages");
- printf("%-" . $max_strlen . "s %8s %10s %8s %8s %8s %8s\n", "Instance", "Wakeups", "Re-wakeup", "Scanned", "Rclmed", "Sync-IO", "ASync-IO");
+ printf("%-" . $max_strlen . "s %8s %10s %8s %8s %8s %8s\n", "Kswapd", "Kswapd", "Order", "Pages", "Pages", "Pages", "Pages");
+ printf("%-" . $max_strlen . "s %8s %10s %8s %8s %8s %8s\n", "Instance", "Wakeups", "Re-wakeup", "Scanned", "Rclmed", "Sync-IO", "ASync-IO");
foreach $process_pid (keys %stats) {
if (!$stats{$process_pid}->{MM_VMSCAN_KSWAPD_WAKE}) {
@@ -595,7 +595,7 @@ sub dump_stats {
$total_kswapd_writepage_file_async += $stats{$process_pid}->{MM_VMSCAN_WRITEPAGE_FILE_ASYNC};
$total_kswapd_writepage_anon_async += $stats{$process_pid}->{MM_VMSCAN_WRITEPAGE_ANON_ASYNC};
- printf("%-" . $max_strlen . "s %8d %10d %8u %8u %8i %8u",
+ printf("%-" . $max_strlen . "s %8d %10d %8u %8u %8i %8u ",
$process_pid,
$stats{$process_pid}->{MM_VMSCAN_KSWAPD_WAKE},
$stats{$process_pid}->{HIGH_KSWAPD_REWAKEUP},
--
2.23.0.rc1
^ permalink raw reply related
* Re: [PATCH v4 2/2] hwmon: pmbus: Add Inspur Power System power supply driver
From: Vijay Khemka @ 2019-08-15 18:43 UTC (permalink / raw)
To: John Wang, jdelvare@suse.com, linux@roeck-us.net, corbet@lwn.net,
linux-hwmon@vger.kernel.org, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org, openbmc@lists.ozlabs.org,
duanzhijia01@inspur.com, mine260309@gmail.com, joel@jms.id.au
In-Reply-To: <20190813083412.8668-1-wangzqbj@inspur.com>
On 8/13/19, 1:36 AM, "openbmc on behalf of John Wang" <openbmc-bounces+vijaykhemka=fb.com@lists.ozlabs.org on behalf of wangzqbj@inspur.com> wrote:
Add the driver to monitor Inspur Power System power supplies
with hwmon over pmbus.
This driver adds sysfs attributes for additional power supply data,
including vendor, model, part_number, serial number,
firmware revision, hardware revision, and psu mode(active/standby).
Signed-off-by: John Wang <wangzqbj@inspur.com>
---
v4:
- Remove the additional tabs in the Makefile
- Rebased on 5.3-rc4, not 5.2
v3:
- Sort kconfig/makefile entries alphabetically
- Remove unnecessary initialization
- Use ATTRIBUTE_GROUPS instead of expanding directly
- Use memscan to avoid reimplementation
v2:
- Fix typos in commit message
- Invert Christmas tree
- Configure device with sysfs attrs, not debugfs entries
- Fix errno in fw_version_read, ENODATA to EPROTO
- Change the print format of fw-version
- Use sysfs_streq instead of strcmp("xxx" "\n", "xxx")
- Document sysfs attributes
---
Documentation/hwmon/inspur-ipsps1.rst | 79 +++++++++
drivers/hwmon/pmbus/Kconfig | 9 +
drivers/hwmon/pmbus/Makefile | 1 +
drivers/hwmon/pmbus/inspur-ipsps.c | 226 ++++++++++++++++++++++++++
4 files changed, 315 insertions(+)
create mode 100644 Documentation/hwmon/inspur-ipsps1.rst
create mode 100644 drivers/hwmon/pmbus/inspur-ipsps.c
diff --git a/Documentation/hwmon/inspur-ipsps1.rst b/Documentation/hwmon/inspur-ipsps1.rst
new file mode 100644
index 000000000000..aa19f0ccc8b0
--- /dev/null
+++ b/Documentation/hwmon/inspur-ipsps1.rst
@@ -0,0 +1,79 @@
+Kernel driver inspur-ipsps1
+=======================
+
+Supported chips:
+
+ * Inspur Power System power supply unit
+
+Author: John Wang <wangzqbj@inspur.com>
+
+Description
+-----------
+
+This driver supports Inspur Power System power supplies. This driver
+is a client to the core PMBus driver.
+
+Usage Notes
+-----------
+
+This driver does not auto-detect devices. You will have to instantiate the
+devices explicitly. Please see Documentation/i2c/instantiating-devices for
+details.
+
+Sysfs entries
+-------------
+
+The following attributes are supported:
+
+======================= ======================================================
+curr1_input Measured input current
+curr1_label "iin"
+curr1_max Maximum current
+curr1_max_alarm Current high alarm
+curr2_input Measured output current in mA.
+curr2_label "iout1"
+curr2_crit Critical maximum current
+curr2_crit_alarm Current critical high alarm
+curr2_max Maximum current
+curr2_max_alarm Current high alarm
+
Please align above details.
+fan1_alarm Fan 1 warning.
+fan1_fault Fan 1 fault.
+fan1_input Fan 1 speed in RPM.
+
+in1_alarm Input voltage under-voltage alarm.
+in1_input Measured input voltage in mV.
+in1_label "vin"
+in2_input Measured output voltage in mV.
+in2_label "vout1"
+in2_lcrit Critical minimum output voltage
+in2_lcrit_alarm Output voltage critical low alarm
+in2_max Maximum output voltage
+in2_max_alarm Output voltage high alarm
+in2_min Minimum output voltage
+in2_min_alarm Output voltage low alarm
+
+power1_alarm Input fault or alarm.
+power1_input Measured input power in uW.
+power1_label "pin"
+power1_max Input power limit
+power2_max_alarm Output power high alarm
+power2_max Output power limit
+power2_input Measured output power in uW.
+power2_label "pout"
+
Same alignment issue in description.
+temp[1-3]_input Measured temperature
+temp[1-2]_max Maximum temperature
+temp[1-3]_max_alarm Temperature high alarm
+
+vendor Manufacturer name
+model Product model
+part_number Product part number
+serial_number Product serial number
+fw_version Firmware version
+hw_version Hardware version
+mode Work mode. Can be set to active or
+ standby, when set to standby, PSU will
+ automatically switch between standby
+ and redundancy mode.
+======================= ======================================================
diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index b6588483fae1..d62d69bb7e49 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -46,6 +46,15 @@ config SENSORS_IBM_CFFPS
This driver can also be built as a module. If so, the module will
be called ibm-cffps.
+config SENSORS_INSPUR_IPSPS
+ tristate "INSPUR Power System Power Supply"
+ help
+ If you say yes here you get hardware monitoring support for the INSPUR
+ Power System power supply.
+
+ This driver can also be built as a module. If so, the module will
+ be called inspur-ipsps.
+
config SENSORS_IR35221
tristate "Infineon IR35221"
help
diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
index c950ea9a5d00..03bacfcfd660 100644
--- a/drivers/hwmon/pmbus/Makefile
+++ b/drivers/hwmon/pmbus/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_PMBUS) += pmbus_core.o
obj-$(CONFIG_SENSORS_PMBUS) += pmbus.o
obj-$(CONFIG_SENSORS_ADM1275) += adm1275.o
obj-$(CONFIG_SENSORS_IBM_CFFPS) += ibm-cffps.o
+obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o
obj-$(CONFIG_SENSORS_IR35221) += ir35221.o
obj-$(CONFIG_SENSORS_IR38064) += ir38064.o
obj-$(CONFIG_SENSORS_IRPS5401) += irps5401.o
diff --git a/drivers/hwmon/pmbus/inspur-ipsps.c b/drivers/hwmon/pmbus/inspur-ipsps.c
new file mode 100644
index 000000000000..fa981b881a60
--- /dev/null
+++ b/drivers/hwmon/pmbus/inspur-ipsps.c
@@ -0,0 +1,226 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright 2019 Inspur Corp.
+ */
+
+#include <linux/debugfs.h>
+#include <linux/device.h>
+#include <linux/fs.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/pmbus.h>
+#include <linux/hwmon-sysfs.h>
+
+#include "pmbus.h"
+
+#define IPSPS_REG_VENDOR_ID 0x99
+#define IPSPS_REG_MODEL 0x9A
+#define IPSPS_REG_FW_VERSION 0x9B
+#define IPSPS_REG_PN 0x9C
+#define IPSPS_REG_SN 0x9E
+#define IPSPS_REG_HW_VERSION 0xB0
+#define IPSPS_REG_MODE 0xFC
+
+#define MODE_ACTIVE 0x55
+#define MODE_STANDBY 0x0E
+#define MODE_REDUNDANCY 0x00
+
+#define MODE_ACTIVE_STRING "active"
+#define MODE_STANDBY_STRING "standby"
+#define MODE_REDUNDANCY_STRING "redundancy"
+
+enum ipsps_index {
+ vendor,
+ model,
+ fw_version,
+ part_number,
+ serial_number,
+ hw_version,
+ mode,
+ num_regs,
+};
+
+static const u8 ipsps_regs[num_regs] = {
+ [vendor] = IPSPS_REG_VENDOR_ID,
+ [model] = IPSPS_REG_MODEL,
+ [fw_version] = IPSPS_REG_FW_VERSION,
+ [part_number] = IPSPS_REG_PN,
+ [serial_number] = IPSPS_REG_SN,
+ [hw_version] = IPSPS_REG_HW_VERSION,
+ [mode] = IPSPS_REG_MODE,
+};
+
+static ssize_t ipsps_string_show(struct device *dev,
+ struct device_attribute *devattr,
+ char *buf)
+{
+ u8 reg;
+ int rc;
+ char *p;
+ char data[I2C_SMBUS_BLOCK_MAX + 1];
+ struct i2c_client *client = to_i2c_client(dev->parent);
+ struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+
+ reg = ipsps_regs[attr->index];
+ rc = i2c_smbus_read_block_data(client, reg, data);
+ if (rc < 0)
+ return rc;
+
+ /* filled with printable characters, ending with # */
+ p = memscan(data, '#', rc);
+ *p = '\0';
+
+ return snprintf(buf, PAGE_SIZE, "%s\n", data);
+}
+
+static ssize_t ipsps_fw_version_show(struct device *dev,
+ struct device_attribute *devattr,
+ char *buf)
+{
+ u8 reg;
+ int rc;
+ u8 data[I2C_SMBUS_BLOCK_MAX] = { 0 };
+ struct i2c_client *client = to_i2c_client(dev->parent);
+ struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+
+ reg = ipsps_regs[attr->index];
+ rc = i2c_smbus_read_block_data(client, reg, data);
+ if (rc < 0)
+ return rc;
+
+ if (rc != 6)
+ return -EPROTO;
+
+ return snprintf(buf, PAGE_SIZE, "%u.%02u%u-%u.%02u\n",
+ data[1], data[2]/* < 100 */, data[3]/*< 10*/,
+ data[4], data[5]/* < 100 */);
+}
+
+static ssize_t ipsps_mode_show(struct device *dev,
+ struct device_attribute *devattr, char *buf)
+{
+ u8 reg;
+ int rc;
+ struct i2c_client *client = to_i2c_client(dev->parent);
+ struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+
+ reg = ipsps_regs[attr->index];
+ rc = i2c_smbus_read_byte_data(client, reg);
+ if (rc < 0)
+ return rc;
+
+ switch (rc) {
+ case MODE_ACTIVE:
+ return snprintf(buf, PAGE_SIZE, "[%s] %s %s\n",
+ MODE_ACTIVE_STRING,
+ MODE_STANDBY_STRING, MODE_REDUNDANCY_STRING);
+ case MODE_STANDBY:
+ return snprintf(buf, PAGE_SIZE, "%s [%s] %s\n",
+ MODE_ACTIVE_STRING,
+ MODE_STANDBY_STRING, MODE_REDUNDANCY_STRING);
+ case MODE_REDUNDANCY:
+ return snprintf(buf, PAGE_SIZE, "%s %s [%s]\n",
+ MODE_ACTIVE_STRING,
+ MODE_STANDBY_STRING, MODE_REDUNDANCY_STRING);
+ default:
+ return snprintf(buf, PAGE_SIZE, "unspecified\n");
+ }
+}
+
+static ssize_t ipsps_mode_store(struct device *dev,
+ struct device_attribute *devattr,
+ const char *buf, size_t count)
+{
+ u8 reg;
+ int rc;
+ struct i2c_client *client = to_i2c_client(dev->parent);
+ struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+
+ reg = ipsps_regs[attr->index];
+ if (sysfs_streq(MODE_STANDBY_STRING, buf)) {
+ rc = i2c_smbus_write_byte_data(client, reg,
+ MODE_STANDBY);
+ if (rc < 0)
+ return rc;
+ return count;
+ } else if (sysfs_streq(MODE_ACTIVE_STRING, buf)) {
+ rc = i2c_smbus_write_byte_data(client, reg,
+ MODE_ACTIVE);
+ if (rc < 0)
+ return rc;
+ return count;
+ }
+
+ return -EINVAL;
+}
+
+static SENSOR_DEVICE_ATTR_RO(vendor, ipsps_string, vendor);
+static SENSOR_DEVICE_ATTR_RO(model, ipsps_string, model);
+static SENSOR_DEVICE_ATTR_RO(part_number, ipsps_string, part_number);
+static SENSOR_DEVICE_ATTR_RO(serial_number, ipsps_string, serial_number);
+static SENSOR_DEVICE_ATTR_RO(hw_version, ipsps_string, hw_version);
+static SENSOR_DEVICE_ATTR_RO(fw_version, ipsps_fw_version, fw_version);
+static SENSOR_DEVICE_ATTR_RW(mode, ipsps_mode, mode);
+
+static struct attribute *ipsps_attrs[] = {
+ &sensor_dev_attr_vendor.dev_attr.attr,
+ &sensor_dev_attr_model.dev_attr.attr,
+ &sensor_dev_attr_part_number.dev_attr.attr,
+ &sensor_dev_attr_serial_number.dev_attr.attr,
+ &sensor_dev_attr_hw_version.dev_attr.attr,
+ &sensor_dev_attr_fw_version.dev_attr.attr,
+ &sensor_dev_attr_mode.dev_attr.attr,
+ NULL,
+};
+
+ATTRIBUTE_GROUPS(ipsps);
+
+static struct pmbus_driver_info ipsps_info = {
+ .pages = 1,
+ .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
+ PMBUS_HAVE_IIN | PMBUS_HAVE_POUT | PMBUS_HAVE_PIN |
+ PMBUS_HAVE_FAN12 | PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 |
+ PMBUS_HAVE_TEMP3 | PMBUS_HAVE_STATUS_VOUT |
+ PMBUS_HAVE_STATUS_IOUT | PMBUS_HAVE_STATUS_INPUT |
+ PMBUS_HAVE_STATUS_TEMP | PMBUS_HAVE_STATUS_FAN12,
This can be dynamic read by chip identify function
+ .groups = ipsps_groups,
+};
+
+static struct pmbus_platform_data ipsps_pdata = {
+ .flags = PMBUS_SKIP_STATUS_CHECK,
+};
+
+static int ipsps_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ client->dev.platform_data = &ipsps_pdata;
Allocate memory for this platform data inside tis function rather than having global variable.
+ return pmbus_do_probe(client, id, &ipsps_info);
+}
+
+static const struct i2c_device_id ipsps_id[] = {
+ { "inspur_ipsps1", 0 },
+ {}
+};
+MODULE_DEVICE_TABLE(i2c, ipsps_id);
+
+static const struct of_device_id ipsps_of_match[] = {
+ { .compatible = "inspur,ipsps1" },
+ {}
+};
+MODULE_DEVICE_TABLE(of, ipsps_of_match);
+
+static struct i2c_driver ipsps_driver = {
+ .driver = {
+ .name = "inspur-ipsps",
+ .of_match_table = ipsps_of_match,
+ },
+ .probe = ipsps_probe,
+ .remove = pmbus_do_remove,
+ .id_table = ipsps_id,
+};
+
+module_i2c_driver(ipsps_driver);
+
+MODULE_AUTHOR("John Wang");
+MODULE_DESCRIPTION("PMBus driver for Inspur Power System power supplies");
+MODULE_LICENSE("GPL");
--
2.17.1
^ permalink raw reply
* Re: [PATCH v4 2/2] hwmon: pmbus: Add Inspur Power System power supply driver
From: Guenter Roeck @ 2019-08-15 19:41 UTC (permalink / raw)
To: Vijay Khemka
Cc: John Wang, jdelvare@suse.com, corbet@lwn.net,
linux-hwmon@vger.kernel.org, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org, openbmc@lists.ozlabs.org,
duanzhijia01@inspur.com, mine260309@gmail.com, joel@jms.id.au
In-Reply-To: <70B3A211-2F43-4712-9B92-D407AA3C3934@fb.com>
On Thu, Aug 15, 2019 at 06:43:52PM +0000, Vijay Khemka wrote:
>
>
> On 8/13/19, 1:36 AM, "openbmc on behalf of John Wang" <openbmc-bounces+vijaykhemka=fb.com@lists.ozlabs.org on behalf of wangzqbj@inspur.com> wrote:
>
> Add the driver to monitor Inspur Power System power supplies
> with hwmon over pmbus.
>
> This driver adds sysfs attributes for additional power supply data,
> including vendor, model, part_number, serial number,
> firmware revision, hardware revision, and psu mode(active/standby).
>
> Signed-off-by: John Wang <wangzqbj@inspur.com>
> ---
> v4:
> - Remove the additional tabs in the Makefile
> - Rebased on 5.3-rc4, not 5.2
> v3:
> - Sort kconfig/makefile entries alphabetically
> - Remove unnecessary initialization
> - Use ATTRIBUTE_GROUPS instead of expanding directly
> - Use memscan to avoid reimplementation
> v2:
> - Fix typos in commit message
> - Invert Christmas tree
> - Configure device with sysfs attrs, not debugfs entries
> - Fix errno in fw_version_read, ENODATA to EPROTO
> - Change the print format of fw-version
> - Use sysfs_streq instead of strcmp("xxx" "\n", "xxx")
> - Document sysfs attributes
> ---
> Documentation/hwmon/inspur-ipsps1.rst | 79 +++++++++
> drivers/hwmon/pmbus/Kconfig | 9 +
> drivers/hwmon/pmbus/Makefile | 1 +
> drivers/hwmon/pmbus/inspur-ipsps.c | 226 ++++++++++++++++++++++++++
> 4 files changed, 315 insertions(+)
> create mode 100644 Documentation/hwmon/inspur-ipsps1.rst
> create mode 100644 drivers/hwmon/pmbus/inspur-ipsps.c
>
> diff --git a/Documentation/hwmon/inspur-ipsps1.rst b/Documentation/hwmon/inspur-ipsps1.rst
> new file mode 100644
> index 000000000000..aa19f0ccc8b0
> --- /dev/null
> +++ b/Documentation/hwmon/inspur-ipsps1.rst
> @@ -0,0 +1,79 @@
> +Kernel driver inspur-ipsps1
> +=======================
> +
> +Supported chips:
> +
> + * Inspur Power System power supply unit
> +
> +Author: John Wang <wangzqbj@inspur.com>
> +
> +Description
> +-----------
> +
> +This driver supports Inspur Power System power supplies. This driver
> +is a client to the core PMBus driver.
> +
> +Usage Notes
> +-----------
> +
> +This driver does not auto-detect devices. You will have to instantiate the
> +devices explicitly. Please see Documentation/i2c/instantiating-devices for
> +details.
> +
> +Sysfs entries
> +-------------
> +
> +The following attributes are supported:
> +
> +======================= ======================================================
> +curr1_input Measured input current
> +curr1_label "iin"
> +curr1_max Maximum current
> +curr1_max_alarm Current high alarm
> +curr2_input Measured output current in mA.
> +curr2_label "iout1"
> +curr2_crit Critical maximum current
> +curr2_crit_alarm Current critical high alarm
> +curr2_max Maximum current
> +curr2_max_alarm Current high alarm
> +
> Please align above details.
> +fan1_alarm Fan 1 warning.
> +fan1_fault Fan 1 fault.
> +fan1_input Fan 1 speed in RPM.
> +
> +in1_alarm Input voltage under-voltage alarm.
> +in1_input Measured input voltage in mV.
> +in1_label "vin"
> +in2_input Measured output voltage in mV.
> +in2_label "vout1"
> +in2_lcrit Critical minimum output voltage
> +in2_lcrit_alarm Output voltage critical low alarm
> +in2_max Maximum output voltage
> +in2_max_alarm Output voltage high alarm
> +in2_min Minimum output voltage
> +in2_min_alarm Output voltage low alarm
> +
> +power1_alarm Input fault or alarm.
> +power1_input Measured input power in uW.
> +power1_label "pin"
> +power1_max Input power limit
> +power2_max_alarm Output power high alarm
> +power2_max Output power limit
> +power2_input Measured output power in uW.
> +power2_label "pout"
> +
> Same alignment issue in description.
> +temp[1-3]_input Measured temperature
> +temp[1-2]_max Maximum temperature
> +temp[1-3]_max_alarm Temperature high alarm
> +
> +vendor Manufacturer name
> +model Product model
> +part_number Product part number
> +serial_number Product serial number
> +fw_version Firmware version
> +hw_version Hardware version
> +mode Work mode. Can be set to active or
> + standby, when set to standby, PSU will
> + automatically switch between standby
> + and redundancy mode.
> +======================= ======================================================
> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> index b6588483fae1..d62d69bb7e49 100644
> --- a/drivers/hwmon/pmbus/Kconfig
> +++ b/drivers/hwmon/pmbus/Kconfig
> @@ -46,6 +46,15 @@ config SENSORS_IBM_CFFPS
> This driver can also be built as a module. If so, the module will
> be called ibm-cffps.
>
> +config SENSORS_INSPUR_IPSPS
> + tristate "INSPUR Power System Power Supply"
> + help
> + If you say yes here you get hardware monitoring support for the INSPUR
> + Power System power supply.
> +
> + This driver can also be built as a module. If so, the module will
> + be called inspur-ipsps.
> +
> config SENSORS_IR35221
> tristate "Infineon IR35221"
> help
> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
> index c950ea9a5d00..03bacfcfd660 100644
> --- a/drivers/hwmon/pmbus/Makefile
> +++ b/drivers/hwmon/pmbus/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_PMBUS) += pmbus_core.o
> obj-$(CONFIG_SENSORS_PMBUS) += pmbus.o
> obj-$(CONFIG_SENSORS_ADM1275) += adm1275.o
> obj-$(CONFIG_SENSORS_IBM_CFFPS) += ibm-cffps.o
> +obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o
> obj-$(CONFIG_SENSORS_IR35221) += ir35221.o
> obj-$(CONFIG_SENSORS_IR38064) += ir38064.o
> obj-$(CONFIG_SENSORS_IRPS5401) += irps5401.o
> diff --git a/drivers/hwmon/pmbus/inspur-ipsps.c b/drivers/hwmon/pmbus/inspur-ipsps.c
> new file mode 100644
> index 000000000000..fa981b881a60
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/inspur-ipsps.c
> @@ -0,0 +1,226 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright 2019 Inspur Corp.
> + */
> +
> +#include <linux/debugfs.h>
> +#include <linux/device.h>
> +#include <linux/fs.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/pmbus.h>
> +#include <linux/hwmon-sysfs.h>
> +
> +#include "pmbus.h"
> +
> +#define IPSPS_REG_VENDOR_ID 0x99
> +#define IPSPS_REG_MODEL 0x9A
> +#define IPSPS_REG_FW_VERSION 0x9B
> +#define IPSPS_REG_PN 0x9C
> +#define IPSPS_REG_SN 0x9E
> +#define IPSPS_REG_HW_VERSION 0xB0
> +#define IPSPS_REG_MODE 0xFC
> +
> +#define MODE_ACTIVE 0x55
> +#define MODE_STANDBY 0x0E
> +#define MODE_REDUNDANCY 0x00
> +
> +#define MODE_ACTIVE_STRING "active"
> +#define MODE_STANDBY_STRING "standby"
> +#define MODE_REDUNDANCY_STRING "redundancy"
> +
> +enum ipsps_index {
> + vendor,
> + model,
> + fw_version,
> + part_number,
> + serial_number,
> + hw_version,
> + mode,
> + num_regs,
> +};
> +
> +static const u8 ipsps_regs[num_regs] = {
> + [vendor] = IPSPS_REG_VENDOR_ID,
> + [model] = IPSPS_REG_MODEL,
> + [fw_version] = IPSPS_REG_FW_VERSION,
> + [part_number] = IPSPS_REG_PN,
> + [serial_number] = IPSPS_REG_SN,
> + [hw_version] = IPSPS_REG_HW_VERSION,
> + [mode] = IPSPS_REG_MODE,
> +};
> +
> +static ssize_t ipsps_string_show(struct device *dev,
> + struct device_attribute *devattr,
> + char *buf)
> +{
> + u8 reg;
> + int rc;
> + char *p;
> + char data[I2C_SMBUS_BLOCK_MAX + 1];
> + struct i2c_client *client = to_i2c_client(dev->parent);
> + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +
> + reg = ipsps_regs[attr->index];
> + rc = i2c_smbus_read_block_data(client, reg, data);
> + if (rc < 0)
> + return rc;
> +
> + /* filled with printable characters, ending with # */
> + p = memscan(data, '#', rc);
> + *p = '\0';
> +
> + return snprintf(buf, PAGE_SIZE, "%s\n", data);
> +}
> +
> +static ssize_t ipsps_fw_version_show(struct device *dev,
> + struct device_attribute *devattr,
> + char *buf)
> +{
> + u8 reg;
> + int rc;
> + u8 data[I2C_SMBUS_BLOCK_MAX] = { 0 };
> + struct i2c_client *client = to_i2c_client(dev->parent);
> + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +
> + reg = ipsps_regs[attr->index];
> + rc = i2c_smbus_read_block_data(client, reg, data);
> + if (rc < 0)
> + return rc;
> +
> + if (rc != 6)
> + return -EPROTO;
> +
> + return snprintf(buf, PAGE_SIZE, "%u.%02u%u-%u.%02u\n",
> + data[1], data[2]/* < 100 */, data[3]/*< 10*/,
> + data[4], data[5]/* < 100 */);
> +}
> +
> +static ssize_t ipsps_mode_show(struct device *dev,
> + struct device_attribute *devattr, char *buf)
> +{
> + u8 reg;
> + int rc;
> + struct i2c_client *client = to_i2c_client(dev->parent);
> + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +
> + reg = ipsps_regs[attr->index];
> + rc = i2c_smbus_read_byte_data(client, reg);
> + if (rc < 0)
> + return rc;
> +
> + switch (rc) {
> + case MODE_ACTIVE:
> + return snprintf(buf, PAGE_SIZE, "[%s] %s %s\n",
> + MODE_ACTIVE_STRING,
> + MODE_STANDBY_STRING, MODE_REDUNDANCY_STRING);
> + case MODE_STANDBY:
> + return snprintf(buf, PAGE_SIZE, "%s [%s] %s\n",
> + MODE_ACTIVE_STRING,
> + MODE_STANDBY_STRING, MODE_REDUNDANCY_STRING);
> + case MODE_REDUNDANCY:
> + return snprintf(buf, PAGE_SIZE, "%s %s [%s]\n",
> + MODE_ACTIVE_STRING,
> + MODE_STANDBY_STRING, MODE_REDUNDANCY_STRING);
> + default:
> + return snprintf(buf, PAGE_SIZE, "unspecified\n");
> + }
> +}
> +
> +static ssize_t ipsps_mode_store(struct device *dev,
> + struct device_attribute *devattr,
> + const char *buf, size_t count)
> +{
> + u8 reg;
> + int rc;
> + struct i2c_client *client = to_i2c_client(dev->parent);
> + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +
> + reg = ipsps_regs[attr->index];
> + if (sysfs_streq(MODE_STANDBY_STRING, buf)) {
> + rc = i2c_smbus_write_byte_data(client, reg,
> + MODE_STANDBY);
> + if (rc < 0)
> + return rc;
> + return count;
> + } else if (sysfs_streq(MODE_ACTIVE_STRING, buf)) {
> + rc = i2c_smbus_write_byte_data(client, reg,
> + MODE_ACTIVE);
> + if (rc < 0)
> + return rc;
> + return count;
> + }
> +
> + return -EINVAL;
> +}
> +
> +static SENSOR_DEVICE_ATTR_RO(vendor, ipsps_string, vendor);
> +static SENSOR_DEVICE_ATTR_RO(model, ipsps_string, model);
> +static SENSOR_DEVICE_ATTR_RO(part_number, ipsps_string, part_number);
> +static SENSOR_DEVICE_ATTR_RO(serial_number, ipsps_string, serial_number);
> +static SENSOR_DEVICE_ATTR_RO(hw_version, ipsps_string, hw_version);
> +static SENSOR_DEVICE_ATTR_RO(fw_version, ipsps_fw_version, fw_version);
> +static SENSOR_DEVICE_ATTR_RW(mode, ipsps_mode, mode);
> +
> +static struct attribute *ipsps_attrs[] = {
> + &sensor_dev_attr_vendor.dev_attr.attr,
> + &sensor_dev_attr_model.dev_attr.attr,
> + &sensor_dev_attr_part_number.dev_attr.attr,
> + &sensor_dev_attr_serial_number.dev_attr.attr,
> + &sensor_dev_attr_hw_version.dev_attr.attr,
> + &sensor_dev_attr_fw_version.dev_attr.attr,
> + &sensor_dev_attr_mode.dev_attr.attr,
> + NULL,
> +};
> +
> +ATTRIBUTE_GROUPS(ipsps);
> +
> +static struct pmbus_driver_info ipsps_info = {
> + .pages = 1,
> + .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
> + PMBUS_HAVE_IIN | PMBUS_HAVE_POUT | PMBUS_HAVE_PIN |
> + PMBUS_HAVE_FAN12 | PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 |
> + PMBUS_HAVE_TEMP3 | PMBUS_HAVE_STATUS_VOUT |
> + PMBUS_HAVE_STATUS_IOUT | PMBUS_HAVE_STATUS_INPUT |
> + PMBUS_HAVE_STATUS_TEMP | PMBUS_HAVE_STATUS_FAN12,
> This can be dynamic read by chip identify function
PMBUS_SKIP_STATUS_CHECK weakens auto-detetcion to some degree,
and auto-detection takes time since it needs to poll all registers
to determine if they exist. I don't mind if you insist, but I don't
immediately see the benefits.
> + .groups = ipsps_groups,
> +};
> +
> +static struct pmbus_platform_data ipsps_pdata = {
> + .flags = PMBUS_SKIP_STATUS_CHECK,
> +};
> +
> +static int ipsps_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + client->dev.platform_data = &ipsps_pdata;
> Allocate memory for this platform data inside tis function rather than having global variable.
Does that have any value other than consuming more memory
if there are multiple instances of the driver ?
> + return pmbus_do_probe(client, id, &ipsps_info);
> +}
> +
> +static const struct i2c_device_id ipsps_id[] = {
> + { "inspur_ipsps1", 0 },
> + {}
> +};
> +MODULE_DEVICE_TABLE(i2c, ipsps_id);
> +
> +static const struct of_device_id ipsps_of_match[] = {
> + { .compatible = "inspur,ipsps1" },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, ipsps_of_match);
> +
> +static struct i2c_driver ipsps_driver = {
> + .driver = {
> + .name = "inspur-ipsps",
> + .of_match_table = ipsps_of_match,
> + },
> + .probe = ipsps_probe,
> + .remove = pmbus_do_remove,
> + .id_table = ipsps_id,
> +};
> +
> +module_i2c_driver(ipsps_driver);
> +
> +MODULE_AUTHOR("John Wang");
> +MODULE_DESCRIPTION("PMBus driver for Inspur Power System power supplies");
> +MODULE_LICENSE("GPL");
> --
> 2.17.1
>
>
>
^ permalink raw reply
* Re: [PATCH] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h
From: Thomas Gleixner @ 2019-08-15 20:14 UTC (permalink / raw)
To: Borislav Petkov
Cc: Lendacky, Thomas, linux-kernel@vger.kernel.org,
linux-doc@vger.kernel.org, linux-pm@vger.kernel.org,
x86@kernel.org, Ingo Molnar, Rafael J . Wysocki, Pavel Machek,
Chen Yu, Jonathan Corbet
In-Reply-To: <20190815153447.GH15313@zn.tnic>
On Thu, 15 Aug 2019, Borislav Petkov wrote:
> On Thu, Aug 15, 2019 at 01:47:24PM +0000, Lendacky, Thomas wrote:
> > I think this is a clearer indication that the action has taken place.
>
> Yeah, but what does that bring us? You wanna know this now, while
> testing. Once that whole effort is done, it is a useless printing of
> info which you have in cpuinfo already.
No. Print something useful in dmesg, telling the user that and also why
this has been disabled.
That avoids stupid questions and if they come up nevertheless we can reduce
the answer to LMGT4Y.
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h
From: Andrew Cooper @ 2019-08-15 20:59 UTC (permalink / raw)
To: Lendacky, Thomas, linux-kernel@vger.kernel.org,
linux-doc@vger.kernel.org, linux-pm@vger.kernel.org,
x86@kernel.org
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Rafael J . Wysocki,
Pavel Machek, Chen Yu, Jonathan Corbet
In-Reply-To: <776cb5c2d33e7fd0d2893904724c0e52b394f24a.1565817448.git.thomas.lendacky@amd.com>
On 14/08/2019 22:17, Lendacky, Thomas wrote:
> +static void init_hide_rdrand(struct cpuinfo_x86 *c)
> +{
> + /*
> + * The nordrand option can clear X86_FEATURE_RDRAND, so check for
> + * RDRAND support using the CPUID function directly.
> + */
> + if (!(cpuid_ecx(1) & BIT(30)) || rdrand_force)
> + return;
> +
> + msr_clear_bit(MSR_AMD64_CPUID_FN_00000001, 62);
> + clear_cpu_cap(c, X86_FEATURE_RDRAND);
> + pr_info_once("hiding RDRAND via CPUID\n");
If you're virtualised, the write to MSR_AMD64_CPUID_FN_1 almost
certainly won't take effect, which means userspace will still be able to
see the bit.
Best to leave everything untouched if you can't actually clear the bit.
All you can do is trust that your hypervisor knows what it is doing.
~Andrew
^ permalink raw reply
* Re: [PATCH] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h
From: Thomas Gleixner @ 2019-08-15 21:04 UTC (permalink / raw)
To: Andrew Cooper
Cc: Lendacky, Thomas, linux-kernel@vger.kernel.org,
linux-doc@vger.kernel.org, linux-pm@vger.kernel.org,
x86@kernel.org, Ingo Molnar, Borislav Petkov, Rafael J . Wysocki,
Pavel Machek, Chen Yu, Jonathan Corbet
In-Reply-To: <a24a2c7d-cfab-a049-37e8-7260a9063a7c@citrix.com>
[-- Attachment #1: Type: text/plain, Size: 1048 bytes --]
On Thu, 15 Aug 2019, Andrew Cooper wrote:
> On 14/08/2019 22:17, Lendacky, Thomas wrote:
> > +static void init_hide_rdrand(struct cpuinfo_x86 *c)
> > +{
> > + /*
> > + * The nordrand option can clear X86_FEATURE_RDRAND, so check for
> > + * RDRAND support using the CPUID function directly.
> > + */
> > + if (!(cpuid_ecx(1) & BIT(30)) || rdrand_force)
> > + return;
> > +
> > + msr_clear_bit(MSR_AMD64_CPUID_FN_00000001, 62);
> > + clear_cpu_cap(c, X86_FEATURE_RDRAND);
> > + pr_info_once("hiding RDRAND via CPUID\n");
>
> If you're virtualised, the write to MSR_AMD64_CPUID_FN_1 almost
> certainly won't take effect, which means userspace will still be able to
> see the bit.
>
> Best to leave everything untouched if you can't actually clear the bit.
> All you can do is trust that your hypervisor knows what it is doing.
Well, we can read the CPUID entry again after writing that MSR bit. If it
still says RDRAND is available then we know that the hypervisor did not
allow the write and print something to that effect.
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH] Documentation/admin-guide: Embargoed hardware security issues
From: Greg Kroah-Hartman @ 2019-08-15 21:04 UTC (permalink / raw)
To: Jiri Kosina
Cc: linux-kernel, Jonathan Corbet, security, linux-doc,
Thomas Gleixner, Mauro Carvalho Chehab
In-Reply-To: <20190805151244.GA28296@kroah.com>
On Mon, Aug 05, 2019 at 05:12:44PM +0200, Greg Kroah-Hartman wrote:
> On Sun, Aug 04, 2019 at 02:17:00AM +0200, Jiri Kosina wrote:
> > On Thu, 25 Jul 2019, Greg Kroah-Hartman wrote:
> >
> > > To address the requirements of embargoed hardware issues, like Meltdown,
> > > Spectre, L1TF, etc. it is necessary to define and document a process for
> > > handling embargoed hardware security issues.
> >
> > I don't know what exactly went wrong, but there is a much more up-to-date
> > version of that document (especially when it comes to vendor contacts),
> > which I sent around on Thu, 2 May 2019 20:23:48 +0200 (CEST) already.
> > Please find it below.
>
> Ah, sorry, don't know what happened here, we had too many different
> versions floating around.
>
> I'll take your version, make the edits recommended and send out a new
> one in a few days, thanks!
Looks like your version only had the difference being the list of
ambassadors and a bunch of people who reviewed the document. No
text changes in the document itself, which was good to see we all agreed
on the correct wording :)
thanks,
greg k-h
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox