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

[nathanchance]

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.

[nickdesaulniers]

cc @topperc

[topperc]

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.

[nickdesaulniers]

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)

[nickdesaulniers]

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.

[nathanchance]

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.

[nathanchance]

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.

[nickdesaulniers]

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

[nathanchance]

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/