From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([208.118.235.92]:53210)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <pbonzini@redhat.com>) id 1SwVRa-0000yX-Qk
	for qemu-devel@nongnu.org; Wed, 01 Aug 2012 05:42:24 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <pbonzini@redhat.com>) id 1SwVRZ-0007nZ-P4
	for qemu-devel@nongnu.org; Wed, 01 Aug 2012 05:42:18 -0400
Received: from mx1.redhat.com ([209.132.183.28]:18685)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <pbonzini@redhat.com>) id 1SwVRZ-0007nR-FX
	for qemu-devel@nongnu.org; Wed, 01 Aug 2012 05:42:17 -0400
Message-ID: <5018F9F0.9020206@redhat.com>
Date: Wed, 01 Aug 2012 11:42:08 +0200
From: Paolo Bonzini <pbonzini@redhat.com>
MIME-Version: 1.0
References: <1343811943-3972-1-git-send-email-mc@linux.vnet.ibm.com>
In-Reply-To: <1343811943-3972-1-git-send-email-mc@linux.vnet.ibm.com>
Content-Type: text/plain; charset=ISO-8859-15
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH v2] add suport for ATA_PASSTHROUGH_xx scsi
	command
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Cong Meng <mc@linux.vnet.ibm.com>
Cc: zwanp@cn.ibm.com, linuxram@us.ibm.com, stefanha@linux.vnet.ibm.com, qemu-devel@nongnu.org

Il 01/08/2012 11:05, Cong Meng ha scritto:
> +    case ATA_PASSTHROUGH_12:
> +        if (dev->type != TYPE_ROM) {
> +            if ((buf[2] & 0x3) == 2) {
> +                cmd->xfer = buf[4] * dev->blocksize;
> +            }
> +        }
> +        break;
> +    case ATA_PASSTHROUGH_16:
> +        if ((buf[2] & 0x3) == 2) {
> +            cmd->xfer = ((buf[5] << 8) | buf[6]) * dev->blocksize;
> +        }
> +        break;

Hmm, I think you're only handling this partially.

Four bits of buf[2] count; bits 0..1 are T_LENGTH, bit 2 is BYTE_BLOCK,
bit 4 is T_TYPE:

If buf[2] is xxxxxx00, cmd->xfer = 0

else

   if buf[2] is xxxxx0xx, xfer_unit = 1
   else if buf[2] is xxx0x1xx, xfer_unit = 512
   else xfer_unit = dev->blocksize (this is when buf[2] is xxx1x1xx)

   if buf[2] is xxxxxx01, set cmd->xfer to the FEATURES field
   if buf[2] is xxxxxx10, set cmd->xfer to the SECTOR_COUNT

   for ATA_PASSTHROUGH_16, if buf[1] bit 0 is 0, then cmd->xfer &= 255;

   cmd->xfer *= xfer_unit;

Also we cannot support buf[2] is xxxxxx11.  Please add a check to
hw/scsi-generic.c, so that the request is failed in this case.

This is better encapsulated in a separate function, of course.

On top of this, the direction is not necessarily TO_DEV (as in the
current code for scsi_cmd_xfer_mode).  It is TO_DEV if buf[2] bit 3
(T_DIR) is zero; it is FROM_DEV if buf[2] bit 3 is one.

Do you have a copy of the SAT (SCSI/ATA translation) standard?  This is
all in paragraph 12.2.2.2 in my copy.

Paolo