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

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

[Leif N Huhn]

Hi. This is my first time trying to contribute to qemu. This patch
works correctly for architectures with the same bit-width, for example
32bit arm host and i386 user binary. Here is an example with the sg_simple2
executable from https://github.com/hreinecke/sg3_utils

32-bit ARM native:

  strace -e trace=ioctl ./sg_simple2 /dev/sg0
  ioctl(3, SG_GET_VERSION_NUM, [30536])   = 0
  ioctl(3, SG_IO, {interface_id='S', dxfer_direction=SG_DXFER_FROM_DEV, cmd_len=6, cmdp="\x12\x00\x00\x00\x60\x00", mx_sb_len=32, iovec_count=0, dxfer_len=96, timeout=20000, flags=0, dxferp="\x05\x80\x00\x32\x5b\x00\x00\x00\x48\x4c\x2d\x44\x54\x2d\x53\x54\x42\x44\x2d\x52\x45\x20\x20\x57\x48\x31\x36\x4e\x53\x34\x30\x20"..., status=0, masked_status=0, msg_status=0, sb_len_wr=0, sbp="", host_status=0, driver_status=0, resid=0, duration=3, info=0}) = 0
  Some of the INQUIRY command's results:
      HL-DT-ST  BD-RE  WH16NS40   1.05  [wide=0 sync=0 cmdque=0 sftre=0]
  ioctl(3, SG_IO, {interface_id='S', dxfer_direction=SG_DXFER_NONE, cmd_len=6, cmdp="\x00\x00\x00\x00\x00\x00", mx_sb_len=32, iovec_count=0, dxfer_len=0, timeout=20000, flags=0, status=0, masked_status=0, msg_status=0, sb_len_wr=0, sbp="", host_status=0, driver_status=0, resid=0, duration=4, info=0}) = 0
  Test Unit Ready successful so unit is ready!
  +++ exited with 0 +++

i386 binary on 32-bit arm host:

  strace -f -e trace=ioctl qemu/build/i386-linux-user/qemu-i386 sg3_utils/examples/sg_simple2 /dev/sg0
  strace: Process 690 attached
  [pid   689] ioctl(3, SG_GET_VERSION_NUM, [30536]) = 0
  [pid   689] ioctl(3, SG_IO, {interface_id='S', dxfer_direction=SG_DXFER_FROM_DEV, cmd_len=6, cmdp="\x12\x00\x00\x00\x60\x00", mx_sb_len=32, iovec_count=0, dxfer_len=96, timeout=20000, flags=0, dxferp="\x05\x80\x00\x32\x5b\x00\x00\x00\x48\x4c\x2d\x44\x54\x2d\x53\x54\x42\x44\x2d\x52\x45\x20\x20\x57\x48\x31\x36\x4e\x53\x34\x30\x20"..., status=0, masked_status=0, msg_status=0, sb_len_wr=0, sbp="", host_status=0, driver_status=0, resid=0, duration=3, info=0}) = 0
  Some of the INQUIRY command's results:
      HL-DT-ST  BD-RE  WH16NS40   1.05  [wide=0 sync=0 cmdque=0 sftre=0]
  [pid   689] ioctl(3, SG_IO, {interface_id='S', dxfer_direction=SG_DXFER_NONE, cmd_len=6, cmdp="\x00\x00\x00\x00\x00\x00", mx_sb_len=32, iovec_count=0, dxfer_len=0, timeout=20000, flags=0, status=0, masked_status=0, msg_status=0, sb_len_wr=0, sbp="", host_status=0, driver_status=0, resid=0, duration=3, info=0}) = 0
  Test Unit Ready successful so unit is ready!
  [pid   690] +++ exited with 0 +++
  +++ exited with 0 +++

However when I try i386 guest on x86_64 host, the cmdp bytes in the
first SG_IO call are zero, incorrectly. I assume that is because I need
to write a special ioctl handler. Is that correct? Should I be calling
lock_user(VERIFY_WRITE...) to copy the buffers over?

Also, is the current patch acceptable as is, or does it need to be
reworked until the ioctl works with different architecture bit-widths?

Thanks!

Leif N Huhn (1):
  linux-user: Add support for SG_IO and SG_GET_VERSION_NUM raw SCSI
    ioctls

 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(+)

-- 
2.28.0

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

[Leif N Huhn]

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)
+  IOCTL(SG_GET_VERSION_NUM, 0, TYPE_INT)
+  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) */
+
 #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)
+
 STRUCT(hd_geometry,
        TYPE_CHAR, TYPE_CHAR, TYPE_SHORT, TYPE_ULONG)
 
-- 
2.28.0

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

[Filip Bozuta]

Hello Leif,

