[Qemu-devel] [PATCH v1 0/2] Reporting of rotation rate for disks

[Qemu-devel] [PATCH v1 0/2] Reporting of rotation rate for disks

[Daniel P. Berrange]

While looking at libvirt tagged questions on serverfault I saw
someone ask how to indicate that a virtual disk is an SSD rather
than rotating rust.

https://serverfault.com/questions/876467/how-to-add-virtual-storage-as-ssd-in-kvm

IIUC, IDE / SCSI don't really have an explicit concept of a
disk being SSD vs HDD, but they do have a means of reporting
the disk rotation rate and both specify that SSDs should
report RPM==1.

Linux uses this to determine whether to set the 'rotational'
property to 0 for the I/O queue, and also whether to allow
the disk to contribute random entropy (only HDDs contribute).

This felt like something QEMU ought to allow the mgmt application
to set based on the storage it is using to back the virtual block
devices. So this series adds a 'rotation_rate' property to the
SCSI and IDE disks, taking an RPM value per their respective
specifications.

There is no mechanism to report this information to virtio-blk.
We could perhaps argue that people should use virtio-scsi instead,
because fixing virtio-blk would require enhancement to both QEMU
and Linux virtio-blk drivers (and other guest OS drivers too)

I'm unclear if there is anything I should have done wrt the
device migration vmstate when adding this extra field, or if it
is safe to just expect the mgmt app to set the property correctly
on src+dst ?

Daniel P. Berrange (2):
  scsi-disk: support reporting of rotation rate
  ide: support reporting of rotation rate

 hw/ide/core.c             |  1 +
 hw/ide/qdev.c             |  1 +
 hw/scsi/scsi-disk.c       | 20 ++++++++++++++++++++
 include/hw/ide/internal.h |  8 ++++++++
 4 files changed, 30 insertions(+)

-- 
2.13.5

[Qemu-devel] [PATCH v1 1/2] scsi-disk: support reporting of rotation rate

[Daniel P. Berrange]

The Linux kernel will query the SCSI "Block device characteristics"
VPD to determine the rotations per minute of the disk. If this has
the value 1, it is taken to be an SSD and so Linux sets the
'rotational' flag to 0 for the I/O queue and will stop using that
disk as a source of random entropy. Other operating systems may
also take into account rotation rate when setting up default
behaviour.

Mgmt apps should be able to set the rotation rate for virtualized
block devices, based on characteristics of the host storage in use,
so that the guest OS gets sensible behaviour out of the box. This
patch thus adds a 'rotation-rate' parameter for 'scsi-hd' and
'scsi-block' device types. For the latter, this parameter will be
ignored unless the host device has TYPE_DISK.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 hw/scsi/scsi-disk.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 6e841fb5ff..a518080e7d 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -104,6 +104,14 @@ typedef struct SCSIDiskState
     char *product;
     bool tray_open;
     bool tray_locked;
+    /*
+     * 0x0000        - rotation rate not reported
+     * 0x0001        - non-rotating medium (SSD)
+     * 0x0002-0x0400 - reserved
+     * 0x0401-0xffe  - rotations per minute
+     * 0xffff        - reserved
+     */
+    uint16_t rotation_rate;
 } SCSIDiskState;
 
 static bool scsi_handle_rw_error(SCSIDiskReq *r, int error, bool acct_failed);
@@ -605,6 +613,7 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
             outbuf[buflen++] = 0x83; // device identification
             if (s->qdev.type == TYPE_DISK) {
                 outbuf[buflen++] = 0xb0; // block limits
+                outbuf[buflen++] = 0xb1; /* block device characteristics */
                 outbuf[buflen++] = 0xb2; // thin provisioning
             }
             break;
@@ -747,6 +756,15 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
             outbuf[43] = max_io_sectors & 0xff;
             break;
         }
