| Message ID | 20241129091237.3409304-2-luca.fancellu@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Prerequisite patches for R82 upstreaming | expand |
On 29.11.2024 10:12, Luca Fancellu wrote: > --- a/xen/include/xen/xvmalloc.h > +++ b/xen/include/xen/xvmalloc.h > @@ -40,20 +40,46 @@ > ((typeof(ptr))_xvrealloc(ptr, offsetof(typeof(*(ptr)), field[nr]), \ > __alignof__(typeof(*(ptr))))) > > +#ifdef CONFIG_HAS_VMAP > + > /* Free any of the above. */ > void xvfree(void *va); > > +/* Underlying functions */ > +void *_xvmalloc(size_t size, unsigned int align); > +void *_xvzalloc(size_t size, unsigned int align); > +void *_xvrealloc(void *va, size_t size, unsigned int align); > + > +#else /* !CONFIG_HAS_VMAP */ > + > +static inline void xvfree(void *va) > +{ > + xfree(va); > +} > + > +static inline void *_xvmalloc(size_t size, unsigned int align) > +{ > + return _xmalloc(size, align); > +} > + > +static inline void *_xvzalloc(size_t size, unsigned int align) > +{ > + return _xzalloc(size, align); > +} > + > +static inline void *_xvrealloc(void *va, size_t size, unsigned int align) > +{ > + return _xrealloc(va, size, align); > +} Just to double check: Was it at least considered to use simple #define-s to effect the aliasing? Wrapper functions like the above ones have the downside of needing touching (easy to miss) when the wrapped function types change in whichever minor way. (And yes, I do understand that we generally aim at using inline functions in preference to macros.) Jan
Hi Jan, > On 29 Nov 2024, at 11:06, Jan Beulich <jbeulich@suse.com> wrote: > > On 29.11.2024 10:12, Luca Fancellu wrote: >> --- a/xen/include/xen/xvmalloc.h >> +++ b/xen/include/xen/xvmalloc.h >> @@ -40,20 +40,46 @@ >> ((typeof(ptr))_xvrealloc(ptr, offsetof(typeof(*(ptr)), field[nr]), \ >> __alignof__(typeof(*(ptr))))) >> >> +#ifdef CONFIG_HAS_VMAP >> + >> /* Free any of the above. */ >> void xvfree(void *va); >> >> +/* Underlying functions */ >> +void *_xvmalloc(size_t size, unsigned int align); >> +void *_xvzalloc(size_t size, unsigned int align); >> +void *_xvrealloc(void *va, size_t size, unsigned int align); >> + >> +#else /* !CONFIG_HAS_VMAP */ >> + >> +static inline void xvfree(void *va) >> +{ >> + xfree(va); >> +} >> + >> +static inline void *_xvmalloc(size_t size, unsigned int align) >> +{ >> + return _xmalloc(size, align); >> +} >> + >> +static inline void *_xvzalloc(size_t size, unsigned int align) >> +{ >> + return _xzalloc(size, align); >> +} >> + >> +static inline void *_xvrealloc(void *va, size_t size, unsigned int align) >> +{ >> + return _xrealloc(va, size, align); >> +} > > Just to double check: Was it at least considered to use simple #define-s > to effect the aliasing? Wrapper functions like the above ones have the > downside of needing touching (easy to miss) when the wrapped function > types change in whichever minor way. (And yes, I do understand that we > generally aim at using inline functions in preference to macros.) Yes, I think I tried and I didn’t have issues using #define-s, I asked here https://patchwork.kernel.org/project/xen-devel/patch/20241115105036.218418-2-luca.fancellu@arm.com/#26123448 about a preferred approach, but I didn’t have any reply, so I went for what I believed was preferred as you said, static inline-s instead of macros. Cheers, Luca
On 29.11.2024 12:14, Luca Fancellu wrote: > Hi Jan, > >> On 29 Nov 2024, at 11:06, Jan Beulich <jbeulich@suse.com> wrote: >> >> On 29.11.2024 10:12, Luca Fancellu wrote: >>> --- a/xen/include/xen/xvmalloc.h >>> +++ b/xen/include/xen/xvmalloc.h >>> @@ -40,20 +40,46 @@ >>> ((typeof(ptr))_xvrealloc(ptr, offsetof(typeof(*(ptr)), field[nr]), \ >>> __alignof__(typeof(*(ptr))))) >>> >>> +#ifdef CONFIG_HAS_VMAP >>> + >>> /* Free any of the above. */ >>> void xvfree(void *va); >>> >>> +/* Underlying functions */ >>> +void *_xvmalloc(size_t size, unsigned int align); >>> +void *_xvzalloc(size_t size, unsigned int align); >>> +void *_xvrealloc(void *va, size_t size, unsigned int align); >>> + >>> +#else /* !CONFIG_HAS_VMAP */ >>> + >>> +static inline void xvfree(void *va) >>> +{ >>> + xfree(va); >>> +} >>> + >>> +static inline void *_xvmalloc(size_t size, unsigned int align) >>> +{ >>> + return _xmalloc(size, align); >>> +} >>> + >>> +static inline void *_xvzalloc(size_t size, unsigned int align) >>> +{ >>> + return _xzalloc(size, align); >>> +} >>> + >>> +static inline void *_xvrealloc(void *va, size_t size, unsigned int align) >>> +{ >>> + return _xrealloc(va, size, align); >>> +} >> >> Just to double check: Was it at least considered to use simple #define-s >> to effect the aliasing? Wrapper functions like the above ones have the >> downside of needing touching (easy to miss) when the wrapped function >> types change in whichever minor way. (And yes, I do understand that we >> generally aim at using inline functions in preference to macros.) > > Yes, I think I tried and I didn’t have issues using #define-s, I asked here > https://patchwork.kernel.org/project/xen-devel/patch/20241115105036.218418-2-luca.fancellu@arm.com/#26123448 > about a preferred approach, but I didn’t have any reply, so I went for what > I believed was preferred as you said, static inline-s instead of macros. As Andrew's idea didn't work out, personally I think the simple #define approach you suggested would be preferable in this case. There is in particular no type-safety concern here, as the wrapped functions will all have the intended type checking applied. Jan
>>> >>> Just to double check: Was it at least considered to use simple #define-s >>> to effect the aliasing? Wrapper functions like the above ones have the >>> downside of needing touching (easy to miss) when the wrapped function >>> types change in whichever minor way. (And yes, I do understand that we >>> generally aim at using inline functions in preference to macros.) >> >> Yes, I think I tried and I didn’t have issues using #define-s, I asked here >> https://patchwork.kernel.org/project/xen-devel/patch/20241115105036.218418-2-luca.fancellu@arm.com/#26123448 >> about a preferred approach, but I didn’t have any reply, so I went for what >> I believed was preferred as you said, static inline-s instead of macros. > > As Andrew's idea didn't work out, personally I think the simple #define > approach you suggested would be preferable in this case. There is in > particular no type-safety concern here, as the wrapped functions will > all have the intended type checking applied. ok, I’ll re-spin this one with #defines. > > Jan
diff --git a/xen/include/xen/vmap.h b/xen/include/xen/vmap.h index c1dd7ac22f30..26c831757a11 100644 --- a/xen/include/xen/vmap.h +++ b/xen/include/xen/vmap.h @@ -5,7 +5,7 @@ * purpose area (VMAP_DEFAULT) and a livepatch-specific area (VMAP_XEN). The * latter is used when loading livepatches and the former for everything else. */ -#if !defined(__XEN_VMAP_H__) && defined(VMAP_VIRT_START) +#ifndef __XEN_VMAP_H__ #define __XEN_VMAP_H__ #include <xen/mm-frame.h> diff --git a/xen/include/xen/xvmalloc.h b/xen/include/xen/xvmalloc.h index 440d85a284bb..e97a30f61e96 100644 --- a/xen/include/xen/xvmalloc.h +++ b/xen/include/xen/xvmalloc.h @@ -40,20 +40,46 @@ ((typeof(ptr))_xvrealloc(ptr, offsetof(typeof(*(ptr)), field[nr]), \ __alignof__(typeof(*(ptr))))) +#ifdef CONFIG_HAS_VMAP + /* Free any of the above. */ void xvfree(void *va); +/* Underlying functions */ +void *_xvmalloc(size_t size, unsigned int align); +void *_xvzalloc(size_t size, unsigned int align); +void *_xvrealloc(void *va, size_t size, unsigned int align); + +#else /* !CONFIG_HAS_VMAP */ + +static inline void xvfree(void *va) +{ + xfree(va); +} + +static inline void *_xvmalloc(size_t size, unsigned int align) +{ + return _xmalloc(size, align); +} + +static inline void *_xvzalloc(size_t size, unsigned int align) +{ + return _xzalloc(size, align); +} + +static inline void *_xvrealloc(void *va, size_t size, unsigned int align) +{ + return _xrealloc(va, size, align); +} + +#endif /* CONFIG_HAS_VMAP */ + /* Free an allocation, and zero the pointer to it. */ #define XVFREE(p) do { \ xvfree(p); \ (p) = NULL; \ } while ( false ) -/* Underlying functions */ -void *_xvmalloc(size_t size, unsigned int align); -void *_xvzalloc(size_t size, unsigned int align); -void *_xvrealloc(void *va, size_t size, unsigned int align); - static inline void *_xvmalloc_array( size_t size, unsigned int align, unsigned long num) {
When HAS_VMAP is disabled, the xv{malloc,zalloc,...} functions should fall back to the simple x{malloc,zalloc,...} variant, implement that because MPU systems won't have virtual memory. Additionally remove VMAP_VIRT_START from vmap.h guards since MPU systems won't have it defined. Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> --- Changes from v2: - Don't protect declarations. Changes from v1: - put back static inline iounmap - changed commit message - hide not used declaration for system with !HAS_VMAP - correct function declared in xvmalloc.h to be static inline - prefer '#ifdef' instead of '#if defined' where possible --- --- xen/include/xen/vmap.h | 2 +- xen/include/xen/xvmalloc.h | 36 +++++++++++++++++++++++++++++++----- 2 files changed, 32 insertions(+), 6 deletions(-)