[RFC PATCH v3 2/3] drm: implement decoding of DRM ioctls

[Eugene Syromiatnikov]

On Wed, Jul 31, 2019 at 02:49:45PM +0800, Zhibin Li wrote:
> On Mon, Jul 29, 2019 at 9:58 AM Eugene Syromiatnikov <esyr at redhat.com>
> wrote:
> > On Sat, Jul 27, 2019 at 11:57:16PM +0800, Zhibin Li wrote:
> >
> > > (drm_version, drm_unique, drm_get_map, drm_get_client, drm_get_stats,
> > > drm_add_map, drm_add_bufs, drm_mark_bufs, drm_info_bufs, drm_map_bufs,
> > > drm_free_bufs, drm_rm_map, drm_sarea_ctx, drm_res_ctx, drm_agp_enable,
> > > drm_agp_info, drm_agp_alloc, drm_agp_free, drm_mode_get_connector,
> > > drm_scatter_gather, drm_wait_vblank, drm_mode_getplaneresources,
> > > drm_mode_add_fb2, drm_mode_obj_getproperties, drm_mode_obj_setproperty
> > > drm_agp_bind): New functions.
> > > (drm_ioctl_mpers): Use them.
> >
> > I don't thing the list of functions is required for a new file GNU-style
> > changelog entry.
> 
> May I ask when this is changed? I guess I'm not following up.

It hasn't been changed, it's just one of deviations of chagnelog style
used in strace.  I have no objection against having these functions
mentioned, however (and, AFAIK, neither does Dmitry).


> > Note also that this file can be rather old and do not contain all the
> > ioctl constants, so it is likely also worth having an xlat with all
> > related DRM_IOC constants.
> 
> OK, I will give it a try. And does this also mean those ifdefs around
> ioctl constants which you said are unreliable could be removed
> because all DRM_IOC constants can be found in this case?

Yes.  Note, however, that related structure definitions should
be supplied in this case as well (I think, the PTP ioctl decoder
implementation could be an example).


