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