ARM: Allow cp10/cp11 coprocessor register access with a warning

[falstaff84]

Summary

The Linux kernel uses quite some mcr/mrc instructions throughout the ARM port. Probably most of them could be converted to the appropriate vmsr/vmrs instructions along with appropriate FPU definition, e.g.:

arch/arm/vfp/vfpmodule.c:345:2: error: invalid operand for instruction
        fmxr(FPEXC, fpexc & ~(FPEXC_EX|FPEXC_DEX|FPEXC_FP2V|FPEXC_VV|FPEXC_TRAP_MASK));
        ^
arch/arm/vfp/vfpinstr.h:82:6: note: expanded from macro 'fmxr'
        asm("mcr p10, 7, %0, " vfpreg(_vfp_) ", cr0, 0 @ fmxr   " #_vfp_ ", %0" \
            ^
<inline asm>:1:6: note: instantiated into assembly here
        mcr p10, 7, r0, cr8, cr0, 0 @ fmxr      FPEXC, r0
            ^

However, there is at least one case where an unofficial/(SoC specific?) control register is accessed:

static u32 venum_read_pmresr(void)
{
        u32 val;
        asm volatile("mrc p10, 7, %0, c11, c0, 0" : "=r" (val));
        return val;
}

There is no register definition for control register 11 in lib/Target/ARM/ARMRegisterInfo.td.

This patch converts the restriction of mcr/mrc cp10/cp11 instructions to warnings. The warnings can be suppressed using -no-deprecated-warn which is probably what the Linux kernel will use for the time being.

See also the discussion on the kernel side, tracked in this issue: https://github.com/ClangBuiltLinux/linux/issues/306.

[peter.smith]

I prefer having the warning here as there are use cases for using the coprocessor interface for VFP instructions, notably when access to the instructions is gated at run-time and the same source needs to assemble on multiple targets. From what I can see the mrc in venum_read_pmresr() is not a VFP/Neon instruction. It hails from arch/arm/kernel/perf_event_v7.c and is specific to Qualcomm's Scorpion and Krait CPUs. My understanding is that CP15 is used for the PMU in v7, it is possible that the use of p10 there is a mistake but I'm not knowledgeable enough about Scorpion and Krait to know whether this is the case.

You'll need to update test/MC/ARM/diagnostics.s as this is currently failing with this patch as it tests whether the old error message is being produced. You can run the tests from the build directory with make/ninja check-llvm. To run an individual test with output then from the build dir: bin/llvm-lit -v -a /path/to/llvm/test/MC/ARM/diagnostics.s

[falstaff84]

In D59733#1441197, @peter.smith wrote:

From what I can see the mrc in venum_read_pmresr() is not a VFP/Neon instruction. It hails from arch/arm/kernel/perf_event_v7.c and is specific to Qualcomm's Scorpion and Krait CPUs. My understanding is that CP15 is used for the PMU in v7, it is possible that the use of p10 there is a mistake but I'm not knowledgeable enough about Scorpion and Krait to know whether this is the case.

Yeah not sure, I tried to research the origin of this code, and it seems to be there since quite a while (and also got refactored over time). I assume this code has been used and is working.

You'll need to update test/MC/ARM/diagnostics.s as this is currently failing with this patch as it tests whether the old error message is being produced. You can run the tests from the build directory with make/ninja check-llvm. To run an individual test with output then from the build dir: bin/llvm-lit -v -a /path/to/llvm/test/MC/ARM/diagnostics.s

Updated this test and another failing test in llvm/test/MC/ARM/coprocessors.s.

[simon_tatham]

I've made a couple of nitpicks to the patch, but my more general question is: if we think that this pair of coprocessors should be changed from "reject completely" to "accept with a warning", why not all of them? What argument applies to CP10 and CP11 that doesn't apply to all the others, apart from the immediate short-term argument of "this is the one that's currently causing somebody a problem"?

llvm/lib/Target/ARM/ARMBaseInstrInfo.h

692 ↗

(On Diff #272269)

I think it's worth leaving a comment here to explain why we're not disallowing CP10 and CP11. The point of this function is to centralize all the coprocessor validity tests in one place, so I can easily imagine someone else coming along later and adding the "missing" one if there isn't something here telling them that it's omitted on purpose.

llvm/test/MC/ARM/coprocessors.s

52 ↗

(On Diff #272269)

This seems like a confusing way to change the test. The check prefix ACCEPT-AB is for test runs where coprocessors 10 and 11 (A and B) are accepted, and REJECT-AB for where they are rejected. So please leave these check lines as they were, so that each check prefix matches the message it's expecting, and instead change the set of --check-prefix options on the llvm-mc command whose behavior is changing.

[falstaff84]

@simon_tatham not sure what you exactly mean, for ARMv7 we only disallowed this pair, and this change removes that restriction. So now, all of them are allowed...

I did not look into the ARMv8 situation all to much, but the ARMv8-M reference manual explicitly mentions cp10/cp11 as allowed coprocessor names:

ARMv8-M reference manual (ARM DDI 0553A.e (ID060617)) for MCR/MCR coproc says:

The valid coprocessor names are p10, p11, p14, and p15.

llvm/test/MC/ARM/coprocessors.s

52 ↗

(On Diff #272269)

Oh I see, now I get what does numbers mean! For some reason, I just dismissed the idea that this numbers have meaning :-)

Of course, will fix this.

[simon_tatham]

for ARMv7 we only disallowed this pair, and this change removes that restriction. So now, all of them are allowed...

Yes, for Arm v7.

What I'm getting at is: should we turn the whole of isValidCoprocessorNumber into warnings of this kind, on the same principle (that it's OK to write any CDP instruction if it's in the context of code that will dynamically decide at run time which instructions to actually execute)? If not, what makes this v7 constraint different from the v8 ones?

[falstaff84]

In D59733#2106433, @simon_tatham wrote:

for ARMv7 we only disallowed this pair, and this change removes that restriction. So now, all of them are allowed...

Yes, for Arm v7.

What I'm getting at is: should we turn the whole of isValidCoprocessorNumber into warnings of this kind, on the same principle (that it's OK to write any CDP instruction if it's in the context of code that will dynamically decide at run time which instructions to actually execute)? If not, what makes this v7 constraint different from the v8 ones?

Going by the architecture reference manuals, I came up with the following data:

• ARMv7-A/R (ARM DDI 0406C.c): <coproc> ... The generic coprocessor names are p0-p15.
• ARMv7-M (ARM DDI 0403E.b): <coproc> ... The standard generic coprocessor names are p0-p15.
• Armv8-A (ARM DDI 0487F.b): <coproc> .. It can have the following values .. p14, p15 (MCR/MRC only, also chapter G1.19 "The AArch32 System register interface" says "[...] with permitted coproc values of 0b1110 and 0b1111")
• ARMv8-M (ARM DDI 0553A.e): <coproc> ... The valid coprocessor names are p0 to p7, p10, and p11. (for CBD) and ... The valid coprocessor names are p10, p11, p14, and p15 (for MCR/MRC)
• Armv8-M (ARM DDI 0553B.i): <coproc> ... The generic coprocessor names are p0 to p15. (for Armv8-M Main Extension only?), BUT, there is decoding pseudocode:

if coproc IN {'100x', '101x', '111x'} then SEE "Floating-point and MVE";
if !HaveMainExt() then UNDEFINED;
cp = UInt(coproc);

So the restrictions for Armv8-A seems correct to me. Not sure about Armv8-M... It seems that restrictions changed between ARMv8-M and Armv8.1-M. So to me it seems that with main extension, all of them are allowed. Just p8, p9, p10, p11, p14, p15 are in the "Floating-point and MVE" range...

[simon_tatham]

OK, fair enough. In that case, please correct my nitpicks and I'll approve the revised patch.

[falstaff84]

• Added comment why allowing cp10/cp11 make sense
• Use check prefixes in coprocessors.s in a sensible mannor

[simon_tatham]

LGTM. Thanks for the fixes.

lib/Target/ARM/ARMInstrInfo.td

def MCR : MovRCopro<"mcr", 0 /* from ARM core register to coprocessor */,
                                   imm:$CRm, imm:$opc2)]>,
                     ComplexDeprecationPredicate<"MCR">;
 def : ARMInstAlias<"mcr${p} $cop, $opc1, $Rt, $CRn, $CRm",
                    (MCR p_imm:$cop, imm0_7:$opc1, GPR:$Rt, c_imm:$CRn,
                         c_imm:$CRm, 0, pred:$p)>;
 def MRC : MovRCopro<"mrc", 1 /* from coprocessor to ARM core register */,
                     (outs GPRwithAPSR:$Rt),
                     (ins p_imm:$cop, imm0_7:$opc1, c_imm:$CRn, c_imm:$CRm,
-                         imm0_7:$opc2), []>;
+                         imm0_7:$opc2), []>,
+                    ComplexDeprecationPredicate<"MRC">;
 def : ARMInstAlias<"mrc${p} $cop, $opc1, $Rt, $CRn, $CRm",
                    (MRC GPRwithAPSR:$Rt, p_imm:$cop, imm0_7:$opc1, c_imm:$CRn,
                         c_imm:$CRm, 0, pred:$p)>;
 
 def : ARMPat<(int_arm_mrc imm:$cop, imm:$opc1, imm:$CRn, imm:$CRm, imm:$opc2),
              (MRC imm:$cop, imm:$opc1, imm:$CRn, imm:$CRm, imm:$opc2)>;
 
 class MovRCopro2<string opc, bit direction, dag oops, dag iops,

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

