Re: [Xen-devel] [PATCH 2/4] x86/page: Remove bifurcated PAGE_HYPERVISOR constant
On 15.01.2020 13:53, Andrew Cooper wrote: > On 14/01/2020 16:25, Jan Beulich wrote: >>> --- a/xen/include/asm-x86/x86_64/page.h >>> +++ b/xen/include/asm-x86/x86_64/page.h >>> @@ -172,18 +172,11 @@ static inline intpte_t put_pte_flags(unsigned int x) >>> #define PAGE_HYPERVISOR_RX (__PAGE_HYPERVISOR_RX | _PAGE_GLOBAL) >>> #define PAGE_HYPERVISOR_RWX (__PAGE_HYPERVISOR | _PAGE_GLOBAL) >>> >>> -#ifdef __ASSEMBLY__ >>> -/* Dependency on NX being available can't be expressed. */ >>> -# define PAGE_HYPERVISOR PAGE_HYPERVISOR_RWX >>> -# define PAGE_HYPERVISOR_UCMINUS (__PAGE_HYPERVISOR_UCMINUS | _PAGE_GLOBAL) >>> -# define PAGE_HYPERVISOR_UC (__PAGE_HYPERVISOR_UC | _PAGE_GLOBAL) >>> -#else >>> # define PAGE_HYPERVISOR PAGE_HYPERVISOR_RW >>> # define PAGE_HYPERVISOR_UCMINUS (__PAGE_HYPERVISOR_UCMINUS | \ >>>_PAGE_GLOBAL | _PAGE_NX) >>> # define PAGE_HYPERVISOR_UC (__PAGE_HYPERVISOR_UC | \ >>>_PAGE_GLOBAL | _PAGE_NX) >> ... I'm afraid the assembler error resulting from someone actually >> (and mistakenly) using one of the constants making use of _PAGE_NX >> is going to be rather cryptic. Which in turn may motivate people >> to actually try to make _PAGE_NX "work" in assembly code. Therefore >> I'd like to ask that together with the changes here _PAGE_NX's >> #define be framed by #ifndef __ASSEMBLY__, such that any >> diagnostic, if it mentions a symbol name, would name the actual >> problem, rather than a derived one. > > I can do this, but it doesn't make the error any less cryptic. > > With _PAGE_NX hidden: > > head.S: Assembler messages: > head.S:677: Error: invalid operands (*ABS* and *UND* sections) for `|' This is something that could be improved in the future in gas (by simply naming the symbol found to be *UND*). > With it visible: > > head.S: Assembler messages: > head.S:677: Error: invalid character '?' in operand 1 This, otoh, can't sensibly be expected to see an improvement. (Well, perhaps the full line could be quoted, but that's true for about every parsing error.) Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 2/4] x86/page: Remove bifurcated PAGE_HYPERVISOR constant
On 14/01/2020 16:25, Jan Beulich wrote: >> --- a/xen/include/asm-x86/x86_64/page.h >> +++ b/xen/include/asm-x86/x86_64/page.h >> @@ -172,18 +172,11 @@ static inline intpte_t put_pte_flags(unsigned int x) >> #define PAGE_HYPERVISOR_RX (__PAGE_HYPERVISOR_RX | _PAGE_GLOBAL) >> #define PAGE_HYPERVISOR_RWX (__PAGE_HYPERVISOR | _PAGE_GLOBAL) >> >> -#ifdef __ASSEMBLY__ >> -/* Dependency on NX being available can't be expressed. */ >> -# define PAGE_HYPERVISOR PAGE_HYPERVISOR_RWX >> -# define PAGE_HYPERVISOR_UCMINUS (__PAGE_HYPERVISOR_UCMINUS | _PAGE_GLOBAL) >> -# define PAGE_HYPERVISOR_UC (__PAGE_HYPERVISOR_UC | _PAGE_GLOBAL) >> -#else >> # define PAGE_HYPERVISOR PAGE_HYPERVISOR_RW >> # define PAGE_HYPERVISOR_UCMINUS (__PAGE_HYPERVISOR_UCMINUS | \ >>_PAGE_GLOBAL | _PAGE_NX) >> # define PAGE_HYPERVISOR_UC (__PAGE_HYPERVISOR_UC | \ >>_PAGE_GLOBAL | _PAGE_NX) > ... I'm afraid the assembler error resulting from someone actually > (and mistakenly) using one of the constants making use of _PAGE_NX > is going to be rather cryptic. Which in turn may motivate people > to actually try to make _PAGE_NX "work" in assembly code. Therefore > I'd like to ask that together with the changes here _PAGE_NX's > #define be framed by #ifndef __ASSEMBLY__, such that any > diagnostic, if it mentions a symbol name, would name the actual > problem, rather than a derived one. I can do this, but it doesn't make the error any less cryptic. With _PAGE_NX hidden: head.S: Assembler messages: head.S:677: Error: invalid operands (*ABS* and *UND* sections) for `|' With it visible: head.S: Assembler messages: head.S:677: Error: invalid character '?' in operand 1 I'm not aware of any way to get a useful symbol name out of the error. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 2/4] x86/page: Remove bifurcated PAGE_HYPERVISOR constant
On 13.01.2020 18:50, Andrew Cooper wrote: > Despite being vaguely aware, the difference between PAGE_HYPERVISOR in ASM and > C code has nevertheless caused several bugs I should have known better about, > and contributed to review confusion. > > There are exactly 4 uses of these constants in asm code (and one is shortly > going to disappear). > > Instead of creating the constants which behave differently between ASM and C > code, expose all the constants and use non-ambiguous non-NX ones in ASM. I'm okay with this in principle, but ... > --- a/xen/include/asm-x86/x86_64/page.h > +++ b/xen/include/asm-x86/x86_64/page.h > @@ -172,18 +172,11 @@ static inline intpte_t put_pte_flags(unsigned int x) > #define PAGE_HYPERVISOR_RX (__PAGE_HYPERVISOR_RX | _PAGE_GLOBAL) > #define PAGE_HYPERVISOR_RWX (__PAGE_HYPERVISOR | _PAGE_GLOBAL) > > -#ifdef __ASSEMBLY__ > -/* Dependency on NX being available can't be expressed. */ > -# define PAGE_HYPERVISOR PAGE_HYPERVISOR_RWX > -# define PAGE_HYPERVISOR_UCMINUS (__PAGE_HYPERVISOR_UCMINUS | _PAGE_GLOBAL) > -# define PAGE_HYPERVISOR_UC (__PAGE_HYPERVISOR_UC | _PAGE_GLOBAL) > -#else > # define PAGE_HYPERVISOR PAGE_HYPERVISOR_RW > # define PAGE_HYPERVISOR_UCMINUS (__PAGE_HYPERVISOR_UCMINUS | \ >_PAGE_GLOBAL | _PAGE_NX) > # define PAGE_HYPERVISOR_UC (__PAGE_HYPERVISOR_UC | \ >_PAGE_GLOBAL | _PAGE_NX) ... I'm afraid the assembler error resulting from someone actually (and mistakenly) using one of the constants making use of _PAGE_NX is going to be rather cryptic. Which in turn may motivate people to actually try to make _PAGE_NX "work" in assembly code. Therefore I'd like to ask that together with the changes here _PAGE_NX's #define be framed by #ifndef __ASSEMBLY__, such that any diagnostic, if it mentions a symbol name, would name the actual problem, rather than a derived one. Furthermore from a style perspective the blanks between # and "define" will also want to go away now. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 2/4] x86/page: Remove bifurcated PAGE_HYPERVISOR constant
Despite being vaguely aware, the difference between PAGE_HYPERVISOR in ASM and C code has nevertheless caused several bugs I should have known better about, and contributed to review confusion. There are exactly 4 uses of these constants in asm code (and one is shortly going to disappear). Instead of creating the constants which behave differently between ASM and C code, expose all the constants and use non-ambiguous non-NX ones in ASM. Signed-off-by: Andrew Cooper --- CC: Jan Beulich CC: Wei Liu CC: Roger Pau Monné --- xen/arch/x86/boot/head.S | 2 +- xen/arch/x86/boot/x86_64.S| 6 +++--- xen/include/asm-x86/x86_64/page.h | 7 --- 3 files changed, 4 insertions(+), 11 deletions(-) diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S index aaf0e119db..c5acbf56ae 100644 --- a/xen/arch/x86/boot/head.S +++ b/xen/arch/x86/boot/head.S @@ -674,7 +674,7 @@ trampoline_setup: * the transition into long mode), using 2M superpages. */ lea sym_esi(start),%ebx -lea (1<= 0xa0 && pfn < 0xc0 -.quad (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR_UCMINUS | MAP_SMALL_PAGES +.quad (pfn << PAGE_SHIFT) | __PAGE_HYPERVISOR_UCMINUS | _PAGE_GLOBAL | MAP_SMALL_PAGES .else -.quad (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR | MAP_SMALL_PAGES +.quad (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR_RWX | MAP_SMALL_PAGES .endif pfn = pfn + 1 .endr @@ -89,7 +89,7 @@ GLOBAL(l2_xenmap) .quad 0 idx = 1 .rept 7 -.quad sym_offs(__image_base__) + (idx << L2_PAGETABLE_SHIFT) + (PAGE_HYPERVISOR | _PAGE_PSE) +.quad sym_offs(__image_base__) + (idx << L2_PAGETABLE_SHIFT) + (PAGE_HYPERVISOR_RWX | _PAGE_PSE) idx = idx + 1 .endr .fill L2_PAGETABLE_ENTRIES - 8, 8, 0 diff --git a/xen/include/asm-x86/x86_64/page.h b/xen/include/asm-x86/x86_64/page.h index 4fe0205553..1a4af85469 100644 --- a/xen/include/asm-x86/x86_64/page.h +++ b/xen/include/asm-x86/x86_64/page.h @@ -172,18 +172,11 @@ static inline intpte_t put_pte_flags(unsigned int x) #define PAGE_HYPERVISOR_RX (__PAGE_HYPERVISOR_RX | _PAGE_GLOBAL) #define PAGE_HYPERVISOR_RWX (__PAGE_HYPERVISOR | _PAGE_GLOBAL) -#ifdef __ASSEMBLY__ -/* Dependency on NX being available can't be expressed. */ -# define PAGE_HYPERVISOR PAGE_HYPERVISOR_RWX -# define PAGE_HYPERVISOR_UCMINUS (__PAGE_HYPERVISOR_UCMINUS | _PAGE_GLOBAL) -# define PAGE_HYPERVISOR_UC (__PAGE_HYPERVISOR_UC | _PAGE_GLOBAL) -#else # define PAGE_HYPERVISOR PAGE_HYPERVISOR_RW # define PAGE_HYPERVISOR_UCMINUS (__PAGE_HYPERVISOR_UCMINUS | \ _PAGE_GLOBAL | _PAGE_NX) # define PAGE_HYPERVISOR_UC (__PAGE_HYPERVISOR_UC | \ _PAGE_GLOBAL | _PAGE_NX) -#endif #endif /* __X86_64_PAGE_H__ */ -- 2.11.0 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
