| Message ID | 10816604a8625b5052f134e54c406fb4e7b6c898.1712649614.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/riscv: check whether the assembler has Zbb extension support | expand |
On 09.04.2024 10:00, Oleksii Kurochko wrote: > Update the argument of the as-insn for the Zbb case to verify that > Zbb is supported not only by a compiler, but also by an assembler. > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> While technically all if fine here, I'm afraid I have a couple of nits: > --- a/xen/arch/riscv/arch.mk > +++ b/xen/arch/riscv/arch.mk > @@ -11,7 +11,8 @@ riscv-march-$(CONFIG_RISCV_ISA_C) := $(riscv-march-y)c > > riscv-generic-flags := $(riscv-abi-y) -march=$(riscv-march-y) > > -zbb := $(call as-insn,$(CC) $(riscv-generic-flags)_zbb,"",_zbb) > +zbb_insn := "andn t0, t0, t0" As can be seen on the following line (as-insn, riscv-generic-flags) we prefer dashes over underscores in new variables' names. (Another question is whether the variable is needed in the first place, but that's pretty surely personal taste territory.) Furthermore this extra variable suggests there's yet more room for abstraction (as already suggested before). > +zbb := $(call as-insn,$(CC) $(riscv-generic-flags)_zbb,${zbb_insn},_zbb) Why figure braces in one case when everywhere else we use parentheses for variable references? There's no functional difference sure, but inconsistent use specifically may raise the question for some future reader whether there actually is one. Jan
On Thu, 2024-04-18 at 12:00 +0200, Jan Beulich wrote: > On 09.04.2024 10:00, Oleksii Kurochko wrote: > > Update the argument of the as-insn for the Zbb case to verify that > > Zbb is supported not only by a compiler, but also by an assembler. > > > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> > > While technically all if fine here, I'm afraid I have a couple of > nits: > > > --- a/xen/arch/riscv/arch.mk > > +++ b/xen/arch/riscv/arch.mk > > @@ -11,7 +11,8 @@ riscv-march-$(CONFIG_RISCV_ISA_C) := > > $(riscv-march-y)c > > > > riscv-generic-flags := $(riscv-abi-y) -march=$(riscv-march-y) > > > > -zbb := $(call as-insn,$(CC) $(riscv-generic-flags)_zbb,"",_zbb) > > +zbb_insn := "andn t0, t0, t0" > > As can be seen on the following line (as-insn, riscv-generic-flags) > we > prefer dashes over underscores in new variables' names. (Another > question is > whether the variable is needed in the first place, but that's pretty > surely > personal taste territory.) It seems to me that we need it; otherwise, if we don't use the variable and put everything directly: zbb := $(call as-insn,$(CC) $(riscv-generic-flags)_zbb,"andn t0, t0, t0",_zbb) Then as-insn will receive incorrect arguments because of the ',' used in the instruction. It will parse it as 3 separete arguments ("and, t0 and t0"), which will lead to a compilation error: /bin/sh: -c: line 1: unexpected EOF while looking for matching `'' /bin/sh: -c: line 2: syntax error: unexpected end of file Probably I am missing something and it can be done in a different way. > > Furthermore this extra variable suggests there's yet more room for > abstraction (as already suggested before). > > > +zbb := $(call as-insn,$(CC) $(riscv-generic- > > flags)_zbb,${zbb_insn},_zbb) > > Why figure braces in one case when everywhere else we use parentheses > for > variable references? There's no functional difference sure, but > inconsistent > use specifically may raise the question for some future reader > whether there > actually is one. I see the usage of {} somewhere else in code base, so automatically use them here too. Sure, it should be consistent. Thanks. ~ Oleksii
On 18.04.2024 15:09, Oleksii wrote: > On Thu, 2024-04-18 at 12:00 +0200, Jan Beulich wrote: >> On 09.04.2024 10:00, Oleksii Kurochko wrote: >>> Update the argument of the as-insn for the Zbb case to verify that >>> Zbb is supported not only by a compiler, but also by an assembler. >>> >>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> >> >> While technically all if fine here, I'm afraid I have a couple of >> nits: >> >>> --- a/xen/arch/riscv/arch.mk >>> +++ b/xen/arch/riscv/arch.mk >>> @@ -11,7 +11,8 @@ riscv-march-$(CONFIG_RISCV_ISA_C) := >>> $(riscv-march-y)c >>> >>> riscv-generic-flags := $(riscv-abi-y) -march=$(riscv-march-y) >>> >>> -zbb := $(call as-insn,$(CC) $(riscv-generic-flags)_zbb,"",_zbb) >>> +zbb_insn := "andn t0, t0, t0" >> >> As can be seen on the following line (as-insn, riscv-generic-flags) >> we >> prefer dashes over underscores in new variables' names. (Another >> question is >> whether the variable is needed in the first place, but that's pretty >> surely >> personal taste territory.) > > It seems to me that we need it; otherwise, if we don't use the variable > and put everything directly: > zbb := $(call as-insn,$(CC) $(riscv-generic-flags)_zbb,"andn t0, t0, > t0",_zbb) > Then as-insn will receive incorrect arguments because of the ',' used > in the instruction. It will parse it as 3 separete arguments ("and, t0 > and t0"), which will lead to a compilation error: > /bin/sh: -c: line 1: unexpected EOF while looking for matching `'' > /bin/sh: -c: line 2: syntax error: unexpected end of file > > Probably I am missing something and it can be done in a different way. Well, as said - this is perhaps largely personal taste. Yet technically it isn't needed, assuming you're aware of the "comma" and "space" macros that we have at the top of ./Config.mk. You'll find $(comma) used for similar purposes in x86. Jan
diff --git a/xen/arch/riscv/arch.mk b/xen/arch/riscv/arch.mk index 53f3575e7d..6c53953acb 100644 --- a/xen/arch/riscv/arch.mk +++ b/xen/arch/riscv/arch.mk @@ -11,7 +11,8 @@ riscv-march-$(CONFIG_RISCV_ISA_C) := $(riscv-march-y)c riscv-generic-flags := $(riscv-abi-y) -march=$(riscv-march-y) -zbb := $(call as-insn,$(CC) $(riscv-generic-flags)_zbb,"",_zbb) +zbb_insn := "andn t0, t0, t0" +zbb := $(call as-insn,$(CC) $(riscv-generic-flags)_zbb,${zbb_insn},_zbb) zihintpause := $(call as-insn, \ $(CC) $(riscv-generic-flags)_zihintpause,"pause",_zihintpause)
Update the argument of the as-insn for the Zbb case to verify that Zbb is supported not only by a compiler, but also by an assembler. Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- xen/arch/riscv/arch.mk | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)