On 30.7.20. 04:55, Leif N Huhn wrote:
> Hi. This is my first time trying to contribute to qemu. This patch
> works correctly for architectures with the same bit-width, for example
> 32bit arm host and i386 user binary. Here is an example with the sg_simple2
> executable from https://github.com/hreinecke/sg3_utils
>
> 32-bit ARM native:
>
>    strace -e trace=ioctl ./sg_simple2 /dev/sg0
>    ioctl(3, SG_GET_VERSION_NUM, [30536])   = 0
>    ioctl(3, SG_IO, {interface_id='S', dxfer_direction=SG_DXFER_FROM_DEV, cmd_len=6, cmdp="\x12\x00\x00\x00\x60\x00", mx_sb_len=32, iovec_count=0, dxfer_len=96, timeout=20000, flags=0, dxferp="\x05\x80\x00\x32\x5b\x00\x00\x00\x48\x4c\x2d\x44\x54\x2d\x53\x54\x42\x44\x2d\x52\x45\x20\x20\x57\x48\x31\x36\x4e\x53\x34\x30\x20"..., status=0, masked_status=0, msg_status=0, sb_len_wr=0, sbp="", host_status=0, driver_status=0, resid=0, duration=3, info=0}) = 0
>    Some of the INQUIRY command's results:
>        HL-DT-ST  BD-RE  WH16NS40   1.05  [wide=0 sync=0 cmdque=0 sftre=0]
>    ioctl(3, SG_IO, {interface_id='S', dxfer_direction=SG_DXFER_NONE, cmd_len=6, cmdp="\x00\x00\x00\x00\x00\x00", mx_sb_len=32, iovec_count=0, dxfer_len=0, timeout=20000, flags=0, status=0, masked_status=0, msg_status=0, sb_len_wr=0, sbp="", host_status=0, driver_status=0, resid=0, duration=4, info=0}) = 0
>    Test Unit Ready successful so unit is ready!
>    +++ exited with 0 +++
>
> i386 binary on 32-bit arm host:
>
>    strace -f -e trace=ioctl qemu/build/i386-linux-user/qemu-i386 sg3_utils/examples/sg_simple2 /dev/sg0
>    strace: Process 690 attached
>    [pid   689] ioctl(3, SG_GET_VERSION_NUM, [30536]) = 0
>    [pid   689] ioctl(3, SG_IO, {interface_id='S', dxfer_direction=SG_DXFER_FROM_DEV, cmd_len=6, cmdp="\x12\x00\x00\x00\x60\x00", mx_sb_len=32, iovec_count=0, dxfer_len=96, timeout=20000, flags=0, dxferp="\x05\x80\x00\x32\x5b\x00\x00\x00\x48\x4c\x2d\x44\x54\x2d\x53\x54\x42\x44\x2d\x52\x45\x20\x20\x57\x48\x31\x36\x4e\x53\x34\x30\x20"..., status=0, masked_status=0, msg_status=0, sb_len_wr=0, sbp="", host_status=0, driver_status=0, resid=0, duration=3, info=0}) = 0
>    Some of the INQUIRY command's results:
>        HL-DT-ST  BD-RE  WH16NS40   1.05  [wide=0 sync=0 cmdque=0 sftre=0]
>    [pid   689] ioctl(3, SG_IO, {interface_id='S', dxfer_direction=SG_DXFER_NONE, cmd_len=6, cmdp="\x00\x00\x00\x00\x00\x00", mx_sb_len=32, iovec_count=0, dxfer_len=0, timeout=20000, flags=0, status=0, masked_status=0, msg_status=0, sb_len_wr=0, sbp="", host_status=0, driver_status=0, resid=0, duration=3, info=0}) = 0
>    Test Unit Ready successful so unit is ready!
>    [pid   690] +++ exited with 0 +++
>    +++ exited with 0 +++
>
> However when I try i386 guest on x86_64 host, the cmdp bytes in the
> first SG_IO call are zero, incorrectly. I assume that is because I need
> to write a special ioctl handler. Is that correct? Should I be calling
> lock_user(VERIFY_WRITE...) to copy the buffers over?

Yes I think you need a special ioctl handler for SG_IO. I also tried 
your patch by printing the

cmdp as pointer in hex format and noticed that it doesn't change after 
each cross execution

through QEMU as it should (which is the case for native execution) so 
there is definitely a problem there.

Check out IOCTL_SPECIAL() in file 'ioctls.h'. It enables defining a 
special ioctl handling

function which is called specifically for that ioctl in cross execution.

>
> Also, is the current patch acceptable as is, or does it need to be
> reworked until the ioctl works with different architecture bit-widths?

No, patches are usually not accepted until they work for all targets. 
There is also a little

issue with SG_GET_VERSION_NUM. I will write you a review in your patch 
so you can

see.


By the way, I like the form of your patch description.


Best regards,

Filip

>
> Thanks!
>
> Leif N Huhn (1):
>    linux-user: Add support for SG_IO and SG_GET_VERSION_NUM raw SCSI
>      ioctls
>
>   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(+)
>

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

[Filip Bozuta]

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)
>