| Message ID | 20171004114008.14849-3-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Reporting of rotation rate for disks | expand |
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 */ >
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.)
[ 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
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
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
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 */
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(+)