Re: [PATCH 1/1] linux-user: Add support for SG_IO and SG_GET_VERSION_NUM raw SCSI ioctls

[Filip Bozuta <Filip.Bozuta@syrmia.com>]

On 30.7.20. 04:55, Leif N Huhn wrote:
> This patch implements functionalities of following ioctls:
>
> SG_GET_VERSION_NUM - Returns SG driver version number
>
>      The sg version numbers are of the form "x.y.z" and the single number given
>      by the SG_GET_VERSION_NUM ioctl() is calculated by
>      (x * 10000 + y * 100 + z).
>
> SG_IO - Permits user applications to send SCSI commands to a device
>
>      It is logically equivalent to a write followed by a read.
>
> Implementation notes:
>
>      For SG_GET_VERSION_NUM the value is an int and the implementation is
>      straightforward.
>
>      For SG_IO, the generic thunk mechanism is used, and works correctly when
>      the host and guest architecture have the same pointer size. A special ioctl
>      handler may be needed in other situations and is not covered in this
>      implementation.
>
> Signed-off-by: Leif N Huhn <leif.n.huhn@gmail.com>
> ---
>   linux-user/ioctls.h        |  2 ++
>   linux-user/syscall.c       |  1 +
>   linux-user/syscall_defs.h  | 33 +++++++++++++++++++++++++++++++++
>   linux-user/syscall_types.h |  5 +++++
>   4 files changed, 41 insertions(+)
>
> diff --git a/linux-user/ioctls.h b/linux-user/ioctls.h
> index 0713ae1311..92e2f65e05 100644
> --- a/linux-user/ioctls.h
> +++ b/linux-user/ioctls.h
> @@ -333,6 +333,8 @@
>     IOCTL(CDROM_DRIVE_STATUS, 0, TYPE_NULL)
>     IOCTL(CDROM_DISC_STATUS, 0, TYPE_NULL)
>     IOCTL(CDROMAUDIOBUFSIZ, 0, TYPE_INT)
I think there should be a space between ioctls of different groups (in 
this case CDROM and SG).
> +  IOCTL(SG_GET_VERSION_NUM, 0, TYPE_INT)

SG_GET_VERSION_NUM reads the SG driver version number which means it is of

type IOC_R. The 0 is used only for ioctl types that don't have the third 
argument. Also,

the third argument is a pointer to a 'TYPE_INT' since it is used for 
reading. I tried

your patch with sg_simple1 and I get the different version number with 
native and

cross execution so I think this needs to be corrected. The IOCTL(...) 
definition should be:

         IOCTL(SG_GET_VERSION_NUM, IOC_R, MK_PTR(TYPE_INT))

After this, the 'SG_GET_VERSION_NUM' works fine.

