<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Jun 13, 2019 at 10:38 PM Dmitry V. Levin <<a href="mailto:ldv@altlinux.org">ldv@altlinux.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Thu, Jun 13, 2019 at 12:47:19AM +0800, Zhibin Li wrote:<br>
[...]<br>
> > > +#ifdef HAVE_DRM_H<br>
> > > +# include <drm.h><br>
> > > +#else<br>
> > > +# include <drm/drm.h><br>
> > > +#endif<br>
> > <br>
> > This means that either <drm.h> or <drm/drm.h> is always available.<br>
> > Can we assume this?<br>
> <br>
> I think so, too. But here's what Patrik said about it:<br>
> DRM is a bit tricky in this regard. All userspace applications should go<br>
> through libdrm and never access ioctls directly. Some distributions even<br>
> remove the drm headers from their kernel headers package to prevent any<br>
> misuse.<br>
> Will there be a situation that drm headers is removed while libdrm is<br>
> absent? In such case they are not available. I'm not so sure about this.<br>
<br>
libdrm headers are not necessarily installed, so if kernel headers do not<br>
provide drm headers, then neither <drm.h> nor <drm/drm.h> is available.<br>
<br>
We should handle this situation gracefully, either ifdef the whole bulk<br>
of drm parsers or bail out at configure stage.<br>
<br></blockquote><div>When I try to ifdef the whole bulk of drm parsers, everything goes well in drm.c</div><div>but in drm_mpers.c, I always get errors like:</div><div><br></div><div>mpers-mx32/struct_drm_version.c:7: error: unterminated #if<br> 7 | #if defined(HAVE_DRM_H) || defined(HAVE_DRM_DRM_H)<br> | <br>mpers-m32/struct_drm_version.c:7: error: unterminated #if<br> 7 | #if defined(HAVE_DRM_H) || defined(HAVE_DRM_DRM_H)</div><div><br></div><div>I guess something goes wrong in the mpers code because the last #endif is missing.</div><div>Any clues what might be the cause?</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> > > +<br>
> > > +#define PRINT_IOWR(nr, size, str) \<br>
> > > + tprintf("DRM_IOWR(%#x, %#x) /* %s */", nr, size, str)<br>
> > <br>
> > What's the benefit of making it a macro instead of a function?<br>
> <br>
> I'm not sure if there's any. Any suggestions?<br>
<br>
If there is no benefit of making it a macro, let it be a function.<br>
<br>
> > > +int<br>
> > > +drm_decode_number(struct tcb *const tcp, const unsigned int code)<br>
> > > +{<br>
> > > + const unsigned int nr = _IOC_NR(code);<br>
> > > +<br>
> > > + if (_IOC_DIR(code) == (_IOC_READ | _IOC_WRITE)) {<br>
> > > + switch (nr) {<br>
> > > + case 0xa7:<br>
> > > + if (_IOC_SIZE(code) != 0x50) {<br>
> > > + PRINT_IOWR(nr, _IOC_SIZE(code),<br>
> > > + "DRM_IOCTL_MODE_GETCONNECTOR");<br>
> > > + return IOCTL_NUMBER_STOP_LOOKUP;<br>
> > <br>
> > What is 0x50? Why do we skip lookup for this particular value?<br>
> <br>
> 0x50 is the size of struct drm_mode_get_connector. In v3.12-rc7~26^2~2 a<br>
> u32 padding had been added into the structure, which made its size 0x50.<br>
> That is, in an older version, its size is 0x4c so in this case if we use<br>
> ioctl_lookup, the corresponding item in ioctlent0.h will not be found.<br>
> It's supposed to be { "DRM_IOCTL_MODE_GETCONNECTOR", 0xc05064a7 }, but<br>
> in the case mentioned above, the ioctl code is 0xc04c64a7.<br>
> Actually I've discussed a littel bit about this with esyr and I got some<br>
> sugguestions from him. Details are in [1].<br>
> [1] <a href="https://lists.strace.io/pipermail/strace-devel/2019-April/008710.html" rel="noreferrer" target="_blank">https://lists.strace.io/pipermail/strace-devel/2019-April/008710.html</a><br>
<br>
Then it makes sense to do this just for 0x4c and add an explanatory comment.<br>
<br>
> > > + } else {<br>
> > > + return 0;<br>
> > > + }<br>
> > > + }<br>
> > > + }<br>
> > > +<br>
> > > + return 0;<br>
> > > +}<br>
> > > +<br>
> > > +static int<br>
> > > +drm_set_version(struct tcb *const tcp, long arg)<br>
> > > +{<br>
> > > + struct drm_set_version ver;<br>
> > > +<br>
> > > + if (entering(tcp)) {<br>
> > > + tprints(", ");<br>
> > > + if (umove_or_printaddr(tcp, arg, &ver))<br>
> > > + return RVAL_IOCTL_DECODED;<br>
> > > + PRINT_FIELD_D("write:{", ver, drm_di_major);<br>
> > > + PRINT_FIELD_D(", ", ver, drm_di_minor);<br>
> > > + PRINT_FIELD_D(", ", ver, drm_dd_major);<br>
> > > + PRINT_FIELD_D(", ", ver, drm_dd_minor);<br>
> > > + tprints("}");<br>
> > > +<br>
> > > + return 0;<br>
> > > + }<br>
> > > +<br>
> > > + if (!syserror(tcp) && !umove(tcp, arg, &ver)) {<br>
> > > + PRINT_FIELD_D(", read:{", ver, drm_di_major);<br>
> > > + PRINT_FIELD_D(", ", ver, drm_di_minor);<br>
> > > + PRINT_FIELD_D(", ", ver, drm_dd_major);<br>
> > > + PRINT_FIELD_D(", ", ver, drm_dd_minor);<br>
> > > + tprints("}");<br>
> > > + }<br>
> > <br>
> > Why does this differ from our traditional way of printing read-write<br>
> > structures?<br>
> <br>
> Any references to the traditional way? I got this from Patrik's original<br>
> patch and I didn't notice this is against the tradition.<br>
<br>
git grep '" => '<br>
<br>
> > > +int<br>
> > > +drm_ioctl(struct tcb *const tcp, const unsigned int code,<br>
> > > + const kernel_ulong_t arg)<br>
> > > +{<br>
> > > + switch (code) {<br>
> > > + case DRM_IOCTL_SET_VERSION:<br>
> > > + return drm_set_version(tcp, arg);<br>
> > > + case DRM_IOCTL_GET_MAGIC:<br>
> > > + return drm_get_magic(tcp, arg);<br>
> > > + case DRM_IOCTL_MODE_GETRESOURCES:<br>
> > > + return drm_mode_get_resources(tcp, arg);<br>
> > > + case DRM_IOCTL_MODE_GETCRTC:<br>
> > > + return drm_mode_get_crtc(tcp, arg);<br>
> > > + case DRM_IOCTL_MODE_SETCRTC:<br>
> > > + return drm_mode_set_crtc(tcp, arg);<br>
> > > + case DRM_IOCTL_MODE_CURSOR:<br>
> > > + return drm_mode_cursor(tcp, arg);<br>
> > > + case DRM_IOCTL_MODE_CURSOR2:<br>
> > > + return drm_mode_cursor2(tcp, arg);<br>
> > > + case DRM_IOCTL_MODE_GETGAMMA:<br>
> > > + return drm_mode_get_gamma(tcp, arg);<br>
> > > + case DRM_IOCTL_MODE_SETGAMMA:<br>
> > > + return drm_mode_set_gamma(tcp, arg);<br>
> > > + case DRM_IOCTL_MODE_GETENCODER:<br>
> > > + return drm_mode_get_encoder(tcp, arg);<br>
> > > + case DRM_IOCTL_MODE_GETPROPERTY:<br>
> > > + return drm_mode_get_property(tcp, arg);<br>
> > > + case DRM_IOCTL_MODE_SETPROPERTY:<br>
> > > + return drm_mode_set_property(tcp, arg);<br>
> > > + case DRM_IOCTL_MODE_GETPROPBLOB:<br>
> > > + return drm_mode_get_prop_blob(tcp, arg);<br>
> > > + case DRM_IOCTL_MODE_GETFB:<br>
> > > + return drm_mode_get_fb(tcp, arg);<br>
> > > + case DRM_IOCTL_MODE_ADDFB:<br>
> > > + return drm_mode_add_fb(tcp, arg);<br>
> > > + case DRM_IOCTL_MODE_RMFB:<br>
> > > + return drm_mode_rm_fb(tcp, arg);<br>
> > > + case DRM_IOCTL_MODE_PAGE_FLIP:<br>
> > > + return drm_mode_page_flip(tcp, arg);<br>
> > > + case DRM_IOCTL_MODE_DIRTYFB:<br>
> > > + return drm_mode_dirty_fb(tcp, arg);<br>
> > > + case DRM_IOCTL_MODE_CREATE_DUMB:<br>
> > > + return drm_mode_create_dumb(tcp, arg);<br>
> > > + case DRM_IOCTL_MODE_MAP_DUMB:<br>
> > > + return drm_mode_map_dumb(tcp, arg);<br>
> > > + case DRM_IOCTL_MODE_DESTROY_DUMB:<br>
> > > + return drm_mode_destroy_dumb(tcp, arg);<br>
> > > + case DRM_IOCTL_GEM_CLOSE:<br>
> > > + return drm_gem_close(tcp, arg);<br>
> > > + default: {<br>
> > > + int rc = drm_ioctl_mpers(tcp, code, arg);<br>
> > > + if (rc != RVAL_DECODED)<br>
> > > + return rc;<br>
> > <br>
> > Why RVAL_DECODED is filtered here?<br>
> > <br>
> <br>
> I took v4.22~55 "evdev: move mpers-specific parsers to a separate file"<br>
> as reference. Based on what I've learned about RVAL_* macros, I think<br>
> in this case RVAL_DECODED means the decoding procedure has to be<br>
> continued. Because in drm_mpers.c RVAL_DECODED is returned when no cases<br>
> match the ioctl code.<br>
<br>
In evdev.c multi-number fixed-length commands are handled after<br>
evdev_write_ioctl_mpers invocation, that explains RVAL_DECODED check<br>
there. I don't see anything similar in drm.c and the check for<br>
RVAL_DECODED looks redundant.<br>
<br>
> > > +#include DEF_MPERS_TYPE(struct_drm_version)<br>
> > > +#include DEF_MPERS_TYPE(struct_drm_unique)<br>
> > > +#include DEF_MPERS_TYPE(union_drm_wait_vblank)<br>
> > > +#include DEF_MPERS_TYPE(struct_drm_mode_get_connector)<br>
> > > +#include DEF_MPERS_TYPE(struct_drm_mode_fb_cmd2)<br>
> > <br>
> > Interesting. Are the last two structures really differ between<br>
> > personalities?<br>
> <br>
> As mentioned above, the size of struct drm_mode_get_connector in older<br>
> versions of headers is 0x4c. In the case of 32 bit aligment, the decoder<br>
> will try to fetch a struct of 0x4c while in 64 bit, 0x50 will be<br>
> fetched.<br>
<br>
Shouldn't we just handle both old 0x4c and 0x50 sizes regardless of<br>
personality?<br>
<br>
> As for struct drm_mode_fb_cmd2, I see that in drivers/gpu/drm/drm_ioc32.c<br>
> there is a compat struct drm_mode_fb_cmd232 with __attribute__((packed))<br>
> flag. Shouldn't it be different between personalities?<br>
<br>
Yes, there is a difference indeed.<br>
<br>
<br>
-- <br>
ldv<br>
-- <br>
Strace-devel mailing list<br>
<a href="mailto:Strace-devel@lists.strace.io" target="_blank">Strace-devel@lists.strace.io</a><br>
<a href="https://lists.strace.io/mailman/listinfo/strace-devel" rel="noreferrer" target="_blank">https://lists.strace.io/mailman/listinfo/strace-devel</a><br>
</blockquote></div></div>