| Message ID | fc8e042e-fef8-ac38-34d8-16b13e4b0135@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: some assembler macro rework | expand |
On Wed, Jul 15, 2020 at 12:48:14PM +0200, Jan Beulich wrote: > Introduce proper assembler macros instead, enabled only when the > assembler itself doesn't support the insns. To avoid duplicating the > macros for assembly and C files, have them processed into asm-macros.h. > This in turn requires adding a multiple inclusion guard when generating > that header. > > No change to generated code. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/arch/x86/Makefile > +++ b/xen/arch/x86/Makefile > @@ -235,7 +235,10 @@ $(BASEDIR)/include/asm-x86/asm-macros.h: > echo '#if 0' >$@.new > echo '.if 0' >>$@.new > echo '#endif' >>$@.new > + echo '#ifndef __ASM_MACROS_H__' >>$@.new > + echo '#define __ASM_MACROS_H__' >>$@.new > echo 'asm ( ".include \"$@\"" );' >>$@.new > + echo '#endif /* __ASM_MACROS_H__ */' >>$@.new > echo '#if 0' >>$@.new > echo '.endif' >>$@.new > cat $< >>$@.new > --- a/xen/arch/x86/arch.mk > +++ b/xen/arch/x86/arch.mk > @@ -20,6 +20,7 @@ $(call as-option-add,CFLAGS,CC,"rdrand % > $(call as-option-add,CFLAGS,CC,"rdfsbase %rax",-DHAVE_AS_FSGSBASE) > $(call as-option-add,CFLAGS,CC,"xsaveopt (%rax)",-DHAVE_AS_XSAVEOPT) > $(call as-option-add,CFLAGS,CC,"rdseed %eax",-DHAVE_AS_RDSEED) > +$(call as-option-add,CFLAGS,CC,"clac",-DHAVE_AS_CLAC_STAC) > $(call as-option-add,CFLAGS,CC,"clwb (%rax)",-DHAVE_AS_CLWB) > $(call as-option-add,CFLAGS,CC,".equ \"x\"$$(comma)1",-DHAVE_AS_QUOTED_SYM) > $(call as-option-add,CFLAGS,CC,"invpcid (%rax)$$(comma)%rax",-DHAVE_AS_INVPCID) > --- a/xen/arch/x86/asm-macros.c > +++ b/xen/arch/x86/asm-macros.c > @@ -1 +1,2 @@ > +#include <asm/asm-defns.h> > #include <asm/alternative-asm.h> > --- /dev/null > +++ b/xen/include/asm-x86/asm-defns.h Maybe this could be asm-insn.h or a different name? I find it confusing to have asm-defns.h and an asm_defs.h. Thanks, Roger.
On 27.07.2020 16:55, Roger Pau Monné wrote: > On Wed, Jul 15, 2020 at 12:48:14PM +0200, Jan Beulich wrote: >> --- /dev/null >> +++ b/xen/include/asm-x86/asm-defns.h > > Maybe this could be asm-insn.h or a different name? I find it > confusing to have asm-defns.h and an asm_defs.h. While indeed I anticipated a reply to this effect, I don't consider asm-insn.h or asm-macros.h suitable: We don't want to limit this header to a more narrow purpose than "all sorts of definition", I don't think. Hence I chose that name despite its similarity to the C header's one. Jan
On Mon, Jul 27, 2020 at 09:47:52PM +0200, Jan Beulich wrote: > On 27.07.2020 16:55, Roger Pau Monné wrote: > > On Wed, Jul 15, 2020 at 12:48:14PM +0200, Jan Beulich wrote: > > > --- /dev/null > > > +++ b/xen/include/asm-x86/asm-defns.h > > > > Maybe this could be asm-insn.h or a different name? I find it > > confusing to have asm-defns.h and an asm_defs.h. > > While indeed I anticipated a reply to this effect, I don't consider > asm-insn.h or asm-macros.h suitable: We don't want to limit this > header to a more narrow purpose than "all sorts of definition", I > don't think. Hence I chose that name despite its similarity to the > C header's one. I think it's confusing, but I also think the whole magic we do with asm includes is already confusing (me), so if you and Andrew agree this is the best name I'm certainly fine with it. FWIW: Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Please quote the clac/stac instructions in order to match the other usages of ALTERNATIVE. Thanks, Roger.
On 15/07/2020 11:48, Jan Beulich wrote: > --- a/xen/arch/x86/arch.mk > +++ b/xen/arch/x86/arch.mk > @@ -20,6 +20,7 @@ $(call as-option-add,CFLAGS,CC,"rdrand % > $(call as-option-add,CFLAGS,CC,"rdfsbase %rax",-DHAVE_AS_FSGSBASE) > $(call as-option-add,CFLAGS,CC,"xsaveopt (%rax)",-DHAVE_AS_XSAVEOPT) > $(call as-option-add,CFLAGS,CC,"rdseed %eax",-DHAVE_AS_RDSEED) > +$(call as-option-add,CFLAGS,CC,"clac",-DHAVE_AS_CLAC_STAC) Kconfig please, rather than extending this legacy section. That said, surely stac/clac support is old enough for us to start using unconditionally? Could we see about sorting a reasonable minimum toolchain version, before we churn all the logic to deal with obsolete toolchains? ~Andrew
On 27/07/2020 20:47, Jan Beulich wrote: > On 27.07.2020 16:55, Roger Pau Monné wrote: >> On Wed, Jul 15, 2020 at 12:48:14PM +0200, Jan Beulich wrote: >>> --- /dev/null >>> +++ b/xen/include/asm-x86/asm-defns.h >> >> Maybe this could be asm-insn.h or a different name? I find it >> confusing to have asm-defns.h and an asm_defs.h. > > While indeed I anticipated a reply to this effect, I don't consider > asm-insn.h or asm-macros.h suitable: We don't want to limit this > header to a more narrow purpose than "all sorts of definition", I > don't think. Hence I chose that name despite its similarity to the > C header's one. Roger is correct. Having asm-defns.h and asm_defs.h is too confusing, and there is already too much behind the scenes magic here. What is the anticipated end result, file wise, because that might highlight a better way forward. ~Andrew
On 28.07.2020 15:55, Andrew Cooper wrote: > On 15/07/2020 11:48, Jan Beulich wrote: >> --- a/xen/arch/x86/arch.mk >> +++ b/xen/arch/x86/arch.mk >> @@ -20,6 +20,7 @@ $(call as-option-add,CFLAGS,CC,"rdrand % >> $(call as-option-add,CFLAGS,CC,"rdfsbase %rax",-DHAVE_AS_FSGSBASE) >> $(call as-option-add,CFLAGS,CC,"xsaveopt (%rax)",-DHAVE_AS_XSAVEOPT) >> $(call as-option-add,CFLAGS,CC,"rdseed %eax",-DHAVE_AS_RDSEED) >> +$(call as-option-add,CFLAGS,CC,"clac",-DHAVE_AS_CLAC_STAC) > > Kconfig please, rather than extending this legacy section. Did you forget for a moment that we're still to discuss this use of Kconfig before we extend it to further instances? I'm pretty sure I gave an ack to one of the respective changes of yours only on the condition that we'd sort out whether this is indeed the way forward, without a preset outcome (and without reasoning like "let's do it because Linux does so"). > That said, surely stac/clac support is old enough for us to start using > unconditionally? Can't check right now, but I'm sure I wouldn't have introduced the construct if we could rely on all supported tool chains to have support for them. > Could we see about sorting a reasonable minimum toolchain version, > before we churn all the logic to deal with obsolete toolchains? Who's "we" here? I see you keep proposing this every once in a while, but I don't see who's going to do the work. The main reason why, while I agree we should bump the base line, I don't see myself do this is because I don't see any even just half way clear criteria by which to decide what the new level is supposed to be. Once again I don't think "let's follow what Linux does" is a suitable approach. Additionally I fear that with raising the tool chain base line, people may start considering to raise other minimum versions. While I'm personally quite fine building my own binutils and gcc (and maybe a few other pieces), I don't fancy having to rebuild, say, coreutils just to be able to build Xen. Maybe a topic for the next community call, which isn't too far out? Jan
On 28.07.2020 15:59, Andrew Cooper wrote: > On 27/07/2020 20:47, Jan Beulich wrote: >> On 27.07.2020 16:55, Roger Pau Monné wrote: >>> On Wed, Jul 15, 2020 at 12:48:14PM +0200, Jan Beulich wrote: >>>> --- /dev/null >>>> +++ b/xen/include/asm-x86/asm-defns.h >>> >>> Maybe this could be asm-insn.h or a different name? I find it >>> confusing to have asm-defns.h and an asm_defs.h. >> >> While indeed I anticipated a reply to this effect, I don't consider >> asm-insn.h or asm-macros.h suitable: We don't want to limit this >> header to a more narrow purpose than "all sorts of definition", I >> don't think. Hence I chose that name despite its similarity to the >> C header's one. > > Roger is correct. Having asm-defns.h and asm_defs.h is too confusing, > and there is already too much behind the scenes magic here. > > What is the anticipated end result, file wise, because that might > highlight a better way forward. For one I'm afraid I don't understand "file wise" here. The one meaning I could guess can't be it: The name of the file. And then, "the anticipated end result" is at least ambiguous too: You can surely see what the file contains by the end of this series, so again this can't be meant. I have no immediate plans beyond this series, so I can only state what I did say in reply to Roger's remark already: "all sorts of asm definitions". I'd also like to emphasize that asm-defns.h really is a companion of asm_defns.h, supposed to be include only by the latter (as I think can be seen from the patches). In this role I think its name being as similar to its "parent" as suggested makes sufficient sense. Jan
On 28.07.2020 15:55, Andrew Cooper wrote: > On 15/07/2020 11:48, Jan Beulich wrote: >> --- a/xen/arch/x86/arch.mk >> +++ b/xen/arch/x86/arch.mk >> @@ -20,6 +20,7 @@ $(call as-option-add,CFLAGS,CC,"rdrand % >> $(call as-option-add,CFLAGS,CC,"rdfsbase %rax",-DHAVE_AS_FSGSBASE) >> $(call as-option-add,CFLAGS,CC,"xsaveopt (%rax)",-DHAVE_AS_XSAVEOPT) >> $(call as-option-add,CFLAGS,CC,"rdseed %eax",-DHAVE_AS_RDSEED) >> +$(call as-option-add,CFLAGS,CC,"clac",-DHAVE_AS_CLAC_STAC) > > Kconfig please, rather than extending this legacy section. > > That said, surely stac/clac support is old enough for us to start using > unconditionally? It's available in binutils 2.24 and newer. Jan
On 28.07.2020 11:06, Roger Pau Monné wrote: > On Mon, Jul 27, 2020 at 09:47:52PM +0200, Jan Beulich wrote: >> On 27.07.2020 16:55, Roger Pau Monné wrote: >>> On Wed, Jul 15, 2020 at 12:48:14PM +0200, Jan Beulich wrote: >>>> --- /dev/null >>>> +++ b/xen/include/asm-x86/asm-defns.h >>> >>> Maybe this could be asm-insn.h or a different name? I find it >>> confusing to have asm-defns.h and an asm_defs.h. >> >> While indeed I anticipated a reply to this effect, I don't consider >> asm-insn.h or asm-macros.h suitable: We don't want to limit this >> header to a more narrow purpose than "all sorts of definition", I >> don't think. Hence I chose that name despite its similarity to the >> C header's one. > > I think it's confusing, but I also think the whole magic we do with > asm includes is already confusing (me), so if you and Andrew agree > this is the best name I'm certainly fine with it. FWIW: > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> > > Please quote the clac/stac instructions in order to match the other > usages of ALTERNATIVE. We're not consistently quoting when there's just a single word, see in particular spec_ctrl_asm.h. And thinking about it again I also don't see why we would want or need to enforce quotation when none is needed. Therefore both here and in patch 2 I'll keep (or make, when I touch a line anyway) things consistently unquoted where no quotes are needed. Please let me know if your R-b holds without the requested adjustment. Jan
On Fri, Jul 31, 2020 at 10:05:07AM +0200, Jan Beulich wrote: > On 28.07.2020 11:06, Roger Pau Monné wrote: > > On Mon, Jul 27, 2020 at 09:47:52PM +0200, Jan Beulich wrote: > >> On 27.07.2020 16:55, Roger Pau Monné wrote: > >>> On Wed, Jul 15, 2020 at 12:48:14PM +0200, Jan Beulich wrote: > >>>> --- /dev/null > >>>> +++ b/xen/include/asm-x86/asm-defns.h > >>> > >>> Maybe this could be asm-insn.h or a different name? I find it > >>> confusing to have asm-defns.h and an asm_defs.h. > >> > >> While indeed I anticipated a reply to this effect, I don't consider > >> asm-insn.h or asm-macros.h suitable: We don't want to limit this > >> header to a more narrow purpose than "all sorts of definition", I > >> don't think. Hence I chose that name despite its similarity to the > >> C header's one. > > > > I think it's confusing, but I also think the whole magic we do with > > asm includes is already confusing (me), so if you and Andrew agree > > this is the best name I'm certainly fine with it. FWIW: > > > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> > > > > Please quote the clac/stac instructions in order to match the other > > usages of ALTERNATIVE. > > We're not consistently quoting when there's just a single word, see > in particular spec_ctrl_asm.h. And thinking about it again I also > don't see why we would want or need to enforce quotation when none > is needed. Therefore both here and in patch 2 I'll keep (or make, > when I touch a line anyway) things consistently unquoted where no > quotes are needed. Please let me know if your R-b holds without the > requested adjustment. Yes, I'm fine as long as we are consistent with quoting of single word instructions. Ideally I would like that we quote both single and multi word for consistency, but you are the one doing the work so I'm not going to oppose to not quoting single words. Thanks, Roger.
--- a/xen/arch/x86/Makefile +++ b/xen/arch/x86/Makefile @@ -235,7 +235,10 @@ $(BASEDIR)/include/asm-x86/asm-macros.h: echo '#if 0' >$@.new echo '.if 0' >>$@.new echo '#endif' >>$@.new + echo '#ifndef __ASM_MACROS_H__' >>$@.new + echo '#define __ASM_MACROS_H__' >>$@.new echo 'asm ( ".include \"$@\"" );' >>$@.new + echo '#endif /* __ASM_MACROS_H__ */' >>$@.new echo '#if 0' >>$@.new echo '.endif' >>$@.new cat $< >>$@.new --- a/xen/arch/x86/arch.mk +++ b/xen/arch/x86/arch.mk @@ -20,6 +20,7 @@ $(call as-option-add,CFLAGS,CC,"rdrand % $(call as-option-add,CFLAGS,CC,"rdfsbase %rax",-DHAVE_AS_FSGSBASE) $(call as-option-add,CFLAGS,CC,"xsaveopt (%rax)",-DHAVE_AS_XSAVEOPT) $(call as-option-add,CFLAGS,CC,"rdseed %eax",-DHAVE_AS_RDSEED) +$(call as-option-add,CFLAGS,CC,"clac",-DHAVE_AS_CLAC_STAC) $(call as-option-add,CFLAGS,CC,"clwb (%rax)",-DHAVE_AS_CLWB) $(call as-option-add,CFLAGS,CC,".equ \"x\"$$(comma)1",-DHAVE_AS_QUOTED_SYM) $(call as-option-add,CFLAGS,CC,"invpcid (%rax)$$(comma)%rax",-DHAVE_AS_INVPCID) --- a/xen/arch/x86/asm-macros.c +++ b/xen/arch/x86/asm-macros.c @@ -1 +1,2 @@ +#include <asm/asm-defns.h> #include <asm/alternative-asm.h> --- /dev/null +++ b/xen/include/asm-x86/asm-defns.h @@ -0,0 +1,9 @@ +#ifndef HAVE_AS_CLAC_STAC +.macro clac + .byte 0x0f, 0x01, 0xca +.endm + +.macro stac + .byte 0x0f, 0x01, 0xcb +.endm +#endif --- a/xen/include/asm-x86/asm_defns.h +++ b/xen/include/asm-x86/asm_defns.h @@ -13,10 +13,12 @@ #include <asm/alternative.h> #ifdef __ASSEMBLY__ +#include <asm/asm-defns.h> #ifndef CONFIG_INDIRECT_THUNK .equ CONFIG_INDIRECT_THUNK, 0 #endif #else +#include <asm/asm-macros.h> asm ( "\t.equ CONFIG_INDIRECT_THUNK, " __stringify(IS_ENABLED(CONFIG_INDIRECT_THUNK)) ); #endif @@ -200,34 +202,27 @@ register unsigned long current_stack_poi #endif -/* "Raw" instruction opcodes */ -#define __ASM_CLAC ".byte 0x0f,0x01,0xca" -#define __ASM_STAC ".byte 0x0f,0x01,0xcb" - #ifdef __ASSEMBLY__ .macro ASM_STAC - ALTERNATIVE "", __ASM_STAC, X86_FEATURE_XEN_SMAP + ALTERNATIVE "", stac, X86_FEATURE_XEN_SMAP .endm .macro ASM_CLAC - ALTERNATIVE "", __ASM_CLAC, X86_FEATURE_XEN_SMAP + ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP .endm #else static always_inline void clac(void) { /* Note: a barrier is implicit in alternative() */ - alternative("", __ASM_CLAC, X86_FEATURE_XEN_SMAP); + alternative("", "clac", X86_FEATURE_XEN_SMAP); } static always_inline void stac(void) { /* Note: a barrier is implicit in alternative() */ - alternative("", __ASM_STAC, X86_FEATURE_XEN_SMAP); + alternative("", "stac", X86_FEATURE_XEN_SMAP); } #endif -#undef __ASM_STAC -#undef __ASM_CLAC - #ifdef __ASSEMBLY__ .macro SAVE_ALL op, compat=0 .ifeqs "\op", "CLAC"
Introduce proper assembler macros instead, enabled only when the assembler itself doesn't support the insns. To avoid duplicating the macros for assembly and C files, have them processed into asm-macros.h. This in turn requires adding a multiple inclusion guard when generating that header. No change to generated code. Signed-off-by: Jan Beulich <jbeulich@suse.com>