[Qemu-devel] [PATCH 0/3] fixes for the SCSI disk emulation

[Qemu-devel] [PATCH 0/3] fixes for the SCSI disk emulation

[Rik van Riel]

This small patch series contains several fixes for the
SCSI disk emulation:
- fixes for signed/unsigned overflows
- support for >2TB SCSI disks
- report the correct number of sectors in read capacity

Thanks go out to Paul Brook and Jamie Lokier for reviewing
earlier versions of these patches and pointing additional
TODOs.

-- 
All rights reversed.

[Qemu-devel] [PATCH 1/3] fix signed/unsigned overflows in SCSI disk

[Rik van Riel]

[-- Attachment #1: qemu-upstream.diff --]
[-- Type: text/plain, Size: 1784 bytes --]

Sector numbers can overflow on a virtual scsi disk of over 1TB
in size.  Qemu's bdrv_read expects an int64_t, so fix the overflow
by going to that data type.

On large disks, we clip the capacity to 2TB instead of returning
"capacity modulo 2TB".

Turn sector_count into an unsigned to prevent a signed/unsigned
overflow with SCSI transfers larger than 2TB.  We're unlikely to
ever hit this bug, but fixing it is just one line.

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


Index: qemu/trunk/hw/scsi-disk.c
===================================================================
--- qemu.orig/trunk/hw/scsi-disk.c
+++ qemu/trunk/hw/scsi-disk.c
@@ -47,11 +47,11 @@ do { fprintf(stderr, "scsi-disk: " fmt ,
 typedef struct SCSIRequest {
     SCSIDeviceState *dev;
     uint32_t tag;
-    /* ??? We should probably keep track of whether the data trasfer is
+    /* ??? We should probably keep track of whether the data transfer is
        a read or a write.  Currently we rely on the host getting it right.  */
     /* Both sector and sector_count are in terms of qemu 512 byte blocks.  */
-    int sector;
-    int sector_count;
+    uint64_t sector;
+    uint32_t sector_count;
     /* The amounnt of data in the buffer.  */
     int buf_len;
     uint8_t *dma_buf;
@@ -731,6 +731,9 @@ static int32_t scsi_send_command(SCSIDev
         /* Returned value is the address of the last sector.  */
         if (nb_sectors) {
             nb_sectors--;
+            /* Clip to 2TB, instead of returning capacity modulo 2TB. */
+            if (nb_sectors > UINT32_MAX)
+                nb_sectors = UINT32_MAX;
             outbuf[0] = (nb_sectors >> 24) & 0xff;
             outbuf[1] = (nb_sectors >> 16) & 0xff;
             outbuf[2] = (nb_sectors >> 8) & 0xff;

-- 
All rights reversed.

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

[Rik van Riel]

[-- Attachment #1: qemu-scsi-largedisks.patch --]
[-- Type: text/plain, Size: 4543 bytes --]

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.

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


Index: qemu/trunk/hw/scsi-disk.c
===================================================================
--- qemu.orig/trunk/hw/scsi-disk.c
+++ qemu/trunk/hw/scsi-disk.c
@@ -346,7 +346,7 @@ static int32_t scsi_send_command(SCSIDev
 {
     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 @@ static int32_t scsi_send_command(SCSIDev
     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;
@@ -750,13 +756,15 @@ static int32_t scsi_send_command(SCSIDev
 	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;
@@ -820,6 +828,37 @@ static int32_t scsi_send_command(SCSIDev
         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)

-- 
All rights reversed.

[Qemu-devel] [PATCH 3/3] SCSI divide capacity by s->cluster_size

[Rik van Riel]

[-- Attachment #1: qemu-capacity-sectors.patch --]
[-- Type: text/plain, Size: 1316 bytes --]

Paul Brook pointed out that the number of sectors reported
by the SCSI read capacity commands needs to be divided by
s->cluster_size, because bdrv_get_geometry reports the number
of 512 byte sectors, while emulated CDROMs report 2048 byte
sectors back to the guest.

This has no consequences for emulated hard disks, which use
a cluster size of 1.

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

Index: qemu/trunk/hw/scsi-disk.c
===================================================================
--- qemu.orig/trunk/hw/scsi-disk.c
+++ qemu/trunk/hw/scsi-disk.c
@@ -734,6 +734,7 @@ static int32_t scsi_send_command(SCSIDev
         /* The normal LEN field for this command is zero.  */
 	memset(outbuf, 0, 8);
 	bdrv_get_geometry(s->bdrv, &nb_sectors);
+        nb_sectors =/ s->cluster_size;
         /* Returned value is the address of the last sector.  */
         if (nb_sectors) {
             nb_sectors--;
@@ -834,6 +835,7 @@ static int32_t scsi_send_command(SCSIDev
             DPRINTF("SAI READ CAPACITY(16)\n");
             memset(outbuf, 0, len);
             bdrv_get_geometry(s->bdrv, &nb_sectors);
+            nb_sectors =/ s->cluster_size;
             /* Returned value is the address of the last sector.  */
             if (nb_sectors) {
                 nb_sectors--;

-- 
All rights reversed.

Re: [Qemu-devel] [PATCH 3/3] SCSI divide capacity by s->cluster_size

[Rene Rebe]

Hi,

 > --- qemu.orig/trunk/hw/scsi-disk.c
 > +++ qemu/trunk/hw/scsi-disk.c
 > @@ -734,6 +734,7 @@ static int32_t scsi_send_command(SCSIDev
 >          /* The normal LEN field for this command is zero.  */
 >  	memset(outbuf, 0, 8);
 >  	bdrv_get_geometry(s->bdrv, &nb_sectors);
 > +        nb_sectors =/ s->cluster_size;

/=

 >          /* Returned value is the address of the last sector.  */
 >          if (nb_sectors) {
 >              nb_sectors--;
 > @@ -834,6 +835,7 @@ static int32_t scsi_send_command(SCSIDev
 >              DPRINTF("SAI READ CAPACITY(16)\n");
 >              memset(outbuf, 0, len);
 >              bdrv_get_geometry(s->bdrv, &nb_sectors);
 > +            nb_sectors =/ s->cluster_size;

dito?

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

-- 
   René Rebe - ExactCODE GmbH - Europe, Germany, Berlin
   http://exactcode.de | http://t2-project.org | http://rene.rebe.name

Re: [Qemu-devel] [PATCH 3/3] SCSI divide capacity by s->cluster_size

[Rik van Riel]

Rene Rebe wrote:
> Hi,
> 
>  > --- qemu.orig/trunk/hw/scsi-disk.c
>  > +++ qemu/trunk/hw/scsi-disk.c
>  > @@ -734,6 +734,7 @@ static int32_t scsi_send_command(SCSIDev
>  >          /* The normal LEN field for this command is zero.  */
>  >      memset(outbuf, 0, 8);
>  >      bdrv_get_geometry(s->bdrv, &nb_sectors);
>  > +        nb_sectors =/ s->cluster_size;
> 
> /=

Doh!  That's what I get for trying to make the code a little
nicer after compiling it.

-- 
All rights reversed.

Re: [Qemu-devel] [PATCH 3/3] SCSI divide capacity by s->cluster_size

[Anthony Liguori]

Rik van Riel wrote:
> ndex: qemu/trunk/hw/scsi-disk.c
> ===================================================================
> --- qemu.orig/trunk/hw/scsi-disk.c
> +++ qemu/trunk/hw/scsi-disk.c
> @@ -734,6 +734,7 @@ static int32_t scsi_send_command(SCSIDev
>          /* The normal LEN field for this command is zero.  */
>      memset(outbuf, 0, 8);
>      bdrv_get_geometry(s->bdrv, &nb_sectors);
> +        nb_sectors =/ s->cluster_size;

An emoticon, perhaps, but certainly not a C operator.. :-)

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 0/3] fixes for the SCSI disk emulation

[Anthony Liguori]

Rik van Riel wrote:
> This small patch series contains several fixes for the
> SCSI disk emulation:
> - fixes for signed/unsigned overflows
> - support for >2TB SCSI disks
> - report the correct number of sectors in read capacity
>
> Thanks go out to Paul Brook and Jamie Lokier for reviewing
> earlier versions of these patches and pointing additional
> TODOs.
>
>   
Applied (fixing patch 3).  Thanks.

Regards,

Anthony Liguori