Re: Usage of "p" constraint in BPF inline asm

["Jose E. Marchesi" <jose.marchesi@oracle.com>]

> On Thu, 2023-08-10 at 10:45 -0700, Yonghong Song wrote:
>> 
>> On 8/10/23 10:39 AM, Jose E. Marchesi wrote:
>> > 
>> > > On 8/10/23 3:35 AM, Jose E. Marchesi wrote:
>> > > > Hello.
>> > > > We found that some of the BPF selftests use the "p" constraint in
>> > > > inline
>> > > > assembly snippets, for input operands for MOV (rN = rM) instructions.
>> > > > This is mainly done via the __imm_ptr macro defined in
>> > > > tools/testing/selftests/bpf/progs/bpf_misc.h:
>> > > >     #define __imm_ptr(name) [name]"p"(&name)
>> > > > Example:
>> > > >     int consume_first_item_only(void *ctx)
>> > > >     {
>> > > >           struct bpf_iter_num iter;
>> > > >           asm volatile (
>> > > >                   /* create iterator */
>> > > >                   "r1 = %[iter];"
>> > > >                   [...]
>> > > >                   :
>> > > >                   : __imm_ptr(iter)
>> > > >                   : CLOBBERS);
>> > > >           [...]
>> > > >     }
>> > > > Little equivalent reproducer:
>> > > >     int bar ()
>> > > >     {
>> > > >       int jorl;
>> > > >       asm volatile ("r1 = %a[jorl]" : : [jorl]"p"(&jorl));
>> > > >       return jorl;
>> > > >     }
>> > > > The "p" constraint is a tricky one.  It is documented in the GCC
>> > > > manual
>> > > > section "Simple Constraints":
>> > > >     An operand that is a valid memory address is allowed.  This is
>> > > > for
>> > > >     ``load address'' and ``push address'' instructions.
>> > > >     p in the constraint must be accompanied by address_operand as the
>> > > >     predicate in the match_operand.  This predicate interprets the mode
>> > > >     specified in the match_operand as the mode of the memory reference for
>> > > >     which the address would be valid.
>> > > > There are two problems:
>> > > > 1. It is questionable whether that constraint was ever intended to
>> > > > be
>> > > >      used in inline assembly templates, because its behavior really
>> > > >      depends on compiler internals.  A "memory address" is not the same
>> > > >      than a "memory operand" or a "memory reference" (constraint "m"), and
>> > > >      in fact its usage in the template above results in an error in both
>> > > >      x86_64-linux-gnu and bpf-unkonwn-none:
>> > > >        foo.c: In function ‘bar’:
>> > > >        foo.c:6:3: error: invalid 'asm': invalid expression as operand
>> > > >           6 |   asm volatile ("r1 = %[jorl]" : : [jorl]"p"(&jorl));
>> > > >             |   ^~~
>> > > >      I would assume the same happens with aarch64, riscv, and
>> > > > most/all
>> > > >      other targets in GCC, that do not accept operands of the form A + B
>> > > >      that are not wrapped either in a const or in a memory reference.
>> > > >      To avoid that error, the usage of the "p" constraint in internal
>> > > > GCC
>> > > >      instruction templates is supposed to be complemented by the 'a'
>> > > >      modifier, like in:
>> > > >        asm volatile ("r1 = %a[jorl]" : : [jorl]"p"(&jorl));
>> > > >      Internally documented (in GCC's final.cc) as:
>> > > >        %aN means expect operand N to be a memory address
>> > > >           (not a memory reference!) and print a reference
>> > > >           to that address.
>> > > >      That works because when the modifier 'a' is found, GCC prints an
>> > > >      "operand address", which is not the same than an "operand".
>> > > >      But...
>> > > > 2. Even if we used the internal 'a' modifier (we shouldn't) the 'rN
>> > > > =
>> > > >      rM' instruction really requires a register argument.  In cases
>> > > >      involving automatics, like in the examples above, we easily end with:
>> > > >        bar:
>> > > >           #APP
>> > > >               r1 = r10-4
>> > > >           #NO_APP
>> > > >      In other cases we could conceibly also end with a 64-bit label
>> > > > that
>> > > >      may overflow the 32-bit immediate operand of `rN = imm32'
>> > > >      instructions:
>> > > >           r1 = foo
>> > > >      All of which is clearly wrong.
>> > > > clang happens to do "the right thing" in the current usage of
>> > > > __imm_ptr
>> > > > in the BPF tests, because even with -O2 it seems to "reload" the
>> > > > fp-relative address of the automatic to a register like in:
>> > > >     bar:
>> > > > 	r1 = r10
>> > > > 	r1 += -4
>> > > > 	#APP
>> > > > 	r1 = r1
>> > > > 	#NO_APP
>> > > 
>> > > Unfortunately, the modifier 'a' won't work for clang.
>> > > 
>> > > $ cat t.c  int bar ()  {     int jorl;     asm volatile ("r1 =
>> > > %a[jorl]" : : [jorl]"p"(&jorl));     return jorl;  }  $ gcc -O2 -g -S
>> > > t.c  $ clang --target=bpf -O2 -g -S t.c  clang:
>> > > ../lib/Target/BPF/BPFAsmPrinter.cpp:126: virtual bool
>> > > {anonymous}::BPFAsmPrinter::PrintAsmMemoryOperand(const
>> > > llvm::MachineInstr*, unsigned int, const char*, llvm::raw_ostream&):
>> > > Assertion `Offs
>> > > etMO.isImm() && "Unexpected offset for inline asm memory operand."' failed.
>> > > ...
>> > > 
>> > > I guess BPF backend can try to add support for this 'a' modifier
>> > > if necessary.
>> > 
>> > I wouldn't advise that: it is an internal GCC detail that just happens
>> > to work in inline asm.  Also, even if you did that constraint may result
>> > in operands that are not single registers.  It would be better to use
>> > "r" constraint instead.
>> 
>> Sounds good. We also do not want to add support for this 'a' thing
>> if there are alternatives.
>> 
>> > 
>> > > 
>> > > > Which is what GCC would generate with -O0.  Whether this is by chance or
>> > > > by design (Nick, do you know?) I don't think the compiler should be
>> > > > expected to do that reload driven by the "p" constraint.
>> > > > I would suggest to change that macro (and similar out of macro
>> > > > usages of
>> > > > the "p" constraint in selftests/bpf/progs/iters.c) to use the "r"
>> > > > constraint instead.  If a register is what is required, we should let
>> > > > the compiler know.
>> > > 
>> > > Could you specify what is the syntax ("r" constraint) which will work
>> > > for both clang and gcc?
>> > 
>> > Instead of:
>> > 
>> >     #define __imm_ptr(name) [name]"p"(&name)
>> > 
>> > Use this:
>> > 
>> >     #define __imm_ptr(name) [name]"r"(&name)
>> > 
>> > That assures that the operand (the pointer value) will be available in
>> > the form of a single register.
>> 
>> Okay, this seems work for both gcc and clang.
>> Eduard, what do you think about the above suggested change?
>
> BPF selftests are passing with this change.
> The macro in question is used in 3 files:
> - verifier_subprog_precision.c
> - iters_state_safety.c
> - iters_looping.c
>
> I don't see any difference in the generated object files
> (at-least for cpuv4).
>
> So, I guess we should be fine.

Note the same fix would be needed in the inline asm in
selftests/bpf/progs/iters.c:iter_err_unsafe_asm_loop.

>
>> 
>> > 
>> > > 
>> > > > Thoughts?
>> > > > PS: I am aware that the x86 port of the kernel uses the "p"
>> > > > constraint
>> > > >       in the percpu macros (arch/x86/include/asm/percpu.h) but that usage
>> > > >       is in a different context (I would assume it is used in x86
>> > > >       instructions that get constant addresses or global addresses loaded
>> > > >       in registers and not automatics) where it seems to work well.
>> > > > 
>>