[Qemu-devel] [PATCH] support >2TB SCSI disks

[Qemu-devel] [PATCH] support >2TB SCSI disks

[Rik van Riel]

Implement SCSI READ(16), WRITE(16) and SAI READ CAPACITY(16) commands,
so SCSI disks larger than 2TB can work with guests that support these
newer SCSI commands.

The cast to (uint64_t) is needed because otherwise gcc will use a
signed int, which gets sign extended into uint64_t lba, resulting
in bad block numbers for READ 10 and READ 16 with block numbers
larger than 2^31.

This patch depends on the sector overflow fix patch I mailed yesterday.

Signed-off-by: Rik van Riel <riel@redhat.com>


Index: trunk/hw/scsi-disk.c
===================================================================
--- trunk/hw/scsi-disk.c	(revision 6465)
+++ trunk/hw/scsi-disk.c	(working copy)
@@ -346,7 +346,7 @@
 {
     SCSIDeviceState *s = d->state;
     uint64_t nb_sectors;
-    uint32_t lba;
+    uint64_t lba;
     uint32_t len;
     int cmdlen;
     int is_write;
@@ -368,23 +368,29 @@
     DPRINTF("Command: lun=%d tag=0x%x data=0x%02x", lun, tag, buf[0]);
     switch (command >> 5) {
     case 0:
-        lba = buf[3] | (buf[2] << 8) | ((buf[1] & 0x1f) << 16);
+        lba = (uint64_t) buf[3] | ((uint64_t) buf[2] << 8) |
+              (((uint64_t) buf[1] & 0x1f) << 16);
         len = buf[4];
         cmdlen = 6;
         break;
     case 1:
     case 2:
-        lba = buf[5] | (buf[4] << 8) | (buf[3] << 16) | (buf[2] << 24);
+        lba = (uint64_t) buf[5] | ((uint64_t) buf[4] << 8) |
+              ((uint64_t) buf[3] << 16) | ((uint64_t) buf[2] << 24);
         len = buf[8] | (buf[7] << 8);
         cmdlen = 10;
         break;
     case 4:
-        lba = buf[5] | (buf[4] << 8) | (buf[3] << 16) | (buf[2] << 24);
+        lba = (uint64_t) buf[9] | ((uint64_t) buf[8] << 8) |
+              ((uint64_t) buf[7] << 16) | ((uint64_t) buf[6] << 24) |
+              ((uint64_t) buf[5] << 32) | ((uint64_t) buf[4] << 40) |
+              ((uint64_t) buf[3] << 48) | ((uint64_t) buf[2] << 56);
         len = buf[13] | (buf[12] << 8) | (buf[11] << 16) | (buf[10] << 24);
         cmdlen = 16;
         break;
     case 5:
-        lba = buf[5] | (buf[4] << 8) | (buf[3] << 16) | (buf[2] << 24);
+        lba = (uint64_t) buf[5] | ((uint64_t) buf[4] << 8) |
+              ((uint64_t) buf[3] << 16) | ((uint64_t) buf[2] << 24);
         len = buf[9] | (buf[8] << 8) | (buf[7] << 16) | (buf[6] << 24);
         cmdlen = 12;
         break;
@@ -747,13 +753,15 @@
 	break;
     case 0x08:
     case 0x28:
-        DPRINTF("Read (sector %d, count %d)\n", lba, len);
+    case 0x88:
+        DPRINTF("Read (sector %lld, count %d)\n", lba, len);
         r->sector = lba * s->cluster_size;
         r->sector_count = len * s->cluster_size;
         break;
     case 0x0a:
     case 0x2a:
-        DPRINTF("Write (sector %d, count %d)\n", lba, len);
+    case 0x8a:
+        DPRINTF("Write (sector %lld, count %d)\n", lba, len);
         r->sector = lba * s->cluster_size;
         r->sector_count = len * s->cluster_size;
         is_write = 1;
@@ -817,6 +825,37 @@
         if (buf[1] & 3)
             goto fail;
         break;
+    case 0x9e:
+        /* Service Action In subcommands. */
+        if ((buf[1] & 31) == 0x10) {
+            DPRINTF("SAI READ CAPACITY(16)\n");
+            memset(outbuf, 0, len);
+            bdrv_get_geometry(s->bdrv, &nb_sectors);
+            /* Returned value is the address of the last sector.  */
+            if (nb_sectors) {
+                nb_sectors--;
+                outbuf[0] = (nb_sectors >> 56) & 0xff;
+                outbuf[1] = (nb_sectors >> 48) & 0xff;
+                outbuf[2] = (nb_sectors >> 40) & 0xff;
+                outbuf[3] = (nb_sectors >> 32) & 0xff;
+                outbuf[4] = (nb_sectors >> 24) & 0xff;
+                outbuf[5] = (nb_sectors >> 16) & 0xff;
+                outbuf[6] = (nb_sectors >> 8) & 0xff;
+                outbuf[7] = nb_sectors & 0xff;
+                outbuf[8] = 0;
+                outbuf[9] = 0;
+                outbuf[10] = s->cluster_size * 2;
+                outbuf[11] = 0;
+                /* Protection, exponent and lowest lba field left blank. */
+                r->buf_len = len;
+            } else {
+                scsi_command_complete(r, STATUS_CHECK_CONDITION, SENSE_NOT_READY);
+                return 0;
+            }
+            break;
+        }
+        DPRINTF("Unsupported Service Action In\n");
+        goto fail;
     case 0xa0:
         DPRINTF("Report LUNs (len %d)\n", len);
         if (len < 16)

Re: [Qemu-devel] [PATCH] support >2TB SCSI disks

[Paul Brook]

>      case 0:
> -        lba = buf[3] | (buf[2] << 8) | ((buf[1] & 0x1f) << 16);
> +        lba = (uint64_t) buf[3] | ((uint64_t) buf[2] << 8) |
> +              (((uint64_t) buf[1] & 0x1f) << 16);

This is not required, though I guess it's harmless.

>      case 4:
>...
>          len = buf[13] | (buf[12] << 8) | (buf[11] << 16) | (buf[10] << 24);
>...
> +    case 0x88:
>          r->sector_count = len * s->cluster_size;

Implementing these commands introduces several overflows. There are several 
places (including SCSIRequest->sector_count and the return value from 
scsi_send_comand) that assume the transfer length fits in a signed (32-bit) 
int.

We should to implement the Block Limits VPD page, and enforce these limits.

> +            /* Returned value is the address of the last sector.  */
> +            if (nb_sectors) {
> +                nb_sectors--;

By my reading both this and the current Read Capacity(10)  are incorrect.
They need to divide by s->cluster_size.

Paul

Re: [Qemu-devel] [PATCH] support >2TB SCSI disks

[Rik van Riel]

Paul Brook wrote:
>>      case 0:
>> -        lba = buf[3] | (buf[2] << 8) | ((buf[1] & 0x1f) << 16);
>> +        lba = (uint64_t) buf[3] | ((uint64_t) buf[2] << 8) |
>> +              (((uint64_t) buf[1] & 0x1f) << 16);
> 
> This is not required, though I guess it's harmless.

I thought I'd keep them all consistent :)

>>      case 4:
>> ...
>>          len = buf[13] | (buf[12] << 8) | (buf[11] << 16) | (buf[10] << 24);
>> ...
>> +    case 0x88:
>>          r->sector_count = len * s->cluster_size;
> 
> Implementing these commands introduces several overflows. There are several 
> places (including SCSIRequest->sector_count and the return value from 
> scsi_send_comand) that assume the transfer length fits in a signed (32-bit) 
> int.

True, a SCSI transfer of more than 2GB would cause an overflow.

>> +            /* Returned value is the address of the last sector.  */
>> +            if (nb_sectors) {
>> +                nb_sectors--;
> 
> By my reading both this and the current Read Capacity(10)  are incorrect.
> They need to divide by s->cluster_size.

Good point.  Want me to send in a separate patch that does that?

-- 
All rights reversed.

Re: [Qemu-devel] [PATCH] support >2TB SCSI disks

[M. Warner Losh]

In message: <200901281230.29455.paul@codesourcery.com>
            Paul Brook <paul@codesourcery.com> writes:
: >      case 0:
: > -        lba = buf[3] | (buf[2] << 8) | ((buf[1] & 0x1f) << 16);
: > +        lba = (uint64_t) buf[3] | ((uint64_t) buf[2] << 8) |
: > +              (((uint64_t) buf[1] & 0x1f) << 16);
: 
: This is not required, though I guess it's harmless.

Actually, I think it is required.  ANSI-C promotion rules say that a
char or unsigned char is promoted to an int when used in an
expression.  This causes the result to be a 32-bit number which is
sign-extended to a 64-bit number before being assigned to lba.

I've hit this "bug" in C before many times, and the patch specifically
called it out as a problem.

Warner

Re: [Qemu-devel] [PATCH] support >2TB SCSI disks

[Paul Brook]

> : >      case 0:
> : > -        lba = buf[3] | (buf[2] << 8) | ((buf[1] & 0x1f) << 16);
> : > +        lba = (uint64_t) buf[3] | ((uint64_t) buf[2] << 8) |
> : > +              (((uint64_t) buf[1] & 0x1f) << 16);
> :
> : This is not required, though I guess it's harmless.
>
> Actually, I think it is required.  ANSI-C promotion rules say that a
> char or unsigned char is promoted to an int when used in an
> expression.  This causes the result to be a 32-bit number which is
> sign-extended to a 64-bit number before being assigned to lba.

Read the code again. You'll notice that the largest possible value is 
0x001fffff, so it doesn't matter which way you extend from 32 to 64 bits.

Paul