ClangBuiltLinux/linux

"invalid output constraint '+Q' in asm" when building RISCV allyesconfig

nathanchance opened this issue · 8 comments

On mainline RISCV allyesconfig:

$ make -j$(nproc) -s ARCH=riscv CROSS_COMPILE=riscv64-linux-gnu- LD=riscv64-linux-gnu-ld LLVM=1 LLVM_IAS=1 O=out/riscv64 distclean allyesconfig drivers/crypto/hisilicon/qm.o
drivers/crypto/hisilicon/qm.c:355:10: error: invalid output constraint '+Q' in asm
                       "+Q" (*((char __iomem *)fun_base))
                       ^

A look at the function:

/* 128 bit should be written to hardware at one time to trigger a mailbox */
static void qm_mb_write(struct hisi_qm *qm, const void *src)
{
	void __iomem *fun_base = qm->io_base + QM_MB_CMD_SEND_BASE;
	unsigned long tmp0 = 0, tmp1 = 0;

	if (!IS_ENABLED(CONFIG_ARM64)) {
		memcpy_toio(fun_base, src, 16);
		wmb();
		return;
	}

	asm volatile("ldp %0, %1, %3\n"
		     "stp %0, %1, %2\n"
		     "dsb sy\n"
		     : "=&r" (tmp0),
		       "=&r" (tmp1),
		       "+Q" (*((char __iomem *)fun_base))
		     : "Q" (*((char *)src))
		     : "memory");
}

GCC would have the same error but it does not evaluate the inline asm because it realizes that it is dead code due to the !IS_ENABLE(CONFIG_ARM64) (byproduct of #3, even though we are using the integrated assembler). If you remove that if block, GCC throws the same error:

drivers/crypto/hisilicon/qm.c: In function 'qm_mb':
drivers/crypto/hisilicon/qm.c:344:2: error: impossible constraint in 'asm'
  344 |  asm volatile("ldp %0, %1, %3\n"
      |  ^~~

This driver does not actually run on anything other than ARM64 but it gets selected by COMPILE_TEST, which was added by @arndb in a7174f9. I know that IS_ENABLED(CONFIG_...) is preferred to #ifdef CONFIG_... but there is no point in evaluating asm code that is specific to one architecture.

diff --git a/drivers/crypto/hisilicon/qm.c b/drivers/crypto/hisilicon/qm.c
index f795fb557630..fbbe14b6e92c 100644
--- a/drivers/crypto/hisilicon/qm.c
+++ b/drivers/crypto/hisilicon/qm.c
@@ -341,12 +341,7 @@ static void qm_mb_write(struct hisi_qm *qm, const void *src)
        void __iomem *fun_base = qm->io_base + QM_MB_CMD_SEND_BASE;
        unsigned long tmp0 = 0, tmp1 = 0;

-       if (!IS_ENABLED(CONFIG_ARM64)) {
-               memcpy_toio(fun_base, src, 16);
-               wmb();
-               return;
-       }
-
+#ifdef CONFIG_ARM64
        asm volatile("ldp %0, %1, %3\n"
                     "stp %0, %1, %2\n"
                     "dsb sy\n"
@@ -355,6 +350,10 @@ static void qm_mb_write(struct hisi_qm *qm, const void *src)
                       "+Q" (*((char __iomem *)fun_base))
                     : "Q" (*((char *)src))
                     : "memory");
+#else
+       memcpy_toio(fun_base, src, 16);
+       wmb();
+#endif
 }

 static int qm_mb(struct hisi_qm *qm, u8 cmd, dma_addr_t dma_addr, u16 queue,

Open to other ideas though.

I'm not sure how easy it will be to fix clang for that. I think it's happening during parsing while clang is collecting other information about the inline assembly. I think it needs to know the meaning of the constraints to collect that information in the AST.

cc @echristo who probably knows more about this area than me.

I don't see anything about Q in the generic or machine specific docs, is Q new? Oh...

GCC would have the same error but it does not evaluate the inline asm because it realizes that it is dead code due to the !IS_ENABLE(CONFIG_ARM64)

It's aarch64...I see. Sorry for the noise @topperc (I thought Q was an unsupported constraint for riscv...looks like that's not the case). We've seen this kind of issue before and can clean up in source (see also #3)

If the function has aarch64 specific machine constraints, it is not architecture portable code and should be using KConfig to disable the driver by depending on arm64.

It used to before a7174f9. It is a pattern in the kernel to allow compile testing of drivers for specific architectures through CONFIG_COMPILE_TEST so that build errors can be more easily caught. I will send the diff that I have above along shortly, I believe that it is the proper fix.

Actually, this is no longer reproducible after c73d187, because RISC-V does not implement ACPI. Closing for now, this can be revisited if it is ever relevant again.

I saw this mentioned in ClangBuiltLinux/meeting-notes#41. @nathanchance is there more to do here?

I do not think so, the series from the RISC-V folks to turn on ACPI has a patch to avoid this issue, so we should notice no breakage: https://lore.kernel.org/20230404182037.863533-24-sunilvl@ventanamicro.com/