+        case 0xb1: /* block device characteristics */
+        {
+            buflen = 8;
+            outbuf[4] = (s->rotation_rate >> 8) & 0xff;
+            outbuf[5] = s->rotation_rate & 0xff;
+            outbuf[6] = 0;
+            outbuf[7] = 0;
+            break;
+        }
         case 0xb2: /* thin provisioning */
         {
             buflen = 8;
@@ -2911,6 +2929,7 @@ static Property scsi_hd_properties[] = {
                        DEFAULT_MAX_UNMAP_SIZE),
     DEFINE_PROP_UINT64("max_io_size", SCSIDiskState, max_io_size,
                        DEFAULT_MAX_IO_SIZE),
+    DEFINE_PROP_UINT16("rotation_rate", SCSIDiskState, rotation_rate, 0),
     DEFINE_BLOCK_CHS_PROPERTIES(SCSIDiskState, qdev.conf),
     DEFINE_PROP_END_OF_LIST(),
 };
@@ -2982,6 +3001,7 @@ static const TypeInfo scsi_cd_info = {
 static Property scsi_block_properties[] = {
     DEFINE_BLOCK_ERROR_PROPERTIES(SCSIDiskState, qdev.conf),         \
     DEFINE_PROP_DRIVE("drive", SCSIDiskState, qdev.conf.blk),
+    DEFINE_PROP_UINT16("rotation_rate", SCSIDiskState, rotation_rate, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.13.5

Re: [Qemu-devel] [PATCH v1 1/2] scsi-disk: support reporting of rotation rate

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2129 bytes --]

On 10/04/2017 06:40 AM, Daniel P. Berrange wrote:
> The Linux kernel will query the SCSI "Block device characteristics"
> VPD to determine the rotations per minute of the disk. If this has
> the value 1, it is taken to be an SSD and so Linux sets the
> 'rotational' flag to 0 for the I/O queue and will stop using that
> disk as a source of random entropy. Other operating systems may
> also take into account rotation rate when setting up default
> behaviour.
> 
> Mgmt apps should be able to set the rotation rate for virtualized
> block devices, based on characteristics of the host storage in use,
> so that the guest OS gets sensible behaviour out of the box. This
> patch thus adds a 'rotation-rate' parameter for 'scsi-hd' and
> 'scsi-block' device types. For the latter, this parameter will be
> ignored unless the host device has TYPE_DISK.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  hw/scsi/scsi-disk.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)

>      bool tray_locked;
> +    /*
> +     * 0x0000        - rotation rate not reported
> +     * 0x0001        - non-rotating medium (SSD)
> +     * 0x0002-0x0400 - reserved
> +     * 0x0401-0xffe  - rotations per minute

s/0xffe/0xfffe/

> +     * 0xffff        - reserved
> +     */
> +    uint16_t rotation_rate;
>  } SCSIDiskState;
>  
>  static bool scsi_handle_rw_error(SCSIDiskReq *r, int error, bool acct_failed);
> @@ -605,6 +613,7 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
>              outbuf[buflen++] = 0x83; // device identification
>              if (s->qdev.type == TYPE_DISK) {
>                  outbuf[buflen++] = 0xb0; // block limits
> +                outbuf[buflen++] = 0xb1; /* block device characteristics */

This function is awkward - it is a non-local audit to see whether we are
 at risk of overflowing any buffers due to the new output.  But from
what I can see, I think you're safe.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v1 1/2] scsi-disk: support reporting of rotation rate

[Dr. David Alan Gilbert]

* Daniel P. Berrange (berrange@redhat.com) wrote:
> The Linux kernel will query the SCSI "Block device characteristics"
> VPD to determine the rotations per minute of the disk. If this has
> the value 1, it is taken to be an SSD and so Linux sets the
> 'rotational' flag to 0 for the I/O queue and will stop using that
> disk as a source of random entropy. Other operating systems may
> also take into account rotation rate when setting up default
> behaviour.
> 
> Mgmt apps should be able to set the rotation rate for virtualized
> block devices, based on characteristics of the host storage in use,
> so that the guest OS gets sensible behaviour out of the box. This
> patch thus adds a 'rotation-rate' parameter for 'scsi-hd' and
> 'scsi-block' device types. For the latter, this parameter will be
> ignored unless the host device has TYPE_DISK.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  hw/scsi/scsi-disk.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index 6e841fb5ff..a518080e7d 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -104,6 +104,14 @@ typedef struct SCSIDiskState
>      char *product;
>      bool tray_open;
>      bool tray_locked;
> +    /*
> +     * 0x0000        - rotation rate not reported
> +     * 0x0001        - non-rotating medium (SSD)
> +     * 0x0002-0x0400 - reserved
> +     * 0x0401-0xffe  - rotations per minute
> +     * 0xffff        - reserved
> +     */
> +    uint16_t rotation_rate;
>  } SCSIDiskState;
>  
>  static bool scsi_handle_rw_error(SCSIDiskReq *r, int error, bool acct_failed);
> @@ -605,6 +613,7 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
>              outbuf[buflen++] = 0x83; // device identification
>              if (s->qdev.type == TYPE_DISK) {
>                  outbuf[buflen++] = 0xb0; // block limits
> +                outbuf[buflen++] = 0xb1; /* block device characteristics */
>                  outbuf[buflen++] = 0xb2; // thin provisioning
>              }
>              break;
> @@ -747,6 +756,15 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
>              outbuf[43] = max_io_sectors & 0xff;
>              break;
>          }
> +        case 0xb1: /* block device characteristics */
> +        {
> +            buflen = 8;
> +            outbuf[4] = (s->rotation_rate >> 8) & 0xff;
> +            outbuf[5] = s->rotation_rate & 0xff;
> +            outbuf[6] = 0;
> +            outbuf[7] = 0;
> +            break;
> +        }
>          case 0xb2: /* thin provisioning */
>          {
>              buflen = 8;
> @@ -2911,6 +2929,7 @@ static Property scsi_hd_properties[] = {
>                         DEFAULT_MAX_UNMAP_SIZE),
>      DEFINE_PROP_UINT64("max_io_size", SCSIDiskState, max_io_size,
>                         DEFAULT_MAX_IO_SIZE),
> +    DEFINE_PROP_UINT16("rotation_rate", SCSIDiskState, rotation_rate, 0),
>      DEFINE_BLOCK_CHS_PROPERTIES(SCSIDiskState, qdev.conf),
>      DEFINE_PROP_END_OF_LIST(),
>  };
> @@ -2982,6 +3001,7 @@ static const TypeInfo scsi_cd_info = {
>  static Property scsi_block_properties[] = {
>      DEFINE_BLOCK_ERROR_PROPERTIES(SCSIDiskState, qdev.conf),         \
>      DEFINE_PROP_DRIVE("drive", SCSIDiskState, qdev.conf.blk),
> +    DEFINE_PROP_UINT16("rotation_rate", SCSIDiskState, rotation_rate, 0),
>      DEFINE_PROP_END_OF_LIST(),
>  };

Are you sure you want this as a property of SCSIDiskState etc rather
than a property on the underlying drive?
Then if virtio devices etc wanted to pick this up later they could
without needing extra parameters.

Dave

>  
> -- 
> 2.13.5
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v1 1/2] scsi-disk: support reporting of rotation rate

