* [PATCH v2 5/9] driver/core: Convert to use built-in RCU list checking
From: Joel Fernandes (Google) @ 2019-07-12 17:00 UTC (permalink / raw)
To: linux-kernel
Cc: Joel Fernandes (Google), Greg Kroah-Hartman, Alexey Kuznetsov,
Bjorn Helgaas, Borislav Petkov, c0d1n61at3, David S. Miller,
edumazet, Hideaki YOSHIFUJI, H. Peter Anvin, Ingo Molnar,
Jonathan Corbet, Josh Triplett, keescook, kernel-hardening,
kernel-team, Lai Jiangshan, Len Brown, linux-acpi, linux-doc,
linux-pci, linux-pm, Mathieu Desnoyers, neilb, netdev,
Oleg Nesterov, Paul E. McKenney, Pavel Machek, peterz,
Rafael J. Wysocki, Rasmus Villemoes, rcu, Steven Rostedt,
Tejun Heo, Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In-Reply-To: <20190712170024.111093-1-joel@joelfernandes.org>
list_for_each_entry_rcu has built-in RCU and lock checking. Make use of
it in driver core.
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
drivers/base/base.h | 1 +
drivers/base/core.c | 10 ++++++++++
drivers/base/power/runtime.c | 15 ++++++++++-----
3 files changed, 21 insertions(+), 5 deletions(-)
diff --git a/drivers/base/base.h b/drivers/base/base.h
index b405436ee28e..0d32544b6f91 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -165,6 +165,7 @@ static inline int devtmpfs_init(void) { return 0; }
/* Device links support */
extern int device_links_read_lock(void);
extern void device_links_read_unlock(int idx);
+extern int device_links_read_lock_held(void);
extern int device_links_check_suppliers(struct device *dev);
extern void device_links_driver_bound(struct device *dev);
extern void device_links_driver_cleanup(struct device *dev);
diff --git a/drivers/base/core.c b/drivers/base/core.c
index fd7511e04e62..6c5ca9685647 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -68,6 +68,11 @@ void device_links_read_unlock(int idx)
{
srcu_read_unlock(&device_links_srcu, idx);
}
+
+int device_links_read_lock_held(void)
+{
+ return srcu_read_lock_held(&device_links_srcu);
+}
#else /* !CONFIG_SRCU */
static DECLARE_RWSEM(device_links_lock);
@@ -91,6 +96,11 @@ void device_links_read_unlock(int not_used)
{
up_read(&device_links_lock);
}
+
+int device_links_read_lock_held(void)
+{
+ return lock_is_held(&device_links_lock);
+}
#endif /* !CONFIG_SRCU */
/**
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 952a1e7057c7..7a10e8379a70 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -287,7 +287,8 @@ static int rpm_get_suppliers(struct device *dev)
{
struct device_link *link;
- list_for_each_entry_rcu(link, &dev->links.suppliers, c_node) {
+ list_for_each_entry_rcu(link, &dev->links.suppliers, c_node,
+ device_links_read_lock_held()) {
int retval;
if (!(link->flags & DL_FLAG_PM_RUNTIME) ||
@@ -309,7 +310,8 @@ static void rpm_put_suppliers(struct device *dev)
{
struct device_link *link;
- list_for_each_entry_rcu(link, &dev->links.suppliers, c_node) {
+ list_for_each_entry_rcu(link, &dev->links.suppliers, c_node,
+ device_links_read_lock_held()) {
if (READ_ONCE(link->status) == DL_STATE_SUPPLIER_UNBIND)
continue;
@@ -1640,7 +1642,8 @@ void pm_runtime_clean_up_links(struct device *dev)
idx = device_links_read_lock();
- list_for_each_entry_rcu(link, &dev->links.consumers, s_node) {
+ list_for_each_entry_rcu(link, &dev->links.consumers, s_node,
+ device_links_read_lock_held()) {
if (link->flags & DL_FLAG_STATELESS)
continue;
@@ -1662,7 +1665,8 @@ void pm_runtime_get_suppliers(struct device *dev)
idx = device_links_read_lock();
- list_for_each_entry_rcu(link, &dev->links.suppliers, c_node)
+ list_for_each_entry_rcu(link, &dev->links.suppliers, c_node,
+ device_links_read_lock_held())
if (link->flags & DL_FLAG_PM_RUNTIME) {
link->supplier_preactivated = true;
refcount_inc(&link->rpm_active);
@@ -1683,7 +1687,8 @@ void pm_runtime_put_suppliers(struct device *dev)
idx = device_links_read_lock();
- list_for_each_entry_rcu(link, &dev->links.suppliers, c_node)
+ list_for_each_entry_rcu(link, &dev->links.suppliers, c_node,
+ device_links_read_lock_held())
if (link->supplier_preactivated) {
link->supplier_preactivated = false;
if (refcount_dec_not_one(&link->rpm_active))
--
2.22.0.510.g264f2c817a-goog
^ permalink raw reply related
* [PATCH v2 6/9] workqueue: Convert for_each_wq to use built-in list check
From: Joel Fernandes (Google) @ 2019-07-12 17:00 UTC (permalink / raw)
To: linux-kernel
Cc: Joel Fernandes (Google), Alexey Kuznetsov, Bjorn Helgaas,
Borislav Petkov, c0d1n61at3, David S. Miller, edumazet,
Greg Kroah-Hartman, Hideaki YOSHIFUJI, H. Peter Anvin,
Ingo Molnar, Jonathan Corbet, Josh Triplett, keescook,
kernel-hardening, kernel-team, Lai Jiangshan, Len Brown,
linux-acpi, linux-doc, linux-pci, linux-pm, Mathieu Desnoyers,
neilb, netdev, Oleg Nesterov, Paul E. McKenney, Pavel Machek,
peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu, Steven Rostedt,
Tejun Heo, Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In-Reply-To: <20190712170024.111093-1-joel@joelfernandes.org>
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 explictly
checking in the caller.
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
kernel/workqueue.c | 10 ++--------
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 9657315405de..5e88449bdd83 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -363,11 +363,6 @@ static void workqueue_sysfs_unregister(struct workqueue_struct *wq);
!lockdep_is_held(&wq_pool_mutex), \
"RCU or wq_pool_mutex should be held")
-#define assert_rcu_or_wq_mutex(wq) \
- RCU_LOCKDEP_WARN(!rcu_read_lock_held() && \
- !lockdep_is_held(&wq->mutex), \
- "RCU or wq->mutex should be held")
-
#define assert_rcu_or_wq_mutex_or_pool_mutex(wq) \
RCU_LOCKDEP_WARN(!rcu_read_lock_held() && \
!lockdep_is_held(&wq->mutex) && \
@@ -424,9 +419,8 @@ static void workqueue_sysfs_unregister(struct workqueue_struct *wq);
* ignored.
*/
#define for_each_pwq(pwq, wq) \
- list_for_each_entry_rcu((pwq), &(wq)->pwqs, pwqs_node) \
- if (({ assert_rcu_or_wq_mutex(wq); false; })) { } \
- else
+ list_for_each_entry_rcu((pwq), &(wq)->pwqs, pwqs_node, \
+ lock_is_held(&(wq->mutex).dep_map))
#ifdef CONFIG_DEBUG_OBJECTS_WORK
--
2.22.0.510.g264f2c817a-goog
^ permalink raw reply related
* [PATCH v2 7/9] x86/pci: Pass lockdep condition to pcm_mmcfg_list iterator
From: Joel Fernandes (Google) @ 2019-07-12 17:00 UTC (permalink / raw)
To: linux-kernel
Cc: Joel Fernandes (Google), Alexey Kuznetsov, Bjorn Helgaas,
Borislav Petkov, c0d1n61at3, David S. Miller, edumazet,
Greg Kroah-Hartman, Hideaki YOSHIFUJI, H. Peter Anvin,
Ingo Molnar, Jonathan Corbet, Josh Triplett, keescook,
kernel-hardening, kernel-team, Lai Jiangshan, Len Brown,
linux-acpi, linux-doc, linux-pci, linux-pm, Mathieu Desnoyers,
neilb, netdev, Oleg Nesterov, Paul E. McKenney, Pavel Machek,
peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu, Steven Rostedt,
Tejun Heo, Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In-Reply-To: <20190712170024.111093-1-joel@joelfernandes.org>
The pcm_mmcfg_list is traversed with list_for_each_entry_rcu without a
reader-lock held, because the pci_mmcfg_lock is already held. Make this
known to the list macro so that it fixes new lockdep warnings that
trigger due to lockdep checks added to list_for_each_entry_rcu().
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
arch/x86/pci/mmconfig-shared.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c
index 7389db538c30..6fa42e9c4e6f 100644
--- a/arch/x86/pci/mmconfig-shared.c
+++ b/arch/x86/pci/mmconfig-shared.c
@@ -29,6 +29,7 @@
static bool pci_mmcfg_running_state;
static bool pci_mmcfg_arch_init_failed;
static DEFINE_MUTEX(pci_mmcfg_lock);
+#define pci_mmcfg_lock_held() lock_is_held(&(pci_mmcfg_lock).dep_map)
LIST_HEAD(pci_mmcfg_list);
@@ -54,7 +55,7 @@ static void list_add_sorted(struct pci_mmcfg_region *new)
struct pci_mmcfg_region *cfg;
/* keep list sorted by segment and starting bus number */
- list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list) {
+ list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list, pci_mmcfg_lock_held()) {
if (cfg->segment > new->segment ||
(cfg->segment == new->segment &&
cfg->start_bus >= new->start_bus)) {
@@ -118,7 +119,7 @@ struct pci_mmcfg_region *pci_mmconfig_lookup(int segment, int bus)
{
struct pci_mmcfg_region *cfg;
- list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list)
+ list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list, pci_mmcfg_lock_held())
if (cfg->segment == segment &&
cfg->start_bus <= bus && bus <= cfg->end_bus)
return cfg;
--
2.22.0.510.g264f2c817a-goog
^ permalink raw reply related
* [PATCH v2 9/9] doc: Update documentation about list_for_each_entry_rcu
From: Joel Fernandes (Google) @ 2019-07-12 17:00 UTC (permalink / raw)
To: linux-kernel
Cc: Joel Fernandes (Google), Alexey Kuznetsov, Bjorn Helgaas,
Borislav Petkov, c0d1n61at3, David S. Miller, edumazet,
Greg Kroah-Hartman, Hideaki YOSHIFUJI, H. Peter Anvin,
Ingo Molnar, Jonathan Corbet, Josh Triplett, keescook,
kernel-hardening, kernel-team, Lai Jiangshan, Len Brown,
linux-acpi, linux-doc, linux-pci, linux-pm, Mathieu Desnoyers,
neilb, netdev, Oleg Nesterov, Paul E. McKenney, Pavel Machek,
peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu, Steven Rostedt,
Tejun Heo, Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In-Reply-To: <20190712170024.111093-1-joel@joelfernandes.org>
This patch updates the documentation with information about
usage of lockdep with list_for_each_entry_rcu().
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
Documentation/RCU/lockdep.txt | 15 +++++++++++----
Documentation/RCU/whatisRCU.txt | 9 ++++++++-
2 files changed, 19 insertions(+), 5 deletions(-)
diff --git a/Documentation/RCU/lockdep.txt b/Documentation/RCU/lockdep.txt
index da51d3068850..3d967df3a801 100644
--- a/Documentation/RCU/lockdep.txt
+++ b/Documentation/RCU/lockdep.txt
@@ -96,7 +96,14 @@ other flavors of rcu_dereference(). On the other hand, it is illegal
to use rcu_dereference_protected() if either the RCU-protected pointer
or the RCU-protected data that it points to can change concurrently.
-There are currently only "universal" versions of the rcu_assign_pointer()
-and RCU list-/tree-traversal primitives, which do not (yet) check for
-being in an RCU read-side critical section. In the future, separate
-versions of these primitives might be created.
+Similar to rcu_dereference_protected, The RCU list and hlist traversal
+primitives also check for whether there are called from within a reader
+section. However, an optional lockdep expression can be passed to them as
+the last argument in case they are called under other non-RCU protection.
+
+For example, the workqueue for_each_pwq() macro is implemented as follows.
+It is safe to call for_each_pwq() outside a reader section but under protection
+of wq->mutex:
+#define for_each_pwq(pwq, wq)
+ list_for_each_entry_rcu((pwq), &(wq)->pwqs, pwqs_node,
+ lock_is_held(&(wq->mutex).dep_map))
diff --git a/Documentation/RCU/whatisRCU.txt b/Documentation/RCU/whatisRCU.txt
index 981651a8b65d..a08c03735963 100644
--- a/Documentation/RCU/whatisRCU.txt
+++ b/Documentation/RCU/whatisRCU.txt
@@ -290,7 +290,7 @@ rcu_dereference()
at any time, including immediately after the rcu_dereference().
And, again like rcu_assign_pointer(), rcu_dereference() is
typically used indirectly, via the _rcu list-manipulation
- primitives, such as list_for_each_entry_rcu().
+ primitives, such as list_for_each_entry_rcu() [2].
[1] The variant rcu_dereference_protected() can be used outside
of an RCU read-side critical section as long as the usage is
@@ -305,6 +305,13 @@ rcu_dereference()
a lockdep splat is emitted. See RCU/Design/Requirements/Requirements.html
and the API's code comments for more details and example usage.
+ [2] In case the list_for_each_entry_rcu() primitive is intended
+ to be used outside of an RCU reader section such as when
+ protected by a lock, then an additional lockdep expression can be
+ passed as the last argument to it so that RCU lockdep checking code
+ knows that the dereference of the list pointers are safe. If the
+ indicated protection is not provided, a lockdep splat is emitted.
+
The following diagram shows how each API communicates among the
reader, updater, and reclaimer.
--
2.22.0.510.g264f2c817a-goog
^ permalink raw reply related
* [PATCH v2 8/9] acpi: Use built-in RCU list checking for acpi_ioremaps list
From: Joel Fernandes (Google) @ 2019-07-12 17:00 UTC (permalink / raw)
To: linux-kernel
Cc: Joel Fernandes (Google), Alexey Kuznetsov, Bjorn Helgaas,
Borislav Petkov, c0d1n61at3, David S. Miller, edumazet,
Greg Kroah-Hartman, Hideaki YOSHIFUJI, H. Peter Anvin,
Ingo Molnar, Jonathan Corbet, Josh Triplett, keescook,
kernel-hardening, kernel-team, Lai Jiangshan, Len Brown,
linux-acpi, linux-doc, linux-pci, linux-pm, Mathieu Desnoyers,
neilb, netdev, Oleg Nesterov, Paul E. McKenney, Pavel Machek,
peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu, Steven Rostedt,
Tejun Heo, Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In-Reply-To: <20190712170024.111093-1-joel@joelfernandes.org>
list_for_each_entry_rcu has built-in RCU and lock checking. Make use of
it for acpi_ioremaps list traversal.
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
drivers/acpi/osl.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index f29e427d0d1d..c8b5d712c7ae 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -28,6 +28,7 @@
#include <linux/slab.h>
#include <linux/mm.h>
#include <linux/highmem.h>
+#include <linux/lockdep.h>
#include <linux/pci.h>
#include <linux/interrupt.h>
#include <linux/kmod.h>
@@ -94,6 +95,7 @@ struct acpi_ioremap {
static LIST_HEAD(acpi_ioremaps);
static DEFINE_MUTEX(acpi_ioremap_lock);
+#define acpi_ioremap_lock_held() lock_is_held(&acpi_ioremap_lock.dep_map)
static void __init acpi_request_region (struct acpi_generic_address *gas,
unsigned int length, char *desc)
@@ -220,7 +222,7 @@ acpi_map_lookup(acpi_physical_address phys, acpi_size size)
{
struct acpi_ioremap *map;
- list_for_each_entry_rcu(map, &acpi_ioremaps, list)
+ list_for_each_entry_rcu(map, &acpi_ioremaps, list, acpi_ioremap_lock_held())
if (map->phys <= phys &&
phys + size <= map->phys + map->size)
return map;
@@ -263,7 +265,7 @@ acpi_map_lookup_virt(void __iomem *virt, acpi_size size)
{
struct acpi_ioremap *map;
- list_for_each_entry_rcu(map, &acpi_ioremaps, list)
+ list_for_each_entry_rcu(map, &acpi_ioremaps, list, acpi_ioremap_lock_held())
if (map->virt <= virt &&
virt + size <= map->virt + map->size)
return map;
--
2.22.0.510.g264f2c817a-goog
^ permalink raw reply related
* [PATCH v2 2/9] rcu: Add support for consolidated-RCU reader checking
From: Joel Fernandes (Google) @ 2019-07-12 17:00 UTC (permalink / raw)
To: linux-kernel
Cc: Joel Fernandes (Google), Alexey Kuznetsov, Bjorn Helgaas,
Borislav Petkov, c0d1n61at3, David S. Miller, edumazet,
Greg Kroah-Hartman, Hideaki YOSHIFUJI, H. Peter Anvin,
Ingo Molnar, Jonathan Corbet, Josh Triplett, keescook,
kernel-hardening, kernel-team, Lai Jiangshan, Len Brown,
linux-acpi, linux-doc, linux-pci, linux-pm, Mathieu Desnoyers,
neilb, netdev, Oleg Nesterov, Paul E. McKenney, Pavel Machek,
peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu, Steven Rostedt,
Tejun Heo, Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In-Reply-To: <20190712170024.111093-1-joel@joelfernandes.org>
This patch adds support for checking RCU reader sections in list
traversal macros. Optionally, if the list macro is called under SRCU or
other lock/mutex protection, then appropriate lockdep expressions can be
passed to make the checks pass.
Existing list_for_each_entry_rcu() invocations don't need to pass the
optional fourth argument (cond) unless they are under some non-RCU
protection and needs to make lockdep check pass.
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
include/linux/rculist.h | 28 +++++++++++++++++++++++-----
include/linux/rcupdate.h | 7 +++++++
kernel/rcu/Kconfig.debug | 11 +++++++++++
kernel/rcu/update.c | 14 ++++++++++++++
4 files changed, 55 insertions(+), 5 deletions(-)
diff --git a/include/linux/rculist.h b/include/linux/rculist.h
index e91ec9ddcd30..1048160625bb 100644
--- a/include/linux/rculist.h
+++ b/include/linux/rculist.h
@@ -40,6 +40,20 @@ static inline void INIT_LIST_HEAD_RCU(struct list_head *list)
*/
#define list_next_rcu(list) (*((struct list_head __rcu **)(&(list)->next)))
+/*
+ * Check during list traversal that we are within an RCU reader
+ */
+
+#ifdef CONFIG_PROVE_RCU_LIST
+#define __list_check_rcu(dummy, cond, ...) \
+ ({ \
+ RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \
+ "RCU-list traversed in non-reader section!"); \
+ })
+#else
+#define __list_check_rcu(dummy, cond, ...) ({})
+#endif
+
/*
* Insert a new entry between two known consecutive entries.
*
@@ -343,14 +357,16 @@ static inline void list_splice_tail_init_rcu(struct list_head *list,
* @pos: the type * to use as a loop cursor.
* @head: the head for your list.
* @member: the name of the list_head within the struct.
+ * @cond: optional lockdep expression if called from non-RCU protection.
*
* This list-traversal primitive may safely run concurrently with
* the _rcu list-mutation primitives such as list_add_rcu()
* as long as the traversal is guarded by rcu_read_lock().
*/
-#define list_for_each_entry_rcu(pos, head, member) \
- for (pos = list_entry_rcu((head)->next, typeof(*pos), member); \
- &pos->member != (head); \
+#define list_for_each_entry_rcu(pos, head, member, cond...) \
+ for (__list_check_rcu(dummy, ## cond, 0), \
+ pos = list_entry_rcu((head)->next, typeof(*pos), member); \
+ &pos->member != (head); \
pos = list_entry_rcu(pos->member.next, typeof(*pos), member))
/**
@@ -616,13 +632,15 @@ static inline void hlist_add_behind_rcu(struct hlist_node *n,
* @pos: the type * to use as a loop cursor.
* @head: the head for your list.
* @member: the name of the hlist_node within the struct.
+ * @cond: optional lockdep expression if called from non-RCU protection.
*
* This list-traversal primitive may safely run concurrently with
* the _rcu list-mutation primitives such as hlist_add_head_rcu()
* as long as the traversal is guarded by rcu_read_lock().
*/
-#define hlist_for_each_entry_rcu(pos, head, member) \
- for (pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\
+#define hlist_for_each_entry_rcu(pos, head, member, cond...) \
+ for (__list_check_rcu(dummy, ## cond, 0), \
+ pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\
typeof(*(pos)), member); \
pos; \
pos = hlist_entry_safe(rcu_dereference_raw(hlist_next_rcu(\
diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 922bb6848813..712b464ab960 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -223,6 +223,7 @@ int debug_lockdep_rcu_enabled(void);
int rcu_read_lock_held(void);
int rcu_read_lock_bh_held(void);
int rcu_read_lock_sched_held(void);
+int rcu_read_lock_any_held(void);
#else /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */
@@ -243,6 +244,12 @@ static inline int rcu_read_lock_sched_held(void)
{
return !preemptible();
}
+
+static inline int rcu_read_lock_any_held(void)
+{
+ return !preemptible();
+}
+
#endif /* #else #ifdef CONFIG_DEBUG_LOCK_ALLOC */
#ifdef CONFIG_PROVE_RCU
diff --git a/kernel/rcu/Kconfig.debug b/kernel/rcu/Kconfig.debug
index 0ec7d1d33a14..b20d0e2903d1 100644
--- a/kernel/rcu/Kconfig.debug
+++ b/kernel/rcu/Kconfig.debug
@@ -7,6 +7,17 @@ menu "RCU Debugging"
config PROVE_RCU
def_bool PROVE_LOCKING
+config PROVE_RCU_LIST
+ bool "RCU list lockdep debugging"
+ depends on PROVE_RCU
+ default n
+ help
+ Enable RCU lockdep checking for list usages. By default it is
+ turned off since there are several list RCU users that still
+ need to be converted to pass a lockdep expression. To prevent
+ false-positive splats, we keep it default disabled but once all
+ users are converted, we can remove this config option.
+
config TORTURE_TEST
tristate
default n
diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
index bb961cd89e76..0cc7be0fb6b5 100644
--- a/kernel/rcu/update.c
+++ b/kernel/rcu/update.c
@@ -294,6 +294,20 @@ int rcu_read_lock_bh_held(void)
}
EXPORT_SYMBOL_GPL(rcu_read_lock_bh_held);
+int rcu_read_lock_any_held(void)
+{
+ if (!debug_lockdep_rcu_enabled())
+ return 1;
+ if (!rcu_is_watching())
+ return 0;
+ if (!rcu_lockdep_current_cpu_online())
+ return 0;
+ if (lock_is_held(&rcu_lock_map) || lock_is_held(&rcu_sched_lock_map))
+ return 1;
+ return !preemptible();
+}
+EXPORT_SYMBOL_GPL(rcu_read_lock_any_held);
+
#endif /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */
/**
--
2.22.0.510.g264f2c817a-goog
^ permalink raw reply related
* [PATCH v2 0/9] Harden list_for_each_entry_rcu() and family
From: Joel Fernandes (Google) @ 2019-07-12 17:00 UTC (permalink / raw)
To: linux-kernel
Cc: Joel Fernandes (Google), Alexey Kuznetsov, Bjorn Helgaas,
Borislav Petkov, c0d1n61at3, David S. Miller, edumazet,
Greg Kroah-Hartman, Hideaki YOSHIFUJI, H. Peter Anvin,
Ingo Molnar, Jonathan Corbet, Josh Triplett, keescook,
kernel-hardening, kernel-team, Lai Jiangshan, Len Brown,
linux-acpi, linux-doc, linux-pci, linux-pm, Mathieu Desnoyers,
neilb, netdev, Oleg Nesterov, Paul E. McKenney, Pavel Machek,
peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu, Steven Rostedt,
Tejun Heo, Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
Hi,
This series aims to provide lockdep checking to RCU list macros for additional
kernel hardening.
RCU has a number of primitives for "consumption" of an RCU protected pointer.
Most of the time, these consumers make sure that such accesses are under a RCU
reader-section (such as rcu_dereference{,sched,bh} or under a lock, such as
with rcu_dereference_protected()).
However, there are other ways to consume RCU pointers, such as by
list_for_each_entry_rcu or hlist_for_each_enry_rcu. Unlike the rcu_dereference
family, these consumers do no lockdep checking at all. And with the growing
number of RCU list uses (1000+), it is possible for bugs to creep in and go
unnoticed which lockdep checks can catch.
Since RCU consolidation efforts last year, the different traditional RCU
flavors (preempt, bh, sched) are all consolidated. In other words, any of these
flavors can cause a reader section to occur and all of them must cease before
the reader section is considered to be unlocked. Thanks to this, we can
generically check if we are in an RCU reader. This is what patch 1 does. Note
that the list_for_each_entry_rcu and family are different from the
rcu_dereference family in that, there is no _bh or _sched version of this
macro. They are used under many different RCU reader flavors, and also SRCU.
Patch 1 adds a new internal function rcu_read_lock_any_held() which checks
if any reader section is active at all, when these macros are called. If no
reader section exists, then the optional fourth argument to
list_for_each_entry_rcu() can be a lockdep expression which is evaluated
(similar to how rcu_dereference_check() works). If no lockdep expression is
passed, and we are not in a reader, then a splat occurs. Just take off the
lockdep expression after applying the patches, by using the following diff and
see what happens:
+++ b/arch/x86/pci/mmconfig-shared.c
@@ -55,7 +55,7 @@ static void list_add_sorted(struct pci_mmcfg_region *new)
struct pci_mmcfg_region *cfg;
/* keep list sorted by segment and starting bus number */
- list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list, pci_mmcfg_lock_held()) {
+ list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list) {
The optional argument trick to list_for_each_entry_rcu() can also be used in
the future to possibly remove rcu_dereference_{,bh,sched}_protected() API and
we can pass an optional lockdep expression to rcu_dereference() itself. Thus
eliminating 3 more RCU APIs.
Note that some list macro wrappers already do their own lockdep checking in the
caller side. These can be eliminated in favor of the built-in lockdep checking
in the list macro that this series adds. For example, workqueue code has a
assert_rcu_or_wq_mutex() function which is called in for_each_wq(). This
series replaces that in favor of the built-in check.
Also in the future, we can extend these checks to list_entry_rcu() and other
list macros as well, if needed.
Please note that I have kept this option default-disabled under a new config:
CONFIG_PROVE_RCU_LIST. This is so that until all users are converted to pass
the optional argument, we should keep the check disabled. There are about a
1000 or so users and it is not possible to pass in the optional lockdep
expression in a single series since it is done on a case-by-case basis. I did
convert a few users in this series itself.
v1->v2: Have assert_rcu_or_wq_mutex deleted (Daniel Jordan)
Simplify rcu_read_lock_any_held() (Peter Zijlstra)
Simplified rcu-sync logic (Oleg Nesterov)
Updated documentation and rculist comments.
Added GregKH ack.
RFC->v1:
Simplify list checking macro (Rasmus Villemoes)
Joel Fernandes (Google) (9):
rcu/update: Remove useless check for debug_locks
rcu: Add support for consolidated-RCU reader checking
rcu/sync: Remove custom check for reader-section
ipv4: add lockdep condition to fix for_each_entry
driver/core: Convert to use built-in RCU list checking
workqueue: Convert for_each_wq to use built-in list check
x86/pci: Pass lockdep condition to pcm_mmcfg_list iterator
acpi: Use built-in RCU list checking for acpi_ioremaps list
doc: Update documentation about list_for_each_entry_rcu
Documentation/RCU/lockdep.txt | 15 +++++++++++----
Documentation/RCU/whatisRCU.txt | 9 ++++++++-
arch/x86/pci/mmconfig-shared.c | 5 +++--
drivers/acpi/osl.c | 6 ++++--
drivers/base/base.h | 1 +
drivers/base/core.c | 10 ++++++++++
drivers/base/power/runtime.c | 15 ++++++++++-----
include/linux/rcu_sync.h | 5 ++---
include/linux/rculist.h | 28 +++++++++++++++++++++++-----
include/linux/rcupdate.h | 7 +++++++
kernel/rcu/Kconfig.debug | 11 +++++++++++
kernel/rcu/sync.c | 22 ----------------------
kernel/rcu/update.c | 20 +++++++++++++++-----
kernel/workqueue.c | 10 ++--------
net/ipv4/fib_frontend.c | 3 ++-
15 files changed, 109 insertions(+), 58 deletions(-)
--
2.22.0.510.g264f2c817a-goog
^ permalink raw reply
* Re: [PATCH v2 1/3] x86/boot: Introduce the kernel_info
From: hpa @ 2019-07-12 16:04 UTC (permalink / raw)
To: Daniel Kiper, linux-doc, linux-kernel, x86
Cc: bp, corbet, dpsmith, eric.snowberg, kanth.ghatraju, konrad.wilk,
mingo, ross.philipson, tglx
In-Reply-To: <20190704163612.14311-2-daniel.kiper@oracle.com>
On July 4, 2019 9:36:10 AM PDT, Daniel Kiper <daniel.kiper@oracle.com> wrote:
>The relationships between the headers are analogous to the various data
>sections:
>
> setup_header = .data
> boot_params/setup_data = .bss
>
>What is missing from the above list? That's right:
>
> kernel_info = .rodata
>
>We have been (ab)using .data for things that could go into .rodata or
>.bss for
>a long time, for lack of alternatives and -- especially early on --
>inertia.
>Also, the BIOS stub is responsible for creating boot_params, so it
>isn't
>available to a BIOS-based loader (setup_data is, though).
>
>setup_header is permanently limited to 144 bytes due to the reach of
>the
>2-byte jump field, which doubles as a length field for the structure,
>combined
>with the size of the "hole" in struct boot_params that a protected-mode
>loader
>or the BIOS stub has to copy it into. It is currently 119 bytes long,
>which
>leaves us with 25 very precious bytes. This isn't something that can be
>fixed
>without revising the boot protocol entirely, breaking backwards
>compatibility.
>
>boot_params proper is limited to 4096 bytes, but can be arbitrarily
>extended
>by adding setup_data entries. It cannot be used to communicate
>properties of
>the kernel image, because it is .bss and has no image-provided content.
>
>kernel_info solves this by providing an extensible place for
>information about
>the kernel image. It is readonly, because the kernel cannot rely on a
>bootloader copying its contents anywhere, but that is OK; if it becomes
>necessary it can still contain data items that an enabled bootloader
>would be
>expected to copy into a setup_data chunk.
>
>This patch does not bump setup_header version in arch/x86/boot/header.S
>because it will be followed by additional changes coming into the
>Linux/x86 boot protocol.
>
>Suggested-by: H. Peter Anvin <hpa@zytor.com>
>Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
>Reviewed-by: Eric Snowberg <eric.snowberg@oracle.com>
>Reviewed-by: Ross Philipson <ross.philipson@oracle.com>
>---
>v2 - suggestions/fixes:
> - rename setup_header2 to kernel_info,
> (suggested by H. Peter Anvin),
> - change kernel_info.header value to "InfO" (0x4f666e49),
> - new kernel_info description in Documentation/x86/boot.rst,
> (suggested by H. Peter Anvin),
> - drop kernel_info_offset_update() as an overkill and
> update kernel_info offset directly from main(),
> (suggested by Eric Snowberg),
> - new commit message
> (suggested by H. Peter Anvin),
> - fix some commit message misspellings
> (suggested by Eric Snowberg).
>---
>Documentation/x86/boot.rst | 89
>++++++++++++++++++++++++++++++++++
> arch/x86/boot/Makefile | 2 +-
> arch/x86/boot/compressed/Makefile | 4 +-
> arch/x86/boot/compressed/kernel_info.S | 12 +++++
> arch/x86/boot/header.S | 1 +
> arch/x86/boot/tools/build.c | 5 ++
> arch/x86/include/uapi/asm/bootparam.h | 1 +
> 7 files changed, 111 insertions(+), 3 deletions(-)
> create mode 100644 arch/x86/boot/compressed/kernel_info.S
>
>diff --git a/Documentation/x86/boot.rst b/Documentation/x86/boot.rst
>index 08a2f100c0e6..a934a56f0516 100644
>--- a/Documentation/x86/boot.rst
>+++ b/Documentation/x86/boot.rst
>@@ -68,8 +68,25 @@ Protocol 2.12 (Kernel 3.8) Added the xloadflags
>field and extension fields
> Protocol 2.13 (Kernel 3.14) Support 32- and 64-bit flags being set in
> xloadflags to support booting a 64-bit kernel from 32-bit
> EFI
>+
>+Protocol 2.14: BURNT BY INCORRECT COMMIT
>ae7e1238e68f2a472a125673ab506d49158c1889
>+ (x86/boot: Add ACPI RSDP address to setup_header)
>+ DO NOT USE!!! ASSUME SAME AS 2.13.
>+
>+Protocol 2.15: (Kernel 5.3) Added the kernel_info.
>============= ============================================================
>
>+.. note::
>+ The protocol version number should be changed only if the setup
>header
>+ is changed. There is no need to update the version number if
>boot_params
>+ or kernel_info are changed. Additionally, it is recommended to
>use
>+ xloadflags (in this case the protocol version number should not
>be
>+ updated either) or kernel_info to communicate supported Linux
>kernel
>+ features to the boot loader. Due to very limited space available
>in
>+ the original setup header every update to it should be considered
>+ with great care. Starting from the protocol 2.15 the primary way
>to
>+ communicate things to the boot loader is the kernel_info.
>+
>
> Memory Layout
> =============
>@@ -207,6 +224,7 @@ Offset/Size Proto Name Meaning
> 0258/8 2.10+ pref_address Preferred loading address
> 0260/4 2.10+ init_size Linear memory required during initialization
> 0264/4 2.11+ handover_offset Offset of handover entry point
>+0268/4 2.15+ kernel_info_offset Offset of the kernel_info
>=========== ======== ===================== ============================================
>
> .. note::
>@@ -855,6 +873,77 @@ Offset/size: 0x264/4
>
> See EFI HANDOVER PROTOCOL below for more details.
>
>+============ ==================
>+Field name: kernel_info_offset
>+Type: read
>+Offset/size: 0x268/4
>+Protocol: 2.15+
>+============ ==================
>+
>+ This field is the offset from the beginning of the kernel image to
>the
>+ kernel_info. It is embedded in the Linux image in the uncompressed
>+ protected mode region.
>+
>+
>+The kernel_info
>+===============
>+
>+The relationships between the headers are analogous to the various
>data
>+sections:
>+
>+ setup_header = .data
>+ boot_params/setup_data = .bss
>+
>+What is missing from the above list? That's right:
>+
>+ kernel_info = .rodata
>+
>+We have been (ab)using .data for things that could go into .rodata or
>.bss for
>+a long time, for lack of alternatives and -- especially early on --
>inertia.
>+Also, the BIOS stub is responsible for creating boot_params, so it
>isn't
>+available to a BIOS-based loader (setup_data is, though).
>+
>+setup_header is permanently limited to 144 bytes due to the reach of
>the
>+2-byte jump field, which doubles as a length field for the structure,
>combined
>+with the size of the "hole" in struct boot_params that a
>protected-mode loader
>+or the BIOS stub has to copy it into. It is currently 119 bytes long,
>which
>+leaves us with 25 very precious bytes. This isn't something that can
>be fixed
>+without revising the boot protocol entirely, breaking backwards
>compatibility.
>+
>+boot_params proper is limited to 4096 bytes, but can be arbitrarily
>extended
>+by adding setup_data entries. It cannot be used to communicate
>properties of
>+the kernel image, because it is .bss and has no image-provided
>content.
>+
>+kernel_info solves this by providing an extensible place for
>information about
>+the kernel image. It is readonly, because the kernel cannot rely on a
>+bootloader copying its contents anywhere, but that is OK; if it
>becomes
>+necessary it can still contain data items that an enabled bootloader
>would be
>+expected to copy into a setup_data chunk.
>+
>+It is recommended to not store large data chunks, e.g. strings,
>directly in the
>+kernel_info struct. Such data should be placed outside of it and
>pointed from
>+the kernel_info structure using offsets from the beginning of the
>structure,
>+the kernel_info.header field.
>+
>+
>+Details of the kernel_info Fields
>+=================================
>+
>+============ ========
>+Field name: header
>+Offset/size: 0x0000/4
>+============ ========
>+
>+ Contains the magic number "InfO" (0x4f666e49).
>+
>+============ ========
>+Field name: size
>+Offset/size: 0x0004/4
>+============ ========
>+
>+ This field contains the size of the kernel_info including
>kernel_info.header.
>+ It should be used by the boot loader to detect supported fields in
>the kernel_info.
>+
>
> The Image Checksum
> ==================
>diff --git a/arch/x86/boot/Makefile b/arch/x86/boot/Makefile
>index e2839b5c246c..c30a9b642a86 100644
>--- a/arch/x86/boot/Makefile
>+++ b/arch/x86/boot/Makefile
>@@ -87,7 +87,7 @@ $(obj)/vmlinux.bin: $(obj)/compressed/vmlinux FORCE
>
> SETUP_OBJS = $(addprefix $(obj)/,$(setup-y))
>
>-sed-zoffset := -e 's/^\([0-9a-fA-F]*\) [ABCDGRSTVW]
>\(startup_32\|startup_64\|efi32_stub_entry\|efi64_stub_entry\|efi_pe_entry\|input_data\|_end\|_ehead\|_text\|z_.*\)$$/\#define
>ZO_\2 0x\1/p'
>+sed-zoffset := -e 's/^\([0-9a-fA-F]*\) [ABCDGRSTVW]
>\(startup_32\|startup_64\|efi32_stub_entry\|efi64_stub_entry\|efi_pe_entry\|input_data\|kernel_info\|_end\|_ehead\|_text\|z_.*\)$$/\#define
>ZO_\2 0x\1/p'
>
> quiet_cmd_zoffset = ZOFFSET $@
> cmd_zoffset = $(NM) $< | sed -n $(sed-zoffset) > $@
>diff --git a/arch/x86/boot/compressed/Makefile
>b/arch/x86/boot/compressed/Makefile
>index 6b84afdd7538..fad3b18e2cc3 100644
>--- a/arch/x86/boot/compressed/Makefile
>+++ b/arch/x86/boot/compressed/Makefile
>@@ -72,8 +72,8 @@ $(obj)/../voffset.h: vmlinux FORCE
>
> $(obj)/misc.o: $(obj)/../voffset.h
>
>-vmlinux-objs-y := $(obj)/vmlinux.lds $(obj)/head_$(BITS).o
>$(obj)/misc.o \
>- $(obj)/string.o $(obj)/cmdline.o $(obj)/error.o \
>+vmlinux-objs-y := $(obj)/vmlinux.lds $(obj)/kernel_info.o
>$(obj)/head_$(BITS).o \
>+ $(obj)/misc.o $(obj)/string.o $(obj)/cmdline.o $(obj)/error.o \
> $(obj)/piggy.o $(obj)/cpuflags.o
>
> vmlinux-objs-$(CONFIG_EARLY_PRINTK) += $(obj)/early_serial_console.o
>diff --git a/arch/x86/boot/compressed/kernel_info.S
>b/arch/x86/boot/compressed/kernel_info.S
>new file mode 100644
>index 000000000000..3f1cb301b9ff
>--- /dev/null
>+++ b/arch/x86/boot/compressed/kernel_info.S
>@@ -0,0 +1,12 @@
>+/* SPDX-License-Identifier: GPL-2.0 */
>+
>+ .section ".rodata.kernel_info", "a"
>+
>+ .global kernel_info
>+
>+kernel_info:
>+ /* Header. */
>+ .ascii "InfO"
>+ /* Size. */
>+ .long kernel_info_end - kernel_info
>+kernel_info_end:
>diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S
>index 850b8762e889..ec6a25a43148 100644
>--- a/arch/x86/boot/header.S
>+++ b/arch/x86/boot/header.S
>@@ -557,6 +557,7 @@ pref_address: .quad LOAD_PHYSICAL_ADDR # preferred
>load addr
>
> init_size: .long INIT_SIZE # kernel initialization size
> handover_offset: .long 0 # Filled in by build.c
>+kernel_info_offset: .long 0 # Filled in by build.c
>
># End of setup header
>#####################################################
>
>diff --git a/arch/x86/boot/tools/build.c b/arch/x86/boot/tools/build.c
>index a93d44e58f9c..55e669d29e54 100644
>--- a/arch/x86/boot/tools/build.c
>+++ b/arch/x86/boot/tools/build.c
>@@ -56,6 +56,7 @@ u8 buf[SETUP_SECT_MAX*512];
> unsigned long efi32_stub_entry;
> unsigned long efi64_stub_entry;
> unsigned long efi_pe_entry;
>+unsigned long kernel_info;
> unsigned long startup_64;
>
>/*----------------------------------------------------------------------*/
>@@ -321,6 +322,7 @@ static void parse_zoffset(char *fname)
> PARSE_ZOFS(p, efi32_stub_entry);
> PARSE_ZOFS(p, efi64_stub_entry);
> PARSE_ZOFS(p, efi_pe_entry);
>+ PARSE_ZOFS(p, kernel_info);
> PARSE_ZOFS(p, startup_64);
>
> p = strchr(p, '\n');
>@@ -410,6 +412,9 @@ int main(int argc, char ** argv)
>
> efi_stub_entry_update();
>
>+ /* Update kernel_info offset. */
>+ put_unaligned_le32(kernel_info, &buf[0x268]);
>+
> crc = partial_crc32(buf, i, crc);
> if (fwrite(buf, 1, i, dest) != i)
> die("Writing setup failed");
>diff --git a/arch/x86/include/uapi/asm/bootparam.h
>b/arch/x86/include/uapi/asm/bootparam.h
>index 60733f137e9a..b05318112452 100644
>--- a/arch/x86/include/uapi/asm/bootparam.h
>+++ b/arch/x86/include/uapi/asm/bootparam.h
>@@ -86,6 +86,7 @@ struct setup_header {
> __u64 pref_address;
> __u32 init_size;
> __u32 handover_offset;
>+ __u32 kernel_info_offset;
> } __attribute__((packed));
>
> struct sys_desc_table {
I should like to make make things a bit more stringent: additional data should be made offsets from the kernel_info structure *and they should live in the .rodata.kernel_info section*. We should add a size field for the entire .kernel_info section, thus ensuring it is a single self-contained blob.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply
* Re: [PATCH v2 3/3] x86/boot: Introduce the kernel_info.setup_type_max
From: hpa @ 2019-07-12 15:59 UTC (permalink / raw)
To: Daniel Kiper, linux-doc, linux-kernel, x86
Cc: bp, corbet, dpsmith, eric.snowberg, kanth.ghatraju, konrad.wilk,
mingo, ross.philipson, tglx
In-Reply-To: <20190704163612.14311-4-daniel.kiper@oracle.com>
On July 4, 2019 9:36:12 AM PDT, Daniel Kiper <daniel.kiper@oracle.com> wrote:
>This field contains maximal allowed type for setup_data and
>setup_indirect structs.
>
>And finally bump setup_header version in arch/x86/boot/header.S.
>
>Suggested-by: H. Peter Anvin <hpa@zytor.com>
>Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
>Reviewed-by: Ross Philipson <ross.philipson@oracle.com>
>Reviewed-by: Eric Snowberg <eric.snowberg@oracle.com>
>---
> Documentation/x86/boot.rst | 10 +++++++++-
> arch/x86/boot/compressed/kernel_info.S | 4 ++++
> arch/x86/boot/header.S | 2 +-
> arch/x86/include/uapi/asm/bootparam.h | 3 +++
> 4 files changed, 17 insertions(+), 2 deletions(-)
>
>diff --git a/Documentation/x86/boot.rst b/Documentation/x86/boot.rst
>index 23d3726d54fc..63609fd0517f 100644
>--- a/Documentation/x86/boot.rst
>+++ b/Documentation/x86/boot.rst
>@@ -73,7 +73,8 @@ Protocol 2.14: BURNT BY INCORRECT COMMIT
>ae7e1238e68f2a472a125673ab506d49158c188
> (x86/boot: Add ACPI RSDP address to setup_header)
> DO NOT USE!!! ASSUME SAME AS 2.13.
>
>-Protocol 2.15: (Kernel 5.3) Added the kernel_info and setup_indirect.
>+Protocol 2.15: (Kernel 5.3) Added the kernel_info,
>kernel_info.setup_type_max
>+ and setup_indirect.
>============= ============================================================
>
> .. note::
>@@ -980,6 +981,13 @@ Offset/size: 0x0004/4
>This field contains the size of the kernel_info including
>kernel_info.header.
>It should be used by the boot loader to detect supported fields in the
>kernel_info.
>
>+============ ==============
>+Field name: setup_type_max
>+Offset/size: 0x0008/4
>+============ ==============
>+
>+ This field contains maximal allowed type for setup_data and
>setup_indirect structs.
>+
>
> The Image Checksum
> ==================
>diff --git a/arch/x86/boot/compressed/kernel_info.S
>b/arch/x86/boot/compressed/kernel_info.S
>index 3f1cb301b9ff..2f28aabf6558 100644
>--- a/arch/x86/boot/compressed/kernel_info.S
>+++ b/arch/x86/boot/compressed/kernel_info.S
>@@ -1,5 +1,7 @@
> /* SPDX-License-Identifier: GPL-2.0 */
>
>+#include <asm/bootparam.h>
>+
> .section ".rodata.kernel_info", "a"
>
> .global kernel_info
>@@ -9,4 +11,6 @@ kernel_info:
> .ascii "InfO"
> /* Size. */
> .long kernel_info_end - kernel_info
>+ /* Maximal allowed type for setup_data and setup_indirect
>structs. */
>+ .long SETUP_TYPE_MAX
> kernel_info_end:
>diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S
>index ec6a25a43148..893a456663ab 100644
>--- a/arch/x86/boot/header.S
>+++ b/arch/x86/boot/header.S
>@@ -300,7 +300,7 @@ _start:
> # Part 2 of the header, from the old setup.S
>
> .ascii "HdrS" # header signature
>- .word 0x020d # header version number (>= 0x0105)
>+ .word 0x020f # header version number (>= 0x0105)
> # or else old loadlin-1.5 will fail)
> .globl realmode_swtch
> realmode_swtch: .word 0, 0 # default_switch, SETUPSEG
>diff --git a/arch/x86/include/uapi/asm/bootparam.h
>b/arch/x86/include/uapi/asm/bootparam.h
>index aaaa17fa6ad6..2ba870dae6f3 100644
>--- a/arch/x86/include/uapi/asm/bootparam.h
>+++ b/arch/x86/include/uapi/asm/bootparam.h
>@@ -12,6 +12,9 @@
> #define SETUP_JAILHOUSE 6
> #define SETUP_INDIRECT 7
>
>+/* max(SETUP_*) */
>+#define SETUP_TYPE_MAX SETUP_INDIRECT
>+
> /* ram_size flags */
> #define RAMDISK_IMAGE_START_MASK 0x07FF
> #define RAMDISK_PROMPT_FLAG 0x8000
Bump the version number and add setup_max before adding the indirect stuff. That will nicely double as at the very least a first-order validity check.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply
* Re: [PATCH v2 2/3] x86/boot: Introduce the setup_indirect
From: hpa @ 2019-07-12 15:56 UTC (permalink / raw)
To: Daniel Kiper, linux-doc, linux-kernel, x86
Cc: bp, corbet, dpsmith, eric.snowberg, kanth.ghatraju, konrad.wilk,
mingo, ross.philipson, tglx
In-Reply-To: <20190704163612.14311-3-daniel.kiper@oracle.com>
On July 4, 2019 9:36:11 AM PDT, Daniel Kiper <daniel.kiper@oracle.com> wrote:
>The setup_data is a bit awkward to use for extremely large data
>objects,
>both because the setup_data header has to be adjacent to the data
>object
>and because it has a 32-bit length field. However, it is important that
>intermediate stages of the boot process have a way to identify which
>chunks of memory are occupied by kernel data.
>
>Thus we introduce a uniform way to specify such indirect data as
>setup_indirect struct and SETUP_INDIRECT type.
>
>This patch does not bump setup_header version in arch/x86/boot/header.S
>because it will be followed by additional changes coming into the
>Linux/x86 boot protocol.
>
>Suggested-by: H. Peter Anvin <hpa@zytor.com>
>Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
>Reviewed-by: Eric Snowberg <eric.snowberg@oracle.com>
>Reviewed-by: Ross Philipson <ross.philipson@oracle.com>
>---
>v2 - suggestions/fixes:
> - add setup_indirect usage example
> (suggested by Eric Snowberg and Ross Philipson).
>---
>Documentation/x86/boot.rst | 38
>++++++++++++++++++++++++++++++++++-
> arch/x86/include/uapi/asm/bootparam.h | 11 +++++++++-
> 2 files changed, 47 insertions(+), 2 deletions(-)
>
>diff --git a/Documentation/x86/boot.rst b/Documentation/x86/boot.rst
>index a934a56f0516..23d3726d54fc 100644
>--- a/Documentation/x86/boot.rst
>+++ b/Documentation/x86/boot.rst
>@@ -73,7 +73,7 @@ Protocol 2.14: BURNT BY INCORRECT COMMIT
>ae7e1238e68f2a472a125673ab506d49158c188
> (x86/boot: Add ACPI RSDP address to setup_header)
> DO NOT USE!!! ASSUME SAME AS 2.13.
>
>-Protocol 2.15: (Kernel 5.3) Added the kernel_info.
>+Protocol 2.15: (Kernel 5.3) Added the kernel_info and setup_indirect.
>============= ============================================================
>
> .. note::
>@@ -827,6 +827,42 @@ Protocol: 2.09+
> sure to consider the case where the linked list already contains
> entries.
>
>+ The setup_data is a bit awkward to use for extremely large data
>objects,
>+ both because the setup_data header has to be adjacent to the data
>object
>+ and because it has a 32-bit length field. However, it is important
>that
>+ intermediate stages of the boot process have a way to identify which
>+ chunks of memory are occupied by kernel data.
>+
>+ Thus setup_indirect struct and SETUP_INDIRECT type were introduced
>in
>+ protocol 2.15.
>+
>+ struct setup_indirect {
>+ __u32 type;
>+ __u32 reserved; /* Reserved, must be set to zero. */
>+ __u64 len;
>+ __u64 addr;
>+ };
>+
>+ The type member is itself simply a SETUP_* type. However, it cannot
>be
>+ SETUP_INDIRECT since making the setup_indirect a tree structure
>could
>+ require a lot of stack space in something that needs to parse it and
>+ stack space can be limited in boot contexts.
>+
>+ Let's give an example how to point to SETUP_E820_EXT data using
>setup_indirect.
>+ In this case setup_data and setup_indirect will look like this:
>+
>+ struct setup_data {
>+ __u64 next = 0 or <addr_of_next_setup_data_struct>;
>+ __u32 type = SETUP_INDIRECT;
>+ __u32 len = sizeof(setup_data);
>+ __u8 data[sizeof(setup_indirect)] = struct setup_indirect {
>+ __u32 type = SETUP_E820_EXT;
>+ __u32 reserved = 0;
>+ __u64 len = <len_of_SETUP_E820_EXT_data>;
>+ __u64 addr = <addr_of_SETUP_E820_EXT_data>;
>+ }
>+ }
>+
> ============ ============
> Field name: pref_address
> Type: read (reloc)
>diff --git a/arch/x86/include/uapi/asm/bootparam.h
>b/arch/x86/include/uapi/asm/bootparam.h
>index b05318112452..aaaa17fa6ad6 100644
>--- a/arch/x86/include/uapi/asm/bootparam.h
>+++ b/arch/x86/include/uapi/asm/bootparam.h
>@@ -2,7 +2,7 @@
> #ifndef _ASM_X86_BOOTPARAM_H
> #define _ASM_X86_BOOTPARAM_H
>
>-/* setup_data types */
>+/* setup_data/setup_indirect types */
> #define SETUP_NONE 0
> #define SETUP_E820_EXT 1
> #define SETUP_DTB 2
>@@ -10,6 +10,7 @@
> #define SETUP_EFI 4
> #define SETUP_APPLE_PROPERTIES 5
> #define SETUP_JAILHOUSE 6
>+#define SETUP_INDIRECT 7
>
> /* ram_size flags */
> #define RAMDISK_IMAGE_START_MASK 0x07FF
>@@ -47,6 +48,14 @@ struct setup_data {
> __u8 data[0];
> };
>
>+/* extensible setup indirect data node */
>+struct setup_indirect {
>+ __u32 type;
>+ __u32 reserved; /* Reserved, must be set to zero. */
>+ __u64 len;
>+ __u64 addr;
>+};
>+
> struct setup_header {
> __u8 setup_sects;
> __u16 root_flags;
This needs actual implementation; we can't advertise it until the kernel knows how to consume the data! It probably should be moved to after the 3/3 patch.
Implementing this has two parts:
1. The kernel needs to be augmented so it can find current objects via indirection.
2. And this is the main reason for this in the first place: the early code needs to walk the list and map out the memory areas which are occupied so it doesn't clobber anything; this allows this code to be generic as opposed to having to know what is a pointer and what size it might point to.
(The decompressor didn't need this until kaslr entered the picture, but now it does, sadly.)
Optional/future enhancements that might be nice:
3. Add some kind of description (e.g. foo=u64 ; bar=str ; baz=blob) to make it possible to write a bootloader that can load these kinds of objects without specific enabling.
4. Add support for mapping initramfs fragments this way.
5. Add support for passingload-on-boot kernel modules.
6. ... ?
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply
* [PATCH v4] tpm: Document UEFI event log quirks
From: Jarkko Sakkinen @ 2019-07-12 15:44 UTC (permalink / raw)
To: linux-kernel, linux-integrity, linux-doc
Cc: tweek, matthewgarrett, jorhand, rdunlap, Jarkko Sakkinen,
Jonathan Corbet, Sasha Levin
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 3652 bytes --]
There are some weird quirks when it comes to UEFI event log. Provide a
brief introduction to TPM event log mechanism and describe the quirks
and how they can be sorted out.
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
v4: - Unfortanely -> Unfortunately
v3: - Add a section for refs and use a bullet list to enumerate them.
- Remove an invalid author info.
v2: - Fix one typo.
- Refine the last paragraph to better explain how the two halves
of the event log are concatenated.
Documentation/security/tpm/index.rst | 1 +
Documentation/security/tpm/tpm_event_log.rst | 55 ++++++++++++++++++++
2 files changed, 56 insertions(+)
create mode 100644 Documentation/security/tpm/tpm_event_log.rst
diff --git a/Documentation/security/tpm/index.rst b/Documentation/security/tpm/index.rst
index af77a7bbb070..db566350bcd5 100644
--- a/Documentation/security/tpm/index.rst
+++ b/Documentation/security/tpm/index.rst
@@ -4,4 +4,5 @@ Trusted Platform Module documentation
.. toctree::
+ tpm_event_log
tpm_vtpm_proxy
diff --git a/Documentation/security/tpm/tpm_event_log.rst b/Documentation/security/tpm/tpm_event_log.rst
new file mode 100644
index 000000000000..f00f7a1d5e92
--- /dev/null
+++ b/Documentation/security/tpm/tpm_event_log.rst
@@ -0,0 +1,55 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+=============
+TPM Event Log
+=============
+
+This document briefly describes what TPM log is and how it is handed
+over from the preboot firmware to the operating system.
+
+Introduction
+============
+
+The preboot firmware maintains an event log that gets new entries every
+time something gets hashed by it to any of the PCR registers. The events
+are segregated by their type and contain the value of the hashed PCR
+register. Typically, the preboot firmware will hash the components to
+who execution is to be handed over or actions relevant to the boot
+process.
+
+The main application for this is remote attestation and the reason why
+it is useful is nicely put in the very first section of [1]:
+
+"Attestation is used to provide information about the platform’s state
+to a challenger. However, PCR contents are difficult to interpret;
+therefore, attestation is typically more useful when the PCR contents
+are accompanied by a measurement log. While not trusted on their own,
+the measurement log contains a richer set of information than do the PCR
+contents. The PCR contents are used to provide the validation of the
+measurement log."
+
+UEFI event log
+==============
+
+UEFI provided event log has a few somewhat weird quirks.
+
+Before calling ExitBootServices() Linux EFI stub copies the event log to
+a custom configuration table defined by the stub itself. Unfortunately,
+the events generated by ExitBootServices() don't end up in the table.
+
+The firmware provides so called final events configuration table to sort
+out this issue. Events gets mirrored to this table after the first time
+EFI_TCG2_PROTOCOL.GetEventLog() gets called.
+
+This introduces another problem: nothing guarantees that it is not called
+before the Linux EFI stub gets to run. Thus, it needs to calculate and save the
+final events table size while the stub is still running to the custom
+configuration table so that the TPM driver can later on skip these events when
+concatenating two halves of the event log from the custom configuration table
+and the final events table.
+
+References
+==========
+
+- [1] https://trustedcomputinggroup.org/resource/pc-client-specific-platform-firmware-profile-specification/
+- [2] The final concatenation is done in drivers/char/tpm/eventlog/efi.c
--
2.20.1
^ permalink raw reply related
* Re: [PATCH v3] tpm: Document UEFI event log quirks
From: Jarkko Sakkinen @ 2019-07-12 15:45 UTC (permalink / raw)
To: Randy Dunlap, linux-kernel, linux-integrity, linux-doc
Cc: tweek, matthewgarrett, jorhand, Jonathan Corbet, Sasha Levin
In-Reply-To: <6c974f53-6dca-33fd-5aca-056ab8b274ed@infradead.org>
On Fri, 2019-07-12 at 07:55 -0700, Randy Dunlap wrote:
> +Before calling ExitBootServices() Linux EFI stub copies the event log to
> > +a custom configuration table defined by the stub itself. Unfortanely,
>
> [again:] Unfortunately,
Ugh, I'm sorry. Sent an update.
/Jarkko
^ permalink raw reply
* Re: [PATCH] treewide: Rename rcu_dereference_raw_notrace to _check
From: Joel Fernandes @ 2019-07-12 15:34 UTC (permalink / raw)
To: Paul E. McKenney
Cc: linux-kernel, Benjamin Herrenschmidt, Ingo Molnar,
Jonathan Corbet, Josh Triplett, kvm-ppc, Lai Jiangshan, linux-doc,
linuxppc-dev, Mathieu Desnoyers, Michael Ellerman, Paul Mackerras,
rcu, Steven Rostedt, byungchul.park, kernel-team
In-Reply-To: <20190712150107.GT26519@linux.ibm.com>
On Fri, Jul 12, 2019 at 08:01:07AM -0700, Paul E. McKenney wrote:
> On Thu, Jul 11, 2019 at 04:45:41PM -0400, Joel Fernandes (Google) wrote:
> > The rcu_dereference_raw_notrace() API name is confusing.
> > It is equivalent to rcu_dereference_raw() except that it also does
> > sparse pointer checking.
> >
> > There are only a few users of rcu_dereference_raw_notrace(). This
> > patches renames all of them to be rcu_dereference_raw_check with the
> > "check" indicating sparse checking.
> >
> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>
> I queued this, but reworked the commit log and fixed a couple of
> irritating checkpatch issues that were in the original code.
> Does this work for you?
Thanks, yes it looks good to me.
thanks,
- Joel
>
> Thanx, Paul
>
> ------------------------------------------------------------------------
>
> commit bd5c0fea6016c90cf7a9eb0435cd0c373dfdac2f
> Author: Joel Fernandes (Google) <joel@joelfernandes.org>
> Date: Thu Jul 11 16:45:41 2019 -0400
>
> treewide: Rename rcu_dereference_raw_notrace() to _check()
>
> The rcu_dereference_raw_notrace() API name is confusing. It is equivalent
> to rcu_dereference_raw() except that it also does sparse pointer checking.
>
> There are only a few users of rcu_dereference_raw_notrace(). This patches
> renames all of them to be rcu_dereference_raw_check() with the "_check()"
> indicating sparse checking.
>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> [ paulmck: Fix checkpatch warnings about parentheses. ]
> Signed-off-by: Paul E. McKenney <paulmck@linux.ibm.com>
>
> diff --git a/Documentation/RCU/Design/Requirements/Requirements.html b/Documentation/RCU/Design/Requirements/Requirements.html
> index f04c467e55c5..467251f7fef6 100644
> --- a/Documentation/RCU/Design/Requirements/Requirements.html
> +++ b/Documentation/RCU/Design/Requirements/Requirements.html
> @@ -2514,7 +2514,7 @@ disabled across the entire RCU read-side critical section.
> <p>
> It is possible to use tracing on RCU code, but tracing itself
> uses RCU.
> -For this reason, <tt>rcu_dereference_raw_notrace()</tt>
> +For this reason, <tt>rcu_dereference_raw_check()</tt>
> is provided for use by tracing, which avoids the destructive
> recursion that could otherwise ensue.
> This API is also used by virtualization in some architectures,
> diff --git a/arch/powerpc/include/asm/kvm_book3s_64.h b/arch/powerpc/include/asm/kvm_book3s_64.h
> index 21b1ed5df888..53388a311967 100644
> --- a/arch/powerpc/include/asm/kvm_book3s_64.h
> +++ b/arch/powerpc/include/asm/kvm_book3s_64.h
> @@ -546,7 +546,7 @@ static inline void note_hpte_modification(struct kvm *kvm,
> */
> static inline struct kvm_memslots *kvm_memslots_raw(struct kvm *kvm)
> {
> - return rcu_dereference_raw_notrace(kvm->memslots[0]);
> + return rcu_dereference_raw_check(kvm->memslots[0]);
> }
>
> extern void kvmppc_mmu_debugfs_init(struct kvm *kvm);
> diff --git a/include/linux/rculist.h b/include/linux/rculist.h
> index e91ec9ddcd30..932296144131 100644
> --- a/include/linux/rculist.h
> +++ b/include/linux/rculist.h
> @@ -622,7 +622,7 @@ static inline void hlist_add_behind_rcu(struct hlist_node *n,
> * as long as the traversal is guarded by rcu_read_lock().
> */
> #define hlist_for_each_entry_rcu(pos, head, member) \
> - for (pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\
> + for (pos = hlist_entry_safe(rcu_dereference_raw(hlist_first_rcu(head)),\
> typeof(*(pos)), member); \
> pos; \
> pos = hlist_entry_safe(rcu_dereference_raw(hlist_next_rcu(\
> @@ -642,10 +642,10 @@ static inline void hlist_add_behind_rcu(struct hlist_node *n,
> * not do any RCU debugging or tracing.
> */
> #define hlist_for_each_entry_rcu_notrace(pos, head, member) \
> - for (pos = hlist_entry_safe (rcu_dereference_raw_notrace(hlist_first_rcu(head)),\
> + for (pos = hlist_entry_safe(rcu_dereference_raw_check(hlist_first_rcu(head)),\
> typeof(*(pos)), member); \
> pos; \
> - pos = hlist_entry_safe(rcu_dereference_raw_notrace(hlist_next_rcu(\
> + pos = hlist_entry_safe(rcu_dereference_raw_check(hlist_next_rcu(\
> &(pos)->member)), typeof(*(pos)), member))
>
> /**
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index 0c9b92799abc..e5161e377ad4 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -478,7 +478,7 @@ do { \
> * The no-tracing version of rcu_dereference_raw() must not call
> * rcu_read_lock_held().
> */
> -#define rcu_dereference_raw_notrace(p) __rcu_dereference_check((p), 1, __rcu)
> +#define rcu_dereference_raw_check(p) __rcu_dereference_check((p), 1, __rcu)
>
> /**
> * rcu_dereference_protected() - fetch RCU pointer when updates prevented
> diff --git a/kernel/trace/ftrace_internal.h b/kernel/trace/ftrace_internal.h
> index 0515a2096f90..0456e0a3dab1 100644
> --- a/kernel/trace/ftrace_internal.h
> +++ b/kernel/trace/ftrace_internal.h
> @@ -6,22 +6,22 @@
>
> /*
> * Traverse the ftrace_global_list, invoking all entries. The reason that we
> - * can use rcu_dereference_raw_notrace() is that elements removed from this list
> + * can use rcu_dereference_raw_check() is that elements removed from this list
> * are simply leaked, so there is no need to interact with a grace-period
> - * mechanism. The rcu_dereference_raw_notrace() calls are needed to handle
> + * mechanism. The rcu_dereference_raw_check() calls are needed to handle
> * concurrent insertions into the ftrace_global_list.
> *
> * Silly Alpha and silly pointer-speculation compiler optimizations!
> */
> #define do_for_each_ftrace_op(op, list) \
> - op = rcu_dereference_raw_notrace(list); \
> + op = rcu_dereference_raw_check(list); \
> do
>
> /*
> * Optimized for just a single item in the list (as that is the normal case).
> */
> #define while_for_each_ftrace_op(op) \
> - while (likely(op = rcu_dereference_raw_notrace((op)->next)) && \
> + while (likely(op = rcu_dereference_raw_check((op)->next)) && \
> unlikely((op) != &ftrace_list_end))
>
> extern struct ftrace_ops __rcu *ftrace_ops_list;
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 2c92b3d9ea30..1d69110d9e5b 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -2642,10 +2642,10 @@ static void ftrace_exports(struct ring_buffer_event *event)
>
> preempt_disable_notrace();
>
> - export = rcu_dereference_raw_notrace(ftrace_exports_list);
> + export = rcu_dereference_raw_check(ftrace_exports_list);
> while (export) {
> trace_process_export(export, event);
> - export = rcu_dereference_raw_notrace(export->next);
> + export = rcu_dereference_raw_check(export->next);
> }
>
> preempt_enable_notrace();
^ permalink raw reply
* Re: [PATCH] treewide: Rename rcu_dereference_raw_notrace to _check
From: Paul E. McKenney @ 2019-07-12 15:01 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: linux-kernel, Benjamin Herrenschmidt, Ingo Molnar,
Jonathan Corbet, Josh Triplett, kvm-ppc, Lai Jiangshan, linux-doc,
linuxppc-dev, Mathieu Desnoyers, Michael Ellerman, Paul Mackerras,
rcu, Steven Rostedt, byungchul.park, kernel-team
In-Reply-To: <20190711204541.28940-1-joel@joelfernandes.org>
On Thu, Jul 11, 2019 at 04:45:41PM -0400, Joel Fernandes (Google) wrote:
> The rcu_dereference_raw_notrace() API name is confusing.
> It is equivalent to rcu_dereference_raw() except that it also does
> sparse pointer checking.
>
> There are only a few users of rcu_dereference_raw_notrace(). This
> patches renames all of them to be rcu_dereference_raw_check with the
> "check" indicating sparse checking.
>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
I queued this, but reworked the commit log and fixed a couple of
irritating checkpatch issues that were in the original code.
Does this work for you?
Thanx, Paul
------------------------------------------------------------------------
commit bd5c0fea6016c90cf7a9eb0435cd0c373dfdac2f
Author: Joel Fernandes (Google) <joel@joelfernandes.org>
Date: Thu Jul 11 16:45:41 2019 -0400
treewide: Rename rcu_dereference_raw_notrace() to _check()
The rcu_dereference_raw_notrace() API name is confusing. It is equivalent
to rcu_dereference_raw() except that it also does sparse pointer checking.
There are only a few users of rcu_dereference_raw_notrace(). This patches
renames all of them to be rcu_dereference_raw_check() with the "_check()"
indicating sparse checking.
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
[ paulmck: Fix checkpatch warnings about parentheses. ]
Signed-off-by: Paul E. McKenney <paulmck@linux.ibm.com>
diff --git a/Documentation/RCU/Design/Requirements/Requirements.html b/Documentation/RCU/Design/Requirements/Requirements.html
index f04c467e55c5..467251f7fef6 100644
--- a/Documentation/RCU/Design/Requirements/Requirements.html
+++ b/Documentation/RCU/Design/Requirements/Requirements.html
@@ -2514,7 +2514,7 @@ disabled across the entire RCU read-side critical section.
<p>
It is possible to use tracing on RCU code, but tracing itself
uses RCU.
-For this reason, <tt>rcu_dereference_raw_notrace()</tt>
+For this reason, <tt>rcu_dereference_raw_check()</tt>
is provided for use by tracing, which avoids the destructive
recursion that could otherwise ensue.
This API is also used by virtualization in some architectures,
diff --git a/arch/powerpc/include/asm/kvm_book3s_64.h b/arch/powerpc/include/asm/kvm_book3s_64.h
index 21b1ed5df888..53388a311967 100644
--- a/arch/powerpc/include/asm/kvm_book3s_64.h
+++ b/arch/powerpc/include/asm/kvm_book3s_64.h
@@ -546,7 +546,7 @@ static inline void note_hpte_modification(struct kvm *kvm,
*/
static inline struct kvm_memslots *kvm_memslots_raw(struct kvm *kvm)
{
- return rcu_dereference_raw_notrace(kvm->memslots[0]);
+ return rcu_dereference_raw_check(kvm->memslots[0]);
}
extern void kvmppc_mmu_debugfs_init(struct kvm *kvm);
diff --git a/include/linux/rculist.h b/include/linux/rculist.h
index e91ec9ddcd30..932296144131 100644
--- a/include/linux/rculist.h
+++ b/include/linux/rculist.h
@@ -622,7 +622,7 @@ static inline void hlist_add_behind_rcu(struct hlist_node *n,
* as long as the traversal is guarded by rcu_read_lock().
*/
#define hlist_for_each_entry_rcu(pos, head, member) \
- for (pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\
+ for (pos = hlist_entry_safe(rcu_dereference_raw(hlist_first_rcu(head)),\
typeof(*(pos)), member); \
pos; \
pos = hlist_entry_safe(rcu_dereference_raw(hlist_next_rcu(\
@@ -642,10 +642,10 @@ static inline void hlist_add_behind_rcu(struct hlist_node *n,
* not do any RCU debugging or tracing.
*/
#define hlist_for_each_entry_rcu_notrace(pos, head, member) \
- for (pos = hlist_entry_safe (rcu_dereference_raw_notrace(hlist_first_rcu(head)),\
+ for (pos = hlist_entry_safe(rcu_dereference_raw_check(hlist_first_rcu(head)),\
typeof(*(pos)), member); \
pos; \
- pos = hlist_entry_safe(rcu_dereference_raw_notrace(hlist_next_rcu(\
+ pos = hlist_entry_safe(rcu_dereference_raw_check(hlist_next_rcu(\
&(pos)->member)), typeof(*(pos)), member))
/**
diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 0c9b92799abc..e5161e377ad4 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -478,7 +478,7 @@ do { \
* The no-tracing version of rcu_dereference_raw() must not call
* rcu_read_lock_held().
*/
-#define rcu_dereference_raw_notrace(p) __rcu_dereference_check((p), 1, __rcu)
+#define rcu_dereference_raw_check(p) __rcu_dereference_check((p), 1, __rcu)
/**
* rcu_dereference_protected() - fetch RCU pointer when updates prevented
diff --git a/kernel/trace/ftrace_internal.h b/kernel/trace/ftrace_internal.h
index 0515a2096f90..0456e0a3dab1 100644
--- a/kernel/trace/ftrace_internal.h
+++ b/kernel/trace/ftrace_internal.h
@@ -6,22 +6,22 @@
/*
* Traverse the ftrace_global_list, invoking all entries. The reason that we
- * can use rcu_dereference_raw_notrace() is that elements removed from this list
+ * can use rcu_dereference_raw_check() is that elements removed from this list
* are simply leaked, so there is no need to interact with a grace-period
- * mechanism. The rcu_dereference_raw_notrace() calls are needed to handle
+ * mechanism. The rcu_dereference_raw_check() calls are needed to handle
* concurrent insertions into the ftrace_global_list.
*
* Silly Alpha and silly pointer-speculation compiler optimizations!
*/
#define do_for_each_ftrace_op(op, list) \
- op = rcu_dereference_raw_notrace(list); \
+ op = rcu_dereference_raw_check(list); \
do
/*
* Optimized for just a single item in the list (as that is the normal case).
*/
#define while_for_each_ftrace_op(op) \
- while (likely(op = rcu_dereference_raw_notrace((op)->next)) && \
+ while (likely(op = rcu_dereference_raw_check((op)->next)) && \
unlikely((op) != &ftrace_list_end))
extern struct ftrace_ops __rcu *ftrace_ops_list;
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 2c92b3d9ea30..1d69110d9e5b 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -2642,10 +2642,10 @@ static void ftrace_exports(struct ring_buffer_event *event)
preempt_disable_notrace();
- export = rcu_dereference_raw_notrace(ftrace_exports_list);
+ export = rcu_dereference_raw_check(ftrace_exports_list);
while (export) {
trace_process_export(export, event);
- export = rcu_dereference_raw_notrace(export->next);
+ export = rcu_dereference_raw_check(export->next);
}
preempt_enable_notrace();
^ permalink raw reply related
* Re: [PATCH v3] tpm: Document UEFI event log quirks
From: Randy Dunlap @ 2019-07-12 14:55 UTC (permalink / raw)
To: Jarkko Sakkinen, linux-kernel, linux-integrity, linux-doc
Cc: tweek, matthewgarrett, jorhand, Jonathan Corbet, Sasha Levin
In-Reply-To: <20190712124912.23630-1-jarkko.sakkinen@linux.intel.com>
On 7/12/19 5:49 AM, Jarkko Sakkinen wrote:
> There are some weird quirks when it comes to UEFI event log. Provide a
> brief introduction to TPM event log mechanism and describe the quirks
> and how they can be sorted out.
>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
> v3: Add a section and use bullet list for references. Remove (invalid)
> author info.
> v2: Fixed one type, adjusted the last paragraph and added the file
is that typo or type?
(one more below)
> to index.rst
> Documentation/security/tpm/index.rst | 1 +
> Documentation/security/tpm/tpm_event_log.rst | 55 ++++++++++++++++++++
> 2 files changed, 56 insertions(+)
> create mode 100644 Documentation/security/tpm/tpm_event_log.rst
>
> diff --git a/Documentation/security/tpm/index.rst b/Documentation/security/tpm/index.rst
> index 15783668644f..9e0815cb1e7f 100644
> --- a/Documentation/security/tpm/index.rst
> +++ b/Documentation/security/tpm/index.rst
> @@ -4,5 +4,6 @@ Trusted Platform Module documentation
>
> .. toctree::
>
> + tpm_event_log
> tpm_ftpm_tee
> tpm_vtpm_proxy
> diff --git a/Documentation/security/tpm/tpm_event_log.rst b/Documentation/security/tpm/tpm_event_log.rst
> new file mode 100644
> index 000000000000..068eeb659bb9
> --- /dev/null
> +++ b/Documentation/security/tpm/tpm_event_log.rst
> @@ -0,0 +1,55 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +=============
> +TPM Event Log
> +=============
> +
> +This document briefly describes what TPM log is and how it is handed
> +over from the preboot firmware to the operating system.
> +
> +Introduction
> +============
> +
> +The preboot firmware maintains an event log that gets new entries every
> +time something gets hashed by it to any of the PCR registers. The events
> +are segregated by their type and contain the value of the hashed PCR
> +register. Typically, the preboot firmware will hash the components to
> +who execution is to be handed over or actions relevant to the boot
> +process.
> +
> +The main application for this is remote attestation and the reason why
> +it is useful is nicely put in the very first section of [1]:
> +
> +"Attestation is used to provide information about the platform’s state
> +to a challenger. However, PCR contents are difficult to interpret;
> +therefore, attestation is typically more useful when the PCR contents
> +are accompanied by a measurement log. While not trusted on their own,
> +the measurement log contains a richer set of information than do the PCR
> +contents. The PCR contents are used to provide the validation of the
> +measurement log."
> +
> +UEFI event log
> +==============
> +
> +UEFI provided event log has a few somewhat weird quirks.
> +
> +Before calling ExitBootServices() Linux EFI stub copies the event log to
> +a custom configuration table defined by the stub itself. Unfortanely,
[again:] Unfortunately,
> +the events generated by ExitBootServices() don't end up in the table.
> +
> +The firmware provides so called final events configuration table to sort
> +out this issue. Events gets mirrored to this table after the first time
> +EFI_TCG2_PROTOCOL.GetEventLog() gets called.
> +
> +This introduces another problem: nothing guarantees that it is not called
> +before the Linux EFI stub gets to run. Thus, it needs to calculate and save the
> +final events table size while the stub is still running to the custom
> +configuration table so that the TPM driver can later on skip these events when
> +concatenating two halves of the event log from the custom configuration table
> +and the final events table.
> +
> +References
> +==========
> +
> +- [1] https://trustedcomputinggroup.org/resource/pc-client-specific-platform-firmware-profile-specification/
> +- [2] The final concatenation is done in drivers/char/tpm/eventlog/efi.c
>
--
~Randy
^ permalink raw reply
* [PATCH v3] tpm: Document UEFI event log quirks
From: Jarkko Sakkinen @ 2019-07-12 12:49 UTC (permalink / raw)
To: linux-kernel, linux-integrity, linux-doc
Cc: tweek, matthewgarrett, jorhand, rdunlap, Jarkko Sakkinen,
Jonathan Corbet, Sasha Levin
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 3567 bytes --]
There are some weird quirks when it comes to UEFI event log. Provide a
brief introduction to TPM event log mechanism and describe the quirks
and how they can be sorted out.
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
v3: Add a section and use bullet list for references. Remove (invalid)
author info.
v2: Fixed one type, adjusted the last paragraph and added the file
to index.rst
Documentation/security/tpm/index.rst | 1 +
Documentation/security/tpm/tpm_event_log.rst | 55 ++++++++++++++++++++
2 files changed, 56 insertions(+)
create mode 100644 Documentation/security/tpm/tpm_event_log.rst
diff --git a/Documentation/security/tpm/index.rst b/Documentation/security/tpm/index.rst
index 15783668644f..9e0815cb1e7f 100644
--- a/Documentation/security/tpm/index.rst
+++ b/Documentation/security/tpm/index.rst
@@ -4,5 +4,6 @@ Trusted Platform Module documentation
.. toctree::
+ tpm_event_log
tpm_ftpm_tee
tpm_vtpm_proxy
diff --git a/Documentation/security/tpm/tpm_event_log.rst b/Documentation/security/tpm/tpm_event_log.rst
new file mode 100644
index 000000000000..068eeb659bb9
--- /dev/null
+++ b/Documentation/security/tpm/tpm_event_log.rst
@@ -0,0 +1,55 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+=============
+TPM Event Log
+=============
+
+This document briefly describes what TPM log is and how it is handed
+over from the preboot firmware to the operating system.
+
+Introduction
+============
+
+The preboot firmware maintains an event log that gets new entries every
+time something gets hashed by it to any of the PCR registers. The events
+are segregated by their type and contain the value of the hashed PCR
+register. Typically, the preboot firmware will hash the components to
+who execution is to be handed over or actions relevant to the boot
+process.
+
+The main application for this is remote attestation and the reason why
+it is useful is nicely put in the very first section of [1]:
+
+"Attestation is used to provide information about the platform’s state
+to a challenger. However, PCR contents are difficult to interpret;
+therefore, attestation is typically more useful when the PCR contents
+are accompanied by a measurement log. While not trusted on their own,
+the measurement log contains a richer set of information than do the PCR
+contents. The PCR contents are used to provide the validation of the
+measurement log."
+
+UEFI event log
+==============
+
+UEFI provided event log has a few somewhat weird quirks.
+
+Before calling ExitBootServices() Linux EFI stub copies the event log to
+a custom configuration table defined by the stub itself. Unfortanely,
+the events generated by ExitBootServices() don't end up in the table.
+
+The firmware provides so called final events configuration table to sort
+out this issue. Events gets mirrored to this table after the first time
+EFI_TCG2_PROTOCOL.GetEventLog() gets called.
+
+This introduces another problem: nothing guarantees that it is not called
+before the Linux EFI stub gets to run. Thus, it needs to calculate and save the
+final events table size while the stub is still running to the custom
+configuration table so that the TPM driver can later on skip these events when
+concatenating two halves of the event log from the custom configuration table
+and the final events table.
+
+References
+==========
+
+- [1] https://trustedcomputinggroup.org/resource/pc-client-specific-platform-firmware-profile-specification/
+- [2] The final concatenation is done in drivers/char/tpm/eventlog/efi.c
--
2.20.1
^ permalink raw reply related
* [PATCH v2] tpm: Document UEFI event log quirks
From: Jarkko Sakkinen @ 2019-07-12 12:43 UTC (permalink / raw)
To: linux-kernel, linux-integrity, linux-doc
Cc: tweek, matthewgarrett, jorhand, rdunlap, Jarkko Sakkinen,
Jonathan Corbet, Sasha Levin
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 3510 bytes --]
There are some weird quirks when it comes to UEFI event log. Provide a
brief introduction to TPM event log mechanism and describe the quirks
and how they can be sorted out.
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
v2: Fixed one type, adjusted the last paragraph and added the file
to index.rst
Documentation/security/tpm/index.rst | 1 +
Documentation/security/tpm/tpm_event_log.rst | 56 ++++++++++++++++++++
2 files changed, 57 insertions(+)
create mode 100644 Documentation/security/tpm/tpm_event_log.rst
diff --git a/Documentation/security/tpm/index.rst b/Documentation/security/tpm/index.rst
index 15783668644f..9e0815cb1e7f 100644
--- a/Documentation/security/tpm/index.rst
+++ b/Documentation/security/tpm/index.rst
@@ -4,5 +4,6 @@ Trusted Platform Module documentation
.. toctree::
+ tpm_event_log
tpm_ftpm_tee
tpm_vtpm_proxy
diff --git a/Documentation/security/tpm/tpm_event_log.rst b/Documentation/security/tpm/tpm_event_log.rst
new file mode 100644
index 000000000000..b8c39a1a3f33
--- /dev/null
+++ b/Documentation/security/tpm/tpm_event_log.rst
@@ -0,0 +1,56 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+=============
+TPM Event Log
+=============
+
+| Authors:
+| Stefan Berger <stefanb@linux.vnet.ibm.com>
+
+This document briefly describes what TPM log is and how it is handed
+over from the preboot firmware to the operating system.
+
+Introduction
+============
+
+The preboot firmware maintains an event log that gets new entries every
+time something gets hashed by it to any of the PCR registers. The events
+are segregated by their type and contain the value of the hashed PCR
+register. Typically, the preboot firmware will hash the components to
+who execution is to be handed over or actions relevant to the boot
+process.
+
+The main application for this is remote attestation and the reason why
+it is useful is nicely put in the very first section of [1]:
+
+"Attestation is used to provide information about the platform’s state
+to a challenger. However, PCR contents are difficult to interpret;
+therefore, attestation is typically more useful when the PCR contents
+are accompanied by a measurement log. While not trusted on their own,
+the measurement log contains a richer set of information than do the PCR
+contents. The PCR contents are used to provide the validation of the
+measurement log."
+
+UEFI event log
+==============
+
+UEFI provided event log has a few somewhat weird quirks.
+
+Before calling ExitBootServices() Linux EFI stub copies the event log to
+a custom configuration table defined by the stub itself. Unfortanely,
+the events generated by ExitBootServices() don't end up in the table.
+
+The firmware provides so called final events configuration table to sort
+out this issue. Events gets mirrored to this table after the first time
+EFI_TCG2_PROTOCOL.GetEventLog() gets called.
+
+This introduces another problem: nothing guarantees that it is not called
+before the Linux EFI stub gets to run. Thus, it needs to calculate and save the
+final events table size while the stub is still running to the custom
+configuration table so that the TPM driver can later on skip these events when
+concatenating two halves of the event log from the custom configuration table
+and the final events table.
+
+[1]
+https://trustedcomputinggroup.org/resource/pc-client-specific-platform-firmware-profile-specification/
+[2] The final concatenation is done in drivers/char/tpm/eventlog/efi.c
--
2.20.1
^ permalink raw reply related
* Re: [PATCH] tpm: Document UEFI event log quirks
From: Jarkko Sakkinen @ 2019-07-12 12:41 UTC (permalink / raw)
To: Matthew Garrett
Cc: Linux Kernel Mailing List, linux-integrity, linux-doc,
Thiébaud Weksteen, Jonathan Corbet
In-Reply-To: <CACdnJuu0gFySbcMY7Fpps-j8KP+rCifznOeo18P47UBQAygPVQ@mail.gmail.com>
On Mon, Jul 08, 2019 at 01:43:14PM -0700, Matthew Garrett wrote:
> On Wed, Jul 3, 2019 at 9:11 AM Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> > +Before calling ExitBootServices() Linux EFI stub copies the event log to
> > +a custom configuration table defined by the stub itself. Unfortanely,
> > +the events generated by ExitBootServices() do end up to the table.
>
> "Unfortunately, the events generated by ExitBootServices() occur after
> this and don't end up in the table"?
Oops, it is a typo, thanks :-)
/Jarkko
^ permalink raw reply
* Re: [PATCH v2 0/3] x86/boot: Introduce the kernel_info et consortes
From: Daniel Kiper @ 2019-07-12 11:58 UTC (permalink / raw)
To: linux-doc, linux-kernel, x86
Cc: bp, corbet, dpsmith, eric.snowberg, hpa, kanth.ghatraju,
konrad.wilk, mingo, ross.philipson, tglx
In-Reply-To: <20190704163612.14311-1-daniel.kiper@oracle.com>
On Thu, Jul 04, 2019 at 06:36:09PM +0200, Daniel Kiper wrote:
> Hi,
>
> Due to very limited space in the setup_header this patch series introduces new
> kernel_info struct which will be used to convey information from the kernel to
> the bootloader. This way the boot protocol can be extended regardless of the
> setup_header limitations. Additionally, the patch series introduces some
> convenience features like the setup_indirect struct and the
> kernel_info.setup_type_max field.
Ping?
Daniel
^ permalink raw reply
* Re: [PATCH v7] Documentation: filesystem: Convert xfs.txt to ReST
From: Sheriff Esseson @ 2019-07-12 11:51 UTC (permalink / raw)
To: skhan
Cc: darrick.wong, linux-xfs, corbet, linux-doc, linux-kernel,
linux-kernel-mentees, sheriffesseson
Convert xfs.txt to ReST and fix broken references.
Signed-off-by: Sheriff Esseson <sheriffesseson@gmail.com>
---
Changes in v7:
- move xfs.rst to admin (Suggested by Matthew Wilcox).
- Update admin index.
- fix another typo (Caught by Matthew Wilcox).
Documentation/admin-guide/index.rst | 1 +
.../xfs.txt => admin-guide/xfs.rst} | 123 +++++++++---------
Documentation/filesystems/dax.txt | 2 +-
MAINTAINERS | 2 +-
4 files changed, 61 insertions(+), 67 deletions(-)
rename Documentation/{filesystems/xfs.txt => admin-guide/xfs.rst} (82%)
diff --git a/Documentation/admin-guide/index.rst b/Documentation/admin-guide/index.rst
index 24fbe0568eff..0615ea3a744c 100644
--- a/Documentation/admin-guide/index.rst
+++ b/Documentation/admin-guide/index.rst
@@ -70,6 +70,7 @@ configure specific aspects of kernel behavior to your liking.
ras
bcache
ext4
+ xfs
binderfs
pm/index
thunderbolt
diff --git a/Documentation/filesystems/xfs.txt b/Documentation/admin-guide/xfs.rst
similarity index 82%
rename from Documentation/filesystems/xfs.txt
rename to Documentation/admin-guide/xfs.rst
index a5cbb5e0e3db..9836ac3428f9 100644
--- a/Documentation/filesystems/xfs.txt
+++ b/Documentation/admin-guide/xfs.rst
@@ -1,4 +1,6 @@
+.. SPDX-License-Identifier: GPL-2.0
+======================
The SGI XFS Filesystem
======================
@@ -18,8 +20,6 @@ Mount Options
=============
When mounting an XFS filesystem, the following options are accepted.
-For boolean mount options, the names with the (*) suffix is the
-default behaviour.
allocsize=size
Sets the buffered I/O end-of-file preallocation size when
@@ -31,46 +31,43 @@ default behaviour.
preallocation size, which uses a set of heuristics to
optimise the preallocation size based on the current
allocation patterns within the file and the access patterns
- to the file. Specifying a fixed allocsize value turns off
+ to the file. Specifying a fixed ``allocsize`` value turns off
the dynamic behaviour.
- attr2
- noattr2
+ attr2 or noattr2
The options enable/disable an "opportunistic" improvement to
be made in the way inline extended attributes are stored
on-disk. When the new form is used for the first time when
- attr2 is selected (either when setting or removing extended
+ ``attr2`` is selected (either when setting or removing extended
attributes) the on-disk superblock feature bit field will be
updated to reflect this format being in use.
The default behaviour is determined by the on-disk feature
- bit indicating that attr2 behaviour is active. If either
- mount option it set, then that becomes the new default used
+ bit indicating that ``attr2`` behaviour is active. If either
+ mount option is set, then that becomes the new default used
by the filesystem.
- CRC enabled filesystems always use the attr2 format, and so
- will reject the noattr2 mount option if it is set.
+ CRC enabled filesystems always use the ``attr2`` format, and so
+ will reject the ``noattr2`` mount option if it is set.
- discard
- nodiscard (*)
+ discard or nodiscard (default)
Enable/disable the issuing of commands to let the block
device reclaim space freed by the filesystem. This is
useful for SSD devices, thinly provisioned LUNs and virtual
machine images, but may have a performance impact.
- Note: It is currently recommended that you use the fstrim
- application to discard unused blocks rather than the discard
+ Note: It is currently recommended that you use the ``fstrim``
+ application to ``discard`` unused blocks rather than the ``discard``
mount option because the performance impact of this option
is quite severe.
- grpid/bsdgroups
- nogrpid/sysvgroups (*)
+ grpid/bsdgroups or nogrpid/sysvgroups (default)
These options define what group ID a newly created file
- gets. When grpid is set, it takes the group ID of the
+ gets. When ``grpid`` is set, it takes the group ID of the
directory in which it is created; otherwise it takes the
- fsgid of the current process, unless the directory has the
- setgid bit set, in which case it takes the gid from the
- parent directory, and also gets the setgid bit set if it is
+ ``fsgid`` of the current process, unless the directory has the
+ ``setgid`` bit set, in which case it takes the ``gid`` from the
+ parent directory, and also gets the ``setgid`` bit set if it is
a directory itself.
filestreams
@@ -78,46 +75,42 @@ default behaviour.
across the entire filesystem rather than just on directories
configured to use it.
- ikeep
- noikeep (*)
- When ikeep is specified, XFS does not delete empty inode
- clusters and keeps them around on disk. When noikeep is
+ ikeep or noikeep (default)
+ When ``ikeep`` is specified, XFS does not delete empty inode
+ clusters and keeps them around on disk. When ``noikeep`` is
specified, empty inode clusters are returned to the free
space pool.
- inode32
- inode64 (*)
- When inode32 is specified, it indicates that XFS limits
+ inode32 or inode64 (default)
+ When ``inode32`` is specified, it indicates that XFS limits
inode creation to locations which will not result in inode
numbers with more than 32 bits of significance.
- When inode64 is specified, it indicates that XFS is allowed
+ When ``inode64`` is specified, it indicates that XFS is allowed
to create inodes at any location in the filesystem,
including those which will result in inode numbers occupying
- more than 32 bits of significance.
+ more than 32 bits of significance.
- inode32 is provided for backwards compatibility with older
+ ``inode32`` is provided for backwards compatibility with older
systems and applications, since 64 bits inode numbers might
cause problems for some applications that cannot handle
large inode numbers. If applications are in use which do
- not handle inode numbers bigger than 32 bits, the inode32
+ not handle inode numbers bigger than 32 bits, the ``inode32``
option should be specified.
-
- largeio
- nolargeio (*)
- If "nolargeio" is specified, the optimal I/O reported in
- st_blksize by stat(2) will be as small as possible to allow
+ largeio or nolargeio (default)
+ If ``nolargeio`` is specified, the optimal I/O reported in
+ ``st_blksize`` by **stat(2)** will be as small as possible to allow
user applications to avoid inefficient read/modify/write
I/O. This is typically the page size of the machine, as
this is the granularity of the page cache.
- If "largeio" specified, a filesystem that was created with a
- "swidth" specified will return the "swidth" value (in bytes)
- in st_blksize. If the filesystem does not have a "swidth"
- specified but does specify an "allocsize" then "allocsize"
+ If ``largeio`` is specified, a filesystem that was created with a
+ ``swidth`` specified will return the ``swidth`` value (in bytes)
+ in ``st_blksize``. If the filesystem does not have a ``swidth``
+ specified but does specify an ``allocsize`` then ``allocsize``
(in bytes) will be returned instead. Otherwise the behaviour
- is the same as if "nolargeio" was specified.
+ is the same as if ``nolargeio`` was specified.
logbufs=value
Set the number of in-memory log buffers. Valid numbers
@@ -127,7 +120,7 @@ default behaviour.
If the memory cost of 8 log buffers is too high on small
systems, then it may be reduced at some cost to performance
- on metadata intensive workloads. The logbsize option below
+ on metadata intensive workloads. The ``logbsize`` option below
controls the size of each buffer and so is also relevant to
this case.
@@ -138,7 +131,7 @@ default behaviour.
and 32768 (32k). Valid sizes for version 2 logs also
include 65536 (64k), 131072 (128k) and 262144 (256k). The
logbsize must be an integer multiple of the log
- stripe unit configured at mkfs time.
+ stripe unit configured at **mkfs(8)** time.
The default value for for version 1 logs is 32768, while the
default value for version 2 logs is MAX(32768, log_sunit).
@@ -153,21 +146,21 @@ default behaviour.
noalign
Data allocations will not be aligned at stripe unit
boundaries. This is only relevant to filesystems created
- with non-zero data alignment parameters (sunit, swidth) by
- mkfs.
+ with non-zero data alignment parameters (``sunit``, ``swidth``) by
+ **mkfs(8)**.
norecovery
The filesystem will be mounted without running log recovery.
If the filesystem was not cleanly unmounted, it is likely to
- be inconsistent when mounted in "norecovery" mode.
+ be inconsistent when mounted in ``norecovery`` mode.
Some files or directories may not be accessible because of this.
- Filesystems mounted "norecovery" must be mounted read-only or
+ Filesystems mounted ``norecovery`` must be mounted read-only or
the mount will fail.
nouuid
Don't check for double mounted file systems using the file
- system uuid. This is useful to mount LVM snapshot volumes,
- and often used in combination with "norecovery" for mounting
+ system ``uuid``. This is useful to mount LVM snapshot volumes,
+ and often used in combination with ``norecovery`` for mounting
read-only snapshots.
noquota
@@ -176,15 +169,15 @@ default behaviour.
uquota/usrquota/uqnoenforce/quota
User disk quota accounting enabled, and limits (optionally)
- enforced. Refer to xfs_quota(8) for further details.
+ enforced. Refer to **xfs_quota(8)** for further details.
gquota/grpquota/gqnoenforce
Group disk quota accounting enabled and limits (optionally)
- enforced. Refer to xfs_quota(8) for further details.
+ enforced. Refer to **xfs_quota(8)** for further details.
pquota/prjquota/pqnoenforce
Project disk quota accounting enabled and limits (optionally)
- enforced. Refer to xfs_quota(8) for further details.
+ enforced. Refer to **xfs_quota(8)** for further details.
sunit=value and swidth=value
Used to specify the stripe unit and width for a RAID device
@@ -192,11 +185,11 @@ default behaviour.
block units. These options are only relevant to filesystems
that were created with non-zero data alignment parameters.
- The sunit and swidth parameters specified must be compatible
+ The ``sunit`` and ``swidth`` parameters specified must be compatible
with the existing filesystem alignment characteristics. In
- general, that means the only valid changes to sunit are
- increasing it by a power-of-2 multiple. Valid swidth values
- are any integer multiple of a valid sunit value.
+ general, that means the only valid changes to ``sunit`` are
+ increasing it by a power-of-2 multiple. Valid ``swidth`` values
+ are any integer multiple of a valid ``sunit`` value.
Typically the only time these mount options are necessary if
after an underlying RAID device has had it's geometry
@@ -302,27 +295,27 @@ The following sysctls are available for the XFS filesystem:
fs.xfs.inherit_sync (Min: 0 Default: 1 Max: 1)
Setting this to "1" will cause the "sync" flag set
- by the xfs_io(8) chattr command on a directory to be
+ by the **xfs_io(8)** chattr command on a directory to be
inherited by files in that directory.
fs.xfs.inherit_nodump (Min: 0 Default: 1 Max: 1)
Setting this to "1" will cause the "nodump" flag set
- by the xfs_io(8) chattr command on a directory to be
+ by the **xfs_io(8)** chattr command on a directory to be
inherited by files in that directory.
fs.xfs.inherit_noatime (Min: 0 Default: 1 Max: 1)
Setting this to "1" will cause the "noatime" flag set
- by the xfs_io(8) chattr command on a directory to be
+ by the **xfs_io(8)** chattr command on a directory to be
inherited by files in that directory.
fs.xfs.inherit_nosymlinks (Min: 0 Default: 1 Max: 1)
Setting this to "1" will cause the "nosymlinks" flag set
- by the xfs_io(8) chattr command on a directory to be
+ by the **xfs_io(8)** chattr command on a directory to be
inherited by files in that directory.
fs.xfs.inherit_nodefrag (Min: 0 Default: 1 Max: 1)
Setting this to "1" will cause the "nodefrag" flag set
- by the xfs_io(8) chattr command on a directory to be
+ by the **xfs_io(8)** chattr command on a directory to be
inherited by files in that directory.
fs.xfs.rotorstep (Min: 1 Default: 1 Max: 256)
@@ -368,7 +361,7 @@ handler:
-error handlers:
Defines the behavior for a specific error.
-The filesystem behavior during an error can be set via sysfs files. Each
+The filesystem behavior during an error can be set via ``sysfs`` files. Each
error handler works independently - the first condition met by an error handler
for a specific class will cause the error to be propagated rather than reset and
retried.
@@ -419,7 +412,7 @@ level directory:
handler configurations.
Note: there is no guarantee that fail_at_unmount can be set while an
- unmount is in progress. It is possible that the sysfs entries are
+ unmount is in progress. It is possible that the ``sysfs`` entries are
removed by the unmounting filesystem before a "retry forever" error
handler configuration causes unmount to hang, and hence the filesystem
must be configured appropriately before unmount begins to prevent
@@ -428,7 +421,7 @@ level directory:
Each filesystem has specific error class handlers that define the error
propagation behaviour for specific errors. There is also a "default" error
handler defined, which defines the behaviour for all errors that don't have
-specific handlers defined. Where multiple retry constraints are configuredi for
+specific handlers defined. Where multiple retry constraints are configured for
a single error, the first retry configuration that expires will cause the error
to be propagated. The handler configurations are found in the directory:
@@ -463,7 +456,7 @@ to be propagated. The handler configurations are found in the directory:
Setting the value to "N" (where 0 < N < Max) will allow XFS to retry the
operation for up to "N" seconds before propagating the error.
-Note: The default behaviour for a specific error handler is dependent on both
+**Note:** The default behaviour for a specific error handler is dependent on both
the class and error context. For example, the default values for
"metadata/ENODEV" are "0" rather than "-1" so that this error handler defaults
to "fail immediately" behaviour. This is done because ENODEV is a fatal,
diff --git a/Documentation/filesystems/dax.txt b/Documentation/filesystems/dax.txt
index 6d2c0d340dea..679729442fd2 100644
--- a/Documentation/filesystems/dax.txt
+++ b/Documentation/filesystems/dax.txt
@@ -76,7 +76,7 @@ exposure of uninitialized data through mmap.
These filesystems may be used for inspiration:
- ext2: see Documentation/filesystems/ext2.txt
- ext4: see Documentation/filesystems/ext4/
-- xfs: see Documentation/filesystems/xfs.txt
+- xfs: see Documentation/admin-guide/xfs.rst
Handling Media Errors
diff --git a/MAINTAINERS b/MAINTAINERS
index 43ca94856944..3b6e0b6d8cbd 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17453,7 +17453,7 @@ L: linux-xfs@vger.kernel.org
W: http://xfs.org/
T: git git://git.kernel.org/pub/scm/fs/xfs/xfs-linux.git
S: Supported
-F: Documentation/filesystems/xfs.txt
+F: Documentation/admin-guide/xfs.rst
F: fs/xfs/
XILINX AXI ETHERNET DRIVER
--
2.22.0
^ permalink raw reply related
* RE: [PATCH v2 3/5] perf: stm32: ddrperfm driver creation
From: Gerald BAEZA @ 2019-07-12 10:00 UTC (permalink / raw)
To: Mark Rutland
Cc: Will Deacon, robh+dt@kernel.org, mcoquelin.stm32@gmail.com,
Alexandre TORGUE, corbet@lwn.net, linux@armlinux.org.uk,
olof@lixom.net, horms+renesas@verge.net.au, arnd@arndb.de,
linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org,
linux-stm32@st-md-mailman.stormreply.com,
linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org
In-Reply-To: <20190626122228.GB20313@lakrids.cambridge.arm.com>
Hi Mark
Thanks a lot for your review !
I started to rework the driver, based on your feedback but I have some questions for you: they are in line below... (where I also answer to your questions)
Best regards
Gérald
> -----Original Message-----
> From: Mark Rutland <Mark.Rutland@arm.com>
> Sent: mercredi 26 juin 2019 14:23
> To: Gerald BAEZA <gerald.baeza@st.com>
> Cc: Will Deacon <Will.Deacon@arm.com>; robh+dt@kernel.org;
> mcoquelin.stm32@gmail.com; Alexandre TORGUE
> <alexandre.torgue@st.com>; corbet@lwn.net; linux@armlinux.org.uk;
> olof@lixom.net; horms+renesas@verge.net.au; arnd@arndb.de; linux-arm-
> kernel@lists.infradead.org; devicetree@vger.kernel.org; linux-stm32@st-
> md-mailman.stormreply.com; linux-kernel@vger.kernel.org; linux-
> doc@vger.kernel.org
> Subject: Re: [PATCH v2 3/5] perf: stm32: ddrperfm driver creation
>
> On Mon, May 20, 2019 at 03:27:17PM +0000, Gerald BAEZA wrote:
> > The DDRPERFM is the DDR Performance Monitor embedded in STM32MP1
> SOC.
> >
> > This perf drivers supports the read, write, activate, idle and total
> > time counters, described in the reference manual RM0436.
>
> Is this document publicly accessible anywhere?
Yes
> If so, could you please provide a link?
https://www.st.com/resource/en/reference_manual/DM00327659.pdf
> > A 'bandwidth' attribute is added in the 'ddrperfm' event_source in
> > order to directly get the read and write bandwidths (in MB/s), from
> > the last read, write and total time counters reading.
> > This attribute is aside the 'events' attributes group because it is
> > not a counter, as seen by perf tool.
>
> This should be removed. This is unusually stateful, and this can be calculated
> in userspace by using the events. I'm also not keen on creating a precedent
> for weird sysfs bits for PMUs.
Ok, I will remove it in the v3.
This will also have impact on the bindings and dts since the DDR frequency
will disappear from the clocks.
Just to be sure : should I do the bandwidth computing on userspace (via
a script, for instance) or are you suggesting to do this in perf tool ?
> > Signed-off-by: Gerald Baeza <gerald.baeza@st.com>
> > ---
> > drivers/perf/Kconfig | 6 +
> > drivers/perf/Makefile | 1 +
> > drivers/perf/stm32_ddr_pmu.c | 512
> > +++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 519 insertions(+)
> > create mode 100644 drivers/perf/stm32_ddr_pmu.c
> >
> > diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig index
> > a94e586..9add8a7 100644
> > --- a/drivers/perf/Kconfig
> > +++ b/drivers/perf/Kconfig
> > @@ -105,6 +105,12 @@ config THUNDERX2_PMU
> > The SoC has PMU support in its L3 cache controller (L3C) and
> > in the DDR4 Memory Controller (DMC).
> >
> > +config STM32_DDR_PMU
> > + tristate "STM32 DDR PMU"
> > + depends on MACH_STM32MP157
> > + help
> > + Support for STM32 DDR performance monitor (DDRPERFM).
> > +
> > config XGENE_PMU
> > depends on ARCH_XGENE
> > bool "APM X-Gene SoC PMU"
> > diff --git a/drivers/perf/Makefile b/drivers/perf/Makefile index
> > 3048994..fa64719 100644
> > --- a/drivers/perf/Makefile
> > +++ b/drivers/perf/Makefile
> > @@ -8,6 +8,7 @@ obj-$(CONFIG_ARM_SMMU_V3_PMU) += arm_smmuv3_pmu.o
> > obj-$(CONFIG_HISI_PMU) += hisilicon/
> > obj-$(CONFIG_QCOM_L2_PMU)+= qcom_l2_pmu.o
> > obj-$(CONFIG_QCOM_L3_PMU) += qcom_l3_pmu.o
> > +obj-$(CONFIG_STM32_DDR_PMU) += stm32_ddr_pmu.o
> > obj-$(CONFIG_THUNDERX2_PMU) += thunderx2_pmu.o
> > obj-$(CONFIG_XGENE_PMU) += xgene_pmu.o
> > obj-$(CONFIG_ARM_SPE_PMU) += arm_spe_pmu.o diff --git
> > a/drivers/perf/stm32_ddr_pmu.c b/drivers/perf/stm32_ddr_pmu.c new file
> > mode 100644 index 0000000..ae4a813
> > --- /dev/null
> > +++ b/drivers/perf/stm32_ddr_pmu.c
> > @@ -0,0 +1,512 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * This file is the STM32 DDR performance monitor (DDRPERFM) driver
> > + *
> > + * Copyright (C) 2019, STMicroelectronics - All Rights Reserved
> > + * Author: Gerald Baeza <gerald.baeza@st.com> */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/hrtimer.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/perf_event.h>
> > +#include <linux/reset.h>
> > +#include <linux/slab.h>
> > +#include <linux/types.h>
> > +
> > +#define POLL_MS4000/* The counter roll over after ~8s @533MHz */
>
> I take it this is because there's no IRQ? If so, please expand the comment to
> call that out, e.g.
>
> /*
> * The PMU has no overflow IRQ, so we must poll to avoid losing events.
> * The fastest counter can overflow in ~8s @533MHz, so we poll in 4s
> * intervals to ensure we don't miss a rollover.
> */
> #define POLL_MS4000
The IP is able to freeze all counters and generate an interrupt when there is
a counter overflow. But, relying on this means that I lose all the events that
occur between the freeze and the interrupt handler execution, corresponding
to the interrupt latency. I can avoid such events losing in polling, so that is
why I preferred to use a polling mechanism.
> Which clock specifically is operating at 533MHz, and is this something that
> may vary? Is it possible that this may go higher in future?
533 MHz is the DDR frequency and this is the maximum for STM32MP15.
> > +#define WORD_LENGTH4/* Bytes */
> > +#define BURST_LENGTH8/* Words */
> > +
> > +#define DDRPERFM_CTL0x000
> > +#define DDRPERFM_CFG0x004
> > +#define DDRPERFM_STATUS 0x008
> > +#define DDRPERFM_CCR0x00C
> > +#define DDRPERFM_IER0x010
> > +#define DDRPERFM_ISR0x014
> > +#define DDRPERFM_ICR0x018
> > +#define DDRPERFM_TCNT0x020
> > +#define DDRPERFM_CNT(X)(0x030 + 8 * (X))
> > +#define DDRPERFM_HWCFG0x3F0
> > +#define DDRPERFM_VER0x3F4
> > +#define DDRPERFM_ID0x3F8
> > +#define DDRPERFM_SID0x3FC
> > +
> > +#define CTL_START0x00000001
> > +#define CTL_STOP0x00000002
> > +#define CCR_CLEAR_ALL0x8000000F
> > +#define SID_MAGIC_ID0xA3C5DD01
> > +
> > +#define STRING "Read = %llu, Write = %llu, Read & Write = %llu (MB/s)\n"
>
> If you need this, please expand it in-place. As-is, this is needless obfuscation.
This will be removed in v3, so no problem.
> > +
> > +enum {
> > +READ_CNT,
> > +WRITE_CNT,
> > +ACTIVATE_CNT,
> > +IDLE_CNT,
> > +TIME_CNT,
> > +PMU_NR_COUNTERS
> > +};
>
> I take it these are fixed-purpose counters in the hardware?
Yes
> > +
> > +struct stm32_ddr_pmu {
> > +struct pmu pmu;
> > +void __iomem *membase;
> > +struct clk *clk;
> > +struct clk *clk_ddr;
> > +unsigned long clk_ddr_rate;
> > +struct hrtimer hrtimer;
> > +ktime_t poll_period;
> > +spinlock_t lock; /* for shared registers access */
>
> All accesses to a PMU instance should be serialized on the same CPU, so you
> shouldn't need a lock (though you will need to disable IRQs to serialize with
> the interrupt handler).
>
> The perf subsystem cannot sanely access the PMU across multiple CPUs.
Ok, so I will only control the IP from one core and remove the lock in v3.
> > +struct perf_event *events[PMU_NR_COUNTERS];
> > +u64 events_cnt[PMU_NR_COUNTERS];
> > +};
> > +
> > +static inline struct stm32_ddr_pmu *pmu_to_stm32_ddr_pmu(struct
> pmu
> > +*p) { return container_of(p, struct stm32_ddr_pmu, pmu); }
> > +
> > +static inline struct stm32_ddr_pmu *hrtimer_to_stm32_ddr_pmu(struct
> > +hrtimer *h) { return container_of(h, struct stm32_ddr_pmu, hrtimer);
> > +}
> > +
> > +static u64 stm32_ddr_pmu_compute_bw(struct stm32_ddr_pmu
> *stm32_ddr_pmu,
> > + int counter)
> > +{
> > +u64 bw = stm32_ddr_pmu->events_cnt[counter], tmp;
> > +u64 div = stm32_ddr_pmu->events_cnt[TIME_CNT];
> > +u32 prediv = 1, premul = 1;
> > +
> > +if (bw && div) {
> > +/* Maximize the dividend into 64 bits */ while ((bw <
> > +0x8000000000000000ULL) &&
> > + (premul < 0x40000000UL)) {
> > +bw = bw << 1;
> > +premul *= 2;
> > +}
> > +/* Minimize the dividor to fit in 32 bits */ while ((div >
> > +0xffffffffUL) && (prediv < 0x40000000UL)) { div = div >> 1; prediv *=
> > +2; }
> > +/* Divide counter per time and multiply per DDR settings */
> > +do_div(bw, div); tmp = bw * BURST_LENGTH * WORD_LENGTH; tmp *=
> > +stm32_ddr_pmu->clk_ddr_rate; if (tmp < bw) goto error; bw = tmp;
> > +/* Cancel the prediv and premul factors */ while (prediv > 1) { bw =
> > +bw >> 1; prediv /= 2; } while (premul > 1) { bw = bw >> 1; premul /=
> > +2; }
> > +/* Convert MHz to Hz and B to MB, to finally get MB/s */ tmp = bw *
> > +1000000; if (tmp < bw) goto error; bw = tmp; premul = 1024 * 1024;
> > +while (premul > 1) { bw = bw >> 1; premul /= 2; } } return bw;
> > +
> > +error:
> > +pr_warn("stm32-ddr-pmu: overflow detected\n"); return 0; }
>
> IIUC this is for the stateful sysfs stuff, which should be removed.
>
Yes, you are right : to be removed in v3.
> > +
> > +static void stm32_ddr_pmu_event_configure(struct perf_event *event) {
> > +struct stm32_ddr_pmu *stm32_ddr_pmu =
> > +pmu_to_stm32_ddr_pmu(event->pmu); unsigned long lock_flags,
> > +config_base = event->hw.config_base;
> > +u32 val;
> > +
> > +spin_lock_irqsave(&stm32_ddr_pmu->lock, lock_flags);
> > +writel_relaxed(CTL_STOP, stm32_ddr_pmu->membase + DDRPERFM_CTL);
> > +
> > +if (config_base < TIME_CNT) {
> > +val = readl_relaxed(stm32_ddr_pmu->membase + DDRPERFM_CFG); val |= (1
> > +<< config_base); writel_relaxed(val, stm32_ddr_pmu->membase +
> > +DDRPERFM_CFG); } spin_unlock_irqrestore(&stm32_ddr_pmu->lock,
> > +lock_flags); }
>
> You don't need the lock here. This is called from your pmu::{start,add}
> callbacks, and the perf core already ensures those are serialized (via the
> context lock), and called with IRQs disabled.
>
Ok
> > +
> > +static void stm32_ddr_pmu_event_read(struct perf_event *event) {
> > +struct stm32_ddr_pmu *stm32_ddr_pmu =
> > +pmu_to_stm32_ddr_pmu(event->pmu); unsigned long flags, config_base =
> > +event->hw.config_base; struct hw_perf_event *hw = &event->hw;
> > +u64 prev_count, new_count, mask;
> > +u32 val, offset, bit;
> > +
> > +spin_lock_irqsave(&stm32_ddr_pmu->lock, flags);
> > +
> > +writel_relaxed(CTL_STOP, stm32_ddr_pmu->membase + DDRPERFM_CTL);
> > +
> > +if (config_base == TIME_CNT) {
> > +offset = DDRPERFM_TCNT;
> > +bit = 1 << 31;
> > +} else {
> > +offset = DDRPERFM_CNT(config_base);
> > +bit = 1 << config_base;
> > +}
> > +val = readl_relaxed(stm32_ddr_pmu->membase + DDRPERFM_STATUS);
> > +if (val & bit) pr_warn("stm32_ddr_pmu hardware overflow\n"); val =
> > +readl_relaxed(stm32_ddr_pmu->membase + offset); writel_relaxed(bit,
> > +stm32_ddr_pmu->membase + DDRPERFM_CCR);
> > + writel_relaxed(CTL_START, stm32_ddr_pmu->membase + DDRPERFM_CTL);
> > +
> > +do {
> > +prev_count = local64_read(&hw->prev_count); new_count = prev_count
> > +val; } while (local64_xchg(&hw->prev_count, new_count) !=
> > +prev_count);
> > +
> > +mask = GENMASK_ULL(31, 0);
> > +local64_add(val & mask, &event->count);
> > +
> > +if (new_count < prev_count)
> > +pr_warn("STM32 DDR PMU counter saturated\n");
>
> Do the counter saturate, or do they roll-over?
>
> I think that the message is misleading here, but I just want to check.
This check is on the software counter, that may roll-over after a very long capture time.
I took the "counter saturated" warning from "cache-l2x0-pmu.c" but I can change to
something more explicit.
The hardware counters, in DDRPERFM IP, are frozen in case of overflow (this is when
the interrupt mentioned above can be generated).
If this occurs, a "stm32_ddr_pmu hardware overflow" warning is generated.
Is this clearer ? Should I change anything ?
> > +
> > +spin_unlock_irqrestore(&stm32_ddr_pmu->lock, flags); }
> > +
> > +static void stm32_ddr_pmu_event_start(struct perf_event *event, int
> > +flags) { struct stm32_ddr_pmu *stm32_ddr_pmu =
> > +pmu_to_stm32_ddr_pmu(event->pmu);
> > +struct hw_perf_event *hw = &event->hw; unsigned long lock_flags;
> > +
> > +if (WARN_ON_ONCE(!(hw->state & PERF_HES_STOPPED))) return;
> > +
> > +if (flags & PERF_EF_RELOAD)
> > +WARN_ON_ONCE(!(hw->state & PERF_HES_UPTODATE));
> > +
> > +stm32_ddr_pmu_event_configure(event);
> > +
> > +/* Clear all counters to synchronize them, then start */
> > +spin_lock_irqsave(&stm32_ddr_pmu->lock, lock_flags);
> > +writel_relaxed(CCR_CLEAR_ALL, stm32_ddr_pmu->membase + DDRPERFM_CCR);
> > +writel_relaxed(CTL_START, stm32_ddr_pmu->membase + DDRPERFM_CTL);
> > +spin_unlock_irqrestore(&stm32_ddr_pmu->lock, lock_flags);
>
> By 'clear' do you mean set the counters to zero?
>
> Or is there a control bit that determine whether they count?
There is one "enable" bit per counter (except the total counter that is enabled by default).
"Clear" means that the counters are set to zero and the counting will only be started with
the "Start" for all enabled counters.
> If we're updating the counter, we could update prev_count at the same
> instant.
This synchronization is just for the start because the events are enabled & started one
per one by perf tool. Since each counter has to be compared to the "time" count that is
the overall reference for all, I want to ensure that "time" and all the enabled counters
finally start at the same time. Thus, this synchro.
Do you mean that, to be consistent, I should set prev_count to zero here also ?
>
> > +
> > +hw->state = 0;
> > +}
> > +
> > +static void stm32_ddr_pmu_event_stop(struct perf_event *event, int
> > +flags) { struct stm32_ddr_pmu *stm32_ddr_pmu =
> > +pmu_to_stm32_ddr_pmu(event->pmu);
> > +unsigned long lock_flags, config_base = event->hw.config_base; struct
> > +hw_perf_event *hw = &event->hw;
> > +u32 val, bit;
> > +
> > +if (WARN_ON_ONCE(hw->state & PERF_HES_STOPPED)) return;
> > +
> > +spin_lock_irqsave(&stm32_ddr_pmu->lock, lock_flags);
> > +writel_relaxed(CTL_STOP, stm32_ddr_pmu->membase +
> DDRPERFM_CTL); if
> > +(config_base == TIME_CNT) bit = 1 << 31; else bit = 1 << config_base;
> > +writel_relaxed(bit, stm32_ddr_pmu->membase + DDRPERFM_CCR); if
> > +(config_base < TIME_CNT) { val = readl_relaxed(stm32_ddr_pmu-
> >membase
> > ++ DDRPERFM_CFG); val &= ~bit; writel_relaxed(val,
> > +stm32_ddr_pmu->membase + DDRPERFM_CFG); }
> > +spin_unlock_irqrestore(&stm32_ddr_pmu->lock, lock_flags);
> > +
> > +hw->state |= PERF_HES_STOPPED;
> > +
> > +if (flags & PERF_EF_UPDATE) {
> > +stm32_ddr_pmu_event_read(event);
> > +hw->state |= PERF_HES_UPTODATE;
> > +}
> > +}
> > +
> > +static int stm32_ddr_pmu_event_add(struct perf_event *event, int
> > +flags) { struct stm32_ddr_pmu *stm32_ddr_pmu =
> > +pmu_to_stm32_ddr_pmu(event->pmu);
> > +unsigned long config_base = event->hw.config_base; struct
> > +hw_perf_event *hw = &event->hw;
> > +
> > +stm32_ddr_pmu->events_cnt[config_base] = 0;
> > +stm32_ddr_pmu->events[config_base] = event;
> > +
> > +clk_enable(stm32_ddr_pmu->clk);
> > +hrtimer_start(&stm32_ddr_pmu->hrtimer, stm32_ddr_pmu-poll_period,
> > + HRTIMER_MODE_REL);
> > +
> > +stm32_ddr_pmu_event_configure(event);
> > +
> > +hw->state = PERF_HES_STOPPED | PERF_HES_UPTODATE;
> > +
> > +if (flags & PERF_EF_START)
> > +stm32_ddr_pmu_event_start(event, 0);
> > +
> > +return 0;
> > +}
> > +
> > +static void stm32_ddr_pmu_event_del(struct perf_event *event, int
> > +flags) { struct stm32_ddr_pmu *stm32_ddr_pmu =
> > +pmu_to_stm32_ddr_pmu(event->pmu);
> > +unsigned long config_base = event->hw.config_base; bool stop = true;
> > +int i;
> > +
> > +stm32_ddr_pmu_event_stop(event, PERF_EF_UPDATE);
> > +
> > +stm32_ddr_pmu->events_cnt[config_base] +=
> > +local64_read(&event->count); stm32_ddr_pmu->events[config_base] =
> > +NULL;
> > +
> > +for (i = 0; i < PMU_NR_COUNTERS; i++) if (stm32_ddr_pmu->events[i])
> > +stop = false; if (stop) hrtimer_cancel(&stm32_ddr_pmu->hrtimer);
> > +
> > +clk_disable(stm32_ddr_pmu->clk);
>
> This doesn't look right. If I add two events A and B, then delete event A,
> surely we want the clock on for event B?
>
clk_enable() is called for each event enable and clk_disable() is called for
each event disable. So with your example we have:
+A -> clk_enable()
+B -> clk_enable()
-B -> clk_disable()
And the clock remains enabled until A is also deleted.
> Does the clock only affect whether the counters count, or does it also affect
> whether the register file is usable?
Both
>
> > +}
> > +
> > +static int stm32_ddr_pmu_event_init(struct perf_event *event) {
> > +struct hw_perf_event *hw = &event->hw;
> > +
> > +if (is_sampling_event(event))
> > +return -EINVAL;
> > +
> > +if (event->attach_state & PERF_ATTACH_TASK) return -EINVAL;
> > +
> > +if (event->attr.exclude_user ||
> > + event->attr.exclude_kernel ||
> > + event->attr.exclude_hv ||
> > + event->attr.exclude_idle ||
> > + event->attr.exclude_host ||
> > + event->attr.exclude_guest)
> > +return -EINVAL;
> > +
> > +if (event->cpu < 0)
> > +return -EINVAL;
>
> For a system PMU like this, you must ensure that all events are assigned to
> the same CPU.
>
> You'll need to designate some arbitrary CPU to mange the PMU, expose that
> under sysfs, and upon hotplug events you must choose a new CPU and
> migrate existing events.
>
> For a simple example, see arch/arm/mm/cache-l2x0-pmu.c.
Among CPU#0 and CPU#1, the CPU#0 is the only one that can be running alone
(in other words, we disable CPU#1 before low power transition and CPU#0 will
finally enter low power).
So I think I can avoid to manage any migration by systematically launching the PMU
on CPU#0.
Is this fine for you or do you anyway prefer a generic approach that I could find
in arch/arm/mm/cache-l2x0-pmu.c ?
> > +
> > +hw->config_base = event->attr.config;
> > +
> > +return 0;
> > +}
> > +
> > +static enum hrtimer_restart stm32_ddr_pmu_poll(struct hrtimer
> > +*hrtimer) { struct stm32_ddr_pmu *stm32_ddr_pmu =
> > +hrtimer_to_stm32_ddr_pmu(hrtimer);
> > +int i;
> > +
> > +for (i = 0; i < PMU_NR_COUNTERS; i++) if (stm32_ddr_pmu->events[i])
> > +stm32_ddr_pmu_event_read(stm32_ddr_pmu->events[i]);
> > +
> > +hrtimer_forward_now(hrtimer, stm32_ddr_pmu->poll_period);
> > +
> > +return HRTIMER_RESTART;
> > +}
> > +
> > +static ssize_t stm32_ddr_pmu_sysfs_show(struct device *dev, struct
> > +device_attribute *attr, char *buf) { struct dev_ext_attribute *eattr;
> > +
> > +eattr = container_of(attr, struct dev_ext_attribute, attr);
> > +
> > +return sprintf(buf, "config=0x%lx\n", (unsigned long)eattr->var); }
> > +
> > +static ssize_t bandwidth_show(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buf)
> > +{
> > +struct stm32_ddr_pmu *stm32_ddr_pmu = dev_get_drvdata(dev);
> > +u64 r_bw, w_bw;
> > +int ret;
> > +
> > +if (stm32_ddr_pmu->events_cnt[TIME_CNT]) { r_bw =
> > +stm32_ddr_pmu_compute_bw(stm32_ddr_pmu, READ_CNT); w_bw =
> > +stm32_ddr_pmu_compute_bw(stm32_ddr_pmu, WRITE_CNT);
> > +
> > +ret = snprintf(buf, PAGE_SIZE, STRING,
> > + r_bw, w_bw, (r_bw + w_bw));
> > +} else {
> > +ret = snprintf(buf, PAGE_SIZE, "No data available\n"); }
> > +
> > +return ret;
> > +}
>
> As commented above, this should not be exposed under sysfs. It doesn't
> follow the usual ABI rules, it's weirdly stateful, and it's redundant.
>
Ok
> > +
> > +#define STM32_DDR_PMU_ATTR(_name, _func, _config)\ (&((struct
> > +dev_ext_attribute[]) {\
> > +{ __ATTR(_name, 0444, _func, NULL), (void *)_config } \
> > +})[0].attr.attr)
> > +
> > +#define STM32_DDR_PMU_EVENT_ATTR(_name, _config)\
> > +STM32_DDR_PMU_ATTR(_name, stm32_ddr_pmu_sysfs_show,\
> > + (unsigned long)_config)
> > +
> > +static struct attribute *stm32_ddr_pmu_event_attrs[] = {
> > +STM32_DDR_PMU_EVENT_ATTR(read_cnt, READ_CNT),
> > +STM32_DDR_PMU_EVENT_ATTR(write_cnt, WRITE_CNT),
> > +STM32_DDR_PMU_EVENT_ATTR(activate_cnt, ACTIVATE_CNT),
> > +STM32_DDR_PMU_EVENT_ATTR(idle_cnt, IDLE_CNT),
> > +STM32_DDR_PMU_EVENT_ATTR(time_cnt, TIME_CNT), NULL };
> > +
> > +static DEVICE_ATTR_RO(bandwidth);
> > +static struct attribute *stm32_ddr_pmu_bandwidth_attrs[] = {
> > +&dev_attr_bandwidth.attr, NULL, };
> > +
> > +static struct attribute_group stm32_ddr_pmu_event_attrs_group = {
> > +.name = "events", .attrs = stm32_ddr_pmu_event_attrs, };
> > +
> > +static struct attribute_group stm32_ddr_pmu_bandwidth_attrs_group = {
> > +.attrs = stm32_ddr_pmu_bandwidth_attrs, };
> > +
> > +static const struct attribute_group *stm32_ddr_pmu_attr_groups[] = {
> > +&stm32_ddr_pmu_event_attrs_group,
> > +&stm32_ddr_pmu_bandwidth_attrs_group,
> > +NULL,
> > +};
> > +
> > +static int stm32_ddr_pmu_device_probe(struct platform_device *pdev) {
> > +struct stm32_ddr_pmu *stm32_ddr_pmu; struct reset_control *rst;
> > +struct resource *res; int i, ret;
> > +u32 val;
> > +
> > +stm32_ddr_pmu = devm_kzalloc(&pdev->dev, sizeof(struct
> stm32_ddr_pmu),
> > + GFP_KERNEL);
> > +if (!stm32_ddr_pmu)
> > +return -ENOMEM;
> > +platform_set_drvdata(pdev, stm32_ddr_pmu);
> > +
> > +res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +stm32_ddr_pmu->membase = devm_ioremap_resource(&pdev->dev,
> res); if
> > +(IS_ERR(stm32_ddr_pmu->membase)) { pr_warn("Unable to get STM32
> DDR
> > +PMU membase\n"); return PTR_ERR(stm32_ddr_pmu->membase); }
> > +
> > +stm32_ddr_pmu->clk = devm_clk_get(&pdev->dev, "bus"); if
> > +(IS_ERR(stm32_ddr_pmu->clk)) { pr_warn("Unable to get STM32 DDR
> PMU
> > +clock\n"); return PTR_ERR(stm32_ddr_pmu->clk); }
> > +
> > +ret = clk_prepare_enable(stm32_ddr_pmu->clk);
> > +if (ret) {
> > +pr_warn("Unable to prepare STM32 DDR PMU clock\n"); return ret; }
> > +
> > +stm32_ddr_pmu->clk_ddr = devm_clk_get(&pdev->dev, "ddr"); if
> > +(IS_ERR(stm32_ddr_pmu->clk_ddr)) { pr_warn("Unable to get STM32
> DDR
> > +clock\n"); return PTR_ERR(stm32_ddr_pmu->clk_ddr); }
> > +stm32_ddr_pmu->clk_ddr_rate = clk_get_rate(stm32_ddr_pmu-
> >clk_ddr);
> > +stm32_ddr_pmu->clk_ddr_rate /= 1000000;
> > +
> > +stm32_ddr_pmu->poll_period = ms_to_ktime(POLL_MS);
> > +hrtimer_init(&stm32_ddr_pmu->hrtimer, CLOCK_MONOTONIC,
> > + HRTIMER_MODE_REL);
> > +stm32_ddr_pmu->hrtimer.function = stm32_ddr_pmu_poll;
> > +spin_lock_init(&stm32_ddr_pmu->lock);
> > +
> > +for (i = 0; i < PMU_NR_COUNTERS; i++) { stm32_ddr_pmu->events[i] =
> > +NULL; stm32_ddr_pmu->events_cnt[i] = 0; }
> > +
> > +val = readl_relaxed(stm32_ddr_pmu->membase + DDRPERFM_SID); if
> (val
> > +!= SID_MAGIC_ID) return -EINVAL;
> > +
> > +stm32_ddr_pmu->pmu = (struct pmu) {
> > +.task_ctx_nr = perf_invalid_context,
> > +.start = stm32_ddr_pmu_event_start,
> > +.stop = stm32_ddr_pmu_event_stop,
> > +.add = stm32_ddr_pmu_event_add,
> > +.del = stm32_ddr_pmu_event_del,
> > +.event_init = stm32_ddr_pmu_event_init, .attr_groups =
> > +stm32_ddr_pmu_attr_groups, }; ret =
> > +perf_pmu_register(&stm32_ddr_pmu->pmu, "ddrperfm", -1);
>
> Please give this a better name. The usual convention is to use "_pmu" as the
> suffix, so we should use that rather than "perfm".
>
> To be unambiguous, something like "stm32_ddr_pmu" would be good.
Ok, understood
>
> Thanks,
> Mark.
>
> > +if (ret) {
> > +pr_warn("Unable to register STM32 DDR PMU\n"); return ret; }
> > +
> > +rst = devm_reset_control_get_exclusive(&pdev->dev, NULL); if
> > +(!IS_ERR(rst)) { reset_control_assert(rst); udelay(2);
> > +reset_control_deassert(rst); }
> > +
> > +pr_info("stm32-ddr-pmu: probed (ID=0x%08x VER=0x%08x),
> DDR@%luMHz\n",
> > +readl_relaxed(stm32_ddr_pmu->membase + DDRPERFM_ID),
> > +readl_relaxed(stm32_ddr_pmu->membase + DDRPERFM_VER),
> > +stm32_ddr_pmu->clk_ddr_rate);
> > +
> > +clk_disable(stm32_ddr_pmu->clk);
> > +
> > +return 0;
> > +}
> > +
> > +static int stm32_ddr_pmu_device_remove(struct platform_device *pdev)
> > +{ struct stm32_ddr_pmu *stm32_ddr_pmu =
> platform_get_drvdata(pdev);
> > +
> > +perf_pmu_unregister(&stm32_ddr_pmu->pmu);
> > +
> > +return 0;
> > +}
> > +
> > +static const struct of_device_id stm32_ddr_pmu_of_match[] = { {
> > +.compatible = "st,stm32-ddr-pmu" }, { }, };
> > +
> > +static struct platform_driver stm32_ddr_pmu_driver = { .driver = {
> > +.name= "stm32-ddr-pmu", .of_match_table =
> > +of_match_ptr(stm32_ddr_pmu_of_match),
> > +},
> > +.probe = stm32_ddr_pmu_device_probe,
> > +.remove = stm32_ddr_pmu_device_remove, };
> > +
> > +module_platform_driver(stm32_ddr_pmu_driver);
> > +
> > +MODULE_DESCRIPTION("Perf driver for STM32 DDR performance
> monitor");
> > +MODULE_AUTHOR("Gerald Baeza <gerald.baeza@st.com>");
> > +MODULE_LICENSE("GPL v2");
> > --
> > 2.7.4
> IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended recipient,
> please notify the sender immediately and do not disclose the contents to any
> other person, use it for any purpose, or store or copy the information in any
> medium. Thank you.
^ permalink raw reply
* [PATCH] doc:it_IT: translations in process/
From: Federico Vaga @ 2019-07-12 9:48 UTC (permalink / raw)
To: Jonathan Corbet; +Cc: linux-doc, linux-kernel, Federico Vaga
This patch add translations for:
- programming-languages
- kernel-docs (It is better to not translate this since English is
a requirement to get something useful out of it)
Signed-off-by: Federico Vaga <federico.vaga@vaga.pv.it>
---
.../translations/it_IT/process/index.rst | 1 +
.../it_IT/process/kernel-docs.rst | 11 ++--
.../it_IT/process/programming-language.rst | 51 +++++++++++++++++++
3 files changed, 60 insertions(+), 3 deletions(-)
create mode 100644 Documentation/translations/it_IT/process/programming-language.rst
diff --git a/Documentation/translations/it_IT/process/index.rst b/Documentation/translations/it_IT/process/index.rst
index 2eda85d5cd1e..012de0f3154a 100644
--- a/Documentation/translations/it_IT/process/index.rst
+++ b/Documentation/translations/it_IT/process/index.rst
@@ -27,6 +27,7 @@ Di seguito le guide che ogni sviluppatore dovrebbe leggere.
code-of-conduct
development-process
submitting-patches
+ programming-language
coding-style
maintainer-pgp-guide
email-clients
diff --git a/Documentation/translations/it_IT/process/kernel-docs.rst b/Documentation/translations/it_IT/process/kernel-docs.rst
index 7bd70d661737..38e0a955121a 100644
--- a/Documentation/translations/it_IT/process/kernel-docs.rst
+++ b/Documentation/translations/it_IT/process/kernel-docs.rst
@@ -1,6 +1,7 @@
.. include:: ../disclaimer-ita.rst
:Original: :ref:`Documentation/process/kernel-docs.rst <kernel_docs>`
+:Translator: Federico Vaga <federico.vaga@vaga.pv.it>
.. _it_kernel_docs:
@@ -8,6 +9,10 @@
Indice di documenti per le persone interessate a capire e/o scrivere per il kernel Linux
========================================================================================
-.. warning::
-
- TODO ancora da tradurre
+.. note::
+ Questo documento contiene riferimenti a documenti in lingua inglese; inoltre
+ utilizza dai campi *ReStructuredText* di supporto alla ricerca e che per
+ questo motivo è meglio non tradurre al fine di garantirne un corretto
+ utilizzo.
+ Per questi motivi il documento non verrà tradotto. Per favore fate
+ riferimento al documento originale in lingua inglese.
diff --git a/Documentation/translations/it_IT/process/programming-language.rst b/Documentation/translations/it_IT/process/programming-language.rst
new file mode 100644
index 000000000000..f4b006395849
--- /dev/null
+++ b/Documentation/translations/it_IT/process/programming-language.rst
@@ -0,0 +1,51 @@
+.. include:: ../disclaimer-ita.rst
+
+:Original: :ref:`Documentation/process/programming-language.rst <programming_language>`
+:Translator: Federico Vaga <federico.vaga@vaga.pv.it>
+
+.. _it_programming_language:
+
+Linguaggio di programmazione
+============================
+
+Il kernel è scritto nel linguaggio di programmazione C [c-language]_.
+Più precisamente, il kernel viene compilato con ``gcc`` [gcc]_ usando
+l'opzione ``-std=gnu89`` [gcc-c-dialect-options]_: il dialetto GNU
+dello standard ISO C90 (con l'aggiunta di alcune funzionalità da C99)
+
+Questo dialetto contiene diverse estensioni al linguaggio [gnu-extensions]_,
+e molte di queste vengono usate sistematicamente dal kernel.
+
+Il kernel offre un certo livello di supporto per la compilazione con ``clang``
+[clang]_ e ``icc`` [icc]_ su diverse architetture, tuttavia in questo momento
+il supporto non è completo e richiede delle patch aggiuntive.
+
+Attributi
+---------
+
+Una delle estensioni più comuni e usate nel kernel sono gli attributi
+[gcc-attribute-syntax]_. Gli attributi permettono di aggiungere una semantica,
+definita dell'implementazione, alle entità del linguaggio (come le variabili,
+le funzioni o i tipi) senza dover fare importanti modifiche sintattiche al
+linguaggio stesso (come l'aggiunta di nuove parole chiave) [n2049]_.
+
+In alcuni casi, gli attributi sono opzionali (ovvero un compilatore che non
+dovesse supportarli dovrebbe produrre comunque codice corretto, anche se
+più lento o che non esegue controlli aggiuntivi durante la compilazione).
+
+Il kernel definisce alcune pseudo parole chiave (per esempio ``__pure``)
+in alternativa alla sintassi GNU per gli attributi (per esempio
+``__attribute__((__pure__))``) allo scopo di mostrare quali funzionalità si
+possono usare e/o per accorciare il codice.
+
+Per maggiori informazioni consultate il file d'intestazione
+``include/linux/compiler_attributes.h``.
+
+.. [c-language] http://www.open-std.org/jtc1/sc22/wg14/www/standards
+.. [gcc] https://gcc.gnu.org
+.. [clang] https://clang.llvm.org
+.. [icc] https://software.intel.com/en-us/c-compilers
+.. [gcc-c-dialect-options] https://gcc.gnu.org/onlinedocs/gcc/C-Dialect-Options.html
+.. [gnu-extensions] https://gcc.gnu.org/onlinedocs/gcc/C-Extensions.html
+.. [gcc-attribute-syntax] https://gcc.gnu.org/onlinedocs/gcc/Attribute-Syntax.html
+.. [n2049] http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2049.pdf
--
2.21.0
^ permalink raw reply related
* [PATCH] Documentation: proc.txt: emphasize that iowait cannot be relied on
From: Alan Jenkins @ 2019-07-12 8:22 UTC (permalink / raw)
To: linux-doc, Jonathan Corbet
Cc: Doug Smythies, Peter Zijlstra, linux-kernel, Alan Jenkins
CPU "iowait" time in /proc/stat does not work on my laptop.
I saw the documentation mention several problems with "iowait". However
each problem appeared to be qualified. It gave me the impression I could
probably account for each problem. My impression was wrong.
There are a couple of writeups explaining the specific problem I had.[1][2]
[1] "[RFC PATCH 0/8] rework iowait accounting", 2014-06-26:
https://lore.kernel.org/lkml/53ABE28F.6010402@jp.fujitsu.com/
[2] A recent writeup by myself:
https://unix.stackexchange.com/questions/517757/my-basic-assumption-about-system-iowait-does-not-hold/527836#527836
This might just be me. Partly, my small knowledge about the scheduler
allowed for false assumptions. But I think we can emphasize more strongly
how broken iowait is on SMP. Overall, I aim to make it sound much scarier
to analyze iowait. I add some precise details, and also some
anxiety-inducing vagueness :-).
[Detailed reasons for the specific points I included:]
1. Let us say that "iowait _can_ be massively under-accounted". It is
likely to remain true in future. At least since v4.16, the
under-accounting problem seems very exposed on non-virtual, multi-CPU
systems. In theory the wheel might turn again; this exposure might be
reduced in future. But even on v4.15, I can reproduce the problem using
CPU affinity.
2. Point to NO_HZ_IDLE, as a good hint towards i) the nature of the problem
and ii) and how widespread it is. To give a more comprehensive picture,
also point to NO_HZ_FULL and VIRT_CPU_ACCOUNTING_NATIVE.
Setting down my exact scenario would require a lot of specifics. That
would be going beyond the point. We could link to one of the writeups as
well, but I don't think we need to.
3. My own "use case" did not expose the problem when I ran it on a virtual
machine. Even using my CPU affinity method.[2] I haven't tracked down
why. This is a significant qualification to point 1. Explicitly
acknowledge this. It's a pain, but it makes the main point easier to
verify, and hence more credible.
(I suspect this is common at least to small test VMs. It appears true
for both a Fedora 30 VM (5.1.x) and a Debian 9 VM (4.9.x). I also tried
some different storage options, virtio-blk v.s. virtio-scsi v.s. isilogic.)
[:end of details]
Signed-off-by: Alan Jenkins <alan.christopher.jenkins@gmail.com>
---
Documentation/filesystems/proc.txt | 25 ++++++++++++++++---------
1 file changed, 16 insertions(+), 9 deletions(-)
diff --git a/Documentation/filesystems/proc.txt b/Documentation/filesystems/proc.txt
index 66cad5c86171..f1da71cd276e 100644
--- a/Documentation/filesystems/proc.txt
+++ b/Documentation/filesystems/proc.txt
@@ -1348,16 +1348,23 @@ second). The meanings of the columns are as follows, from left to right:
- nice: niced processes executing in user mode
- system: processes executing in kernel mode
- idle: twiddling thumbs
-- iowait: In a word, iowait stands for waiting for I/O to complete. But there
- are several problems:
- 1. Cpu will not wait for I/O to complete, iowait is the time that a task is
- waiting for I/O to complete. When cpu goes into idle state for
- outstanding task io, another task will be scheduled on this CPU.
- 2. In a multi-core CPU, the task waiting for I/O to complete is not running
- on any CPU, so the iowait of each CPU is difficult to calculate.
- 3. The value of iowait field in /proc/stat will decrease in certain
+- iowait: In a word, iowait stands for waiting for I/O to complete. This
+ number is not reliable. The problems include:
+ 1. A CPU does not wait for I/O to complete; iowait is the time that a task
+ is waiting for I/O to complete. When a CPU goes into idle state for
+ outstanding task I/O, another task will be scheduled on this CPU.
+ 2. iowait was extended to support systems with multiple CPUs. But the
+ extended version is misleading. Consider a two-CPU system, where you see
+ 50% iowait. This could represent two tasks that could use 100% of both
+ CPUs, if they were not waiting for I/O.
+ 3. iowait can be massively under-accounted on modern kernels. The iowait
+ code does not account for the behaviour of NO_HZ_IDLE, NO_HZ_FULL, or
+ VIRT_CPU_ACCOUNTING_NATIVE on multi-CPU systems. The amount of
+ under-accounting varies depending on the exact system configuration and
+ kernel version. The effects might be less obvious when running in a
+ virtual machine.
+ 4. The value of iowait field in /proc/stat will decrease in certain
conditions.
- So, the iowait is not reliable by reading from /proc/stat.
- irq: servicing interrupts
- softirq: servicing softirqs
- steal: involuntary wait
--
2.21.0
^ permalink raw reply related
* [PATCH v9 00/18] kunit: introduce KUnit, the Linux kernel unit testing framework
From: Brendan Higgins @ 2019-07-12 8:17 UTC (permalink / raw)
To: frowand.list, gregkh, jpoimboe, keescook, kieran.bingham, mcgrof,
peterz, robh, sboyd, shuah, tytso, yamada.masahiro
Cc: devicetree, dri-devel, kunit-dev, linux-doc, linux-fsdevel,
linux-kbuild, linux-kernel, linux-kselftest, linux-nvdimm,
linux-um, Alexander.Levin, Tim.Bird, amir73il, dan.carpenter,
daniel, jdike, joel, julia.lawall, khilman, knut.omang, logang,
mpe, pmladek, rdunlap, richard, rientjes, rostedt, wfg,
Brendan Higgins, Michal Marek, Jonathan Corbet, Iurii Zaikin
## TL;DR
This new patch set only contains a very minor change to address a sparse
warning in the PROC SYSCTL KUnit test. Otherwise this patchset is
identical to the previous.
As I mentioned in the previous patchset, all patches now have acks and
reviews.
## Background
This patch set proposes KUnit, a lightweight unit testing and mocking
framework for the Linux kernel.
Unlike Autotest and kselftest, KUnit is a true unit testing framework;
it does not require installing the kernel on a test machine or in a VM
(however, KUnit still allows you to run tests on test machines or in VMs
if you want[1]) and does not require tests to be written in userspace
running on a host kernel. Additionally, KUnit is fast: From invocation
to completion KUnit can run several dozen tests in about a second.
Currently, the entire KUnit test suite for KUnit runs in under a second
from the initial invocation (build time excluded).
KUnit is heavily inspired by JUnit, Python's unittest.mock, and
Googletest/Googlemock for C++. KUnit provides facilities for defining
unit test cases, grouping related test cases into test suites, providing
common infrastructure for running tests, mocking, spying, and much more.
### What's so special about unit testing?
A unit test is supposed to test a single unit of code in isolation,
hence the name. There should be no dependencies outside the control of
the test; this means no external dependencies, which makes tests orders
of magnitudes faster. Likewise, since there are no external dependencies,
there are no hoops to jump through to run the tests. Additionally, this
makes unit tests deterministic: a failing unit test always indicates a
problem. Finally, because unit tests necessarily have finer granularity,
they are able to test all code paths easily solving the classic problem
of difficulty in exercising error handling code.
### Is KUnit trying to replace other testing frameworks for the kernel?
No. Most existing tests for the Linux kernel are end-to-end tests, which
have their place. A well tested system has lots of unit tests, a
reasonable number of integration tests, and some end-to-end tests. KUnit
is just trying to address the unit test space which is currently not
being addressed.
### More information on KUnit
There is a bunch of documentation near the end of this patch set that
describes how to use KUnit and best practices for writing unit tests.
For convenience I am hosting the compiled docs here[2].
Additionally for convenience, I have applied these patches to a
branch[3]. The repo may be cloned with:
git clone https://kunit.googlesource.com/linux
This patchset is on the kunit/rfc/v5.2/v9 branch.
## Changes Since Last Version
Like I said in the TL;DR, there is only one minor change since the
previous revision. That change only affects patch 17/18; it addresses a
sparse warning in the PROC SYSCTL unit test.
Thanks to Masahiro for applying previous patches to a branch in his
kbuild tree and running sparse and other static analysis tools against
my patches.
[1] https://google.github.io/kunit-docs/third_party/kernel/docs/usage.html#kunit-on-non-uml-architectures
[2] https://google.github.io/kunit-docs/third_party/kernel/docs/
[3] https://kunit.googlesource.com/linux/+/kunit/rfc/v5.2/v9
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply
* [PATCH v9 04/18] kunit: test: add kunit_stream a std::stream like logger
From: Brendan Higgins @ 2019-07-12 8:17 UTC (permalink / raw)
To: frowand.list, gregkh, jpoimboe, keescook, kieran.bingham, mcgrof,
peterz, robh, sboyd, shuah, tytso, yamada.masahiro
Cc: devicetree, dri-devel, kunit-dev, linux-doc, linux-fsdevel,
linux-kbuild, linux-kernel, linux-kselftest, linux-nvdimm,
linux-um, Alexander.Levin, Tim.Bird, amir73il, dan.carpenter,
daniel, jdike, joel, julia.lawall, khilman, knut.omang, logang,
mpe, pmladek, rdunlap, richard, rientjes, rostedt, wfg,
Brendan Higgins
In-Reply-To: <20190712081744.87097-1-brendanhiggins@google.com>
A lot of the expectation and assertion infrastructure prints out fairly
complicated test failure messages, so add a C++ style log library for
for logging test results.
Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
---
include/kunit/kunit-stream.h | 81 +++++++++++++++++++++++
include/kunit/test.h | 3 +
kunit/Makefile | 3 +-
kunit/kunit-stream.c | 123 +++++++++++++++++++++++++++++++++++
kunit/test.c | 6 ++
5 files changed, 215 insertions(+), 1 deletion(-)
create mode 100644 include/kunit/kunit-stream.h
create mode 100644 kunit/kunit-stream.c
diff --git a/include/kunit/kunit-stream.h b/include/kunit/kunit-stream.h
new file mode 100644
index 0000000000000..a7b53eabf6be4
--- /dev/null
+++ b/include/kunit/kunit-stream.h
@@ -0,0 +1,81 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * C++ stream style string formatter and printer used in KUnit for outputting
+ * KUnit messages.
+ *
+ * Copyright (C) 2019, Google LLC.
+ * Author: Brendan Higgins <brendanhiggins@google.com>
+ */
+
+#ifndef _KUNIT_KUNIT_STREAM_H
+#define _KUNIT_KUNIT_STREAM_H
+
+#include <linux/types.h>
+#include <kunit/string-stream.h>
+
+struct kunit;
+
+/**
+ * struct kunit_stream - a std::stream style string builder.
+ *
+ * A std::stream style string builder. Allows messages to be built up and
+ * printed all at once.
+ */
+struct kunit_stream {
+ /* private: internal use only. */
+ struct kunit *test;
+ const char *level;
+ struct string_stream *internal_stream;
+};
+
+/**
+ * alloc_kunit_stream() - constructs a new &struct kunit_stream.
+ * @test: The test context object.
+ * @level: The log level at which to print out the message.
+ *
+ * Constructs a new test managed &struct kunit_stream.
+ */
+struct kunit_stream *alloc_kunit_stream(struct kunit *test, const char *level);
+
+/**
+ * kunit_stream_add(): adds the formatted input to the internal buffer.
+ * @kstream: the stream being operated on.
+ * @fmt: printf style format string to append to stream.
+ *
+ * Appends the formatted string, @fmt, to the internal buffer.
+ */
+void __printf(2, 3) kunit_stream_add(struct kunit_stream *kstream,
+ const char *fmt, ...);
+
+/**
+ * kunit_stream_append(): appends the contents of @other to @kstream.
+ * @kstream: the stream to which @other is appended.
+ * @other: the stream whose contents are appended to @kstream.
+ *
+ * Appends the contents of @other to @kstream.
+ */
+void kunit_stream_append(struct kunit_stream *kstream,
+ struct kunit_stream *other);
+
+/**
+ * kunit_stream_commit(): prints out the internal buffer to the user.
+ * @kstream: the stream being operated on.
+ *
+ * Outputs the contents of the internal buffer as a kunit_printk formatted
+ * output. KUNIT_STREAM ONLY OUTPUTS ITS BUFFER TO THE USER IF COMMIT IS
+ * CALLED!!! The reason for this is that it allows us to construct a message
+ * before we know whether we want to print it out; this can be extremely handy
+ * if there is information you might need for a failure message that is easiest
+ * to collect in the steps leading up to the actual check.
+ */
+void kunit_stream_commit(struct kunit_stream *kstream);
+
+/**
+ * kunit_stream_clear(): clears the internal buffer.
+ * @kstream: the stream being operated on.
+ *
+ * Clears the contents of the internal buffer.
+ */
+void kunit_stream_clear(struct kunit_stream *kstream);
+
+#endif /* _KUNIT_KUNIT_STREAM_H */
diff --git a/include/kunit/test.h b/include/kunit/test.h
index bdf41d31c343c..bc7dbdcf8abab 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -11,6 +11,7 @@
#include <linux/types.h>
#include <linux/slab.h>
+#include <kunit/kunit-stream.h>
struct kunit_resource;
@@ -184,6 +185,8 @@ struct kunit {
void kunit_init_test(struct kunit *test, const char *name);
+void kunit_fail(struct kunit *test, struct kunit_stream *stream);
+
int kunit_run_tests(struct kunit_suite *suite);
/**
diff --git a/kunit/Makefile b/kunit/Makefile
index 275b565a0e81f..6ddc622ee6b1c 100644
--- a/kunit/Makefile
+++ b/kunit/Makefile
@@ -1,2 +1,3 @@
obj-$(CONFIG_KUNIT) += test.o \
- string-stream.o
+ string-stream.o \
+ kunit-stream.o
diff --git a/kunit/kunit-stream.c b/kunit/kunit-stream.c
new file mode 100644
index 0000000000000..8bea1f22eafb5
--- /dev/null
+++ b/kunit/kunit-stream.c
@@ -0,0 +1,123 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * C++ stream style string formatter and printer used in KUnit for outputting
+ * KUnit messages.
+ *
+ * Copyright (C) 2019, Google LLC.
+ * Author: Brendan Higgins <brendanhiggins@google.com>
+ */
+
+#include <kunit/test.h>
+#include <kunit/kunit-stream.h>
+#include <kunit/string-stream.h>
+
+void kunit_stream_add(struct kunit_stream *kstream, const char *fmt, ...)
+{
+ va_list args;
+ struct string_stream *stream = kstream->internal_stream;
+
+ va_start(args, fmt);
+
+ if (string_stream_vadd(stream, fmt, args) < 0)
+ kunit_err(kstream->test,
+ "Failed to allocate fragment: %s\n",
+ fmt);
+
+ va_end(args);
+}
+
+void kunit_stream_append(struct kunit_stream *kstream,
+ struct kunit_stream *other)
+{
+ struct string_stream *other_stream = other->internal_stream;
+ const char *other_content;
+
+ other_content = string_stream_get_string(other_stream);
+
+ if (!other_content) {
+ kunit_err(kstream->test,
+ "Failed to get string from second argument for appending\n");
+ return;
+ }
+
+ kunit_stream_add(kstream, other_content);
+}
+
+void kunit_stream_clear(struct kunit_stream *kstream)
+{
+ string_stream_clear(kstream->internal_stream);
+}
+
+void kunit_stream_commit(struct kunit_stream *kstream)
+{
+ struct string_stream *stream = kstream->internal_stream;
+ struct string_stream_fragment *fragment;
+ struct kunit *test = kstream->test;
+ char *buf;
+
+ buf = string_stream_get_string(stream);
+ if (!buf) {
+ kunit_err(test,
+ "Could not allocate buffer, dumping stream:\n");
+ list_for_each_entry(fragment, &stream->fragments, node) {
+ kunit_err(test, fragment->fragment);
+ }
+ kunit_err(test, "\n");
+ goto cleanup;
+ }
+
+ kunit_printk(kstream->level, test, buf);
+ kfree(buf);
+
+cleanup:
+ kunit_stream_clear(kstream);
+}
+
+static int kunit_stream_init(struct kunit_resource *res, void *context)
+{
+ struct kunit *test = context;
+ struct kunit_stream *stream;
+
+ stream = kzalloc(sizeof(*stream), GFP_KERNEL);
+ if (!stream)
+ return -ENOMEM;
+
+ res->allocation = stream;
+ stream->test = test;
+ stream->internal_stream = alloc_string_stream(test);
+
+ if (!stream->internal_stream)
+ return -ENOMEM;
+
+ return 0;
+}
+
+static void kunit_stream_free(struct kunit_resource *res)
+{
+ struct kunit_stream *stream = res->allocation;
+
+ if (!string_stream_is_empty(stream->internal_stream)) {
+ kunit_err(stream->test,
+ "End of test case reached with uncommitted stream entries\n");
+ kunit_stream_commit(stream);
+ }
+}
+
+struct kunit_stream *alloc_kunit_stream(struct kunit *test, const char *level)
+{
+ struct kunit_stream *kstream;
+ struct kunit_resource *res;
+
+ res = kunit_alloc_resource(test,
+ kunit_stream_init,
+ kunit_stream_free,
+ test);
+
+ if (!res)
+ return NULL;
+
+ kstream = res->allocation;
+ kstream->level = level;
+
+ return kstream;
+}
diff --git a/kunit/test.c b/kunit/test.c
index f165c9d8e10b0..29edf34a89a37 100644
--- a/kunit/test.c
+++ b/kunit/test.c
@@ -120,6 +120,12 @@ static void kunit_print_test_case_ok_not_ok(struct kunit_case *test_case,
test_case->name);
}
+void kunit_fail(struct kunit *test, struct kunit_stream *stream)
+{
+ kunit_set_failure(test);
+ kunit_stream_commit(stream);
+}
+
void kunit_init_test(struct kunit *test, const char *name)
{
mutex_init(&test->lock);
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
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