* [PATCH 1/8] xen/elfstructs: Include xen/types.h
2025-03-12 17:45 [PATCH 0/8] xen: Untangle mm.h Andrew Cooper
@ 2025-03-12 17:45 ` Andrew Cooper
2025-03-13 9:10 ` Jan Beulich
2025-03-12 17:45 ` [PATCH 2/8] xen/livepatch: Fix include hierarchy Andrew Cooper
` (8 subsequent siblings)
9 siblings, 1 reply; 22+ messages in thread
From: Andrew Cooper @ 2025-03-12 17:45 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, Anthony PERARD, Michal Orzel, Jan Beulich,
Julien Grall, Roger Pau Monné, Stefano Stabellini,
Volodymyr Babchuk, Bertrand Marquis, Oleksii Kurochko,
Shawn Anastasio
elfstructs.h needs the stdint.h types. Two headers arrange this manually, but
elf.h and livepatch.h do not, which breaks source files whose headers are
properly sorted.
elfstructs.h is used by tools too, so limit this to Xen only.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Anthony PERARD <anthony.perard@vates.tech>
CC: Michal Orzel <michal.orzel@amd.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Julien Grall <julien@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Bertrand Marquis <bertrand.marquis@arm.com>
CC: Oleksii Kurochko <oleksii.kurochko@gmail.com>
CC: Shawn Anastasio <sanastasio@raptorengineering.com>
---
xen/include/xen/elfstructs.h | 7 ++++++-
xen/include/xen/livepatch_elf.h | 1 -
xen/include/xen/version.h | 1 -
3 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/xen/include/xen/elfstructs.h b/xen/include/xen/elfstructs.h
index eb6b87a823a8..f64ecec01990 100644
--- a/xen/include/xen/elfstructs.h
+++ b/xen/include/xen/elfstructs.h
@@ -26,6 +26,11 @@
* THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*/
+/* Outside of Xen, the includer must provide stdint.h or equivalent. */
+#ifdef __XEN__
+#include <xen/types.h>
+#endif
+
typedef uint32_t Elf32_Addr; /* Unsigned program address */
typedef uint32_t Elf32_Off; /* Unsigned file offset */
typedef uint16_t Elf32_Half; /* Unsigned medium integer */
@@ -45,7 +50,7 @@ typedef uint64_t Elf64_Xword;
/*
* e_ident[] identification indexes
- * See http://www.caldera.com/developers/gabi/2000-07-17/ch4.eheader.html
+ * See http://www.caldera.com/developers/gabi/2000-07-17/ch4.eheader.html
*/
#define EI_MAG0 0 /* file ID */
#define EI_MAG1 1 /* file ID */
diff --git a/xen/include/xen/livepatch_elf.h b/xen/include/xen/livepatch_elf.h
index 842111e14518..a8aafecd34b1 100644
--- a/xen/include/xen/livepatch_elf.h
+++ b/xen/include/xen/livepatch_elf.h
@@ -5,7 +5,6 @@
#ifndef __XEN_LIVEPATCH_ELF_H__
#define __XEN_LIVEPATCH_ELF_H__
-#include <xen/types.h>
#include <xen/elfstructs.h>
/* The following describes an Elf file as consumed by Xen Live Patch. */
diff --git a/xen/include/xen/version.h b/xen/include/xen/version.h
index 4856ad1b446d..bc69ec9fb029 100644
--- a/xen/include/xen/version.h
+++ b/xen/include/xen/version.h
@@ -1,7 +1,6 @@
#ifndef __XEN_VERSION_H__
#define __XEN_VERSION_H__
-#include <xen/types.h>
#include <xen/elfstructs.h>
const char *xen_compile_date(void);
base-commit: 8e60d47cf0112c145b6b0e454d102b04c857db8c
--
2.39.5
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 1/8] xen/elfstructs: Include xen/types.h
2025-03-12 17:45 ` [PATCH 1/8] xen/elfstructs: Include xen/types.h Andrew Cooper
@ 2025-03-13 9:10 ` Jan Beulich
0 siblings, 0 replies; 22+ messages in thread
From: Jan Beulich @ 2025-03-13 9:10 UTC (permalink / raw)
To: Andrew Cooper
Cc: Anthony PERARD, Michal Orzel, Julien Grall, Roger Pau Monné,
Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis,
Oleksii Kurochko, Shawn Anastasio, Xen-devel
On 12.03.2025 18:45, Andrew Cooper wrote:
> elfstructs.h needs the stdint.h types.
But then why do you include xen/types.h rather than just xen/stdint.h there?
Instead I guess ...
> --- a/xen/include/xen/elfstructs.h
> +++ b/xen/include/xen/elfstructs.h
> @@ -26,6 +26,11 @@
> * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> */
>
> +/* Outside of Xen, the includer must provide stdint.h or equivalent. */
> +#ifdef __XEN__
> +#include <xen/types.h>
> +#endif
#else
#include <stdint.h>
would make sense to add at the same time then.
Jan
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 2/8] xen/livepatch: Fix include hierarchy
2025-03-12 17:45 [PATCH 0/8] xen: Untangle mm.h Andrew Cooper
2025-03-12 17:45 ` [PATCH 1/8] xen/elfstructs: Include xen/types.h Andrew Cooper
@ 2025-03-12 17:45 ` Andrew Cooper
2025-03-12 17:45 ` [PATCH 3/8] xen: Sort includes Andrew Cooper
` (7 subsequent siblings)
9 siblings, 0 replies; 22+ messages in thread
From: Andrew Cooper @ 2025-03-12 17:45 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, Anthony PERARD, Michal Orzel, Jan Beulich,
Julien Grall, Roger Pau Monné, Stefano Stabellini,
Volodymyr Babchuk, Bertrand Marquis, Oleksii Kurochko,
Shawn Anastasio
xen/livepatch.h includes public/sysctl.h twice, which can be deduplicated, and
includes asm/livepatch.h meaning that each livepatch.c does not need to
include both.
Comment the #else and #endif cases to aid legibility.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Anthony PERARD <anthony.perard@vates.tech>
CC: Michal Orzel <michal.orzel@amd.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Julien Grall <julien@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Bertrand Marquis <bertrand.marquis@arm.com>
CC: Oleksii Kurochko <oleksii.kurochko@gmail.com>
CC: Shawn Anastasio <sanastasio@raptorengineering.com>
---
xen/arch/arm/arm32/livepatch.c | 1 -
xen/arch/arm/arm64/livepatch.c | 1 -
xen/arch/arm/livepatch.c | 1 -
xen/arch/x86/livepatch.c | 1 -
xen/include/xen/livepatch.h | 10 +++++-----
5 files changed, 5 insertions(+), 9 deletions(-)
diff --git a/xen/arch/arm/arm32/livepatch.c b/xen/arch/arm/arm32/livepatch.c
index 134d07a175bb..8541c71d6e2e 100644
--- a/xen/arch/arm/arm32/livepatch.c
+++ b/xen/arch/arm/arm32/livepatch.c
@@ -9,7 +9,6 @@
#include <xen/livepatch.h>
#include <asm/page.h>
-#include <asm/livepatch.h>
void arch_livepatch_apply(const struct livepatch_func *func,
struct livepatch_fstate *state)
diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
index 6efe4ec770d4..01e6db94be67 100644
--- a/xen/arch/arm/arm64/livepatch.c
+++ b/xen/arch/arm/arm64/livepatch.c
@@ -13,7 +13,6 @@
#include <asm/bitops.h>
#include <asm/byteorder.h>
#include <asm/insn.h>
-#include <asm/livepatch.h>
void arch_livepatch_apply(const struct livepatch_func *func,
struct livepatch_fstate *state)
diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
index 3805b2974663..2fbb7bce60bb 100644
--- a/xen/arch/arm/livepatch.c
+++ b/xen/arch/arm/livepatch.c
@@ -11,7 +11,6 @@
#include <xen/vmap.h>
#include <asm/cpufeature.h>
-#include <asm/livepatch.h>
/* Override macros from asm/page.h to make them work with mfn_t */
#undef virt_to_mfn
diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index be40f625d206..bdca355dc6cc 100644
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -17,7 +17,6 @@
#include <asm/endbr.h>
#include <asm/fixmap.h>
#include <asm/nmi.h>
-#include <asm/livepatch.h>
#include <asm/setup.h>
static bool has_active_waitqueue(const struct vm_event_domain *ved)
diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
index d074a5bebecc..c1e76ef55404 100644
--- a/xen/include/xen/livepatch.h
+++ b/xen/include/xen/livepatch.h
@@ -14,12 +14,14 @@ struct xen_sysctl_livepatch_op;
#include <xen/elfstructs.h>
#include <xen/errno.h> /* For -ENOSYS or -EOVERFLOW */
-#include <public/sysctl.h> /* For LIVEPATCH_OPAQUE_SIZE */
+#include <public/sysctl.h>
#ifdef CONFIG_LIVEPATCH
#include <xen/lib.h>
+#include <asm/livepatch.h>
+
/*
* We use alternative and exception table code - which by default are __init
* only, however we need them during runtime. These macros allows us to build
@@ -93,8 +95,6 @@ int arch_livepatch_secure(const void *va, unsigned int pages, enum va_type types
void arch_livepatch_init(void);
-#include <public/sysctl.h> /* For struct livepatch_func. */
-#include <asm/livepatch.h>
int arch_livepatch_verify_func(const struct livepatch_func *func);
static inline
@@ -143,7 +143,7 @@ struct payload;
int revert_payload(struct payload *data);
void revert_payload_tail(struct payload *data);
-#else
+#else /* !CONFIG_LIVEPATCH */
/*
* If not compiling with Live Patch certain functionality should stay as
@@ -165,7 +165,7 @@ static inline bool is_patch(const void *addr)
{
return 0;
}
-#endif /* CONFIG_LIVEPATCH */
+#endif /* !CONFIG_LIVEPATCH */
#endif /* __XEN_LIVEPATCH_H__ */
--
2.39.5
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 3/8] xen: Sort includes
2025-03-12 17:45 [PATCH 0/8] xen: Untangle mm.h Andrew Cooper
2025-03-12 17:45 ` [PATCH 1/8] xen/elfstructs: Include xen/types.h Andrew Cooper
2025-03-12 17:45 ` [PATCH 2/8] xen/livepatch: Fix include hierarchy Andrew Cooper
@ 2025-03-12 17:45 ` Andrew Cooper
2025-03-13 9:11 ` Jan Beulich
2025-03-12 17:45 ` [PATCH 4/8] xen/common: Split tlb-clock.h out of mm.h Andrew Cooper
` (6 subsequent siblings)
9 siblings, 1 reply; 22+ messages in thread
From: Andrew Cooper @ 2025-03-12 17:45 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, Anthony PERARD, Michal Orzel, Jan Beulich,
Julien Grall, Roger Pau Monné, Stefano Stabellini,
Volodymyr Babchuk, Bertrand Marquis, Oleksii Kurochko,
Shawn Anastasio
... needing later adjustment. Drop types.h when it's clearly not needed.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Anthony PERARD <anthony.perard@vates.tech>
CC: Michal Orzel <michal.orzel@amd.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Julien Grall <julien@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Bertrand Marquis <bertrand.marquis@arm.com>
CC: Oleksii Kurochko <oleksii.kurochko@gmail.com>
CC: Shawn Anastasio <sanastasio@raptorengineering.com>
---
xen/arch/arm/mmu/setup.c | 2 +-
xen/arch/x86/alternative.c | 12 ++++++------
xen/arch/x86/livepatch.c | 8 ++++----
xen/common/memory.c | 4 +++-
xen/common/page_alloc.c | 5 ++---
xen/include/xen/mm.h | 6 +++---
6 files changed, 19 insertions(+), 18 deletions(-)
diff --git a/xen/arch/arm/mmu/setup.c b/xen/arch/arm/mmu/setup.c
index 30afe9778194..f6119ccacf15 100644
--- a/xen/arch/arm/mmu/setup.c
+++ b/xen/arch/arm/mmu/setup.c
@@ -12,8 +12,8 @@
#include <xen/sizes.h>
#include <xen/vmap.h>
-#include <asm/setup.h>
#include <asm/fixmap.h>
+#include <asm/setup.h>
/* Override macros from asm/page.h to make them work with mfn_t */
#undef mfn_to_virt
diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
index 1ba35cb9ede9..46b04c9cb83d 100644
--- a/xen/arch/x86/alternative.c
+++ b/xen/arch/x86/alternative.c
@@ -4,18 +4,18 @@
*/
#include <xen/delay.h>
-#include <xen/types.h>
+#include <xen/init.h>
+#include <xen/livepatch.h>
+
+#include <asm/alternative.h>
#include <asm/apic.h>
#include <asm/endbr.h>
+#include <asm/nmi.h>
+#include <asm/nops.h>
#include <asm/processor.h>
-#include <asm/alternative.h>
-#include <xen/init.h>
#include <asm/setup.h>
#include <asm/system.h>
#include <asm/traps.h>
-#include <asm/nmi.h>
-#include <asm/nops.h>
-#include <xen/livepatch.h>
#define MAX_PATCH_LEN (255-1)
diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index bdca355dc6cc..5158e91f7e6e 100644
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -5,14 +5,14 @@
#include <xen/errno.h>
#include <xen/init.h>
#include <xen/lib.h>
+#include <xen/livepatch.h>
+#include <xen/livepatch_elf.h>
#include <xen/mm.h>
#include <xen/pfn.h>
-#include <xen/vmap.h>
-#include <xen/livepatch_elf.h>
-#include <xen/livepatch.h>
#include <xen/sched.h>
-#include <xen/vm_event.h>
#include <xen/virtual_region.h>
+#include <xen/vm_event.h>
+#include <xen/vmap.h>
#include <asm/endbr.h>
#include <asm/fixmap.h>
diff --git a/xen/common/memory.c b/xen/common/memory.c
index 8ca4e1a8425b..61a94b23abae 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -25,12 +25,14 @@
#include <xen/sched.h>
#include <xen/sections.h>
#include <xen/trace.h>
-#include <xen/types.h>
+
#include <asm/current.h>
#include <asm/hardirq.h>
#include <asm/p2m.h>
#include <asm/page.h>
+
#include <public/memory.h>
+
#include <xsm/xsm.h>
#ifdef CONFIG_X86
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 5f9c9305ef37..bc029ea797a2 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -133,7 +133,6 @@
#include <xen/param.h>
#include <xen/perfc.h>
#include <xen/pfn.h>
-#include <xen/types.h>
#include <xen/sched.h>
#include <xen/sections.h>
#include <xen/softirq.h>
@@ -144,14 +143,14 @@
#include <asm/flushtlb.h>
#include <asm/page.h>
-#include <public/sysctl.h>
#include <public/sched.h>
+#include <public/sysctl.h>
#ifdef CONFIG_X86
#include <asm/guest.h>
#include <asm/p2m.h>
-#include <asm/setup.h> /* for highmem_start only */
#include <asm/paging.h>
+#include <asm/setup.h>
#else
#define p2m_pod_offline_or_broken_hit(pg) 0
#define p2m_pod_offline_or_broken_replace(pg) BUG_ON(pg)
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 16f733281af3..45000cc3f64b 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -63,11 +63,11 @@
#include <xen/bug.h>
#include <xen/compiler.h>
-#include <xen/mm-frame.h>
-#include <xen/types.h>
#include <xen/list.h>
-#include <xen/spinlock.h>
+#include <xen/mm-frame.h>
#include <xen/perfc.h>
+#include <xen/spinlock.h>
+
#include <public/memory.h>
struct page_info;
--
2.39.5
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 3/8] xen: Sort includes
2025-03-12 17:45 ` [PATCH 3/8] xen: Sort includes Andrew Cooper
@ 2025-03-13 9:11 ` Jan Beulich
0 siblings, 0 replies; 22+ messages in thread
From: Jan Beulich @ 2025-03-13 9:11 UTC (permalink / raw)
To: Andrew Cooper
Cc: Anthony PERARD, Michal Orzel, Julien Grall, Roger Pau Monné,
Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis,
Oleksii Kurochko, Shawn Anastasio, Xen-devel
On 12.03.2025 18:45, Andrew Cooper wrote:
> ... needing later adjustment. Drop types.h when it's clearly not needed.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 4/8] xen/common: Split tlb-clock.h out of mm.h
2025-03-12 17:45 [PATCH 0/8] xen: Untangle mm.h Andrew Cooper
` (2 preceding siblings ...)
2025-03-12 17:45 ` [PATCH 3/8] xen: Sort includes Andrew Cooper
@ 2025-03-12 17:45 ` Andrew Cooper
2025-03-13 12:59 ` Jan Beulich
2025-03-12 17:45 ` [PATCH 4/8] xen/common: Split tlk-clock.h " Andrew Cooper
` (5 subsequent siblings)
9 siblings, 1 reply; 22+ messages in thread
From: Andrew Cooper @ 2025-03-12 17:45 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, Anthony PERARD, Michal Orzel, Jan Beulich,
Julien Grall, Roger Pau Monné, Stefano Stabellini,
Volodymyr Babchuk, Bertrand Marquis, Oleksii Kurochko,
Shawn Anastasio
xen/mm.h includes asm/tlbflush.h almost at the end, which creates a horrible
tangle. This is in order to provide two common files with an abstraction over
the x86-specific TLB clock logic.
First, introduce CONFIG_HAS_TLB_CLOCK, selected by x86 only. Next, introduce
xen/tlb-clock.h, providing empty stubs, and include this into memory.c and
page_alloc.c
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Anthony PERARD <anthony.perard@vates.tech>
CC: Michal Orzel <michal.orzel@amd.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Julien Grall <julien@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Bertrand Marquis <bertrand.marquis@arm.com>
CC: Oleksii Kurochko <oleksii.kurochko@gmail.com>
CC: Shawn Anastasio <sanastasio@raptorengineering.com>
There is still a mess here with the common vs x86 split, but it's better
contained than before.
---
xen/arch/x86/Kconfig | 1 +
xen/common/Kconfig | 3 +++
xen/common/memory.c | 1 +
xen/common/page_alloc.c | 1 +
xen/include/xen/mm.h | 27 --------------------
xen/include/xen/tlb-clock.h | 49 +++++++++++++++++++++++++++++++++++++
6 files changed, 55 insertions(+), 27 deletions(-)
create mode 100644 xen/include/xen/tlb-clock.h
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 6e41bc0fb435..e9a166ee3dd0 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -28,6 +28,7 @@ config X86
select HAS_PCI_MSI
select HAS_PIRQ
select HAS_SCHED_GRANULARITY
+ select HAS_TLB_CLOCK
select HAS_UBSAN
select HAS_VMAP
select HAS_VPCI if HVM
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 6166327f4d14..dcf7d9d00d0a 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -83,6 +83,9 @@ config HAS_PMAP
config HAS_SCHED_GRANULARITY
bool
+config HAS_TLB_CLOCK
+ bool
+
config HAS_UBSAN
bool
diff --git a/xen/common/memory.c b/xen/common/memory.c
index 61a94b23abae..9138fd096696 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -24,6 +24,7 @@
#include <xen/perfc.h>
#include <xen/sched.h>
#include <xen/sections.h>
+#include <xen/tlb-clock.h>
#include <xen/trace.h>
#include <asm/current.h>
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index bc029ea797a2..b90c3d7988b4 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -137,6 +137,7 @@
#include <xen/sections.h>
#include <xen/softirq.h>
#include <xen/spinlock.h>
+#include <xen/tlb-clock.h>
#include <xen/vm_event.h>
#include <xen/xvmalloc.h>
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 45000cc3f64b..fff36ff903d6 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -588,33 +588,6 @@ unsigned long get_upper_mfn_bound(void);
#include <asm/flushtlb.h>
-static inline void accumulate_tlbflush(bool *need_tlbflush,
- const struct page_info *page,
- uint32_t *tlbflush_timestamp)
-{
- if ( page->u.free.need_tlbflush &&
- page->tlbflush_timestamp <= tlbflush_current_time() &&
- (!*need_tlbflush ||
- page->tlbflush_timestamp > *tlbflush_timestamp) )
- {
- *need_tlbflush = true;
- *tlbflush_timestamp = page->tlbflush_timestamp;
- }
-}
-
-static inline void filtered_flush_tlb_mask(uint32_t tlbflush_timestamp)
-{
- cpumask_t mask;
-
- cpumask_copy(&mask, &cpu_online_map);
- tlbflush_filter(&mask, tlbflush_timestamp);
- if ( !cpumask_empty(&mask) )
- {
- perfc_incr(need_flush_tlb_flush);
- arch_flush_tlb_mask(&mask);
- }
-}
-
enum XENSHARE_flags {
SHARE_rw,
SHARE_ro,
diff --git a/xen/include/xen/tlb-clock.h b/xen/include/xen/tlb-clock.h
new file mode 100644
index 000000000000..796c0be7fbef
--- /dev/null
+++ b/xen/include/xen/tlb-clock.h
@@ -0,0 +1,49 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef XEN_TLB_CLOCK_H
+#define XEN_TLB_CLOCK_H
+
+#include <xen/types.h>
+
+#ifdef CONFIG_HAS_TLB_CLOCK
+
+#include <xen/mm.h>
+
+#include <asm/flushtlb.h>
+
+static inline void accumulate_tlbflush(
+ bool *need_tlbflush, const struct page_info *page,
+ uint32_t *tlbflush_timestamp)
+{
+ if ( page->u.free.need_tlbflush &&
+ page->tlbflush_timestamp <= tlbflush_current_time() &&
+ (!*need_tlbflush ||
+ page->tlbflush_timestamp > *tlbflush_timestamp) )
+ {
+ *need_tlbflush = true;
+ *tlbflush_timestamp = page->tlbflush_timestamp;
+ }
+}
+
+static inline void filtered_flush_tlb_mask(uint32_t tlbflush_timestamp)
+{
+ cpumask_t mask;
+
+ cpumask_copy(&mask, &cpu_online_map);
+ tlbflush_filter(&mask, tlbflush_timestamp);
+ if ( !cpumask_empty(&mask) )
+ {
+ perfc_incr(need_flush_tlb_flush);
+ arch_flush_tlb_mask(&mask);
+ }
+}
+
+#else /* !CONFIG_HAS_TLB_CLOCK */
+
+struct page_info;
+static inline void accumulate_tlbflush(
+ bool *need_tlbflush, const struct page_info *page,
+ uint32_t *tlbflush_timestamp) {}
+static inline void filtered_flush_tlb_mask(uint32_t tlbflush_timestamp) {}
+
+#endif /* !CONFIG_HAS_TLB_CLOCK*/
+#endif /* XEN_TLB_CLOCK_H */
--
2.39.5
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 4/8] xen/common: Split tlb-clock.h out of mm.h
2025-03-12 17:45 ` [PATCH 4/8] xen/common: Split tlb-clock.h out of mm.h Andrew Cooper
@ 2025-03-13 12:59 ` Jan Beulich
2025-03-13 13:35 ` Andrew Cooper
0 siblings, 1 reply; 22+ messages in thread
From: Jan Beulich @ 2025-03-13 12:59 UTC (permalink / raw)
To: Andrew Cooper
Cc: Anthony PERARD, Michal Orzel, Julien Grall, Roger Pau Monné,
Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis,
Oleksii Kurochko, Shawn Anastasio, Xen-devel
On 12.03.2025 18:45, Andrew Cooper wrote:
> xen/mm.h includes asm/tlbflush.h almost at the end, which creates a horrible
> tangle. This is in order to provide two common files with an abstraction over
> the x86-specific TLB clock logic.
>
> First, introduce CONFIG_HAS_TLB_CLOCK, selected by x86 only. Next, introduce
> xen/tlb-clock.h, providing empty stubs, and include this into memory.c and
> page_alloc.c
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Anthony PERARD <anthony.perard@vates.tech>
> CC: Michal Orzel <michal.orzel@amd.com>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Julien Grall <julien@xen.org>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> CC: Bertrand Marquis <bertrand.marquis@arm.com>
> CC: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> CC: Shawn Anastasio <sanastasio@raptorengineering.com>
>
> There is still a mess here with the common vs x86 split, but it's better
> contained than before.
> ---
> xen/arch/x86/Kconfig | 1 +
> xen/common/Kconfig | 3 +++
> xen/common/memory.c | 1 +
> xen/common/page_alloc.c | 1 +
> xen/include/xen/mm.h | 27 --------------------
> xen/include/xen/tlb-clock.h | 49 +++++++++++++++++++++++++++++++++++++
> 6 files changed, 55 insertions(+), 27 deletions(-)
> create mode 100644 xen/include/xen/tlb-clock.h
>
> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
> index 6e41bc0fb435..e9a166ee3dd0 100644
> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -28,6 +28,7 @@ config X86
> select HAS_PCI_MSI
> select HAS_PIRQ
> select HAS_SCHED_GRANULARITY
> + select HAS_TLB_CLOCK
> select HAS_UBSAN
> select HAS_VMAP
> select HAS_VPCI if HVM
> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> index 6166327f4d14..dcf7d9d00d0a 100644
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -83,6 +83,9 @@ config HAS_PMAP
> config HAS_SCHED_GRANULARITY
> bool
>
> +config HAS_TLB_CLOCK
> + bool
> +
> config HAS_UBSAN
> bool
>
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index 61a94b23abae..9138fd096696 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -24,6 +24,7 @@
> #include <xen/perfc.h>
> #include <xen/sched.h>
> #include <xen/sections.h>
> +#include <xen/tlb-clock.h>
> #include <xen/trace.h>
>
> #include <asm/current.h>
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index bc029ea797a2..b90c3d7988b4 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -137,6 +137,7 @@
> #include <xen/sections.h>
> #include <xen/softirq.h>
> #include <xen/spinlock.h>
> +#include <xen/tlb-clock.h>
> #include <xen/vm_event.h>
> #include <xen/xvmalloc.h>
>
> diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
> index 45000cc3f64b..fff36ff903d6 100644
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -588,33 +588,6 @@ unsigned long get_upper_mfn_bound(void);
>
> #include <asm/flushtlb.h>
>
> -static inline void accumulate_tlbflush(bool *need_tlbflush,
> - const struct page_info *page,
> - uint32_t *tlbflush_timestamp)
> -{
> - if ( page->u.free.need_tlbflush &&
> - page->tlbflush_timestamp <= tlbflush_current_time() &&
> - (!*need_tlbflush ||
> - page->tlbflush_timestamp > *tlbflush_timestamp) )
> - {
> - *need_tlbflush = true;
> - *tlbflush_timestamp = page->tlbflush_timestamp;
> - }
> -}
> -
> -static inline void filtered_flush_tlb_mask(uint32_t tlbflush_timestamp)
> -{
> - cpumask_t mask;
> -
> - cpumask_copy(&mask, &cpu_online_map);
> - tlbflush_filter(&mask, tlbflush_timestamp);
> - if ( !cpumask_empty(&mask) )
> - {
> - perfc_incr(need_flush_tlb_flush);
> - arch_flush_tlb_mask(&mask);
> - }
> -}
> -
> enum XENSHARE_flags {
> SHARE_rw,
> SHARE_ro,
> diff --git a/xen/include/xen/tlb-clock.h b/xen/include/xen/tlb-clock.h
> new file mode 100644
> index 000000000000..796c0be7fbef
> --- /dev/null
> +++ b/xen/include/xen/tlb-clock.h
> @@ -0,0 +1,49 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef XEN_TLB_CLOCK_H
> +#define XEN_TLB_CLOCK_H
> +
> +#include <xen/types.h>
> +
> +#ifdef CONFIG_HAS_TLB_CLOCK
> +
> +#include <xen/mm.h>
> +
> +#include <asm/flushtlb.h>
> +
> +static inline void accumulate_tlbflush(
> + bool *need_tlbflush, const struct page_info *page,
> + uint32_t *tlbflush_timestamp)
> +{
> + if ( page->u.free.need_tlbflush &&
> + page->tlbflush_timestamp <= tlbflush_current_time() &&
> + (!*need_tlbflush ||
> + page->tlbflush_timestamp > *tlbflush_timestamp) )
> + {
> + *need_tlbflush = true;
> + *tlbflush_timestamp = page->tlbflush_timestamp;
> + }
> +}
> +
> +static inline void filtered_flush_tlb_mask(uint32_t tlbflush_timestamp)
> +{
> + cpumask_t mask;
> +
> + cpumask_copy(&mask, &cpu_online_map);
> + tlbflush_filter(&mask, tlbflush_timestamp);
> + if ( !cpumask_empty(&mask) )
> + {
> + perfc_incr(need_flush_tlb_flush);
Would this perf counter then perhaps also want to become dependent upon
HAS_TLB_CLOCK=y (or become an arch-specific one)? It's used elsewhere in x86,
but not for any of the other arch-es.
However, see below.
> + arch_flush_tlb_mask(&mask);
> + }
> +}
> +
> +#else /* !CONFIG_HAS_TLB_CLOCK */
> +
> +struct page_info;
> +static inline void accumulate_tlbflush(
> + bool *need_tlbflush, const struct page_info *page,
> + uint32_t *tlbflush_timestamp) {}
> +static inline void filtered_flush_tlb_mask(uint32_t tlbflush_timestamp) {}
Is doing nothing here correct? mark_page_free() can set a page's
->u.free.need_tlbflush. And with that flag set the full
static inline void accumulate_tlbflush(
bool *need_tlbflush, const struct page_info *page,
uint32_t *tlbflush_timestamp)
{
if ( page->u.free.need_tlbflush &&
page->tlbflush_timestamp <= tlbflush_current_time() &&
(!*need_tlbflush ||
page->tlbflush_timestamp > *tlbflush_timestamp) )
{
*need_tlbflush = true;
*tlbflush_timestamp = page->tlbflush_timestamp;
}
}
reduces to (considering that tlbflush_current_time() resolves to constant 0,
which also implies every page's ->tlbflush_timestamp is only ever 0)
static inline void accumulate_tlbflush(
bool *need_tlbflush, const struct page_info *page,
uint32_t *tlbflush_timestamp)
{
if ( !*need_tlbflush )
*need_tlbflush = true;
}
which means a not-stubbed-out filtered_flush_tlb_mask(), with tlbflush_filter()
doing nothing, would actually invoke arch_flush_tlb_mask() (with all online CPUs
set in the mask) when called. And arch_flush_tlb_mask() isn't a no-op on Arm.
I therefore think that while moving stuff into a separate header makes sense,
HAS_TLB_CLOCK isn't overly useful to introduce.
Jan
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 4/8] xen/common: Split tlb-clock.h out of mm.h
2025-03-13 12:59 ` Jan Beulich
@ 2025-03-13 13:35 ` Andrew Cooper
2025-03-13 19:43 ` Nicola Vetrini
0 siblings, 1 reply; 22+ messages in thread
From: Andrew Cooper @ 2025-03-13 13:35 UTC (permalink / raw)
To: Jan Beulich
Cc: Anthony PERARD, Michal Orzel, Julien Grall, Roger Pau Monné,
Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis,
Oleksii Kurochko, Shawn Anastasio, Xen-devel
On 13/03/2025 12:59 pm, Jan Beulich wrote:
> On 12.03.2025 18:45, Andrew Cooper wrote:
>> xen/mm.h includes asm/tlbflush.h almost at the end, which creates a horrible
>> tangle. This is in order to provide two common files with an abstraction over
>> the x86-specific TLB clock logic.
>>
>> First, introduce CONFIG_HAS_TLB_CLOCK, selected by x86 only. Next, introduce
>> xen/tlb-clock.h, providing empty stubs, and include this into memory.c and
>> page_alloc.c
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Anthony PERARD <anthony.perard@vates.tech>
>> CC: Michal Orzel <michal.orzel@amd.com>
>> CC: Jan Beulich <jbeulich@suse.com>
>> CC: Julien Grall <julien@xen.org>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
>> CC: Bertrand Marquis <bertrand.marquis@arm.com>
>> CC: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>> CC: Shawn Anastasio <sanastasio@raptorengineering.com>
>>
>> There is still a mess here with the common vs x86 split, but it's better
>> contained than before.
>> ---
>> xen/arch/x86/Kconfig | 1 +
>> xen/common/Kconfig | 3 +++
>> xen/common/memory.c | 1 +
>> xen/common/page_alloc.c | 1 +
>> xen/include/xen/mm.h | 27 --------------------
>> xen/include/xen/tlb-clock.h | 49 +++++++++++++++++++++++++++++++++++++
>> 6 files changed, 55 insertions(+), 27 deletions(-)
>> create mode 100644 xen/include/xen/tlb-clock.h
>>
>> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
>> index 6e41bc0fb435..e9a166ee3dd0 100644
>> --- a/xen/arch/x86/Kconfig
>> +++ b/xen/arch/x86/Kconfig
>> @@ -28,6 +28,7 @@ config X86
>> select HAS_PCI_MSI
>> select HAS_PIRQ
>> select HAS_SCHED_GRANULARITY
>> + select HAS_TLB_CLOCK
>> select HAS_UBSAN
>> select HAS_VMAP
>> select HAS_VPCI if HVM
>> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
>> index 6166327f4d14..dcf7d9d00d0a 100644
>> --- a/xen/common/Kconfig
>> +++ b/xen/common/Kconfig
>> @@ -83,6 +83,9 @@ config HAS_PMAP
>> config HAS_SCHED_GRANULARITY
>> bool
>>
>> +config HAS_TLB_CLOCK
>> + bool
>> +
>> config HAS_UBSAN
>> bool
>>
>> diff --git a/xen/common/memory.c b/xen/common/memory.c
>> index 61a94b23abae..9138fd096696 100644
>> --- a/xen/common/memory.c
>> +++ b/xen/common/memory.c
>> @@ -24,6 +24,7 @@
>> #include <xen/perfc.h>
>> #include <xen/sched.h>
>> #include <xen/sections.h>
>> +#include <xen/tlb-clock.h>
>> #include <xen/trace.h>
>>
>> #include <asm/current.h>
>> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
>> index bc029ea797a2..b90c3d7988b4 100644
>> --- a/xen/common/page_alloc.c
>> +++ b/xen/common/page_alloc.c
>> @@ -137,6 +137,7 @@
>> #include <xen/sections.h>
>> #include <xen/softirq.h>
>> #include <xen/spinlock.h>
>> +#include <xen/tlb-clock.h>
>> #include <xen/vm_event.h>
>> #include <xen/xvmalloc.h>
>>
>> diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
>> index 45000cc3f64b..fff36ff903d6 100644
>> --- a/xen/include/xen/mm.h
>> +++ b/xen/include/xen/mm.h
>> @@ -588,33 +588,6 @@ unsigned long get_upper_mfn_bound(void);
>>
>> #include <asm/flushtlb.h>
>>
>> -static inline void accumulate_tlbflush(bool *need_tlbflush,
>> - const struct page_info *page,
>> - uint32_t *tlbflush_timestamp)
>> -{
>> - if ( page->u.free.need_tlbflush &&
>> - page->tlbflush_timestamp <= tlbflush_current_time() &&
>> - (!*need_tlbflush ||
>> - page->tlbflush_timestamp > *tlbflush_timestamp) )
>> - {
>> - *need_tlbflush = true;
>> - *tlbflush_timestamp = page->tlbflush_timestamp;
>> - }
>> -}
>> -
>> -static inline void filtered_flush_tlb_mask(uint32_t tlbflush_timestamp)
>> -{
>> - cpumask_t mask;
>> -
>> - cpumask_copy(&mask, &cpu_online_map);
>> - tlbflush_filter(&mask, tlbflush_timestamp);
>> - if ( !cpumask_empty(&mask) )
>> - {
>> - perfc_incr(need_flush_tlb_flush);
>> - arch_flush_tlb_mask(&mask);
>> - }
>> -}
>> -
>> enum XENSHARE_flags {
>> SHARE_rw,
>> SHARE_ro,
>> diff --git a/xen/include/xen/tlb-clock.h b/xen/include/xen/tlb-clock.h
>> new file mode 100644
>> index 000000000000..796c0be7fbef
>> --- /dev/null
>> +++ b/xen/include/xen/tlb-clock.h
>> @@ -0,0 +1,49 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +#ifndef XEN_TLB_CLOCK_H
>> +#define XEN_TLB_CLOCK_H
>> +
>> +#include <xen/types.h>
>> +
>> +#ifdef CONFIG_HAS_TLB_CLOCK
>> +
>> +#include <xen/mm.h>
>> +
>> +#include <asm/flushtlb.h>
>> +
>> +static inline void accumulate_tlbflush(
>> + bool *need_tlbflush, const struct page_info *page,
>> + uint32_t *tlbflush_timestamp)
>> +{
>> + if ( page->u.free.need_tlbflush &&
>> + page->tlbflush_timestamp <= tlbflush_current_time() &&
>> + (!*need_tlbflush ||
>> + page->tlbflush_timestamp > *tlbflush_timestamp) )
>> + {
>> + *need_tlbflush = true;
>> + *tlbflush_timestamp = page->tlbflush_timestamp;
>> + }
>> +}
>> +
>> +static inline void filtered_flush_tlb_mask(uint32_t tlbflush_timestamp)
>> +{
>> + cpumask_t mask;
>> +
>> + cpumask_copy(&mask, &cpu_online_map);
>> + tlbflush_filter(&mask, tlbflush_timestamp);
>> + if ( !cpumask_empty(&mask) )
>> + {
>> + perfc_incr(need_flush_tlb_flush);
> Would this perf counter then perhaps also want to become dependent upon
> HAS_TLB_CLOCK=y (or become an arch-specific one)? It's used elsewhere in x86,
> but not for any of the other arch-es.
There's nothing inherently x86-specific about our tlb-clk implementation.
I don't have time to do it, but if another arch wants to add support,
then it's probably just a case of shuffling some bits out of
asm/flushtlb.h into tlb-clock.h.
>
> However, see below.
>
>> + arch_flush_tlb_mask(&mask);
>> + }
>> +}
>> +
>> +#else /* !CONFIG_HAS_TLB_CLOCK */
>> +
>> +struct page_info;
>> +static inline void accumulate_tlbflush(
>> + bool *need_tlbflush, const struct page_info *page,
>> + uint32_t *tlbflush_timestamp) {}
>> +static inline void filtered_flush_tlb_mask(uint32_t tlbflush_timestamp) {}
> Is doing nothing here correct?
Yeah, it's not, but this only occurred to me after sending the series.
Interestingly, CI is green across the board for ARM, which suggests to
me that this logic isn't getting a workout.
> mark_page_free() can set a page's
> ->u.free.need_tlbflush. And with that flag set the full
>
> static inline void accumulate_tlbflush(
> bool *need_tlbflush, const struct page_info *page,
> uint32_t *tlbflush_timestamp)
> {
> if ( page->u.free.need_tlbflush &&
> page->tlbflush_timestamp <= tlbflush_current_time() &&
> (!*need_tlbflush ||
> page->tlbflush_timestamp > *tlbflush_timestamp) )
> {
> *need_tlbflush = true;
> *tlbflush_timestamp = page->tlbflush_timestamp;
> }
> }
>
> reduces to (considering that tlbflush_current_time() resolves to constant 0,
> which also implies every page's ->tlbflush_timestamp is only ever 0)
>
> static inline void accumulate_tlbflush(
> bool *need_tlbflush, const struct page_info *page,
> uint32_t *tlbflush_timestamp)
> {
> if ( !*need_tlbflush )
> *need_tlbflush = true;
> }
>
> which means a not-stubbed-out filtered_flush_tlb_mask(), with tlbflush_filter()
> doing nothing, would actually invoke arch_flush_tlb_mask() (with all online CPUs
> set in the mask) when called. And arch_flush_tlb_mask() isn't a no-op on Arm.
Yes. Sadly, fixing this (without Eclair complaining in the middle of
the series) isn't as easy as I'd hoped.
> I therefore think that while moving stuff into a separate header makes sense,
> HAS_TLB_CLOCK isn't overly useful to introduce.
It takes a cpumask_t off the stack, because we can pass cpu_online_mask
straight into arch_flush_tlb_mask(), and it removes a bitmap_copy that
the compiler can't optimise out.
~Andrew
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 4/8] xen/common: Split tlb-clock.h out of mm.h
2025-03-13 13:35 ` Andrew Cooper
@ 2025-03-13 19:43 ` Nicola Vetrini
2025-03-13 19:55 ` Andrew Cooper
0 siblings, 1 reply; 22+ messages in thread
From: Nicola Vetrini @ 2025-03-13 19:43 UTC (permalink / raw)
To: Andrew Cooper
Cc: Jan Beulich, Anthony PERARD, Michal Orzel, Julien Grall,
Roger Pau Monné, Stefano Stabellini, Volodymyr Babchuk,
Bertrand Marquis, Oleksii Kurochko, Shawn Anastasio, Xen-devel
On 2025-03-13 14:35, Andrew Cooper wrote:
> On 13/03/2025 12:59 pm, Jan Beulich wrote:
>> On 12.03.2025 18:45, Andrew Cooper wrote:
>>> xen/mm.h includes asm/tlbflush.h almost at the end, which creates a
>>> horrible
>>> tangle. This is in order to provide two common files with an
>>> abstraction over
>>> the x86-specific TLB clock logic.
>>>
>>> First, introduce CONFIG_HAS_TLB_CLOCK, selected by x86 only. Next,
>>> introduce
>>> xen/tlb-clock.h, providing empty stubs, and include this into
>>> memory.c and
>>> page_alloc.c
>>>
>>> No functional change.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> ---
>>> CC: Anthony PERARD <anthony.perard@vates.tech>
>>> CC: Michal Orzel <michal.orzel@amd.com>
>>> CC: Jan Beulich <jbeulich@suse.com>
>>> CC: Julien Grall <julien@xen.org>
>>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>> CC: Stefano Stabellini <sstabellini@kernel.org>
>>> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
>>> CC: Bertrand Marquis <bertrand.marquis@arm.com>
>>> CC: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>>> CC: Shawn Anastasio <sanastasio@raptorengineering.com>
>>>
>>> There is still a mess here with the common vs x86 split, but it's
>>> better
>>> contained than before.
>>> ---
>>> xen/arch/x86/Kconfig | 1 +
>>> xen/common/Kconfig | 3 +++
>>> xen/common/memory.c | 1 +
>>> xen/common/page_alloc.c | 1 +
>>> xen/include/xen/mm.h | 27 --------------------
>>> xen/include/xen/tlb-clock.h | 49
>>> +++++++++++++++++++++++++++++++++++++
>>> 6 files changed, 55 insertions(+), 27 deletions(-)
>>> create mode 100644 xen/include/xen/tlb-clock.h
>>>
>> However, see below.
>>
>>> + arch_flush_tlb_mask(&mask);
>>> + }
>>> +}
>>> +
>>> +#else /* !CONFIG_HAS_TLB_CLOCK */
>>> +
>>> +struct page_info;
>>> +static inline void accumulate_tlbflush(
>>> + bool *need_tlbflush, const struct page_info *page,
>>> + uint32_t *tlbflush_timestamp) {}
>>> +static inline void filtered_flush_tlb_mask(uint32_t
>>> tlbflush_timestamp) {}
>> Is doing nothing here correct?
>
> Yeah, it's not, but this only occurred to me after sending the series.
>
> Interestingly, CI is green across the board for ARM, which suggests to
> me that this logic isn't getting a workout.
>
>> mark_page_free() can set a page's
>> ->u.free.need_tlbflush. And with that flag set the full
>>
>> static inline void accumulate_tlbflush(
>> bool *need_tlbflush, const struct page_info *page,
>> uint32_t *tlbflush_timestamp)
>> {
>> if ( page->u.free.need_tlbflush &&
>> page->tlbflush_timestamp <= tlbflush_current_time() &&
>> (!*need_tlbflush ||
>> page->tlbflush_timestamp > *tlbflush_timestamp) )
>> {
>> *need_tlbflush = true;
>> *tlbflush_timestamp = page->tlbflush_timestamp;
>> }
>> }
>>
>> reduces to (considering that tlbflush_current_time() resolves to
>> constant 0,
>> which also implies every page's ->tlbflush_timestamp is only ever 0)
>>
>> static inline void accumulate_tlbflush(
>> bool *need_tlbflush, const struct page_info *page,
>> uint32_t *tlbflush_timestamp)
>> {
>> if ( !*need_tlbflush )
>> *need_tlbflush = true;
>> }
>>
>> which means a not-stubbed-out filtered_flush_tlb_mask(), with
>> tlbflush_filter()
>> doing nothing, would actually invoke arch_flush_tlb_mask() (with all
>> online CPUs
>> set in the mask) when called. And arch_flush_tlb_mask() isn't a no-op
>> on Arm.
>
> Yes. Sadly, fixing this (without Eclair complaining in the middle of
> the series) isn't as easy as I'd hoped.
>
Hi Andrew,
I didn't quite follow the whole thread (been busy the last couple of
days), but could you explain briefly what's the issue here? Just a link
to a failing pipeline should be fine as well.
>> I therefore think that while moving stuff into a separate header makes
>> sense,
>> HAS_TLB_CLOCK isn't overly useful to introduce.
>
> It takes a cpumask_t off the stack, because we can pass cpu_online_mask
> straight into arch_flush_tlb_mask(), and it removes a bitmap_copy that
> the compiler can't optimise out.
>
> ~Andrew
--
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 4/8] xen/common: Split tlb-clock.h out of mm.h
2025-03-13 19:43 ` Nicola Vetrini
@ 2025-03-13 19:55 ` Andrew Cooper
0 siblings, 0 replies; 22+ messages in thread
From: Andrew Cooper @ 2025-03-13 19:55 UTC (permalink / raw)
To: Nicola Vetrini
Cc: Jan Beulich, Anthony PERARD, Michal Orzel, Julien Grall,
Roger Pau Monné, Stefano Stabellini, Volodymyr Babchuk,
Bertrand Marquis, Oleksii Kurochko, Shawn Anastasio, Xen-devel
On 13/03/2025 7:43 pm, Nicola Vetrini wrote:
> On 2025-03-13 14:35, Andrew Cooper wrote:
>> On 13/03/2025 12:59 pm, Jan Beulich wrote:
>>> On 12.03.2025 18:45, Andrew Cooper wrote:
>>>> xen/mm.h includes asm/tlbflush.h almost at the end, which creates a
>>>> horrible
>>>> tangle. This is in order to provide two common files with an
>>>> abstraction over
>>>> the x86-specific TLB clock logic.
>>>>
>>>> First, introduce CONFIG_HAS_TLB_CLOCK, selected by x86 only. Next,
>>>> introduce
>>>> xen/tlb-clock.h, providing empty stubs, and include this into
>>>> memory.c and
>>>> page_alloc.c
>>>>
>>>> No functional change.
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> ---
>>>> CC: Anthony PERARD <anthony.perard@vates.tech>
>>>> CC: Michal Orzel <michal.orzel@amd.com>
>>>> CC: Jan Beulich <jbeulich@suse.com>
>>>> CC: Julien Grall <julien@xen.org>
>>>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>>> CC: Stefano Stabellini <sstabellini@kernel.org>
>>>> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
>>>> CC: Bertrand Marquis <bertrand.marquis@arm.com>
>>>> CC: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>>>> CC: Shawn Anastasio <sanastasio@raptorengineering.com>
>>>>
>>>> There is still a mess here with the common vs x86 split, but it's
>>>> better
>>>> contained than before.
>>>> ---
>>>> xen/arch/x86/Kconfig | 1 +
>>>> xen/common/Kconfig | 3 +++
>>>> xen/common/memory.c | 1 +
>>>> xen/common/page_alloc.c | 1 +
>>>> xen/include/xen/mm.h | 27 --------------------
>>>> xen/include/xen/tlb-clock.h | 49
>>>> +++++++++++++++++++++++++++++++++++++
>>>> 6 files changed, 55 insertions(+), 27 deletions(-)
>>>> create mode 100644 xen/include/xen/tlb-clock.h
>>>>
>
>
>>> However, see below.
>>>
>>>> + arch_flush_tlb_mask(&mask);
>>>> + }
>>>> +}
>>>> +
>>>> +#else /* !CONFIG_HAS_TLB_CLOCK */
>>>> +
>>>> +struct page_info;
>>>> +static inline void accumulate_tlbflush(
>>>> + bool *need_tlbflush, const struct page_info *page,
>>>> + uint32_t *tlbflush_timestamp) {}
>>>> +static inline void filtered_flush_tlb_mask(uint32_t
>>>> tlbflush_timestamp) {}
>>> Is doing nothing here correct?
>>
>> Yeah, it's not, but this only occurred to me after sending the series.
>>
>> Interestingly, CI is green across the board for ARM, which suggests to
>> me that this logic isn't getting a workout.
>>
>>> mark_page_free() can set a page's
>>> ->u.free.need_tlbflush. And with that flag set the full
>>>
>>> static inline void accumulate_tlbflush(
>>> bool *need_tlbflush, const struct page_info *page,
>>> uint32_t *tlbflush_timestamp)
>>> {
>>> if ( page->u.free.need_tlbflush &&
>>> page->tlbflush_timestamp <= tlbflush_current_time() &&
>>> (!*need_tlbflush ||
>>> page->tlbflush_timestamp > *tlbflush_timestamp) )
>>> {
>>> *need_tlbflush = true;
>>> *tlbflush_timestamp = page->tlbflush_timestamp;
>>> }
>>> }
>>>
>>> reduces to (considering that tlbflush_current_time() resolves to
>>> constant 0,
>>> which also implies every page's ->tlbflush_timestamp is only ever 0)
>>>
>>> static inline void accumulate_tlbflush(
>>> bool *need_tlbflush, const struct page_info *page,
>>> uint32_t *tlbflush_timestamp)
>>> {
>>> if ( !*need_tlbflush )
>>> *need_tlbflush = true;
>>> }
>>>
>>> which means a not-stubbed-out filtered_flush_tlb_mask(), with
>>> tlbflush_filter()
>>> doing nothing, would actually invoke arch_flush_tlb_mask() (with all
>>> online CPUs
>>> set in the mask) when called. And arch_flush_tlb_mask() isn't a
>>> no-op on Arm.
>>
>> Yes. Sadly, fixing this (without Eclair complaining in the middle of
>> the series) isn't as easy as I'd hoped.
>>
>
> Hi Andrew,
>
> I didn't quite follow the whole thread (been busy the last couple of
> days), but could you explain briefly what's the issue here? Just a
> link to a failing pipeline should be fine as well.
There isn't one.
But to untangle this the easy way, I'd need to have a duplicate
declaration for arch_flush_tlb_mask() for a patch of two.
Which I know Eclair would complain about, and therefore I need to find a
different way to untangle it.
~Andrew
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 4/8] xen/common: Split tlk-clock.h out of mm.h
2025-03-12 17:45 [PATCH 0/8] xen: Untangle mm.h Andrew Cooper
` (3 preceding siblings ...)
2025-03-12 17:45 ` [PATCH 4/8] xen/common: Split tlb-clock.h out of mm.h Andrew Cooper
@ 2025-03-12 17:45 ` Andrew Cooper
2025-03-12 17:45 ` [PATCH 5/8] xen/arch: Strip out tlb-clock stubs for non-implementors Andrew Cooper
` (4 subsequent siblings)
9 siblings, 0 replies; 22+ messages in thread
From: Andrew Cooper @ 2025-03-12 17:45 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, Anthony PERARD, Michal Orzel, Jan Beulich,
Julien Grall, Roger Pau Monné, Stefano Stabellini,
Volodymyr Babchuk, Bertrand Marquis, Oleksii Kurochko,
Shawn Anastasio
xen/mm.h includes asm/tlbflush.h almost at the end, which creates a horrible
tangle. This is in order to provide two common files with an abstraction over
the x86-specific TLB clock logic.
First, introduce CONFIG_HAS_TLB_CLOCK, selected by x86 only. Next, introduce
xen/tlb-clock.h, providing empty stubs, and include this into memory.c and
page_alloc.c
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Anthony PERARD <anthony.perard@vates.tech>
CC: Michal Orzel <michal.orzel@amd.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Julien Grall <julien@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Bertrand Marquis <bertrand.marquis@arm.com>
CC: Oleksii Kurochko <oleksii.kurochko@gmail.com>
CC: Shawn Anastasio <sanastasio@raptorengineering.com>
There is still a mess here with the common vs x86 split, but it's better
contained than before.
---
xen/arch/x86/Kconfig | 1 +
xen/common/Kconfig | 3 +++
xen/common/memory.c | 1 +
xen/common/page_alloc.c | 1 +
xen/include/xen/mm.h | 27 --------------------
xen/include/xen/tlb-clock.h | 49 +++++++++++++++++++++++++++++++++++++
6 files changed, 55 insertions(+), 27 deletions(-)
create mode 100644 xen/include/xen/tlb-clock.h
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 6e41bc0fb435..e9a166ee3dd0 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -28,6 +28,7 @@ config X86
select HAS_PCI_MSI
select HAS_PIRQ
select HAS_SCHED_GRANULARITY
+ select HAS_TLB_CLOCK
select HAS_UBSAN
select HAS_VMAP
select HAS_VPCI if HVM
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 6166327f4d14..dcf7d9d00d0a 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -83,6 +83,9 @@ config HAS_PMAP
config HAS_SCHED_GRANULARITY
bool
+config HAS_TLB_CLOCK
+ bool
+
config HAS_UBSAN
bool
diff --git a/xen/common/memory.c b/xen/common/memory.c
index 61a94b23abae..9138fd096696 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -24,6 +24,7 @@
#include <xen/perfc.h>
#include <xen/sched.h>
#include <xen/sections.h>
+#include <xen/tlb-clock.h>
#include <xen/trace.h>
#include <asm/current.h>
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index bc029ea797a2..b90c3d7988b4 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -137,6 +137,7 @@
#include <xen/sections.h>
#include <xen/softirq.h>
#include <xen/spinlock.h>
+#include <xen/tlb-clock.h>
#include <xen/vm_event.h>
#include <xen/xvmalloc.h>
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 45000cc3f64b..fff36ff903d6 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -588,33 +588,6 @@ unsigned long get_upper_mfn_bound(void);
#include <asm/flushtlb.h>
-static inline void accumulate_tlbflush(bool *need_tlbflush,
- const struct page_info *page,
- uint32_t *tlbflush_timestamp)
-{
- if ( page->u.free.need_tlbflush &&
- page->tlbflush_timestamp <= tlbflush_current_time() &&
- (!*need_tlbflush ||
- page->tlbflush_timestamp > *tlbflush_timestamp) )
- {
- *need_tlbflush = true;
- *tlbflush_timestamp = page->tlbflush_timestamp;
- }
-}
-
-static inline void filtered_flush_tlb_mask(uint32_t tlbflush_timestamp)
-{
- cpumask_t mask;
-
- cpumask_copy(&mask, &cpu_online_map);
- tlbflush_filter(&mask, tlbflush_timestamp);
- if ( !cpumask_empty(&mask) )
- {
- perfc_incr(need_flush_tlb_flush);
- arch_flush_tlb_mask(&mask);
- }
-}
-
enum XENSHARE_flags {
SHARE_rw,
SHARE_ro,
diff --git a/xen/include/xen/tlb-clock.h b/xen/include/xen/tlb-clock.h
new file mode 100644
index 000000000000..796c0be7fbef
--- /dev/null
+++ b/xen/include/xen/tlb-clock.h
@@ -0,0 +1,49 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef XEN_TLB_CLOCK_H
+#define XEN_TLB_CLOCK_H
+
+#include <xen/types.h>
+
+#ifdef CONFIG_HAS_TLB_CLOCK
+
+#include <xen/mm.h>
+
+#include <asm/flushtlb.h>
+
+static inline void accumulate_tlbflush(
+ bool *need_tlbflush, const struct page_info *page,
+ uint32_t *tlbflush_timestamp)
+{
+ if ( page->u.free.need_tlbflush &&
+ page->tlbflush_timestamp <= tlbflush_current_time() &&
+ (!*need_tlbflush ||
+ page->tlbflush_timestamp > *tlbflush_timestamp) )
+ {
+ *need_tlbflush = true;
+ *tlbflush_timestamp = page->tlbflush_timestamp;
+ }
+}
+
+static inline void filtered_flush_tlb_mask(uint32_t tlbflush_timestamp)
+{
+ cpumask_t mask;
+
+ cpumask_copy(&mask, &cpu_online_map);
+ tlbflush_filter(&mask, tlbflush_timestamp);
+ if ( !cpumask_empty(&mask) )
+ {
+ perfc_incr(need_flush_tlb_flush);
+ arch_flush_tlb_mask(&mask);
+ }
+}
+
+#else /* !CONFIG_HAS_TLB_CLOCK */
+
+struct page_info;
+static inline void accumulate_tlbflush(
+ bool *need_tlbflush, const struct page_info *page,
+ uint32_t *tlbflush_timestamp) {}
+static inline void filtered_flush_tlb_mask(uint32_t tlbflush_timestamp) {}
+
+#endif /* !CONFIG_HAS_TLB_CLOCK*/
+#endif /* XEN_TLB_CLOCK_H */
--
2.39.5
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 5/8] xen/arch: Strip out tlb-clock stubs for non-implementors
2025-03-12 17:45 [PATCH 0/8] xen: Untangle mm.h Andrew Cooper
` (4 preceding siblings ...)
2025-03-12 17:45 ` [PATCH 4/8] xen/common: Split tlk-clock.h " Andrew Cooper
@ 2025-03-12 17:45 ` Andrew Cooper
2025-03-13 13:05 ` Jan Beulich
2025-03-12 17:45 ` [PATCH 6/8] xen/mm: Exclude flushtlb.h from mm.h for PPC and RISC-V Andrew Cooper
` (3 subsequent siblings)
9 siblings, 1 reply; 22+ messages in thread
From: Andrew Cooper @ 2025-03-12 17:45 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, Anthony PERARD, Michal Orzel, Jan Beulich,
Julien Grall, Roger Pau Monné, Stefano Stabellini,
Volodymyr Babchuk, Bertrand Marquis, Oleksii Kurochko,
Shawn Anastasio
Now that there's a common stub implementation TLB clocks, there's no need for
architectures to provide their own.
Repeatedly zeroing page->tlbflush_timestamp is no use, so provide an even more
empty common stub for page_set_tlbflush_timestamp().
No practical change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Anthony PERARD <anthony.perard@vates.tech>
CC: Michal Orzel <michal.orzel@amd.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Julien Grall <julien@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Bertrand Marquis <bertrand.marquis@arm.com>
CC: Oleksii Kurochko <oleksii.kurochko@gmail.com>
CC: Shawn Anastasio <sanastasio@raptorengineering.com>
---
xen/arch/arm/include/asm/flushtlb.h | 14 --------------
xen/arch/ppc/include/asm/flushtlb.h | 14 --------------
xen/arch/riscv/include/asm/flushtlb.h | 14 --------------
xen/include/xen/tlb-clock.h | 1 +
4 files changed, 1 insertion(+), 42 deletions(-)
diff --git a/xen/arch/arm/include/asm/flushtlb.h b/xen/arch/arm/include/asm/flushtlb.h
index e45fb6d97b02..6f69a5bdc8c2 100644
--- a/xen/arch/arm/include/asm/flushtlb.h
+++ b/xen/arch/arm/include/asm/flushtlb.h
@@ -3,20 +3,6 @@
#include <xen/cpumask.h>
-/*
- * Filter the given set of CPUs, removing those that definitely flushed their
- * TLB since @page_timestamp.
- */
-/* XXX lazy implementation just doesn't clear anything.... */
-static inline void tlbflush_filter(cpumask_t *mask, uint32_t page_timestamp) {}
-
-#define tlbflush_current_time() (0)
-
-static inline void page_set_tlbflush_timestamp(struct page_info *page)
-{
- page->tlbflush_timestamp = tlbflush_current_time();
-}
-
#if defined(CONFIG_ARM_32)
# include <asm/arm32/flushtlb.h>
#elif defined(CONFIG_ARM_64)
diff --git a/xen/arch/ppc/include/asm/flushtlb.h b/xen/arch/ppc/include/asm/flushtlb.h
index afcb74078368..f89037bd4543 100644
--- a/xen/arch/ppc/include/asm/flushtlb.h
+++ b/xen/arch/ppc/include/asm/flushtlb.h
@@ -4,20 +4,6 @@
#include <xen/cpumask.h>
-/*
- * Filter the given set of CPUs, removing those that definitely flushed their
- * TLB since @page_timestamp.
- */
-/* XXX lazy implementation just doesn't clear anything.... */
-static inline void tlbflush_filter(cpumask_t *mask, uint32_t page_timestamp) {}
-
-#define tlbflush_current_time() (0)
-
-static inline void page_set_tlbflush_timestamp(struct page_info *page)
-{
- page->tlbflush_timestamp = tlbflush_current_time();
-}
-
/* Flush specified CPUs' TLBs */
void arch_flush_tlb_mask(const cpumask_t *mask);
diff --git a/xen/arch/riscv/include/asm/flushtlb.h b/xen/arch/riscv/include/asm/flushtlb.h
index 51c8f753c51e..23739a22fb2a 100644
--- a/xen/arch/riscv/include/asm/flushtlb.h
+++ b/xen/arch/riscv/include/asm/flushtlb.h
@@ -20,20 +20,6 @@ static inline void flush_tlb_range_va(vaddr_t va, size_t size)
sbi_remote_sfence_vma(NULL, va, size);
}
-/*
- * Filter the given set of CPUs, removing those that definitely flushed their
- * TLB since @page_timestamp.
- */
-/* XXX lazy implementation just doesn't clear anything.... */
-static inline void tlbflush_filter(cpumask_t *mask, uint32_t page_timestamp) {}
-
-#define tlbflush_current_time() (0)
-
-static inline void page_set_tlbflush_timestamp(struct page_info *page)
-{
- BUG_ON("unimplemented");
-}
-
/* Flush specified CPUs' TLBs */
void arch_flush_tlb_mask(const cpumask_t *mask);
diff --git a/xen/include/xen/tlb-clock.h b/xen/include/xen/tlb-clock.h
index 796c0be7fbef..467f6d64a6ca 100644
--- a/xen/include/xen/tlb-clock.h
+++ b/xen/include/xen/tlb-clock.h
@@ -44,6 +44,7 @@ static inline void accumulate_tlbflush(
bool *need_tlbflush, const struct page_info *page,
uint32_t *tlbflush_timestamp) {}
static inline void filtered_flush_tlb_mask(uint32_t tlbflush_timestamp) {}
+static inline void page_set_tlbflush_timestamp(struct page_info *page) {}
#endif /* !CONFIG_HAS_TLB_CLOCK*/
#endif /* XEN_TLB_CLOCK_H */
--
2.39.5
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 5/8] xen/arch: Strip out tlb-clock stubs for non-implementors
2025-03-12 17:45 ` [PATCH 5/8] xen/arch: Strip out tlb-clock stubs for non-implementors Andrew Cooper
@ 2025-03-13 13:05 ` Jan Beulich
2025-03-13 14:11 ` Andrew Cooper
0 siblings, 1 reply; 22+ messages in thread
From: Jan Beulich @ 2025-03-13 13:05 UTC (permalink / raw)
To: Andrew Cooper
Cc: Anthony PERARD, Michal Orzel, Julien Grall, Roger Pau Monné,
Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis,
Oleksii Kurochko, Shawn Anastasio, Xen-devel
On 12.03.2025 18:45, Andrew Cooper wrote:
> Now that there's a common stub implementation TLB clocks, there's no need for
> architectures to provide their own.
>
> Repeatedly zeroing page->tlbflush_timestamp is no use, so provide an even more
> empty common stub for page_set_tlbflush_timestamp().
At which point the field itself could in principle go away. There are three
printk()s (accompanying BUG()s) which use it; surely we can find a way to
abstract that out. This may then still be enough of a reason to introduce
HAS_TLB_CLOCK.
Jan
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 5/8] xen/arch: Strip out tlb-clock stubs for non-implementors
2025-03-13 13:05 ` Jan Beulich
@ 2025-03-13 14:11 ` Andrew Cooper
2025-03-13 14:22 ` Jan Beulich
0 siblings, 1 reply; 22+ messages in thread
From: Andrew Cooper @ 2025-03-13 14:11 UTC (permalink / raw)
To: Jan Beulich
Cc: Anthony PERARD, Michal Orzel, Julien Grall, Roger Pau Monné,
Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis,
Oleksii Kurochko, Shawn Anastasio, Xen-devel
On 13/03/2025 1:05 pm, Jan Beulich wrote:
> On 12.03.2025 18:45, Andrew Cooper wrote:
>> Now that there's a common stub implementation TLB clocks, there's no need for
>> architectures to provide their own.
>>
>> Repeatedly zeroing page->tlbflush_timestamp is no use, so provide an even more
>> empty common stub for page_set_tlbflush_timestamp().
> At which point the field itself could in principle go away. There are three
> printk()s (accompanying BUG()s) which use it; surely we can find a way to
> abstract that out. This may then still be enough of a reason to introduce
> HAS_TLB_CLOCK.
I wanted to remove the field, but it wasn't trivial, and I've probably
spent more time than I can afford on this.
I'm tempted to leave a TODO in tlb-clock.h to make it clear that there's
more that ought to be done.
~Andrew
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 5/8] xen/arch: Strip out tlb-clock stubs for non-implementors
2025-03-13 14:11 ` Andrew Cooper
@ 2025-03-13 14:22 ` Jan Beulich
0 siblings, 0 replies; 22+ messages in thread
From: Jan Beulich @ 2025-03-13 14:22 UTC (permalink / raw)
To: Andrew Cooper
Cc: Anthony PERARD, Michal Orzel, Julien Grall, Roger Pau Monné,
Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis,
Oleksii Kurochko, Shawn Anastasio, Xen-devel
On 13.03.2025 15:11, Andrew Cooper wrote:
> On 13/03/2025 1:05 pm, Jan Beulich wrote:
>> On 12.03.2025 18:45, Andrew Cooper wrote:
>>> Now that there's a common stub implementation TLB clocks, there's no need for
>>> architectures to provide their own.
>>>
>>> Repeatedly zeroing page->tlbflush_timestamp is no use, so provide an even more
>>> empty common stub for page_set_tlbflush_timestamp().
>> At which point the field itself could in principle go away. There are three
>> printk()s (accompanying BUG()s) which use it; surely we can find a way to
>> abstract that out. This may then still be enough of a reason to introduce
>> HAS_TLB_CLOCK.
>
> I wanted to remove the field, but it wasn't trivial, and I've probably
> spent more time than I can afford on this.
I can understand this. It'll remain to be seen how useful HAS_TLB_CLOCK is
with patch 4 corrected. And of course it's ...
> I'm tempted to leave a TODO in tlb-clock.h to make it clear that there's
> more that ought to be done.
... kind of okay to leave parts to be done later, as long as it's at least
halfway clear what it is that wants doing.
Jan
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 6/8] xen/mm: Exclude flushtlb.h from mm.h for PPC and RISC-V
2025-03-12 17:45 [PATCH 0/8] xen: Untangle mm.h Andrew Cooper
` (5 preceding siblings ...)
2025-03-12 17:45 ` [PATCH 5/8] xen/arch: Strip out tlb-clock stubs for non-implementors Andrew Cooper
@ 2025-03-12 17:45 ` Andrew Cooper
2025-03-12 17:45 ` [PATCH 7/8] xen/mm: Exclude flushtlb.h from mm.h for ARM Andrew Cooper
` (2 subsequent siblings)
9 siblings, 0 replies; 22+ messages in thread
From: Andrew Cooper @ 2025-03-12 17:45 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, Anthony PERARD, Michal Orzel, Jan Beulich,
Julien Grall, Roger Pau Monné, Stefano Stabellini,
Volodymyr Babchuk, Bertrand Marquis, Oleksii Kurochko,
Shawn Anastasio
put_page_alloc_ref(), the final function in xen/mm.h uses test_and_clear_bit()
which is picked up transitively by all architectures. RISC-V gets it only via
flushtlb.h, hence why it notices here.
ARM and x86 will be cleaned up in subsequent patches.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Anthony PERARD <anthony.perard@vates.tech>
CC: Michal Orzel <michal.orzel@amd.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Julien Grall <julien@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Bertrand Marquis <bertrand.marquis@arm.com>
CC: Oleksii Kurochko <oleksii.kurochko@gmail.com>
CC: Shawn Anastasio <sanastasio@raptorengineering.com>
---
xen/include/xen/mm.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index fff36ff903d6..154e649db9e4 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -61,6 +61,7 @@
#ifndef __XEN_MM_H__
#define __XEN_MM_H__
+#include <xen/bitops.h>
#include <xen/bug.h>
#include <xen/compiler.h>
#include <xen/list.h>
@@ -586,7 +587,9 @@ void destroy_ring_for_helper(void **_va, struct page_info *page);
/* Return the upper bound of MFNs, including hotplug memory. */
unsigned long get_upper_mfn_bound(void);
+#if defined(CONFIG_X86) || defined(CONFIG_ARM)
#include <asm/flushtlb.h>
+#endif
enum XENSHARE_flags {
SHARE_rw,
--
2.39.5
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 7/8] xen/mm: Exclude flushtlb.h from mm.h for ARM
2025-03-12 17:45 [PATCH 0/8] xen: Untangle mm.h Andrew Cooper
` (6 preceding siblings ...)
2025-03-12 17:45 ` [PATCH 6/8] xen/mm: Exclude flushtlb.h from mm.h for PPC and RISC-V Andrew Cooper
@ 2025-03-12 17:45 ` Andrew Cooper
2025-03-12 17:45 ` [PATCH 8/8] xen/mm: Exclude flushtlb.h from mm.h for x86 Andrew Cooper
2025-03-12 18:02 ` [PATCH 0/8] xen: Untangle mm.h Andrew Cooper
9 siblings, 0 replies; 22+ messages in thread
From: Andrew Cooper @ 2025-03-12 17:45 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, Anthony PERARD, Michal Orzel, Jan Beulich,
Julien Grall, Roger Pau Monné, Stefano Stabellini,
Volodymyr Babchuk, Bertrand Marquis, Oleksii Kurochko,
Shawn Anastasio
A number of files pick up flushtlb.h transitively through mm.h, while the
flushtlb.h hierachy themselves aren't even self-sufficient.
Address all of these, and exclude flushtlb.h from mm.h
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Anthony PERARD <anthony.perard@vates.tech>
CC: Michal Orzel <michal.orzel@amd.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Julien Grall <julien@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Bertrand Marquis <bertrand.marquis@arm.com>
CC: Oleksii Kurochko <oleksii.kurochko@gmail.com>
CC: Shawn Anastasio <sanastasio@raptorengineering.com>
---
xen/arch/arm/include/asm/arm32/flushtlb.h | 2 ++
xen/arch/arm/include/asm/arm64/flushtlb.h | 2 ++
xen/arch/arm/include/asm/fixmap.h | 2 ++
xen/arch/arm/include/asm/pmap.h | 1 +
xen/arch/arm/mmu/domain_page.c | 2 ++
xen/arch/arm/mmu/pt.c | 1 +
xen/arch/arm/mmu/setup.c | 1 +
xen/arch/arm/traps.c | 1 +
xen/include/xen/mm.h | 2 +-
9 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/include/asm/arm32/flushtlb.h b/xen/arch/arm/include/asm/arm32/flushtlb.h
index 61c25a318998..510eb649a5c6 100644
--- a/xen/arch/arm/include/asm/arm32/flushtlb.h
+++ b/xen/arch/arm/include/asm/arm32/flushtlb.h
@@ -1,6 +1,8 @@
#ifndef __ASM_ARM_ARM32_FLUSHTLB_H__
#define __ASM_ARM_ARM32_FLUSHTLB_H__
+#include <asm/arm32/sysregs.h>
+
/*
* Every invalidation operation use the following patterns:
*
diff --git a/xen/arch/arm/include/asm/arm64/flushtlb.h b/xen/arch/arm/include/asm/arm64/flushtlb.h
index 45642201d147..b98c8d14a78f 100644
--- a/xen/arch/arm/include/asm/arm64/flushtlb.h
+++ b/xen/arch/arm/include/asm/arm64/flushtlb.h
@@ -1,6 +1,8 @@
#ifndef __ASM_ARM_ARM64_FLUSHTLB_H__
#define __ASM_ARM_ARM64_FLUSHTLB_H__
+#include <asm/alternative.h>
+
/*
* Every invalidation operation use the following patterns:
*
diff --git a/xen/arch/arm/include/asm/fixmap.h b/xen/arch/arm/include/asm/fixmap.h
index 0cb5d54d1c74..68d82d3d4ea2 100644
--- a/xen/arch/arm/include/asm/fixmap.h
+++ b/xen/arch/arm/include/asm/fixmap.h
@@ -22,6 +22,8 @@
#ifndef __ASSEMBLY__
+#include <asm/lpae.h>
+
/*
* Direct access to xen_fixmap[] should only happen when {set,
* clear}_fixmap() is unusable (e.g. where we would end up to
diff --git a/xen/arch/arm/include/asm/pmap.h b/xen/arch/arm/include/asm/pmap.h
index bca3381796f3..1162e8a6e4d2 100644
--- a/xen/arch/arm/include/asm/pmap.h
+++ b/xen/arch/arm/include/asm/pmap.h
@@ -4,6 +4,7 @@
#include <xen/mm.h>
#include <asm/fixmap.h>
+#include <asm/flushtlb.h>
static inline void arch_pmap_map(unsigned int slot, mfn_t mfn)
{
diff --git a/xen/arch/arm/mmu/domain_page.c b/xen/arch/arm/mmu/domain_page.c
index 3a43601623f0..850214925a3b 100644
--- a/xen/arch/arm/mmu/domain_page.c
+++ b/xen/arch/arm/mmu/domain_page.c
@@ -3,6 +3,8 @@
#include <xen/pmap.h>
#include <xen/vmap.h>
+#include <asm/flushtlb.h>
+
/* Override macros from asm/page.h to make them work with mfn_t */
#undef virt_to_mfn
#define virt_to_mfn(va) _mfn(__virt_to_mfn(va))
diff --git a/xen/arch/arm/mmu/pt.c b/xen/arch/arm/mmu/pt.c
index da28d669e796..16c73da853e6 100644
--- a/xen/arch/arm/mmu/pt.c
+++ b/xen/arch/arm/mmu/pt.c
@@ -13,6 +13,7 @@
#include <asm/current.h>
#include <asm/fixmap.h>
+#include <asm/flushtlb.h>
#ifdef NDEBUG
static inline void
diff --git a/xen/arch/arm/mmu/setup.c b/xen/arch/arm/mmu/setup.c
index f6119ccacf15..64a06e30e8fb 100644
--- a/xen/arch/arm/mmu/setup.c
+++ b/xen/arch/arm/mmu/setup.c
@@ -13,6 +13,7 @@
#include <xen/vmap.h>
#include <asm/fixmap.h>
+#include <asm/flushtlb.h>
#include <asm/setup.h>
/* Override macros from asm/page.h to make them work with mfn_t */
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 445e7378dd79..52f267fb11f8 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -34,6 +34,7 @@
#include <asm/cpuerrata.h>
#include <asm/cpufeature.h>
#include <asm/event.h>
+#include <asm/flushtlb.h>
#include <asm/hsr.h>
#include <asm/mem_access.h>
#include <asm/mmio.h>
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 154e649db9e4..27e420e302d8 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -587,7 +587,7 @@ void destroy_ring_for_helper(void **_va, struct page_info *page);
/* Return the upper bound of MFNs, including hotplug memory. */
unsigned long get_upper_mfn_bound(void);
-#if defined(CONFIG_X86) || defined(CONFIG_ARM)
+#if defined(CONFIG_X86)
#include <asm/flushtlb.h>
#endif
--
2.39.5
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 8/8] xen/mm: Exclude flushtlb.h from mm.h for x86
2025-03-12 17:45 [PATCH 0/8] xen: Untangle mm.h Andrew Cooper
` (7 preceding siblings ...)
2025-03-12 17:45 ` [PATCH 7/8] xen/mm: Exclude flushtlb.h from mm.h for ARM Andrew Cooper
@ 2025-03-12 17:45 ` Andrew Cooper
2025-03-13 9:13 ` Jan Beulich
2025-03-12 18:02 ` [PATCH 0/8] xen: Untangle mm.h Andrew Cooper
9 siblings, 1 reply; 22+ messages in thread
From: Andrew Cooper @ 2025-03-12 17:45 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, Anthony PERARD, Michal Orzel, Jan Beulich,
Julien Grall, Roger Pau Monné, Stefano Stabellini,
Volodymyr Babchuk, Bertrand Marquis, Oleksii Kurochko,
Shawn Anastasio
alternative.c and livepatch.c pick up flushtlb.h transitively through mm.h.
Fix these, and finally resolve the TODO in microcode/amd.c
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Anthony PERARD <anthony.perard@vates.tech>
CC: Michal Orzel <michal.orzel@amd.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Julien Grall <julien@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Bertrand Marquis <bertrand.marquis@arm.com>
CC: Oleksii Kurochko <oleksii.kurochko@gmail.com>
CC: Shawn Anastasio <sanastasio@raptorengineering.com>
---
xen/arch/x86/alternative.c | 1 +
xen/arch/x86/cpu/microcode/amd.c | 2 +-
xen/arch/x86/livepatch.c | 1 +
xen/include/xen/mm.h | 4 ----
4 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
index 46b04c9cb83d..d97eda129c32 100644
--- a/xen/arch/x86/alternative.c
+++ b/xen/arch/x86/alternative.c
@@ -10,6 +10,7 @@
#include <asm/alternative.h>
#include <asm/apic.h>
#include <asm/endbr.h>
+#include <asm/flushtlb.h>
#include <asm/nmi.h>
#include <asm/nops.h>
#include <asm/processor.h>
diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
index ee7de5282b2a..d84dc5b0ef1f 100644
--- a/xen/arch/x86/cpu/microcode/amd.c
+++ b/xen/arch/x86/cpu/microcode/amd.c
@@ -17,9 +17,9 @@
#include <xen/bsearch.h>
#include <xen/err.h>
#include <xen/init.h>
-#include <xen/mm.h> /* TODO: Fix asm/tlbflush.h breakage */
#include <xen/sha2.h>
+#include <asm/flushtlb.h>
#include <asm/msr.h>
#include "private.h"
diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index 5158e91f7e6e..5c1d16ecf5a8 100644
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -16,6 +16,7 @@
#include <asm/endbr.h>
#include <asm/fixmap.h>
+#include <asm/flushtlb.h>
#include <asm/nmi.h>
#include <asm/setup.h>
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 27e420e302d8..088f77eed5e5 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -587,10 +587,6 @@ void destroy_ring_for_helper(void **_va, struct page_info *page);
/* Return the upper bound of MFNs, including hotplug memory. */
unsigned long get_upper_mfn_bound(void);
-#if defined(CONFIG_X86)
-#include <asm/flushtlb.h>
-#endif
-
enum XENSHARE_flags {
SHARE_rw,
SHARE_ro,
--
2.39.5
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 8/8] xen/mm: Exclude flushtlb.h from mm.h for x86
2025-03-12 17:45 ` [PATCH 8/8] xen/mm: Exclude flushtlb.h from mm.h for x86 Andrew Cooper
@ 2025-03-13 9:13 ` Jan Beulich
2025-03-13 11:50 ` Andrew Cooper
0 siblings, 1 reply; 22+ messages in thread
From: Jan Beulich @ 2025-03-13 9:13 UTC (permalink / raw)
To: Andrew Cooper
Cc: Anthony PERARD, Michal Orzel, Julien Grall, Roger Pau Monné,
Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis,
Oleksii Kurochko, Shawn Anastasio, Xen-devel
On 12.03.2025 18:45, Andrew Cooper wrote:
> alternative.c and livepatch.c pick up flushtlb.h transitively through mm.h.
>
> Fix these, and finally resolve the TODO in microcode/amd.c
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 8/8] xen/mm: Exclude flushtlb.h from mm.h for x86
2025-03-13 9:13 ` Jan Beulich
@ 2025-03-13 11:50 ` Andrew Cooper
0 siblings, 0 replies; 22+ messages in thread
From: Andrew Cooper @ 2025-03-13 11:50 UTC (permalink / raw)
To: Jan Beulich
Cc: Anthony PERARD, Michal Orzel, Julien Grall, Roger Pau Monné,
Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis,
Oleksii Kurochko, Shawn Anastasio, Xen-devel
On 13/03/2025 9:13 am, Jan Beulich wrote:
> On 12.03.2025 18:45, Andrew Cooper wrote:
>> alternative.c and livepatch.c pick up flushtlb.h transitively through mm.h.
>>
>> Fix these, and finally resolve the TODO in microcode/amd.c
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>
>
>
Thanks. Randconfig thus far has found that
xen/arch/x86/guest/hyperv/tlb.c needs an extra include too, which I've
fixed up locally.
~Andrew
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/8] xen: Untangle mm.h
2025-03-12 17:45 [PATCH 0/8] xen: Untangle mm.h Andrew Cooper
` (8 preceding siblings ...)
2025-03-12 17:45 ` [PATCH 8/8] xen/mm: Exclude flushtlb.h from mm.h for x86 Andrew Cooper
@ 2025-03-12 18:02 ` Andrew Cooper
9 siblings, 0 replies; 22+ messages in thread
From: Andrew Cooper @ 2025-03-12 18:02 UTC (permalink / raw)
To: Xen-devel
Cc: Anthony PERARD, Michal Orzel, Jan Beulich, Julien Grall,
Roger Pau Monné, Stefano Stabellini, Volodymyr Babchuk,
Bertrand Marquis, Oleksii Kurochko, Shawn Anastasio
On 12/03/2025 5:45 pm, Andrew Cooper wrote:
> This started out trying to fix one little TODO in x86's microcode loader, and
> escalated somewhat...
>
> tlb-clock.h is definitely not as clean as it could be, but it's an improvment
> over today, and given how long it's taken to get this to compile, I'm not
> looking to rewrite everything.
The other thing to say, I expect randconfig to shake out some things,
and FreeBSD Randconfig already has.
I'll let the piplines run for a bit before accumulating.
~Andrew
^ permalink raw reply [flat|nested] 22+ messages in thread