xen/arch/ppc/Kconfig | 1 + xen/arch/ppc/include/asm/types.h | 2 ++ xen/arch/ppc/mm-radix.c | 2 +- xen/common/Kconfig | 3 +++ xen/common/efi/boot.c | 4 ++-- xen/common/vmap.c | 2 +- xen/include/xen/mm.h | 8 ++++++-- xen/include/xen/vmap.h | 3 ++- 8 files changed, 18 insertions(+), 7 deletions(-)
Xen's memory management APIs map_pages_to_xen, modify_xen_mappings,
set_fixmap, ioremap_attr, and __vmap all use an unsigned int to
represent architecture-dependent page table entry flags. This assumption
does not work on PPC/radix where some flags go past 32-bits, so
introduce the pte_attr_t type to allow architectures to opt in to larger
types to store PTE flags.
Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Shawn Anastasio <sanastasio@raptorengineering.com>
---
xen/arch/ppc/Kconfig | 1 +
xen/arch/ppc/include/asm/types.h | 2 ++
xen/arch/ppc/mm-radix.c | 2 +-
xen/common/Kconfig | 3 +++
xen/common/efi/boot.c | 4 ++--
xen/common/vmap.c | 2 +-
xen/include/xen/mm.h | 8 ++++++--
xen/include/xen/vmap.h | 3 ++-
8 files changed, 18 insertions(+), 7 deletions(-)
diff --git a/xen/arch/ppc/Kconfig b/xen/arch/ppc/Kconfig
index 6db575a48d..3fae481117 100644
--- a/xen/arch/ppc/Kconfig
+++ b/xen/arch/ppc/Kconfig
@@ -3,6 +3,7 @@ config PPC
select FUNCTION_ALIGNMENT_4B
select HAS_DEVICE_TREE
select HAS_VMAP
+ select HAS_PTE_ATTR_T
config PPC64
def_bool y
diff --git a/xen/arch/ppc/include/asm/types.h b/xen/arch/ppc/include/asm/types.h
index ffaf378a4d..a0b12f816a 100644
--- a/xen/arch/ppc/include/asm/types.h
+++ b/xen/arch/ppc/include/asm/types.h
@@ -8,4 +8,6 @@ typedef unsigned long vaddr_t;
#define INVALID_PADDR (~0UL)
#define PRIpaddr "016lx"
+typedef unsigned long pte_attr_t;
+
#endif /* _ASM_PPC_TYPES_H */
diff --git a/xen/arch/ppc/mm-radix.c b/xen/arch/ppc/mm-radix.c
index 24232f3907..e02dffa7c5 100644
--- a/xen/arch/ppc/mm-radix.c
+++ b/xen/arch/ppc/mm-radix.c
@@ -265,7 +265,7 @@ int destroy_xen_mappings(unsigned long s, unsigned long e)
int map_pages_to_xen(unsigned long virt,
mfn_t mfn,
unsigned long nr_mfns,
- unsigned int flags)
+ pte_attr_t flags)
{
BUG_ON("unimplemented");
}
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 6166327f4d..017075d667 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -80,6 +80,9 @@ config HAS_PIRQ
config HAS_PMAP
bool
+config HAS_PTE_ATTR_T
+ bool
+
config HAS_SCHED_GRANULARITY
bool
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index efbec00af9..c200a27523 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -1656,7 +1656,7 @@ void __init efi_init_memory(void)
struct rt_extra {
struct rt_extra *next;
unsigned long smfn, emfn;
- unsigned int prot;
+ pte_attr_t prot;
} *extra, *extra_head = NULL;
free_ebmalloc_unused_mem();
@@ -1671,7 +1671,7 @@ void __init efi_init_memory(void)
EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
u64 len = desc->NumberOfPages << EFI_PAGE_SHIFT;
unsigned long smfn, emfn;
- unsigned int prot = PAGE_HYPERVISOR_RWX;
+ pte_attr_t prot = PAGE_HYPERVISOR_RWX;
paddr_t mem_base;
unsigned long mem_npages;
diff --git a/xen/common/vmap.c b/xen/common/vmap.c
index 47225fecc0..d6991421f3 100644
--- a/xen/common/vmap.c
+++ b/xen/common/vmap.c
@@ -222,7 +222,7 @@ static void vm_free(const void *va)
}
void *__vmap(const mfn_t *mfn, unsigned int granularity,
- unsigned int nr, unsigned int align, unsigned int flags,
+ unsigned int nr, unsigned int align, pte_attr_t flags,
enum vmap_region type)
{
void *va = vm_alloc(nr * granularity, align, type);
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 16f733281a..5386f931e7 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -70,6 +70,10 @@
#include <xen/perfc.h>
#include <public/memory.h>
+#ifndef CONFIG_HAS_PTE_ATTR_T
+typedef unsigned int pte_attr_t;
+#endif
+
struct page_info;
extern bool using_static_heap;
@@ -113,9 +117,9 @@ int map_pages_to_xen(
unsigned long virt,
mfn_t mfn,
unsigned long nr_mfns,
- unsigned int flags);
+ pte_attr_t flags);
/* Alter the permissions of a range of Xen virtual address space. */
-int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int nf);
+int modify_xen_mappings(unsigned long s, unsigned long e, pte_attr_t nf);
void modify_xen_mappings_lite(unsigned long s, unsigned long e,
unsigned int nf);
int destroy_xen_mappings(unsigned long s, unsigned long e);
diff --git a/xen/include/xen/vmap.h b/xen/include/xen/vmap.h
index 26c831757a..9e2edd1252 100644
--- a/xen/include/xen/vmap.h
+++ b/xen/include/xen/vmap.h
@@ -8,6 +8,7 @@
#ifndef __XEN_VMAP_H__
#define __XEN_VMAP_H__
+#include <xen/mm.h>
#include <xen/mm-frame.h>
#include <xen/page-size.h>
@@ -57,7 +58,7 @@ void vm_init_type(enum vmap_region type, void *start, void *end);
* @return Pointer to the mapped area on success; NULL otherwise.
*/
void *__vmap(const mfn_t *mfn, unsigned int granularity, unsigned int nr,
- unsigned int align, unsigned int flags, enum vmap_region type);
+ unsigned int align, pte_attr_t flags, enum vmap_region type);
/*
* Map an array of pages contiguously into the VMAP_DEFAULT vmap region
--
2.30.2On 31.12.2024 19:27, Shawn Anastasio wrote: > Xen's memory management APIs map_pages_to_xen, modify_xen_mappings, > set_fixmap, ioremap_attr, and __vmap all use an unsigned int to > represent architecture-dependent page table entry flags. This assumption > does not work on PPC/radix where some flags go past 32-bits, so > introduce the pte_attr_t type to allow architectures to opt in to larger > types to store PTE flags. > > Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> > Signed-off-by: Shawn Anastasio <sanastasio@raptorengineering.com> As iirc Andrew had indicated when suggesting this, what you say for PPC is true for x86 as well. Yet still we get around with unsigned int. Hence I think the change needs describing differently. > --- a/xen/include/xen/mm.h > +++ b/xen/include/xen/mm.h > @@ -70,6 +70,10 @@ > #include <xen/perfc.h> > #include <public/memory.h> > > +#ifndef CONFIG_HAS_PTE_ATTR_T > +typedef unsigned int pte_attr_t; > +#endif This imo makes the Kconfig control a misnomer: We'll always have pte_attr_t. I wonder whether this actually needs a Kconfig setting in the first place: It's hardly the end of the world for all architectures to specify the type (later: the underlying type, for the real type to become type-safe) explicitly. Jan
On 1/9/25 3:15 AM, Jan Beulich wrote: > On 31.12.2024 19:27, Shawn Anastasio wrote: >> Xen's memory management APIs map_pages_to_xen, modify_xen_mappings, >> set_fixmap, ioremap_attr, and __vmap all use an unsigned int to >> represent architecture-dependent page table entry flags. This assumption >> does not work on PPC/radix where some flags go past 32-bits, so >> introduce the pte_attr_t type to allow architectures to opt in to larger >> types to store PTE flags. >> >> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> >> Signed-off-by: Shawn Anastasio <sanastasio@raptorengineering.com> > > As iirc Andrew had indicated when suggesting this, what you say for PPC is > true for x86 as well. Yet still we get around with unsigned int. Hence I > think the change needs describing differently. > I see what you mean -- I'll reword this to correctly reflect that the change is not strictly necessary to accomodate architectures with greater than 32-bit pte flags. > Jan Thanks, Shawn
On 09/01/2025 9:15 am, Jan Beulich wrote: > On 31.12.2024 19:27, Shawn Anastasio wrote: >> Xen's memory management APIs map_pages_to_xen, modify_xen_mappings, >> set_fixmap, ioremap_attr, and __vmap all use an unsigned int to >> represent architecture-dependent page table entry flags. This assumption >> does not work on PPC/radix where some flags go past 32-bits, so >> introduce the pte_attr_t type to allow architectures to opt in to larger >> types to store PTE flags. >> >> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> >> Signed-off-by: Shawn Anastasio <sanastasio@raptorengineering.com> > As iirc Andrew had indicated when suggesting this, what you say for PPC is > true for x86 as well. Yet still we get around with unsigned int. Hence I > think the change needs describing differently. x86 is currently unsigned int, and with some reasonably expensive packing/unpacking under the hood. x86 ought to become unsigned long, now we don't have the 32bit build to worry about. (back of the envelope calculation says up/down: 400/-3868 (-3468) but I've not checked that this boots). > >> --- a/xen/include/xen/mm.h >> +++ b/xen/include/xen/mm.h >> @@ -70,6 +70,10 @@ >> #include <xen/perfc.h> >> #include <public/memory.h> >> >> +#ifndef CONFIG_HAS_PTE_ATTR_T >> +typedef unsigned int pte_attr_t; >> +#endif > This imo makes the Kconfig control a misnomer: We'll always have pte_attr_t. > I wonder whether this actually needs a Kconfig setting in the first place: > It's hardly the end of the world for all architectures to specify the type > (later: the underlying type, for the real type to become type-safe) > explicitly. All architectures strictly need this. It's not optional, so doesn't warrant a Kconfig option. Either, arch/mm.h is required to provide the typedef, or we could have a common one as so: #ifndef pte_attr_t typedef unsigned int pte_attr_t; #endif which architectures can influence with: typedef unsigned long pte_attr_t; #define pte_attr_t pte_attr_t in the usual way. One thing however does occur to me. ARM and RISC-V have systems with MPU protections rather than MMU, with Xen already starting to support this on ARM. Therefore we might want to reconsider the name pte_attr_t to be something slightly less pagetable specific. Then again, I'm not sure how much overlap there is between the map* functions and MPUs, where mapping is of course not possible. Finally, this wants to be at least 2 patches. One introducing pte_attr_t, and one changing PPC to be unsigned long. ~Andrew
On 1/9/25 4:56 AM, Andrew Cooper wrote: > Either, arch/mm.h is required to provide the typedef, This approach seems fine to me. I wanted to avoid changes to other architectures where possible, but if this is acceptable then this is probably the route I'd want to go down (see below). For the reason outlined below, this would also probably mean that the typedef would go before other includes at the top of arch/mm.h, and I'm not sure if that would be an issue from a style perspective. Alternatively, I could have arch/type.h define it on every architecture if that would be acceptable. > or we could have a common one as so: > > #ifndef pte_attr_t > typedef unsigned int pte_attr_t; > #endif > > which architectures can influence with: > > typedef unsigned long pte_attr_t; > #define pte_attr_t pte_attr_t > > in the usual way. > There seems to be a pretty messy header dependency tree with xen/mm.h where it #includes arch/mm.h halfway through the file after defining (among other things) the prototypes for the functions that would need pte_attr_t. Defining pte_attr_t this late in the file won't work since the definition also needs to be visible to xen/vmap.h which gets transitively included by the arch headers. The kconfig route I went with sidesteps this by allowing the common definition to not depend on previous header inclusions in its #ifndef guard, but I agree with yours and Jan's comments about it not being the best approach. > > One thing however does occur to me. ARM and RISC-V have systems with > MPU protections rather than MMU, with Xen already starting to support > this on ARM. > > Therefore we might want to reconsider the name pte_attr_t to be > something slightly less pagetable specific. Then again, I'm not sure > how much overlap there is between the map* functions and MPUs, where > mapping is of course not possible. > Without too much familiarity with ARM/RISC-V's MPU situation, at first glance it seems to me that it would warrant a separate type, even if it happened to coincide with the flag type used by the MMU on those arches. > > Finally, this wants to be at least 2 patches. One introducing > pte_attr_t, and one changing PPC to be unsigned long. > Gotcha -- not a problem. > ~Andrew Thanks, Shawn
© 2016 - 2025 Red Hat, Inc.