> +  IOCTL(SG_IO, IOC_RW, MK_PTR(MK_STRUCT(STRUCT_sg_io_hdr)))
>   
>   #if 0
>     IOCTL(SNDCTL_COPR_HALT, IOC_RW, MK_PTR(TYPE_INT))
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 945fc25279..d846ef1af2 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -115,6 +115,7 @@
>   #ifdef HAVE_DRM_H
>   #include <libdrm/drm.h>
>   #endif
> +#include <scsi/sg.h>
>   #include "linux_loop.h"
>   #include "uname.h"
>   
> diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
> index 3c261cff0e..0e3004eb31 100644
> --- a/linux-user/syscall_defs.h
> +++ b/linux-user/syscall_defs.h
> @@ -2774,4 +2774,37 @@ struct target_statx {
>      /* 0x100 */
>   };
>   
> +/* from kernel's include/scsi/sg.h */
> +
> +#define TARGET_SG_GET_VERSION_NUM 0x2282 /* Example: version 2.1.34 yields 20134 */
> +/* synchronous SCSI command ioctl, (only in version 3 interface) */
> +#define TARGET_SG_IO 0x2285   /* similar effect as write() followed by read() */
> +
> +struct target_sg_io_hdr
> +{
> +    int interface_id;           /* [i] 'S' for SCSI generic (required) */
> +    int dxfer_direction;        /* [i] data transfer direction  */
> +    unsigned char cmd_len;      /* [i] SCSI command length */
> +    unsigned char mx_sb_len;    /* [i] max length to write to sbp */
> +    unsigned short iovec_count; /* [i] 0 implies no scatter gather */
> +    unsigned int dxfer_len;     /* [i] byte count of data transfer */
> +    abi_ulong    dxferp;	/* [i], [*io] points to data transfer memory
> +					      or scatter gather list */
> +    abi_ulong    cmdp;          /* [i], [*i] points to command to perform */
> +    abi_ulong    sbp;		/* [i], [*o] points to sense_buffer memory */
> +    unsigned int timeout;       /* [i] MAX_UINT->no timeout (unit: millisec) */
> +    unsigned int flags;         /* [i] 0 -> default, see SG_FLAG... */
> +    int pack_id;                /* [i->o] unused internally (normally) */
> +    abi_ulong     usr_ptr;      /* [i->o] unused internally */
> +    unsigned char status;       /* [o] scsi status */
> +    unsigned char masked_status;/* [o] shifted, masked scsi status */
> +    unsigned char msg_status;   /* [o] messaging level data (optional) */
> +    unsigned char sb_len_wr;    /* [o] byte count actually written to sbp */
> +    unsigned short host_status; /* [o] errors from host adapter */
> +    unsigned short driver_status;/* [o] errors from software driver */
> +    int resid;                  /* [o] dxfer_len - actual_transferred */
> +    unsigned int duration;      /* [o] time taken by cmd (unit: millisec) */
> +    unsigned int info;          /* [o] auxiliary information */
> +};  /* 64 bytes long (on i386) */
> +

This target structure is defined, but is not used anywhere. It should be 
used

in a special ioctl handling function for SG_IO to convert the values of 
'sg_io_hdr'

between host and target.

>   #endif
> diff --git a/linux-user/syscall_types.h b/linux-user/syscall_types.h
> index 3f1f033464..3752d217e2 100644
> --- a/linux-user/syscall_types.h
> +++ b/linux-user/syscall_types.h
> @@ -59,6 +59,11 @@ STRUCT(cdrom_read_audio,
>          TYPE_CHAR, TYPE_CHAR, TYPE_CHAR, TYPE_CHAR, TYPE_CHAR, TYPE_INT, TYPE_PTRVOID,
>          TYPE_NULL)
>   
> +STRUCT(sg_io_hdr,
> +       TYPE_INT, TYPE_INT, TYPE_CHAR, TYPE_CHAR, TYPE_SHORT, TYPE_INT, TYPE_PTRVOID,
> +       TYPE_PTRVOID, TYPE_PTRVOID, TYPE_INT, TYPE_INT, TYPE_INT, TYPE_PTRVOID, TYPE_CHAR,
> +       TYPE_CHAR, TYPE_CHAR, TYPE_CHAR, TYPE_SHORT, TYPE_SHORT, TYPE_INT, TYPE_INT, TYPE_INT)
> +

I think a nicer and cleaner way to define the thunk types is to write 
the types in separate lines

followed by a tail comment which describes the field type. Something like:

        STRUCT(sg_io_hdr,

                      TYPE_INT, /* interface_id */

                      TYPE_INT, /* dxfer_direction */

                      TYPE_CHAR, /* cmd_len */

                       ...

This way it is less likely that an issue can ocurr (i.e. to forget a 
field) and it makes the

code more reviewable. You can check out other examples from 
'syscall_types.h' (i.e. snd_timer_id,

loop_info).


Best regards,

Filip

>   STRUCT(hd_geometry,
>          TYPE_CHAR, TYPE_CHAR, TYPE_SHORT, TYPE_ULONG)
>