| Message ID | 1471890809-4383-55-git-send-email-mikko.rapeli@iki.fi (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Aug 22, 2016 at 08:33:11PM +0200, Mikko Rapeli wrote: > xen/interface/xen.h is not exported from kernel headers so remove the > dependency and provide needed defines for domid_t and xen_pfn_t if they > are not already defined by some other e.g. Xen specific headers. I'm confused. How did we end up with a 64-bit PFN number on ARM? It's insane - especially as the kernel uses "unsigned long" almost everywhere for PFNs - we can't have physical addresses more than 44 bits (32 bit pfn + 4k page size). > Suggested by Andrew Cooper <andrew.cooper3@citrix.com> on lkml message > <5569F9C9.8000607@citrix.com>. > > The ifdef for ARM is ugly but did not find better solutions for it. #ifdef __arm__ maybe? Even if not, the unsightly parens are not necessary.
On Tue, 23 Aug 2016, Russell King - ARM Linux wrote: > On Mon, Aug 22, 2016 at 08:33:11PM +0200, Mikko Rapeli wrote: > > xen/interface/xen.h is not exported from kernel headers so remove the > > dependency and provide needed defines for domid_t and xen_pfn_t if they > > are not already defined by some other e.g. Xen specific headers. > > I'm confused. How did we end up with a 64-bit PFN number on ARM? It's > insane - especially as the kernel uses "unsigned long" almost everywhere > for PFNs - we can't have physical addresses more than 44 bits (32 bit > pfn + 4k page size). That's because xen_pfn_t is the type used to store pfns in structures passed to Xen via hypercalls. The Xen hypercall ABI is shared between ARM and ARM64. On x86_32 and x86_64 we have different sizes for pfn types in the hypercall ABI and it caused quite a bit of trouble in the past as it is possible to run 32bit domains on a 64bit hypervisor. Having a common type for pfns on ARM and ARM64 helped a lot in that respect. > > Suggested by Andrew Cooper <andrew.cooper3@citrix.com> on lkml message > > <5569F9C9.8000607@citrix.com>. > > > > The ifdef for ARM is ugly but did not find better solutions for it. > > #ifdef __arm__ > > maybe? Even if not, the unsightly parens are not necessary. Yes, I think it should be: #if defined(__arm__) || defined(__aarch64__) > diff --git a/arch/arm/include/asm/xen/interface.h b/arch/arm/include/asm/xen/interface.h > index 75d5968..6898ee1 100644 > --- a/arch/arm/include/asm/xen/interface.h > +++ b/arch/arm/include/asm/xen/interface.h > @@ -38,7 +38,7 @@ > * fine since it simply wouldn't be able to create any sure pfns in > * the first place. > */ > -typedef uint64_t xen_pfn_t; > +typedef __u64 xen_pfn_t; > #define PRI_xen_pfn "llx" > typedef uint64_t xen_ulong_t; > #define PRI_xen_ulong "llx" Why this change?
On Tue, Aug 23, 2016 at 11:13:52AM -0700, Stefano Stabellini wrote: > On Tue, 23 Aug 2016, Russell King - ARM Linux wrote: > > On Mon, Aug 22, 2016 at 08:33:11PM +0200, Mikko Rapeli wrote: > > > xen/interface/xen.h is not exported from kernel headers so remove the > > > dependency and provide needed defines for domid_t and xen_pfn_t if they > > > are not already defined by some other e.g. Xen specific headers. > > > > I'm confused. How did we end up with a 64-bit PFN number on ARM? It's > > insane - especially as the kernel uses "unsigned long" almost everywhere > > for PFNs - we can't have physical addresses more than 44 bits (32 bit > > pfn + 4k page size). > > That's because xen_pfn_t is the type used to store pfns in structures > passed to Xen via hypercalls. The Xen hypercall ABI is shared between > ARM and ARM64. On x86_32 and x86_64 we have different sizes for pfn > types in the hypercall ABI and it caused quite a bit of trouble in the > past as it is possible to run 32bit domains on a 64bit hypervisor. > Having a common type for pfns on ARM and ARM64 helped a lot in that > respect. > > > > > Suggested by Andrew Cooper <andrew.cooper3@citrix.com> on lkml message > > > <5569F9C9.8000607@citrix.com>. > > > > > > The ifdef for ARM is ugly but did not find better solutions for it. > > > > #ifdef __arm__ > > > > maybe? Even if not, the unsightly parens are not necessary. > > Yes, I think it should be: > > #if defined(__arm__) || defined(__aarch64__) Thanks, I will send a new version with this change. > > diff --git a/arch/arm/include/asm/xen/interface.h b/arch/arm/include/asm/xen/interface.h > > index 75d5968..6898ee1 100644 > > --- a/arch/arm/include/asm/xen/interface.h > > +++ b/arch/arm/include/asm/xen/interface.h > > @@ -38,7 +38,7 @@ > > * fine since it simply wouldn't be able to create any sure pfns in > > * the first place. > > */ > > -typedef uint64_t xen_pfn_t; > > +typedef __u64 xen_pfn_t; > > #define PRI_xen_pfn "llx" > > typedef uint64_t xen_ulong_t; > > #define PRI_xen_ulong "llx" > > Why this change? I will double check but I think the other fix exposed then the common "<stdint.h> definitions not available in userspace <linux/types.h>" and this was needed to please the compiler. If so, I'll add this to commit message. If you would prefer to include libc's <stdint.h> in userspace, well, join the club. drm and fuse maintainers want the same but my patches with those fixes got rejected in the past. Example: https://lkml.org/lkml/2015/6/1/160 -Mikko
On Tue, 23 Aug 2016, Mikko Rapeli wrote: > On Tue, Aug 23, 2016 at 11:13:52AM -0700, Stefano Stabellini wrote: > > On Tue, 23 Aug 2016, Russell King - ARM Linux wrote: > > > On Mon, Aug 22, 2016 at 08:33:11PM +0200, Mikko Rapeli wrote: > > > > xen/interface/xen.h is not exported from kernel headers so remove the > > > > dependency and provide needed defines for domid_t and xen_pfn_t if they > > > > are not already defined by some other e.g. Xen specific headers. > > > > > > I'm confused. How did we end up with a 64-bit PFN number on ARM? It's > > > insane - especially as the kernel uses "unsigned long" almost everywhere > > > for PFNs - we can't have physical addresses more than 44 bits (32 bit > > > pfn + 4k page size). > > > > That's because xen_pfn_t is the type used to store pfns in structures > > passed to Xen via hypercalls. The Xen hypercall ABI is shared between > > ARM and ARM64. On x86_32 and x86_64 we have different sizes for pfn > > types in the hypercall ABI and it caused quite a bit of trouble in the > > past as it is possible to run 32bit domains on a 64bit hypervisor. > > Having a common type for pfns on ARM and ARM64 helped a lot in that > > respect. > > > > > > > > Suggested by Andrew Cooper <andrew.cooper3@citrix.com> on lkml message > > > > <5569F9C9.8000607@citrix.com>. > > > > > > > > The ifdef for ARM is ugly but did not find better solutions for it. > > > > > > #ifdef __arm__ > > > > > > maybe? Even if not, the unsightly parens are not necessary. > > > > Yes, I think it should be: > > > > #if defined(__arm__) || defined(__aarch64__) > > Thanks, I will send a new version with this change. > > > > diff --git a/arch/arm/include/asm/xen/interface.h b/arch/arm/include/asm/xen/interface.h > > > index 75d5968..6898ee1 100644 > > > --- a/arch/arm/include/asm/xen/interface.h > > > +++ b/arch/arm/include/asm/xen/interface.h > > > @@ -38,7 +38,7 @@ > > > * fine since it simply wouldn't be able to create any sure pfns in > > > * the first place. > > > */ > > > -typedef uint64_t xen_pfn_t; > > > +typedef __u64 xen_pfn_t; > > > #define PRI_xen_pfn "llx" > > > typedef uint64_t xen_ulong_t; > > > #define PRI_xen_ulong "llx" > > > > Why this change? > > I will double check but I think the other fix exposed then the common > "<stdint.h> definitions not available in userspace <linux/types.h>" and this > was needed to please the compiler. If so, I'll add this to commit message. That would be strange, because I don't think arch/arm/include/asm/xen/interface.h is exposed to userspace. If it was, we would need to replace the other definitions there too.
diff --git a/arch/arm/include/asm/xen/interface.h b/arch/arm/include/asm/xen/interface.h index 75d5968..6898ee1 100644 --- a/arch/arm/include/asm/xen/interface.h +++ b/arch/arm/include/asm/xen/interface.h @@ -38,7 +38,7 @@ * fine since it simply wouldn't be able to create any sure pfns in * the first place. */ -typedef uint64_t xen_pfn_t; +typedef __u64 xen_pfn_t; #define PRI_xen_pfn "llx" typedef uint64_t xen_ulong_t; #define PRI_xen_ulong "llx" diff --git a/include/uapi/xen/privcmd.h b/include/uapi/xen/privcmd.h index 7ddeeda..16c11f9 100644 --- a/include/uapi/xen/privcmd.h +++ b/include/uapi/xen/privcmd.h @@ -35,7 +35,17 @@ #include <linux/types.h> #include <linux/compiler.h> -#include <xen/interface/xen.h> + +/* Defined by include/xen/interface/xen.h, but it is not part of Linux uapi */ +#ifndef __XEN_PUBLIC_XEN_H__ +typedef __u16 domid_t; + +#if (defined __ARMEL__ || defined __ARMEB__) +typedef __u64 xen_pfn_t; +#else +typedef unsigned long xen_pfn_t; +#endif /* (defined __ARMEL__ || defined __ARMEB__) */ +#endif /* __XEN_PUBLIC_XEN_H__ */ struct privcmd_hypercall { __u64 op;
xen/interface/xen.h is not exported from kernel headers so remove the dependency and provide needed defines for domid_t and xen_pfn_t if they are not already defined by some other e.g. Xen specific headers. Suggested by Andrew Cooper <andrew.cooper3@citrix.com> on lkml message <5569F9C9.8000607@citrix.com>. The ifdef for ARM is ugly but did not find better solutions for it. Fixes userspace compilation error: xen/privcmd.h:38:31: fatal error: xen/interface/xen.h: No such file or directory Signed-off-by: Mikko Rapeli <mikko.rapeli@iki.fi> Cc: David Vrabel <david.vrabel@citrix.com> --- arch/arm/include/asm/xen/interface.h | 2 +- include/uapi/xen/privcmd.h | 12 +++++++++++- 2 files changed, 12 insertions(+), 2 deletions(-)