Re: [PATCH v9 00/38] x86: Secure Memory Encryption (AMD)
* Tom Lendacky wrote: > This patch series provides support for AMD's new Secure Memory Encryption > (SME) > feature. I'm wondering, what's the typical performance hit to DRAM access latency when SME is enabled? On that same note, if the performance hit is noticeable I'd expect SME to not be enabled in native kernels typically - but still it looks like a useful hardware feature. Since it's controlled at the page table level, have you considered allowing SME-activated vmas via mmap(), even on kernels that are otherwise not using encrypted DRAM? One would think that putting encryption keys into such encrypted RAM regions would generally improve robustness against various physical space attacks that want to extract keys but don't have full control of the CPU. Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 04/38] x86/CPU/AMD: Add the Secure Memory Encryption CPU feature
On Fri, Jul 7, 2017 at 9:38 AM, Tom Lendacky wrote:
> Update the CPU features to include identifying and reporting on the
> Secure Memory Encryption (SME) feature. SME is identified by CPUID
> 0x801f, but requires BIOS support to enable it (set bit 23 of
> MSR_K8_SYSCFG). Only show the SME feature as available if reported by
> CPUID and enabled by BIOS.
>
> Reviewed-by: Borislav Petkov
> Signed-off-by: Tom Lendacky
> ---
> arch/x86/include/asm/cpufeatures.h |1 +
> arch/x86/include/asm/msr-index.h |2 ++
> arch/x86/kernel/cpu/amd.c | 13 +
> arch/x86/kernel/cpu/scattered.c|1 +
> 4 files changed, 17 insertions(+)
>
> diff --git a/arch/x86/include/asm/cpufeatures.h
> b/arch/x86/include/asm/cpufeatures.h
> index 2701e5f..2b692df 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -196,6 +196,7 @@
>
> #define X86_FEATURE_HW_PSTATE ( 7*32+ 8) /* AMD HW-PState */
> #define X86_FEATURE_PROC_FEEDBACK ( 7*32+ 9) /* AMD ProcFeedbackInterface */
> +#define X86_FEATURE_SME( 7*32+10) /* AMD Secure Memory
> Encryption */
Given that this feature is available only in long mode, this should be
added to disabled-features.h as disabled for 32-bit builds.
> #define X86_FEATURE_INTEL_PPIN ( 7*32+14) /* Intel Processor Inventory
> Number */
> #define X86_FEATURE_INTEL_PT ( 7*32+15) /* Intel Processor Trace */
> diff --git a/arch/x86/include/asm/msr-index.h
> b/arch/x86/include/asm/msr-index.h
> index 18b1623..460ac01 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -352,6 +352,8 @@
> #define MSR_K8_TOP_MEM10xc001001a
> #define MSR_K8_TOP_MEM20xc001001d
> #define MSR_K8_SYSCFG 0xc0010010
> +#define MSR_K8_SYSCFG_MEM_ENCRYPT_BIT 23
> +#define MSR_K8_SYSCFG_MEM_ENCRYPT BIT_ULL(MSR_K8_SYSCFG_MEM_ENCRYPT_BIT)
> #define MSR_K8_INT_PENDING_MSG 0xc0010055
> /* C1E active bits in int pending message */
> #define K8_INTP_C1E_ACTIVE_MASK0x1800
> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
> index bb5abe8..c47ceee 100644
> --- a/arch/x86/kernel/cpu/amd.c
> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -611,6 +611,19 @@ static void early_init_amd(struct cpuinfo_x86 *c)
> */
> if (cpu_has_amd_erratum(c, amd_erratum_400))
> set_cpu_bug(c, X86_BUG_AMD_E400);
> +
> + /*
> +* BIOS support is required for SME. If BIOS has not enabled SME
> +* then don't advertise the feature (set in scattered.c)
> +*/
> + if (cpu_has(c, X86_FEATURE_SME)) {
> + u64 msr;
> +
> + /* Check if SME is enabled */
> + rdmsrl(MSR_K8_SYSCFG, msr);
> + if (!(msr & MSR_K8_SYSCFG_MEM_ENCRYPT))
> + clear_cpu_cap(c, X86_FEATURE_SME);
> + }
This should be conditional on CONFIG_X86_64.
> }
>
> static void init_amd_k8(struct cpuinfo_x86 *c)
> diff --git a/arch/x86/kernel/cpu/scattered.c b/arch/x86/kernel/cpu/scattered.c
> index 23c2350..05459ad 100644
> --- a/arch/x86/kernel/cpu/scattered.c
> +++ b/arch/x86/kernel/cpu/scattered.c
> @@ -31,6 +31,7 @@ struct cpuid_bit {
> { X86_FEATURE_HW_PSTATE,CPUID_EDX, 7, 0x8007, 0 },
> { X86_FEATURE_CPB, CPUID_EDX, 9, 0x8007, 0 },
> { X86_FEATURE_PROC_FEEDBACK,CPUID_EDX, 11, 0x8007, 0 },
> + { X86_FEATURE_SME, CPUID_EAX, 0, 0x801f, 0 },
This should also be conditional. We don't want to set this feature on
32-bit, even if the processor has support.
> { 0, 0, 0, 0, 0 }
> };
--
Brian Gerst
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 07/38] x86/mm: Remove phys_to_virt() usage in ioremap()
On Fri, Jul 7, 2017 at 9:39 AM, Tom Lendacky wrote: > Currently there is a check if the address being mapped is in the ISA > range (is_ISA_range()), and if it is, then phys_to_virt() is used to > perform the mapping. When SME is active, the default is to add pagetable > mappings with the encryption bit set unless specifically overridden. The > resulting pagetable mapping from phys_to_virt() will result in a mapping > that has the encryption bit set. With SME, the use of ioremap() is > intended to generate pagetable mappings that do not have the encryption > bit set through the use of the PAGE_KERNEL_IO protection value. > > Rather than special case the SME scenario, remove the ISA range check and > usage of phys_to_virt() and have ISA range mappings continue through the > remaining ioremap() path. > > Signed-off-by: Tom Lendacky > --- > arch/x86/mm/ioremap.c |7 +-- > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c > index 4c1b5fd..bfc3e2d 100644 > --- a/arch/x86/mm/ioremap.c > +++ b/arch/x86/mm/ioremap.c > @@ -13,6 +13,7 @@ > #include > #include > #include > +#include > > #include > #include > @@ -106,12 +107,6 @@ static void __iomem *__ioremap_caller(resource_size_t > phys_addr, > } > > /* > -* Don't remap the low PCI/ISA area, it's always mapped.. > -*/ > - if (is_ISA_range(phys_addr, last_addr)) > - return (__force void __iomem *)phys_to_virt(phys_addr); > - > - /* > * Don't allow anybody to remap normal RAM that we're using.. > */ > pfn = phys_addr >> PAGE_SHIFT; > Removing this also affects 32-bit, which is more likely to access legacy devices in this range. Put in a check for SME instead (provided you follow my recommendations to not set the SME feature bit on 32-bit even when the processor supports it). -- Brian Gerst -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [v6, 2/3] watchdog: introduce watchdog.open_timeout commandline parameter
On Tue, May 30, 2017 at 10:56:46AM +0200, Rasmus Villemoes wrote:
> The watchdog framework takes care of feeding a hardware watchdog until
> userspace opens /dev/watchdogN. If that never happens for some reason
> (buggy init script, corrupt root filesystem or whatnot) but the kernel
> itself is fine, the machine stays up indefinitely. This patch allows
> setting an upper limit for how long the kernel will take care of the
> watchdog, thus ensuring that the watchdog will eventually reset the
> machine.
>
> This is particularly useful for embedded devices where some fallback
> logic is implemented in the bootloader (e.g., use a different root
> partition, boot from network, ...).
>
> The open timeout is also used as a maximum time for an application to
> re-open /dev/watchdogN after closing it.
>
> A value of 0 (the default) means infinite timeout, preserving the
> current behaviour.
>
> The unit is milliseconds rather than seconds because that covers more
> use cases. For example, one can effectively disable the kernel handling
> by setting the open_timeout to 1 ms. There are also customers with very
> strict requirements that may want to set the open_timeout to something
> like 4500 ms, which combined with a hardware watchdog that must be
> pinged every 250 ms ensures userspace is up no more than 5 seconds after
> the bootloader hands control to the kernel (250 ms until the driver gets
> registered and kernel handling starts, 4500 ms of kernel handling, and
> then up to 250 ms from the last ping until userspace takes over).
>
> Signed-off-by: Rasmus Villemoes
I finally found the time to look into this. It is sufficiently different
to handle_boot_enabled to keep it separate. I am mostly ok with the patch.
One comment below.
> ---
> Documentation/watchdog/watchdog-parameters.txt | 8
> drivers/watchdog/watchdog_dev.c| 26
> +-
> 2 files changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/watchdog/watchdog-parameters.txt
> b/Documentation/watchdog/watchdog-parameters.txt
> index 914518a..8577c27 100644
> --- a/Documentation/watchdog/watchdog-parameters.txt
> +++ b/Documentation/watchdog/watchdog-parameters.txt
> @@ -8,6 +8,14 @@ See Documentation/admin-guide/kernel-parameters.rst for
> information on
> providing kernel parameters for builtin drivers versus loadable
> modules.
>
> +The watchdog core parameter watchdog.open_timeout is the maximum time,
> +in milliseconds, for which the watchdog framework will take care of
> +pinging a hardware watchdog until userspace opens the corresponding
> +/dev/watchdogN device. A value of 0 (the default) means an infinite
> +timeout. Setting this to a non-zero value can be useful to ensure that
> +either userspace comes up properly, or the board gets reset and allows
> +fallback logic in the bootloader to try something else.
> +
>
> -
> acquirewdt:
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index caa4b90..c807067 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -66,6 +66,7 @@ struct watchdog_core_data {
> struct mutex lock;
> unsigned long last_keepalive;
> unsigned long last_hw_keepalive;
> + unsigned long open_deadline;
> struct delayed_work work;
> unsigned long status; /* Internal status bits */
> #define _WDOG_DEV_OPEN 0 /* Opened ? */
> @@ -80,6 +81,21 @@ static struct watchdog_core_data *old_wd_data;
>
> static struct workqueue_struct *watchdog_wq;
>
> +static unsigned open_timeout;
> +module_param(open_timeout, uint, 0644);
MODULE_PARM_DESC is missing. I would also prefer to keep module_param()
and MODULE_PARM_DESC() together with the existing module parameter, ie at
the end of the file.
Thanks,
Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [v6,3/3] watchdog: introduce CONFIG_WATCHDOG_OPEN_TIMEOUT
On Tue, May 30, 2017 at 10:56:47AM +0200, Rasmus Villemoes wrote: > This allows setting a default value for the watchdog.open_timeout > commandline parameter via Kconfig. > > Some BSPs allow remote updating of the kernel image and root file > system, but updating the bootloader requires physical access. Hence, if > one has a firmware update that requires relaxing the > watchdog.open_timeout a little, the value used must be baked into the > kernel image itself and cannot come from the u-boot environment via the > kernel command line. > > Being able to set the initial value in .config doesn't change the fact > that the value on the command line, if present, takes precedence, and is > of course immensely useful for development purposes while one has > console acccess, as well as usable in the cases where one can make a > permanent update of the kernel command line. > > Signed-off-by: Rasmus Villemoes Wim, any thoughts on making this configurable ? I used to be opposed to it, but it does seem to make some sense to me now after thinking about it. Thanks, Guenter > --- > Documentation/watchdog/watchdog-parameters.txt | 3 ++- > drivers/watchdog/Kconfig | 9 + > drivers/watchdog/watchdog_dev.c| 2 +- > 3 files changed, 12 insertions(+), 2 deletions(-) > > diff --git a/Documentation/watchdog/watchdog-parameters.txt > b/Documentation/watchdog/watchdog-parameters.txt > index 8577c27..fa34625 100644 > --- a/Documentation/watchdog/watchdog-parameters.txt > +++ b/Documentation/watchdog/watchdog-parameters.txt > @@ -11,7 +11,8 @@ modules. > The watchdog core parameter watchdog.open_timeout is the maximum time, > in milliseconds, for which the watchdog framework will take care of > pinging a hardware watchdog until userspace opens the corresponding > -/dev/watchdogN device. A value of 0 (the default) means an infinite > +/dev/watchdogN device. The defalt value is > +CONFIG_WATCHDOG_OPEN_TIMEOUT. A value of 0 means an infinite > timeout. Setting this to a non-zero value can be useful to ensure that > either userspace comes up properly, or the board gets reset and allows > fallback logic in the bootloader to try something else. > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > index 8b9049d..11946fb 100644 > --- a/drivers/watchdog/Kconfig > +++ b/drivers/watchdog/Kconfig > @@ -52,6 +52,15 @@ config WATCHDOG_SYSFS > Say Y here if you want to enable watchdog device status read through > sysfs attributes. > > +config WATCHDOG_OPEN_TIMEOUT > + int "Timeout value for opening watchdog device" > + default 0 > + help > + The maximum time, in milliseconds, for which the watchdog > + framework takes care of pinging a hardware watchdog. A value > + of 0 means infinite. The value set here can be overridden by > + the commandline parameter "watchdog.open_timeout". > + > # > # General Watchdog drivers > # > diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c > index c807067..098b9cb 100644 > --- a/drivers/watchdog/watchdog_dev.c > +++ b/drivers/watchdog/watchdog_dev.c > @@ -81,7 +81,7 @@ static struct watchdog_core_data *old_wd_data; > > static struct workqueue_struct *watchdog_wq; > > -static unsigned open_timeout; > +static unsigned open_timeout = CONFIG_WATCHDOG_OPEN_TIMEOUT; > module_param(open_timeout, uint, 0644); > > static bool watchdog_past_open_deadline(struct watchdog_core_data *data) -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
