[DTrace-devel] [PATCH 03/12] cg: reserve %r7 and %r8 in trampolines

[Kris Van Hees]

On Thu, Jan 11, 2024 at 03:00:54PM -0500, Eugene Loh via DTrace-devel wrote:
> Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
> 
> I guess what I don't get is whether we're using our regset management in
> trampoline cg.  The policy used to be, "No.  We know what we're doing."  In
> contrast, clause cg says, "Use regset since you don't know which regs are in
> use."  This patch seems to muddy the waters.  We reserve %r7 and %r8, but we
> use all the other regs without spill/fill guards.  (E.g., calling a BPF
> helper uses %r0-%r5, and some providers have trampolines that use %r6.)

The reason is that upcoming work in this patch series (accessing variables
from trampolines) makes use of the code generator rather than emitting pure
assembler instructions.  And since the code generator uses register
allocation, we need to make sure it won't step on the crucial %r7 and %r8
registers.

> So, is there a clear policy on when trampoline cg should or shouldn't use
> regset?  We didn't use to do this "because it's trampoline code."  But we
> aren't adding regset in trampoline code for the other regs we're using.
> 
> Anyhow, maybe there is no need to be careful (yet).  This patch should work
> and get us expediently through an immediate problem?

Right now, only the io provider will make use of this and it is done in a very
careful way where we do not use other registers yet.  But yes, in the future
we will have to put more guards (register allocation) even in the trampoline
code if a provider is going to make use of the code generator itself.

> On 1/5/24 20:09, Kris Van Hees wrote:
> > On Fri, Jan 05, 2024 at 04:20:29PM -0500, Eugene Loh via DTrace-devel wrote:
> > > I do not understand this patch.
> > > 
> > > Why isn't the 04/12 patch squashed back into this one?
> > Two different things.
> > 
> > > Since when are we using the immature reg-management mechanism for the
> > > trampoline cg?
> > Because the io provider will need it because it is using codw generator
> > features that use reg alloc/free.
> > 
> > > Why do %r7 and %r8 in the trampoline count against any DIF reg limit?  Isn't
> > > their use outside the realm of DIF registers?
> > No, only %r9 is reserved.  We allow the code generator to allocate any
> > registers from %r0 through %r8.
> > 
> > > We use %r9 in the trampoline;  why doesn't this patch also reserve it?  Same
> > > with %r6.
> > Because the register allocator doesn't know about %r9.
> > 
> > We do not use %r6 as a reserved register AFAIK.
> > 
> > > Those are all basically rhetorical questions.  The real point is that this
> > > patch is intended to support the -xiregs option but the right thing to do is
> > > to kill/deprecate that option.  The only legal value is -xiregs=goldilocks.
> > > Okay, cultural reference, but there is only one value that works.  Because
> > > of BPF (and how we've wired things), iiuc there is only one legal value of
> > > -xiregs.  So we should simply back-support this option by telling users who
> > > try to change it that they cannot.
> > The -xiregs= option is a different thing.  this patch is about the code
> > generator needing to be told that %r7 and %r8 are in use in trampoline code,
> > so that they do not get allocated (or if they are, get spilled/reloaded).
> > 
> > > Or, what am I missing?
> > > 
> > > On 1/5/24 00:26, Kris Van Hees via DTrace-devel wrote:
> > > > Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> > > > ---
> > > >    libdtrace/dt_cg.c | 12 +++++++++++-
> > > >    1 file changed, 11 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> > > > index 30b1da16..d9d56f0a 100644
> > > > --- a/libdtrace/dt_cg.c
> > > > +++ b/libdtrace/dt_cg.c
> > > > @@ -1,6 +1,6 @@
> > > >    /*
> > > >     * Oracle Linux DTrace.
> > > > - * Copyright (c) 2005, 2023, Oracle and/or its affiliates. All rights reserved.
> > > > + * Copyright (c) 2005, 2024, Oracle and/or its affiliates. All rights reserved.
> > > >     * Licensed under the Universal Permissive License v 1.0 as shown at
> > > >     * http://oss.oracle.com/licenses/upl.
> > > >     */
> > > > @@ -138,6 +138,7 @@ dt_cg_tramp_prologue_act(dt_pcb_t *pcb, dt_activity_t act)
> > > >    {
> > > >    	dtrace_hdl_t	*dtp = pcb->pcb_hdl;
> > > >    	dt_irlist_t	*dlp = &pcb->pcb_ir;
> > > > +	dt_regset_t	*drp = pcb->pcb_regs;
> > > >    	dt_ident_t	*aggs = dt_dlib_get_map(dtp, "aggs");
> > > >    	dt_ident_t	*mem = dt_dlib_get_map(dtp, "mem");
> > > >    	dt_ident_t	*state = dt_dlib_get_map(dtp, "state");
> > > > @@ -151,6 +152,10 @@ dt_cg_tramp_prologue_act(dt_pcb_t *pcb, dt_activity_t act)
> > > >    	assert(prid != NULL);
> > > >    	assert(ro_off != NULL);
> > > > +	/* Reserve %r7 and %r8. */
> > > > +	dt_regset_xalloc(drp, BPF_REG_7);
> > > > +	dt_regset_xalloc(drp, BPF_REG_8);
> > > > +
> > > >    	/*
> > > >    	 * On input, %r1 is the BPF context.
> > > >    	 *
> > > > @@ -718,6 +723,7 @@ void
> > > >    dt_cg_tramp_return(dt_pcb_t *pcb)
> > > >    {
> > > >    	dt_irlist_t	*dlp = &pcb->pcb_ir;
> > > > +	dt_regset_t	*drp = pcb->pcb_regs;
> > > >    	/*
> > > >    	 * exit:
> > > > @@ -728,6 +734,10 @@ dt_cg_tramp_return(dt_pcb_t *pcb)
> > > >    	emitl(dlp, pcb->pcb_exitlbl,
> > > >    		   BPF_MOV_IMM(BPF_REG_0, 0));
> > > >    	emit(dlp,  BPF_RETURN());
> > > > +
> > > > +	/* Free %r7 and %r8. */
> > > > +	dt_regset_free(drp, BPF_REG_7);
> > > > +	dt_regset_free(drp, BPF_REG_8);
> > > >    }
> > > >    void
> > > _______________________________________________
> > > DTrace-devel mailing list
> > > DTrace-devel at oss.oracle.com
> > > https://oss.oracle.com/mailman/listinfo/dtrace-devel
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel