[RFC PATCH] ioctl: decode VFIO_IOMMU_MAP_DMA
Dmitry V. Levin
ldv at altlinux.org
Sat May 8 01:23:36 UTC 2021
On Thu, May 06, 2021 at 05:07:43PM +0000, Alyssa Ross wrote:
> But the main reason I'm submitting this as an RFC is that I'm not sure
> how to handle the 'request' argument of the ioctl() call
> (i.e. VFIO_IOMMU_MAP_DMA). Some of the VFIO ioctl request numbers are
> reused. VFIO_IOMMU_MAP_DMA == VFIO_DEVICE_GET_PCI_HOT_RESET_INFO. To
> tell them apart, we have to check the device type of the file the
> ioctl was sent to. It's easy enough to do that in vfio_ioctl(), as
> I've done here, but that means that the request argument will still
> show up in strace as "VFIO_DEVICE_PCI_HOT_RESET or VFIO_IOMMU_MAP_DMA".
> Ideally, we'd do this check earlier, before printing the request
> argument, so we could figure out which of the two candidate names to
> print. But this doesn't seem to be a problem strace has solved before,
> and it's not at all obvious to me how ioctl.c could be modified to
> accomodate this, in a way where the disambiguated name could be passed
> through ioctl_decode() to vfio_ioctl(). I'd appreciate any direction I
> could be offered on this.
There is a way to override "VFIO_DEVICE_PCI_HOT_RESET or VFIO_IOMMU_MAP_DMA"
output: when ioctl_decode_command_number() return value contains
IOCTL_NUMBER_STOP_LOOKUP, ioctl_lookup() is not called.
The function called by ioctl_decode_command_number() can use e.g.
set_tcb_priv_data() to pass data to the function called by ioctl_decode().
> Another, easier question: should I use a bundled header for this? I
> did that in my initial implementation, just because it felt easiest to
> me at the time. (I've ommitted the content of the header in this
> patch, since that should make reviewing easier, and I don't expect
> this patch to be applied as-is anyway.) I don't really understand
> when it's preferable to let xlat detect values, when fallbacks should
> be given, and when bundled headers should be used.
Bundled headers were introduced recently to avoid portability issues.
I suggest to use bundled headers in the new code.
> @@ -684,6 +683,14 @@ printfd_pid_tracee_ns(struct tcb *tcp, pid_t pid, int fd)
> printfd_pid(tcp, strace_pid, fd);
> +stat_fd_in_proc(pid_t pid, int fd, strace_stat_t *statbuf)
This has to be "struct tcb *" instead of "pid_t" as in getfdproto(), so
that get_proc_pid() could be used to obtain the pid suitable for /proc/.
> + char path;
> + xsprintf(path, "/proc/%d/fd/%d", pid, fd);
Wouldn't it be better to use the expression behind this magic value,
e.g. sizeof("/proc/" STRINGIFY(INT_MIN) "/fd/" STRINGIFY(INT_MIN))?
> + int fd = open("/dev/vfio/vfio", O_RDONLY);
> + if (fd == -1)
> + return 77;
Consider using perror_msg_and_skip("/dev/vfio/vfio").
Consider enabling .git/hooks/pre-commit to avoid trailing whitespace.
More information about the Strace-devel