ARMAsmParser::parseCoprocNumOperand(OperandVector &Operands) {
   SMLoc S = Parser.getTok().getLoc();
   const AsmToken &Tok = Parser.getTok();
   if (Tok.isNot(AsmToken::Identifier))
     return MatchOperand_NoMatch;
 
   int Num = MatchCoprocessorOperandName(Tok.getString(), 'p');
   if (Num == -1)
     return MatchOperand_NoMatch;
-  // ARMv7 and v8 don't allow cp10/cp11 due to VFP/NEON specific instructions
-  if ((hasV7Ops() || hasV8Ops()) && (Num == 10 || Num == 11))
-    return MatchOperand_NoMatch;
 
   Parser.Lex(); // Eat identifier token.
   Operands.push_back(ARMOperand::CreateCoprocNum(Num, S));
   return MatchOperand_Success;
 }
 
 /// parseCoprocRegOperand - Try to parse an coprocessor register operand. The
 /// token must be an Identifier when called, and if it is a coprocessor

lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp

if (STI.getFeatureBits()[llvm::ARM::HasV7Ops] &&
     // Checks for the deprecated CP15DMB encoding:
     // mcr p15, #0, rX, c7, c10, #5
     if (MI.getOperand(4).isImm() && MI.getOperand(4).getImm() == 10 &&
         (MI.getOperand(5).isImm() && MI.getOperand(5).getImm() == 5)) {
       Info = "deprecated since v7, use 'dmb'";
       return true;
     }
   }