[Daniel P. Berrange]

On Wed, Oct 04, 2017 at 05:49:44PM +0100, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrange (berrange@redhat.com) wrote:
> > The Linux kernel will query the SCSI "Block device characteristics"
> > VPD to determine the rotations per minute of the disk. If this has
> > the value 1, it is taken to be an SSD and so Linux sets the
> > 'rotational' flag to 0 for the I/O queue and will stop using that
> > disk as a source of random entropy. Other operating systems may
> > also take into account rotation rate when setting up default
> > behaviour.
> > 
> > Mgmt apps should be able to set the rotation rate for virtualized
> > block devices, based on characteristics of the host storage in use,
> > so that the guest OS gets sensible behaviour out of the box. This
> > patch thus adds a 'rotation-rate' parameter for 'scsi-hd' and
> > 'scsi-block' device types. For the latter, this parameter will be
> > ignored unless the host device has TYPE_DISK.
> > 
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> >  hw/scsi/scsi-disk.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> > 
> > diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> > index 6e841fb5ff..a518080e7d 100644
> > --- a/hw/scsi/scsi-disk.c
> > +++ b/hw/scsi/scsi-disk.c
> > @@ -104,6 +104,14 @@ typedef struct SCSIDiskState
> >      char *product;
> >      bool tray_open;
> >      bool tray_locked;
> > +    /*
> > +     * 0x0000        - rotation rate not reported
> > +     * 0x0001        - non-rotating medium (SSD)
> > +     * 0x0002-0x0400 - reserved
> > +     * 0x0401-0xffe  - rotations per minute
> > +     * 0xffff        - reserved
> > +     */
> > +    uint16_t rotation_rate;
> >  } SCSIDiskState;
> >  
> >  static bool scsi_handle_rw_error(SCSIDiskReq *r, int error, bool acct_failed);
> > @@ -605,6 +613,7 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
> >              outbuf[buflen++] = 0x83; // device identification
> >              if (s->qdev.type == TYPE_DISK) {
> >                  outbuf[buflen++] = 0xb0; // block limits
> > +                outbuf[buflen++] = 0xb1; /* block device characteristics */
> >                  outbuf[buflen++] = 0xb2; // thin provisioning
> >              }
> >              break;
> > @@ -747,6 +756,15 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
> >              outbuf[43] = max_io_sectors & 0xff;
> >              break;
> >          }
> > +        case 0xb1: /* block device characteristics */
> > +        {
> > +            buflen = 8;
> > +            outbuf[4] = (s->rotation_rate >> 8) & 0xff;
> > +            outbuf[5] = s->rotation_rate & 0xff;
> > +            outbuf[6] = 0;
> > +            outbuf[7] = 0;
> > +            break;
> > +        }
> >          case 0xb2: /* thin provisioning */
> >          {
> >              buflen = 8;
> > @@ -2911,6 +2929,7 @@ static Property scsi_hd_properties[] = {
> >                         DEFAULT_MAX_UNMAP_SIZE),
> >      DEFINE_PROP_UINT64("max_io_size", SCSIDiskState, max_io_size,
> >                         DEFAULT_MAX_IO_SIZE),
> > +    DEFINE_PROP_UINT16("rotation_rate", SCSIDiskState, rotation_rate, 0),
> >      DEFINE_BLOCK_CHS_PROPERTIES(SCSIDiskState, qdev.conf),
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> > @@ -2982,6 +3001,7 @@ static const TypeInfo scsi_cd_info = {
> >  static Property scsi_block_properties[] = {
> >      DEFINE_BLOCK_ERROR_PROPERTIES(SCSIDiskState, qdev.conf),         \
> >      DEFINE_PROP_DRIVE("drive", SCSIDiskState, qdev.conf.blk),
> > +    DEFINE_PROP_UINT16("rotation_rate", SCSIDiskState, rotation_rate, 0),
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> 
> Are you sure you want this as a property of SCSIDiskState etc rather
> than a property on the underlying drive?
> Then if virtio devices etc wanted to pick this up later they could
> without needing extra parameters.

That would make 'rotation_rate' available for everything, regardless
of whether its used/supported by the frontend device. I was trying to
restrict this to only the places where its appropriate, so only disks,
not cdroms for example.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

[Qemu-devel] [PATCH v1 2/2] ide: support reporting of rotation rate

[Daniel P. Berrange]

The Linux kernel will query the ATA IDENTITY DEVICE data, word 217
to determine the rotations per minute of the disk. If this has
the value 1, it is taken to be an SSD and so Linux sets the
'rotational' flag to 0 for the I/O queue and will stop using that
disk as a source of random entropy. Other operating systems may
also take into account rotation rate when setting up default
behaviour.

Mgmt apps should be able to set the rotation rate for virtualized
block devices, based on characteristics of the host storage in use,
so that the guest OS gets sensible behaviour out of the box. This
patch thus adds a 'rotation-rate' parameter for 'ide-hd' device
types.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 hw/ide/core.c             | 1 +
 hw/ide/qdev.c             | 1 +
 include/hw/ide/internal.h | 8 ++++++++
 3 files changed, 10 insertions(+)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 5f1cd3b91f..a04766aee7 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -208,6 +208,7 @@ static void ide_identify(IDEState *s)
     if (dev && dev->conf.discard_granularity) {
         put_le16(p + 169, 1); /* TRIM support */
     }
+    put_le16(p + 217, dev->rotation_rate); /* Nominal media rotation rate */
 
     ide_identify_size(s);
     s->identify_set = 1;
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index d60ac25be0..a5181b4448 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -299,6 +299,7 @@ static Property ide_hd_properties[] = {
     DEFINE_BLOCK_CHS_PROPERTIES(IDEDrive, dev.conf),
     DEFINE_PROP_BIOS_CHS_TRANS("bios-chs-trans",
                 IDEDrive, dev.chs_trans, BIOS_ATA_TRANSLATION_AUTO),
+    DEFINE_PROP_UINT16("rotation_rate", IDEDrive, dev.rotation_rate, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index e641012b48..31851b44d1 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -508,6 +508,14 @@ struct IDEDevice {
     char *serial;
     char *model;
     uint64_t wwn;
+    /*
+     * 0x0000        - rotation rate not reported
+     * 0x0001        - non-rotating medium (SSD)
+     * 0x0002-0x0400 - reserved
+     * 0x0401-0xffe  - rotations per minute
+     * 0xffff        - reserved
+     */
+    uint16_t rotation_rate;
 };
 
 /* These are used for the error_status field of IDEBus */
-- 
2.13.5

Re: [Qemu-devel] [PATCH v1 2/2] ide: support reporting of rotation rate

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1518 bytes --]

On 10/04/2017 06:40 AM, Daniel P. Berrange wrote:
> The Linux kernel will query the ATA IDENTITY DEVICE data, word 217
> to determine the rotations per minute of the disk. If this has
> the value 1, it is taken to be an SSD and so Linux sets the
> 'rotational' flag to 0 for the I/O queue and will stop using that
> disk as a source of random entropy. Other operating systems may
> also take into account rotation rate when setting up default
> behaviour.
> 
> Mgmt apps should be able to set the rotation rate for virtualized
> block devices, based on characteristics of the host storage in use,
> so that the guest OS gets sensible behaviour out of the box. This
> patch thus adds a 'rotation-rate' parameter for 'ide-hd' device
> types.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  hw/ide/core.c             | 1 +

> +++ b/include/hw/ide/internal.h
> @@ -508,6 +508,14 @@ struct IDEDevice {
>      char *serial;
>      char *model;
>      uint64_t wwn;
> +    /*
> +     * 0x0000        - rotation rate not reported
> +     * 0x0001        - non-rotating medium (SSD)
> +     * 0x0002-0x0400 - reserved
> +     * 0x0401-0xffe  - rotations per minute

s/0xffe/0xfffe/

> +     * 0xffff        - reserved
> +     */
> +    uint16_t rotation_rate;
>  };
>  
>  /* These are used for the error_status field of IDEBus */
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v1 2/2] ide: support reporting of rotation rate

[John Snow]

On 10/04/2017 07:40 AM, Daniel P. Berrange wrote:
> The Linux kernel will query the ATA IDENTITY DEVICE data, word 217
> to determine the rotations per minute of the disk. If this has
> the value 1, it is taken to be an SSD and so Linux sets the
> 'rotational' flag to 0 for the I/O queue and will stop using that
> disk as a source of random entropy. Other operating systems may
> also take into account rotation rate when setting up default
> behaviour.
> 
> Mgmt apps should be able to set the rotation rate for virtualized
> block devices, based on characteristics of the host storage in use,
> so that the guest OS gets sensible behaviour out of the box. This
> patch thus adds a 'rotation-rate' parameter for 'ide-hd' device
> types.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  hw/ide/core.c             | 1 +
>  hw/ide/qdev.c             | 1 +
>  include/hw/ide/internal.h | 8 ++++++++
>  3 files changed, 10 insertions(+)
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 5f1cd3b91f..a04766aee7 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -208,6 +208,7 @@ static void ide_identify(IDEState *s)
>      if (dev && dev->conf.discard_granularity) {
>          put_le16(p + 169, 1); /* TRIM support */
>      }
> +    put_le16(p + 217, dev->rotation_rate); /* Nominal media rotation rate */
>  
>      ide_identify_size(s);
>      s->identify_set = 1;
> diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
> index d60ac25be0..a5181b4448 100644
> --- a/hw/ide/qdev.c
> +++ b/hw/ide/qdev.c
> @@ -299,6 +299,7 @@ static Property ide_hd_properties[] = {
>      DEFINE_BLOCK_CHS_PROPERTIES(IDEDrive, dev.conf),
>      DEFINE_PROP_BIOS_CHS_TRANS("bios-chs-trans",
>                  IDEDrive, dev.chs_trans, BIOS_ATA_TRANSLATION_AUTO),
> +    DEFINE_PROP_UINT16("rotation_rate", IDEDrive, dev.rotation_rate, 0),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
> index e641012b48..31851b44d1 100644
> --- a/include/hw/ide/internal.h
> +++ b/include/hw/ide/internal.h
> @@ -508,6 +508,14 @@ struct IDEDevice {
>      char *serial;
>      char *model;
>      uint64_t wwn;
> +    /*
> +     * 0x0000        - rotation rate not reported
> +     * 0x0001        - non-rotating medium (SSD)
> +     * 0x0002-0x0400 - reserved
> +     * 0x0401-0xffe  - rotations per minute
> +     * 0xffff        - reserved
> +     */
> +    uint16_t rotation_rate;
>  };
>  
>  /* These are used for the error_status field of IDEBus */
> 

With Eric's comment addressed:

Reviewed-by: John Snow <jsnow@redhat.com>

It'd be nice if we could have a magic autodetect value, but this is a
strict improvement anyway.

(Actually, probably most of the identify data needs to be audited, but
the perceived cost:benefit ratio doesn't look too favorable.)

Re: [Qemu-devel] [PATCH v1 2/2] ide: support reporting of rotation rate

[Kevin Wolf]

[ Cc: qemu-block ]

Am 04.10.2017 um 13:40 hat Daniel P. Berrange geschrieben:
> The Linux kernel will query the ATA IDENTITY DEVICE data, word 217
> to determine the rotations per minute of the disk. If this has
> the value 1, it is taken to be an SSD and so Linux sets the
> 'rotational' flag to 0 for the I/O queue and will stop using that
> disk as a source of random entropy. Other operating systems may
> also take into account rotation rate when setting up default
> behaviour.
> 
> Mgmt apps should be able to set the rotation rate for virtualized
> block devices, based on characteristics of the host storage in use,
> so that the guest OS gets sensible behaviour out of the box. This
> patch thus adds a 'rotation-rate' parameter for 'ide-hd' device
> types.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  hw/ide/core.c             | 1 +
>  hw/ide/qdev.c             | 1 +
>  include/hw/ide/internal.h | 8 ++++++++
>  3 files changed, 10 insertions(+)
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 5f1cd3b91f..a04766aee7 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -208,6 +208,7 @@ static void ide_identify(IDEState *s)
>      if (dev && dev->conf.discard_granularity) {
>          put_le16(p + 169, 1); /* TRIM support */
>      }
> +    put_le16(p + 217, dev->rotation_rate); /* Nominal media rotation rate */

Coverity points out that all other dereferences of dev have a NULL check
first. Are we sure that it is always non-NULL?

A follow-up patch is necessary either way. Either fix the missing NULL
check here or remove useless NULL checks in the other places.

Kevin

Re: [Qemu-devel] [PATCH v1 2/2] ide: support reporting of rotation rate

[Daniel P. Berrange]

On Fri, Oct 20, 2017 at 10:42:21AM +0200, Kevin Wolf wrote:
> [ Cc: qemu-block ]
> 
> Am 04.10.2017 um 13:40 hat Daniel P. Berrange geschrieben:
> > The Linux kernel will query the ATA IDENTITY DEVICE data, word 217
> > to determine the rotations per minute of the disk. If this has
> > the value 1, it is taken to be an SSD and so Linux sets the
> > 'rotational' flag to 0 for the I/O queue and will stop using that
> > disk as a source of random entropy. Other operating systems may
> > also take into account rotation rate when setting up default
> > behaviour.
> > 
> > Mgmt apps should be able to set the rotation rate for virtualized
> > block devices, based on characteristics of the host storage in use,
> > so that the guest OS gets sensible behaviour out of the box. This
> > patch thus adds a 'rotation-rate' parameter for 'ide-hd' device
> > types.
> > 
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> >  hw/ide/core.c             | 1 +
> >  hw/ide/qdev.c             | 1 +
> >  include/hw/ide/internal.h | 8 ++++++++
> >  3 files changed, 10 insertions(+)
> > 
> > diff --git a/hw/ide/core.c b/hw/ide/core.c
> > index 5f1cd3b91f..a04766aee7 100644
> > --- a/hw/ide/core.c
> > +++ b/hw/ide/core.c
> > @@ -208,6 +208,7 @@ static void ide_identify(IDEState *s)
> >      if (dev && dev->conf.discard_granularity) {
> >          put_le16(p + 169, 1); /* TRIM support */
> >      }
> > +    put_le16(p + 217, dev->rotation_rate); /* Nominal media rotation rate */
> 
> Coverity points out that all other dereferences of dev have a NULL check
> first. Are we sure that it is always non-NULL?
> 
> A follow-up patch is necessary either way. Either fix the missing NULL
> check here or remove useless NULL checks in the other places.

'dev' comes from:

    IDEDevice *dev = s->unit ? s->bus->slave : s->bus->master;

IIUC, this is choosing either the first or the second unit on the IDE
bus. Presumably this can be lead to dev==NULL, when the guest OS calls
identify on a unit that doesn't have a drive attached. Soo the NULL
checks looks like its required to me.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v1 2/2] ide: support reporting of rotation rate

[John Snow]

On 10/20/2017 05:02 AM, Daniel P. Berrange wrote:
> On Fri, Oct 20, 2017 at 10:42:21AM +0200, Kevin Wolf wrote:
>> [ Cc: qemu-block ]
>>
>> Am 04.10.2017 um 13:40 hat Daniel P. Berrange geschrieben:
>>> The Linux kernel will query the ATA IDENTITY DEVICE data, word 217
>>> to determine the rotations per minute of the disk. If this has
>>> the value 1, it is taken to be an SSD and so Linux sets the
>>> 'rotational' flag to 0 for the I/O queue and will stop using that
>>> disk as a source of random entropy. Other operating systems may
>>> also take into account rotation rate when setting up default
>>> behaviour.
>>>
>>> Mgmt apps should be able to set the rotation rate for virtualized
>>> block devices, based on characteristics of the host storage in use,
>>> so that the guest OS gets sensible behaviour out of the box. This
>>> patch thus adds a 'rotation-rate' parameter for 'ide-hd' device
>>> types.
>>>
>>> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
>>> ---
>>>  hw/ide/core.c             | 1 +
>>>  hw/ide/qdev.c             | 1 +
>>>  include/hw/ide/internal.h | 8 ++++++++
>>>  3 files changed, 10 insertions(+)
>>>
>>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>>> index 5f1cd3b91f..a04766aee7 100644
>>> --- a/hw/ide/core.c
>>> +++ b/hw/ide/core.c
>>> @@ -208,6 +208,7 @@ static void ide_identify(IDEState *s)
>>>      if (dev && dev->conf.discard_granularity) {
>>>          put_le16(p + 169, 1); /* TRIM support */
>>>      }
>>> +    put_le16(p + 217, dev->rotation_rate); /* Nominal media rotation rate */
>>
>> Coverity points out that all other dereferences of dev have a NULL check
>> first. Are we sure that it is always non-NULL?
>>
>> A follow-up patch is necessary either way. Either fix the missing NULL
>> check here or remove useless NULL checks in the other places.
> 
> 'dev' comes from:
> 
>     IDEDevice *dev = s->unit ? s->bus->slave : s->bus->master;
> 
> IIUC, this is choosing either the first or the second unit on the IDE
> bus. Presumably this can be lead to dev==NULL, when the guest OS calls
> identify on a unit that doesn't have a drive attached. Soo the NULL
> checks looks like its required to me.
> 
> Regards,
> Daniel
> 

I'm sorry I didn't notice. I had an unexpected LOA, has this been addressed?

CC me and I will take care of it.

--John

Re: [Qemu-devel] [PATCH v1 0/2] Reporting of rotation rate for disks

[Daniel P. Berrange]

On Wed, Oct 04, 2017 at 12:40:06PM +0100, Daniel P. Berrange wrote:
> While looking at libvirt tagged questions on serverfault I saw
> someone ask how to indicate that a virtual disk is an SSD rather
> than rotating rust.
> 
> https://serverfault.com/questions/876467/how-to-add-virtual-storage-as-ssd-in-kvm
> 
> IIUC, IDE / SCSI don't really have an explicit concept of a
> disk being SSD vs HDD, but they do have a means of reporting
> the disk rotation rate and both specify that SSDs should
> report RPM==1.
> 
> Linux uses this to determine whether to set the 'rotational'
> property to 0 for the I/O queue, and also whether to allow
> the disk to contribute random entropy (only HDDs contribute).
> 
> This felt like something QEMU ought to allow the mgmt application
> to set based on the storage it is using to back the virtual block
> devices. So this series adds a 'rotation_rate' property to the
> SCSI and IDE disks, taking an RPM value per their respective
> specifications.

BTW, to verify these patches there's two ways:

  # smartctl -a /dev/sda | grep Rotation
  Rotation Rate:        15000 rpm

  # smartctl -a /dev/sdb | grep Rotation
  Rotation Rate:        Solid State Device

Before these patches, 'Rotation rate' is not reported by smartctl
at all. With these patches the default value of '0' means nothing
will be reported too. Only if set to a non-zero value will it
be reported.

Or look at kernel queue:

  # cat /sys/block/sda/queue/rotational 
  1

  # cat /sys/block/sdb/queue/rotational 
  0



Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v1 0/2] Reporting of rotation rate for disks

[Paolo Bonzini]

On 04/10/2017 13:40, Daniel P. Berrange wrote:
> 
> There is no mechanism to report this information to virtio-blk.
> We could perhaps argue that people should use virtio-scsi instead,
> because fixing virtio-blk would require enhancement to both QEMU
> and Linux virtio-blk drivers (and other guest OS drivers too)

I think long-term virtio-blk should only be used for high-performance
scenarios where the guest SCSI layer slows down things sensibly.  So
perhaps virtio-blk should always report itself as non-rotational.

> I'm unclear if there is anything I should have done wrt the
> device migration vmstate when adding this extra field, or if it
> is safe to just expect the mgmt app to set the property correctly
> on src+dst ?

That should be enough, yes.

Paolo