| Message ID | 2b7f3e29fc1790978e2f615ee634f3a84bc340c9.1738789214.git.sanastasio@raptorengineering.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] xen/mm: Introduce per-arch pte_attr_t type for PTE flags | expand |
On 05.02.2025 22:02, 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 > is not well-suited for 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> > --- > Changes in v2: > - Drop Kconfig option and use `#define pte_attr_t pte_attr_t` for arches to > opt-in to defining the type. > - Move default pte_attr_definition to xen/types.h I'm unconvinced of types.h being an appropriate place for something mm- related. mm-types.h maybe? > - Update commit message to reflect that this change isn't strictly > necessary for arches w/ >32bit pte flags I can't seem to be able to associate this with anything in the commit message. The comment here to me reads as if this was optional (but then for arches with <=32-bit PTE flags), yet in the description I can't spot anything to the same effect. Recall that it was said before that on x86 we also have flags extending beyond bit 31, just that we pass them around in a compacted manner (which, as Andrew has been suggesting, may be undue extra overhead). Jan
On 06.02.2025 13:29, Jan Beulich wrote: > On 05.02.2025 22:02, 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 >> is not well-suited for 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> >> --- >> Changes in v2: >> - Drop Kconfig option and use `#define pte_attr_t pte_attr_t` for arches to >> opt-in to defining the type. >> - Move default pte_attr_definition to xen/types.h > > I'm unconvinced of types.h being an appropriate place for something mm- > related. mm-types.h maybe? To add to this (in an attempt to keep you from introducing a header of this name, just to then include it from types.h): I don't think this type wants exposing to all CUs. Ones entirely unrelated to mm (take e.g. everything that's under lib/) shouldn't get to see it. Jan
Hi Jan, On 2/6/25 6:29 AM, Jan Beulich wrote: > On 05.02.2025 22:02, 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 >> is not well-suited for 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> >> --- >> Changes in v2: >> - Drop Kconfig option and use `#define pte_attr_t pte_attr_t` for arches to >> opt-in to defining the type. >> - Move default pte_attr_definition to xen/types.h > > I'm unconvinced of types.h being an appropriate place for something mm- > related. mm-types.h maybe? > This sounds reasonable (and re your follow-up -- for now the file should only need to be included in xen/vmap.h, xen/mm.h and so on. Definitely no need to include it in types.h). >> - Update commit message to reflect that this change isn't strictly >> necessary for arches w/ >32bit pte flags > > I can't seem to be able to associate this with anything in the commit > message. The comment here to me reads as if this was optional (but then > for arches with <=32-bit PTE flags), yet in the description I can't spot > anything to the same effect. Recall that it was said before that on x86 > we also have flags extending beyond bit 31, just that we pass them > around in a compacted manner (which, as Andrew has been suggesting, may > be undue extra overhead). > Admittedly the change was subtle, but I changed the wording in the commit message as follows: - This assumption does not work on PPC/radix where some flags go past 32-bits, so [...] + This assumption is not well-suited for PPC/radix where some flags go past 32-bits, so [...] The softening of "does not work" to "is not well-suited" was meant to address your earlier comment and clarify that the change is not strictly necessary. Though as you and Andrew pointed out x86_64 is able to make do with the 32 bits, I would still argue that the hardcoded `unsigned int` flags type is still not well-suited to that application. > Jan Thanks, Shawn
On 14.02.2025 00:05, Shawn Anastasio wrote: > On 2/6/25 6:29 AM, Jan Beulich wrote: >> On 05.02.2025 22:02, 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 >>> is not well-suited for 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> >>> --- >>> Changes in v2: >>> - Drop Kconfig option and use `#define pte_attr_t pte_attr_t` for arches to >>> opt-in to defining the type. >>> - Move default pte_attr_definition to xen/types.h >>>[...] >>> - Update commit message to reflect that this change isn't strictly >>> necessary for arches w/ >32bit pte flags >> >> I can't seem to be able to associate this with anything in the commit >> message. The comment here to me reads as if this was optional (but then >> for arches with <=32-bit PTE flags), yet in the description I can't spot >> anything to the same effect. Recall that it was said before that on x86 >> we also have flags extending beyond bit 31, just that we pass them >> around in a compacted manner (which, as Andrew has been suggesting, may >> be undue extra overhead). >> > > Admittedly the change was subtle, but I changed the wording in the > commit message as follows: > > - This assumption does not work on PPC/radix where some flags go past > 32-bits, so [...] > + This assumption is not well-suited for PPC/radix where some flags go > past 32-bits, so [...] > > > The softening of "does not work" to "is not well-suited" was meant to > address your earlier comment and clarify that the change is not strictly > necessary. Though as you and Andrew pointed out x86_64 is able to make > do with the 32 bits, I would still argue that the hardcoded `unsigned > int` flags type is still not well-suited to that application. Oh, okay, fair enough then. Jan
diff --git a/xen/arch/ppc/include/asm/types.h b/xen/arch/ppc/include/asm/types.h index ffaf378a4d..9d80e92e44 100644 --- a/xen/arch/ppc/include/asm/types.h +++ b/xen/arch/ppc/include/asm/types.h @@ -8,4 +8,7 @@ typedef unsigned long vaddr_t; #define INVALID_PADDR (~0UL) #define PRIpaddr "016lx" +typedef unsigned long pte_attr_t; +#define pte_attr_t 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/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..708705ce72 100644 --- a/xen/include/xen/mm.h +++ b/xen/include/xen/mm.h @@ -113,9 +113,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/types.h b/xen/include/xen/types.h index 543bfb2159..8f593c6f74 100644 --- a/xen/include/xen/types.h +++ b/xen/include/xen/types.h @@ -18,6 +18,10 @@ typedef signed long ssize_t; typedef __PTRDIFF_TYPE__ ptrdiff_t; +#ifndef pte_attr_t +typedef unsigned int pte_attr_t; +#endif + /* * Users of this macro are expected to pass a positive value. * 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
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 is not well-suited for 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> --- Changes in v2: - Drop Kconfig option and use `#define pte_attr_t pte_attr_t` for arches to opt-in to defining the type. - Move default pte_attr_definition to xen/types.h - Update commit message to reflect that this change isn't strictly necessary for arches w/ >32bit pte flags xen/arch/ppc/include/asm/types.h | 3 +++ xen/arch/ppc/mm-radix.c | 2 +- xen/common/efi/boot.c | 4 ++-- xen/common/vmap.c | 2 +- xen/include/xen/mm.h | 4 ++-- xen/include/xen/types.h | 4 ++++ xen/include/xen/vmap.h | 3 ++- 7 files changed, 15 insertions(+), 7 deletions(-) -- 2.30.2