+  if (STI.getFeatureBits()[llvm::ARM::HasV7Ops] &&
+      ((MI.getOperand(0).isImm() && MI.getOperand(0).getImm() == 10) ||
+       (MI.getOperand(0).isImm() && MI.getOperand(0).getImm() == 11))) {
+    Info = "since v7, cp10 and cp11 are reserved for advanced SIMD or floating "
+           "point instructions";
+    return true;
+  }
+  return false;
+}
+
+static bool getMRCDeprecationInfo(MCInst &MI, const MCSubtargetInfo &STI,
+                                  std::string &Info) {
+  if (STI.getFeatureBits()[llvm::ARM::HasV7Ops] &&
+      ((MI.getOperand(0).isImm() && MI.getOperand(0).getImm() == 10) ||
+       (MI.getOperand(0).isImm() && MI.getOperand(0).getImm() == 11))) {
+    Info = "since v7, cp10 and cp11 are reserved for advanced SIMD or floating "
+           "point instructions";
+    return true;
+  }
   return false;
 }
 
 static bool getITDeprecationInfo(MCInst &MI, const MCSubtargetInfo &STI,
                                  std::string &Info) {
   if (STI.getFeatureBits()[llvm::ARM::HasV8Ops] && MI.getOperand(1).isImm() &&
       MI.getOperand(1).getImm() != 8) {
     Info = "applying IT instruction to more than one subsequent instruction is "