* 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: [RFC 06/19] ktf: A simple debugfs interface to test results
From: Knut Omang @ 2019-08-15 10:35 UTC (permalink / raw)
To: Greg Kroah-Hartman
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: <20190815084921.GE3512@kroah.com>
On Thu, 2019-08-15 at 10:49 +0200, Greg Kroah-Hartman wrote:
> On Wed, Aug 14, 2019 at 07:17:07PM +0200, Knut Omang wrote:
> > I notice the discussion and your response here:
> > http://linux-kernel.2935.n7.nabble.com/debugfs-and-module-unloading-td865175.html
> > I assume that means that protection against module unload while a debugfs file
> > is open is now safe.
>
> It should be, if you set the *owner field of your file_operations
> properly. Try it and see!
Might be a case for a KTF selftest to play with the timing to increase the chance :)
Wasn't able to make it crash with these simple, short files.
I notice I had set the .owner field correctly myself in that driver
code I referred to, so that's a "copy regression".
> > On older kernels, having this code in place is far better than an unprotected
> > debugfs entry/exit - I have tested it extensively in the past :-)
>
> Yes, it seems to work, but again, it really is racy and will fail.
> Please don't use it.
>
> > 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.
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)
Thanks!
Knut
>
> thanks,
>
> greg k-h
^ permalink raw reply
* Re: [RFC 06/19] ktf: A simple debugfs interface to test results
From: Greg Kroah-Hartman @ 2019-08-15 8:49 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: <a63bea757e02656a38463cc794da7da15273dd16.camel@oracle.com>
On Wed, Aug 14, 2019 at 07:17:07PM +0200, Knut Omang wrote:
> I notice the discussion and your response here:
> http://linux-kernel.2935.n7.nabble.com/debugfs-and-module-unloading-td865175.html
> I assume that means that protection against module unload while a debugfs file
> is open is now safe.
It should be, if you set the *owner field of your file_operations
properly. Try it and see!
> On older kernels, having this code in place is far better than an unprotected
> debugfs entry/exit - I have tested it extensively in the past :-)
Yes, it seems to work, but again, it really is racy and will fail.
Please don't use it.
> 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.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h
From: Borislav Petkov @ 2019-08-15 7:21 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: <776cb5c2d33e7fd0d2893904724c0e52b394f24a.1565817448.git.thomas.lendacky@amd.com>
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
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.
> #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.
> +
> +static void init_hide_rdrand(struct cpuinfo_x86 *c)
clear_rdrand_cpuid_bit()
is what this function does.
> +{
> + /*
> + * 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.
> +}
> +
> +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?
> + 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().
Thx.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply
* Re: [PATCH 06/10] iommu: Remember when default domain type was set on kernel command line
From: Joerg Roedel @ 2019-08-15 7:19 UTC (permalink / raw)
To: Lu Baolu
Cc: corbet, tony.luck, fenghua.yu, tglx, mingo, bp, hpa, x86,
linux-doc, linux-ia64, iommu, linux-kernel, Thomas.Lendacky,
Suravee.Suthikulpanit, Joerg Roedel
In-Reply-To: <754a526e-a6d4-8a3f-0b35-9dd3def5d24b@linux.intel.com>
Hey Lu Baolu,
thanks for your review!
On Thu, Aug 15, 2019 at 01:01:57PM +0800, Lu Baolu wrote:
> > +#define IOMMU_CMD_LINE_DMA_API (1 << 0)
>
> Prefer BIT() micro?
Yes, I'll change that.
> > + iommu_set_cmd_line_dma_api();
>
> IOMMU command line is also set in other places, for example,
> iommu_setup() (arch/x86/kernel/pci-dma.c). Need to call this there as
> well?
You are right, I'll better add a 'bool cmd_line' parameter to the
iommu_set_default_*() functions and tell the IOMMU core this way. That
will also fix iommu=pt/nopt.
Thanks,
Joerg
^ permalink raw reply
* Re: [PATCH 08/10] iommu: Set default domain type at runtime
From: Lu Baolu @ 2019-08-15 6:55 UTC (permalink / raw)
To: Joerg Roedel
Cc: baolu.lu, corbet, tony.luck, fenghua.yu, tglx, mingo, bp, hpa,
x86, linux-doc, linux-ia64, iommu, linux-kernel, Thomas.Lendacky,
Suravee.Suthikulpanit, Joerg Roedel
In-Reply-To: <20190814133841.7095-9-joro@8bytes.org>
Hi,
On 8/14/19 9:38 PM, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
>
> Set the default domain-type at runtime, not at compile-time.
> This keeps default domain type setting in one place when we
> have to change it at runtime.
>
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
> drivers/iommu/iommu.c | 23 +++++++++++++++--------
> 1 file changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 233bc22b487e..96cc7cc8ab21 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -26,11 +26,8 @@
>
> static struct kset *iommu_group_kset;
> static DEFINE_IDA(iommu_group_ida);
> -#ifdef CONFIG_IOMMU_DEFAULT_PASSTHROUGH
> -static unsigned int iommu_def_domain_type = IOMMU_DOMAIN_IDENTITY;
> -#else
> -static unsigned int iommu_def_domain_type = IOMMU_DOMAIN_DMA;
> -#endif
> +
> +static unsigned int iommu_def_domain_type __read_mostly;
> static bool iommu_dma_strict __read_mostly = true;
> static u32 iommu_cmd_line __read_mostly;
>
> @@ -76,7 +73,7 @@ static void iommu_set_cmd_line_dma_api(void)
> iommu_cmd_line |= IOMMU_CMD_LINE_DMA_API;
> }
>
> -static bool __maybe_unused iommu_cmd_line_dma_api(void)
> +static bool iommu_cmd_line_dma_api(void)
> {
> return !!(iommu_cmd_line & IOMMU_CMD_LINE_DMA_API);
> }
> @@ -115,8 +112,18 @@ static const char *iommu_domain_type_str(unsigned int t)
>
> static int __init iommu_subsys_init(void)
> {
> - pr_info("Default domain type: %s\n",
> - iommu_domain_type_str(iommu_def_domain_type));
> + bool cmd_line = iommu_cmd_line_dma_api();
> +
> + if (!cmd_line) {
> + if (IS_ENABLED(CONFIG_IOMMU_DEFAULT_PASSTHROUGH))
> + iommu_set_default_passthrough();
> + else
> + iommu_set_default_translated();
This overrides kernel parameters parsed in iommu_setup(), for example,
iommu=pt won't work anymore.
Best regards,
Lu Baolu
> + }
> +
> + pr_info("Default domain type: %s %s\n",
> + iommu_domain_type_str(iommu_def_domain_type),
> + cmd_line ? "(set via kernel command line)" : "");
>
> return 0;
> }
>
^ permalink raw reply
* Re: [PATCH] Ext4 documentation fixes.
From: Andreas Dilger @ 2019-08-15 6:09 UTC (permalink / raw)
To: Ayush Ranjan
Cc: Theodore Ts'o, Jonathan Corbet, Ext4 Developers List,
linux-doc, Linux Kernel Mailing List
In-Reply-To: <CA+UE=SNWDBGuFpS9Y7g5iurJEJX41c+LMwis3ZGotbJ=DSSaJA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 978 bytes --]
On Aug 14, 2019, at 6:47 PM, Ayush Ranjan <ayushr2@illinois.edu> wrote:
> diff --git a/Documentation/filesystems/ext4/inodes.rst b/Documentation/filesystems/ext4/inodes.rst
> index 6bd35e506..c468a3171 100644
> --- a/Documentation/filesystems/ext4/inodes.rst
> +++ b/Documentation/filesystems/ext4/inodes.rst
> @@ -470,8 +470,8 @@ inode, which allows struct ext4\_inode to grow for a new kernel without
> having to upgrade all of the on-disk inodes. Access to fields beyond
> EXT2\_GOOD\_OLD\_INODE\_SIZE should be verified to be within
> ``i_extra_isize``. By default, ext4 inode records are 256 bytes, and (as
> -of October 2013) the inode structure is 156 bytes
> -(``i_extra_isize = 28``). The extra space between the end of the inode
> +of October 2013) the inode structure is 160 bytes
This should be changed to "as of August 2019", or possibly the date on
which the last field (i_projid) was added, namely "October, 2015".
Cheers, Andreas
[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]
^ permalink raw reply
* Re: [PATCH 07/10] iommu: Print default domain type on boot
From: Lu Baolu @ 2019-08-15 5:04 UTC (permalink / raw)
To: Joerg Roedel
Cc: baolu.lu, corbet, tony.luck, fenghua.yu, tglx, mingo, bp, hpa,
x86, linux-doc, linux-ia64, iommu, linux-kernel, Thomas.Lendacky,
Suravee.Suthikulpanit, Joerg Roedel
In-Reply-To: <20190814133841.7095-8-joro@8bytes.org>
Hi,
On 8/14/19 9:38 PM, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
>
> Introduce a subsys_initcall for IOMMU code and use it to
> print the default domain type at boot.
>
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
> drivers/iommu/iommu.c | 30 +++++++++++++++++++++++++++++-
> 1 file changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index e1feb4061b8b..233bc22b487e 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -93,12 +93,40 @@ struct iommu_group_attribute iommu_group_attr_##_name = \
> static LIST_HEAD(iommu_device_list);
> static DEFINE_SPINLOCK(iommu_device_lock);
>
> +/*
> + * Use a function instead of an array here because the domain-type is a
> + * bit-field, so an array would waste memory.
> + */
> +static const char *iommu_domain_type_str(unsigned int t)
> +{
> + switch (t) {
> + case IOMMU_DOMAIN_BLOCKED:
> + return "Blocked";
> + case IOMMU_DOMAIN_IDENTITY:
> + return "Passthrough";
> + case IOMMU_DOMAIN_UNMANAGED:
> + return "Unmanaged";
> + case IOMMU_DOMAIN_DMA:
> + return "Translated";
> + default:
> + return "Unknown";
> + }
> +}
Run scripts/checkpatch.pl:
ERROR: switch and case should be at the same indent
#28: FILE: drivers/iommu/iommu.c:102:
+ switch (t) {
+ case IOMMU_DOMAIN_BLOCKED:
[...]
+ case IOMMU_DOMAIN_IDENTITY:
[...]
+ case IOMMU_DOMAIN_UNMANAGED:
[...]
+ case IOMMU_DOMAIN_DMA:
[...]
+ default:
Best regards,
Lu Baolu
> +
> +static int __init iommu_subsys_init(void)
> +{
> + pr_info("Default domain type: %s\n",
> + iommu_domain_type_str(iommu_def_domain_type));
> +
> + return 0;
> +}
> +subsys_initcall(iommu_subsys_init);
> +
> int iommu_device_register(struct iommu_device *iommu)
> {
> spin_lock(&iommu_device_lock);
> list_add_tail(&iommu->list, &iommu_device_list);
> spin_unlock(&iommu_device_lock);
> -
> return 0;
> }
>
>
^ permalink raw reply
* Re: [PATCH 06/10] iommu: Remember when default domain type was set on kernel command line
From: Lu Baolu @ 2019-08-15 5:01 UTC (permalink / raw)
To: Joerg Roedel
Cc: baolu.lu, corbet, tony.luck, fenghua.yu, tglx, mingo, bp, hpa,
x86, linux-doc, linux-ia64, iommu, linux-kernel, Thomas.Lendacky,
Suravee.Suthikulpanit, Joerg Roedel
In-Reply-To: <20190814133841.7095-7-joro@8bytes.org>
Hi,
On 8/14/19 9:38 PM, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
>
> Introduce an extensible concept to remember when certain
> configuration settings for the IOMMU code have been set on
> the kernel command line.
>
> This will be used later to prevent overwriting these
> settings with other defaults.
>
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
> drivers/iommu/iommu.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index f187e85a074b..e1feb4061b8b 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -32,6 +32,7 @@ static unsigned int iommu_def_domain_type = IOMMU_DOMAIN_IDENTITY;
> static unsigned int iommu_def_domain_type = IOMMU_DOMAIN_DMA;
> #endif
> static bool iommu_dma_strict __read_mostly = true;
> +static u32 iommu_cmd_line __read_mostly;
>
> struct iommu_group {
> struct kobject kobj;
> @@ -68,6 +69,18 @@ static const char * const iommu_group_resv_type_string[] = {
> [IOMMU_RESV_SW_MSI] = "msi",
> };
>
> +#define IOMMU_CMD_LINE_DMA_API (1 << 0)
Prefer BIT() micro?
> +
> +static void iommu_set_cmd_line_dma_api(void)
> +{
> + iommu_cmd_line |= IOMMU_CMD_LINE_DMA_API;
> +}
> +
> +static bool __maybe_unused iommu_cmd_line_dma_api(void)
> +{
> + return !!(iommu_cmd_line & IOMMU_CMD_LINE_DMA_API);
> +}
> +
> #define IOMMU_GROUP_ATTR(_name, _mode, _show, _store) \
> struct iommu_group_attribute iommu_group_attr_##_name = \
> __ATTR(_name, _mode, _show, _store)
> @@ -165,6 +178,8 @@ static int __init iommu_set_def_domain_type(char *str)
> if (ret)
> return ret;
>
> + iommu_set_cmd_line_dma_api();
IOMMU command line is also set in other places, for example,
iommu_setup() (arch/x86/kernel/pci-dma.c). Need to call this there as
well?
Best regards,
Lu Baolu
> +
> iommu_def_domain_type = pt ? IOMMU_DOMAIN_IDENTITY : IOMMU_DOMAIN_DMA;
> return 0;
> }
>
^ permalink raw reply
* Re: [PATCH 04/10] x86/dma: Get rid of iommu_pass_through
From: Lu Baolu @ 2019-08-15 4:53 UTC (permalink / raw)
To: Joerg Roedel
Cc: baolu.lu, fenghua.yu, tony.luck, linux-ia64, corbet, Joerg Roedel,
x86, linux-doc, linux-kernel, iommu, mingo, bp, Thomas.Lendacky,
hpa, tglx
In-Reply-To: <20190814133841.7095-5-joro@8bytes.org>
Hi,
On 8/14/19 9:38 PM, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
>
> This variable has no users anymore. Remove it and tell the
> IOMMU code via its new functions about requested DMA modes.
>
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
This will also simplify the procedures in iommu_probe_device() on x86
platforms.
Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
> arch/x86/include/asm/iommu.h | 1 -
> arch/x86/kernel/pci-dma.c | 11 +++--------
> 2 files changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/arch/x86/include/asm/iommu.h b/arch/x86/include/asm/iommu.h
> index baedab8ac538..b91623d521d9 100644
> --- a/arch/x86/include/asm/iommu.h
> +++ b/arch/x86/include/asm/iommu.h
> @@ -4,7 +4,6 @@
>
> extern int force_iommu, no_iommu;
> extern int iommu_detected;
> -extern int iommu_pass_through;
>
> /* 10 seconds */
> #define DMAR_OPERATION_TIMEOUT ((cycles_t) tsc_khz*10*1000)
> diff --git a/arch/x86/kernel/pci-dma.c b/arch/x86/kernel/pci-dma.c
> index f62b498b18fb..a6fd479d4a71 100644
> --- a/arch/x86/kernel/pci-dma.c
> +++ b/arch/x86/kernel/pci-dma.c
> @@ -1,6 +1,7 @@
> // SPDX-License-Identifier: GPL-2.0
> #include <linux/dma-direct.h>
> #include <linux/dma-debug.h>
> +#include <linux/iommu.h>
> #include <linux/dmar.h>
> #include <linux/export.h>
> #include <linux/memblock.h>
> @@ -43,12 +44,6 @@ int iommu_detected __read_mostly = 0;
> * It is also possible to disable by default in kernel config, and enable with
> * iommu=nopt at boot time.
> */
> -#ifdef CONFIG_IOMMU_DEFAULT_PASSTHROUGH
> -int iommu_pass_through __read_mostly = 1;
> -#else
> -int iommu_pass_through __read_mostly;
> -#endif
> -
> extern struct iommu_table_entry __iommu_table[], __iommu_table_end[];
>
> void __init pci_iommu_alloc(void)
> @@ -120,9 +115,9 @@ static __init int iommu_setup(char *p)
> swiotlb = 1;
> #endif
> if (!strncmp(p, "pt", 2))
> - iommu_pass_through = 1;
> + iommu_set_default_passthrough();
> if (!strncmp(p, "nopt", 4))
> - iommu_pass_through = 0;
> + iommu_set_default_translated();
>
> gart_parse_options(p);
>
>
^ permalink raw reply
* Re: [PATCH 03/10] iommu/vt-d: Request passthrough mode from IOMMU core
From: Lu Baolu @ 2019-08-15 4:46 UTC (permalink / raw)
To: Joerg Roedel
Cc: baolu.lu, corbet, tony.luck, fenghua.yu, tglx, mingo, bp, hpa,
x86, linux-doc, linux-ia64, iommu, linux-kernel, Thomas.Lendacky,
Suravee.Suthikulpanit, Joerg Roedel
In-Reply-To: <20190814133841.7095-4-joro@8bytes.org>
Hi Joerg,
On 8/14/19 9:38 PM, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
>
> Get rid of the iommu_pass_through variable and request
> passthrough mode via the new iommu core function.
>
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
Looks good to me.
Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
> drivers/iommu/intel-iommu.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index bdaed2da8a55..234bc2b55c59 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -3267,7 +3267,7 @@ static int __init init_dmars(void)
> iommu->flush.flush_iotlb(iommu, 0, 0, 0, DMA_TLB_GLOBAL_FLUSH);
> }
>
> - if (iommu_pass_through)
> + if (iommu_default_passthrough())
> iommu_identity_mapping |= IDENTMAP_ALL;
>
> #ifdef CONFIG_INTEL_IOMMU_BROKEN_GFX_WA
>
^ permalink raw reply
* Re: [PATCH v3 01/12] fpga: dfl: fme: support 512bit data width PR
From: Wu Hao @ 2019-08-15 3:58 UTC (permalink / raw)
To: Scott Wood
Cc: Greg KH, mdf, linux-fpga, linux-kernel, linux-api, linux-doc,
atull, Ananda Ravuri, Xu Yilun
In-Reply-To: <32c46e3de1a6641eb0d5940868f7d8b8a30181d3.camel@redhat.com>
On Wed, Aug 14, 2019 at 11:34:15AM -0500, Scott Wood wrote:
> On Wed, 2019-07-24 at 22:22 +0800, Wu Hao wrote:
> > On Wed, Jul 24, 2019 at 11:35:32AM +0200, Greg KH wrote:
> > > On Tue, Jul 23, 2019 at 12:51:24PM +0800, Wu Hao wrote:
> > > >
> > > > @@ -67,8 +69,43 @@
> > > > #define PR_WAIT_TIMEOUT 8000000
> > > > #define PR_HOST_STATUS_IDLE 0
> > > >
> > > > +#if defined(CONFIG_X86) && defined(CONFIG_AS_AVX512)
> > > > +
> > > > +#include <linux/cpufeature.h>
> > > > +#include <asm/fpu/api.h>
> > > > +
> > > > +static inline int is_cpu_avx512_enabled(void)
> > > > +{
> > > > + return cpu_feature_enabled(X86_FEATURE_AVX512F);
> > > > +}
> > >
> > > That's a very arch specific function, why would a driver ever care about
> > > this?
> >
> > Yes, this is only applied to a specific FPGA solution, which FPGA
> > has been integrated with XEON. Hardware indicates this using register
> > to software. As it's cpu integrated solution, so CPU always has this
> > AVX512 capability. The only check we do, is make sure this is not
> > manually disabled by kernel.
> >
> > With this hardware, software could use AVX512 to accelerate the FPGA
> > partial reconfiguration as mentioned in the patch commit message.
> > It brings performance benifits to people who uses it. This is only one
> > optimization (512 vs 32bit data write to hw) for a specific hardware.
>
> I thought earlier you said that 512 bit accesses were required for this
> particular integrated-only version of the device, and not just an
> optimization?
yes, some optimization implemented in a specific integrated-only version
of hardware, this patch is used to support that particular hardware. This
is also the reason you see code here to check hardware revision in this
patch.
>
> > > > +#else
> > > > +static inline int is_cpu_avx512_enabled(void)
> > > > +{
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static inline void copy512(const void *src, void __iomem *dst)
> > > > +{
> > > > + WARN_ON_ONCE(1);
> > >
> > > Are you trying to get reports from syzbot? :)
> >
> > Oh.. no.. I will remove it. :)
> >
> > Thank you very much!
>
> What's wrong with this? The driver should never call copy512() if
> is_cpu_avx512_enabled() returns 0, and if syzbot can somehow make the driver
> do so, then yes we do want a report.
Yes, you are right, in previous version, it doesn't have avx512 enable check
there, so it's possible to have false reporting, it should be fine after
driver does early check on this during probe. As this patch has been dropped
from main patchset, may rework it later and resubmit. Thanks for the comments.
Hao
>
> -Scott
>
^ permalink raw reply
* Re: Non-random RDRAND Re: [PATCH] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h
From: Pavel Machek @ 2019-08-14 23:38 UTC (permalink / raw)
To: Lendacky, Thomas, tytso, nhorman, security
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>
[-- Attachment #1: Type: text/plain, Size: 1425 bytes --]
On Thu 2019-08-15 01:24:35, 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?
Plus... We trust the RDRAND in some configurations:
random.trust_cpu={on,off}
[KNL] Enable or disable trusting the
use of the CPU's random
number generator (if available) to
fully seed the
kernel's CRNG. Default is controlled by
CONFIG_RANDOM_TRUST_CPU.
so.. does this mean /dev/random was giving non-random values for some
users?
Certainly it means userland users were getting non-random values. That
sounds like something worth CVE and informing affected users?
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Non-random RDRAND Re: [PATCH] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h
From: Pavel Machek @ 2019-08-14 23:24 UTC (permalink / raw)
To: Lendacky, Thomas, tytso, nhorman
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: <776cb5c2d33e7fd0d2893904724c0e52b394f24a.1565817448.git.thomas.lendacky@amd.com>
[-- Attachment #1: Type: text/plain, Size: 1487 bytes --]
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?
>
> + 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
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH v3 1/2] rcu/tree: Add basic support for kfree_rcu batching
From: Paul E. McKenney @ 2019-08-14 23:02 UTC (permalink / raw)
To: Joel Fernandes
Cc: linux-kernel, Rao Shoaib, max.byungchul.park, byungchul.park,
kernel-team, kernel-team, Andrew Morton, Davidlohr Bueso,
Jonathan Corbet, Josh Triplett, Kees Cook, Lai Jiangshan,
linux-doc, Mathieu Desnoyers, Mauro Carvalho Chehab, rcu,
Steven Rostedt, Thomas Gleixner
In-Reply-To: <20190814223413.GB69375@google.com>
On Wed, Aug 14, 2019 at 06:34:13PM -0400, Joel Fernandes wrote:
> On Wed, Aug 14, 2019 at 11:44:29AM -0700, Paul E. McKenney wrote:
> > On Wed, Aug 14, 2019 at 01:22:33PM -0400, Joel Fernandes wrote:
> > > On Wed, Aug 14, 2019 at 10:38:17AM -0400, Joel Fernandes wrote:
> > > > On Tue, Aug 13, 2019 at 12:07:38PM -0700, Paul E. McKenney wrote:
> > > [snip]
> > > > > > - * Queue an RCU callback for lazy invocation after a grace period.
> > > > > > - * This will likely be later named something like "call_rcu_lazy()",
> > > > > > - * but this change will require some way of tagging the lazy RCU
> > > > > > - * callbacks in the list of pending callbacks. Until then, this
> > > > > > - * function may only be called from __kfree_rcu().
> > > > > > + * Maximum number of kfree(s) to batch, if this limit is hit then the batch of
> > > > > > + * kfree(s) is queued for freeing after a grace period, right away.
> > > > > > */
> > > > > > -void kfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
> > > > > > +struct kfree_rcu_cpu {
> > > > > > + /* The rcu_work node for queuing work with queue_rcu_work(). The work
> > > > > > + * is done after a grace period.
> > > > > > + */
> > > > > > + struct rcu_work rcu_work;
> > > > > > +
> > > > > > + /* The list of objects being queued in a batch but are not yet
> > > > > > + * scheduled to be freed.
> > > > > > + */
> > > > > > + struct rcu_head *head;
> > > > > > +
> > > > > > + /* The list of objects that have now left ->head and are queued for
> > > > > > + * freeing after a grace period.
> > > > > > + */
> > > > > > + struct rcu_head *head_free;
> > > > >
> > > > > So this is not yet the one that does multiple batches concurrently
> > > > > awaiting grace periods, correct? Or am I missing something subtle?
> > > >
> > > > Yes, it is not. I honestly, still did not understand that idea. Or how it
> > > > would improve things. May be we can discuss at LPC on pen and paper? But I
> > > > think that can also be a follow-up optimization.
> > >
> > > I got it now. Basically we can benefit a bit more by having another list
> > > (that is have multiple kfree_rcu batches in flight). I will think more about
> > > it - but hopefully we don't need to gate this patch by that.
> >
> > I am willing to take this as a later optimization.
> >
> > > It'll be interesting to see what rcuperf says about such an improvement :)
> >
> > Indeed, no guarantees either way. The reason for hope assumes a busy
> > system where each grace period is immediately followed by another
> > grace period. On such a system, the current setup allows each CPU to
> > make use only of every second grace period for its kfree_rcu() work.
> > The hope would therefore be that this would reduce the memory footprint
> > substantially with no increase in overhead.
>
> Good news! I was able to bring down memory foot print by almost 30% by adding
> another batch. Below is the patch. Thanks for the suggestion!
Nice!
> I can add this as a patch on top of the initial one, for easier review.
Yes, please!
> The memory consumed drops from 300-350MB to 200-250MB. Increasing
> KFREE_N_BATCHES did not cause a reduction in memory, though.
OK, good to know.
Thanx, Paul
> ---8<-----------------------
>
> From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
> Subject: [PATCH] WIP: Multiple batches
>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> ---
> kernel/rcu/tree.c | 58 +++++++++++++++++++++++++++++++++--------------
> 1 file changed, 41 insertions(+), 17 deletions(-)
>
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 1d1847cadea2..a272c893dbdc 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -2596,26 +2596,35 @@ EXPORT_SYMBOL_GPL(call_rcu);
>
> /* Maximum number of jiffies to wait before draining a batch. */
> #define KFREE_DRAIN_JIFFIES (HZ / 50)
> +#define KFREE_N_BATCHES 2
> +
> +struct kfree_rcu_work {
> + /* The rcu_work node for queuing work with queue_rcu_work(). The work
> + * is done after a grace period.
> + */
> + struct rcu_work rcu_work;
> +
> + /* The list of objects that have now left ->head and are queued for
> + * freeing after a grace period.
> + */
> + struct rcu_head *head_free;
> +
> + struct kfree_rcu_cpu *krc;
> +};
> +static DEFINE_PER_CPU(__typeof__(struct kfree_rcu_work)[KFREE_N_BATCHES], krw);
>
> /*
> * Maximum number of kfree(s) to batch, if this limit is hit then the batch of
> * kfree(s) is queued for freeing after a grace period, right away.
> */
> struct kfree_rcu_cpu {
> - /* The rcu_work node for queuing work with queue_rcu_work(). The work
> - * is done after a grace period.
> - */
> - struct rcu_work rcu_work;
>
> /* The list of objects being queued in a batch but are not yet
> * scheduled to be freed.
> */
> struct rcu_head *head;
>
> - /* The list of objects that have now left ->head and are queued for
> - * freeing after a grace period.
> - */
> - struct rcu_head *head_free;
> + struct kfree_rcu_work *krw;
>
> /* Protect concurrent access to this structure. */
> spinlock_t lock;
> @@ -2638,12 +2647,15 @@ static void kfree_rcu_work(struct work_struct *work)
> {
> unsigned long flags;
> struct rcu_head *head, *next;
> - struct kfree_rcu_cpu *krcp = container_of(to_rcu_work(work),
> - struct kfree_rcu_cpu, rcu_work);
> + struct kfree_rcu_work *krw = container_of(to_rcu_work(work),
> + struct kfree_rcu_work, rcu_work);
> + struct kfree_rcu_cpu *krcp;
> +
> + krcp = krw->krc;
>
> spin_lock_irqsave(&krcp->lock, flags);
> - head = krcp->head_free;
> - krcp->head_free = NULL;
> + head = krw->head_free;
> + krw->head_free = NULL;
> spin_unlock_irqrestore(&krcp->lock, flags);
>
> /*
> @@ -2666,19 +2678,30 @@ static void kfree_rcu_work(struct work_struct *work)
> */
> static inline bool queue_kfree_rcu_work(struct kfree_rcu_cpu *krcp)
> {
> + int i = 0;
> + struct kfree_rcu_work *krw = NULL;
> +
> lockdep_assert_held(&krcp->lock);
> + while (i < KFREE_N_BATCHES) {
> + if (!krcp->krw[i].head_free) {
> + krw = &(krcp->krw[i]);
> + break;
> + }
> + i++;
> + }
>
> - /* If a previous RCU batch work is already in progress, we cannot queue
> + /* If both RCU batches are already in progress, we cannot queue
> * another one, just refuse the optimization and it will be retried
> * again in KFREE_DRAIN_JIFFIES time.
> */
> - if (krcp->head_free)
> + if (!krw)
> return false;
>
> - krcp->head_free = krcp->head;
> + krw->head_free = krcp->head;
> + krw->krc = krcp; /* Should need to do only once, optimize later. */
> krcp->head = NULL;
> - INIT_RCU_WORK(&krcp->rcu_work, kfree_rcu_work);
> - queue_rcu_work(system_wq, &krcp->rcu_work);
> + INIT_RCU_WORK(&krw->rcu_work, kfree_rcu_work);
> + queue_rcu_work(system_wq, &krw->rcu_work);
>
> return true;
> }
> @@ -3631,6 +3654,7 @@ static void __init kfree_rcu_batch_init(void)
> struct kfree_rcu_cpu *krcp = per_cpu_ptr(&krc, cpu);
>
> spin_lock_init(&krcp->lock);
> + krcp->krw = &(per_cpu(krw, cpu)[0]);
> INIT_DELAYED_WORK(&krcp->monitor_work, kfree_rcu_monitor);
> }
> }
> --
> 2.23.0.rc1.153.gdeed80330f-goog
>
^ permalink raw reply
* Re: [PATCH 1/3] workqueue: Convert for_each_wq to use built-in list check (v2)
From: Joel Fernandes @ 2019-08-14 22:42 UTC (permalink / raw)
To: Tejun Heo
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
In-Reply-To: <20190814194841.GA588936@devbig004.ftw2.facebook.com>
On Wed, Aug 14, 2019 at 12:48:41PM -0700, Tejun Heo wrote:
> Hello, Joel.
>
> On Sun, Aug 11, 2019 at 06:11:09PM -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.
> >
> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>
> Acked-by: Tejun Heo <tj@kernel.org>
Thanks.
> > #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, \
> > + lock_is_held(&(wq->mutex).dep_map))
>
> Why not lockdep_is_held() tho?
Yes, that's better.
thanks,
- Joel
^ permalink raw reply
* Re: [PATCH v3 1/2] rcu/tree: Add basic support for kfree_rcu batching
From: Joel Fernandes @ 2019-08-14 22:34 UTC (permalink / raw)
To: Paul E. McKenney
Cc: linux-kernel, Rao Shoaib, max.byungchul.park, byungchul.park,
kernel-team, kernel-team, Andrew Morton, Davidlohr Bueso,
Jonathan Corbet, Josh Triplett, Kees Cook, Lai Jiangshan,
linux-doc, Mathieu Desnoyers, Mauro Carvalho Chehab, rcu,
Steven Rostedt, Thomas Gleixner
In-Reply-To: <20190814184429.GV28441@linux.ibm.com>
On Wed, Aug 14, 2019 at 11:44:29AM -0700, Paul E. McKenney wrote:
> On Wed, Aug 14, 2019 at 01:22:33PM -0400, Joel Fernandes wrote:
> > On Wed, Aug 14, 2019 at 10:38:17AM -0400, Joel Fernandes wrote:
> > > On Tue, Aug 13, 2019 at 12:07:38PM -0700, Paul E. McKenney wrote:
> > [snip]
> > > > > - * Queue an RCU callback for lazy invocation after a grace period.
> > > > > - * This will likely be later named something like "call_rcu_lazy()",
> > > > > - * but this change will require some way of tagging the lazy RCU
> > > > > - * callbacks in the list of pending callbacks. Until then, this
> > > > > - * function may only be called from __kfree_rcu().
> > > > > + * Maximum number of kfree(s) to batch, if this limit is hit then the batch of
> > > > > + * kfree(s) is queued for freeing after a grace period, right away.
> > > > > */
> > > > > -void kfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
> > > > > +struct kfree_rcu_cpu {
> > > > > + /* The rcu_work node for queuing work with queue_rcu_work(). The work
> > > > > + * is done after a grace period.
> > > > > + */
> > > > > + struct rcu_work rcu_work;
> > > > > +
> > > > > + /* The list of objects being queued in a batch but are not yet
> > > > > + * scheduled to be freed.
> > > > > + */
> > > > > + struct rcu_head *head;
> > > > > +
> > > > > + /* The list of objects that have now left ->head and are queued for
> > > > > + * freeing after a grace period.
> > > > > + */
> > > > > + struct rcu_head *head_free;
> > > >
> > > > So this is not yet the one that does multiple batches concurrently
> > > > awaiting grace periods, correct? Or am I missing something subtle?
> > >
> > > Yes, it is not. I honestly, still did not understand that idea. Or how it
> > > would improve things. May be we can discuss at LPC on pen and paper? But I
> > > think that can also be a follow-up optimization.
> >
> > I got it now. Basically we can benefit a bit more by having another list
> > (that is have multiple kfree_rcu batches in flight). I will think more about
> > it - but hopefully we don't need to gate this patch by that.
>
> I am willing to take this as a later optimization.
>
> > It'll be interesting to see what rcuperf says about such an improvement :)
>
> Indeed, no guarantees either way. The reason for hope assumes a busy
> system where each grace period is immediately followed by another
> grace period. On such a system, the current setup allows each CPU to
> make use only of every second grace period for its kfree_rcu() work.
> The hope would therefore be that this would reduce the memory footprint
> substantially with no increase in overhead.
Good news! I was able to bring down memory foot print by almost 30% by adding
another batch. Below is the patch. Thanks for the suggestion!
I can add this as a patch on top of the initial one, for easier review.
The memory consumed drops from 300-350MB to 200-250MB. Increasing
KFREE_N_BATCHES did not cause a reduction in memory, though.
---8<-----------------------
From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
Subject: [PATCH] WIP: Multiple batches
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
kernel/rcu/tree.c | 58 +++++++++++++++++++++++++++++++++--------------
1 file changed, 41 insertions(+), 17 deletions(-)
diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 1d1847cadea2..a272c893dbdc 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -2596,26 +2596,35 @@ EXPORT_SYMBOL_GPL(call_rcu);
/* Maximum number of jiffies to wait before draining a batch. */
#define KFREE_DRAIN_JIFFIES (HZ / 50)
+#define KFREE_N_BATCHES 2
+
+struct kfree_rcu_work {
+ /* The rcu_work node for queuing work with queue_rcu_work(). The work
+ * is done after a grace period.
+ */
+ struct rcu_work rcu_work;
+
+ /* The list of objects that have now left ->head and are queued for
+ * freeing after a grace period.
+ */
+ struct rcu_head *head_free;
+
+ struct kfree_rcu_cpu *krc;
+};
+static DEFINE_PER_CPU(__typeof__(struct kfree_rcu_work)[KFREE_N_BATCHES], krw);
/*
* Maximum number of kfree(s) to batch, if this limit is hit then the batch of
* kfree(s) is queued for freeing after a grace period, right away.
*/
struct kfree_rcu_cpu {
- /* The rcu_work node for queuing work with queue_rcu_work(). The work
- * is done after a grace period.
- */
- struct rcu_work rcu_work;
/* The list of objects being queued in a batch but are not yet
* scheduled to be freed.
*/
struct rcu_head *head;
- /* The list of objects that have now left ->head and are queued for
- * freeing after a grace period.
- */
- struct rcu_head *head_free;
+ struct kfree_rcu_work *krw;
/* Protect concurrent access to this structure. */
spinlock_t lock;
@@ -2638,12 +2647,15 @@ static void kfree_rcu_work(struct work_struct *work)
{
unsigned long flags;
struct rcu_head *head, *next;
- struct kfree_rcu_cpu *krcp = container_of(to_rcu_work(work),
- struct kfree_rcu_cpu, rcu_work);
+ struct kfree_rcu_work *krw = container_of(to_rcu_work(work),
+ struct kfree_rcu_work, rcu_work);
+ struct kfree_rcu_cpu *krcp;
+
+ krcp = krw->krc;
spin_lock_irqsave(&krcp->lock, flags);
- head = krcp->head_free;
- krcp->head_free = NULL;
+ head = krw->head_free;
+ krw->head_free = NULL;
spin_unlock_irqrestore(&krcp->lock, flags);
/*
@@ -2666,19 +2678,30 @@ static void kfree_rcu_work(struct work_struct *work)
*/
static inline bool queue_kfree_rcu_work(struct kfree_rcu_cpu *krcp)
{
+ int i = 0;
+ struct kfree_rcu_work *krw = NULL;
+
lockdep_assert_held(&krcp->lock);
+ while (i < KFREE_N_BATCHES) {
+ if (!krcp->krw[i].head_free) {
+ krw = &(krcp->krw[i]);
+ break;
+ }
+ i++;
+ }
- /* If a previous RCU batch work is already in progress, we cannot queue
+ /* If both RCU batches are already in progress, we cannot queue
* another one, just refuse the optimization and it will be retried
* again in KFREE_DRAIN_JIFFIES time.
*/
- if (krcp->head_free)
+ if (!krw)
return false;
- krcp->head_free = krcp->head;
+ krw->head_free = krcp->head;
+ krw->krc = krcp; /* Should need to do only once, optimize later. */
krcp->head = NULL;
- INIT_RCU_WORK(&krcp->rcu_work, kfree_rcu_work);
- queue_rcu_work(system_wq, &krcp->rcu_work);
+ INIT_RCU_WORK(&krw->rcu_work, kfree_rcu_work);
+ queue_rcu_work(system_wq, &krw->rcu_work);
return true;
}
@@ -3631,6 +3654,7 @@ static void __init kfree_rcu_batch_init(void)
struct kfree_rcu_cpu *krcp = per_cpu_ptr(&krc, cpu);
spin_lock_init(&krcp->lock);
+ krcp->krw = &(per_cpu(krw, cpu)[0]);
INIT_DELAYED_WORK(&krcp->monitor_work, kfree_rcu_monitor);
}
}
--
2.23.0.rc1.153.gdeed80330f-goog
^ permalink raw reply related
* [PATCH] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h
From: Lendacky, Thomas @ 2019-08-14 21:17 UTC (permalink / raw)
To: 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
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.
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
#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);
+
+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");
+}
+
+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);
+
+ 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,
+ },
+ {}
+};
+
+typedef int (*pm_cpu_match_t)(const struct x86_cpu_id *);
+static int pm_cpu_check(const struct x86_cpu_id *c)
+{
+ const struct x86_cpu_id *m;
+ int ret = 0;
+
+ m = x86_match_cpu(msr_save_cpu_table);
+ if (m) {
+ pm_cpu_match_t fn;
+
+ fn = (pm_cpu_match_t)m->driver_data;
+ ret = fn(m);
+ }
+
+ return ret;
+}
+
static int pm_check_save_msr(void)
{
dmi_check_system(msr_save_dmi_table);
+ pm_cpu_check(msr_save_cpu_table);
+
return 0;
}
--
2.17.1
^ permalink raw reply related
* [PATCH v5 14/18] compat_ioctl: handle PPPIOCGIDLE for 64-bit time_t
From: Arnd Bergmann @ 2019-08-14 20:54 UTC (permalink / raw)
To: linux-kernel, viro, linux-fsdevel, David S. Miller,
Jonathan Corbet, Paul Mackerras
Cc: Arnd Bergmann, netdev, linux-doc, linux-ppp
In-Reply-To: <20190814204259.120942-1-arnd@arndb.de>
The ppp_idle structure is defined in terms of __kernel_time_t, which is
defined as 'long' on all architectures, and this usage is not affected
by the y2038 problem since it transports a time interval rather than an
absolute time.
However, the ppp user space defines the same structure as time_t, which
may be 64-bit wide on new libc versions even on 32-bit architectures.
It's easy enough to just handle both possible structure layouts on
all architectures, to deal with the possibility that a user space ppp
implementation comes with its own ppp_idle structure definition, as well
as to document the fact that the driver is y2038-safe.
Doing this also avoids the need for a special compat mode translation,
since 32-bit and 64-bit kernels now support the same interfaces. The old
32-bit structure is also available on native 64-bit architectures now,
but this is harmless.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
Documentation/networking/ppp_generic.txt | 2 ++
drivers/net/ppp/ppp_generic.c | 19 ++++++++++----
fs/compat_ioctl.c | 32 ++----------------------
include/uapi/linux/ppp-ioctl.h | 2 ++
include/uapi/linux/ppp_defs.h | 14 +++++++++++
5 files changed, 34 insertions(+), 35 deletions(-)
diff --git a/Documentation/networking/ppp_generic.txt b/Documentation/networking/ppp_generic.txt
index 61daf4b39600..fd563aff5fc9 100644
--- a/Documentation/networking/ppp_generic.txt
+++ b/Documentation/networking/ppp_generic.txt
@@ -378,6 +378,8 @@ an interface unit are:
CONFIG_PPP_FILTER option is enabled, the set of packets which reset
the transmit and receive idle timers is restricted to those which
pass the `active' packet filter.
+ Two versions of this command exist, to deal with user space
+ expecting times as either 32-bit or 64-bit time_t seconds.
* PPPIOCSMAXCID sets the maximum connection-ID parameter (and thus the
number of connection slots) for the TCP header compressor and
diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 2ab67bad6224..6b4e227cb002 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -612,7 +612,8 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
struct ppp_file *pf;
struct ppp *ppp;
int err = -EFAULT, val, val2, i;
- struct ppp_idle idle;
+ struct ppp_idle32 idle32;
+ struct ppp_idle64 idle64;
struct npioctl npi;
int unit, cflags;
struct slcompress *vj;
@@ -735,10 +736,18 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
err = 0;
break;
- case PPPIOCGIDLE:
- idle.xmit_idle = (jiffies - ppp->last_xmit) / HZ;
- idle.recv_idle = (jiffies - ppp->last_recv) / HZ;
- if (copy_to_user(argp, &idle, sizeof(idle)))
+ case PPPIOCGIDLE32:
+ idle32.xmit_idle = (jiffies - ppp->last_xmit) / HZ;
+ idle32.recv_idle = (jiffies - ppp->last_recv) / HZ;
+ if (copy_to_user(argp, &idle32, sizeof(idle32)))
+ break;
+ err = 0;
+ break;
+
+ case PPPIOCGIDLE64:
+ idle64.xmit_idle = (jiffies - ppp->last_xmit) / HZ;
+ idle64.recv_idle = (jiffies - ppp->last_recv) / HZ;
+ if (copy_to_user(argp, &idle64, sizeof(idle64)))
break;
err = 0;
break;
diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c
index 0b5a732d7afd..f97cf698cfdd 100644
--- a/fs/compat_ioctl.c
+++ b/fs/compat_ioctl.c
@@ -99,33 +99,6 @@ static int sg_grt_trans(struct file *file,
}
#endif /* CONFIG_BLOCK */
-struct ppp_idle32 {
- compat_time_t xmit_idle;
- compat_time_t recv_idle;
-};
-#define PPPIOCGIDLE32 _IOR('t', 63, struct ppp_idle32)
-
-static int ppp_gidle(struct file *file, unsigned int cmd,
- struct ppp_idle32 __user *idle32)
-{
- struct ppp_idle __user *idle;
- __kernel_time_t xmit, recv;
- int err;
-
- idle = compat_alloc_user_space(sizeof(*idle));
-
- err = do_ioctl(file, PPPIOCGIDLE, (unsigned long) idle);
-
- if (!err) {
- if (get_user(xmit, &idle->xmit_idle) ||
- get_user(recv, &idle->recv_idle) ||
- put_user(xmit, &idle32->xmit_idle) ||
- put_user(recv, &idle32->recv_idle))
- err = -EFAULT;
- }
- return err;
-}
-
/*
* simple reversible transform to make our table more evenly
* distributed after sorting.
@@ -192,7 +165,8 @@ COMPATIBLE_IOCTL(PPPIOCGDEBUG)
COMPATIBLE_IOCTL(PPPIOCSDEBUG)
/* PPPIOCSPASS is translated */
/* PPPIOCSACTIVE is translated */
-/* PPPIOCGIDLE is translated */
+COMPATIBLE_IOCTL(PPPIOCGIDLE32)
+COMPATIBLE_IOCTL(PPPIOCGIDLE64)
COMPATIBLE_IOCTL(PPPIOCNEWUNIT)
COMPATIBLE_IOCTL(PPPIOCATTACH)
COMPATIBLE_IOCTL(PPPIOCDETACH)
@@ -217,8 +191,6 @@ static long do_ioctl_trans(unsigned int cmd,
void __user *argp = compat_ptr(arg);
switch (cmd) {
- case PPPIOCGIDLE32:
- return ppp_gidle(file, cmd, argp);
#ifdef CONFIG_BLOCK
case SG_GET_REQUEST_TABLE:
return sg_grt_trans(file, cmd, argp);
diff --git a/include/uapi/linux/ppp-ioctl.h b/include/uapi/linux/ppp-ioctl.h
index 88b5f9990320..7bd2a5a75348 100644
--- a/include/uapi/linux/ppp-ioctl.h
+++ b/include/uapi/linux/ppp-ioctl.h
@@ -104,6 +104,8 @@ struct pppol2tp_ioc_stats {
#define PPPIOCGDEBUG _IOR('t', 65, int) /* Read debug level */
#define PPPIOCSDEBUG _IOW('t', 64, int) /* Set debug level */
#define PPPIOCGIDLE _IOR('t', 63, struct ppp_idle) /* get idle time */
+#define PPPIOCGIDLE32 _IOR('t', 63, struct ppp_idle32) /* 32-bit times */
+#define PPPIOCGIDLE64 _IOR('t', 63, struct ppp_idle64) /* 64-bit times */
#define PPPIOCNEWUNIT _IOWR('t', 62, int) /* create new ppp unit */
#define PPPIOCATTACH _IOW('t', 61, int) /* attach to ppp unit */
#define PPPIOCDETACH _IOW('t', 60, int) /* obsolete, do not use */
diff --git a/include/uapi/linux/ppp_defs.h b/include/uapi/linux/ppp_defs.h
index fff51b91b409..0039fa39a358 100644
--- a/include/uapi/linux/ppp_defs.h
+++ b/include/uapi/linux/ppp_defs.h
@@ -142,10 +142,24 @@ struct ppp_comp_stats {
/*
* The following structure records the time in seconds since
* the last NP packet was sent or received.
+ *
+ * Linux implements both 32-bit and 64-bit time_t versions
+ * for compatibility with user space that defines ppp_idle
+ * based on the libc time_t.
*/
struct ppp_idle {
__kernel_time_t xmit_idle; /* time since last NP packet sent */
__kernel_time_t recv_idle; /* time since last NP packet received */
};
+struct ppp_idle32 {
+ __s32 xmit_idle; /* time since last NP packet sent */
+ __s32 recv_idle; /* time since last NP packet received */
+};
+
+struct ppp_idle64 {
+ __s64 xmit_idle; /* time since last NP packet sent */
+ __s64 recv_idle; /* time since last NP packet received */
+};
+
#endif /* _UAPI_PPP_DEFS_H_ */
--
2.20.0
^ permalink raw reply related
* Re: [PATCH 1/3] workqueue: Convert for_each_wq to use built-in list check (v2)
From: Tejun Heo @ 2019-08-14 19:48 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
In-Reply-To: <20190811221111.99401-1-joel@joelfernandes.org>
Hello, Joel.
On Sun, Aug 11, 2019 at 06:11:09PM -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.
>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
Acked-by: Tejun Heo <tj@kernel.org>
> #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, \
> + lock_is_held(&(wq->mutex).dep_map))
Why not lockdep_is_held() tho?
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH v3 1/2] rcu/tree: Add basic support for kfree_rcu batching
From: Paul E. McKenney @ 2019-08-14 18:44 UTC (permalink / raw)
To: Joel Fernandes
Cc: linux-kernel, Rao Shoaib, max.byungchul.park, byungchul.park,
kernel-team, kernel-team, Andrew Morton, Davidlohr Bueso,
Jonathan Corbet, Josh Triplett, Kees Cook, Lai Jiangshan,
linux-doc, Mathieu Desnoyers, Mauro Carvalho Chehab, rcu,
Steven Rostedt, Thomas Gleixner
In-Reply-To: <20190814172233.GA68498@google.com>
On Wed, Aug 14, 2019 at 01:22:33PM -0400, Joel Fernandes wrote:
> On Wed, Aug 14, 2019 at 10:38:17AM -0400, Joel Fernandes wrote:
> > On Tue, Aug 13, 2019 at 12:07:38PM -0700, Paul E. McKenney wrote:
> [snip]
> > > > - * Queue an RCU callback for lazy invocation after a grace period.
> > > > - * This will likely be later named something like "call_rcu_lazy()",
> > > > - * but this change will require some way of tagging the lazy RCU
> > > > - * callbacks in the list of pending callbacks. Until then, this
> > > > - * function may only be called from __kfree_rcu().
> > > > + * Maximum number of kfree(s) to batch, if this limit is hit then the batch of
> > > > + * kfree(s) is queued for freeing after a grace period, right away.
> > > > */
> > > > -void kfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
> > > > +struct kfree_rcu_cpu {
> > > > + /* The rcu_work node for queuing work with queue_rcu_work(). The work
> > > > + * is done after a grace period.
> > > > + */
> > > > + struct rcu_work rcu_work;
> > > > +
> > > > + /* The list of objects being queued in a batch but are not yet
> > > > + * scheduled to be freed.
> > > > + */
> > > > + struct rcu_head *head;
> > > > +
> > > > + /* The list of objects that have now left ->head and are queued for
> > > > + * freeing after a grace period.
> > > > + */
> > > > + struct rcu_head *head_free;
> > >
> > > So this is not yet the one that does multiple batches concurrently
> > > awaiting grace periods, correct? Or am I missing something subtle?
> >
> > Yes, it is not. I honestly, still did not understand that idea. Or how it
> > would improve things. May be we can discuss at LPC on pen and paper? But I
> > think that can also be a follow-up optimization.
>
> I got it now. Basically we can benefit a bit more by having another list
> (that is have multiple kfree_rcu batches in flight). I will think more about
> it - but hopefully we don't need to gate this patch by that.
I am willing to take this as a later optimization.
> It'll be interesting to see what rcuperf says about such an improvement :)
Indeed, no guarantees either way. The reason for hope assumes a busy
system where each grace period is immediately followed by another
grace period. On such a system, the current setup allows each CPU to
make use only of every second grace period for its kfree_rcu() work.
The hope would therefore be that this would reduce the memory footprint
substantially with no increase in overhead.
But no way to know without trying it! ;-)
Thanx, Paul
^ permalink raw reply
* Re: [PATCH v5 2/6] mm/page_idle: Add support for handling swapped PG_Idle pages
From: Michal Hocko @ 2019-08-14 18:36 UTC (permalink / raw)
To: Joel Fernandes
Cc: khlebnikov, linux-kernel, Minchan Kim, Alexey Dobriyan,
Andrew Morton, Borislav Petkov, Brendan Gregg, Catalin Marinas,
Christian Hansen, dancol, fmayer, H. Peter Anvin, Ingo Molnar,
Jonathan Corbet, Kees Cook, kernel-team, linux-api, linux-doc,
linux-fsdevel, linux-mm, Mike Rapoport, namhyung, paulmck,
Robin Murphy, Roman Gushchin, Stephen Rothwell, surenb,
Thomas Gleixner, tkjos, Vladimir Davydov, Vlastimil Babka,
Will Deacon
In-Reply-To: <20190814163203.GB59398@google.com>
On Wed 14-08-19 12:32:03, Joel Fernandes wrote:
> On Wed, Aug 14, 2019 at 10:05:31AM +0200, Michal Hocko wrote:
> > On Tue 13-08-19 11:36:59, Joel Fernandes wrote:
> > > On Tue, Aug 13, 2019 at 05:04:50PM +0200, Michal Hocko wrote:
> > > > On Wed 07-08-19 13:15:55, Joel Fernandes (Google) wrote:
> > > > > Idle page tracking currently does not work well in the following
> > > > > scenario:
> > > > > 1. mark page-A idle which was present at that time.
> > > > > 2. run workload
> > > > > 3. page-A is not touched by workload
> > > > > 4. *sudden* memory pressure happen so finally page A is finally swapped out
> > > > > 5. now see the page A - it appears as if it was accessed (pte unmapped
> > > > > so idle bit not set in output) - but it's incorrect.
> > > > >
> > > > > To fix this, we store the idle information into a new idle bit of the
> > > > > swap PTE during swapping of anonymous pages.
> > > > >
> > > > > Also in the future, madvise extensions will allow a system process
> > > > > manager (like Android's ActivityManager) to swap pages out of a process
> > > > > that it knows will be cold. To an external process like a heap profiler
> > > > > that is doing idle tracking on another process, this procedure will
> > > > > interfere with the idle page tracking similar to the above steps.
> > > >
> > > > This could be solved by checking the !present/swapped out pages
> > > > right? Whoever decided to put the page out to the swap just made it
> > > > idle effectively. So the monitor can make some educated guess for
> > > > tracking. If that is fundamentally not possible then please describe
> > > > why.
> > >
> > > But the monitoring process (profiler) does not have control over the 'whoever
> > > made it effectively idle' process.
> >
> > Why does that matter? Whether it is a global/memcg reclaim or somebody
> > calling MADV_PAGEOUT or whatever it is a decision to make the page not
> > hot. Sure you could argue that a missing idle bit on swap entries might
> > mean that the swap out decision was pre-mature/sub-optimal/wrong but is
> > this the aim of the interface?
> >
> > > As you said it will be a guess, it will not be accurate.
> >
> > Yes and the point I am trying to make is that having some space and not
> > giving a guarantee sounds like a safer option for this interface because
>
> I do see your point of view, but jJust because a future (and possibly not
> going to happen) usecase which you mentioned as pte reclaim, makes you feel
> that userspace may be subject to inaccuracies anyway, doesn't mean we should
> make everything inaccurate.. We already know idle page tracking is not
> completely accurate. But that doesn't mean we miss out on the opportunity to
> make the "non pte-reclaim" usecase inaccurate as well.
Just keep in mind that you will add more burden to future features
because they would have to somehow overcome this user visible behavior
and we will get to the usual question - Is this going to break
something that relies on the idle bit being stable?
> IMO, we should do our best for today, and not hypothesize. How likely is pte
> reclaim and is there a thread to describe that direction?
Not that I am aware of now but with large NVDIMM mapped files I can see
that this will get more and more interesting.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH v13 00/18] kunit: introduce KUnit, the Linux kernel unit testing framework
From: Stephen Boyd @ 2019-08-14 17:23 UTC (permalink / raw)
To: Brendan Higgins, Frank Rowand, Greg KH, Josh Poimboeuf, Kees Cook,
Kieran Bingham, Luis Chamberlain, Masahiro Yamada, Peter Zijlstra,
Rob Herring, Theodore Ts'o, shuah
Cc: devicetree, dri-devel, kunit-dev, open list:DOCUMENTATION,
linux-fsdevel, linux-kbuild, Linux Kernel Mailing List,
open list:KERNEL SELFTEST FRAMEWORK, linux-nvdimm, linux-um,
Sasha Levin, Bird, Timothy, Amir Goldstein, Dan Carpenter,
Daniel Vetter, Jeff Dike, Joel Stanley, Julia Lawall,
Kevin Hilman, Knut Omang, Logan Gunthorpe, Michael Ellerman,
Petr Mladek, Randy Dunlap, Richard Weinberger, David Rientjes,
Steven Rostedt, wfg, Bjorn Helgaas
In-Reply-To: <CAFd5g45NdQEcP0JQpZc3HYYgNZfsBsHL+ByXRK+OupWObwMuqg@mail.gmail.com>
Quoting Brendan Higgins (2019-08-14 03:03:47)
> On Tue, Aug 13, 2019 at 10:52 PM Brendan Higgins
> <brendanhiggins@google.com> wrote:
> >
> > ## TL;DR
> >
> > This revision addresses comments from Stephen and Bjorn Helgaas. Most
> > changes are pretty minor stuff that doesn't affect the API in anyway.
> > One significant change, however, is that I added support for freeing
> > kunit_resource managed resources before the test case is finished via
> > kunit_resource_destroy(). Additionally, Bjorn pointed out that I broke
> > KUnit on certain configurations (like the default one for x86, whoops).
> >
> > Based on Stephen's feedback on the previous change, I think we are
> > pretty close. I am not expecting any significant changes from here on
> > out.
>
> Stephen, it looks like you have just replied with "Reviewed-bys" on
> all the remaining emails that you looked at. Is there anything else
> that we are missing? Or is this ready for Shuah to apply?
>
I think it's good to go! Thanks for the persistence.
^ permalink raw reply
* Re: [PATCH v3 1/2] rcu/tree: Add basic support for kfree_rcu batching
From: Joel Fernandes @ 2019-08-14 17:22 UTC (permalink / raw)
To: Paul E. McKenney
Cc: linux-kernel, Rao Shoaib, max.byungchul.park, byungchul.park,
kernel-team, kernel-team, Andrew Morton, Davidlohr Bueso,
Jonathan Corbet, Josh Triplett, Kees Cook, Lai Jiangshan,
linux-doc, Mathieu Desnoyers, Mauro Carvalho Chehab, rcu,
Steven Rostedt, Thomas Gleixner
In-Reply-To: <20190814143817.GA253999@google.com>
On Wed, Aug 14, 2019 at 10:38:17AM -0400, Joel Fernandes wrote:
> On Tue, Aug 13, 2019 at 12:07:38PM -0700, Paul E. McKenney wrote:
[snip]
> > > - * Queue an RCU callback for lazy invocation after a grace period.
> > > - * This will likely be later named something like "call_rcu_lazy()",
> > > - * but this change will require some way of tagging the lazy RCU
> > > - * callbacks in the list of pending callbacks. Until then, this
> > > - * function may only be called from __kfree_rcu().
> > > + * Maximum number of kfree(s) to batch, if this limit is hit then the batch of
> > > + * kfree(s) is queued for freeing after a grace period, right away.
> > > */
> > > -void kfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
> > > +struct kfree_rcu_cpu {
> > > + /* The rcu_work node for queuing work with queue_rcu_work(). The work
> > > + * is done after a grace period.
> > > + */
> > > + struct rcu_work rcu_work;
> > > +
> > > + /* The list of objects being queued in a batch but are not yet
> > > + * scheduled to be freed.
> > > + */
> > > + struct rcu_head *head;
> > > +
> > > + /* The list of objects that have now left ->head and are queued for
> > > + * freeing after a grace period.
> > > + */
> > > + struct rcu_head *head_free;
> >
> > So this is not yet the one that does multiple batches concurrently
> > awaiting grace periods, correct? Or am I missing something subtle?
>
> Yes, it is not. I honestly, still did not understand that idea. Or how it
> would improve things. May be we can discuss at LPC on pen and paper? But I
> think that can also be a follow-up optimization.
I got it now. Basically we can benefit a bit more by having another list
(that is have multiple kfree_rcu batches in flight). I will think more about
it - but hopefully we don't need to gate this patch by that.
It'll be interesting to see what rcuperf says about such an improvement :)
thanks,
- Joel
^ permalink raw reply
* Re: [PATCH v8 15/27] mm: Handle shadow stack page fault
From: Yu-cheng Yu @ 2019-08-14 17:00 UTC (permalink / raw)
To: Dave Hansen, Andy Lutomirski
Cc: X86 ML, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, LKML,
open list:DOCUMENTATION, Linux-MM, linux-arch, Linux API,
Arnd Bergmann, Balbir Singh, Borislav Petkov, Cyrill Gorcunov,
Dave Hansen, Eugene Syromiatnikov, Florian Weimer, H.J. Lu,
Jann Horn, Jonathan Corbet, Kees Cook, Mike Kravetz, Nadav Amit,
Oleg Nesterov, Pavel Machek, Peter Zijlstra, Randy Dunlap,
Ravi V. Shankar, Vedvyas Shanbhogue, Dave Martin
In-Reply-To: <bf8a6390-97a6-1ab6-90ef-6399437ed38c@intel.com>
On Wed, 2019-08-14 at 09:48 -0700, Dave Hansen wrote:
> On 8/14/19 9:27 AM, Yu-cheng Yu wrote:
> > On Tue, 2019-08-13 at 15:55 -0700, Andy Lutomirski wrote:
> > > On Tue, Aug 13, 2019 at 2:02 PM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
> > > > When a task does fork(), its shadow stack (SHSTK) must be duplicated
> > > > for the child. This patch implements a flow similar to copy-on-write
> > > > of an anonymous page, but for SHSTK.
> > > >
> > > > A SHSTK PTE must be RO and dirty. This dirty bit requirement is used
> > > > to effect the copying. In copy_one_pte(), clear the dirty bit from a
> > > > SHSTK PTE to cause a page fault upon the next SHSTK access. At that
> > > > time, fix the PTE and copy/re-use the page.
> > >
> > > Is using VM_SHSTK and special-casing all of this really better than
> > > using a special mapping or other pseudo-file-backed VMA and putting
> > > all the magic in the vm_operations?
> >
> > A special mapping is cleaner. However, we also need to exclude normal [RO +
> > dirty] pages from shadow stack.
>
> I don't understand what you are saying.
>
> Are you saying that we need this VM_SHSTK flag in order to exclude
> RO+HW-Dirty pages from being created in non-shadow-stack VMAs?
We use VM_SHSTK for page fault handling (the special-casing). If we have a
special mapping, all these become cleaner (but more code). However, we still
need most of the PTE macros (e.g. ptep_set_wrprotect, PAGE_DIRTY_SW, etc.).
Yu-cheng
^ 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