> > > +static void
> > > +print_drm_iowr(const unsigned int nr, const unsigned int size,
> > > +            const char *str)
> > > +{
> > > +     tprintf("DRM_IOWR(%#x, %#x) /* %s */", nr, size, str);
> > This likely needs xlat style support.
> 
>  Any example or reference?

Hm, it looks like it's already covered by еру relevant ioctl code,
disregard that.


> >
> > > +                             /* In Linux commit v3.12-rc7~26^2~2 a u32
> > padding was added */
> > > +                             /* to struct drm_mode_get_connector so in
> > old kernel headers */
> > > +                             /* the size of this structure is 0x4c
> > instead of 0x50. */
> > drm argument sizes are constantly changing (please take a look at Gleb's
> > ioctl-related commits, they can provide a rough impression; I'm not sure
> > to which extent it is applicable to core DRM ioctls, however), a more
> > robust way of decoding drm ioctl number have to be implemented.
> >
> Yes I've seen Gleb updating ioctl entries according to the kernel release.
> The only thing that is changing is the size. Would _IOC_NR() be enough?

Yes.


> Considering the code below, it may be the case for
> > DRM_IOCTL_MODE_GETPLANERESOURCES, DRM_IOCTL_MODE_OBJ_GETPROPERTIES,
> > and DRM_IOCTL_MODE_OBJ_SETPROPERTY as well.
> >
> >
> It seems the corresponding structure of these three ioctl constants are not
> changing.
> The sizes of them are different among personalities because of
> byte-alignment. I will
> check git history to confirm that.

It may be the case that in case of core DRM ioctls there's not much change over
the years; in that case, a comment stating that would be enough.


> > > +     if (!syserror(tcp) && !umove(tcp, arg, &cap)) {
> > Adding a separate umove_no_syserror() macro might be a good idea.
> >
> >
> You mean umove_nor_syserror() ? Should this macro be added to defs.h
> or just in this file scope? I see some other code uses this, too.

Yes, please add it to defs.h, it should be useful for other ioctl
decoders as well.


> > > +             PRINT_FIELD_U("{", auth, magic);
> > I suspect that "magic" is expected to be hexadecimal.
> 
> I see that in the kernel code drivers/gpu/drm/drm_auth.c, there is a
> DRM_DEBUG() invocation which prints magic using "%u" so I simply
> follow that. Should magic numbers always be hexadecimal?

No, if there's some evidence of established printing format, it's better
to stick to that; it's just that I wasn't aware of it and presumed that
printing format, as magic numbers are more commonly being hexadecimal.


> > Can be factored out.
> >
> 
> Sorry for so many redundant codes. This week I will spend part of
> my time re-factoring codes.

No problem, correct code is better than non-repeating code; code
duplication, however, may lead to mistakes during changes, so it's
better be avoided in the long run.


> > Why handle field is not printed here? I suspect it has to be printed on
> > entering, hasn't it?
> 
> I took reference from the updated kernel code. In
> drivers/gpu/drm/drm_context.c: drm_legacy_getctx(), I couldn't see any use
> of handle field and the flags is also marked to zero. Actually I'm not sure
> what
> is going on here. Any thoughts?

Me neither, I was misguided by the ioctl name; ignore this comment,
then.


> > > +             PRINT_FIELD_U("{", handle, fd);
> > Why not PRINT_FIELD_FD?
> >
> > This also makes order of printing different comparing to order of fields
> > in the structure.
> >
> 
> It seems this one is using the fd the get a handle, shouldn't it be printed
> in such order?

Yes, but it's probably worth noting in a comment that such a deviation
is intentional.


> > > +             PRINT_FIELD_U(", ", handle, handle);
> > I feel that drm_handle_t likely needs a separate PRINT_FIELD_* macro.
> >
> 
> You mean like others in print_fields.h?

Yes, unless it's local to core DRM ioctls only.

> > > +# ifdef DRM_IOCTL_CRTC_GET_SEQUENCE
> > Ugh, those ifdef's make strace behaviour depend on the version of drm
> > headers it is built against, which is quite unreliable.
> >
> 
> That's true. Will adding xlat for all of the DRM_IOC constants avoid this?

And adding (fallback) definitions of related structures, yes.


> > > +     PRINT_FIELD_U("", seq, sequence);
> > ... or at least put this into "else" block, just to improve readability a
> > bit.
> 
> There are some other similar parts in the code. Should I use else block
> for all of them or just because this is only one line?

It really depends on the size of the relevant chunks of the code; so,
yes, here it's better because it's only one line.


> > > +             PRINT_FIELD_PTR("{", res, fb_id_ptr);
> > > +             PRINT_FIELD_PTR(", ", res, crtc_id_ptr);
> > > +             PRINT_FIELD_PTR(", ", res, connector_id_ptr);
> > > +             PRINT_FIELD_PTR(", ", res, encoder_id_ptr);
> > Why PRINT_FIELD_PTR if this is a non-mpers ioctl? Should PRINT_FIELD_ADDR
> > or PRINT_FIELD_ADDR64 be used, for example?
> >
> 
> Oh sorry. I didn't notice that there is a mpers_ptr_t type casting in
> PRINT_FIELD_PTR and I just simply use it as its name imply.
> BTW, I think a short comment or description of this PRINT_FIELD_*
> part would make it more clear considering some of them are similar.

Documentation is indeed lacking.


> > Those are arrays of count_* elements (which are R/W, by the way), they
> > likely
> > has to be treated as such.
> >
> 
> Directly print all the array elements, you mean? Any elegant way
> to do this? I mean putting a lot of for loop makes a lot of redundant
> code. Will a macro or function be better?

print_array() (with print_uint32_array_member as element printing
callback, probably) should do the job.


> > > +             PRINT_FIELD_D(", ", plane, src_x);
> > > +             PRINT_FIELD_D(", ", plane, src_y);
> > > +             PRINT_FIELD_D(", ", plane, src_h);
> > > +             PRINT_FIELD_D(", ", plane, src_w);
> > These are unsigned and are in 16.16 fixed point format.
> 
> Well this is new to me. I should search for information.

I think there's an example of treating fixed point numbers somewhere in
the code (my first thought was about V4L2 ioctl decoders), but readily
I can find only progress_1000 field printing in btrfs.c.


> > > +             PRINT_FIELD_U(", ", atomic, reserved);
> > It usually makes sense to print reserved fields only in case they are
> > non-zero.
> 
> OK, padding field is similar right? Will check them all.

Yes, please do.


> > > +             PRINT_FIELD_D(", ", wait, timeout_nsec);
> > If this timeout is absolute, it might make sense to print a time stamp in
> > ISO 8601 format as well.
> 
> This one is also new to me. I will search for info later.

You can probably refer to PRINT_ST_TIME in print_struct_stat.c as an
example, after confirmation that this timeout is indeed related to RTC
and not some other, like CLOCK_MONOTONIC.


> > > +     switch (code) {
> > I'd rather perform dispatch against _IOC_NR(code) if there's nothing
> > preventing that.
> >
> 
> Why? I saw that other decoders use switch(code) directly.
> But it makes sense here because the size of arguments are
> always changing as we've discussed above. If I do so, does
> that mean I have to add _IOC_NR() to every case DRM_IOCTL
> constants? Would that look redundant?

Well, other ioctl decoders expect that argument size does not change
over time; DRM is a bit special here, as its general ioctl dispatching
code is built with the possibility of growing argument over time;
in the case of core DRM ioctls, however, that may be not the case,
so you can ignore that.


> > > +     tprints(", name=");
> > > +     printstrn(tcp, ptr_to_kulong(ver.name), ver.name_len);
> > Why not PRINT_FIELD_STRN?
> >
> 
>  In include/uapi/drm/drm.h, the ver.name field is a char * type. Seems
> in PRINT_FIELD_STRN macro we have to cast char * to kernel_ulong_t
> which is not achievable in this macro. Am I missing anything here?
> Some other comments below are similar.

The argument is in tracee's memory, and has to be casted using
ptr_to_kulong.


> > > +     tprints(", unique=");
> > > +     printstrn(tcp, ptr_to_kulong(unique.unique), unique.unique_len);
> > Why not PRINT_FIELD_STRN?
> >
> > I suspect that a cast to uintptr_t is sufficient here.
> >
> 
> Yes you're right. BTW what's the difference? I mean when
> to use ptr_to_kulong and when casting to uintptr_t?

Hm, disregard that, I confused mpers'ed code with tests code.


> > It could be worth considering incorporating something like
> > print_local_array from "Add support for printing local arrays to
> > print_array" commit from esyr/evdev_decode_bitset branch instead
> > of open-coding it.
> >
> >
> Will this work for all those arrays pointed by *_ptr fields
> in some structures above and below?

*_ptr fields point to arrays into tracee's memory that are not fetched,
so they have to be printed using conventional print_array().


> > > +             tprintf("{width=%u, height=%u, pixel_format=%#x, flags=%u,
> > "
> > It may be worth considering rewriting this using PRINT_FIELDS_*.
> >
> 
> Indeed.
> 
> 
> >
> > "flags" field can contain DRM_MODE_FB_INTERLACED and DRM_MODE_FB_MODIFIERS
> > flags.
> >
> > > +                     "handles=[%u, %u, %u, %u], "
> > > +                     "pitches=[%u, %u, %u, %u], "
> > > +                     "offsets=[%u, %u, %u, %u]",
> > > +                     cmd.width, cmd.height, cmd.pixel_format, cmd.flags,
> >
> > > +                     cmd.handles[0], cmd.handles[1], cmd.handles[2],
> > > +                     cmd.handles[3], cmd.pitches[0], cmd.pitches[1],
> > > +                     cmd.pitches[2], cmd.pitches[3], cmd.offsets[0],
> > > +                     cmd.offsets[1], cmd.offsets[2], cmd.offsets[3]);
> > This part is quite difficult to read.
> >
> >
> I should use for loop for this or perhaps print_local_array?

It's pretty borderline here, I think that re-formatting it like

		tprintf(", handles=[%u, %u, %u, %u]",
			cmd.handles[0], cmd.handles[1],
			cmd.handles[2], cmd.handles[3]);
		tprintf(", pitches=[%u, %u, %u, %u]",
			cmd.pitches[0], cmd.pitches[1],
			cmd.pitches[2], cmd.pitches[3]);
		tprintf(", offsets=[%u, %u, %u, %u]",
			cmd.offsets[0], cmd.offsets[1],
			cmd.offsets[2], cmd.offsets[3]);

(or using a single tprintf routine) would be sufficient.


> > > +#ifdef HAVE_STRUCT_DRM_MODE_FB_CMD2_MODIFIER
> > > +             tprintf(", modifiers=[%" PRIu64 ", %" PRIu64 ", %" PRIu64
> > ", %" PRIu64 "]",
> > > +                     (uint64_t) cmd.modifier[0], (uint64_t)
> > cmd.modifier[1],
> > > +                     (uint64_t) cmd.modifier[2], (uint64_t)
> > cmd.modifier[3]);
> > > +#endif
> > Again, wrapping part of decoding in ifdef's  seems fragile.
> 
> 
> Well, modifier field in struct drm_mode_fb_cmd2 is added in some
> newer version of libdrm. It's not like add xlat for DRM_IOC constants.
> What's your suggestion on handling this?

I think you can refer to bpf or perf_event_open syscall decoders,
structures that are grow over time are common there.  In general,
we bundle the latest and greatest structure variant and then print
only relevant part of the structure based on its size.


> > > diff --git a/ioctl.c b/ioctl.c
> > > index b80292cb..b9aa9833 100644
> > > --- a/ioctl.c
> > > +++ b/ioctl.c
> > > @@ -195,6 +195,10 @@ ioctl_decode_command_number(struct tcb *tcp)
> > >                               return 1;
> > >                       }
> > >                       return 0;
> > > +#if defined(HAVE_DRM_H) || defined(HAVE_DRM_DRM_H)
> > > +             case 'd':
> > > +                     return drm_decode_number(tcp, code);
> > > +#endif
> > I'd prefer having this code unconditional here and put a stub
> > drm_decode_number
> > in drm_ioctl.c, but it's more up to Dmitry.
> >
> >
> Can you explain this part a little bit? I'm not sure I'm following. Since
> the
> whole bulk of code in drm_ioctl.c is ifdefed, would that be a problem if
> this
> part is used unconditionally?

It's just a bit cleaner and contains the ifdef's only to relevant
implementation files, opposing to all call sites.  You can probably
refer to "# else /* !(DM_VERSION_MAJOR == 4) */" branch in dm.c
for an example.  I'd also consult Dmitry in this regard, however,
since, IIRC, he doesn't like that approach.


> > It's also likely worth considering abbreviating output for some
> > informational ioctls.
> >
> 
> You mean something like -v option? I've been thinking about this and
> planing to add support for this in the next patchset.

Yes.